Remove dependence on callsigns from `bin/commit-hook`
ClosedPublic
Actions

Authored by epriestley on Feb 18 2016, 12:46 AM.

Details

Reviewers

avivey
chad

Maniphest Tasks

T4245: Make repository URIs slightly prettier

Commits

Restricted Diffusion Commit
rP973b8ace8616: Remove dependence on callsigns from `bin/commit-hook`

Summary

Ref T4245. Two effects:

First, let hooks work for future repositories without callsigns.
Second, provide a better error when users push directly to hosted repositories.

Test Plan

Ran bin/commit-hook PHID-REPO-xxx.

Diff Detail

Repository

rP Phabricator

Branch

call1

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 10737
Build 13207: Run Core Tests
Build 13206: arc lint + arc unit

Event Timeline

epriestley updated this revision to Diff 36894.Feb 18 2016, 12:46 AM

epriestley retitled this revision from to Remove dependence on callsigns from `bin/commit-hook`.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added a task: T4245: Make repository URIs slightly prettier.

avivey accepted this revision.Feb 18 2016, 12:48 AM

avivey added a reviewer: avivey.

This revision is now accepted and ready to land.Feb 18 2016, 12:48 AM

Oh, don't actually need that extra parameter to pht() for the error anymore.

In D15293#175823, @epriestley wrote:

Oh, don't actually need that extra parameter to pht() for the error anymore.

Isn't there a lint for that?

Closed by commit rP973b8ace8616: Remove dependence on callsigns from `bin/commit-hook` (authored by epriestley, committed by epriestley). · Explain WhyFeb 18 2016, 12:50 AM

This revision was automatically updated to reflect the committed changes.

I think lint only warns you about too few arguments right now for some arcane but nontrivial-to-resolve technical reason that I don't recall offhand.

Oh, it's that we don't actually have a sprintf parser and just print the string and catch exceptions to see if it's valid:

try {
  xsprintf(null, null, $argv);
} catch (BadFunctionCallException $ex) {
  $this->raiseLintAtNode(
    $call,
    str_replace('xsprintf', $name, $ex->getMessage()));
} catch (InvalidArgumentException $ex) {
  // Ignore.
}

In D15293#175829, @epriestley wrote:
I think lint only warns you about too few arguments right now for some arcane but nontrivial-to-resolve technical reason that I don't recall offhand.

Oh, it's that we don't actually have a sprintf parser and just print the string and catch exceptions to see if it's valid:
try {
  xsprintf(null, null, $argv);
} catch (BadFunctionCallException $ex) {
  $this->raiseLintAtNode(
    $call,
    str_replace('xsprintf', $name, $ex->getMessage()));
} catch (InvalidArgumentException $ex) {
  // Ignore.
}

That is a pretty awesome reason, in a facebook-sort-of-way :P