Bug 36394 – Include git commits in the diff for webkit-patch upload/land.

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Include git commits in the diff for webkit-patch upload

[Ojan Vafai]

Created attachment 51197 [details]
Patch

[Adam Barth]

Cool!  CCing our resident git expert.

I think there are other places where we say "trunk" that we should do this trick instead.

[Adam Barth]

Comment on attachment 51197 [details]
Patch

This might break some of our tests.  Did you run test-webkitpy --all?

[Chris Jerdonek]

Note that check-webkit-style also uses create_patch() when invoked without passing paths.  What implications will this change have?

Also, should the same logic be applied to create_patch_from_local_commit() and create_patch_since_local_commit() for consistency?

Might we want to create a new method instead?

[Evan Martin]

+        # Diff against the remote server to include both working copy and committed diffs.

It's not the remote server (all git operations but push/pull are local), it's the remote branch.


+        return run_command(['git', 'diff', '--binary', remote])

so say you're in this situation

*--A--B--C--D   <- trunk
    \
     X--Y--Z  <- my awesome patch

you're diffing D and Z, so your patch will include all of the stuff in A, B, C, which I don't think you want.

Two options:
1) refuse to do a diff unless your patch is merged with trunk.  (If you did a "merge trunk" into your branch, you'd have a new commit derived from D and Z and your diff command would display the right thing.)

2) or, do "git diff --binary trunk..."  (with the ... added), which basically means "diff the stuff I did not on trunk" and is computed as a diff of Z and A (the point where your branch diverged from trunk)

[Ojan Vafai]

Created attachment 51322 [details]
Patch

[Ojan Vafai]

