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.
_______________________________________________
dev-servo mailing list
dev-servo@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-servo
