Last modified: 2014-05-06 15:23:15 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T43074, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 41074 - report trivial rebases in Gerrit
report trivial rebases in Gerrit
Status: RESOLVED FIXED
Product: Wikimedia
Classification: Unclassified
Git/Gerrit (Other open bugs)
unspecified
All All
: Low enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-16 09:40 UTC by Antoine "hashar" Musso (WMF)
Modified: 2014-05-06 15:23 UTC (History)
9 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Antoine "hashar" Musso (WMF) 2012-10-16 09:40:27 UTC
We are often rebasing changes with no real modification. For example when correcting a typo in a commit message or child changes being automatically rebased when one of their ancestor as been altered.

OpenStack is using a python script that detect such trivial rebase and will send a comment to each changes mentioning it. That might help us a bit when doing code review.

The python script is at:

https://github.com/openstack/openstack-ci-puppet/blob/master/modules/openstack_project/files/gerrit/scripts/trivial_rebase.py

Can be installed in /var/lib/gerrit/scripts/  and added to the patchset-created hook script:


timeout -k 2m 10m python /usr/local/gerrit/scripts/trivial_rebase.py \
    patchset-created \
    --whitespace \
    --private-key-path=<SSH HOST KEY OF TRIVIAL USER> \
    --role-user=trivial-rebase@gerrit.wikimedia.org "$@"

Example output somewhere in comments of https://review.openstack.org/#/c/12324/

Trivial Rebase		Oct 8
Patch Set 5:
New patchset patch-id matches previous patchset, but whitespace content has changed.
Comment 1 Chad H. 2012-11-16 18:24:03 UTC
I want to keep from having the hooks connecting to Gerrit over SSH--so I'm not adding this hack in.

If this is going to be done, it should be done in Gerrit core.

(Also, this exists in Gerrit's contrib/ directory)
Comment 2 Antoine "hashar" Musso (WMF) 2012-11-19 15:38:45 UTC
I still want us to be able to report trivial commit, and we could even reapply the previous votes in such a case so reopening.  Should that be requested to upstream ?
Comment 3 Chad H. 2012-11-19 15:59:29 UTC
Re-applying the votes is already a work-in-progress[0], [1] and is unrelated to reporting rebases when they're done by hand (but are still trivial). If the latter's not filed upstream, it can be.

[0] https://gerrit-review.googlesource.com/#/c/34801/
[1] https://gerrit-review.googlesource.com/#/c/39257/
Comment 4 Antoine "hashar" Musso (WMF) 2014-04-09 16:03:32 UTC
Apparently we can now configure labels to recopy with: copyAllScoresOnTrivialRebase and copyAllScoresIfNoCodeChange

See https://gerrit.wikimedia.org/r/Documentation/config-labels.html#label_copyAllScoresOnTrivialRebase
Comment 5 Bawolff (Brian Wolff) 2014-04-09 16:21:36 UTC
I should note, often votes are of the form -1: No longer applies, please rebase. Or -1: Fix commit message. These types of votes should not be re-applied (Obviously you would need magic to actually know if a vote should be re-applied or not, so not sure if anything can be done about that).
Comment 6 Liangent 2014-04-09 17:18:40 UTC
(In reply to Bawolff (Brian Wolff) from comment #5)
> I should note, often votes are of the form -1: No longer applies, please
> rebase. Or -1: Fix commit message.

I don't like this kind of -1's personally, because "Can Merge: No" is already shown above. I think the purpose of these reviews are to hide the commit from that reviewer's dashboard. It is possible to provide an option for these reviewers "hide / fade changesets that can't be merged automatically"?
Comment 7 Liangent 2014-04-09 17:19:16 UTC
btw. Can inline comments get reapplied automatically as well?
Comment 8 Liangent 2014-04-09 17:22:23 UTC
(In reply to Bawolff (Brian Wolff) from comment #5)
> These types of votes should not be
> re-applied (Obviously you would need magic to actually know if a vote should
> be re-applied or not, so not sure if anything can be done about that).

Also I don't think manual rebases for "-1: No longer applies, please" would be classified as "trivial rebase" in the script, and I believe "-1: No longer applies, please" are / were only used when manual rebase is needed.

Sorry for separated comments.
Comment 9 Alex Monk 2014-04-09 18:02:30 UTC
(In reply to Liangent from comment #6)
> (In reply to Bawolff (Brian Wolff) from comment #5)
> > I should note, often votes are of the form -1: No longer applies, please
> > rebase. Or -1: Fix commit message.
> 
> I don't like this kind of -1's personally, because "Can Merge: No" is
> already shown above. I think the purpose of these reviews are to hide the
> commit from that reviewer's dashboard. It is possible to provide an option
> for these reviewers "hide / fade changesets that can't be merged
> automatically"?

The purpose of these reviews is to alert the author that their change needs rebasing.
Comment 10 Gerrit Notification Bot 2014-04-09 18:09:09 UTC
Change 124891 had a related patch set uploaded by Hashar:
reapply scores on trivial rebases/patchsets

https://gerrit.wikimedia.org/r/124891
Comment 11 Antoine "hashar" Musso (WMF) 2014-04-14 19:07:08 UTC
Christian wrote an email to wikitech-l about this http://lists.wikimedia.org/pipermail/wikitech-l/2014-April/075918.html "Keeping Code-Review votes across trivial changes of	patch sets?"
Comment 12 Gerrit Notification Bot 2014-04-16 10:57:58 UTC
Change 124891 merged by QChris:
reapply scores on trivial rebases/patchsets

https://gerrit.wikimedia.org/r/124891
Comment 13 Antoine "hashar" Musso (WMF) 2014-05-06 15:23:15 UTC
The scores are now reapplied where relevant. That fulfill my original request.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links