Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1622011:
Changes GetSVNBranch to prefer the first ref over the last. The... (Closed)
|
Created:
10 years, 8 months ago by sky Modified:
9 years, 7 months ago CC:
chromium-reviews, M-A Ruel Visibility:
Public. |
DescriptionChanges GetSVNBranch to prefer the first ref over the last. The
scenario I encountered was I've added another one of my machines as a
remote, and because we preferred the last ref my diffs were always
against my remote machine rather than local trunk. This change prefers
the first over the last. Another option would be to prefer something
like remotes/origin/trunk. Let me know what you think.
I've got a similar change to git-cl, but I'll wait until we agree upon
this before uploading that one.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=44094
Patch Set 1 #
Total comments: 1
Messages
Total messages: 12 (0 generated)
lgtm http://codereview.chromium.org/1622011/diff/1/2 File scm.py (right): http://codereview.chromium.org/1622011/diff/1/2#newcode175 scm.py:175: if match and match.group(1) not in svn_refs: nit: you could do instead if match: # To prefer local refs over remote ones we only set the first occurrence. # The assumption being local refs are usually first. svn_refs.setdefault(match.group(1), ref) (Fixed "occurrence")
This looks fine, but note that git-cl has much more complicated logic that has had more battle testing. It might be easier to just shell out to "git cl upstream" to determine the base branch.
On 2010/04/08 17:40:02, Evan Martin wrote: > This looks fine, but note that git-cl has much more complicated logic that has > had more battle testing. It might be easier to just shell out to "git cl > upstream" to determine the base branch. I was planning on making exactly the same change in git-cl. Are you suggesting git-cl and this class try to share code?
Without my change to GetSvnBranch I get: refs/remotes/sviolet-corp3.2/master which is the remote I added. With my changes I get: refs/remotes/origin/HEAD -Scott On Thu, Apr 8, 2010 at 11:31 AM, <evan@chromium.org> wrote: > On 2010/04/08 18:09:58, sky wrote: >> >> On 2010/04/08 17:40:02, Evan Martin wrote: >> > This looks fine, but note that git-cl has much more complicated logic >> > that > > has >> >> > had more battle testing. It might be easier to just shell out to "git >> > cl >> > upstream" to determine the base branch. > >> I was planning on making exactly the same change in git-cl. Are you >> suggesting >> git-cl and this class try to share code? > > Does "git cl upstream" currently print the wrong thing for you? From a > glance > at the code it seems it should handle the situation you've described here > ok. > > http://codereview.chromium.org/1622011/show >
Evan, You OK with my change? -Scott On 2010/04/08 19:26:04, sky wrote: > Without my change to GetSvnBranch I get: > > refs/remotes/sviolet-corp3.2/master > > which is the remote I added. With my changes I get: > > refs/remotes/origin/HEAD > > -Scott > > On Thu, Apr 8, 2010 at 11:31 AM, <mailto:evan@chromium.org> wrote: > > On 2010/04/08 18:09:58, sky wrote: > >> > >> On 2010/04/08 17:40:02, Evan Martin wrote: > >> > This looks fine, but note that git-cl has much more complicated logic > >> > that > > > > has > >> > >> > had more battle testing. It might be easier to just shell out to "git > >> > cl > >> > upstream" to determine the base branch. > > > >> I was planning on making exactly the same change in git-cl. Are you > >> suggesting > >> git-cl and this class try to share code? > > > > Does "git cl upstream" currently print the wrong thing for you? From a > > glance > > at the code it seems it should handle the situation you've described here > > ok. > > > > http://codereview.chromium.org/1622011/show > > >
LGTM
BTW, this broke things for me. :( $ git cl upstream refs/remotes/bunny/master (that is wrong) $ git branch -r bunny/abspath-remove-deprecated bunny/direct bunny/fileutil bunny/jankometer bunny/master bunny/plugin-subproc bunny/plugin-subproc2 bunny/plugins-unify bunny/printf bunny/replace_ext bunny/selectall bunny/sniff_file origin/HEAD -> origin/trunk origin/trunk (should be origin/HEAD)
On 2010/04/14 22:30:34, Evan Martin wrote: > BTW, this broke things for me. :( > > $ git cl upstream > refs/remotes/bunny/master > > (that is wrong) > > > $ git branch -r > bunny/abspath-remove-deprecated > bunny/direct > bunny/fileutil > bunny/jankometer > bunny/master > bunny/plugin-subproc > bunny/plugin-subproc2 > bunny/plugins-unify > bunny/printf > bunny/replace_ext > bunny/selectall > bunny/sniff_file > origin/HEAD -> origin/trunk > origin/trunk > > (should be origin/HEAD) I worked around it by removing the "bunny" remote.
I was worried about this breaking other cases. Should we explicitly
prefer origin/HEAD as in:
diff --git a/git-cl b/git-cl
index 84a94eb..8797490 100755
--- a/git-cl
+++ b/git-cl
@@ -121,7 +121,8 @@ class Settings(object):
svn_refs = {}
for ref in remotes:
match = git_svn_re.search(RunGit(['cat-file', '-p', ref]))
- if match and match.group(1) not in svn_refs:
+ if match and (match.group(1) not in svn_refs or
+ ref == "refs/removes/origin/HEAD"):
# To prefer local refs over remote ones we only set the first
# occurence. The assumption being local refs are usually first.
svn_refs[match.group(1)] = ref
?
-Scott
On Wed, Apr 14, 2010 at 3:31 PM, <evan@chromium.org> wrote:
> On 2010/04/14 22:30:34, Evan Martin wrote:
>>
>> BTW, this broke things for me. :(
>
>> $ git cl upstream
>> refs/remotes/bunny/master
>
>> (that is wrong)
>
>
>> $ git branch -r
>> bunny/abspath-remove-deprecated
>> bunny/direct
>> bunny/fileutil
>> bunny/jankometer
>> bunny/master
>> bunny/plugin-subproc
>> bunny/plugin-subproc2
>> bunny/plugins-unify
>> bunny/printf
>> bunny/replace_ext
>> bunny/selectall
>> bunny/sniff_file
>> origin/HEAD -> origin/trunk
>> origin/trunk
>
>> (should be origin/HEAD)
>
> I worked around it by removing the "bunny" remote.
>
> http://codereview.chromium.org/1622011/show
>
This started biting me today too, but doing the fix outlined below fixes it. Evan, what do you think of the change below? It prefers origin/trunk over all other refs. -Scott On Wed, Apr 14, 2010 at 3:38 PM, Scott Violet <sky@chromium.org> wrote: > I was worried about this breaking other cases. Should we explicitly > prefer origin/HEAD as in: > > diff --git a/git-cl b/git-cl > index 84a94eb..8797490 100755 > --- a/git-cl > +++ b/git-cl > @@ -121,7 +121,8 @@ class Settings(object): > svn_refs = {} > for ref in remotes: > match = git_svn_re.search(RunGit(['cat-file', '-p', ref])) > - if match and match.group(1) not in svn_refs: > + if match and (match.group(1) not in svn_refs or > + ref == "refs/remotes/origin/HEAD"): > # To prefer local refs over remote ones we only set the first > # occurence. The assumption being local refs are usually first. > svn_refs[match.group(1)] = ref > > ? > > -Scott > > On Wed, Apr 14, 2010 at 3:31 PM, <evan@chromium.org> wrote: >> On 2010/04/14 22:30:34, Evan Martin wrote: >>> >>> BTW, this broke things for me. :( >> >>> $ git cl upstream >>> refs/remotes/bunny/master >> >>> (that is wrong) >> >> >>> $ git branch -r >>> bunny/abspath-remove-deprecated >>> bunny/direct >>> bunny/fileutil >>> bunny/jankometer >>> bunny/master >>> bunny/plugin-subproc >>> bunny/plugin-subproc2 >>> bunny/plugins-unify >>> bunny/printf >>> bunny/replace_ext >>> bunny/selectall >>> bunny/sniff_file >>> origin/HEAD -> origin/trunk >>> origin/trunk >> >>> (should be origin/HEAD) >> >> I worked around it by removing the "bunny" remote. >> >> http://codereview.chromium.org/1622011/show >> >
LGTM On Thu, Apr 22, 2010 at 9:21 AM, Scott Violet <sky@chromium.org> wrote: > This started biting me today too, but doing the fix outlined below fixes it. > > Evan, what do you think of the change below? It prefers origin/trunk > over all other refs. > > -Scott > > On Wed, Apr 14, 2010 at 3:38 PM, Scott Violet <sky@chromium.org> wrote: >> I was worried about this breaking other cases. Should we explicitly >> prefer origin/HEAD as in: >> >> diff --git a/git-cl b/git-cl >> index 84a94eb..8797490 100755 >> --- a/git-cl >> +++ b/git-cl >> @@ -121,7 +121,8 @@ class Settings(object): >> svn_refs = {} >> for ref in remotes: >> match = git_svn_re.search(RunGit(['cat-file', '-p', ref])) >> - if match and match.group(1) not in svn_refs: >> + if match and (match.group(1) not in svn_refs or >> + ref == "refs/remotes/origin/HEAD"): >> # To prefer local refs over remote ones we only set the first >> # occurence. The assumption being local refs are usually first. >> svn_refs[match.group(1)] = ref >> >> ? >> >> -Scott >> >> On Wed, Apr 14, 2010 at 3:31 PM, <evan@chromium.org> wrote: >>> On 2010/04/14 22:30:34, Evan Martin wrote: >>>> >>>> BTW, this broke things for me. :( >>> >>>> $ git cl upstream >>>> refs/remotes/bunny/master >>> >>>> (that is wrong) >>> >>> >>>> $ git branch -r >>>> bunny/abspath-remove-deprecated >>>> bunny/direct >>>> bunny/fileutil >>>> bunny/jankometer >>>> bunny/master >>>> bunny/plugin-subproc >>>> bunny/plugin-subproc2 >>>> bunny/plugins-unify >>>> bunny/printf >>>> bunny/replace_ext >>>> bunny/selectall >>>> bunny/sniff_file >>>> origin/HEAD -> origin/trunk >>>> origin/trunk >>> >>>> (should be origin/HEAD) >>> >>> I worked around it by removing the "bunny" remote. >>> >>> http://codereview.chromium.org/1622011/show >>> >> > |
