Page MenuHomePhabricator

staging repo compatibility for older git versions
ClosedPublic

Authored by cburroughs on Sep 1 2015, 7:35 PM.

Details

Summary

The --no-verify flag was not added until git 1.8.2. This
flag is used to avoid running local pre-push hooks. This is likely a
rare configuration and is safe to omit the flag on older versions.
Users with local pre-push hooks and older git version may need to
adjust their workfow.

fixes T9310

Test Plan
  • Ran arc diff with my real git 1.7.10.4 and succeeded with STAGING PUSHED.
  • Edited getGitVersion to be > 1.8.2 and pushed again. Got STAGING FAILED because error: unknown option.

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cburroughs updated this revision to Diff 33927.Sep 1 2015, 7:35 PM
cburroughs retitled this revision from to staging repo compatibility for older git versions.
cburroughs updated this object.
cburroughs edited the test plan for this revision. (Show Details)

I tried a bunch of clever/terser ways but they all ran into 'Unsafe Usage of Dynamic String' or phutil_passthru quoting even the empty string.

Does something like this work to slightly increase terseness/cleverness?

$flags = array();
if (have_version) {
  $flags[] = '--no-verify';
}

$err = phutil_passthru(
  'git push %Ls -- %s %s:refs/tags/%s',
  $flags,
  $staging_uri,
  $commit,
  $tag);

(I'd also sort of like to centralize the version check stuff, but this is fine as is until I get around to it.)

cburroughs updated this revision to Diff 33939.Sep 2 2015, 12:32 PM
cburroughs edited edge metadata.
  • clever++, or is that ++clever?

Ah! I didn't know empty arrays (unlike empty strings) did not get
quoted.

I think they previously didn't work like that (empty array threw an exception?) but we changed the behavior specifically to make this construction (or similar constructions?) easier, IIRC.

There's a sort-of-potentially-bad case where you execute rm -rf %Ls and the array is empty but you didn't realize it, and the behavior of rm with no arguments is very different/much worse than the behavior with one argument (for rm, specifically, it isn't, but for some other command it might be), and you destroy everything on your system or something, but we now have a bunch of callsites where this version of the API is making flag handling easier and safer, and the stars have yet to align to produce a problematic case where some hypothetical command activates "terrible catastrophe mode" when passed no arguments, so I think the moderate reduction in API safety for a great increase in API utility was good on the balance.

epriestley accepted this revision.Sep 2 2015, 1:52 PM
epriestley added a reviewer: epriestley.
This revision is now accepted and ready to land.Sep 2 2015, 1:52 PM
This revision was automatically updated to reflect the committed changes.