From f3552894a46a8658580502831704ee6230292929 Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Sat, 27 Aug 2016 10:06:10 -0300 Subject: [PATCH 1/8] Add `git pull --rebase {base-branch}` call at `GitHelper::squashCommits()` --- src/Helper/GitHelper.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Helper/GitHelper.php b/src/Helper/GitHelper.php index 80bea7c0..2cdba110 100644 --- a/src/Helper/GitHelper.php +++ b/src/Helper/GitHelper.php @@ -638,6 +638,16 @@ public function squashCommits($base, $branchName, $ignoreMultipleAuthors = false '-author' => $author, ] ); + + try { + // Ensure squashed commits don't introduce regressions at base branch + $this->processHelper->runCommand(['git', 'pull', '--rebase', $base]); + } catch (\Exception $e) { + // Error, abort the rebase process + $this->processHelper->runCommand(['git', 'rebase', '--abort'], true); + + throw new \RuntimeException(sprintf('Git rebase failed while trying to synchronize history against "%s".', $base), 0, $e); + } } public function syncWithRemote($remote, $branchName = null) From 2556bcfa21eb8cc7df744fdb0bd317e10f4459c0 Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Sat, 27 Aug 2016 17:43:21 -0300 Subject: [PATCH 2/8] Check branch sync status before squash --- src/Helper/GitHelper.php | 78 +++++++++++++++------------------- tests/Helper/GitHelperTest.php | 2 +- 2 files changed, 35 insertions(+), 45 deletions(-) diff --git a/src/Helper/GitHelper.php b/src/Helper/GitHelper.php index 2cdba110..7657a621 100644 --- a/src/Helper/GitHelper.php +++ b/src/Helper/GitHelper.php @@ -12,6 +12,7 @@ namespace Gush\Helper; use Gush\Exception\CannotSquashMultipleAuthors; +use Gush\Exception\MergeWorkflowException; use Gush\Exception\UserException; use Gush\Exception\WorkingTreeIsNotReady; use Gush\Operation\RemoteMergeOperation; @@ -386,7 +387,7 @@ public function mergeBranch($base, $sourceBranch, $commitMessage, $fastForward = 'allow_failures' => false, ], [ - 'line' => ['git', 'commit', '-F', $tmpName], + 'line' => ['git', 'commit', '--file', $tmpName], 'allow_failures' => false, ], ] @@ -433,7 +434,7 @@ public function mergeBranchWithLog($base, $sourceBranch, $commitMessage, $source $tmpName = $this->filesystemHelper->newTempFilename(); file_put_contents($tmpName, $commitMessage); - $this->processHelper->runCommand(['git', 'commit', '-F', $tmpName]); + $this->processHelper->runCommand(['git', 'commit', '--file', $tmpName]); return trim($this->processHelper->runCommand('git rev-parse HEAD')); } @@ -443,15 +444,7 @@ public function addNotes($notes, $commitHash, $ref) $tmpName = $this->filesystemHelper->newTempFilename(); file_put_contents($tmpName, $notes); - $commands = [ - 'git', - 'notes', - '--ref='.$ref, - 'add', - '-F', - $tmpName, - $commitHash, - ]; + $commands = ['git', 'notes', '--ref='.$ref, 'add', '--file', $tmpName, $commitHash]; $this->processHelper->runCommand($commands, true); } @@ -596,21 +589,26 @@ public function squashCommits($base, $branchName, $ignoreMultipleAuthors = false // Get commits only in the branch but not in base (in reverse order) // we can't use --max-count here because that is applied before the reversing! // - // using git-log works better then finding the fork-point with git-merge-base + // using git-log works better than finding the fork-point with git-merge-base // because this protects against edge cases were there is no valid fork-point - $firstCommitHash = StringUtil::splitLines($this->processHelper->runCommand( - [ - 'git', - '--no-pager', - 'log', - '--oneline', - '--no-color', - '--format=%H', - '--reverse', - $base.'..'.$branchName, - ] - ))[0]; + $firstCommitHash = StringUtil::splitLines($this->processHelper->runCommand([ + 'git', + '--no-pager', + 'log', + '--oneline', + '--no-color', + '--format=%H', + '--reverse', + $base.'..'.$branchName, + ]))[0]; + + $currentBaseHeadCommit = $this->processHelper->runCommand(['git', 'rev-parse', $base]); + $lastKnownCommonCommit = $this->processHelper->runCommand(['git', 'merge-base', '--fork-point', $base, $branchName]); + + if ($currentBaseHeadCommit !== $lastKnownCommonCommit) { + throw new MergeWorkflowException(sprintf('Failed while trying to perform merge against "%s", history is out of sync.', $base)); + } // 0=author anything higher then 0 is the full body $commitData = StringUtil::splitLines( @@ -631,23 +629,7 @@ public function squashCommits($base, $branchName, $ignoreMultipleAuthors = false $message = implode("\n", $commitData); $this->reset($base); - $this->commit( - $message, - [ - 'a', - '-author' => $author, - ] - ); - - try { - // Ensure squashed commits don't introduce regressions at base branch - $this->processHelper->runCommand(['git', 'pull', '--rebase', $base]); - } catch (\Exception $e) { - // Error, abort the rebase process - $this->processHelper->runCommand(['git', 'rebase', '--abort'], true); - - throw new \RuntimeException(sprintf('Git rebase failed while trying to synchronize history against "%s".', $base), 0, $e); - } + $this->commit($message, ['all', 'author' => $author]); } public function syncWithRemote($remote, $branchName = null) @@ -681,9 +663,17 @@ public function commit($message, array $options = []) foreach ($options as $option => $value) { if (is_int($option)) { - $params[] = '-'.$value; + if (1 === strlen($value)) { + $params[] = '-'.$value; + } else { + $params[] = '--'.$value; + } } else { - $params[] = '-'.$option; + if (1 === strlen($option)) { + $params[] = '-'.$option; + } else { + $params[] = '--'.$option; + } $params[] = $value; } } @@ -691,7 +681,7 @@ public function commit($message, array $options = []) $tmpName = $this->filesystemHelper->newTempFilename(); file_put_contents($tmpName, $message); - $this->processHelper->runCommand(array_merge(['git', 'commit', '-F', $tmpName], $params)); + $this->processHelper->runCommand(array_merge(['git', 'commit', '--file', $tmpName], $params)); } /** diff --git a/tests/Helper/GitHelperTest.php b/tests/Helper/GitHelperTest.php index d0f9f82b..bbe0492f 100644 --- a/tests/Helper/GitHelperTest.php +++ b/tests/Helper/GitHelperTest.php @@ -176,7 +176,7 @@ public function merges_remote_branch_in_clean_wc() 'allow_failures' => false, ], [ - 'line' => ['git', 'commit', '-F', $tmpName], + 'line' => ['git', 'commit', '--file', $tmpName], 'allow_failures' => false, ], ] From 33e09370fd04ec6bd84080889ff90611fdbf6f67 Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Sat, 27 Aug 2016 18:35:08 -0300 Subject: [PATCH 3/8] Add new options to `pull-request:merge` command --- .../PullRequest/PullRequestMergeCommand.php | 14 ++++++ src/Helper/GitHelper.php | 7 --- src/Operation/RemoteMergeOperation.php | 45 ++++++++++++++++++- .../PullRequestMergeCommandTest.php | 4 +- 4 files changed, 61 insertions(+), 9 deletions(-) diff --git a/src/Command/PullRequest/PullRequestMergeCommand.php b/src/Command/PullRequest/PullRequestMergeCommand.php index aefaf354..69253302 100644 --- a/src/Command/PullRequest/PullRequestMergeCommand.php +++ b/src/Command/PullRequest/PullRequestMergeCommand.php @@ -40,6 +40,8 @@ protected function configure() ->addOption('fast-forward', null, InputOption::VALUE_NONE, 'Merge pull-request using fast forward (no merge commit will be created)') ->addOption('squash', null, InputOption::VALUE_NONE, 'Squash the PR before merging') ->addOption('force-squash', null, InputOption::VALUE_NONE, 'Force squashing the PR, even if there are multiple authors (this will implicitly use --squash)') + ->addOption('rebase', null, InputOption::VALUE_NONE, 'Rebase the PR before merging') + ->addOption('ensure-sync', null, InputOption::VALUE_NONE, 'Ensure that the pull request history is up to date before merging') ->addOption('switch', null, InputOption::VALUE_REQUIRED, 'Switch the base of the pull request before merging') ->addOption('pat', null, InputOption::VALUE_REQUIRED, 'Give the PR\'s author a pat on the back after the merge') ->setHelp( @@ -79,6 +81,16 @@ protected function configure() $ gush %command.name% --fast-forward 12 +If you want to perform an automatic rebase against the base branch before merging, the --rebase option can be used +in order to try that operation: + + $ gush %command.name% --rebase 12 + +A synchronization check against the base branch can be done before the merge, passing the --ensure-sync option; so +if this check fails, the operation will be aborted: + + $ gush %command.name% --ensure-sync 12 + After the pull request is merged, you can give a pat on the back to its author using the --pat. This option accepts the name of any configured pat's name: @@ -173,6 +185,8 @@ protected function execute(InputInterface $input, OutputInterface $output) $mergeOperation->setTarget($targetRemote, $targetBranch); $mergeOperation->setSource($sourceRemote, $sourceBranch); $mergeOperation->squashCommits($squash, $input->getOption('force-squash')); + $mergeOperation->guardSync($input->getOption('ensure-sync')); + $mergeOperation->rebase($input->getOption('rebase')); $mergeOperation->switchBase($input->getOption('switch')); $mergeOperation->setMergeMessage($messageCallback); $mergeOperation->useFastForward($input->getOption('fast-forward')); diff --git a/src/Helper/GitHelper.php b/src/Helper/GitHelper.php index 7657a621..df373f98 100644 --- a/src/Helper/GitHelper.php +++ b/src/Helper/GitHelper.php @@ -603,13 +603,6 @@ public function squashCommits($base, $branchName, $ignoreMultipleAuthors = false $base.'..'.$branchName, ]))[0]; - $currentBaseHeadCommit = $this->processHelper->runCommand(['git', 'rev-parse', $base]); - $lastKnownCommonCommit = $this->processHelper->runCommand(['git', 'merge-base', '--fork-point', $base, $branchName]); - - if ($currentBaseHeadCommit !== $lastKnownCommonCommit) { - throw new MergeWorkflowException(sprintf('Failed while trying to perform merge against "%s", history is out of sync.', $base)); - } - // 0=author anything higher then 0 is the full body $commitData = StringUtil::splitLines( $this->processHelper->runCommand( diff --git a/src/Operation/RemoteMergeOperation.php b/src/Operation/RemoteMergeOperation.php index 2bc4969b..b85ad677 100644 --- a/src/Operation/RemoteMergeOperation.php +++ b/src/Operation/RemoteMergeOperation.php @@ -31,6 +31,8 @@ class RemoteMergeOperation private $performed = false; private $fastForward = false; private $withLog = false; + private $rebase = false; + private $guardSync = false; public function __construct(GitHelper $gitHelper, FilesystemHelper $filesystemHelper) { @@ -80,6 +82,8 @@ public function setMergeMessage($message, $withLog = false) public function useFastForward($fastForward = true) { $this->fastForward = (bool) $fastForward; + + return $this; } public function performMerge() @@ -143,6 +147,21 @@ public function pushToRemote() $this->gitHelper->pushToRemote($this->targetRemote, $target); } + public function rebase(bool $rebase = false) + { + $this->rebase = $rebase; + $this->guardSync = !$rebase; + + return $this; + } + + public function guardSync(bool $guardSync = false) + { + $this->guardSync = $guardSync; + + return $this; + } + private function createBaseBranch() { $targetBranch = null !== $this->switchBase ? $this->switchBase : $this->targetBranch; @@ -171,7 +190,31 @@ private function createSourceBranch() } if ($this->squash) { - $this->gitHelper->squashCommits($this->targetBase, $sourceBranch, $this->forceSquash); + $this->gitHelper->squashCommits($this->targetBase, $sourceBranch, $this->forceSquash, $this->guardSync); + } + + $currentBaseHeadCommit = $this->processHelper->runCommand(['git', 'rev-parse', $this->targetBase]); + $lastKnownCommonCommit = $this->processHelper->runCommand(['git', 'merge-base', '--fork-point', $this->targetBase, $sourceBranch]); + + if ($currentBaseHeadCommit !== $lastKnownCommonCommit) { + if ($this->rebase) { + try { + $this->processHelper->runCommand(['git', 'pull', '--rebase', $this->targetBase]); + } catch (\Exception $e) { + // Error, abort the rebase operation + $this->processHelper->runCommand(['git', 'rebase', '--abort'], true); + + throw new MergeWorkflowException(sprintf('Git rebase failed while trying to synchronize history against "%s".', $this->targetBase), 0, $e); + } + + // Retrieve the commits again + $currentBaseHeadCommit = $this->processHelper->runCommand(['git', 'rev-parse', $this->targetBase]); + $lastKnownCommonCommit = $this->processHelper->runCommand(['git', 'merge-base', '--fork-point', $this->targetBase, $sourceBranch]); + } + + if ($this->guardSync && $currentBaseHeadCommit !== $lastKnownCommonCommit) { + throw new MergeWorkflowException(sprintf('Failed while trying to perform merge against "%s", history is out of sync.', $this->targetBase)); + } } // Allow a callback to allow late commits list composition diff --git a/tests/Command/PullRequest/PullRequestMergeCommandTest.php b/tests/Command/PullRequest/PullRequestMergeCommandTest.php index aef6b1a2..7761135f 100644 --- a/tests/Command/PullRequest/PullRequestMergeCommandTest.php +++ b/tests/Command/PullRequest/PullRequestMergeCommandTest.php @@ -455,7 +455,7 @@ protected function getGitConfigHelper($notes = true) return $helper; } - private function getLocalGitHelper($message = null, $squash = false, $forceSquash = false, $switch = null, $withComments = true, $fastForward = false) + private function getLocalGitHelper($message = null, $squash = false, $forceSquash = false, $switch = null, $withComments = true, $fastForward = false, $guardSync = false, $rebase = false) { $helper = parent::getGitHelper(); @@ -470,6 +470,8 @@ private function getLocalGitHelper($message = null, $squash = false, $forceSquas $mergeOperation->setTarget('gushphp', 'base_ref')->shouldBeCalled(); $mergeOperation->setSource('cordoval', 'head_ref')->shouldBeCalled(); $mergeOperation->squashCommits($squash, $forceSquash)->shouldBeCalled(); + $mergeOperation->guardSync($guardSync)->shouldBeCalled(); + $mergeOperation->rebase($rebase)->shouldBeCalled(); $mergeOperation->switchBase($switch)->shouldBeCalled(); $mergeOperation->useFastForward($fastForward)->shouldBeCalled(); $mergeOperation->setMergeMessage( From 5c5f593d8769ba69cdf0ac56d160f113edc50015 Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Mon, 29 Aug 2016 12:00:31 -0300 Subject: [PATCH 4/8] Update behavior between `--reabse` and `--ensure-sync` options --- src/Operation/RemoteMergeOperation.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/Operation/RemoteMergeOperation.php b/src/Operation/RemoteMergeOperation.php index b85ad677..91f9044f 100644 --- a/src/Operation/RemoteMergeOperation.php +++ b/src/Operation/RemoteMergeOperation.php @@ -150,7 +150,6 @@ public function pushToRemote() public function rebase(bool $rebase = false) { $this->rebase = $rebase; - $this->guardSync = !$rebase; return $this; } @@ -206,13 +205,7 @@ private function createSourceBranch() throw new MergeWorkflowException(sprintf('Git rebase failed while trying to synchronize history against "%s".', $this->targetBase), 0, $e); } - - // Retrieve the commits again - $currentBaseHeadCommit = $this->processHelper->runCommand(['git', 'rev-parse', $this->targetBase]); - $lastKnownCommonCommit = $this->processHelper->runCommand(['git', 'merge-base', '--fork-point', $this->targetBase, $sourceBranch]); - } - - if ($this->guardSync && $currentBaseHeadCommit !== $lastKnownCommonCommit) { + } elseif ($this->guardSync) { throw new MergeWorkflowException(sprintf('Failed while trying to perform merge against "%s", history is out of sync.', $this->targetBase)); } } From fee8f9300468b88ca2a1512543ada759d2042098 Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Mon, 29 Aug 2016 12:10:36 -0300 Subject: [PATCH 5/8] Split `--ref={ref}` in two arguments --- src/Helper/GitHelper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Helper/GitHelper.php b/src/Helper/GitHelper.php index df373f98..af0add3d 100644 --- a/src/Helper/GitHelper.php +++ b/src/Helper/GitHelper.php @@ -444,7 +444,7 @@ public function addNotes($notes, $commitHash, $ref) $tmpName = $this->filesystemHelper->newTempFilename(); file_put_contents($tmpName, $notes); - $commands = ['git', 'notes', '--ref='.$ref, 'add', '--file', $tmpName, $commitHash]; + $commands = ['git', 'notes', '--ref=', $ref, 'add', '--file', $tmpName, $commitHash]; $this->processHelper->runCommand($commands, true); } From 23635b725e4f83c34492e9e5998059f465cbf4b0 Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Mon, 29 Aug 2016 21:13:56 -0300 Subject: [PATCH 6/8] Remove `=` concat between `--ref` option and its value at `GitHelper::addNotes()` --- src/Helper/GitHelper.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Helper/GitHelper.php b/src/Helper/GitHelper.php index af0add3d..7100c1f6 100644 --- a/src/Helper/GitHelper.php +++ b/src/Helper/GitHelper.php @@ -444,7 +444,7 @@ public function addNotes($notes, $commitHash, $ref) $tmpName = $this->filesystemHelper->newTempFilename(); file_put_contents($tmpName, $notes); - $commands = ['git', 'notes', '--ref=', $ref, 'add', '--file', $tmpName, $commitHash]; + $commands = ['git', 'notes', '--ref', $ref, 'add', '--file', $tmpName, $commitHash]; $this->processHelper->runCommand($commands, true); } From ca8033b7c6b064d2f1628a21daf57e13023264f3 Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Mon, 12 Sep 2016 13:19:23 -0300 Subject: [PATCH 7/8] Update `RemoteMergeOperation` --- src/Helper/GitHelper.php | 2 +- src/Operation/RemoteMergeOperation.php | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Helper/GitHelper.php b/src/Helper/GitHelper.php index 7100c1f6..e2258e5d 100644 --- a/src/Helper/GitHelper.php +++ b/src/Helper/GitHelper.php @@ -343,7 +343,7 @@ public function clearTempBranches() */ public function createRemoteMergeOperation() { - return new RemoteMergeOperation($this, $this->filesystemHelper); + return new RemoteMergeOperation($this, $this->filesystemHelper, $this->processHelper); } /** diff --git a/src/Operation/RemoteMergeOperation.php b/src/Operation/RemoteMergeOperation.php index 91f9044f..27ef884f 100644 --- a/src/Operation/RemoteMergeOperation.php +++ b/src/Operation/RemoteMergeOperation.php @@ -13,11 +13,13 @@ use Gush\Helper\FilesystemHelper; use Gush\Helper\GitHelper; +use Gush\Helper\ProcessHelper; class RemoteMergeOperation { private $gitHelper; private $filesystemHelper; + private $processHelper; private $sourceBranch; private $sourceRemote; @@ -34,10 +36,11 @@ class RemoteMergeOperation private $rebase = false; private $guardSync = false; - public function __construct(GitHelper $gitHelper, FilesystemHelper $filesystemHelper) + public function __construct(GitHelper $gitHelper, FilesystemHelper $filesystemHelper, ProcessHelper $processHelper) { $this->gitHelper = $gitHelper; $this->filesystemHelper = $filesystemHelper; + $this->processHelper = $processHelper; } public function setSource($remote, $branch) From 6a1d22e270f99ce0a974bd85afa2e9e2c558590a Mon Sep 17 00:00:00 2001 From: Javier Spagnoletti Date: Tue, 20 Sep 2016 23:53:41 -0300 Subject: [PATCH 8/8] Update `RemoteMergeOperation::createSourceBranch()` --- src/Operation/RemoteMergeOperation.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Operation/RemoteMergeOperation.php b/src/Operation/RemoteMergeOperation.php index 27ef884f..ef22fec8 100644 --- a/src/Operation/RemoteMergeOperation.php +++ b/src/Operation/RemoteMergeOperation.php @@ -191,10 +191,6 @@ private function createSourceBranch() $this->targetBranch = $this->switchBase; } - if ($this->squash) { - $this->gitHelper->squashCommits($this->targetBase, $sourceBranch, $this->forceSquash, $this->guardSync); - } - $currentBaseHeadCommit = $this->processHelper->runCommand(['git', 'rev-parse', $this->targetBase]); $lastKnownCommonCommit = $this->processHelper->runCommand(['git', 'merge-base', '--fork-point', $this->targetBase, $sourceBranch]); @@ -213,6 +209,10 @@ private function createSourceBranch() } } + if ($this->squash) { + $this->gitHelper->squashCommits($this->targetBase, $sourceBranch, $this->forceSquash); + } + // Allow a callback to allow late commits list composition if ($this->message instanceof \Closure) { $closure = $this->message;