Re-reland r245404 ("If the destination directory doesn't contain the desired repo, delete it")

Re-reland r245404 ("If the destination directory doesn't contain the desired repo, delete it")

BUG=

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

[borenet, 2014-02-06 16:21:47]

Let's try this again...

https://codereview.chromium.org/133073015/diff/30001/scm.py
File scm.py (right):

https://codereview.chromium.org/133073015/diff/30001/scm.py#newcode173
scm.py:173: return False
From what I can tell, the last version was failing to update pure-svn DEPS from
a git-svn checkout.  It was skipping them, because this function was giving
false positives.  The cause is that "git config" will search parent directories
until it finds a .git directory, thus tricking gclient into thinking that all of
the svn DEPS are part of the same git-svn checkout.  Therefore, we enforce that
this function must run in the checkout root where the .git directory would be.

https://codereview.chromium.org/133073015/diff/30001/tests/scm_unittest.py
File tests/scm_unittest.py (right):

https://codereview.chromium.org/133073015/diff/30001/tests/scm_unittest.py#ne...
tests/scm_unittest.py:172: def testIsGitSvn(self):
This already succeeded, but there was no test.

[iannucci, 2014-02-06 23:05:10]

https://codereview.chromium.org/133073015/diff/30001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/133073015/diff/30001/gclient_scm.py#newcode1360
gclient_scm.py:1360: if scm.GIT.IsGitSvn(checkout_root=self.checkout_path):
antipattern. don't use named args to fill in positional values. It leads to pain
when someone decides to rename the parameter on the function later :D

https://codereview.chromium.org/133073015/diff/30001/scm.py
File scm.py (right):

https://codereview.chromium.org/133073015/diff/30001/scm.py#newcode173
scm.py:173: return False
On 2014/02/06 16:21:47, borenet wrote:
> From what I can tell, the last version was failing to update pure-svn DEPS
from
> a git-svn checkout.  It was skipping them, because this function was giving
> false positives.  The cause is that "git config" will search parent
directories
> until it finds a .git directory, thus tricking gclient into thinking that all
of
> the svn DEPS are part of the same git-svn checkout.  Therefore, we enforce
that
> this function must run in the checkout root where the .git directory would be.

Yeah that makes sense... gclient supports too many combinations :/

https://codereview.chromium.org/133073015/diff/30001/tests/scm_unittest.py
File tests/scm_unittest.py (right):

https://codereview.chromium.org/133073015/diff/30001/tests/scm_unittest.py#ne...
tests/scm_unittest.py:172: def testIsGitSvn(self):
On 2014/02/06 16:21:47, borenet wrote:
> This already succeeded, but there was no test.

Neat

[borenet, 2014-02-07 14:32:21]

Uploaded patch set 3.

https://codereview.chromium.org/133073015/diff/30001/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/133073015/diff/30001/gclient_scm.py#newcode1360
gclient_scm.py:1360: if scm.GIT.IsGitSvn(checkout_root=self.checkout_path):
On 2014/02/06 23:05:11, iannucci wrote:
> antipattern. don't use named args to fill in positional values. It leads to
pain
> when someone decides to rename the parameter on the function later :D

Removed.  I wanted to be very explicit that the function only works if the
provided parameter is the root of the checkout (which would contain a .git
directory).

[iannucci, 2014-02-07 20:04:00]

lgtm.

Yeah I actually ran into the "I'll just rename this parameter to make more
sense" issue in gclient before :(.

https://codereview.chromium.org/133073015/diff/160001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/133073015/diff/160001/tests/gclient_smoketest...
tests/gclient_smoketest.py:792: _ = subprocess2.check_output(['git', 'svn',
'clone', svn_url],
Er... why? Though I guess it's just a smoke test so it doesn't matter?

[borenet, 2014-02-07 20:36:10]

Re: Bots not seeming to pick up the new experimental behavior - is it possible
that the slavename (which is what I wrote in the code) is not the same as the
hostname in all cases?  I'm not sure how to log in to the machines to find the
hostnames.

https://codereview.chromium.org/133073015/diff/160001/tests/gclient_smoketest.py
File tests/gclient_smoketest.py (right):

https://codereview.chromium.org/133073015/diff/160001/tests/gclient_smoketest...
tests/gclient_smoketest.py:792: _ = subprocess2.check_output(['git', 'svn',
'clone', svn_url],
On 2014/02/07 20:04:01, iannucci wrote:
> Er... why? Though I guess it's just a smoke test so it doesn't matter?

I assume you're asking about check_output?  I was sick of seeing log spew while
running the test.

[iannucci, 2014-02-07 20:40:17]

On 2014/02/07 20:36:10, borenet wrote:
> Re: Bots not seeming to pick up the new experimental behavior - is it possible
> that the slavename (which is what I wrote in the code) is not the same as the
> hostname in all cases?  I'm not sure how to log in to the machines to find the
> hostnames.
> 
>
https://codereview.chromium.org/133073015/diff/160001/tests/gclient_smoketest.py
> File tests/gclient_smoketest.py (right):
> 
>
https://codereview.chromium.org/133073015/diff/160001/tests/gclient_smoketest...
> tests/gclient_smoketest.py:792: _ = subprocess2.check_output(['git', 'svn',
> 'clone', svn_url],
> On 2014/02/07 20:04:01, iannucci wrote:
> > Er... why? Though I guess it's just a smoke test so it doesn't matter?
> 
> I assume you're asking about check_output?  I was sick of seeing log spew
while
> running the test.

Ah, ok. In that case I would use check_output, drop the `_ =` and add a comment
to the effect that swallowing the output is intentional.

[iannucci, 2014-02-07 20:41:00]

On 2014/02/07 20:40:17, iannucci wrote:
> On 2014/02/07 20:36:10, borenet wrote:
> > Re: Bots not seeming to pick up the new experimental behavior - is it
possible
> > that the slavename (which is what I wrote in the code) is not the same as
the
> > hostname in all cases?  I'm not sure how to log in to the machines to find
the
> > hostnames.

Maybe if CHROME_HEADLESS, print out what socket.gethostname() is returning?

> > 
> >
>
https://codereview.chromium.org/133073015/diff/160001/tests/gclient_smoketest.py
> > File tests/gclient_smoketest.py (right):
> > 
> >
>
https://codereview.chromium.org/133073015/diff/160001/tests/gclient_smoketest...
> > tests/gclient_smoketest.py:792: _ = subprocess2.check_output(['git', 'svn',
> > 'clone', svn_url],
> > On 2014/02/07 20:04:01, iannucci wrote:
> > > Er... why? Though I guess it's just a smoke test so it doesn't matter?
> > 
> > I assume you're asking about check_output?  I was sick of seeing log spew
> while
> > running the test.
> 
> Ah, ok. In that case I would use check_output, drop the `_ =` and add a
comment
> to the effect that swallowing the output is intentional.

[borenet, 2014-02-07 21:00:30]

Uploaded patch set 4.

[iannucci, 2014-02-07 22:27:53]

lgtm

[borenet, 2014-02-11 20:00:16]

The CQ bit was checked by borenet@google.com

[commit-bot: I haz the power, 2014-02-11 20:02:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/borenet@google.com/133073015/280001

[commit-bot: I haz the power, 2014-02-11 20:05:19]

Change committed as 250482