Bug 38156 – many webkit-patch commands fail in a non-svn tracking git checkout

• Component: Tools / Tests
• Status: RESOLVED FIXED

[James Robinson]

I have a git WebKit repo that does not track SVN.  In this repo I have a branch 'master' that tracks remote master and a topic branch my_branch.  I tried posting a patch by running "webkit-patch post --git-commit=master..my_branch" but it failed with:

Are you sure to continue?(y/N) y
Upload server: wkrietveld.appspot.com (change with -s/--server)
Loaded authentication cookies from /home/jamesr/.codereview_upload_cookies
Issue creation errors: {'data': ['Patch set contains no recognizable patches']}

Passing in '--no-fancy-review' let me upload the patch: https://bug-38153-attachments.webkit.org/attachment.cgi?id=54353

[Ojan Vafai]

Created attachment 56907 [details]
Patch

[Eric Seidel (no email)]

I'm not sure my git-fu is up to snuff for reviewing this.

[Ojan Vafai]

Evan's on vacation for a couple weeks. Chris, you feel able to review this?

[Chris Jerdonek]

(In reply to comment #3)
> Evan's on vacation for a couple weeks. Chris, you feel able to review this?

I think so.  Probably tonight.  In the meantime, I have some quick, minor comments.

[Chris Jerdonek]

Comment on attachment 56907 [details]
Patch

> +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py

> +    def _branch_exists(self, branch):
> +        return self.run(['git', 'show-ref', '--quiet', '--verify', branch], return_exit_code=True) == 0

Would it be better to call the parameter something other than "branch" (e.g.
"branch_path") so the caller knows to pass branch names in the form: refs/heads/<branch_name>.

>      def delete_branch(self, branch):
> -        if self.run(['git', 'show-ref', '--quiet', '--verify', 'refs/heads/' + branch], return_exit_code=True) == 0:
> +        if self._branch_exists('refs/heads/' + branch):
>              self.run(['git', 'branch', '-D', branch])

Similarly, would it be better to be more specific with the parameter name
and call it something like branch_name (to distinguish from passing in a path)?

>      def svn_branch_name(self):

Since with this patch this function is no longer being used exclusively for
Git repos that track SVN, is there a more accurate name that can be used?

> +        # A git clone of git://git.webkit.org/WebKit.git that is not tracking svn.
> +        if self._branch_exists('refs/remotes/origin/master'):
> +            return 'refs/remotes/origin/master'
> +
> +        raise ScriptError(message="\"trunk\" and \"origin/master\" branches do not exist. Can't find a branch to diff against.")

I recommend using single quotes -- especially for strings containing double quotes.
That way you don't need to escape the double quotes.

> +    def test_svn_branch_name_git_only(self):
> +        git_checkout2_path = tempfile.mkdtemp(suffix="git_test_checkout2")
> +        run_command(['git', 'clone', '--quiet', self.git_checkout_path,  git_checkout2_path])
> +        os.chdir(git_checkout2_path)
> +        scm = detect_scm_system(git_checkout2_path)
> +        self.assertEqual(scm.svn_branch_name(), 'refs/remotes/origin/master')

Should there be some sort of setUp/tearDown specific to these methods or
equivalent logic to set the working directory back to what is was before
the test ran?  Also, for the tests that don't need to track SVN, can we get
away without setting up the SVN repo in the setUp/tearDown?

[Ojan Vafai]

Created attachment 57032 [details]
Patch

[Ojan Vafai]

(In reply to comment #5)
> (From update of attachment 56907 [details])
> > +++ b/WebKitTools/Scripts/webkitpy/common/checkout/scm.py
> 
> > +    def _branch_exists(self, branch):
> > +        return self.run(['git', 'show-ref', '--quiet', '--verify', branch], return_exit_code=True) == 0
> 
> Would it be better to call the parameter something other than "branch" (e.g.
> "branch_path") so the caller knows to pass branch names in the form: refs/heads/<branch_name>.

Renamed to branch_ref.

> >      def delete_branch(self, branch):
> > -        if self.run(['git', 'show-ref', '--quiet', '--verify', 'refs/heads/' + branch], return_exit_code=True) == 0:
> > +        if self._branch_exists('refs/heads/' + branch):
> >              self.run(['git', 'branch', '-D', branch])
> 
> Similarly, would it be better to be more specific with the parameter name
> and call it something like branch_name (to distinguish from passing in a path)?

Done.

> >      def svn_branch_name(self):
> 
> Since with this patch this function is no longer being used exclusively for
> Git repos that track SVN, is there a more accurate name that can be used?

s/svn/remote here and in a couple other functions.

> > +        # A git clone of git://git.webkit.org/WebKit.git that is not tracking svn.
> > +        if self._branch_exists('refs/remotes/origin/master'):
> > +            return 'refs/remotes/origin/master'
> > +
> > +        raise ScriptError(message="\"trunk\" and \"origin/master\" branches do not exist. Can't find a branch to diff against.")
> 
> I recommend using single quotes -- especially for strings containing double quotes.
> That way you don't need to escape the double quotes.

Yeah. There is a single quote in there as well, but whatever. :)

> > +    def test_svn_branch_name_git_only(self):
> > +        git_checkout2_path = tempfile.mkdtemp(suffix="git_test_checkout2")
> > +        run_command(['git', 'clone', '--quiet', self.git_checkout_path,  git_checkout2_path])
> > +        os.chdir(git_checkout2_path)
> > +        scm = detect_scm_system(git_checkout2_path)
> > +        self.assertEqual(scm.svn_branch_name(), 'refs/remotes/origin/master')
> 
> Should there be some sort of setUp/tearDown specific to these methods or
> equivalent logic to set the working directory back to what is was before
> the test ran?  Also, for the tests that don't need to track SVN, can we get
> away without setting up the SVN repo in the setUp/tearDown?

That's probably better. They should run a lot faster that way.

[Ojan Vafai]

ping

[Chris Jerdonek]

Comment on attachment 57032 [details]
Patch

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!

[Chris Jerdonek]

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

Similarly, the parameter name should probably also be changed if the method name is changed.

[Ojan Vafai]

Created attachment 57369 [details]
Patch

[Ojan Vafai]

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.

[Chris Jerdonek]

Comment on attachment 57369 [details]
Patch

Thanks for your responses.  A couple final comments for you to consider before landing.  r=me

> +        raise ScriptError(message='Can\'t find a branch to diff against. %s branches do not exist.' % "and".join(remote_branch_refs))

Looks like this needs to be " and ".  You can also go back to double quotes now if you want. :)

