Allow gclient clone in non-empty directories

Allow gclient clone in non-empty directories

Add an option in DEPS files to clone a project into a temp dir
and then copy into expected final dir.  This allows checking out a
git repo into a folder which is non-empty. It is useful for projects
that are embedded in src/ but want to specify the revision of
src/ in the embedded project (such as android private).

BUG=165280
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=212720

[Isaac (away), 2013-07-16 10:21:25]

Initial upload for general review.  I still need to write tests / actually run
the code / fix the broken unit tests, but I'm looking for early feedback.

[M-A Ruel, 2013-07-16 13:46:31]

On 2013/07/16 10:21:25, Isaac wrote:
> Initial upload for general review.  I still need to write tests / actually run
> the code / fix the broken unit tests, but I'm looking for early feedback.

I must say I'm not super excited about this. I'd highly prefer enabling
"recursion = 3" to work well and be done with it. It's much more inline with the
general design. The proposed use case is very hackish.

That said, I don't plan to block the patch if someone else vets it.

https://chromiumcodereview.appspot.com/19359002/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/1/gclient_scm.py#newcode444
gclient_scm.py:444: 
.

[Isaac (away), 2013-07-18 09:47:41]

ptal

[Isaac (away), 2013-07-18 09:48:24]

On 2013/07/18 09:47:41, Isaac wrote:
> ptal

(which is Please Take Another Look, the true expansion of 'ptal')

