Page MenuHomePhabricator
Create Task
Maniphest T8709

arc diff: option to just add Differential ref to commit message (middle ground between mutable and immutable workflows)
Closed, WontfixPublic
Actions

Description

There seem to be two ways you can use arc diff with git at the moment:

  • treat history as entirely immutable: there are no arc droppings in the commit history at all, but if you rebase a branch and resubmit, arc diff can't associate the new commit with the right Differential diff Dxyz
  • treat history as entirely mutable: each commit message is rewritten into a very arc-ish format ("Summary:" prepended to the long commit message, "Test Plan" and "Reviewers" most likely also added, etc.) but after a rebase/resubmit, arc does know which Differential diff it is dealing with

I would prefer a middle ground in which arc amends the commit message with the minimum of information it needs to relate the change to its Differential diff - I think that means just the Differential Revision and nothing else, the same way Gerrit adds the Change-Id and nothing else. Perhaps { "history.immutable": "almost" } could be the configuration option for this? :-)

Relatedly, it would be nice if arc could bundle one-line metadata like the Differential Revision into a "pseudo-footer" with other one-line git metadata such as the Signed-off-by, Reviewed-by, etc. that are common in projects borrowing their coding standard from the Linux kernel, or Bug, Applied-upstream etc. in http://dep.debian.net/deps/dep3/, like so:

Fix that thing with the stuff

This thing is wrong and should be fixed, because otherwise badness.
In particular, Alice reported that this caused other stuff to go wrong.

Signed-off-by: Bob <bob@example.com>
Bug: http://phabricator.example.com/T123
Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+bug/1
Differential Review: http://phabricator.example.com/D321

(At the moment arc always adds extra blank lines.)

Related Objects

Event Timeline

smcv created this task.Jun 29 2015, 6:50 PM
smcv raised the priority of this task from to Needs Triage.
smcv updated the task description. (Show Details)
smcv added a project: Arcanist.
smcv moved this task to Backlog on the Arcanist board.
smcv added a subscriber: smcv.
epriestley closed this task as Wontfix.Jun 29 2015, 7:36 PM
epriestley claimed this task.
epriestley added a subscriber: epriestley.

See T8227.

pwithnall added a subscriber: pwithnall.Jul 1 2015, 3:08 PM

I second this request, although if it could be fixed in a way which avoids adding another configuration option, that would be great (I agree with T8227).

The problem is that in immutable mode, arc loses a lot of its power and requires the developer to do a lot of manual work to update Differential diffs, which is rubbish.

But in mutable mode, arc requires the entire project to conform to its workflow, which is sometimes not appropriate. For example, the project might handle testing differently, and not want the ‘Test Plan’ section. Requiring the developer to manually delete that from the commit message each time round is not fun.

Effectively, I think the problem is that arc is trying to enforce policy on developers, when it should just be providing mechanism. If there’s a way to address that without adding more configuration options, that would be great. This might be as simple as changing the mutable workflow to only append the Differential revision URI to the commit message, as @smcv suggests. This is what most other tools I have used do.

epriestley added a comment.Jul 1 2015, 3:10 PM

For example, the project might handle testing differently, and not want the ‘Test Plan’ section.

You can disable the "Test Plan" field by configuring differential.fields. This will stop it from appearing in templates.

smcv added a comment.Jul 1 2015, 3:17 PM

Perhaps a better example of "stuff added by arc" than the Test Plan is that the commit message is rewritten to include the initial lists of reviewers and subscribers, which I don't think are really commit metadata in the same way the Differential Revision is, and do not necessarily reflect reality by the time it actually gets committed.

epriestley added a comment.Jul 1 2015, 3:23 PM

do not necessarily reflect reality by the time it actually gets committed.

In theory, arc land updates them so they do accurately reflect the actual state of the world at commit time.

pwithnall added a comment.Jul 1 2015, 5:34 PM

Another example would be the ‘Summary:’ heading — git commit message formatting already defines what the summary and body of the commit message are.

To be a bit more blunt, I think what would be nice is if arc could produce commit messages which look like what @smcv suggested as ‘Fix that thing with the stuff’ in the task description. I don’t want to change my project’s established conventions and clutter my git logs because arc wants to scatter droppings all over my commit messages.

If that’s possible already with settings like differential.fields, great — can it please be documented?

If not, what changes would you be happy with to make it so? Or, if you really don’t like that commit message format, what’s your reasoning? Thanks.

epriestley added a comment.Jul 1 2015, 5:36 PM

I don’t want to change my project’s established conventions and clutter my git logs because arc wants to scatter droppings all over my commit messages.

It sounds like Phabricator isn't a very good fit for your project. You can find some alternatives on Wikipedia.

pwithnall added a comment.Jul 1 2015, 5:43 PM
In T8709#124109, @epriestley wrote:

I don’t want to change my project’s established conventions and clutter my git logs because arc wants to scatter droppings all over my commit messages.

It sounds like Phabricator isn't a very good fit for your project. You can find some alternatives on Wikipedia.

Shame, because apart from arc’s minor inconvenience here, it’s otherwise a very good fit. Since our workflow is the same one used by a lot of projects in the GNOME and freedesktop world, I suspect you may get more bug reports like this one in future. :-(

Phabricator · Built by Phacility