Hi everyone,

I sometimes break up the critic's review by using `git commit --amend`
to polish my commits. (I'm sorry....)

I read the issue (https://github.com/mozilla/servo/issues/1468) to
learn how to polish commits, but I feel that it have not gotten the
consensus of the best practice to polish commits with your reviews.

So what is the "better" practice to concentrate our commits before
merge it like the reviewing custom in bugzilla@mozilla?

Regards

--
Tetsuharu OHZEKI

On 30/01/14 14:39, Tetsuharu OHZEKI wrote:
> Hi everyone,
>
> I sometimes break up the critic's review by using `git commit --amend`
> to polish my commits. (I'm sorry....)
>
> I read the issue (https://github.com/mozilla/servo/issues/1468) to
> learn how to polish commits, but I feel that it have not gotten the
> consensus of the best practice to polish commits with your reviews.

I don't know what counts as consensus, but I will describe what I think
best practice ought to be, and hopefully explain why.

When you use a critic-like review system which stores a record of which
commits have been reviewed, and which issues have been addressed,
continually squashing the review into a single "patch" in the way that
you would with mq doesn't really work, because the review tool is using
its record of the existing commits to track where comments have been
made and what has already been reviewed.

Even in GitHub (without critic) this continual-squash mode of operation
is a problem because each new set of changes to a patch cause all
comments on previous version to be lost, so you essentially have to
start the review from scratch on each new push.

Therefore I think the optimal system is one where you don't alter
history at all until the review is accepted. At this point you rebase
onto latest master and squash into a single commit. If that resulted in
a non-trivial merge you force-push the branch and get review of the
resulting "effective merge commit". Otherwise just force-push the branch
and ask the reviewer to mark as r+ in github so that bors integrates a
single commit.

So in summary, I think the best practice is :

* Don't do anything that requires a git push -f until your review is
accepted (or unless you really know what you are doing)

* Once your review is accepted, squash into logical commits, rebase on
master and git push -f

* If for some reason you need to rebase, and then make more commits,
push the rebase first, then get the review rebased on critic (I can help
with this, as can Ms2ger) before you push your new commits.

Right now, I've been requesting that people squash those commits
before landing. That does lead to the additional overhead that kmc
describes, but it makes reviewing a lot easier and the history cleaner.
I think it would be optimal if bors squashed automatically when merging.
I guess we need a volunteer to get that done.

HTH
Ms2ger

On Thursday, January 30, 2014 11:04:52 AM UTC-4, Ms2ger wrote:
> Right now, I've been requesting that people squash those commits
>
> before landing. That does lead to the additional overhead that kmc
>
> describes, but it makes reviewing a lot easier and the history cleaner.
>
> I think it would be optimal if bors squashed automatically when merging.
>
> I guess we need a volunteer to get that done.
>
>
>
> HTH
>
> Ms2ger

Indeed - I'd also notice about rebasing against origin/master _before_ forcing update your repository - this way you could catch last-minute code changes that might end up in bors failing at build step. My usual steps are now the following:

1. Create a local branch:
$ git checkout -b <local_branch> --track origin/master

2. Code/hack/do stuff then commit:
$ git commit -a

3. Push local branch to your cloned repository:
$ git push <your_repository> <local_branch>

4. Create a PR in GitHub (which also creates a Critic link automagically)

5. Wait for reviewers' feedback - if something needs to be fixed, fix it in a separate patch: (step 2) and then push it to your cloned repository without forcing (step 3)

6. After receiving "Accepted" in Critic, squash or fixup all your patches together (I usually do fixup, as I tend to provide complete commit changelog on the first patch):
$ git rebase -i origin/master

7. Update origin/master, rebasing your local branch so you make sure your patch still applies correctly:
$ git pull --rebase

8. Finally do a force push (and further ask for r? on #servo):
$ git push -f <your_repository> <local_branch>

This could be documented on Servo's Wiki - what do you guys think?