webkit-patch works oddly on local Git branches

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

If I have one branch 'foo' off of HEAD, and then I branc 'bar' off of foo, ideally, if I have bar checked out I should be able to say webkit-patch -g foo and get just the changes on bar plus whatever is dirty in the tree. Instead, I'm not sure what happens. If I say -g foo..HEAD, I get just the checked in files (which makes sense), but then there is no way to create or edit the ChangeLog and have it get rolled into the patch. You have to run it once, break out, edit the ChangeLog, check it back in, and run it again.

[Dirk Pranke]

Also, it would be nice if webkit-patch looked for branch.<branch>.merge to pick the upstream instead of trying to find the remote.

[Dirk Pranke]

Created attachment 127743 [details]
Patch

[Dirk Pranke]

Okay, I've dug into this a bit more, and I understand what the current code does, but not why. I have also upload a patch that starts to approximate the behavior I'd like, but the patch needs tests and I'm not sure what the best way to handle command line switches is.

First, here's the current implementation, in webkitpy/common/checkout/scm/git.py:

    def merge_base(self, git_commit):                                           
        if git_commit:                                                          
            # Special-case HEAD.. to mean working-copy changes only.            
            if git_commit.upper() == 'HEAD..':                                  
                return 'HEAD'                                                   
                                                                                
            if '..' not in git_commit:                                          
                git_commit = git_commit + "^.." + git_commit                    
            return git_commit                                                   
                                                                                
        return self.remote_merge_base()                                         
                                                                                
where remote_merge_base will diff against the svn head if set, and if not, the remote head (and this is the default if git_commit isn't set).

First, I have no idea why we rewrite '-g bar' into '-g bar^..bar'. 

Second, I don't know why we we rewrite '-g HEAD..' except that it's probably to get around the rewriting of '-g bar'.

However, the '-g bar' rewrite means that I can't actually get the diff I want (diff against a tracking branch *and* including the working changes) without code changes. 

At any rate, this is pretty much never the default behavior I want, at least for 'pretty-patch' or 'upload'. For me, I always want to diff against the tracking branch (which if you are on branch 'foo', is set in the git config variable 'branch.foo.merge'). 

Specifically, what I'd like is, if I am on a branch 'foo' that tracks 'bar' that tracks 'master', and I type 'webkit-patch upload', and I have not yet created a ChangeLog, for it to prepare a changelog based on the changes in 'git diff -g bar' (note that this includes any uncommitted changes in my working directory), preview the diff (including the changelog in the diff), and then post the whole patch up to the bug.

Given the patch I just posted, I can get this with 'webkit-patch -g UPSTREAM..'.

But, it seems to me that -- unless there are reasons to the contrary -- we'd be better off deleting some of these special cases and letting the user specify the intent more directly? E.g., delete the two special cases, default to diffing against the tracking branch, and only diffing against the remote_merge_base() if the tracking branch isn't set?

What do others think? Surely there are reasons for the existing behavior? Do others have strong preferences for the defaults including (or not including) working-copy changes?

[Dirk Pranke]

Anyone have any thoughts on this?

[Evan Martin]

In principle I approve of deleting special cases and just requiring "normal" git syntax for describing what to upload.

In practice I worry that someone relies on the current behavior (after all, they implemented it...).

[Dirk Pranke]

Okay, spelunking around in the history, it looks like much of the behavior was implemented in bug 39364 and bug 39624. On those bugs, mrobinson, ddkilzer, and aroben commented about possible workflow variations.

I'm not sure how many of them will be affected by this, since much of the discussion was around squash/no-squash which (I don't think) I'm affecting. However, adding them to this bug in case they have any thoughts

Ojan, David, Adam (Roben), Martin - any thoughts?

I can also send a note out to webkit-dev to see if anyone else has habits that might be hurt by this.

It looks like the '-g bar' -> '-g bar^..bar' rewrite was probably done so that we could get an easy cherry-pick-style upload. Personally, I'd probably prefer that we use a different flag for that syntax and leave -g for the general commitish notation, but I could also be open to adding a new flag for the new behavior if changing -g would be too undesirable.

[Ojan Vafai]

(In reply to comment #6)
> Okay, spelunking around in the history, it looks like much of the behavior was implemented in bug 39364 and bug 39624. On those bugs, mrobinson, ddkilzer, and aroben commented about possible workflow variations.
> 
> I'm not sure how many of them will be affected by this, since much of the discussion was around squash/no-squash which (I don't think) I'm affecting.

squash/no-squash doesn't exist anymore.

> However, adding them to this bug in case they have any thoughts
> 
> Ojan, David, Adam (Roben), Martin - any thoughts?

I don't have opinions on this and generally agree that fewer special cases is a good thing. In practice, the only workflows I use are not having -g or having -g point to a single commit hash.

> It looks like the '-g bar' -> '-g bar^..bar' rewrite was probably done so that we could get an easy cherry-pick-style upload. Personally, I'd probably prefer that we use a different flag for that syntax and leave -g for the general commitish notation, but I could also be open to adding a new flag for the new behavior if changing -g would be too undesirable.

"webkit-patch upload -g commit_hash" should upload that one commit, no?