(In reply to comment #4)
> Note that check-webkit-style also uses create_patch() when invoked without
> passing paths.  What implications will this change have?

This means that local commits that are not in SVN will also be included in the check-webkit-style call. That seems correct to me.

> Also, should the same logic be applied to create_patch_from_local_commit() and
> create_patch_since_local_commit() for consistency?

I'm not sure. These seem correct to create a patch just for a single commit...I think.

[Adam Barth]

Technically scm.py shouldn't know about prepare-ChangeLog...

[Eric Seidel (no email)]

(In reply to comment #8)
> Technically scm.py shouldn't know about prepare-ChangeLog...

Isn't that what your new fancy webkit_checkout is for?

[Chris Jerdonek]

(In reply to comment #7)
> (In reply to comment #4)
> > Note that check-webkit-style also uses create_patch() when invoked without
> > passing paths.  What implications will this change have?
> 
> This means that local commits that are not in SVN will also be included in the
> check-webkit-style call. That seems correct to me.

Evan's post clarified what you were trying to do.  Yes, XYZ without ABC makes sense.

> > Also, should the same logic be applied to create_patch_from_local_commit() and
> > create_patch_since_local_commit() for consistency?
> 
> I'm not sure. These seem correct to create a patch just for a single commit...I
> think.

create_patch_since_local_commit() creates a patch for several commits (all commits since the given commit).  Since the caller is already providing a commit to diff with in this case, it doesn't seem like it would make sense to apply the same logic (I'm answering my own question here).  If create_patch() was supposed to include ABC in the diff, then the answer wouldn't be as clear.

[Chris Jerdonek]

Do you think you could clarify and update the documentation here?

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/style/optparser.py?rev=56331#L57

I believe where previously people did--

> check-webkit-style

they will now need to do--

> check-webkit-style --git-commit HEAD

[Ojan Vafai]

Created attachment 51443 [details]
Patch

[Ojan Vafai]

Comment on attachment 51443 [details]
Patch

Passes unittests, adds to check-webkit-style documentation and removes the prepare_changelog_arguments method.

[Eric Seidel (no email)]

This seems strange to me.  We should discuss in person.

The old "git-send-bugzilla" command took a commit id and everythign forward from that revision was sent.  That was like how "git diff" worked.  The new webkit-patch used to work like that, and then was changed to work more like git cherry-pick.  I'm confused by this new symantecs.  I'm also confused as to how you'll make land or upload work sanely with changelogs.

[Ojan Vafai]

(In reply to comment #14)
> This seems strange to me.  We should discuss in person.

I'll attempt a written discussion since I'm sure others will wonder the same thing.

> The old "git-send-bugzilla" command took a commit id and everythign forward
> from that revision was sent.  That was like how "git diff" worked. 

git-send-bugzilla sent a separate patch for each local commit. git diff gives you the diff of the working copy. Those seem totally different to me.

> The new
> webkit-patch used to work like that, and then was changed to work more like git
> cherry-pick.

I don't understand how it's like cherry-pick now.

> I'm confused by this new symantecs.  I'm also confused as to how
> you'll make land or upload work sanely with changelogs.

Hopefully this will clarify. The new behavior makes sense if you think about "webkit-patch land" will do.

Consider a branch with the following:
A--B--C--D

Where C and D are local commits and A and B are in the SVN repo. And, just for fun, lets say you also have some changes in the working copy.


Current state of the world:
webkit-patch upload: uploads only your working copy changes
webkit-patch land: commits your working copy changes locally (lets call that commit E), then proceeds to commit C, D and E as separate commits all with the same commit message


git-send-bugzilla behavior:
git-send-bugzilla C: uploads two patches, C and D. Leaves the working copy alone.
git-send-bugzilla D: uploads patch D

There is no git-send-bugzilla land.


New state after this patch:
webkit-patch upload: uploads a single-diff with all the local changes (C, D and working copy)
webkit-patch land: commits a single commit (E) with all the local changes (C, D and working copy) and resets your branch to the svn repo so you end up with A--B--E


Basically, the current state of the world only works if you keep all your changes in the working copy. In order to upload or commit, you need to "git reset trunk" or some equivalent action.

The git-send-bugzilla world could make sense if there was a way to commit a single local commit to svn, but dcommit will commit all local changes. Even if you could do this, I'm not sure it would be a great workflow.

The new state lets you have multiple local commits that get uploaded as a single patch and get committed as a single commit. This allows people to have workflows where they actually use git's ability to do local commits without needing to do a bunch of reset/rebase junk in order to upload/land a patch.

[Adam Barth]

Maybe a --squash option would let folks use both workflows?  It could be an option on both upload and land.

[Ojan Vafai]

(In reply to comment #16)
> Maybe a --squash option would let folks use both workflows?  It could be an
> option on both upload and land.

I don't understand what the other workflow is. When is the current workflow ever superior to the squash workflow?

The current workflow requires all your changes to be in the working copy. The new one lets you continue doing that if you want, but also lets you have committed changes. In theory, the current workflow lets you have local commits that you don't want to upload, but why is that useful if committing to SVN will commit all your local commits as well?

Adding it isn't a big deal I guess, except for maintenance overhead for a feature that probably won't get used. I would want it to at least default to true.

[Eric Seidel (no email)]

Comment on attachment 51443 [details]
Patch

I would rather pass a revision to prepare-ChagneLog to diff against so that you dont' have to compute the remote branch twice in two languages.

 131     print STDERR "  --full-diff    Populate the ChangeLogs from the full diff from trunk including local commits\n";

519520     def commit_with_message(self, message, username=None):

needs to be broken into little _ helper functions for my little brain to have a prayer of understanding.

I thought we had some helper function for running git config?
 588         remote = run_command(['git', 'config', 'svn-remote.svn.fetch'])

Maybe that's in credentials.py?  If so it probably should re-use code in scm.Git

%s is always better than + imo:
 607             args.append(self.svn_branch_name() + '..HEAD')

[Eric Seidel (no email)]

Overall this change is awesome btw.

[David Kilzer (:ddkilzer)]

(In reply to comment #17)
> (In reply to comment #16)
> > Maybe a --squash option would let folks use both workflows?  It could be an
> > option on both upload and land.
> 
> I don't understand what the other workflow is. When is the current workflow
> ever superior to the squash workflow?
> 
> The current workflow requires all your changes to be in the working copy. The
> new one lets you continue doing that if you want, but also lets you have
> committed changes. In theory, the current workflow lets you have local commits
> that you don't want to upload, but why is that useful if committing to SVN will
> commit all your local commits as well?
> 
> Adding it isn't a big deal I guess, except for maintenance overhead for a
> feature that probably won't get used. I would want it to at least default to
> true.

I know I'm in the minority, but I don't want webkit-patch squashing my commits or otherwise modifying my git repository, other than to update placeholders for reviewers (or bug numbers) before it lands local commits individually.

If I wanted commits squashed, I will do it myself.  Likewise I wouldn't have local changes when running webkit-patch--I would have committed them (usually on a branch) before I ran the command.  I tend to have multiple patches in progress on different branches in my git repo, so local changes don't make much sense in that scenario.

[David Kilzer (:ddkilzer)]

(In reply to comment #20)
> I know I'm in the minority, but I don't want webkit-patch squashing my commits
> or otherwise modifying my git repository, other than to update placeholders for
> reviewers (or bug numbers) before it lands local commits individually.
> 
> If I wanted commits squashed, I will do it myself.  Likewise I wouldn't have
> local changes when running webkit-patch--I would have committed them (usually
> on a branch) before I ran the command.  I tend to have multiple patches in
> progress on different branches in my git repo, so local changes don't make much
> sense in that scenario.

Again, because I'm in the minority, don't worry about satisfying my workflow, but I wanted to note the differences in the way I use git and want to use webkit-patch.

[Chris Jerdonek]

(In reply to comment #15)

[Re-ordering parts of the comment for better readability.]

I can't say I use webkit-patch a whole lot yet (mostly because of workflow issues), but I have to agree with David.

> The new state lets you have multiple local commits that get uploaded as a
> single patch and get committed as a single commit.  This allows people to have
> workflows where they actually use git's ability to do local commits without
> needing to do a bunch of reset/rebase junk in order to upload/land a patch.

Do many people use this workflow?  I was when I first started using git, but that was before I found about "git-commit --amend".  In my experience, the --amend option has for the most part eliminated the need for me to squash locally.

For me it's more common that I would want to upload only the most recent one of several local commits rather than a bunch at once.  This is when I'd like to have "serial patches" reviewed separately -- i.e. consecutive patches that depend on the previous being landed.

It seems like WebKit's processes (e.g. ChangeLogs) discourage spreading patches across several commits.  And because WebKit favors small patches anyways, it seems like such commits should perhaps be landed separately, too.

> Basically, the current state of the world only works if you keep all your
> changes in the working copy. In order to upload or commit, you need to "git
> reset trunk" or some equivalent action.

If I understand webkit-patch's capabilities correctly, one thing I think I do agree with you on is wanting the ability to land the most recent local commit -- rather than having to "reset" the most recent local commit into the working directory before landing, etc.  Let me know if I'm misunderstanding anything.  Thanks.

[David Kilzer (:ddkilzer)]

(In reply to comment #22)
> Do many people use this workflow?  I was when I first started using git, but
> that was before I found about "git-commit --amend".  In my experience, the
> --amend option has for the most part eliminated the need for me to squash
> locally.

See also "git merge --squash" and especially "git rebase -i".

[Evan Martin]

When you have a branch with multiple patches in it, do you really get the reviews all lined up for each patch such that you can "git svn dcommit" the whole branch?  Or do you split it out into multiple independent submissions somehow when it comes time to commit some piece?

[David Kilzer (:ddkilzer)]

(In reply to comment #24)
> When you have a branch with multiple patches in it, do you really get the
> reviews all lined up for each patch such that you can "git svn dcommit" the
> whole branch?  Or do you split it out into multiple independent submissions
> somehow when it comes time to commit some piece?

No, I create one branch per bug.  Sometimes I want to do some refactoring before the actual bug is fixed, so I create a series of commits on a branch.  It makes reviewing the changes a lot easier if they are split into smaller, logical pieces.  Most recently for Bug 36560, I had two patches.

[Adam Roben (:aroben)]

(In reply to comment #20)
> I know I'm in the minority, but I don't want webkit-patch squashing my commits
> or otherwise modifying my git repository, other than to update placeholders for
> reviewers (or bug numbers) before it lands local commits individually.
> 
> If I wanted commits squashed, I will do it myself.  Likewise I wouldn't have
> local changes when running webkit-patch--I would have committed them (usually
> on a branch) before I ran the command.  I tend to have multiple patches in
> progress on different branches in my git repo, so local changes don't make much
> sense in that scenario.

My workflow sounds very similar to Dave's. Count me in to the minority!

[Chris Jerdonek]

(In reply to comment #24)
> When you have a branch with multiple patches in it, do you really get the
> reviews all lined up for each patch such that you can "git svn dcommit" the
> whole branch?

I've never done this.  It's probably more trouble than it's worth.

> Or do you split it out into multiple independent submissions
> somehow when it comes time to commit some piece?

I make one branch per bug.  I create each branch as soon as I know it will be separate, so it will already be split out by the time it comes to committing.  For example, I might create--

A
A-B

and submit A and B separately for review (in different bug reports).

After landing A, I will "git svn rebase" A-B and land B (or resubmit B if it hasn't been reviewed yet).  Rarely do I do more than 2 or 3 serial patches this way.  Of course, it's better if you can structure your work so patches can be committed independently of one another, and then you can avoid this process.  But that is not always possible.

[Ojan Vafai]

(In reply to comment #20)
> I know I'm in the minority, but I don't want webkit-patch squashing my commits
> or otherwise modifying my git repository, other than to update placeholders for
> reviewers (or bug numbers) before it lands local commits individually.

I guess I don't quite understand your workflow still. For every bug you have one branch, right? Within that branch you have multiple patches. You them them reviewed and the git svn dcommit them, which makes an svn commit for each commit. Did I get that right?

If that's the case, how do you test each svn commit separately? If you don't test them separately, shouldn't they go in as a single commit? That way, if there is a problem with the commit and someone else needs to roll it out (e.g. on a release branch), they can safely roll out one atomic unit.

> If I wanted commits squashed, I will do it myself.  Likewise I wouldn't have
> local changes when running webkit-patch--I would have committed them (usually
> on a branch) before I ran the command.  I tend to have multiple patches in
> progress on different branches in my git repo, so local changes don't make much
> sense in that scenario.

Another possibility Evan suggested, which makes sense to me:

webkit-patch upload will check if there are multiple local commits and/or working copy changes. If there are, it will ask whether to squash them into one patch, if you say no, it will upload them as separate patches.

webkit-patch land warns that there are working copy changes and asks whether to commit them. Then it checks if there are multiple local commits and asks whether to squash them or not.

Both commands will also have --git-commit option to upload/land just a single local commit and a --squash=true/false option, to avoid the prompts.

I still think squash should be the default. It matches the concept of commiting as an atomic unit the thing you tested.

This is more ambitious than I was starting with, but this can be done in iterations if we agree on the desired end result.

[David Kilzer (:ddkilzer)]

(In reply to comment #28)
> (In reply to comment #20)
> > I know I'm in the minority, but I don't want webkit-patch squashing my commits
> > or otherwise modifying my git repository, other than to update placeholders for
> > reviewers (or bug numbers) before it lands local commits individually.
> 
> I guess I don't quite understand your workflow still. For every bug you have
> one branch, right? Within that branch you have multiple patches. You them them
> reviewed and the git svn dcommit them, which makes an svn commit for each
> commit. Did I get that right?

Correct.

> If that's the case, how do you test each svn commit separately? If you don't
> test them separately, shouldn't they go in as a single commit? That way, if
> there is a problem with the commit and someone else needs to roll it out (e.g.
> on a release branch), they can safely roll out one atomic unit.

You're missing the point of having separate commits--making reviews easier and making small, logical changes together.  Each commit should be able to stand on its own and pass all tests.

There are many ways to run tests against individual commits.  Usually that's done as you're developing, but using "git rebase -i" and marking each commit as "edit" would allow you to run tests against each commit.

> > If I wanted commits squashed, I will do it myself.  Likewise I wouldn't have
> > local changes when running webkit-patch--I would have committed them (usually
> > on a branch) before I ran the command.  I tend to have multiple patches in
> > progress on different branches in my git repo, so local changes don't make much
> > sense in that scenario.
> 
> Another possibility Evan suggested, which makes sense to me:
> 
> webkit-patch upload will check if there are multiple local commits and/or
> working copy changes. If there are, it will ask whether to squash them into one
> patch, if you say no, it will upload them as separate patches.
> 
> webkit-patch land warns that there are working copy changes and asks whether to
> commit them. Then it checks if there are multiple local commits and asks
> whether to squash them or not.
> 
> Both commands will also have --git-commit option to upload/land just a single
> local commit and a --squash=true/false option, to avoid the prompts.
> 
> I still think squash should be the default. It matches the concept of commiting
> as an atomic unit the thing you tested.
> 
> This is more ambitious than I was starting with, but this can be done in
> iterations if we agree on the desired end result.

That's fine to make squashing the default.  It would be cool I could set a default via .gitconfig to make the default not squash for me.

[Ojan Vafai]

Created attachment 52229 [details]
Patch

[Ojan Vafai]

OK. Screw iterations. I just did all of it. Added --squash and --git-commit as well as reading webkit-patch.squash for the git config.

[Adam Barth]

Technically, you shouldn't be passing options to scm.py.  Options are part of the command/step API, but scm.py is one layer further down.

[Ojan Vafai]

(In reply to comment #32)
> Technically, you shouldn't be passing options to scm.py.  Options are part of
> the command/step API, but scm.py is one layer further down.

Yeah, I thought about that as I was writing it. I actually started out giving create_patch, changed_files and commit_with_message optional git_commit and squash arguments. That quickly got very messy though. There's a lot of code that calls those that doesn't have access to options.

Anyways, how about I SCM takes squash and git_commit arguments instead of options?

[Adam Barth]

> Anyways, how about I SCM takes squash and git_commit arguments instead of
> options?

That seems fine.

[Ojan Vafai]

Created attachment 52305 [details]
Patch

[Ojan Vafai]

(In reply to comment #34)
> > Anyways, how about I SCM takes squash and git_commit arguments instead of
> > options?
> 
> That seems fine.

Done.

[WebKit Review Bot]

Attachment 52305 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
long (80 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:558:  line too long (90 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:568:  line too long (98 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:569:  line too long (97 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:573:  line too long (81 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:574:  line too long (83 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:579:  line too long (95 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:582:  line too long (94 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:585:  line too long (88 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:602:  line too long (90 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:618:  line too long (119 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:622:  line too long (89 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:640:  line too long (81 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:641:  line too long (104 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/tool/steps/options.py:45:  line too long (120 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/tool/steps/options.py:56:  line too long (147 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/style/optparser.py:45:  line too long (90 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/tool/steps/preparechangelog.py:57:  line too long (90 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/tool/steps/abstractstep.py:39:  line too long (80 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/tool/main.py:78:  line too long (103 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/tool/main.py:82:  line too long (107 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:487:  line too long (83 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:488:  line too long (82 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:492:  line too long (87 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:493:  line too long (82 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:626:  line too long (82 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:632:  line too long (82 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:729:  line too long (101 characters)  [pep8/E501] [5]
WebKitTools/Scripts/webkitpy/common/checkout/scm_unittest.py:795:  line too long (101 characters)  [pep8/E501] [5]
Total errors found: 42 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1

Oh man.  Our Python style overloads have arrived.  Don't worry about these errors for this patch.  We need to deal with them, but it can be in a separate patch.

[Ojan Vafai]

(In reply to comment #38)
> > Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> 
> Oh man.  Our Python style overloads have arrived.  Don't worry about these
> errors for this patch.  We need to deal with them, but it can be in a separate
> patch.

Yeah, I fixed all the non-line-length ones. Seemed dumb to restrict to 78 chars when the rest of these files done.

[Eric Seidel (no email)]

Comment on attachment 52305 [details]
Patch

Ojan and I reviewed this in person.

r=me on the prepare-ChagneLog changes.
He's going to post a new patch with just the svn_branch_name, svn_merge_base, delete_branch stuff and tests.
And I believe his plan is to re-write this to pass the individual options as arguments to the various functions which need them.  Mostly some different plumbing, but should be easier to understand than a stateful Git class.

[Ojan Vafai]

Created attachment 52542 [details]
Patch

[Ojan Vafai]

Comment on attachment 52542 [details]
Patch

This is the part eric r+'ed. I'll commit this shortly.

[Ojan Vafai]

Committed r57084: <http://trac.webkit.org/changeset/57084>

[Ojan Vafai]

Added bug 37103 for the svn_branch_name, svn_merge_base, delete_branch and assorted other bits.

[Ojan Vafai]

Created attachment 52686 [details]
Patch

[Ojan Vafai]

Comment on attachment 52686 [details]
Patch

Moves git_commit and squash to be arguments to the relevant functions instead of state stored in the Git class.

Also, adds --no-squash and lets --git-commit take a range (e.g. head~3..head).

Changes check-webkit-style to not treat --git-commit as --git-since and gets rid of --git-since. Treating the git-commit as a since is unnecessary (and otherwise limiting) since that can be replicated by just adding a ".." to the end of the commit ID.

[WebKit Review Bot]

Attachment 52686 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKitTools/Scripts/webkitpy/tool/commands/download.py:97:  whitespace before '}'  [pep8/E202] [5]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ojan Vafai]

(In reply to comment #47)
> WebKitTools/Scripts/webkitpy/tool/commands/download.py:97:  whitespace before
> '}'  [pep8/E202] [5]
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

I saw this error when uploading. I think it might be a false positive, but I'm not sure if pep8 is just dumb. I can't imagine it's saying I should put the } at the beginning of the line or at the end of the previous line though.

[Chris Jerdonek]

(In reply to comment #46)
> (From update of attachment 52686 [details])
> Moves git_commit and squash to be arguments to the relevant functions instead
> of state stored in the Git class.
> 
> Also, adds --no-squash and lets --git-commit take a range (e.g. head~3..head).
> 
> Changes check-webkit-style to not treat --git-commit as --git-since and gets
> rid of --git-since. Treating the git-commit as a since is unnecessary (and
> otherwise limiting) since that can be replicated by just adding a ".." to the
> end of the commit ID.

Something weird is happening here:

https://bugs.webkit.org/show_bug.cgi?id=36938#c14

(Eric e-mailed me about this.)

I haven't looked at this patch to understand how it impacts check-webkit-style, but I did notice this:

+        squash_help = ("All diffs from the remote branch are checked."
+                       "If excluded, prompts whether to squash when there are multiple commits.")
+        parser.add_option("-s", "--squash", action="store_true", dest="squash", help=squash_help)
+
+        squash_help = ("Only working copy diffs are checked."
+                       "If excluded, prompts whether to squash when there are multiple commits.")
+        parser.add_option("--no-squash", action="store_false", dest="squash", help=squash_help)
+
         verbose_help = "enable verbose logging."
         parser.add_option("-v", "--verbose", dest="is_verbose", default=False,
                           action="store_true", help=verbose_help)

Can "--squash" and "--no-squash" perhaps be combined into a single argument-less flag?  They are writing to the same "squash" property.  The code for the "--verbose" option above might help which uses a default parameter.

[Adam Barth]

> I saw this error when uploading. I think it might be a false positive, but I'm
> not sure if pep8 is just dumb. I can't imagine it's saying I should put the }
> at the beginning of the line or at the end of the previous line though.

It wants a trailing comma before the }.  Also, no space before the :.

[Adam Barth]

Comment on attachment 52686 [details]
Patch

Please combine --squash and --no-squash into a single option.

[Chris Jerdonek]

(In reply to comment #46)
> (From update of attachment 52686 [details])

I'm starting to wonder if a --squash option is appropriate for checking
style.  Will running check-webkit-style with the squash option actually
change your local branch, or is it just a temporary calculation step needed
for the style check?  It doesn't seem like the act of checking style should
have as a side effect that it alters your branch.

If squashing is necessary for the webkit-patch workflow you envision, then
can the squashing simply be done before the check-webkit-style invocation?

-        if options.git_commit:
-            patch = checkout.create_patch_since_local_commit(options.git_commit)
-        else:
-            patch = checkout.create_patch()
+        patch = checkout.create_patch(options.git_commit, options.squash)

It seems that more side effects and references to global objects are being
added to the method above that would make it harder to unit test than it
already is.  For example, the following is being added deep within the call to
create_patch()--

+            # FIXME: parameterize prompt so this can be tested.
+            response = User.prompt("There are %s local commits%s. Squash into a single commit? [Y/n]: " % (
+                num_local_commits, working_directory_message), repeat=3)

The style code has been moving in the opposite direction by incorporating
more dependency injection to make testing easier (e.g. by adding a mock_stderr
parameter to methods where appropriate).

Would it be possible to take a flatter approach initially, say by either--

(1) requiring the user to know in advance if s/he wants to squash, and to raise
    an error if an incompatible choice is made, rather than prompting, or
(2) structuring the code so that any prompting can take place at one of
    the outer levels of the calling code?
    
Something else that makes this code harder to test is that there is a lot
of branching going on deep down around that prompt, which is one of the
reasons I suggest trying to flatten things out a bit:

+    def should_squash(self, squash):
+        if squash is not None:
+            # Squash is specified on the command-line.
+            return squash
+
+        if Git.squash_prompt is not None:
+            # The user already responded to the prompt below on a previous should_squash call.
+            return Git.squash_prompt
+
+        config_squash = Git.read_git_config('webkit-patch.squash')
+        if (config_squash and config_squash is not ""):
+            return config_squash.lower() == "true"
+
+        # Only prompt if there are actually multiple commits to squash.
+        num_local_commits = len(self.local_commits())
+        if num_local_commits > 1 or num_local_commits > 0 and not self.working_directory_is_clean():
+            working_directory_message = "" if self.working_directory_is_clean() else " and working copy changes"
+            # FIXME: parameterize prompt so this can be tested.
+            response = User.prompt("There are %s local commits%s. Squash into a single commit? [Y/n]: " % (
+                num_local_commits, working_directory_message), repeat=3)
+            if response == "Y":
+                Git.squash_prompt = True
+            elif response == 'n':
+                Git.squash_prompt = False
+            else:
+                raise "Must answer 'Y' or 'n'"
+
+            return Git.squash_prompt
+
+        return False

[Ojan Vafai]

Not sure why this got closed when the bug it depends on was closed.

[Ojan Vafai]

(In reply to comment #52)
> (In reply to comment #46)
> > (From update of attachment 52686 [details] [details])
> 
> I'm starting to wonder if a --squash option is appropriate for checking
> style.  Will running check-webkit-style with the squash option actually
> change your local branch, or is it just a temporary calculation step needed
> for the style check?  It doesn't seem like the act of checking style should
> have as a side effect that it alters your branch.

The only step that actually modifies your local branch is committing. So, upload, check-style, etc. do not.

> -        if options.git_commit:
> -            patch =
> checkout.create_patch_since_local_commit(options.git_commit)
> -        else:
> -            patch = checkout.create_patch()
> +        patch = checkout.create_patch(options.git_commit, options.squash)
> 
> It seems that more side effects and references to global objects are being
> added to the method above that would make it harder to unit test than it
> already is.  For example, the following is being added deep within the call to
> create_patch()--
> 
> +            # FIXME: parameterize prompt so this can be tested.
> +            response = User.prompt("There are %s local commits%s. Squash into
> a single commit? [Y/n]: " % (
> +                num_local_commits, working_directory_message), repeat=3)
> 
> The style code has been moving in the opposite direction by incorporating
> more dependency injection to make testing easier (e.g. by adding a mock_stderr
> parameter to methods where appropriate).
> 
> Would it be possible to take a flatter approach initially, say by either--
> 
> (1) requiring the user to know in advance if s/he wants to squash, and to raise
>     an error if an incompatible choice is made, rather than prompting, or

Raising an error is definitely doable. The more I think about it, the more I'm OK with this. I'll try changing it and see how it looks.

> (2) structuring the code so that any prompting can take place at one of
>     the outer levels of the calling code?

I don't see a reasonable way to do this.

> Something else that makes this code harder to test is that there is a lot
> of branching going on deep down around that prompt, which is one of the
> reasons I suggest trying to flatten things out a bit:

Getting rid of the prompt should make this considerably simpler.

(In reply to comment #49)
> +        squash_help = ("All diffs from the remote branch are checked."
> +                       "If excluded, prompts whether to squash when there are
> multiple commits.")
> +        parser.add_option("-s", "--squash", action="store_true",
> dest="squash", help=squash_help)
> +
> +        squash_help = ("Only working copy diffs are checked."
> +                       "If excluded, prompts whether to squash when there are
> multiple commits.")
> +        parser.add_option("--no-squash", action="store_false", dest="squash",
> help=squash_help)
> +
> Can "--squash" and "--no-squash" perhaps be combined into a single
> argument-less flag?  They are writing to the same "squash" property.  The code
> for the "--verbose" option above might help which uses a default parameter.

The issue is that it's more like there are 3 states, False, True and None. If neither squash nor no-squash is set, then we prompt the user (or raise an error per the above comments). If one of them is set, then we avoid the prompt and assume the user wants that behavior. Another option is to combine it into a single flag with an argument. It could take true, false and confirm as values, or something like that.

[Ojan Vafai]

Created attachment 53851 [details]
Patch-6001

[Ojan Vafai]

Created attachment 53904 [details]
Patch-10001

[Ojan Vafai]

Latest patch fixes --fancy-review to respect --squash and --git-commit.

[Adam Barth]

Comment on attachment 53904 [details]
Patch-10001

WebKitTools/Scripts/webkitpy/common/checkout/scm.py:181
 +      def changed_files(self, git_commit=None, squash=None):
Should this be squash=False?

WebKitTools/Scripts/webkitpy/common/checkout/api.py:131
 +              raise ScriptError(message="Working directory must be clean to revert a revision.")
Why did you add this error handling here?  This is the wrong layer to catch this error.  Notice that apply patch doesn't have a similar check even though it has logically the same issue.

Thanks for sticking with this patch.  I didn't understand the gut part, but I appreciate the tests and the effort you put into it.  Eric should review this change, but he's been slacking off in this department.

[Ojan Vafai]

(In reply to comment #58)
> (From update of attachment 53904 [details])
> WebKitTools/Scripts/webkitpy/common/checkout/scm.py:181
>  +      def changed_files(self, git_commit=None, squash=None):
> Should this be squash=False?

No, this is a bit messy. There are currently three squash states:
None: The behavior before this patch + some explicit failures for unexpected cases (e.g. multiple local commits + working-copy changes)
False: Treat each local commit + working copy separately
True: Treat all commits + working copy as one commit.

My plan is to eventually make squash=True the default and get rid of the None state entirely, but I'd like --squash to bake for a couple weeks first.

> WebKitTools/Scripts/webkitpy/common/checkout/api.py:131
>  +              raise ScriptError(message="Working directory must be clean to
> revert a revision.")
> Why did you add this error handling here?  This is the wrong layer to catch
> this error.  Notice that apply patch doesn't have a similar check even though
> it has logically the same issue.

It's documenting the existing behavior that locally modified ChangeLogs will get reverted along with the reverse_diff's ChangeLogs. It's not a new behavior with this patch though, so I'll just remove the new error.

[Ojan Vafai]

Committed r58261: <http://trac.webkit.org/changeset/58261>

[Chris Jerdonek]

Thanks for the patch, Ojan.  I noticed a couple things in it, though, that I was hoping you could address in a subsequent patch.

> +        # Set logging level to avoid rietveld's logging spew.
> +        old_level_name = logging.getLogger().getEffectiveLevel()
> +        logging.getLogger().setLevel(logging.ERROR)
> +
> +        # Use RealMain instead of calling upload from the commandline so that
> +        # we can pass in the diff ourselves. Otherwise, upload will just use
> +        # git diff for git checkouts, which doesn't respect --squash and --git-commit.
> +        issue, patchset = upload.RealMain(args[1:], data=diff)
> +
> +        # Reset logging level to the original value.
> +        logging.getLogger().setLevel(old_level_name)

It is probably better to throttle the rietveld logging from the calling code rather than by temporarily changing it from within.  Otherwise, it doesn't seem like there is a way for the caller to, say, temporarily re-enable this logging.

You may wish to look at the pattern we're using to disable autoinstall logging from the calling code while running test-webkitpy:

http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/test-webkitpy?rev=56981#L87


> -        # FIXME: Add unit tests.
> -        if git_commit and '..' in git_commit:
> -            # FIXME: If the range is a "...", the code should find the common
> -            #        ancestor and start there.  See git diff --help for how
> -            #        "..." usually works.
> -            self._parse_error('invalid --git-commit option: option does '
> -                              'not support ranges "..": %s' % git_commit)

Above you deleted the FIXME to add more unit tests, but below you removed some unit tests without adding any new ones.  It seems like you should at least preserve the FIXME above to add more unit tests.

> +++ b/WebKitTools/Scripts/webkitpy/style/optparser_unittest.py
> @@ -114,10 +114,6 @@ class ArgumentParserTest(LoggingTestCase):

> -        self.assertRaises(SystemExit, parse, ['--git-diff=aa..bb'])
> -        self.assertLog(['ERROR: invalid --git-commit option: '
> -                        'option does not support ranges "..": aa..bb\n'])
> -

> -        (files, options) = parse(['--git-since=commit'])
> -        self.assertEquals(options.git_commit, 'commit')

Would it be possible for you to create optparser unit tests for the new options you are adding (and to update the existing tests to take into account the new options)?  Up to this point, we have maintained pretty good code coverage of the option parsing code.  Thanks.

[Ojan Vafai]

Sorry it took so long, but I'm just getting back to this.

(In reply to comment #61)
> It is probably better to throttle the rietveld logging from the calling code rather than by temporarily changing it from within.  Otherwise, it doesn't seem like there is a way for the caller to, say, temporarily re-enable this logging.

I agree, but I'm actually going to remove the logging filtering from this entirely. It's no longer something that will run on upload. It will only run on the upload bot or when explicitly run via webkit-patch post-attachment-to-rietveld. So, I think we'd actually prefer to do all the logging.

> > -        # FIXME: Add unit tests.
> > -        if git_commit and '..' in git_commit:
> > -            # FIXME: If the range is a "...", the code should find the common
> > -            #        ancestor and start there.  See git diff --help for how
> > -            #        "..." usually works.
> > -            self._parse_error('invalid --git-commit option: option does '
> > -                              'not support ranges "..": %s' % git_commit)
> 
> Above you deleted the FIXME to add more unit tests, but below you removed some unit tests without adding any new ones.  It seems like you should at least preserve the FIXME above to add more unit tests.

> Would it be possible for you to create optparser unit tests for the new options you are adding (and to update the existing tests to take into account the new options)?  Up to this point, we have maintained pretty good code coverage of the option parsing code.  Thanks.

I'll add tests for passing commit ranges to git-commit. The other options are getting deleted in bug 39624.

[Chris Jerdonek]

(In reply to comment #62)
> Sorry it took so long, but I'm just getting back to this.

Sounds good.  Thanks for eventually getting back to me. :)

Also, for posterity (even though you don't need it here), here is a comment with very basic info on filtering log messages:

https://bugs.webkit.org/show_bug.cgi?id=39614#c1

[Ojan Vafai]

> > > -        # FIXME: Add unit tests.
> > > -        if git_commit and '..' in git_commit:
> > > -            # FIXME: If the range is a "...", the code should find the common
> > > -            #        ancestor and start there.  See git diff --help for how
> > > -            #        "..." usually works.
> > > -            self._parse_error('invalid --git-commit option: option does '
> > > -                              'not support ranges "..": %s' % git_commit)
> > 
> > Above you deleted the FIXME to add more unit tests, but below you removed some unit tests without adding any new ones.  It seems like you should at least preserve the FIXME above to add more unit tests.
> 
> > Would it be possible for you to create optparser unit tests for the new options you are adding (and to update the existing tests to take into account the new options)?  Up to this point, we have maintained pretty good code coverage of the option parsing code.  Thanks.
> 
> I'll add tests for passing commit ranges to git-commit. The other options are getting deleted in bug 39624.

Actually, I went to do this and realized that I don't know what this is talking about testing. This patch made commit ranges work, so the second FIXME above is addressed. I could add a test that optparser doesn't error our when you pass a commit range, but that seems sort of silly.

[Chris Jerdonek]

(In reply to comment #64)
> > I'll add tests for passing commit ranges to git-commit. The other options are getting deleted in bug 39624.
> 
> Actually, I went to do this and realized that I don't know what this is talking about testing. This patch made commit ranges work, so the second FIXME above is addressed. I could add a test that optparser doesn't error our when you pass a commit range, but that seems sort of silly.

Yes, you can disregard that comment.  I think I also realized days after posting that particular comment that it may not have made sense or was no longer applicable.  Thanks for confirming.