[Development] [FYI] new git-gpush features, a.k.a. the smart way of pushing to gerrit

[Rutledge Shawn]

On 24 Oct 2014, at 12:38, Jędrzej Nowacki <jedrzej.nowacki at digia.com> wrote:
> 
> I admire that you want to improve everyones working process, but I think you 
> took the wrong way. You are fixing the problem on odd level, I strongly believe 
> that you should fix Gerrit, enforcing a tool on everyone is at the best far 
> from ideal.
> 
> "don't rebase unnecessarily" -> For us, Gerrit is always cherry-picking, which 
> means a user do not really control final parent of a change. Therefore in 
> theory rebase should not be a problem. Sadly It is because of two things:
> 1. It crates new patchset, which sends notification and reset score
> 2. It breaks web diff.
> Both should be fixable on the Gerrit level

I agree.  I’ve always thought the “don’t rebase” advice is artificial, because I prefer not to try to integrate a patch until I’ve made sure that it can be rebased on top of the latest code, and still works.  (If you don’t do that, then you will often waste the reviewers’ time again anyway, because the need for rebase is a surprise at the end after they’ve finally given in and +2’d.  Maybe even a few hours before a code freeze.)  So it’s often a multi-step process when revising a patch:

1) cherry-pick
2) make changes
3) build and test
4) git checkout sha1 of the parent patch of the one gerrit has; or, use the gerrit checkout link
5) cherry-pick the real patch onto this point in history to keep reviewers from complaining about the rebase
6) make sure it builds (or not; if it applied cleanly it should be OK)
7) push again
8) go back to the branch tip again to continue work on something else
and now because headers were touched because of the jumping back and forth, the build will take some time.

And the only reason AFAICT is that gerrit shows all the intermediate diffs too, not just the changes that I actually made, whenever it compares a new version of a patch with an old version, and there was a rebase in between.  There are two cases: one is that the latest patch can apply cleanly on top of either the sha1 that it was originally applied to, or on top of the newer one.  In that case, gerrit could show a cleaner diff automatically, by doing the same thing that I do manually.  The other case is that the rebase was actually necessary, in which case the reviewers still have to deal with the extra noise anyway.  At least by working on the tip instead of in the past, I already found out about that, so there’s no surprise at the end when trying to integrate.

And besides, when reviewing it’s better to give the entire patch one more look before giving +2, to make sure that the whole thing still makes sense in context.  Some people prefer to see the inter-patch diffs, but if there is noise because of rebasing, what’s wrong with re-reading the whole thing? Many of our patches are small enough that it’s not unreasonable.

We also need to be friendly enough for outside contributors.  I don’t usually complain to them about unnecessary rebasing: better to just be thankful that they are doing necessary work that we didn’t get around to.

But the real reason for not fixing Gerrit is just that we don’t own it, right?

Now you will just brag again about how your tools are better than whatever anyone else can possibly ever think of, and make more of your usual ad-hominem accusations.  But your dictatorial attitude leaves a bad taste and actually does not make me want to try them at all, compared to the mail a couple of days ago where you were just pitching them on the basis of usefulness.  That one was actually tempting.