[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


Chris Jerdonek <cjerdonek at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57032|review?                     |review-
               Flag|                            |




--- Comment #9 from Chris Jerdonek <cjerdonek at webkit.org>  2010-05-28 06:35:56 PST ---
(From update of attachment 57032)
I wasn't too familiar with references other than branches before reviewing
this, so I apologize if some of my comments are basic.

> +++ b/WebKitTools/ChangeLog
> +        If the trunk branch doesn't exist, fallback to the master branch.

When I see "branch", I normally think of my locally created branches (i.e.
the refs/heads, or what gets listed when I execute git-branch), but here you
mean remote-tracking branches.  Would it be better to say "remote-tracking
branch" instead of just "branch"?

It would also be good to state up-front which one is the SVN-tracking branch
and which is the Git-tracking branch for people that don't know off-hand,
perhaps in parentheses.

> +++ 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?

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.

> -    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.

> -    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.

> +            return 'refs/remotes/trunk'

Before you returned just "trunk", but now you return the fully-qualified
reference path.  Would it be better to name this function remote_reference()
(as discussed above)?

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.

> +        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.

>          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?

> +++ 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.

> +        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 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.

>      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 expect this will be good to go once these comments are addressed.  Thanks!

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