[llvm-dev] Git question about revert

[Paul C. Anagnostopoulos via llvm-dev]

This morning I pushed a commit that killed the build, so I reverted it and pushed the new commit to fix the build. Then I did another revert to get my changes back so I can work on them some more.

Is it legitimate to use that second revert commit, which was never pushed, to do the additional work, changing the title to something reasonable? If not, could you explain what I ought to do?

_______________________________________________
LLVM Developers mailing list
llvm...@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

[Thomas Lively via llvm-dev]

Yes, that is a reasonable workflow. Once the changes are made to fix the problems with the original commit, I often see folks land the revert of the revert with a title like `Reland "<original title>"` rather than `Revert "Revert "<original title>""`.

[via llvm-dev]

> -----Original Message-----
> From: llvm-dev <llvm-dev...@lists.llvm.org> On Behalf Of Paul C.
> Anagnostopoulos via llvm-dev

> Sent: Wednesday, January 20, 2021 1:36 PM
> To: llvm...@lists.llvm.org
> Subject: [llvm-dev] Git question about revert
>
> This morning I pushed a commit that killed the build, so I reverted it and
> pushed the new commit to fix the build. Then I did another revert to get
> my changes back so I can work on them some more.
>
> Is it legitimate to use that second revert commit, which was never pushed,
> to do the additional work, changing the title to something reasonable? If
> not, could you explain what I ought to do?

Whatever workflow you do on your own is up to you, really.

Depending on the circumstances, or just my mood, I might do
git reset HEAD~1
which puts HEAD back to the first revert, but leaves the files in your
working tree as they were after the second revert. When you're done,
you can commit them like any other new work. It's like starting over
just before you did that first commit.

Another option is to leave the second revert commit there, and then any
new changes can be added to it using
git commit --amend
This takes the usual "commit" options like -a (all modified files).
Another useful one is "-C HEAD" which leaves the commit message unchanged;
normally --amend will pop you into vim (or whatever editor you have set)
to modify the original commit message. Editing is handy because reviewers
like to see something in the new commit message that says how you fixed
the problem that the first commit caused.

One issue with revert-of-revert is that "git revert" prepends that
"Revert foo" text onto the headline. Do that too many times and the
entire commit message starts to look like
Revert "Revert "Revert "Revert "Revert "Revert ...
and I believe I have actually seen one with 6 Reverts, which made the
llvm-commits email subject line pretty useless. While it might be
amusing the first time you see it, I don't like it because the *real*
commit message is obscured.

What I suggest to people is that if they revert a revert, they should
modify the commit message from
Revert "Revert "my clever patch""
to
Reapply "my clever patch"
(some people prefer "Reland") or just go back to the original
my clever patch
which better describes what the commit is actually about.

--paulr

[Mehdi AMINI via llvm-dev]

On Wed, Jan 20, 2021 at 10:39 AM Paul C. Anagnostopoulos via llvm-dev <llvm...@lists.llvm.org> wrote:

This morning I pushed a commit that killed the build, so I reverted it and pushed the new commit to fix the build. Then I did another revert to get my changes back so I can work on them some more.

Is it legitimate to use that second revert commit, which was never pushed, to do the additional work, changing the title to something reasonable? If not, could you explain what I ought to do?

That works, in general I tend to `git cherry-pick <original commit>` instead, because it brings back the original commit description with it :) (and possible link to the phabricator revision).

I'll still edit the message (git commit --amend) to add that this is a reland after fixes.

-- 

Mehdi

[James Henderson via llvm-dev]

One other thing I find useful when relanding a patch after fixing it is to include in the new commit message the reason for the breakage/how the new patch fixes it.

James

[David Blaikie via llvm-dev]

Definite +1 to this - please include the full history of the patches commit/reverts, and reasons for them. (eg: "originally committed as <hash> reverted due to <buildbot info (link and quote ideally, the logs get cleaned up so the link won't be around forever but the commit message/quoted errors/etc will be)> recommitted as <hash> with <describe the fix/pointing to particular source files/changes>, etc... " - I usually do that in list form:
<hash> originally committed
<hash> reverted due to... 
<hash> recommitted with fix...
...
)

[Fangrui Song via llvm-dev]

On 2021-01-21, David Blaikie via llvm-dev wrote:
>Definite +1 to this - please include the full history of the patches
>commit/reverts, and reasons for them. (eg: "originally committed as <hash>
>reverted due to <buildbot info (link and quote ideally, the logs get
>cleaned up so the link won't be around forever but the commit
>message/quoted errors/etc will be)> recommitted as <hash> with <describe
>the fix/pointing to particular source files/changes>, etc... " - I usually
>do that in list form:
><hash> originally committed
><hash> reverted due to...
><hash> recommitted with fix...
>...
>)

Some new contributors tend to make reverts without a justification.

https://llvm.org/docs/DeveloperPolicy.html#commit-messages actually has
a somewhat related sentence but it is probably buried in a long document.

"If the commit is a bug fix on top of another recently committed patch,
or a revert or reapply of a patch, include the git commit hash of the
prior related commit. This could be as simple as “Revert commit NNNN
because it caused PR#”."

There are some other undocumented good practices, e.g.

* If the patch may take some time to reland or miss something more than
simple test tweaks, consider reopening the differential and (if
requires further review) requesting for changes.
* `git cherry-pick`. Make sure `Differential Revision: ` is in the
reland commit so that it is connected to the original differential.

[Paul C. Anagnostopoulos via llvm-dev]

I haven't found a "reopen differential." What's the right way to do that?

At 1/25/2021 05:23 PM, Fangrui Song wrote:

>* If the patch may take some time to reland or miss something more than
> simple test tweaks, consider reopening the differential and (if
> requires further review) requesting for changes.

_______________________________________________

[James Henderson via llvm-dev]

If you go to a closed Phabricator review, the "Add Action" drop-down menu above where you write comments (where you can change reviewers/approve the patch etc), has a "Reopen Revision" action. Just select that and click submit to reopen (though I think Phabricator recently learnt to recognise pure reverts, so it may no longer be necessary, I'm not sure).

[Paul C. Anagnostopoulos via llvm-dev]

At 1/26/2021 03:15 AM, James Henderson wrote:
>If you go to a closed Phabricator review, the "Add Action" drop-down menu above where you write comments (where you can change reviewers/approve the patch etc), has a "Reopen Revision" action. Just select that and click submit to reopen (though I think Phabricator recently learnt to recognise pure reverts, so it may no longer be necessary, I'm not sure).

Why did I not notice that before? Weird. Thanks!