Add configuration option for upstream branch and improve guessing the svn branch.

Add configuration option for upstream branch and improve guessing the svn branch.

The svn branch heuristic does roughly the same thing git-svn does: We first iterate backwards over the history to find the svn URL (like svn://svn.chromium.org/chrome/trunk/src). Then we go through all defined svn-remote section looking for one where the URL is the root of the svn URL (svn://svn.chromium.org/chrome in that case), and the fetch spec looks like "trunk/src:remotes/origin/trunk". If the base URL + left part (trunk/src) is the full URL, the right part (remotes/origin/trunk) is the git ref that is updated when we do a `git svn fetch`! This works for git-svn branches that fetch from trunk as well as milestone branches.

BUG=none
TEST=none

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

[Bernhard Bauer, 2011-02-25 12:05:41]

Hi Evan, 

could you take a look at this patch? For me, it works far better than the
current svn branch guessing, and it saves me from having to set up the tracking
branch manually for every branch.

Thanks!

[Bernhard Bauer, 2011-02-25 12:46:57]

On 2011/02/25 12:05:41, Bernhard Bauer wrote:
> Hi Evan, 
> 
> could you take a look at this patch? For me, it works far better than the
> current svn branch guessing, and it saves me from having to set up the
tracking
> branch manually for every branch.
> 
> Thanks!

BTW, is the fact that git_cl doesn't have any dependencies on depot_tools just
an artifact from when they were separate repositories? Otherwise we might want
to unfork them somewhat.

[evanm, 2011-02-25 16:20:27]

I don't understand this change.  How is it an improvement?  Perhaps making the
review description more explicit would help.

[Bernhard Bauer, 2011-02-25 16:35:37]

On 2011/02/25 16:20:27, evanm wrote:
> I don't understand this change.  How is it an improvement?  Perhaps making the
> review description more explicit would help.

OK, I updated the description.

The improvement is a bit more subjective, of course ;-) I just found that for
me, the old heuristic uses the first branch whose head commit is directly on the
git-svn commit line (so it has a git-svn-id: field), which usually is a very old
trunk branch in some remote repository.

[evanm, 2011-02-25 16:39:19]

I'm concerned that the previous code was kinda tricky and this will be subtly
different for some case we previously fixed.  Can you look at the blame for the
old code and CC some of the people who tinkered with it?

This LGTM otherwise but it makes me very nervous that it will break someone.

[Bernhard Bauer, 2011-02-25 16:56:29]

On 2011/02/25 16:39:19, evanm wrote:
> I'm concerned that the previous code was kinda tricky and this will be subtly
> different for some case we previously fixed.  Can you look at the blame for
the
> old code and CC some of the people who tinkered with it?

(adding sky@ who added code to prefer origin/HEAD over other refs. The rest of
the code was all you ;-)

> This LGTM otherwise but it makes me very nervous that it will break someone.

[sky, 2011-02-25 17:06:55]

Does this still prefer origin/HEAD? In my tree I pull from a couple of
sources (other local machines). With your change will it still prefer
origin/HEAD in this case?

  -Scott

On Fri, Feb 25, 2011 at 8:56 AM,  <bauerb@chromium.org> wrote:
> On 2011/02/25 16:39:19, evanm wrote:
>>
>> I'm concerned that the previous code was kinda tricky and this will be
>> subtly
>> different for some case we previously fixed.  Can you look at the blame
>> for
>
> the
>>
>> old code and CC some of the people who tinkered with it?
>
> (adding sky@ who added code to prefer origin/HEAD over other refs. The rest
> of
> the code was all you ;-)
>
>> This LGTM otherwise but it makes me very nervous that it will break
>> someone.
>
>
>
> http://codereview.chromium.org/6597004/
>

[Bernhard Bauer, 2011-02-25 17:21:44]

On Fri, Feb 25, 2011 at 18:06, Scott Violet <sky@chromium.org> wrote:
> Does this still prefer origin/HEAD? In my tree I pull from a couple of
> sources (other local machines). With your change will it still prefer
> origin/HEAD in this case?

