[Webkit-unassigned] [Bug 38156] many webkit-patch commands fail in a non-svn tracking git checkout

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38156





--- Comment #12 from Ojan Vafai <ojan at chromium.org>  2010-05-28 13:34:02 PST ---
Thanks for the thorough review.

(In reply to comment #9)
> > +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py
> >      # Git-specific methods:
> > +    def _branch_exists(self, branch_ref):
> > +        return self.run(['git', 'show-ref', '--quiet', '--verify', branch_ref], return_exit_code=True) == 0
> 
> Question: do all git commands also accept a fully-qualified reference path (i.e.
> branch "ref") wherever a simple branch name is accepted?

No. git branch -D fails if you give it a reference. That's the only case I've found so far that doesn't accept a reference.

> Would it be better to call this _reference_exists()?  The documentation for
> git-show-ref calls the values that show-ref displays "references".  See below
> for more comments.

Done.

> > -    def delete_branch(self, branch):
> > -        if self.run(['git', 'show-ref', '--quiet', '--verify', 'refs/heads/' + branch], return_exit_code=True) == 0:
> > -            self.run(['git', 'branch', '-D', branch])
> > +    def delete_branch(self, branch_name):
> > +        if self._branch_exists('refs/heads/' + branch_name):
> > +            self.run(['git', 'branch', '-D', branch_name])
> 
> Here you use the word "branch" exclusively for local "refs/heads" branches,
> whereas above in the _branch_exists() method you use the word "branch" for any
> git reference.
> 
> It might be good to maintain a distinction between these two notions in your
> choice of variable and function names.  For example, "branch" could be used
> exclusively for "refs/heads" references and "reference" for any ref).  You
> could then further qualify these terms using branch_ref (or branch_reference)
> for the fully-qualified name, and branch_name for the short name (as you have
> done above).  Similarly, you could use remote_ref (or remote_reference) to
> refer only to the remote-tracking branches.  Just a suggestion if that also
> makes sense to you.

This makes total sense to me. I changed all the places I could find. I prefer branch_ref to reference. I think reference is too generic of a word. Also, I wouldn't normally abbreviate to ref, but git does it all over the place in git commands (e.g. show-ref), so I think it's OK here.

> > -    def svn_branch_name(self):
> > -        # FIXME: This should so something like: Git.read_git_config('svn-remote.svn.fetch').split(':')[1]
> > -        # but that doesn't work if the git repo is tracking multiple svn branches.
> > -        return 'trunk'
> > +    def remote_branch_name(self):
> > +        # A git-svn checkout as per http://trac.webkit.org/wiki/UsingGitWithWebKit.
> > +        if self._branch_exists('refs/remotes/trunk'):
> > +            # FIXME: This should so something like: Git.read_git_config('svn-remote.svn.fetch').split(':')[1]
> > +            # but that doesn't work if the git repo is tracking multiple svn branches.
> 
> Under what circumstances would someone track multiple SVN branches?  Could they
> be doing that even if they followed the wiki instructions?  The reason I ask
> is that this code seems to assume they are following the wiki instructions
> anyways.

Yes, we used Git.read_git_config('svn-remote.svn.fetch').split(':')[1] for a few days and it broke at least one person (Oliver). I think you would track multiple svn repos if you wanted to also track a fork (e.g. a release branch) from the same git repo. I don't know though. Tracking multiple svn repos was too much for me to wrap my little head around, so I just changed the code back to how it was ('trunk'). We could try to make this more generic, but noone has seemed to need that yet.

> > +            return 'refs/remotes/trunk'
> 
> Out of curiosity, why did you decide to start using the fully-qualified
> reference name instead of the one-word name?  It might be good to include
> a comment explaining why and mentioning where it matters.

There were a couple places that needed the ref and the other places that only needed a branch but were OK with taking the ref. I also prefer that it's more explicit, e.g. if you create refs/heads/trunk, we don't want to use that, right? Added a comment.

> > +        raise ScriptError(message='"trunk" and "origin/master" branches do not exist. Can\'t find a branch to diff against.')
> 
> Saying "remote-tracking branches" might be clearer here.  I would also put the
> "Can't find..." part first, and follow with the more specific info second.

Fixed. I also changed this code around a bit to avoid copy-pasting the branch_refs.

> >          if not len(args):
> > -            args.append('%s..HEAD' % self.svn_branch_name())
> > +            args.append('%s..HEAD' % self.remote_branch_name())
> 
> Again to clarify, it's okay to use fully-qualified reference paths here
> rather than just the one-word name?

Correct.

> > +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py
> > @@ -635,25 +635,55 @@ Q1dTBx0AAAB42itg4GlgYJjGwMDDyODMxMDw34GBgQEAJPQDJA==
> >  
> > +    def setUp(self):
> 
> It would be good to document what this method does and which instance
> variables it sets.

I added a docstring explaining what the method does. I don't feel like documenting each instance variable is worthwhile though and likely to get out of date if the code changes.

> > +        os.chdir(self.git_checkout_path)
> > +        self.scm = detect_scm_system(self.git_checkout_path)
> > +
> > +    def tearDown(self):
> > +        run_command(['rm', '-rf', self.git_checkout_path])
> > +        run_command(['rm', '-rf', self.untracking_checkout_path])
> 
> It seems like you should be os.chdir()'ing back to the original working
> directory in the tearDown().  You can save the original in the setUp().
>
> >      def tearDown(self):
> >          SVNTestRepository.tear_down(self)
> > -        self._tear_down_git_clone_of_svn_repository()
> > +        self._tear_down_git_checkout()
> 
> Again, I would os.chdir() back to the original directory in the tearDown().

I did like SVNTestRepository.tear_down and changed to the checkout root.

> > +    def test_remote_branch_name(self):
> > +        self.assertEqual(self.scm.remote_branch_name(), 'refs/remotes/origin/master')
> > +
> > +    def test_remote_branch_name_untracking(self):
> > +        os.chdir(self.untracking_checkout_path)
> > +        self.assertRaises(ScriptError, self.untracking_scm.remote_branch_name)
> 
> It seems the latter method only needs one of the two checkouts created in the
> setUp().  Maybe to save time you could put both asserts in a single test
> method.

Good point. Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.