Page MenuHomePhabricator

Triangle workflows (pushing to a merge manager)
Closed, WontfixPublic

Description

We currently have a workflow where code is never landed to origin/master directly; instead, an API call is made which returns a remote and branch name to push to, and the proposed code to merge is pushed there. The remote host ensures that tests pass, then merges and pushes the code to origin/master. Subsequent fetches from the client then pick up the newly-added change.

This is currently implemented as a custom workflow, which duplicates a fair amount of code of ArcanistLandWorkflow; as that is marked final, and its core properties are mostly marked private, there is currently not clean way to re-use its code.

The newly-added ArcanistLandEngine abstraction is not marked final, and provides nearly enough rope to implement this cleanly. By subclassing ArcanistGitLandEngine, and overriding pushChange to push the change to an alternate remote, the desired behavior can be implemented in a minimally invasive way. This seems preferable to the bigger change of allowing the the value of TargetRemote to be a URL, instead of a remote name -- in this workflow, the remote that is pushed to is never fetched from, only pushed to, so it is never explicitly added by name. Changing ArcanistGitLandEngine to support URL-only remotes seems like a larger undertaking -- and in some ways, the remote really is origin, just not for the push action.

Git itself supports this as a concept, via the remote.pushdefault configuration; though that itself doesn't provide quite enough flexibility, as the branch name to push into needs to be be provided by an API call.

The difficulty of implementing this cleanly is that the underlying ArcanistLandEngine of arc land is not configurable, and some key behavior is locked away un the un-inspectable and un-subclassable arc land. Do you have suggestions for how to implement this workflow cleanly?

Event Timeline

alexmv created this task.Oct 30 2015, 9:33 PM
alexmv updated the task description. (Show Details)
alexmv added a project: Restricted Project.
alexmv added a subscriber: alexmv.
avivey added a subscriber: avivey.Oct 30 2015, 9:43 PM

You can pull the change directly by revision/diff id, using techniques documented at https://secure.phabricator.com/book/phabricator/article/harbormaster/#change-handoff, and have the Merge Manager in-charge of the merge, instead of the diff author; Your API call will instead initiate the merge instead of suggest a magic location for the push.

You could then add a "Send this Revision to the Merge Master" button on the revision page, which will make the API call.

Hm, interesting. I see a few problems with that technique, though:

  1. It requires that an arc diff have always been run to capture the latest version of the changes. I foresee a not-uncommon case of developers amending one last change in, then attempting a land — but having all changes since their last `diff silently dropped.
  2. It does no local branch management. Current arc land is quite useful in removing branches once they have landed.
  3. It doesn't do a local rebase first. The local rebase not uncommonly catches conflicts — which are impossible for the merge manager to handle cleanly.

Most developers are used to landing their code from the command line, so the button isn't a clear win. By the time one implements an arc workflow that deals with fetching the remote to ensure it is up-to-date, rebasing appropriately, calling arc diff, making the API call, and removing the local branch, one has re-implemented essentially all of arc land.

Can the remote be a magica-virtual ref (Like gerrit's /refs/for/branch), so the API call complexity is removed?

How does removing the API call simplify matters? One still needs to have code to construct the name of the ref to push to (even if that's just a string concatenation). Doing that computation on a remote machine, rather than locally, makes little difference.

Then locally, you would always push to the same place - just configure the remote.origin.pushURL to the server which does the merging.

The URL to push to is constant. The ref to push to, however, would depend on what one was merging into -- hence what I mean by needing to be computed. Implementing this would also require replacing all of git-receive-pack on the server side; pushing into refs named by the server allows the server to not worry about magically hiding that overlap. As is, the server can manage the refs as normal refs, which works effectively.

Though the actual code changes to accomplish this are minimal, I can believe that supporting this land-elsewhere model isn't in scope for core arcanist. However, implementing the built-in workflows as final with private accessors precludes both subclassing and delegation for extensibility. Are there any good options available for implementing this, besides (a) local overrides to core land in our fork, (b) copying large swaths of code into new workflows, or (c) altering the final nature of the classes in our fork to allow them to be subclassed?

I can't think of any good solution right now, other than forking arc / duplicating the code; Forking is not in itself a bad practice - it's the most extreme form of "configurablity", although it comes with a price.
ArcanistGitLandEngine is less than a week old; I'm not sure how it will grow.

personally, I think T182 will become the common way to land things, and it will end up offering more flexibility for installs to customize what "Land" actually mean. I think users will prefer this to using the CLI, and OPs will prefer this centralized approach (No chasing users to upgrade their arc), which will make arc land less likely to allow full customization.

eadler added a subscriber: eadler.Oct 31 2015, 5:33 AM
epriestley closed this task as Wontfix.Nov 1 2015, 12:12 PM
epriestley claimed this task.
epriestley added a subscriber: epriestley.

The upstream pathway for this lies beyond T182.

arc land may eventually look like "check for clean merge, update diff, trigger merge via API, delete local branch", but we can't (or, at least, I do not know how to) get a chain of custody (a guarantee that what lands is what was reviewed) with a pure arc/client-side workflow and I consider this feature important as the upstream.

If you want to use a different merge manager, I'd expect you to eventually replace the action of "Land Revision" instead, then use a vanilla arc land. This would also make your web "Land Revision" work properly, give users better and richer feedback in the UI, and give you a short pathway to chain-of-custody if you ever have projects that need it. The capabilities on this workflow should be a strict (and rich, expansive) superset of the capabilities on a pure client flow.


final is an important "we can break this API freely" signal. If you're comfortable accepting this risk, fork and remove it.

We've had better luck with aligning upstream and users expectations when users have to explicitly "void the warrantee" by removing final. This just makes it really clear that you're doing something we very obviously aren't going to support, even if the problem arises a year from now when whoever is running into the problem and whoever removed final have never met. When the next maintainer can see "oh, we forked and removed final", that's a pretty strong signal about the nature of the approach.

angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Dec 5 2015, 5:08 AM