Try harder to detect an existing checkout before 'fetch'ing.

Try harder to detect an existing checkout before 'fetch'ing.

BUG=230693
R=agable@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=295611

[agable, 2015-06-10 17:54:28]

https://codereview.chromium.org/1169403002/diff/1/fetch.py
File fetch.py (right):

https://codereview.chromium.org/1169403002/diff/1/fetch.py#newcode123
fetch.py:123: def exists(self):
Rather than doing this, I would define exists() on the top-level GclientCheckout
and run self.run_gclient('root') to see if it finds a .gclient file. wdyt?

[Michael Moss, 2015-06-10 18:10:47]

https://codereview.chromium.org/1169403002/diff/1/fetch.py
File fetch.py (right):

https://codereview.chromium.org/1169403002/diff/1/fetch.py#newcode123
fetch.py:123: def exists(self):
On 2015/06/10 17:54:28, agable wrote:
> Rather than doing this, I would define exists() on the top-level
GclientCheckout
> and run self.run_gclient('root') to see if it finds a .gclient file. wdyt?

Yeah, I wasn't sure how strict we wanted to be, so I was just trying to exclude
"redundant" checkouts, as opposed to "any checkout within any checkout" (though
I don't know why anybody would want to do that). I'm not opposed to the latter
if you think that's better.

[agable, 2015-06-10 18:22:17]

lgtm

https://codereview.chromium.org/1169403002/diff/1/fetch.py
File fetch.py (right):

https://codereview.chromium.org/1169403002/diff/1/fetch.py#newcode123
fetch.py:123: def exists(self):
On 2015/06/10 at 18:10:47, Michael Moss wrote:
> On 2015/06/10 17:54:28, agable wrote:
> > Rather than doing this, I would define exists() on the top-level
GclientCheckout
> > and run self.run_gclient('root') to see if it finds a .gclient file. wdyt?
> 
> Yeah, I wasn't sure how strict we wanted to be, so I was just trying to
exclude "redundant" checkouts, as opposed to "any checkout within any checkout"
(though I don't know why anybody would want to do that). I'm not opposed to the
latter if you think that's better.

We don't have any GclientSVN-based fetch recipes anymore (and should probably
delete that code because screw svn). So doing it here is probably fine. But the
purist in me really wants to make sure we cover all the cases. And since there's
a base class that understands gclient anyway...

Not necessary. But I think it would be way more elegant and less likely to
surprise someone later down the line.

[Michael Moss, 2015-06-10 18:44:36]

> Not necessary. But I think it would be way more elegant and less likely to
> surprise someone later down the line.

Done. PTAL.

[agable, 2015-06-10 19:00:48]

https://codereview.chromium.org/1169403002/diff/20001/fetch.py
File fetch.py (right):

https://codereview.chromium.org/1169403002/diff/20001/fetch.py#newcode88
fetch.py:88: def exists(self):
you want this to be in GclientCheckout, not GitCheckout

[Michael Moss, 2015-06-10 19:15:08]

On 2015/06/10 19:00:48, agable wrote:
> https://codereview.chromium.org/1169403002/diff/20001/fetch.py
> File fetch.py (right):
> 
> https://codereview.chromium.org/1169403002/diff/20001/fetch.py#newcode88
> fetch.py:88: def exists(self):
> you want this to be in GclientCheckout, not GitCheckout

Ugh, fixed.

[agable, 2015-06-10 19:25:18]

lgtm

[Michael Moss, 2015-06-10 20:01:42]

Committed patchset #3 (id:40001) manually as 295611.