Changeset View
Standalone View
src/applications/differential/landing/DifferentialLandingToHostedGit.php
- This file was added.
| Property | Old Value | New Value |
|---|---|---|
| File Mode | null | 100755 |
| <?php | |||||
| final class DifferentialLandingToHostedGit | |||||
| implements DifferentialLandingStrategyInterface { | |||||
| public function assertSupportForRepository( | |||||
| PhabricatorRepository $repository) { | |||||
| $vcs = $repository->getVersionControlSystem(); | |||||
| if ($vcs !== PhabricatorRepositoryType::REPOSITORY_TYPE_GIT) { | |||||
| throw new Exception('Only Git repositories are supported'); | |||||
| } | |||||
| if (!$repository->isHosted()) { | |||||
| throw new Exception('Repository must be Hosted'); | |||||
| } | |||||
avivey: I'm not very happy about this api being exposed here, in what's likely to become "reference… | |||||
Not Done Inline ActionsMaybe 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. epriestley: Maybe move it to the base class as a concrete method? Then we at least don't end up with a… | |||||
| if (!$repository->isWorkingCopyBare()) { | |||||
| throw new Exception('Repository must be hosted as "bare".'); | |||||
| } | |||||
| } | |||||
| public function commitRevisionToWorkspace( | |||||
| DifferentialRevision $revision, | |||||
| ArcanistRepositoryAPI $workspace, | |||||
| PhabricatorUser $user) { | |||||
| $diff = $revision->loadActiveDiff(); | |||||
| $diff_id = $diff->getID(); | |||||
| $call = new ConduitCall( | |||||
| 'differential.getrawdiff', | |||||
| array( | |||||
| 'diffID' => $diff_id, | |||||
| )); | |||||
| $call->setUser($user); | |||||
| $raw_diff = $call->execute(); | |||||
| $missing_binary = | |||||
| "\nindex " | |||||
| . "0000000000000000000000000000000000000000.." | |||||
| . "0000000000000000000000000000000000000000\n"; | |||||
| if (strpos($raw_diff, $missing_binary) !== false) { | |||||
| throw new Exception("Patch is missing content for a binary file"); | |||||
| } | |||||
| $tmp_file = new TempFile(); | |||||
| Filesystem::writeFile($tmp_file, $raw_diff); | |||||
| $workspace->execxLocal('apply --index %s', $tmp_file); | |||||
epriestleyUnsubmitted Not Done Inline ActionsYou 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.) epriestley: You should be able to get a Future and then `write(...)` to it to skip this temp file stuff. | |||||
| $workspace->reloadWorkingCopy(); | |||||
| $call = new ConduitCall( | |||||
| 'differential.getcommitmessage', | |||||
| array( | |||||
| 'revision_id' => $revision->getID(), | |||||
| )); | |||||
| $call->setUser($user); | |||||
| $message = $call->execute(); | |||||
| $author = id(new PhabricatorUser())->loadOneWhere( | |||||
| 'phid = %s', | |||||
| $revision->getAuthorPHID()); | |||||
| $author_string = sprintf( | |||||
| '%s <%s>', | |||||
| $author->getRealName(), | |||||
| $author->loadPrimaryEmailAddress()); | |||||
epriestleyUnsubmitted Not Done Inline ActionsIt 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. epriestley: It would be slightly more correct to take the original diff's author/email if they're available… | |||||
| $workspace->execxLocal( | |||||
| '-c user.name=%s -c user.email=%s commit --message=%s --author=%s', | |||||
epriestleyUnsubmitted Not Done Inline ActionsIdeally, 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. epriestley: Ideally, we should pass `--date`, too, although that's not very important.
We may need to `… | |||||
| // -c will set the 'committer' | |||||
| $user->getRealName(), | |||||
| $user->loadPrimaryEmailAddress(), | |||||
epriestleyUnsubmitted Not Done Inline ActionsFor 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. epriestley: For the commiter, I think this is OK (taking the email address), although maybe we should let… | |||||
| $message, | |||||
| $author_string); | |||||
| } | |||||
| public function pushWorkspaceRepository( | |||||
| PhabricatorRepository $repository, | |||||
| ArcanistRepositoryAPI $workspace, | |||||
| PhabricatorUser $user) { | |||||
| $workspace->execxLocal("push origin HEAD:master"); | |||||
epriestleyUnsubmitted Not Done Inline ActionsWe'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. epriestley: We'll probably have to move away from hard coding `master` at some point, but I don't have a… | |||||
| } | |||||
| } | |||||
Not Done Inline ActionsThe 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. avivey: The only reason for this method is to explain what's wrong; but this method will not be invoked… | |||||
Not Done Inline ActionsI actually couldn't find where this method is defined, but it seems to do the right thing. avivey: I actually couldn't find where this method is defined, but it seems to do the right thing. | |||||
Not Done Inline ActionsIt'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). hach-que: It's one of the magic property getters because $revision is a DAO as far as I know (all tables… | |||||
Not Done Inline ActionsYep. (You can disable them, so not every table has those fields, but they're on by default and most objects do have them.) epriestley: Yep. (You can disable them, so not every table has those fields, but they're on by default and… | |||||
Not Done Inline ActionsI 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:
But I don't think this is important for now. Simplifying this code for the moment is probably cleaner. epriestley: I think the "git" and "hosted" cases are plainly obvious, and we probably never need to explain… | |||||
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.