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?

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

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

That works, in general I tend to git cherry-pick <original commit> instead, because it brings back the original commit description with it :slight_smile: (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.

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

Definite +1 to this - please include the full history of the patches commit/reverts, and reasons for them. (eg: "originally committed as 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 with <describe the fix/pointing to particular source files/changes>, etc… " - I usually do that in list form:
originally committed
reverted due to…
recommitted with fix…

)

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.

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

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!