Page MenuHomePhabricator

Some improvements to Mercurial, Arcanist, and Dependent Revisions
Open, Needs TriagePublic

Description

We're starting to use dependent revisions more with Differential (hg evolve is lovely and makes amending to non-head commits a cinch) - when moving between machines while working on revisions (or even having a peer patch down changes to test) a few update would make it more easily to patch down a string of revisions, make updates, diff back up, then switch back to another machine.

It's relevant to note that I use arc:bookmark as the base revision rule. I don't believe these features are dependent on this configuration.

See T12841 for an existing scenario where two revisions dependent revisions were created. I created two revisions D18122 (parent) and D18123 (child) and made the parent a dependency for the child.

  1. When patching a child revision, have bookmarks applied to other revisions that are patched in the process. So in the linked scenario also adding a bookmark arcpatch-D18122 to commit 22:c5421705ed9e since it was patched in the process of patching D18123.
  2. Use the last associated bookmark used by the revision instead of arcpatch-Dnnn if available. It looks like it's being tracked by the revision - you can see on D18122 that the bookmark I used was test. I'm guessing that one of the reasons arcpatch-Dnnn is used as it is unlikely collide with existing bookmarks, though there does appear to be fallback support for appending -1 if there is a collision.
  3. An option to not apply a bookmark when patching a revision. A few of my peers assume bookmarks are evil and prefer to type in numbers or something. They've mentioned that they end up with loads of arcpatch-Dnnn bookmarks that end up cluttering their repository. I'm pretty content with using bookmarks so I will have to investigate if further details are needed about these workflows.
    • I believe in most of these cases no configuration for a base revision rule is set - I think they are passing mercurial changeset IDs to arc land or when specifying
  4. Automatically detecting dependency when creating a new revision with arc diff (I think this is T11343?)

Event Timeline

Some of the (3), (2), (1) stuff is that we're trying to pick a single behavior which addresses most use cases reasonably well. For example, if we use "natural" bookmark names that will tend to make things much worse for users in bucket (3).

T3277 (arc cleanup, to delete all the patch branches/bookmarks) is another approach here, and that "can't" work if we use "natural" names.

I'm not hugely sympathetic to this class of use case because "match patterns in a block of text, then run commands using the matches" seems like a problem that full-time professional software engineers should have the skillset required to overcome handily, and that this should be a problem which takes less time to fix yourself than to complain to someone about.

I think we're likely to start retaining some sort of .arcmetadata about working copies (some discussion in T3875) and begin recording invisible information about branches and bookmarks. On one level I dislike this -- currently, the behavior of arc is mostly just a function of the working copy's state, not "working copy" + "invisible external magic you can't see". However, I think the balance of feedback is that users would like more magic here.

If we magically new that branch apple was created by arc patch but branch banana was not, we could selectively clean up apple during some kind of arc cleanup operation, which would free us up to use "natural" branch names.

But I think any path forward here needs to wait for the next arc iteration.

The other general force here is that I'm very resistant to adding options, and especially resistant to adding config file options. Perhaps this is something we should look at embracing more readily in the next iteration.

And auto-dependencies are unambiguously desirable and covered by T11343, yeah.

For (1), it is expected to not apply bookmarks to dependency changesets that are applied as part of the patch issued? When patched one-by-one they would each obtain their bookmark.

For (2), I don't see the leap in that using "natural" names requires metadata tracking - does existing arcanist features currently rely on identifying arcpatch- bookmarks?

I'm not hugely sympathetic to this class of use case because "match patterns in a block of text, then run commands using the matches" seems like a problem that full-time professional software engineers should have the skillset required to overcome handily, and that this should be a problem which takes less time to fix yourself than to complain to someone about.

To be fair there were scripts being used before I was told about the issue; I had walked into a conversation where one developer was discussing with another that they end up with excess bookmarks and the other was telling them were to get a script they made. I tried to interrogate to understand what they were doing but I couldn't quite follow what their workflow was that they ended up with all these bookmarks. They mentioned previously trying to have arcanist work with Mercurial's mqueue -- it sounded like they are working with multiple (dependent?) commits which do not overlap (no merge conflicts) and in order to switch between them they do lots of diffing and patching. I need to find out more about this though.

For (1), it's currently expected, yes. We don't create branches for dependencies in Git either, currently. I don't think this is terribly unreasonable, but I'm also not sure it's terribly useful (does it just help you keep track of which commits are part of the leaf?) and it makes cleanup more difficult by creating more total branch/bookmark artifacts in the local working copy.

For (2), we don't currently rely on the naming, but a theoretical arc cleanup would, and any scripting users currently use almost certainly does. If we move away from this, cleanup becomes essentially impossible without metadata tracking. It also makes cleanup more important, since all the garbage no longer collects itself in a big block of "arcpatch-..." stuff. So if we started using natural names without introducing metadata, we'd be making the existing cleanup problem worse.

None of this is a major concern -- I think if we introduce metadata these problems reduce to just problems of how many options/settings/preferences we want to have.

jcox added a subscriber: jcox.Jun 30 2017, 5:08 PM

I missed your question in (1). A typical scenario for this would be when working on a feature which requires some refactoring work to be done. A revision is created with just the refactoring work and then a dependent revision is made which includes the actual feature work. During feature work additional refactoring might be made and need to update to that changeset and either add/amend changes, rebase the dependent revision back on top. Having the bookmarks auto created would help especially when using arc:bookmark when updating revisions back to phab.

leoluk added a subscriber: leoluk.Jun 30 2018, 1:48 PM