| Message ID | a27d316855a833aa1726fc20c905dc40e41adf2f.1555061837.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Do not use abbreviated options in tests | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > + disallow_abbreviated_options = > + git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0); ... which means that the value of the environment variable follows the usual "true, yes and 1 all activate it"; very good, > diff --git a/t/README b/t/README > index 656288edce..9ed3051a1c 100644 > --- a/t/README > +++ b/t/README > @@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the > fetch-pack to not request sideband-all (even if the server advertises > sideband-all). > > +GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is > +the default when running tests), errors out when an abbreviated option > +is used. OK. > +test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' ' > + env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ > + test-tool parse-options --ye && > + test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \ > + test-tool parse-options --ye > +' The feature is activated for all tests unless otherwise noted, and the above marked the places that need to be "otherwise noted" in a reasonable way. Good. > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 562c57e685..f1a0fea4e1 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -57,6 +57,13 @@ fi > . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS > export PERL_PATH SHELL_PATH > > +# Disallow the use of abbreviated options in the test suite by default > +if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}" > +then > + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true > + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS > +fi If the original environment has it as flase, as it is set, the substitution will yield "isset" which is not an empty string, so we assign true. If the original environment is not set, or set to an empty, however, the substitution will yield an empty string, so we won't touch the variable. I am not sure in what situation the above behaviour becomes useful. Do you mean more like if test -z "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" then ... assignment ... fi IOW, we'll take an explicit GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false as a sign that the developer for whatever readon wants the disambiguation code in parse-options to kick in for all uses and allow a shortened option names? If on the other hand you are protecting our tests against those who casually have the environment set to false, because they know some of the scripts they use are sloppy *and* for whatever reason they anticipate that someday we will start to disallow abbrevated options by default? If so, an unconditional assignment of true would be more appropriate. I think I can agree with either of the two positions (i.e. we let those who explicitly want to decline do so, or we unconditionally make sure we catch issues in our tests), and I do not think of a third position that are different from these two and that would make sense. Between the two, I'd probably vote for the latter if I was pressed, but even then that is not a very strong preference. Thanks. I very much like the premise of this series, and the above hunk stood out in the range-diff in 0/8.
Junio C Hamano <gitster@pobox.com> writes: > Do you mean more like > ... > I think I can agree with either of the two positions... I am guessing from the earlier iteration that you wanted to say "unless it is given explicitly, we turn it on". As this last-minute style update that was botched, and a typofix in the proposed log message in 8/8, are the only differences, let me locally fix 8/8 up and replace it. It was very good that this round did not have to touch 1/8 which is shared with Denton's keep-base topic; otherwise replacing would have been a little bit messier. Here is what I'll use (unless it gets further replaced by v3 or later, that is). Thanks. -- >8 -- From: Johannes Schindelin <johannes.schindelin@gmx.de> Date: Fri, 12 Apr 2019 02:37:24 -0700 Subject: [PATCH v2bis 8/8] tests: disallow the use of abbreviated options (by default) Git's command-line parsers support uniquely abbreviated options, e.g. `git init --ba` would automatically expand `--ba` to `--bare`. This is a very convenient feature in every day life for Git users, in particular when tab completion is not available. However, it is not a good idea to rely on that in Git's test suite, as something that is a unique abbreviation of a command line option today might no longer be a unique abbreviation tomorrow. For example, if a future contribution added a new mode `git init --babyproofing` and a previously-introduced test case used the fact that `git init --ba` expanded to `git init --bare`, that future contribution would now have to touch seemingly unrelated tests just to keep the test suite from failing. So let's disallow abbreviated options in the test suite by default. Note: for ease of implementation, this patch really only touches the `parse-options` machinery: more and more hand-rolled option parsers are converted to use that internal API, and more and more scripts are converted to built-ins (naturally using the parse-options API, too), so in practice this catches most issues, and is definitely the biggest bang for the buck. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- parse-options.c | 9 +++++++++ t/README | 4 ++++ t/t0040-parse-options.sh | 14 +++++++++++++- t/test-lib.sh | 7 +++++++ 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/parse-options.c b/parse-options.c index cec74522e5..acc3a93660 100644 --- a/parse-options.c +++ b/parse-options.c @@ -6,6 +6,8 @@ #include "color.h" #include "utf8.h" +static int disallow_abbreviated_options; + #define OPT_SHORT 1 #define OPT_UNSET 2 @@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt( return get_value(p, options, all_opts, flags ^ opt_flags); } + if (disallow_abbreviated_options && (ambiguous_option || abbrev_option)) + die("disallowed abbreviated or ambiguous option '%.*s'", + (int)(arg_end - arg), arg); + if (ambiguous_option) { error(_("ambiguous option: %s " "(could be --%s%s or --%s%s)"), @@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix, { struct parse_opt_ctx_t ctx; + disallow_abbreviated_options = + git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0); + parse_options_start(&ctx, argc, argv, prefix, options, flags); switch (parse_options_step(&ctx, options, usagestr)) { case PARSE_OPT_HELP: diff --git a/t/README b/t/README index 656288edce..9ed3051a1c 100644 --- a/t/README +++ b/t/README @@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the fetch-pack to not request sideband-all (even if the server advertises sideband-all). +GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is +the default when running tests), errors out when an abbreviated option +is used. + Naming Tests ------------ diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index b8f366c442..800b3ea5f5 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -203,20 +203,24 @@ file: (not set) EOF test_expect_success 'unambiguously abbreviated option' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'unambiguously abbreviated option with "="' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ test-tool parse-options --expect="integer: 2" --int=2 ' test_expect_success 'ambiguously abbreviated option' ' - test_expect_code 129 test-tool parse-options --strin 123 + test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool parse-options --strin 123 ' test_expect_success 'non ambiguous option (after two options it abbreviates)' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ test-tool parse-options --expect="string: 123" --st 123 ' @@ -325,6 +329,7 @@ file: (not set) EOF test_expect_success 'negation of OPT_NONEG flags is not ambiguous' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ test-tool parse-options --no-ambig >output 2>output.err && test_must_be_empty output.err && test_cmp expect output @@ -370,4 +375,11 @@ test_expect_success '--no-verbose resets multiple verbose to 0' ' test-tool parse-options --expect="verbose: 0" -v -v -v --no-verbose ' +test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool parse-options --ye && + test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \ + test-tool parse-options --ye +' + test_done diff --git a/t/test-lib.sh b/t/test-lib.sh index 562c57e685..c14ebe68d3 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -57,6 +57,13 @@ fi . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS export PERL_PATH SHELL_PATH +# Disallow the use of abbreviated options in the test suite by default +if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}" +then + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS +fi + ################################################################ # It appears that people try to run tests without building... "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
Hi Junio, On Sun, 14 Apr 2019, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > diff --git a/t/test-lib.sh b/t/test-lib.sh > > index 562c57e685..f1a0fea4e1 100644 > > --- a/t/test-lib.sh > > +++ b/t/test-lib.sh > > @@ -57,6 +57,13 @@ fi > > . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS > > export PERL_PATH SHELL_PATH > > > > +# Disallow the use of abbreviated options in the test suite by default > > +if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}" > > +then > > + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true > > + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS > > +fi > > If the original environment has it as flase, as it is set, the > substitution will yield "isset" which is not an empty string, so we > assign true. > > If the original environment is not set, or set to an empty, however, > the substitution will yield an empty string, so we won't touch the > variable. > > I am not sure in what situation the above behaviour becomes useful. > > Do you mean more like > > if test -z "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" > then > ... assignment ... > fi Ævar rightfully pointed out that I introduced a new paradigm, so I switched to imitating the exact way `test-lib.sh` handles similar cases. Which is the `isset` method. Sure, I could do something differently, like `test -z`. But why? I think it is better to stay consistent with the existing checks. > IOW, we'll take an explicit GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false > as a sign that the developer for whatever readon wants the disambiguation > code in parse-options to kick in for all uses and allow a shortened option > names? Yes. Let's assume that there is a developer, or even a team (think e.g. VFS for Git), working on some long-running features that should be eventually contributed to Git, but that are still in flux, and through a merge with the current Git version, they get this change, and it breaks 50 of their test scripts. This is the scenario I wanted to help, however unlikely that is ;-) Of course, eventually they'd want to fix their test scripts. But maybe right now is not such a great time, so let's let them override the new behavior. > If on the other hand you are protecting our tests against those > who casually have the environment set to false, because they know > some of the scripts they use are sloppy *and* for whatever reason > they anticipate that someday we will start to disallow abbrevated > options by default? If so, an unconditional assignment of true > would be more appropriate. > > I think I can agree with either of the two positions (i.e. we let > those who explicitly want to decline do so, or we unconditionally > make sure we catch issues in our tests), and I do not think of a > third position that are different from these two and that would make > sense. Between the two, I'd probably vote for the latter if I was > pressed, but even then that is not a very strong preference. > > Thanks. I very much like the premise of this series, and the above > hunk stood out in the range-diff in 0/8. Thank you! Dscho
Hi Junio, On Mon, 15 Apr 2019, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > > Do you mean more like > > ... > > I think I can agree with either of the two positions... > > I am guessing from the earlier iteration that you wanted to say > "unless it is given explicitly, we turn it on". > > As this last-minute style update that was botched, and a typofix in > the proposed log message in 8/8, are the only differences, let me > locally fix 8/8 up and replace it. Sure. I still would like the `isset` thing, as it makes things more consistent, but I'll not fight for it. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > Hi Junio, > > On Mon, 15 Apr 2019, Junio C Hamano wrote: > >> Junio C Hamano <gitster@pobox.com> writes: >> >> >> > Do you mean more like >> > ... >> > I think I can agree with either of the two positions... >> >> I am guessing from the earlier iteration that you wanted to say >> "unless it is given explicitly, we turn it on". >> >> As this last-minute style update that was botched, and a typofix in >> the proposed log message in 8/8, are the only differences, let me >> locally fix 8/8 up and replace it. > > Sure. I still would like the `isset` thing, as it makes things more > consistent, but I'll not fight for it. ${var:+isset} is fine. Instead of +# Disallow the use of abbreviated options in the test suite by default +if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}" +then + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS +fi + if you used if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}" then ... I won't object. After all, I know I introduced :+isset pattern to our shell script codebase as there are cases where it makes the result easier to follow. But the thing is that your patch had the polarity inverted. Where you must say "if this thing is not set, assign this value", you said "if this thing is set, assign this value", which was totally bogus. As long as that is corrected, that's fine. Having said that. When you check if the variable is set, use of the ":+isset" pattern makes the result often easier to follow by explicitly letting us compare with an explicit "isset" token, e.g. case ",${VAR1:+isset},${VAR2:+isset}," in *,isset,*) : at least one is set ;; *) : neither is set ;; esac This *does* make the code simpler and easier. But when checking for "is it not set?", you can compare with an explicit literal "" and that comparison is plenty clear enough. You won't get as much benefit as the "is it set?" test would out of the pattern. I would not say that it is pointless to use the ":+isset" pattern when checking "is it not set?", but it is very close.
Hi Junio, On Mon, 15 Apr 2019, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > On Mon, 15 Apr 2019, Junio C Hamano wrote: > > > >> Junio C Hamano <gitster@pobox.com> writes: > >> > >> > >> > Do you mean more like > >> > ... > >> > I think I can agree with either of the two positions... > >> > >> I am guessing from the earlier iteration that you wanted to say > >> "unless it is given explicitly, we turn it on". > >> > >> As this last-minute style update that was botched, and a typofix in > >> the proposed log message in 8/8, are the only differences, let me > >> locally fix 8/8 up and replace it. > > > > Sure. I still would like the `isset` thing, as it makes things more > > consistent, but I'll not fight for it. > > ${var:+isset} is fine. Instead of > > +# Disallow the use of abbreviated options in the test suite by default > +if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}" > +then > + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true > + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS > +fi > + > > if you used > > if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}" > then > ... > > I won't object. After all, I know I introduced :+isset pattern to > our shell script codebase as there are cases where it makes the > result easier to follow. > > But the thing is that your patch had the polarity inverted. Ah! I finally understand what you are saying. Took me a while, sorry. > Where you must say "if this thing is not set, assign this value", you > said "if this thing is set, assign this value", which was totally bogus. > As long as that is corrected, that's fine. Of course! It should be `-z` instead of `-n` there. > Having said that. > > When you check if the variable is set, use of the ":+isset" pattern > makes the result often easier to follow by explicitly letting us > compare with an explicit "isset" token, e.g. > > case ",${VAR1:+isset},${VAR2:+isset}," in > *,isset,*) : at least one is set ;; > *) : neither is set ;; > esac > > This *does* make the code simpler and easier. But when checking for > "is it not set?", you can compare with an explicit literal "" and > that comparison is plenty clear enough. You won't get as much > benefit as the "is it set?" test would out of the pattern. I would > not say that it is pointless to use the ":+isset" pattern when > checking "is it not set?", but it is very close. Well, there is also the subtelty that somebody might *want* to set that environment variable to the empty string (and obviously they would not deem it appropriate for us to override their choice). But I do have bigger fish to fry, so if you're fine with the `isset` thing (fixed, of course), please go for it. If you'd prefer the version without it, that's fine enough with me, too. Ciao, Dscho
diff --git a/parse-options.c b/parse-options.c index cec74522e5..acc3a93660 100644 --- a/parse-options.c +++ b/parse-options.c @@ -6,6 +6,8 @@ #include "color.h" #include "utf8.h" +static int disallow_abbreviated_options; + #define OPT_SHORT 1 #define OPT_UNSET 2 @@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt( return get_value(p, options, all_opts, flags ^ opt_flags); } + if (disallow_abbreviated_options && (ambiguous_option || abbrev_option)) + die("disallowed abbreviated or ambiguous option '%.*s'", + (int)(arg_end - arg), arg); + if (ambiguous_option) { error(_("ambiguous option: %s " "(could be --%s%s or --%s%s)"), @@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix, { struct parse_opt_ctx_t ctx; + disallow_abbreviated_options = + git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0); + parse_options_start(&ctx, argc, argv, prefix, options, flags); switch (parse_options_step(&ctx, options, usagestr)) { case PARSE_OPT_HELP: diff --git a/t/README b/t/README index 656288edce..9ed3051a1c 100644 --- a/t/README +++ b/t/README @@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the fetch-pack to not request sideband-all (even if the server advertises sideband-all). +GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is +the default when running tests), errors out when an abbreviated option +is used. + Naming Tests ------------ diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index b8f366c442..6f6f74bfe2 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -203,20 +203,24 @@ file: (not set) EOF test_expect_success 'unambiguously abbreviated option' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'unambiguously abbreviated option with "="' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ test-tool parse-options --expect="integer: 2" --int=2 ' test_expect_success 'ambiguously abbreviated option' ' - test_expect_code 129 test-tool parse-options --strin 123 + test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool parse-options --strin 123 ' test_expect_success 'non ambiguous option (after two options it abbreviates)' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ test-tool parse-options --expect="string: 123" --st 123 ' @@ -325,6 +329,7 @@ file: (not set) EOF test_expect_success 'negation of OPT_NONEG flags is not ambiguous' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ test-tool parse-options --no-ambig >output 2>output.err && test_must_be_empty output.err && test_cmp expect output @@ -370,4 +375,11 @@ test_expect_success '--no-verbose resets multiple verbose to 0' ' test-tool parse-options --expect="verbose: 0" -v -v -v --no-verbose ' +test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' ' + env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool parse-options --ye && + test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \ + test-tool parse-options --ye +' + test_done diff --git a/t/test-lib.sh b/t/test-lib.sh index 562c57e685..f1a0fea4e1 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -57,6 +57,13 @@ fi . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS export PERL_PATH SHELL_PATH +# Disallow the use of abbreviated options in the test suite by default +if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}" +then + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS +fi + ################################################################ # It appears that people try to run tests without building... "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null