I guess it won't, unless your git-svn is also set up to update origin/HEAD :-/

Maybe we should add a global (i.e. not per-branch) upstream branch
configuration option and only fall back to git-svn guessing if that
fails?

Bernhard.

>  -Scott
>
> On Fri, Feb 25, 2011 at 8:56 AM,  <bauerb@chromium.org> wrote:
> > On 2011/02/25 16:39:19, evanm wrote:
> >>
> >> I'm concerned that the previous code was kinda tricky and this will be
> >> subtly
> >> different for some case we previously fixed.  Can you look at the blame
> >> for
> >
> > the
> >>
> >> old code and CC some of the people who tinkered with it?
> >
> > (adding sky@ who added code to prefer origin/HEAD over other refs. The rest
> > of
> > the code was all you ;-)
> >
> >> This LGTM otherwise but it makes me very nervous that it will break
> >> someone.
> >
> >
> >
> > http://codereview.chromium.org/6597004/
> >

[sky, 2011-02-25 17:40:59]

On Fri, Feb 25, 2011 at 9:21 AM, Bernhard Bauer <bauerb@chromium.org> wrote:
> On Fri, Feb 25, 2011 at 18:06, Scott Violet <sky@chromium.org> wrote:
>> Does this still prefer origin/HEAD? In my tree I pull from a couple of
>> sources (other local machines). With your change will it still prefer
>> origin/HEAD in this case?
>
> I guess it won't, unless your git-svn is also set up to update origin/HEAD :-/
>
> Maybe we should add a global (i.e. not per-branch) upstream branch
> configuration option and only fall back to git-svn guessing if that
> fails?

I think that sounds reasonable, but Evan knows way more about this than I do.

  -Scott

> Bernhard.
>
>>  -Scott
>>
>> On Fri, Feb 25, 2011 at 8:56 AM,  <bauerb@chromium.org> wrote:
>> > On 2011/02/25 16:39:19, evanm wrote:
>> >>
>> >> I'm concerned that the previous code was kinda tricky and this will be
>> >> subtly
>> >> different for some case we previously fixed.  Can you look at the blame
>> >> for
>> >
>> > the
>> >>
>> >> old code and CC some of the people who tinkered with it?
>> >
>> > (adding sky@ who added code to prefer origin/HEAD over other refs. The rest
>> > of
>> > the code was all you ;-)
>> >
>> >> This LGTM otherwise but it makes me very nervous that it will break
>> >> someone.
>> >
>> >
>> >
>> > http://codereview.chromium.org/6597004/
>> >
>

[evanm, 2011-02-25 17:59:47]

On 2011/02/25 17:40:59, sky wrote:
> On Fri, Feb 25, 2011 at 9:21 AM, Bernhard Bauer <mailto:bauerb@chromium.org>
wrote:
> > On Fri, Feb 25, 2011 at 18:06, Scott Violet <mailto:sky@chromium.org> wrote:
> >> Does this still prefer origin/HEAD? In my tree I pull from a couple of
> >> sources (other local machines). With your change will it still prefer
> >> origin/HEAD in this case?
> >
> > I guess it won't, unless your git-svn is also set up to update origin/HEAD
:-/
> >
> > Maybe we should add a global (i.e. not per-branch) upstream branch
> > configuration option and only fall back to git-svn guessing if that
> > fails?
> 
> I think that sounds reasonable, but Evan knows way more about this than I do.

Sounds ok to me too.

[Bernhard Bauer, 2011-02-27 16:20:43]

