Changeset View
Changeset View
Standalone View
Standalone View
src/workflow/ArcanistDiffWorkflow.php
Show First 20 Lines • Show All 2,894 Lines • ▼ Show 20 Lines | private function pushChangesToStagingArea($id) { | ||||
} | } | ||||
$refs = array(); | $refs = array(); | ||||
$remote = array( | $remote = array( | ||||
'uri' => $staging_uri, | 'uri' => $staging_uri, | ||||
); | ); | ||||
list($err, $stdout, $stderr) = exec_manual( | |||||
"git ls-files ':(attr:filter=lfs)'"); | |||||
epriestley: Slightly more conventional if written as:
```
list($stdout) = execx(
'git ls-files -- %s'… | |||||
$is_lfs = !empty($stdout); | |||||
Done Inline ActionsYou should test the no-LFS-files case to make sure, but this probably (?) needs to be !empty(trim($stdout)). As written, in a non-LFS repository:
epriestley: You should test the no-LFS-files case to make sure, but this probably (?) needs to be `!empty… | |||||
Done Inline ActionsAfter sleeping on it, I realize my suggested replacement isn't quite right either: it will result in the wrong behavior if the repository has one or more LFS-controlled files which have only tabs and spaces in their names, and no other LFS-controlled files with other characters in their names. This isn't very likely or common, but is a valid repository state. I think a completely correct test is:
epriestley: After sleeping on it, I realize my suggested replacement isn't quite right either: it will… | |||||
// If the base commit is a real commit, we're going to push it. We don't | // If the base commit is a real commit, we're going to push it. We don't | ||||
// use this, but pushing it to a ref reduces the amount of redundant work | // use this, but pushing it to a ref reduces the amount of redundant work | ||||
// that Git does on later pushes by helping it figure out that the remote | // that Git does on later pushes by helping it figure out that the remote | ||||
// already has most of the history. See T10509. | // already has most of the history. See T10509. | ||||
// In the future, we could avoid this push if the staging area is the same | // In the future, we could avoid this push if the staging area is the same | ||||
// as the main repository, or if the staging area is a virtual repository. | // as the main repository, or if the staging area is a virtual repository. | ||||
// In these cases, the staging area should automatically have up-to-date | // In these cases, the staging area should automatically have up-to-date | ||||
// refs. | // refs. | ||||
$base_commit = $api->getSourceControlBaseRevision(); | $base_commit = $api->getSourceControlBaseRevision(); | ||||
if ($base_commit !== ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT) { | if ($base_commit !== ArcanistGitAPI::GIT_MAGIC_ROOT_COMMIT) { | ||||
$refs[] = array( | $refs[] = array( | ||||
'ref' => $base_tag, | 'ref' => $base_tag, | ||||
'type' => 'base', | 'type' => 'base', | ||||
'commit' => $base_commit, | 'commit' => $base_commit, | ||||
'remote' => $remote, | 'remote' => $remote, | ||||
); | ); | ||||
} | } | ||||
// We're always going to push the change itself. | // We're always going to push the change itself. | ||||
$refs[] = array( | $refs[] = array( | ||||
'ref' => $diff_tag, | 'ref' => $diff_tag, | ||||
'type' => 'diff', | 'type' => 'diff', | ||||
'commit' => $commit, | 'commit' => $is_lfs ? $base_commit : $commit, | ||||
'remote' => $remote, | 'remote' => $remote, | ||||
); | ); | ||||
$ref_list = array(); | $ref_list = array(); | ||||
foreach ($refs as $ref) { | foreach ($refs as $ref) { | ||||
$ref_list[] = $ref['commit'].':'.$ref['ref']; | $ref_list[] = $ref['commit'].':'.$ref['ref']; | ||||
} | } | ||||
if ($is_lfs) { | |||||
exec_manual('git lfs uninstall'); | |||||
} | |||||
$err = phutil_passthru( | $err = phutil_passthru( | ||||
'git push %Ls -- %s %Ls', | 'git push %Ls -- %s %Ls', | ||||
$push_flags, | $push_flags, | ||||
$staging_uri, | $staging_uri, | ||||
$ref_list); | $ref_list); | ||||
if ($is_lfs) { | |||||
exec_manual('git lfs install'); | |||||
} | |||||
if ($err) { | if ($err) { | ||||
$this->writeWarn( | $this->writeWarn( | ||||
pht('STAGING FAILED'), | pht('STAGING FAILED'), | ||||
pht('Unable to push changes to the staging area.')); | pht('Unable to push changes to the staging area.')); | ||||
throw new ArcanistUsageException( | throw new ArcanistUsageException( | ||||
pht( | pht( | ||||
'Failed to push changes to staging area. Correct the issue, or '. | 'Failed to push changes to staging area. Correct the issue, or '. | ||||
'use --skip-staging to skip this step.')); | 'use --skip-staging to skip this step.')); | ||||
} | } | ||||
if ($is_lfs) { | |||||
$ref = '+'.$commit.':'.$diff_tag; | |||||
$err = phutil_passthru( | |||||
Not Done Inline ActionsThis err shouldn't be ignored here. maroux: This err shouldn't be ignored here. | |||||
'git push %Ls -- %s %s', | |||||
$push_flags, | |||||
Done Inline ActionsExactly as written, this list will still include --no-verify (and thus not push LFS objects), I think? epriestley: Exactly as written, this list will still include `--no-verify` (and thus not push LFS objects)… | |||||
$staging_uri, | |||||
$ref); | |||||
} | |||||
return $refs; | return $refs; | ||||
} | } | ||||
/** | /** | ||||
* Try to upload lint and unit test results into modern Harbormaster build | * Try to upload lint and unit test results into modern Harbormaster build | ||||
* targets. | * targets. | ||||
* | * | ||||
▲ Show 20 Lines • Show All 125 Lines • Show Last 20 Lines |
Slightly more conventional if written as:
This is effectively the same, but:
I've also added -- as a prayer on the altar of T13491, but this appears to be pointless because Git always treats these patterns as patterns, even when a file with the same name exists on disk:
Some day far in the future we might figure out how to specify inputs to computer programs without ambiguity.