1476425 - Running `arc diff` to update a revision does not update an amended commit message

Status: NEW

(Conduit :: Phabricator, defect, P3)

Description

[Andrew Halberstadt [:ahal]]

STR:
1. hg commit -m "Original message"
2. arc diff
3. hg commit --amend -m "Modified message"
4. arc diff

Expected:
After landing with Lando, the commit message is "Modified message"

Actual:
The commit message is "Original message"


The summary still shows the old message, though in the "Commits" tab the new message is displayed:
https://phabricator.services.mozilla.com/D2136

But here is what landed via Lando:
https://hg.mozilla.org/integration/autoland/rev/e6291f882109

Hopefully there is a flag or something we can use to get around this (I think this behaviour needs to be disabled by default)

Comment 2

[:glob ✱]

after much investigation, arc does support this.  kinda of.

by default arc won't update the title or summary fields off the commit description.
if you pass the --verbatim flag it will, but only if the commit description is formatted as per arc's template.


to work around the actual commit description needing to be arc-formatted (for example in a script that wraps arc), you can specify --message-file and arc will read the fields from that file instead of the commit description.

unfortunately you cannot provide both --verbatim and --message-file; these options are mutually exclusive.


i'll raise an issue upstream to see if they can relax these restrictions.

Comment 3

[:glob ✱]

from phacility:

> As the next major arc iteration looms on the horizon (T13098[1]), I'd tentatively like to
> look toward replacing this flag with one that accepts a list of transaction commands
> instead (see T11934[2]). One small wrench in the gears there is that summaries frequently
> span multiple lines and we'd need some way to escape them, but that seems surmountable.

> Let me see if I can get some traction on T13098 / T11934 soon. If I don't manage to
> get them into the queue relatively soon we can muck around with the --message-file
> semantics since the current behavior is unambiguously bad

[1] https://secure.phabricator.com/T13098
[2] https://secure.phabricator.com/T11934

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Before the upstream gets to it, can we probably do this in the review wrapper? It feels rather unfortunate that the commit message and title / summary can become mismatch.

Comment 5

[:glob ✱]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Before the upstream gets to it, can we probably do this in the review
> wrapper? It feels rather unfortunate that the commit message and title /
> summary can become mismatch.

that was my original intent, however the "unambiguously bad behaviour" in arc doesn't allow us to work around it.

Comment 6

[Karl Tomlinson (:karlt)]

What is the best way to work around this for now?

Can the commit be updated in a way that arc won't treat it as derived from the previous version?
Is that just a matter of removing the id from the commit message?

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> Can the commit be updated in a way that arc won't treat it as derived from
> the previous version?
> Is that just a matter of removing the id from the commit message?

If a commit is added or removed from the series, will that change be visible in the "Stack" tab, or is bugzilla the only record linking the histories?

Comment 9

[:glob ✱]

(In reply to Karl Tomlinson (:karlt) from comment #6)
> What is the best way to work around this for now?

edit it in the UI.

(In reply to Karl Tomlinson (:karlt) from comment #7)
> If a commit is added or removed from the series, will that change be visible
> in the "Stack" tab, or is bugzilla the only record linking the histories?

bugzilla is only used to provide authentication for phabricator; no other features rely on it.

Comment 10

[Karl Tomlinson (:karlt)]

Similar problems with arc patch:

  arc patch --skip-dependencies --nobranch --diff 8101

makes a commit with summary "Bug 1035774 - Rename WidgetUtilsGtk.{cpp,h} to WidgetUtilsGTK.{cpp,h}" because the revision summary has been modified, but that is not the summary for diff 8101.

https://phabricator.services.mozilla.com/D3297?id=8101 shows the correct message in the commits tab.

I wonder whether the commit hashes may be useful, but only base commit hashes are hyperlinked
and it seems that no-one watching #phabricator knows how to use the hashes.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

It's rather weird that, even if I manually update the title, updating revision restore the title to the old one... why's that?

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

You can see an example here: https://phabricator.services.mozilla.com/D4186#90626

I manually removed the "part 2" from the title, but when I use moz-phab to submit update, the old title came back. The updated commit actually also has the new title... It's more surprising and harmful than not updating at all...

Comment 13

[Mark Côté [:mcote]]

We checked with upstream recently on this.  They're working on some major changes to Arcanist right now (https://secure.phabricator.com/T13098) that are apparently prereqs to this.  We'll be watching this closely.

We're going to fix this in moz-phab in a different way.  See bug 1494372.

I'm going to post to dev.platform and firefox-dev soon warning about this behaviour, since I agree it's a problem.