The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/9d6710a8e7b48b51167b2df14b9c4ec533fb76fb

commit 9d6710a8e7b48b51167b2df14b9c4ec533fb76fb
Author: Aviv Keshet <akeshet@chromium.org>
Date: Wed Dec 20 20:53:51 2017

patch: make _AmendCommitMessage safe to use when patch not checked out

BUG=chromium:792781
TEST=New unittest added; verified that test breaks prior to feature.

Change-Id: I50198890d0cf8fca04174307cc389a221fd4b257

[modify] https://crrev.com/9d6710a8e7b48b51167b2df14b9c4ec533fb76fb/lib/patch_unittest.py
[modify] https://crrev.com/9d6710a8e7b48b51167b2df14b9c4ec533fb76fb/lib/patch.py

Possible fallout from above -- changes not being correctly annotated in their commit message anymore?

Example change is https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/789809 which was picked up by CQ run https://luci-milo.appspot.com/buildbot/chromeos/master-paladin/17338 but is not commit-message annotated.

revert at https://chromium-review.googlesource.com/c/chromiumos/chromite/+/846726

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/2b8c0fe5954c5b6525863c21639f3f9e53c90bf8

commit 2b8c0fe5954c5b6525863c21639f3f9e53c90bf8
Author: Aviv Keshet <akeshet@chromium.org>
Date: Tue Jan 02 20:04:37 2018

Revert "patch: make _AmendCommitMessage safe to use when patch not checked out"

This reverts commit 9d6710a8e7b48b51167b2df14b9c4ec533fb76fb.

Reason for revert: Seems to break commit message annotation for
non-merges CLs.

Original change's description:
> patch: make _AmendCommitMessage safe to use when patch not checked out
> 
> BUG=chromium:792781
> TEST=New unittest added; verified that test breaks prior to feature.
> 
> Change-Id: I50198890d0cf8fca04174307cc389a221fd4b257

Bug: chromium:792781
Change-Id: I0d52f68420a1807ffa1900509311634217199e18
Reviewed-on: https://chromium-review.googlesource.com/846726
Tested-by: Aviv Keshet <akeshet@chromium.org>
Trybot-Ready: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: David James <davidjames@chromium.org>

[modify] https://crrev.com/2b8c0fe5954c5b6525863c21639f3f9e53c90bf8/lib/patch_unittest.py
[modify] https://crrev.com/2b8c0fe5954c5b6525863c21639f3f9e53c90bf8/lib/patch.py

Issue 760205 has been merged into this issue.

This had complications. Not actively working on this now.

 Issue 805440  has been merged into this issue.

Not working on this at the moment.

Aviv, can you give a better description for this bug? I don't understand what it's about. https://bugs.chromium.org/p/chromium/issues/detail?id=760205 redirects here, that's why I'm asking. A new report on https://bugs.chromium.org/p/chromium/issues/detail?id=811188#c1 .

Normal commits that land via the CQ get their commit message decorated with the identity of reviewers. See e.g. https://chromium-review.googlesource.com/c/chromiumos/chromite/+/1058827 which has commit message text about Reviewed-on, Reviewed-by, etc.

This is possible because the CQ actually re-cherry-picks the change before it is submitted, and amends the commit message when doing so (and this is a reimplementation of gerrit's behavior when submitting via cherry-pick).


However, for merge commits today that are landed by the CQ, this is not the case. See e.g. https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/832372

This is because the CQ does not re-cherry-pick merge commits today. I attempted to add this behavior back in #3, but then there was some fallout described in #4 that seemed relevant so I reverted. In hindsight that fallout may have actually been due to https://b.corp.google.com/issues/68258327 and my change may have been innocent but I didn't have a chance to go back and investigate.

I see; yes, we need to do this. Seems like a quick one if we can just reland your change.

Mike, can you see if just relanding this change is enough? If not, I'll find another owner at a later time.

I think the change in #3 (https://chromium-review.googlesource.com/c/chromiumos/chromite/+/830533) will not be sufficient. What that change does it make the _AmendCommitMessage do the correct thing in a case it didn't previously handle correctly. The follow up will then be to add a _AmendCommitMessage call to lib/patch.py:Merge (in analogy to the call that already exists in lib/patch.py:CherryPick).

In fact I left a todo comment there:

  # TODO(akeshet): Amend the original merge's commit message before merging
  # it.