On 2011/02/25 17:59:47, evanm wrote:
> On 2011/02/25 17:40:59, sky wrote:
> > On Fri, Feb 25, 2011 at 9:21 AM, Bernhard Bauer <mailto:bauerb@chromium.org>
> wrote:
> > > On Fri, Feb 25, 2011 at 18:06, Scott Violet <mailto:sky@chromium.org>
wrote:
> > >> Does this still prefer origin/HEAD? In my tree I pull from a couple of
> > >> sources (other local machines). With your change will it still prefer
> > >> origin/HEAD in this case?
> > >
> > > I guess it won't, unless your git-svn is also set up to update origin/HEAD
> :-/
> > >
> > > Maybe we should add a global (i.e. not per-branch) upstream branch
> > > configuration option and only fall back to git-svn guessing if that
> > > fails?
> > 
> > I think that sounds reasonable, but Evan knows way more about this than I
do.
> 
> Sounds ok to me too.

done. PTAL?

[evanm, 2011-02-28 22:20:52]

LGTM, please be on the lookout for this regressing people

http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py#newcode187
git_cl/git_cl.py:187: for r in remotes:
Can you use a more descriptive variable name than "r"?

http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py#newcode307
git_cl/git_cl.py:307: remote = RunGit(['config',
'rietveld.upstream-remote']).strip()
I guess the prior code always returned a cached value, while this code will run
git each time it's called.

It is probably fine (the function has "Fetch" in the name rather than "Get" so
maybe callers are aware), but you may want to scan the callers to double check.

[Bernhard Bauer, 2011-02-28 22:32:21]

http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py#newcode307
git_cl/git_cl.py:307: remote = RunGit(['config',
'rietveld.upstream-remote']).strip()
On 2011/02/28 22:20:52, evanm wrote:
> I guess the prior code always returned a cached value, while this code will
run
> git each time it's called.
> 
> It is probably fine (the function has "Fetch" in the name rather than "Get" so
> maybe callers are aware), but you may want to scan the callers to double
check.

As long as the Git config doesn't change during a git-cl run, would Bad Things
happen if a caller relied on the value being cached?

[evanm, 2011-02-28 22:35:54]

On 2011/02/28 22:32:21, Bernhard Bauer wrote:
> As long as the Git config doesn't change during a git-cl run, would Bad Things
> happen if a caller relied on the value being cached?

No, it'd just be slow(er than it needs to be).  And the convention is that
values are always implicitly cached so I am not sure whether the callers of this
function are careful.

[Bernhard Bauer, 2011-02-28 22:46:01]

On Mon, Feb 28, 2011 at 23:35,  <evanm@google.com> wrote:
> On 2011/02/28 22:32:21, Bernhard Bauer wrote:
>>
>> As long as the Git config doesn't change during a git-cl run, would Bad
>> Things
>> happen if a caller relied on the value being cached?
>
> No, it'd just be slow(er than it needs to be).  And the convention is that
> values are always implicitly cached so I am not sure whether the callers of
> this
> function are careful.

They are where it IMO matters (i.e. GetUpstreamBranch caches its
result, the other callers are just one-off cases).

> http://codereview.chromium.org/6597004/
>

[Bernhard Bauer, 2011-03-01 08:59:17]

http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py
File git_cl/git_cl.py (right):

http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py#newcode187
git_cl/git_cl.py:187: for r in remotes:
On 2011/02/28 22:20:52, evanm wrote:
> Can you use a more descriptive variable name than "r"?

Done, and committed. Scott, you might need to configure rietveld.upstream-branch
and rietveld.upstream-branch now.

[Bernhard Bauer, 2011-03-01 09:23:27]

On 2011/03/01 08:59:17, Bernhard Bauer wrote:
> http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py
> File git_cl/git_cl.py (right):
> 
> http://codereview.chromium.org/6597004/diff/2007/git_cl/git_cl.py#newcode187
> git_cl/git_cl.py:187: for r in remotes:
> On 2011/02/28 22:20:52, evanm wrote:
> > Can you use a more descriptive variable name than "r"?
> 
> Done, and committed. Scott, you might need to configure
rietveld.upstream-branch
> and rietveld.upstream-branch now.

(the second one should read rietveld.upstream-remote, of course.)