Differential D21041 Diff 50128 src/workflow/ArcanistDiffWorkflow.php

Changeset 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($stdout) = execx('git ls-files -z -- %s', ':(attr:filter=lfs)');
		$is_lfs = strpos($stdout, "\0") !== false;
		epriestleyUnsubmitted Done Inline Actions Slightly more conventional if written as: list($stdout) = execx( 'git ls-files -- %s', ':(attr:filter=lfs)'); This is effectively the same, but: Throws if `git ls-files` fails, rather than continuing silently, which is generally desirable for consistency with how everything else works. Invokes escaping logic, which is probably always "add single quotes" but may, e.g., differ on Windows. 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: epriestley@orbital ~/scratch/spellbook $ git ls-files ':(attr:filter=lfs)' 1M.blob epriestley@orbital ~/scratch/spellbook $ git ls-files ':(attr:filter=lfs)' -- 1M.blob epriestley@orbital ~/scratch/spellbook $ git ls-files -- ':(attr:filter=lfs)' 1M.blob epriestley@orbital ~/scratch/spellbook $ ls -alh ':(attr:filter=lfs)' -rw-r--r-- 1 epriestley staff 0B Mar 19 17:54 :(attr:filter=lfs) Some day far in the future we might figure out how to specify inputs to computer programs without ambiguity. epriestley: Slightly more conventional if written as: ``` list($stdout) = execx( 'git ls-files -- %s'…

		epriestleyUnsubmitted Done Inline Actions You 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: `$stdout` will be a single newline. `empty("\n")` is `false`. `!empty("\n")` is `true`. LFS will always be detected, even in a non-LFS repository. epriestley: You should test the no-LFS-files case to make sure, but this probably (?) needs to be `!empty…
		epriestleyUnsubmitted Done Inline Actions After 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: Run `git ls-files -z ...` to get "\0" terminators. Test for `$is_lfs = (strpos($stdout, "\0") !== false);` 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'];
}		}

Show All 9 Lines	if ($err) {
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(
		marouxUnsubmitted Not Done Inline Actions This err shouldn't be ignored here. maroux: This err shouldn't be ignored here.
		'git push -- %s %s',
		$staging_uri,
		epriestleyUnsubmitted Done Inline Actions Exactly 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)…
		$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