Page MenuHomePhabricator

Generate default "Depends on" line in commit message when multiple diffs are stacked
Closed, ResolvedPublic

Description

Right now if I do a series of diffs each of which builds on the previous one, I have to manually add a "Depends on" line to all but the first of them. It'd be convenient if arc diff would look at the branch history and see that there are open diffs, and prepopulate their IDs in a "Depends on" line in the default commit message. Then I could edit that as needed.

I'd rather have a list of all of the previous diffs in the same branch by default (as opposed to just the most recent one) since it's easier to cull a list that has too many values than it is to manually look up and add missing IDs.

Event Timeline

sgrimm created this task.Jul 18 2016, 10:26 PM

Obviously I can implement this myself locally, but if it's a lousy idea for whatever reason, I'm hoping filing this request will cause someone to warn me off it.

This is desirable, but complicated in the general case. T3875 (and T3875#182020 in particular) has some discussion of a similar identification problem at the other end of the workflow, in arc land.

There are fewer weird cases at arc diff time than at arc land time, but I suspect the best pathway forward here involves formalizing a way to ask "which revision does <some arbitrary commit> correspond to?" with the intent of making it more flexible later.

There is some code which does this already, but it's not well isolated. In particular:

  • ArcanistFeatureWorkflow->loadRevisions() (which powers identification for arc branch) uses commit messages, Git commit hashes, and Git tree hashes only, and isn't easy to invoke from elsewhere.
  • The various loadWorkingCopyDifferentialRevisions() methods on ArcanistRepositoryAPI subclasses use a larger set of signals (for example, directory paths in Subversion and commit hashes in Mercurial) but can not accept an arbitrary list of commits to identify.

These workflows, arc diff (here) and arc land (in T3875) should all really be using the same code.

From a planning perspective this all runs into the impending mess of T5055 / T10329 too, since it likely involves a fair bit of cleanup and refactoring across multiple workflow and once I'm doing that anyway I might as well go all the way.

I also don't see an especially compelling way to just sweep this under a rug for now. We could add some "do the lookup" function and implement only the pieces you need for now (presumably, commit messages or Git commit or tree hashes) but the API should really take a significant amount of information (local paths, branch names) and emit a significant amount of information (including explanations of why revisions match).

Also be careful that the 'Depends on:' commit message parsing is strictly additive; if you ever change patch order in any way, your dependencies will get out of whack. If you're doing this, you can modify DifferentialTransactionEditor to reset the dependencies before parsing, but this means you throw away any dependencies manually added through the web UI.

lvital added a subscriber: lvital.May 18 2017, 9:33 PM

D18651 is marked as fixing this. The "fix" is very rough and will land in the experimental branch, see T11911 (and perhaps T12996) for followups.

Is the experimental fix git-only or can we test with mercurial as well?

It should also work with Mercurial (D18652 and D18653 were my Mercurial tests).