> +    def tearDown(self):
> +        # Change back to a valid directory so that later calls to os.getcwd() do not fail.
> +        os.chdir(detect_scm_system(os.path.dirname(__file__)).checkout_root)

I would have returned to the original directory to be consistent with the philosophy
of tests leaving no trace behind.

[Chris Jerdonek]

(In reply to comment #12)
> > 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.

From the below, it looks like it accepts a reference.  Perhaps what you meant is that it only accepts branch references (and not, say, remote-tracking references).

> -    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_ref_exists('refs/heads/' + branch_name):
> +            self.run(['git', 'branch', '-D', branch_name])

[Ojan Vafai]

(In reply to comment #14)
> (In reply to comment #12)
> > > 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.
> 
> From the below, it looks like it accepts a reference.  Perhaps what you meant is that it only accepts branch references (and not, say, remote-tracking references).

Nope. We're only passing the branch_name to git branch. We pass the reference to _branch_ref_exists. If you pass the reference to git branch, you get an error.

> > +    def delete_branch(self, branch_name):
> > +        if self._branch_ref_exists('refs/heads/' + branch_name):
> > +            self.run(['git', 'branch', '-D', branch_name])

[Ojan Vafai]

Committed r60633: <http://trac.webkit.org/changeset/60633>

[WebKit Review Bot]

http://trac.webkit.org/changeset/60633 might have broken GTK Linux 64-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/60632
http://trac.webkit.org/changeset/60633
http://trac.webkit.org/changeset/60634
http://trac.webkit.org/changeset/60635
http://trac.webkit.org/changeset/60636