[Dirk Pranke]

(In reply to comment #7)
> > It looks like the '-g bar' -> '-g bar^..bar' rewrite was probably done so that we could get an easy cherry-pick-style upload. Personally, I'd probably prefer that we use a different flag for that syntax and leave -g for the general commitish notation, but I could also be open to adding a new flag for the new behavior if changing -g would be too undesirable.
> 
> "webkit-patch upload -g commit_hash" should upload that one commit, no?

I'm not sure what you mean by "should" here ...

I was suggesting that webkit-patch upload -g commit_hash should upload the patch you'd get by 'git diff commit_hash', which is the difference between that change and your current working copy.

It sounds like you want '-g commit_hash' to produce the patch specified by 'commit_hash^..commit_hash' (which is of course what the code currently does). 

I personally find the latter useless to me, but if it's useful to you then I certainly don't want to just get rid of it :) We could either make the latter a new argument, or make the former a new argument, or come up with some additional syntactic hack to get both kinds of functionality in the existing single argument. Personally, I don't care about either of the first two, but I would prefer not to do the third (the syntactic hack), since I think it's just confusing.

Note that at the same time I am suggesting we have a syntactic shortcut for 'diff against the tracking/upstream branch' so that webkit-patch will figure out what the upstream branch is automatically.

Make sense?

[Dirk Pranke]

Hum ... of course, it would be helpful if I could remember what I actually wrote in my patch. Sorry, Ojan ...

Given the patch as it is uploaded, yes, '-g commit_hash' will produce just that single diff, and in fact I am getting my 'diff against upstream' behavior using a syntactic hack, but I can't actually diff against any other ref and include the working changes. 

Rather, what I was suggesting is to strip out the special-case logic in lines 193-200 as well as include some sort of 'UPSTREAM' tweak. But it sounds like there's a use for that code.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> Ojan, David, Adam (Roben), Martin - any thoughts?
> 
> I can also send a note out to webkit-dev to see if anyone else has habits that might be hurt by this.
> 
> It looks like the '-g bar' -> '-g bar^..bar' rewrite was probably done so that we could get an easy cherry-pick-style upload. Personally, I'd probably prefer that we use a different flag for that syntax and leave -g for the general commitish notation, but I could also be open to adding a new flag for the new behavior if changing -g would be too undesirable.

I use "webkit-patch create-bug" whenever there isn't a bug on bugs.webkit.org yet because it's quicker when I've already committed locally with a ChangeLog.

Otherwise, when using "webkit-patch upload" I almost always use the -g switch with the assumption that it will pick up a single commit, although that's largely because it's how -g was implemented originally.

I'm fine with making the "-g" switch more git-like in general, although an email to webkit-dev is always appreciated when a widely-used tool's behavior changes significantly.

[Dirk Pranke]

As just mentioned on webkit-dev, I think I will change this patch so that -g remains unchanged and we add another command line flag that gives the "pure git" behavior.

[Dirk Pranke]

Created attachment 130460 [details]
Patch

[Dirk Pranke]

Okay, adding another command line flag looked like it would be a huge hassle and make the SCM interface even more confusing, so I've decided to stay with one flag and overload it slightly more:

Assume you have a branch 'foo' with one commit 'commit1', and then a branch 'bar' branched off of foo (using checkout -t -b foo)  with another 'commit2', and then a local working copy with 'commit3':

webkit-patch -g foo  => git diff foo^..foo => (commit1), as before
webkit-patch -g foo.. => git diff foo..HEAD => (commit1, commit2)
webkit-patch -g foo.... => git diff foo => (commit1, commit2, commit3)
webkit-patch -g UPSTREAM => git diff foo^..foo => (commit1)  (since foo is the upstream branch of bar, the current branch)
webkit-patch -g UPSTREAM.. => git diff foo^..HEAD
webkit-patch -g UPSTREAM.... => git diff foo => (commit1, commit2, commit3)

apart from changing ".." to "...." so that it didn't have the opposite semantics of the underlying git syntax, the behavior of -g should be unchanged.

Ojan and/or Adam, please review?

[Ojan Vafai]

Comment on attachment 130460 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130460&action=review

> Tools/Scripts/webkitpy/tool/steps/options.py:47
> +    git_commit = make_option("-g", "--git-commit", action="store", dest="git_commit", help="Operate on a local commit. If a range, the commits are squashed into one. <ref>.. includes the working copy changes. UPSTREAM can be used for the upstream/tracking branch.")

This should be "<ref>.... includes the working copy changes" right?

[Dirk Pranke]

(In reply to comment #14)
> (From update of attachment 130460 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130460&action=review
> 
> > Tools/Scripts/webkitpy/tool/steps/options.py:47
> > +    git_commit = make_option("-g", "--git-commit", action="store", dest="git_commit", help="Operate on a local commit. If a range, the commits are squashed into one. <ref>.. includes the working copy changes. UPSTREAM can be used for the upstream/tracking branch.")
> 
> This should be "<ref>.... includes the working copy changes" right?

right! will fix. Thanks for the review!

[Dirk Pranke]

Committed r110195: <http://trac.webkit.org/changeset/110195>