Page MenuHomePhabricator

report trivial rebases in Gerrit
Closed, ResolvedPublic

Description

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.


Version: unspecified
Severity: enhancement

Details

Reference
bz41074

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 12:51 AM
bzimport added a project: Gerrit.
bzimport set Reference to bz41074.
bzimport added a subscriber: Unknown Object (MLST).

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)

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 ?

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/

Apparently we can now configure labels to recopy with: copyAllScoresOnTrivialRebase and copyAllScoresIfNoCodeChange

See https://gerrit.wikimedia.org/r/Documentation/config-labels.html#label_copyAllScoresOnTrivialRebase

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).

(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"?

btw. Can inline comments get reapplied automatically as well?

(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.

(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.

Change 124891 had a related patch set uploaded by Hashar:
reapply scores on trivial rebases/patchsets

https://gerrit.wikimedia.org/r/124891

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?"

Change 124891 merged by QChris:
reapply scores on trivial rebases/patchsets

https://gerrit.wikimedia.org/r/124891

The scores are now reapplied where relevant. That fulfill my original request.