[M-A Ruel, 2013-07-18 14:55:04]

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:344: (os.path.isdir(self.check_path) and
What's check_path? Was this code path ever exercised? If not, look at exercising
it.

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:793: tmp_dir = tempfile.mkdtemp(
In practice, you want to have this just before the try, so that if this call
fails, like /tmp is full, then the finally has less chances of throwing itself.

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:794: suffix='_%s' % os.path.basename(self.checkout_path),
I'd also prefer a prefix, so it's easier to remove, e.g. "rm -rf gclient_*"

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:828: 
Remove, it's not between file level symbols.

[Isaac (away), 2013-07-19 20:13:23]

Still need to fix about 10 gclient_smoketests tests -- robbie could you take a
look?

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:344: (os.path.isdir(self.check_path) and
On 2013/07/18 14:55:04, Marc-Antoine Ruel wrote:
> What's check_path? Was this code path ever exercised? If not, look at
exercising
> it.

should be checkout_path, I uploaded w/ bypass-hooks to get a general review. 
This is the new code path -- checking into an existing directory that's not
already a git repo.

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:793: tmp_dir = tempfile.mkdtemp(
On 2013/07/18 14:55:04, Marc-Antoine Ruel wrote:
> In practice, you want to have this just before the try, so that if this call
> fails, like /tmp is full, then the finally has less chances of throwing
itself.

Done.

https://chromiumcodereview.appspot.com/19359002/diff/5001/gclient_scm.py#newc...
gclient_scm.py:794: suffix='_%s' % os.path.basename(self.checkout_path),
On 2013/07/18 14:55:04, Marc-Antoine Ruel wrote:
> I'd also prefer a prefix, so it's easier to remove, e.g. "rm -rf gclient_*"

Done.

[M-A Ruel, 2013-07-19 20:42:40]

Seems fine with me, I'll leave the proper review to Robert.

https://chromiumcodereview.appspot.com/19359002/diff/10001/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/10001/gclient_scm.py#new...
gclient_scm.py:796: if not os.path.exists(self.checkout_path):
While at it, replace with isdir()

https://chromiumcodereview.appspot.com/19359002/diff/10001/tests/gclient_scm_...
File tests/gclient_scm_test.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/10001/tests/gclient_scm_...
tests/gclient_scm_test.py:374: # Checkout or update.
?

[Isaac (away), 2013-07-19 21:50:28]

https://codereview.chromium.org/19359002/diff/10001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/19359002/diff/10001/gclient_scm.py#newcode796
gclient_scm.py:796: if not os.path.exists(self.checkout_path):
On 2013/07/19 20:42:40, Marc-Antoine Ruel wrote:
> While at it, replace with isdir()

Done.

https://codereview.chromium.org/19359002/diff/10001/tests/gclient_scm_test.py
File tests/gclient_scm_test.py (right):

https://codereview.chromium.org/19359002/diff/10001/tests/gclient_scm_test.py...
tests/gclient_scm_test.py:374: # Checkout or update.
On 2013/07/19 20:42:40, Marc-Antoine Ruel wrote:
> ?

hmm... fixed.

[iannucci, 2013-07-19 23:59:07]

looks pretty good to me.

"I am OK with this" :)

Just a couple comments.

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py#new...
gclient_scm.py:418: self._FetchAndReset(revision, file_list, options)
This whole clause is superfluous if we're moving .git into place, since we'll
always have a HEAD ref. Unless the developer does rm .git/HEAD.

In which case... yeah.

I would also review the big 'if' tree below for sanity as well

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py#new...
gclient_scm.py:862: if options.verbose:
I think you'll need to rebase. There's some cache_dir logic that's missing.

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py#new...
gclient_scm.py:883: os.path.exists(os.path.join(tmp_dir, '.git'))):
If we fail this way we should completely nuke the attempt. It wouldn't have
unpacked anything, which means it'll download everything anyhow.

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py#new...
gclient_scm.py:894: if revision.startswith('refs/heads/'):
(aside: This substitution feels like we shouldn't be doing it at all...)

[iannucci, 2013-07-20 00:02:59]

Oh, also, probably want to rewrite the description too. It will allow cloning to
non-empty directories, but that's now a beneficial side-effect of this CL :)

[Isaac (away), 2013-07-20 00:38:27]

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py#new...
gclient_scm.py:418: self._FetchAndReset(revision, file_list, options)
On 2013/07/19 23:59:07, iannucci wrote:
> This whole clause is superfluous if we're moving .git into place, since we'll
> always have a HEAD ref. Unless the developer does rm .git/HEAD.
> 
> In which case... yeah.
> 
> I would also review the big 'if' tree below for sanity as well

I'd prefer to modify the 'if' below in a later CL.

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py#new...
gclient_scm.py:862: if options.verbose:
On 2013/07/19 23:59:07, iannucci wrote:
> I think you'll need to rebase. There's some cache_dir logic that's missing.

I'm not sure what you're referring to, this is rebased on origin/master.

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py#new...
gclient_scm.py:883: os.path.exists(os.path.join(tmp_dir, '.git'))):
On 2013/07/19 23:59:07, iannucci wrote:
> If we fail this way we should completely nuke the attempt. It wouldn't have
> unpacked anything, which means it'll download everything anyhow.

Could you elaborate on what you're suggesting?

https://chromiumcodereview.appspot.com/19359002/diff/25001/gclient_scm.py#new...
gclient_scm.py:894: if revision.startswith('refs/heads/'):
On 2013/07/19 23:59:07, iannucci wrote:
> (aside: This substitution feels like we shouldn't be doing it at all...)

I don't know what's going on here but I'd prefer to change it in a separate CL
because it risks breaking stuff :-\

[iannucci, 2013-07-20 00:45:37]

https://chromiumcodereview.appspot.com/19359002/diff/15002/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/15002/gclient_scm.py#new...
gclient_scm.py:876: print('Retrying...')
I'm meaning we should rmtree(tmp_dir) before retrying. Nothing good can come
from keeping it around.

[Isaac (away), 2013-07-20 00:57:53]

https://chromiumcodereview.appspot.com/19359002/diff/15002/gclient_scm.py
File gclient_scm.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/15002/gclient_scm.py#new...
gclient_scm.py:876: print('Retrying...')
On 2013/07/20 00:45:37, iannucci wrote:
> I'm meaning we should rmtree(tmp_dir) before retrying. Nothing good can come
> from keeping it around.

Done.

[iannucci, 2013-07-20 01:35:39]

lgtm % cleanup thingies

https://chromiumcodereview.appspot.com/19359002/diff/42001/gclient_scm.py
File gclient_scm.py (left):

https://chromiumcodereview.appspot.com/19359002/diff/42001/gclient_scm.py#old...
gclient_scm.py:417: if not self._HasHead():
I think HasHead is now dead code too?

https://chromiumcodereview.appspot.com/19359002/diff/42001/tests/gclient_smok...
File tests/gclient_smoketest.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/42001/tests/gclient_smok...
tests/gclient_smoketest.py:853: 'running', 'running'])
wdyt about:

[
  'running',
  ('running', ...),  # Git checkout
  ...
]

And if you're going to make everything on its own line, then we should make sure
to do that for all of these (some still end with """'running', 'running',
'running']""" on a line)

[Isaac (away), 2013-07-20 01:54:52]

sending to cqw

https://chromiumcodereview.appspot.com/19359002/diff/42001/gclient_scm.py
File gclient_scm.py (left):

https://chromiumcodereview.appspot.com/19359002/diff/42001/gclient_scm.py#old...
gclient_scm.py:417: if not self._HasHead():
On 2013/07/20 01:35:39, iannucci wrote:
> I think HasHead is now dead code too?

yep, nice catch.

https://chromiumcodereview.appspot.com/19359002/diff/42001/tests/gclient_smok...
File tests/gclient_smoketest.py (right):

https://chromiumcodereview.appspot.com/19359002/diff/42001/tests/gclient_smok...
tests/gclient_smoketest.py:853: 'running', 'running'])
On 2013/07/20 01:35:39, iannucci wrote:
> wdyt about:
> 
> [
>   'running',
>   ('running', ...),  # Git checkout
>   ...
> ]
> 
> And if you're going to make everything on its own line, then we should make
sure
> to do that for all of these (some still end with """'running', 'running',
> 'running']""" on a line)

I'll remove comments.

[commit-bot: I haz the power, 2013-07-20 01:55:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ilevy@chromium.org/19359002/48001

[commit-bot: I haz the power, 2013-07-20 01:58:09]

Change committed as 212720