arc set-config: warn about unknown keys
ClosedPublic

Authored by avivey on Feb 15 2017, 1:21 PM.

Details

Summary

See T12266. Warn when the user tries to set a value we will probably not read.

Test Plan

arc set-config foo bar, see fancy warning.

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.
avivey created this revision.Feb 15 2017, 1:21 PM
epriestley accepted this revision.Feb 15 2017, 1:29 PM
epriestley added inline comments.
src/workflow/ArcanistSetConfigWorkflow.php
79

I think you can make this translatable like this:

$warning = tsprintf(
  "**%s:** %s\n",
  pht('Warning'),
  pht('The configuration key ...'));
$console->writeErr("%s", $warning);
This revision is now accepted and ready to land.Feb 15 2017, 1:29 PM
avivey updated this revision to Diff 41740.Feb 15 2017, 1:38 PM

make translateable

This revision was automatically updated to reflect the committed changes.
joshuaspence added inline comments.
src/workflow/ArcanistSetConfigWorkflow.php
103

Hmm, now these strings aren't translatable.

I just assumed that writeOut() and friends are a flavor of pht()...

pht() is the only thing that does translations right now.

Omitting tsprintf() means that ANSI stuff won't be escaped. It doesn't really matter here since the string is controlled by the user, but there are quasi-attacks where users create a task or revision with a lot of terminal control characters in it so that arc list or arc todo or whatever potentially take over your terminal. tsprintf() avoids that.

The overall state of affairs isn't great right now since the minimal amount of code you need to write the word "hello" to stderr "correctly" (so it can be translated and is ANSI-safe) is this big mess:

$console->writeErr(
  "%s",
  tsprintf(
    "%s\n",
    pht('hello')));

This is pretty silly, but since pht() needs to be statically extracted we can't easily get rid of it, and merging the $console stuff with tsprintf() is sort of half-pointless if we don't go all the way to try to get rid of the pht() too.

There's also at least some kind of argument that "\n" and ANSI sequences shouldn't be translatable: at best, they're probably pretty confusing.