My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 1703: reopen a changeset after commit revert
18 people starred this issue and may be notified of changes. Back to list
Status:  Accepted
Owner:  ----


Sign in to add a comment
 
Reported by brianjmu...@gmail.com, Dec 5, 2012
Affected Version: 2.1.8 (and beyond?)

What steps will reproduce the problem?
1. submit a changeset to gerrit
2. merge changeset
3. revert changeset
4. try to push a new revision of the changeset using the same change-id

What is the expected outcome?

Gerrit accepts the push and and reopens the changeset

What do you see instead?
 ! [remote rejected] HEAD -> refs/for/master (change 4437 closed)

Please provide any additional information below.

After a previously merged changeset is reverted one really does want to be able to push new revisions to the existing (re-opened) changeset so that the context of the previous revisions is preserved.

For example reviewers could see what changes have been made in the new revision (being proposed in response to the reverted commit) being able to compare it directly with the previous revision(s) including the one that was reverted.
Jan 3, 2013
Project Member #1 edwin.ke...@gmail.com
Fix by [1]. We now generate a new Change-Id for the revert commit. Using this Change-Id new patch sets for the revert change can be uploaded.

[1] https://gerrit-review.googlesource.com/36614
Status: Released
Labels: FixedIn-2.5
Jan 3, 2013
#2 brianjmu...@gmail.com
Ultimately though, can new (i.e. post-revert) patches be compared to the patch that was reverted?

The use-case is:

1. iterate over pushing patches and getting reviews until reviewers are happy with patch
2. submit the patch
3. patch has been found to be defective, so revert it
4. push new patch(es in another patch/review cycle) to the same changeset as was used with steps 1 and 2 so that the new patch(es) can be compared against the patch that was submitted in step #2 (and of course all patches that were submitted in step 1)

The important part here is the ability to use the history feature of gerrit to compare the new patches to the ones pre-revert.
Jan 3, 2013
#3 brianjmu...@gmail.com
To be clear, the reason for my question in comment #2 is that the explanation in comment #1 seems to indicate (to me at least) that when the change is reverted, it gets a new change-id and it's against this new change-id that one submits a new patch to do what the patch that was reverted was meant to do.

That sounds to me like I end up doing the work to fix the patch that was reverted in a new changeset, thereby losing the ability to use the reverted changeset as history against which my new changeset can be diffed.
Jan 3, 2013
#4 sop@google.com
brianjmurrell has an excellent point. When a revert happens we really should let you re-open the original change and use the original change as a place to attempt the retry.

I'm not sure if the revert should be done inside of the original change as a patch set, that might be a little confusing to look at. We may need to specially mark those patch sets with a new column in the patch_sets table so its very clear that e.g. ps4 was a revert of ps3 which had been previously submitted and thus ps4 is a reverse diff of ps3, and a diff of ps3-ps4 should be "empty" (or at least only show the impact of the difference in parents).
Status: Accepted
Jan 3, 2013
#5 brianjmu...@gmail.com
I'm fairly ambivalent about which changeset the revert goes in.  Even if it's in the same changeset as the submitted patch (which would be the same changset that the retry-work is done), one can easily choose to diff between the submitted revision and the "retry" revision, skipping over the reversion revision.
Jan 4, 2013
Project Member #6 edwin.ke...@gmail.com
I agree on the use case. This is indeed a very good idea.

However I would be a bit careful about reopening a change and adding further patch sets to it after it was merged. Until now there are never dependencies between patch sets of the same change and we have at most one merged commit per change. I could imagine that some code is making assumptions about this although I can't name any concrete place. Couldn't we achieve the same by having seperate changes and then making some smart UI which shows them together and allows the comparison between the patch sets across the changes? It's not a strict opinion but just my initial thoughts about this...

Jan 4, 2013
Project Member #7 edwin.ke...@gmail.com
 Issue 725  has been merged into this issue.
Sign in to add a comment

Powered by Google Project Hosting