Page MenuHomePhabricator

Land Revision button for hosted git repos
ClosedPublic

Authored by avivey on Nov 3 2013, 1:06 AM.

Details

Summary

ref T182.

Simple approach of clone, patch, push. While waiting for drydock, implement a hackish mutex
setup for the workspace, which should work ok as long as there's only one committer who is
carefull about theses things.

Less obvious note: This is taking the both author and commiter's 'primary email' for the commit -
which might rub some people wrong.

Test Plan

With a hosted repo, created some diffs and landed them.
Also clicked button for some error cases, got the right error message.

Diff Detail

Branch
landing
Lint
Lint WarningsExcuse: todo
Unit
Unit Tests OK

Event Timeline

I'm guessing we might want to force it into a POST, because it's not idempotent.

The UI right now is a new page with an error message and navigation, which I don't know how to improve (pop-up dialog with the error message over the revision page?).

src/applications/differential/controller/DifferentialRevisionLandController.php
56–72

I expect these 3 calls will be abstracted to 3 "providers" or something, which the flow will have to pick and match based on repo type and features.

I only avoided extracting interfaces/classes due to strict adherence to YAGNI, and I plan my own "push provider" for not-hosted git repos.

Initial thoughts are to create /differential/landing dir which will hold all interfaces/implementations, and somehow config them in the repo control.

src/applications/differential/controller/DifferentialRevisionViewController.php
537–543

I just needed some place to make the link for this.

avivey updated this revision to Unknown Object (????).Nov 4 2013, 1:22 AM
  • extract Strategy
  • check policy
  • only show button when applicable (In revision view).

Some inlines. I think we can improve the strategy stuff, see discussion below. It's stumbling into YAGNI territory but I want to avoid introducing new configuration, and I'm pretty sure we will eventually want more than one strategy per install.

The other big high-level thing is that we should move this to the daemons at some point (so you click "Land", it switches to "Landing", and the daemons actually land it), but it probably makes sense to build a bunch of other infrastructure first (toward Drydock) and this really isn't that terrible, I think.

Overall, this looks quite good to me -- it's not the most perfect high-architecture ivory-tower solution imaginable, but I think it should work fine, it solves all the major issues (like needing a lock), and we can eventually grow it into Drydock without changing too much.

The two things that jump out at me are:

  • Running this for the first time on a large repo is going to suck, since we have to clone it. A savvy admin can pre-clone it, but I don't see other real approaches until Drydock. (We could make the pull daemons pre-warm the working copy, but that seems fairly terrible.)
  • This forces a cherry-pick/squash strategy. We don't have enough data to perform a merge strategy and I think cherry-pick/squash is the best strategy in general, but some users disagree about this.

Maybe the easiest way to deal with these for now is to make the confirm dialog unconditional and just have warning text ("this is new, it always squashes, it may not work")?


For the Strategy stuff, I think it would be slightly cleaner to structure the logic like this:

Add an Event Listener: Add a listener for the revision menu rendering. You can look at DifferentialActionMenuEventListener for an example of how Differential inserts an "Edit Attached Revisions..." item into Maniphest. This listener would do the same thing, but insert "Land to X..." item(s) into Differential.

Find Strategies: The event listener can enumerate all available strategies with PhutilSymbolLoader:

$strategies = id(new PhutilSymbolLoader())
  ->setAncestorClass('BlahBlahStrategy')
  ->loadObjects();

This will instantiate an object of each concrete subclass, including strategies added through src/extensions/ and libraries loaded at runtime.

Let Strategies build a Menu Item: Give Strategy some callback like:

buildMenuItem($view, $viewer, $revision, $repository)

...and let them handle all the work for adding the menu item? This feels a little muddy but it should be very simple. The "local git" would look something like:

if (!$repository_is_git) { return; }
if (!$repository_is_hosted) { return; }

$enabled = $revision_is_landable && $user_has_push_capability_on_repository;

$view->addItem(... $enabled ...);

This would let "Land to Hosted" show up correctly on all the revisions where it could be used, enable/disable itself appropriately, etc., and coexist with other strategies. Although it seems unlikely that an install will want to use several different strategies on the same revision, using several different strategies across revisions does seem pretty reasonable (e.g., land local for local hosted repos, land to GitHub for GitHub repos; or local SVN land for SVN and local Git land for Git). The big win here is that everything happens automatically without needing to configure anything.

Delegate into Strategies? The controller will probably have to look something like:

/revision/id/land/(StrategyClass)/(.*)

