[webkit-dev] webkit-patch, check-webkit-style and git now support --squash and --git-commit

[Chris Jerdonek]

Thanks for your responses, Ojan.

On Fri, May 14, 2010 at 6:29 PM, Ojan Vafai <ojan at chromium.org> wrote:
>> Maybe it already does this, but would it be possible to make the default
>> behavior be to throw an error if there is more than one possibility for
>> what should be committed?
>
> That's what it currently does.
>
>> It seems like this would reduce the chance of
>> accidentally committing the wrong information.  A configuration option
>> could still be used to override this behavior for those who know they want
>> to use one of the two behaviors exclusively.
>
> I think the --squash behavior is actually pretty simple. It's a lot like
> svn. It commits everything in your branch. End of story. Now
> that https://bugs.webkit.org/show_bug.cgi?id=38852 is almost fixed, it's
> much safer as well.


Yes, it's simple.  My point was that it may be safer if the script did not
commit everything by default.  That way we avoid accidental over-commits if
one forgets to type --git-commit or overlooks that their branch contains
multiple patches.

>> I also suggest this because it's not clear to me that there are only two
>> camps.  For example, I maintain one branch per bug (branch-per-bug), but I
>> will often create a branch from one of these when working on a bug that
>> depends on another of my bugs being landed (commit-per-bug).  So I can see
>> myself using both options.
>
> I don't actually see what the third option here is. I'd call both of those
> approaches branch-per-bug. You'll commit the first branch using --squash and
> then you can commit the second branch with --squash. That said, I find
> myself using --squash ~80% of the time and --git-commit the other 20%.
> Changing the default or the webkit-patch.squash value does not preclude you
> using another value via the command-line (i.e. command-line overrides both).


In my workflow I maintain one branch per bug.  I also maintain one commit
per bug because I commit locally using --amend.  So normally I would not
have a need for --squash.  In addition, when I am working on sequential
patches, there are points in my workflow where I have multiple uncommitted
bugs on a single branch.  My main reason for describing this scenario was
to illustrate a workflow where defaulting to committing everything would
increase the likelihood of an accidental over-commit (since I sometimes
have multiple bugs on a branch).

>> Finally, I'm wondering if the three options above could perhaps be
>> simplified
>> to a single option if we made it clear that webkit-patch supports working
>> with only one revision at a time.
>
> I agree, but you're essentially saying everyone should use branch-per-bug.
> There are enough people who specifically use git for the commit-per-bug
> approach that I don't think webkit-patch will meet everyone's needs if we do
> this.


It would support a commit-per-bug workflow.  Just not committing a patch
series all at once with a single invocation of webkit-patch (at least
for now -- see below).

>> If webkit-patch supported only one revision at a time, then it seems like
>> the three options could be reduced to a single --git-commit option, which
>> would specify which commits should go into creating the single
>> revision/patch.
>
> I would be very happy with this. It would greatly simplify webkit-patch
> (both the code and usage). However, we're basically saying we won't
> explicitly support commit-per-bug workflows. If someone wanted to
> upload/commit a range of commits as separate patches, they need to do each
> one as a separate command. That said, people who want to do commit-per-bug
> workflow could easily write a script that calls webkit-patch in a loop for
> each commit.


Yes, adding support via looping is what I had in mind (at least initially).
That way we can focus on simplifying and nailing down the semantics for the
single-patch use case before working out the semantics of committing multiple
patches at once.

> There's a couple edge cases that are unclear to me. a) How do you upload
> just the working copy? Is that the default? b) Does --git-commit=* include
> the working copy? I think it should.


For check-webkit-style, I believe all uncommitted changes used to be the
default (staged and unstaged).  check-webkit-style also used to include
unstaged changes when using the --git-commit syntax (now it doesn't).

Personally, I think webkit-patch and check-webkit-style should include
unstaged changes when using the ".." syntax.  I also don't think it
would be important for the scripts to make a distinction between unstaged
and staged changes -- in part because svn-apply does not behave uniformly
in this regard.  In particular, --git-commit=HEAD.. should be just the
uncommitted changes (staged and unstaged).

--Chris