Page MenuHomePhabricator

Updating commit message in Gerrit resulted in new patchset with older patchset code
Closed, ResolvedPublic

Description

What happened:
During code review of https://gerrit.wikimedia.org/r/#/c/97871/19/includes/specials/SpecialMobileDiff.php I noticed an error (line 208 of patchset 19 should concatenate, not assign). After pointing this out to MaxSem, he fixed the problem with patchset 20 (https://gerrit.wikimedia.org/r/#/c/97871/19..20/includes/specials/SpecialMobileDiff.php). Then, I made some minor changes to the commit message of patchset 20 in the Gerrit interface:
https://gerrit.wikimedia.org/r/#/c/97871/20..21//COMMIT_MSG

However, after making the changes to the commit message, the change that MaxSem made in patchset 20 disappeared from patchset 21:
https://gerrit.wikimedia.org/r/#/c/97871/20..21/includes/specials/SpecialMobileDiff.php

It didn't occur to me retest patchset 21 since I had just updated the commit message, so I merged patchset 21 and wound up breaking mobile diffs in production :(

Expected behavior:
Gerrit would have kept everything from patchset 20 with the exception of the new commit message.


Version: unspecified
Severity: major

Details

Reference
bz59892

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 2:34 AM
bzimport added a project: Gerrit.
bzimport set Reference to bz59892.
bzimport added a subscriber: Unknown Object (MLST).

What you're seeing could be explained by the following time line:

  • patch set 19 gets uploaded
  • Person A opens the change's page
  • Person B uploads patch set 20
  • Person A clicks “Edit Commit Message” (still being on patch set 19, as patch set 20 did not yet show up)
  • Person A clicks “Submit” in the “Create New Patch Set” dialog.

To me, gerrit's behaviour does not feel wrong.

Your expected behaviour would make others complain that they clicked
“Edit Commit Message” for patch set 19, and not patch set 20 and that
gerrit should not second guess them by automatically switching to
patch set 20 behing the users back.

Regardless of which of the two approaches gerrit would take, gerrit
would disappoint some people :-/

Even refusing to change the commit message would not resolve the
problem, as we run into the same problem if you change the commit
message locally. For example by

  • patch set 19 gets uploaded
  • Person A clones latest patch set (i.e.: 19)
  • Person B uploads patch set 20
  • Person A amends the commit message of the most up-to-date patch set of the local checkout (i.e.: 19)
  • Person A submits the amended commit for review (Thereby creating patch set 21 which does not incorporate the changes of patch set 20).

(In reply to comment #1)

What you're seeing could be explained by the following time line:

  • patch set 19 gets uploaded
  • Person A opens the change's page
  • Person B uploads patch set 20
  • Person A clicks “Edit Commit Message” (still being on patch set 19, as patch set 20 did not yet show up)
  • Person A clicks “Submit” in the “Create New Patch Set” dialog.

I think this analysis is correct. I have seen this issue before.

To me, gerrit's behaviour does not feel wrong.

It's a failure to detect an edit conflict, resulting in silent data loss. That's horribly wrong.

Your expected behaviour would make others complain that they clicked
“Edit Commit Message” for patch set 19, and not patch set 20 and that
gerrit should not second guess them by automatically switching to
patch set 20 behing the users back.

It should do what mediawiki and git both do silently to resolve edit conflicts: it should just splice your change into the new revision if there are no conflicts.

Well most importantly it should not be silent of course. Sending the timestamp of when the user opened the edit, when submitting the change can easily tackle that part.

(In reply to comment #2)

it should just splice your change into the new revision if there are
no conflicts.

If different patch sets of a change would sit on top of each other or
had to share a similar relation, then that might make sense.

However, different patch sets of a change are fully unrelated.
They can for example come with different parents.
So automatically, silently merging in patch sets would bring in the
other parents :-(

Two patch sets of the same change need not even be connected in the
graph of commits (e.g.: when force pushing a ref to a graph with a
different, completele unconnected root).
Automatically, merging in other patch sets would connect the different
graphs :-(

And for the issue of the bug's original description ... even if we'd
automagically merge in patch sets, assume in addition to MaxSem's good
patch set 20, I also uploaded a faulty patch set 20.5 after 20. If
we'd automatically merge things, patch set 19, the good patch set 20,
but also my faulty patch set 20.5 would get merged into patch set 21,
and it would have gone live without review and have broken the site.

When editing the commit message through the web UI on a page that
shows patch set 19 as most recent patch set, the resulting patch set
has the code of patch set 19 with the new commit message.

When editing the commit message through the web UI on a page that
shows patch set 20 as most recent patch set, the resulting patch set
has the code of patch set 20 with the new commit message.

Silently merging in other unreviewed parts of newer patch sets in
behind the reviewer's back would make reviews only harder.

  • This bug has been marked as a duplicate of bug 52292 ***