Page MenuHomePhabricator

Land to GitHub + support stuff

Authored by avivey on Nov 11 2013, 3:09 AM.



A usable, Land to GitHub flow.

Still to do:

  • Refactor all git/hg stratagies to a sane structure.
  • Make the dialogs Workflow + explain why it's disabled.
  • Show button and request Link Account if GH is enabled, but user is not linked.
  • After refreshing token, user ends up in the settings stage.

Hacked something in LandController to be able to show an arbitrary dialog from a strategy.
It's not very nice, but I want to make some more refactoring to the controller/strategy/ies anyway.

Also made PhabricatorRepository::getRemoteURIObject() public, because it was very useful in getting
the domain and path for the repo.

Test Plan

Went through these flows:

  • load revision in hosted, github-backed, non-github backed repos to see button as needed.
  • hit land with weak token - sent to refresh it with the extra scope.
  • Land to repo I'm not allowed - got proper error message.
  • Successfully landed; Failed to apply patch.

Diff Detail

Lint OK
Unit Tests OK

Event Timeline

epriestley requested changes to this revision.Nov 12 2013, 1:03 AM

In theory, it would be nice if the GitHub logic looked more like:

  1. Load all of the "GitHub" accounts.
  2. Look for one with the same domain as the remote URI.

This shouldn't be too hard to implement, but will do a good job of future-proofing this.


  • Use %P to mask the command credentials.
  • Consider future-proofing account management (i.e., make all code assume the user may have credentials on more than one install of GitHub. Practically, this corresponds to a link to both and one or more enterprise installs). I'm fine with either approach on this, but it might actually be simpler.
  • Consider simplifying the permissions future; I don't think the edge case perf is that significant for the moderate additional complexity.

This looks pretty reasonable in general.


This should be pht()'d, but should also be the default button label?


I'd just do the whole permissions request here, maybe? Rather than using Futures? This should be a rare case and it's OK if the user waits for an extra second, right?


Instead of doing this, use %P for the sensitive parameter and just execx:

$workspace->execxLocal('push %P HEAD:master', $remote);

This will mask the authentication information. Without this, the token will still appear in, e.g., service call logs and other places you can't control.

I can skip the search, and just look for the right one:

$this->account = id(new PhabricatorExternalAccountQuery())

This ignores the case where the provider is configured, but the user is not linked.


not sure where this came from.

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

Updated according to feedback.

epriestley accepted this revision.Nov 14 2013, 1:24 AM

Nice, thanks!


goodbye moo

you will be missed


Ah, cool, this is much cleaner. I do wonder if we'll hit some "" vs "" issues, but we can wait for those to hit.


Should this (try to) use api.<whatever flavor of github is going on>?

epriestley closed this revision.Nov 14 2013, 1:25 AM

Closed by commit rPdcf909ba56bf (authored by @avivey, committed by @epriestley).

avivey added inline comments.Nov 14 2013, 1:27 AM

although, at least for my install, it'll be github.<company>/api/v3/, so not easy to guess in any case.

We could add an option to the GitHub provider when we fix it so you can add a provider directly against an Enterprise Edition.