...and its body will look something like:

// load revision and repository, check permissions, etc. -- basic shared
// junk common to every strategy. Then:

if (is_subclass($strategy_class, 'BlahBlahStrategy')) {
  $object = newv($strategy_class, array());
  return $object->processLandRequest($request, $revision, $repository);
}

That will let it pop custom dialogs, etc., and generally give it more control over how it behaves. Again, this is a little messy (we're shoving a lot of logic into the strategy) but I think it will be so much simpler that we'll end up ahead with this weaker API.

src/applications/differential/DifferentialGetWorkingCopy.php
3–6

This is fine, we'll move it into Drydock eventually in some form.

13

Discussed a bit below, but since you don't need any of the stuff it offers, using PhabricatorRepository is more consistent. Broadly, these classes (PhabricatorRepository vs RepositoryAPI vs all the Diffusion queries) probably were not perfectly architected and should theoretically split their responsibilities out a little differently than they do, but the current split doesn't seem to cause any major issues.

24–28

Use $repo->newRemoteCommandFuture() so we pick up all the environmental hacks we need on some systems.

This is slightly more correct as clone -- %s than clone %s (that is, add -- to terminate flags), since $path could some day start with a "-".

31

Use PhabricatorGlobalLock (probably much easier) or PhutilFileLock (probably not a good fit). These locks have better release semantics, unit tests, etc.

See PhabricatorRepositoryPullLocalDaemon for an example of creating a GlobalLock on a repo. I'd just put a lock on the whole repo for now -- by the time we support mor than one simultaneous workspace, hopefully we'll be on Drydock.

33–38

Using $repo->execxLocalCommand() is probably slightly preferable here if you don't need the GitAPI elsewhere. In theory we should maybe share more code between Repository and the *API classes, but in practice they mostly tend to deal with dissimilar situations.

43–53

Although it doesn't impact anything for now, this locking should probably happen in the Strategy so we can release it at the right time.

src/applications/differential/controller/DifferentialRevisionLandController.php
12

Double-quote?

15

You can use isFormPost() to verify the request is a POST with CSRF. To make it be POST, you can use workflow on the action link. The non-post case should pop a confirmation dialog ("Really land this?") for users with broken JS or right click -> open in new window. Roughly:

if ($this->isFormPost()) {
  do_land();
  return success_dialog();
}

return confirm_dialog();

If you always want to show the confirm dialog, you can use isDialogFormPost() instead of isFormPost().

33

All this return array(...) stuff should probably just let the exception escape, or wrap it in a ProxyException and rethrow. None of it ever returns more than one string.

src/applications/differential/landing/DifferentialLandingStrategyInterface.php
1

This file has a spurious +x.

3

This can probably just be a base class. I don't anticipate needing to add this interface to other things, and we'll probably want to put concrete code here (maybe for "is revision landable" or "does user have permission to push this repository", or "grab a lock" or whatever else).

src/applications/differential/landing/DifferentialLandingToHostedGit.php
51

You should be able to get a Future and then write(...) to it to skip this temp file stuff.

(We use temp files in arc in some cases, but that's just so the file is left over on failure so we can tell the user to go look at it. That seems less useful here.)

68–71

It would be slightly more correct to take the original diff's author/email if they're available (you'd have to fish around in Diff properties), and then fall back to something like this. Not a big deal.

74

Ideally, we should pass --date, too, although that's not very important.

We may need to --allow-empty and --allow-empty-message, although those seem fine to skip for now.

76–77

For the commiter, I think this is OK (taking the email address), although maybe we should let you designate a "VCS Email" in your profile. We can do that in a followup if we decide we care, though.

88

We'll probably have to move away from hard coding master at some point, but I don't have a strong sense of how configurable vs prompty vs automatic this should be.

src/applications/differential/DifferentialGetWorkingCopy.php
13

Like we discussed on irc - it's kinda awkward right now to use PhabricatorRepository. ArcanistGitApi is mostly missing the "HOME=" env var (which was solved in git 1.8.3.1).

24–28

getLocalCommandFuture() seems to be a better match (Because we ignore the repo's own remote, and our remote here is file://.

src/applications/differential/controller/DifferentialRevisionLandController.php
15

For now I'd feel safer with having the confirmation dialog always show up, and maybe explain what it'd going to do (As in "This is going to squash this revision, rebase, and push it to master").

avivey updated this revision to Unknown Object (????).Nov 5 2013, 6:35 AM

implement most of things.

It feels a bit weird to let "random" classes add menu items, but it does work.

Another issue that I encountered is that this implementation doesn't work on empty repositories - they don't have a "master" branch, so it fails on "git checkout master". I don't know if it's worth it to try to handle that case.

src/applications/differential/config/PhabricatorDifferentialConfigOptions.php
38–43

I'll remove this next iteration; it's not used any more.

src/applications/differential/controller/DifferentialRevisionLandController.php
115

I'd moved the lock into the "main" controller, to minimize the risk of a strategy doing something wrong with the locks. It's also simpler here.

src/applications/differential/landing/DifferentialLandingToHostedGit.php
15

I'm not very happy about this api being exposed here, in what's likely to become "reference implementation" for strategies; But separating it to another parameter is also not ideal.

Maybe make it a method of PhabricatorRepository? That would at least let us hide it and easily replace with drydock.

92

I actually couldn't find where this method is defined, but it seems to do the right thing.

144–159

The only reason for this method is to explain what's wrong; but this method will not be invoked under normal conditions if it's not supposed to work, so it should go away; Probably replace with doesSupportRepository() that just return a boolean. Or just assume that since this class is in charge on creating the link that enables it, than it will always be called under the right conditions.

src/applications/differential/landing/DifferntialLandingActionMenuEventListener.php
45–48 ↗(On Diff #16907)

It felt safer to not provide the $view or $event to the strategy, and limiting it to providing the menu items.

src/applications/differential/landing/DifferentialLandingToHostedGit.php
92

It's one of the magic property getters because $revision is a DAO as far as I know (all tables have a dateCreated and dateModified column).

Minor cleanup inlines, I don't think I caught anything substantial.

I think we can ignore the empty repo case for now. Diffusion itself doesn't handle it well currently, although I've made a bunch of progress toward getting that stuff into shape (T1493).

src/applications/differential/DifferentialGetWorkingCopy.php
23

Slightly simpler as execxLocalCommand(), I think.

src/applications/differential/application/PhabricatorApplicationDifferential.php
34

Differntial is misspelled. :)

50

The (?: and corresponding ) can be removed since that capturing pattern isn't optional.

\w* might be better as [^/]+

src/applications/differential/controller/DifferentialRevisionLandController.php
31–35

(This could probably just 404 if you want, I think it's unlikely anyone will ever hit this by accident.)

41–42

UI strings like this should be wrapped in pht().

43–56

I think if you just let this exception escape, we'll render something approximately as useful without needing any code.

63

Maybe just call this "Done"? It should also have pht().

76

For consistency, prefer {$var} to ${var}. Both work because PHP is a magical language, but we use the former form (nearly?) exclusively. I should just add a lint rule for this, but the grammar is pretty messy and I just punted originally instead of actually writing it into the parser.

130

This is declared static, but called as an instance method on line 115.

src/applications/differential/landing/DifferentialLandingToHostedGit.php
15

Maybe move it to the base class as a concrete method? Then we at least don't end up with a bunch of copy/paste, although I think we'll need to do a bit of work to migrate no matter what we do.

92

Yep. (You can disable them, so not every table has those fields, but they're on by default and most objects do have them.)

144–159

I think the "git" and "hosted" cases are plainly obvious, and we probably never need to explain them. It would be silly/counterproductive to show-but-disable "Land to hosted git" on a diff against a mercurial repository, for instance.

The "bare" case is a little less obvious. I could imagine eventually wanting the workflow to be like:

  • Item is available but disabled.
  • Clicking it shows you a dialog explaining the bare thing, with steps to resolve it.

But I don't think this is important for now. Simplifying this code for the moment is probably cleaner.

src/applications/differential/landing/DifferntialLandingActionMenuEventListener.php
45–48 ↗(On Diff #16907)

Yeah, this seems reasonable to me.

src/applications/differential/controller/DifferentialRevisionLandController.php
43–56

The default handler doesn't special-case ProxyException, and it doesn't add the <pre>, which is important for git errors.

I can reduce some code here though.

avivey updated this revision to Unknown Object (????).Nov 5 2013, 7:34 PM
  • remove config
  • fix filename, uri
  • pht
  • remove assertSupport... method
  • move getCleanGitWorkspace to base strategy

Awesome, thanks!

src/applications/differential/controller/DifferentialRevisionLandController.php
45

baaaa

Closed by commit rP5c0edc935102 (authored by Aviv Eyal <aeyal@groupon.com>, committed by @epriestley).