Handle branch or commit refs in the url for unmanaged git solutions

Handle branch or commit refs in the url for unmanaged git solutions

This allows having a git repo cloned on a specific branch or commit, similar to
what's possible for svn solutions by specifying a url like svnrepo/branches/foo

Having the possibility to check the code out on a specific branch makes the
initial code checkout workflow simpler, and allows specifying all the relevant
info directly in the fetch recipe, instead of  having to deal with branches
manually afterwards (which is the whole purpose of recipes in the first place)

So with this change, I'm able to do the following in my custom fetch recipe:

    solution = {
        'name' : 'src',
        'url' : 'git@github.snei.sony.com:SNEI/chromium.git@refs/heads/custombranch',
        <following lines skipped>

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

[Romain Pokrzywka, 2014-05-21 20:28:24]

Hi Aaron & al.,

Thanks for the reply to my email earlier, here's the matching review CL for the
patch I sent in the email.

Regards,
Romain

[Romain Pokrzywka, 2014-05-23 16:56:15]

On 2014/05/21 20:28:24, Romain Pokrzywka wrote:
> Hi Aaron & al.,
> 
> Thanks for the reply to my email earlier, here's the matching review CL for
the
> patch I sent in the email.
> 
> Regards,
> Romain

Anybody any feedback?

[M-A Ruel, 2014-05-23 17:13:57]

On 2014/05/23 16:56:15, Romain Pokrzywka wrote:
> On 2014/05/21 20:28:24, Romain Pokrzywka wrote:
> > Hi Aaron & al.,
> > 
> > Thanks for the reply to my email earlier, here's the matching review CL for
> the
> > patch I sent in the email.
> > 
> > Regards,
> > Romain
> 
> Anybody any feedback?

Switch to a better list of potential reviewers.

[Romain Pokrzywka, 2014-05-23 18:00:48]

On 2014/05/23 17:13:57, M-A Ruel wrote:
> On 2014/05/23 16:56:15, Romain Pokrzywka wrote:
> > On 2014/05/21 20:28:24, Romain Pokrzywka wrote:
> > > Hi Aaron & al.,
> > > 
> > > Thanks for the reply to my email earlier, here's the matching review CL
for
> > the
> > > patch I sent in the email.
> > > 
> > > Regards,
> > > Romain
> > 
> > Anybody any feedback?
> 
> Switch to a better list of potential reviewers.

OK, tried adding a couple more based on the git logs, but I don't think I was
totally off with iannucci and agable (who even replied to my initial email)...

Thanks,
Romain

[iannucci, 2014-05-23 18:06:58]

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode334
gclient_scm.py:334: # Check again for a revision in case an initial ref was
specified in the url,
80 cols?

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode336
gclient_scm.py:336: url, revision = gclient_utils.SplitUrlRevision(self.url)
isn't this this same as

revision = deps_revision

?

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode824
gclient_scm.py:824: self._Run(['checkout', '--quiet',
revision.replace('refs/heads/', '')],
tbh, this '--quiet' option can be problematic, especially on windows (which can
take a very long time to checkout and has no output). I would rather remove it
and get rid of the if statement below.

[iannucci, 2014-05-23 18:07:44]

Sorry for the lag on this review, there's been a lot of interrupts this week.

[Romain Pokrzywka, 2014-05-23 21:15:44]

Thanks for the comments iannucci, no worries I understand you guys have a lot to
do. I'm gonna go ahead and fix the things you mentionned.

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode334
gclient_scm.py:334: # Check again for a revision in case an initial ref was
specified in the url,
On 2014/05/23 18:06:58, iannucci wrote:
> 80 cols?

indeed, will fix

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode336
gclient_scm.py:336: url, revision = gclient_utils.SplitUrlRevision(self.url)
On 2014/05/23 18:06:58, iannucci wrote:
> isn't this this same as
> 
> revision = deps_revision
> 
> ?

Correct, thanks for catching that. The second call to SplitUrlRevision is
unneeded then, will replace.

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode824
gclient_scm.py:824: self._Run(['checkout', '--quiet',
revision.replace('refs/heads/', '')],
On 2014/05/23 18:06:58, iannucci wrote:
> tbh, this '--quiet' option can be problematic, especially on windows (which
can
> take a very long time to checkout and has no output). I would rather remove it
> and get rid of the if statement below.

Fine with me. That said I feel this isn't directly related to this CL, so if
that's ok with you I'll make the change you suggest but in a separate CL to keep
things clear.

[iannucci, 2014-05-23 21:17:49]

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py
File gclient_scm.py (right):

https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode824
gclient_scm.py:824: self._Run(['checkout', '--quiet',
revision.replace('refs/heads/', '')],
On 2014/05/23 21:15:45, Romain Pokrzywka wrote:
> On 2014/05/23 18:06:58, iannucci wrote:
> > tbh, this '--quiet' option can be problematic, especially on windows (which
> can
> > take a very long time to checkout and has no output). I would rather remove
it
> > and get rid of the if statement below.
> 
> Fine with me. That said I feel this isn't directly related to this CL, so if
> that's ok with you I'll make the change you suggest but in a separate CL to
keep
> things clear.

Yep that's fine, I was just musing :)

[Romain Pokrzywka, 2014-05-23 23:30:58]

On 2014/05/23 21:17:49, iannucci wrote:
> https://codereview.chromium.org/289863012/diff/1/gclient_scm.py
> File gclient_scm.py (right):
> 
> https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode824
> gclient_scm.py:824: self._Run(['checkout', '--quiet',
> revision.replace('refs/heads/', '')],
> On 2014/05/23 21:15:45, Romain Pokrzywka wrote:
> > On 2014/05/23 18:06:58, iannucci wrote:
> > > tbh, this '--quiet' option can be problematic, especially on windows
(which
> > can
> > > take a very long time to checkout and has no output). I would rather
remove
> it
> > > and get rid of the if statement below.
> > 
> > Fine with me. That said I feel this isn't directly related to this CL, so if
> > that's ok with you I'll make the change you suggest but in a separate CL to
> keep
> > things clear.
> 
> Yep that's fine, I was just musing :)

New patch uploaded, PTAL

Thanks,
Romain

[Romain Pokrzywka, 2014-05-29 17:50:10]

On 2014/05/23 23:30:58, Romain Pokrzywka wrote:
> On 2014/05/23 21:17:49, iannucci wrote:
> > https://codereview.chromium.org/289863012/diff/1/gclient_scm.py
> > File gclient_scm.py (right):
> > 
> > https://codereview.chromium.org/289863012/diff/1/gclient_scm.py#newcode824
> > gclient_scm.py:824: self._Run(['checkout', '--quiet',
> > revision.replace('refs/heads/', '')],
> > On 2014/05/23 21:15:45, Romain Pokrzywka wrote:
> > > On 2014/05/23 18:06:58, iannucci wrote:
> > > > tbh, this '--quiet' option can be problematic, especially on windows
> (which
> > > can
> > > > take a very long time to checkout and has no output). I would rather
> remove
> > it
> > > > and get rid of the if statement below.
> > > 
> > > Fine with me. That said I feel this isn't directly related to this CL, so
if
> > > that's ok with you I'll make the change you suggest but in a separate CL
to
> > keep
> > > things clear.
> > 
> > Yep that's fine, I was just musing :)
> 
> New patch uploaded, PTAL
> 
> Thanks,
> Romain

iannucci: can you trigger the trybots if it's ok to you now?

Thanks,
Romain

[iannucci, 2014-05-30 00:00:45]

lgtm. There's no trybots for this, but CQ is enabled.

[iannucci, 2014-05-30 00:01:16]

On 2014/05/30 00:00:45, iannucci wrote:
> lgtm. There's no trybots for this, but CQ is enabled.

(and sorry for the long review time... gclient makes my head hurt because of the
potential impact :))

[Romain Pokrzywka, 2014-05-30 00:03:11]

The CQ bit was checked by romain.pokrzywka@gmail.com

[commit-bot: I haz the power, 2014-05-30 00:04:24]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/romain.pokrzywka@gmail.com/289863012/2...

[commit-bot: I haz the power, 2014-05-30 00:06:08]

Change committed as 273675

[Romain Pokrzywka, 2014-05-30 00:12:26]

On 2014/05/30 00:01:16, iannucci wrote:
> On 2014/05/30 00:00:45, iannucci wrote:
> > lgtm. There's no trybots for this, but CQ is enabled.
> 
> (and sorry for the long review time... gclient makes my head hurt because of
the
> 

I see, that's the first time I used the CQ myself, looks like it landed ok! \o/

Thanks again for your time!
Regards,
Romain

[Michael Moss, 2014-09-08 20:24:00]

mmoss@chromium.org changed reviewers:
+ mmoss@chromium.org

[Michael Moss, 2014-09-08 20:24:01]

https://codereview.chromium.org/289863012/diff/20001/tests/gclient_scm_test.py
File tests/gclient_scm_test.py (right):

https://codereview.chromium.org/289863012/diff/20001/tests/gclient_scm_test.p...
tests/gclient_scm_test.py:1526: def testUpdateCloneOnBranchHead(self):
(Dredging up this old CL because this test is causing a failure in some code I'm
trying to add)

I don't understand the intended difference between testUpdateCloneOnBranchHead()
and testUpdateCloneOnDetachedBranch(). It looks like you expect
@refs/remotes/origin/feature to behave differently from @refs/heads/feature, but
I'm not sure why. I assume they are both meant to refer to the 'feature' branch
in the remote repo, but one is specified from the POV of the mirror
(refs/remotes/origin) and one is specified from the POV of the original
(refs/heads/). Why do you expect or want gclient to check this out detached in
one instance, but not detached in the other? I think synonyms for the same
remote should be checked out in the same way (or just treated as the same thing
in general, which is what my code tries to do).

If your intent is that 'refs/heads/feature' doesn't actually refer to the
remote, but rather to a local branch (hopefully based on that remote), well, I'm
not sure specifying local branches is something that really makes sense for DEPS
files.