Do not use abbreviated options in tests
cover Do not use abbreviated options in tests
We do not want to have tests that need to be changed by completely unrelated patch series, just because abbreviations that used to be unique are being made non-unique by said patch series.
I stumbled over this while investigating the test failures in dl/rebase-i-keep-base
[https://dev.azure.com/gitgitgadget/git/_build/results?buildId=5482&view=ms.vss-test-web.build-test-results-tab]
: the new --keep-base option makes the --keep abbreviation of --keep-empty in t5407 non-unique, which causes the test suite to fail.
Johannes Schindelin (8):
tests (rebase): spell out the --keep-empty option
tests (rebase): spell out the --force-rebase option
t7810: do not abbreviate --no-exclude-standard nor --invert-match
t5531: avoid using an abbreviated option
tests (push): do not abbreviate the --follow-tags option tests (status): spell out the --find-renames option in full tests (pack-objects): use the full, unabbreviated --revs option
tests: disallow the use of abbreviated options (by default)
parse-options.c | 9 ++++++ t/README | 4 +++ t/t0040-parse-options.sh | 7 +++- t/t3415-rebase-autosquash.sh | 2 +- t/t3430-rebase-merges.sh | 4 +-- t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++------------- t/t5407-post-rewrite-hook.sh | 4 +-- t/t5516-fetch-push.sh | 4 +-- t/t5531-deep-submodule-push.sh | 2 +- t/t7525-status-rename.sh | 8 ++--- t/t7810-grep.sh | 16 +++++----- t/test-lib.sh | 6 ++++ 12 files changed, 67 insertions(+), 43 deletions(-)
base-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-167%2Fdscho%2Fdisallow-abbreviated-options-in-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-167/dscho/disallow-abbreviated-options-in-tests-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/167
Signature · 1 line
gitgitgadget
PATCH 1/8 tests (rebase): spell out the `--keep-empty` option
From: Johannes Schindelin <[email protected]>
This test wants to run git rebase with the --keep-empty option, but it really only spelled out --keep and trusted Git's option parsing to determine that this was a unique abbreviation of the real option.
However, Denton Liu contributed a patch series in
https://public-inbox.org/git/[email protected]/
that introduces a new git rebase option called --keep-base, which makes this previously unique abbreviation non-unique.
Whether this patch series is accepted or not, it is actually a bad practice to use abbreviated options in our test suite, because of the issue that those unique option names are not guaranteed to stay unique in the future.
So let's just not use abbreviated options in the test suite.
--- t/t5407-post-rewrite-hook.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t5407-post-rewrite-hook.sh +2 -2
@@ -131,7 +131,7 @@ test_expect_success 'git rebase -m --skip' ' test_expect_success 'git rebase with implicit use of interactive backend' ' git reset --hard D && clear_hook_input && - test_must_fail git rebase --keep --onto A B && + test_must_fail git rebase --keep-empty --onto A B && echo C > foo && git add foo && git rebase --continue &&
@@ -146,7 +146,7 @@ test_expect_success 'git rebase with implicit use of interactive backend' ' test_expect_success 'git rebase --skip with implicit use of interactive backend' ' git reset --hard D && clear_hook_input && - test_must_fail git rebase --keep --onto A B && + test_must_fail git rebase --keep-empty --onto A B && test_must_fail git rebase --skip && echo D > foo && git add foo &&
Signature · 1 line
gitgitgadget
PATCH 2/8 tests (rebase): spell out the `--force-rebase` option
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In quite a few test cases, we were sloppy and used the abbreviation --force, but we really should be precise in what we want to test.
--- t/t3415-rebase-autosquash.sh | 2 +- t/t3430-rebase-merges.sh | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
t/t3415-rebase-autosquash.sh +1 -1
@@ -277,7 +277,7 @@ test_expect_success 'autosquash with empty custom instructionFormat' ' ( set_cat_todo_editor && test_must_fail git -c rebase.instructionFormat= \ - rebase --autosquash --force -i HEAD^ >actual && + rebase --autosquash --force-rebase -i HEAD^ >actual && git log -1 --format="pick %h %s" >expect && test_cmp expect actual )
t/t3430-rebase-merges.sh +2 -2
@@ -271,7 +271,7 @@ test_expect_success 'root commits' ' EOF test_config sequence.editor \""$PWD"/replace-editor.sh\" && test_tick && - git rebase -i --force --root -r && + git rebase -i --force-rebase --root -r && test "Parsnip" = "$(git show -s --format=%an HEAD^)" && test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) && test $(git rev-parse second-root:second-root.t) = \
@@ -364,7 +364,7 @@ test_expect_success 'octopus merges' ' test_cmp_rev HEAD $before && test_tick && - git rebase -i --force -r HEAD^^ && + git rebase -i --force-rebase -r HEAD^^ && test "Hank" = "$(git show -s --format=%an HEAD)" && test "$before" != $(git rev-parse HEAD) && test_cmp_graph HEAD^^.. <<-\EOF
Signature · 1 line
gitgitgadget
PATCH 3/8 t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This script used abbreviated options, which is unnecessarily fragile.
--- t/t7810-grep.sh | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
t/t7810-grep.sh +8 -8
@@ -119,33 +119,33 @@ do test_cmp expected actual ' - test_expect_success "grep -w $L (with --column, --invert)" ' + test_expect_success "grep -w $L (with --column, --invert-match)" ' { echo ${HC}file:1:foo mmap bar echo ${HC}file:1:foo_mmap bar echo ${HC}file:1:foo_mmap bar mmap echo ${HC}file:1:foo mmap bar_mmap } >expected && - git grep --column --invert -w -e baz $H -- file >actual && + git grep --column --invert-match -w -e baz $H -- file >actual && test_cmp expected actual ' - test_expect_success "grep $L (with --column, --invert, extended OR)" ' + test_expect_success "grep $L (with --column, --invert-match, extended OR)" ' { echo ${HC}hello_world:6:HeLLo_world } >expected && - git grep --column --invert -e ll --or --not -e _ $H -- hello_world \ + git grep --column --invert-match -e ll --or --not -e _ $H -- hello_world \ >actual && test_cmp expected actual ' - test_expect_success "grep $L (with --column, --invert, extended AND)" ' + test_expect_success "grep $L (with --column, --invert-match, extended AND)" ' { echo ${HC}hello_world:3:Hello world echo ${HC}hello_world:3:Hello_world echo ${HC}hello_world:6:HeLLo_world } >expected && - git grep --column --invert --not -e _ --and --not -e ll $H -- hello_world \ + git grep --column --invert-match --not -e _ --and --not -e ll $H -- hello_world \ >actual && test_cmp expected actual '
@@ -1010,7 +1010,7 @@ test_expect_success 'outside of git repository' ' echo ".gitignore:.*o*" && cat ../expect.full } >../expect.with.ignored && - git grep --no-index --no-exclude o >../actual.full && + git grep --no-index --no-exclude-standard o >../actual.full && test_cmp ../expect.with.ignored ../actual.full ) '
@@ -1051,7 +1051,7 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' ' echo ".gitignore:.*o*" && cat ../expect.full } >../expect.with.ignored && - git -c grep.fallbackToNoIndex grep --no-exclude o >../actual.full && + git -c grep.fallbackToNoIndex grep --no-exclude-standard o >../actual.full && test_cmp ../expect.with.ignored ../actual.full ) '
Signature · 1 line
gitgitgadget
PATCH 4/8 t5531: avoid using an abbreviated option
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
It was probably just an oversight: the --recurse-submodules option puts the term "submodules" in the plural form, not the singular one.
To avoid future problems in case that another option is introduced that starts with the prefix --recurse-submodule, let's just fix this.
--- t/t5531-deep-submodule-push.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/t5531-deep-submodule-push.sh +1 -1
@@ -363,7 +363,7 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs ) && git add b && git commit -m "added submodule" && - git push --recurse-submodule=check origin master + git push --recurse-submodules=check origin master ) '
Signature · 1 line
gitgitgadget
PATCH 5/8 tests (push): do not abbreviate the `--follow-tags` option
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
We really want to spell out the option in the full form, to avoid any ambiguity that might be introduced by future patches.
--- t/t5516-fetch-push.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t5516-fetch-push.sh +2 -2
@@ -1370,7 +1370,7 @@ test_expect_success 'push does not follow tags by default' ' test_cmp expect actual ' -test_expect_success 'push --follow-tag only pushes relevant tags' ' +test_expect_success 'push --follow-tags only pushes relevant tags' ' mk_test testrepo heads/master && rm -fr src dst && git init src &&
@@ -1384,7 +1384,7 @@ test_expect_success 'push --follow-tag only pushes relevant tags' ' git tag -m "future" future && git checkout master && git for-each-ref refs/heads/master refs/tags/tag >../expect && - git push --follow-tag ../dst master + git push --follow-tags ../dst master ) && ( cd dst &&
Signature · 1 line
gitgitgadget
PATCH 6/8 tests (status): spell out the `--find-renames` option in full
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
To avoid future ambiguities, we really want to use full option names in the test suite. t7525-status-rename.sh used an abbreviated form of the --find-renames option, though, so let's change that.
--- t/t7525-status-rename.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
t/t7525-status-rename.sh +4 -4
@@ -84,7 +84,7 @@ test_expect_success 'status score=100%' ' test_i18ngrep "deleted:" actual && test_i18ngrep "new file:" actual && - git status --find-rename=100% >actual && + git status --find-renames=100% >actual && test_i18ngrep "deleted:" actual && test_i18ngrep "new file:" actual '
@@ -93,11 +93,11 @@ test_expect_success 'status score=01%' ' git status -M=01% >actual && test_i18ngrep "renamed:" actual && - git status --find-rename=01% >actual && + git status --find-renames=01% >actual && test_i18ngrep "renamed:" actual ' -test_expect_success 'copies not overridden by find-rename' ' +test_expect_success 'copies not overridden by find-renames' ' cp renamed copy && git add copy &&
@@ -105,7 +105,7 @@ test_expect_success 'copies not overridden by find-rename' ' test_i18ngrep "copied:" actual && test_i18ngrep "renamed:" actual && - git -c status.renames=copies status --find-rename=01% >actual && + git -c status.renames=copies status --find-renames=01% >actual && test_i18ngrep "copied:" actual && test_i18ngrep "renamed:" actual '
Signature · 1 line
gitgitgadget
PATCH 7/8 tests (pack-objects): use the full, unabbreviated `--revs` option
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
To use the singular form of a word, when the option wants the plural form (and quietly expands it because it thinks it was abbreviated), is an easy mistake to make, and t5317 contains almost two dozen of them.
However, using abbreviated options in tests is a bit fragile, so we will disallow use of abbreviated options in our test suite.
In preparation for this change, let's fixt5317-pack-objects-filter-objects.sh.
--- t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++------------- 1 file changed, 22 insertions(+), 22 deletions(-)
t/t5317-pack-objects-filter-objects.sh +22 -22
@@ -25,7 +25,7 @@ test_expect_success 'verify blob count in normal packfile' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r1 pack-objects --rev --stdout >all.pack <<-EOF && + git -C r1 pack-objects --revs --stdout >all.pack <<-EOF && HEAD EOF git -C r1 index-pack ../all.pack &&
@@ -39,7 +39,7 @@ test_expect_success 'verify blob count in normal packfile' ' ' test_expect_success 'verify blob:none packfile has no blobs' ' - git -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF && + git -C r1 pack-objects --revs --stdout --filter=blob:none >filter.pack <<-EOF && HEAD EOF git -C r1 index-pack ../filter.pack &&
@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' ' git -C r5 commit -m "foo" && del=$(git -C r5 rev-parse HEAD^{tree} | sed "s|..|&/|") && rm r5/.git/objects/$del && - test_must_fail git -C r5 pack-objects --rev --stdout 2>bad_tree <<-EOF && + test_must_fail git -C r5 pack-objects --revs --stdout 2>bad_tree <<-EOF && HEAD EOF grep "bad tree object" bad_tree
@@ -88,7 +88,7 @@ test_expect_success 'setup for tests of tree:0' ' ' test_expect_success 'verify tree:0 packfile has no blobs or trees' ' - git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF && + git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF && HEAD EOF git -C r1 index-pack ../commitsonly.pack &&
@@ -98,7 +98,7 @@ test_expect_success 'verify tree:0 packfile has no blobs or trees' ' test_expect_success 'grab tree directly when using tree:0' ' # We should get the tree specified directly but not its blobs or subtrees. - git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF && + git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF && HEAD: EOF git -C r1 index-pack ../commitsonly.pack &&
@@ -128,7 +128,7 @@ test_expect_success 'verify blob count in normal packfile' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout >all.pack <<-EOF && + git -C r2 pack-objects --revs --stdout >all.pack <<-EOF && HEAD EOF git -C r2 index-pack ../all.pack &&
@@ -142,7 +142,7 @@ test_expect_success 'verify blob count in normal packfile' ' ' test_expect_success 'verify blob:limit=500 omits all blobs' ' - git -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=500 >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -157,7 +157,7 @@ test_expect_success 'verify blob:limit=500 omits all blobs' ' ' test_expect_success 'verify blob:limit=1000' ' - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1000 >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -176,7 +176,7 @@ test_expect_success 'verify blob:limit=1001' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -194,7 +194,7 @@ test_expect_success 'verify blob:limit=10001' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -212,7 +212,7 @@ test_expect_success 'verify blob:limit=1k' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -230,7 +230,7 @@ test_expect_success 'verify explicitly specifying oversized blob in input' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF && HEAD $(git -C r2 rev-parse HEAD:large.10000) EOF
@@ -249,7 +249,7 @@ test_expect_success 'verify blob:limit=1m' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -302,7 +302,7 @@ test_expect_success 'verify blob count in normal packfile' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r3 pack-objects --rev --stdout >all.pack <<-EOF && + git -C r3 pack-objects --revs --stdout >all.pack <<-EOF && HEAD EOF git -C r3 index-pack ../all.pack &&
@@ -320,7 +320,7 @@ test_expect_success 'verify sparse:path=pattern1' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF && + git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF && HEAD EOF git -C r3 index-pack ../filter.pack &&
@@ -352,7 +352,7 @@ test_expect_success 'verify sparse:path=pattern2' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF && + git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF && HEAD EOF git -C r3 index-pack ../filter.pack &&
@@ -404,7 +404,7 @@ test_expect_success 'verify blob count in normal packfile' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r4 pack-objects --rev --stdout >all.pack <<-EOF && + git -C r4 pack-objects --revs --stdout >all.pack <<-EOF && HEAD EOF git -C r4 index-pack ../all.pack &&
@@ -423,7 +423,7 @@ test_expect_success 'verify sparse:oid=OID' ' sort >expected && oid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) && - git -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF && + git -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF && HEAD EOF git -C r4 index-pack ../filter.pack &&
@@ -441,7 +441,7 @@ test_expect_success 'verify sparse:oid=oid-ish' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF && + git -C r4 pack-objects --revs --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF && HEAD EOF git -C r4 index-pack ../filter.pack &&
@@ -470,19 +470,19 @@ test_expect_success 'setup r1 - delete loose blobs' ' ' test_expect_success 'verify pack-objects fails w/ missing objects' ' - test_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF + test_must_fail git -C r1 pack-objects --revs --stdout >miss.pack <<-EOF HEAD EOF ' test_expect_success 'verify pack-objects fails w/ --missing=error' ' - test_must_fail git -C r1 pack-objects --rev --stdout --missing=error >miss.pack <<-EOF + test_must_fail git -C r1 pack-objects --revs --stdout --missing=error >miss.pack <<-EOF HEAD EOF ' test_expect_success 'verify pack-objects w/ --missing=allow-any' ' - git -C r1 pack-objects --rev --stdout --missing=allow-any >miss.pack <<-EOF + git -C r1 pack-objects --revs --stdout --missing=allow-any >miss.pack <<-EOF HEAD EOF '
Signature · 1 line
gitgitgadget
PATCH 8/8 tests: disallow the use of abbreviated options (by default)
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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 modegit init --babyproofing and a previously-introduced test case used the fact that git init --ba expaneded 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.
--- parse-options.c | 9 +++++++++ t/README | 4 ++++ t/t0040-parse-options.sh | 7 ++++++- t/test-lib.sh | 6 ++++++ 4 files changed, 25 insertions(+), 1 deletion(-)
parse-options.c +9 -0
@@ -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 :
t/README +4 -0
@@ -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 ------------
t/t0040-parse-options.sh +6 -1
@@ -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
t/test-lib.sh +6 -0
@@ -57,6 +57,12 @@ fi . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS export PERL_PATH SHELL_PATH +# Disallow the use of abbreviated options in the test suite by default +test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || { + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS +} + ################################################################ # It appears that people try to run tests without building... "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
Signature · 1 line
gitgitgadget
Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)
Hi Johannes,
Thanks for catching this. Perhaps I should've been more diligent and ran the entire test suite before submitting but I was running low on batteries only ran the rebase-related tests.
Quoted 8 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]>
Git's command-line parsers support uniquely abbreviated options, e.g.
git init --bawould automatically expand--bato--bare.This is a very convenient feature in every day life for Git users, in particular when tab completion is not available.
Quoted 7 lines
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 --babyproofingand a previously-introduced test case used the fact thatgit init --baexpaneded togit init --bare, that future
s/expaneded/expanded/
Quoted 100 lines
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-optionsmachinery: 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 <[email protected]>--- parse-options.c | 9 +++++++++ t/README | 4 ++++ t/t0040-parse-options.sh | 7 ++++++- t/test-lib.sh | 6 ++++++ 4 files changed, 25 insertions(+), 1 deletion(-)parse-options.c +9
-0@@ -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 :t/README +4
-0@@ -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 ------------t/t0040-parse-options.sh +5
-1@@ -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 '
t/t0040-parse-options.sh file: (not set) · line 335
Quoted 7 lines
t/t0040-parse-options.sh +1
-0@@ -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
Would it make sense to include a test case to ensure that GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?
Thanks,
Denton
Quoted 19 lines
t/test-lib.sh +6
-0@@ -57,6 +57,12 @@ fi . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS export PERL_PATH SHELL_PATH +# Disallow the use of abbreviated options in the test suite by default +test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || { + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS +} + ################################################################ # It appears that people try to run tests without building... "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/nullSignature · 1 line
gitgitgadget
Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)
Quoted 12 lines · Denton Liu
Hi Johannes,
Thanks for catching this. Perhaps I should've been more diligent and ran the entire test suite before submitting but I was running low on batteries only ran the rebase-related tests.
Quoted 5 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]> [...]
(quoted diff) +0
-0@@ -325,6 +329,7 @@ file: (not set) EOF
Quoted 8 lines
t/t0040-parse-options.sh file: (not set) · line 335
Quoted 5 lines
t/t0040-parse-options.sh +1
-0test_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 outputWould it make sense to include a test case to ensure that
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONSis working properly?
To elaborate on this since one might wonder "but aren't these those tests?". I think you mean if we shouldn't have a "test_must_fail" test there that asserts that parsing the options will die with the default of GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true. Yeah, that makes sense, it's currently a blind spot that we just assume will keep working.
Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)
Hi,
Quoted 24 lines · Ævar Arnfjörð Bjarmason
Quoted 12 lines · Denton Liu
Hi Johannes,
Thanks for catching this. Perhaps I should've been more diligent and ran the entire test suite before submitting but I was running low on batteries only ran the rebase-related tests.
Quoted 5 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]> [...]
(quoted diff) +0
-0@@ -325,6 +329,7 @@ file: (not set) EOFQuoted 8 lines
t/t0040-parse-options.sh file: (not set) · line 335
Quoted 5 lines
t/t0040-parse-options.sh +1
-0test_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 outputWould it make sense to include a test case to ensure that
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONSis working properly?
Quoted 5 lines
To elaborate on this since one might wonder "but aren't these those tests?". I think you mean if we shouldn't have a "
test_must_fail" test there that asserts that parsing the options will die with the default of GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true. Yeah, that makes sense, it's currently a blind spot that we just assume will keep working.
Valid point. I added a test case in v2 (which I'll send out in a few moments...)
Thanks,
Dscho
Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)
In reply to Denton LiuHi Denton,
Quoted 3 lines · Denton Liu
Thanks for catching this. Perhaps I should've been more diligent and ran the entire test suite before submitting but I was running low on batteries only ran the rebase-related tests.
No worries.
Quoted 13 lines · Johannes Schindelin via GitGitGadget
Quoted 12 lines
From: Johannes Schindelin <[email protected]>
Git's command-line parsers support uniquely abbreviated options, e.g.
git init --bawould automatically expand--bato--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.
Quoted 5 lines
Quoted 3 lines
For example, if a future contribution added a new mode
git init --babyproofingand a previously-introduced test case used the fact thatgit init --baexpaneded togit init --bare, that futures/expaneded/expanded/
Thanks, fixed.
Quoted 11 lines
t/t0040-parse-options.sh file: (not set) · line 335
Quoted 8 lines
t/t0040-parse-options.sh +1
-0@@ -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 outputWould it make sense to include a test case to ensure that
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONSis working properly?
Yep, absolutely.
Thank you,
Dscho
Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)
In reply to Johannes Schindelin via GitGitGadgett/test-lib.sh fi · line 65
Quoted 6 lines · Johannes Schindelin via GitGitGadget
t/test-lib.sh +6
-0+# Disallow the use of abbreviated options in the test suite by default +test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || { + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS +} +
Just using the if test ...\nthen\nfi long-form would be consistent with our usual style & the rest of this file.
Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)
Hi Ævar,
Quoted 12 lines · Ævar Arnfjörð Bjarmason
t/test-lib.sh fi · line 65
Quoted 6 lines · Johannes Schindelin via GitGitGadget
t/test-lib.sh +6
-0+# Disallow the use of abbreviated options in the test suite by default +test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || { + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true + export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS +} +Just using the if test ...\nthen\nfi long-form would be consistent with our usual style & the rest of this file.
Yep, and we also use the ${...:+isset} dance, so I imitated that, too.
Thanks for reminding me that when in Rome, do as the Romans do, Dscho
cover allow for configuring option abbreviation + fix
In reply to Johannes Schindelin via GitGitGadgetThis is based on Johannes's just-submitted
https://public-inbox.org/git/[email protected]/
Some of this in my 1/2 could obviously be shortened if the "GIT_TEST_ABBREVIATED_OPTIONS" name was used by default, but I'll leave it up to discussion whether it's worth it to go for the route I took here, and if so to rebase the whole thing or not.
While I'm at it fix a bug I noticed a while ago in 2/2 related to the option abbreviation being silly and tripping over itself over an aliased option.
Ævar Arnfjörð Bjarmason (2):
parse-options: allow for configuring option abbreviation
parse-options: don't emit "ambiguous option" for aliases
Documentation/config/core.txt | 12 ++++++++++++ builtin/clone.c | 4 ++-- parse-options.c | 22 +++++++++++++++++----- parse-options.h | 2 ++ t/README | 4 ++-- t/t0040-parse-options.sh | 27 ++++++++++++++++++++++----- t/test-lib.sh | 6 +++--- 7 files changed, 60 insertions(+), 17 deletions(-)
Signature · 1 line
2.21.0.360.g471c308f928
PATCH 1/2 parse-options: allow for configuring option abbreviation
In reply to Johannes Schindelin via GitGitGadgetAdd a new core.abbreviatedOptions which corresponds to the newly added GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS test mode. This allows e.g. script authors to enact the same sort of paranoia about option abbreviation as our own test suite now uses by default.
I'm renaming GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS to GIT_TEST_ABBREVIATED_OPTIONS for consistency with the new configuration variable, and I've picked the shorter core.abbreviatedOptions instead of core.disallowAbbreviatedOptions because it's easier to reason about a variable that's true by default than about one that has a negation in its name.
While I'm at it, factor out the check for the environment variable into a parse_options_config() which similar to the code added in 62c23938fa ("tests: add a special setup where rebase.useBuiltin is off", 2018-11-14) will first check the environment variable, and then the configuration.
Using git_env_bool() means it's a bit of a pain to distinguish between the case where GIT_TEST_ABBREVIATED_OPTIONS is set to "true" or "false" v.s. being empty, hence the test_when_finished/export/sane_unset dance in the test that's being added, but I think that's more sane than introducing a "bool but not in the same way as the rest" GIT_TEST_* variable.
--- Documentation/config/core.txt | 12 ++++++++++++ parse-options.c | 19 +++++++++++++++---- t/README | 4 ++-- t/t0040-parse-options.sh | 22 +++++++++++++++++----- t/test-lib.sh | 6 +++--- 5 files changed, 49 insertions(+), 14 deletions(-)
Documentation/config/core.txt +12 -0
@@ -1,3 +1,15 @@ +core.abbreviatedOptions:: + Defaults to `true` which allows options to be abbreviated as + long as they aren't ambiguous, e.g. for linkgit:git-init[1] + the `--bare` option can be abbreviated as `--bar`, `--ba` or + even `--b` since no other option starts with those + prefixes. However, if such an option were added in the future + any use of these abbreviations would break. ++ +By setting this to false (e.g. in scripts) you can defend against such +future breakages by enforcing that options must always be fully +provided. + core.fileMode:: Tells Git if the executable bit of files in the working tree is to be honored.
parse-options.c +15 -4
@@ -6,7 +6,7 @@#include "color.h" #include "utf8.h" -static int disallow_abbreviated_options; +static int abbreviated_options =1 ;#define OPT_SHORT 1#define OPT_UNSET 2
@@ -346,7 +346,7 @@ 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)) +if (!abbreviated_options && (ambiguous_option || abbrev_option))die ("disallowed abbreviated or ambiguous option '%.*s'" , (int )(arg_end - arg), arg);
@@ -456,6 +456,17 @@ static void parse_options_check(const struct option *opts)exit (128 ); } +static void parse_options_config (void ) +{ +int env =git_env_bool ("GIT_TEST_ABBREVIATED_OPTIONS" ,-1 ); +if (env !=-1 ) { + abbreviated_options = env; +return ; + } +git_config_get_bool ("core.abbreviatedoptions" , &abbreviated_options); +return ; +} +void parse_options_start (struct parse_opt_ctx_t *ctx,int argc,const char **argv,const char *prefix,const struct option *options,int flags)
@@ -480,6 +491,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx, (flags & PARSE_OPT_KEEP_ARGV0))BUG ("Can't keep argv0 if you don't have it" );parse_options_check (options); +parse_options_config (); }static void show_negated_gitcomp (const struct option *opts,int nr_noopts)
@@ -714,9 +726,8 @@ 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_config ();parse_options_start (&ctx, argc, argv, prefix, options, flags);switch (parse_options_step (&ctx, options, usagestr)) {case PARSE_OPT_HELP :
t/README +2 -2
@@ -399,9 +399,9 @@ 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 +GIT_TEST_ABBREVIATED_OPTIONS=<boolean>, when false (which is the default when running tests), errors out when an abbreviated option -is used. +is used. Overrides the core.abbreviatedOptions setting. Naming Tests ------------
t/t0040-parse-options.sh +17 -5
@@ -203,27 +203,39 @@ file: (not set) EOF test_expect_success 'unambiguously abbreviated option' ' - GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + GIT_TEST_ABBREVIATED_OPTIONS=true \ 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 \ + GIT_TEST_ABBREVIATED_OPTIONS=true \ test-tool parse-options --expect="integer: 2" --int=2 ' test_expect_success 'ambiguously abbreviated option' ' - test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test_expect_code 129 env GIT_TEST_ABBREVIATED_OPTIONS=true \ test-tool parse-options --strin 123 ' test_expect_success 'non ambiguous option (after two options it abbreviates)' ' - GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + GIT_TEST_ABBREVIATED_OPTIONS=true \ test-tool parse-options --expect="string: 123" --st 123 ' +test_expect_success 'abbreviated options configured with core.abbreviatedOptions' ' + test_when_finished " + rm -rf A B C && + GIT_TEST_ABBREVIATED_OPTIONS=$GIT_TEST_ABBREVIATED_OPTIONS && + export GIT_TEST_ABBREVIATED_OPTIONS + " && + git init --bare A && + test_must_fail git init --ba B && + sane_unset GIT_TEST_ABBREVIATED_OPTIONS && + git -c core.abbreviatedOptions=true init --ba C +' + cat >typo.err <<\EOF error: did you mean `--boolean` (with two dashes ?) EOF
@@ -329,7 +341,7 @@ file: (not set) EOF test_expect_success 'negation of OPT_NONEG flags is not ambiguous' ' - GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + GIT_TEST_ABBREVIATED_OPTIONS=true \ test-tool parse-options --no-ambig >output 2>output.err && test_must_be_empty output.err && test_cmp expect output
t/test-lib.sh +3 -3
@@ -58,9 +58,9 @@ fi export PERL_PATH SHELL_PATH # Disallow the use of abbreviated options in the test suite by default -test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || { - GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true - export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS +test -n "$GIT_TEST_ABBREVIATED_OPTIONS" || { + GIT_TEST_ABBREVIATED_OPTIONS=false + export GIT_TEST_ABBREVIATED_OPTIONS } ################################################################
Signature · 1 line
2.21.0.360.g471c308f928
Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation
Documentation/config/core.txt line 11
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Documentation/config/core.txt +11
-0@@ -1,3 +1,15 @@ +core.abbreviatedOptions:: + Defaults to `true` which allows options to be abbreviated as + long as they aren't ambiguous, e.g. for linkgit:git-init[1] + the `--bare` option can be abbreviated as `--bar`, `--ba` or + even `--b` since no other option starts with those + prefixes. However, if such an option were added in the future + any use of these abbreviations would break. ++ +By setting this to false (e.g. in scripts) you can defend against such +future breakages by enforcing that options must always be fully +provided.
I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
export GIT_TEST_ABBREVIATED_OPTIONS
than for it to muck about with git-config or use "git -c core.abbreviatedOptions=false ..." everywhere. The commit message doesn't do a good enough job of justifying the configuration option over the environment variable.
Also, if this is now intended to be more general (aiding script writers) than just being for our test suite, then dropping "TEST" from the name seems warranted:
GIT_ABBREVIATED_OPTIONS
Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation
Quoted 28 lines · Eric Sunshine
Documentation/config/core.txt line 11
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Documentation/config/core.txt +11
-0@@ -1,3 +1,15 @@ +core.abbreviatedOptions:: + Defaults to `true` which allows options to be abbreviated as + long as they aren't ambiguous, e.g. for linkgit:git-init[1] + the `--bare` option can be abbreviated as `--bar`, `--ba` or + even `--b` since no other option starts with those + prefixes. However, if such an option were added in the future + any use of these abbreviations would break. ++ +By setting this to false (e.g. in scripts) you can defend against such +future breakages by enforcing that options must always be fully +provided.I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
exportGIT_TEST_ABBREVIATED_OPTIONSthan for it to muck about with git-config or use "git -c core.abbreviatedOptions=false ..." everywhere. The commit message doesn't do a good enough job of justifying the configuration option over the environment variable.
Quoted 5 lines
Also, if this is now intended to be more general (aiding script writers) than just being for our test suite, then dropping "TEST" from the name seems warranted:
GIT_ABBREVIATED_OPTIONS
If we want to make something user-configurable we tend to add config variables. The GIT_TEST_* variables are only intended for our own test suite, see t/README.
I don't mind documenting this, but it's a well-established pattern, so if we're going to describe how this works/why use one or the other it should probably be some other series to t/README and/or git-config.txt
We traditionally *only* expose this sort of thing to users via config, and not via env variables.
The config system is more flexible in every way. You can set it system-wide, user-wide, repo-wide etc., and if you want exactly the scope of an env variable you can do that too, just start your script with:
# These "''" quotes are not a mistake, it needs to be like this export GIT_CONFIG_PARAMETERS="'core.abbreviatedOptions=false'" git <some-cmd> [...]
So the reason we have GIT_TEST_* is pretty much because we can't just whitelist GIT_CONFIG_PARAMETERS for the test suite, and to make it obvious what test modes we have available.
Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation
Quoted 42 lines · Ævar Arnfjörð Bjarmason
Quoted 28 lines · Eric Sunshine
Documentation/config/core.txt line 11
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Documentation/config/core.txt +11
-0@@ -1,3 +1,15 @@ +core.abbreviatedOptions:: + Defaults to `true` which allows options to be abbreviated as + long as they aren't ambiguous, e.g. for linkgit:git-init[1] + the `--bare` option can be abbreviated as `--bar`, `--ba` or + even `--b` since no other option starts with those + prefixes. However, if such an option were added in the future + any use of these abbreviations would break. ++ +By setting this to false (e.g. in scripts) you can defend against such +future breakages by enforcing that options must always be fully +provided.I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
exportGIT_TEST_ABBREVIATED_OPTIONSthan for it to muck about with git-config or use "git -c core.abbreviatedOptions=false ..." everywhere. The commit message doesn't do a good enough job of justifying the configuration option over the environment variable.
Quoted 5 lines
Also, if this is now intended to be more general (aiding script writers) than just being for our test suite, then dropping "TEST" from the name seems warranted:
GIT_ABBREVIATED_OPTIONSIf we want to make something user-configurable we tend to add config variables. The GIT_TEST_* variables are only intended for our own test suite, see t/README.
Quoted 6 lines
I don't mind documenting this, but it's a well-established pattern, so if we're going to describe how this works/why use one or the other it should probably be some other series to t/README and/or git-config.txt
We traditionally *only* expose this sort of thing to users via config, and not via env variables.
If this is mostly useful for scripts then I agree with Eric an environment variable is the way to go. A configuration variable does not make it more convenient.
And no we don't only export via config. There are a bunch of public env variables in git.txt. "core" namespace is already very crowded. If this one is only rarely used, I'd rather not add a new config variable.
Signature · 1 line
Duy
Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation
Quoted 44 lines · Duy Nguyen
Quoted 42 lines · Ævar Arnfjörð Bjarmason
Quoted 28 lines · Eric Sunshine
Documentation/config/core.txt line 11
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Documentation/config/core.txt +11
-0@@ -1,3 +1,15 @@ +core.abbreviatedOptions:: + Defaults to `true` which allows options to be abbreviated as + long as they aren't ambiguous, e.g. for linkgit:git-init[1] + the `--bare` option can be abbreviated as `--bar`, `--ba` or + even `--b` since no other option starts with those + prefixes. However, if such an option were added in the future + any use of these abbreviations would break. ++ +By setting this to false (e.g. in scripts) you can defend against such +future breakages by enforcing that options must always be fully +provided.I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
exportGIT_TEST_ABBREVIATED_OPTIONSthan for it to muck about with git-config or use "git -c core.abbreviatedOptions=false ..." everywhere. The commit message doesn't do a good enough job of justifying the configuration option over the environment variable.
Quoted 5 lines
Also, if this is now intended to be more general (aiding script writers) than just being for our test suite, then dropping "TEST" from the name seems warranted:
GIT_ABBREVIATED_OPTIONSIf we want to make something user-configurable we tend to add config variables. The GIT_TEST_* variables are only intended for our own test suite, see t/README.
Quoted 6 lines
Quoted 6 lines
I don't mind documenting this, but it's a well-established pattern, so if we're going to describe how this works/why use one or the other it should probably be some other series to t/README and/or git-config.txt
We traditionally *only* expose this sort of thing to users via config, and not via env variables.
FWIW I replied so quickly to this with patches since I had something WIP to do this, it's annoyed me as a user in the past that I couldn't turn this off, IIRC some upgrade of git broke my "bad" muscle memory / scripts.
Quoted 3 lines
If this is mostly useful for scripts then I agree with Eric an environment variable is the way to go. A configuration variable does not make it more convenient.
I think both of you might be assuming that when you want to configure something it's as easy to tweak every run time environment (set an env var) as it is to set a global config. See the trace2.* config discussion for similar use-cases.
Quoted 4 lines
And no we don't only export via config. There are a bunch of public env variables in git.txt. "core" namespace is already very crowded. If this one is only rarely used, I'd rather not add a new config variable.
I don't see how a new "abbreviatedOptions" is plausibly going to crowd out anything else, sounds pretty unambiguous to me.
By my count out of the the existing GIT_* variables in git.txt around 1/3 are already configurable via config, another 1/3 (all the GIT_TRACE stuff) is something we've wanted to have configurable in the past (but nobody's gotten around to writing patches for).
I think it's fair to say that when we normally add user-configurable stuff we do it as config, not as new env vars.
Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation
Quoted 2 lines · Ævar Arnfjörð Bjarmason
I don't see how a new "abbreviatedOptions" is plausibly going to crowd out anything else, sounds pretty unambiguous to me.
By crowded I mean a lot of configuration variables under "core"
Quoted 7 lines
By my count out of the the existing GIT_* variables in git.txt around 1/3 are already configurable via config, another 1/3 (all the
GIT_TRACEstuff) is something we've wanted to have configurable in the past (but nobody's gotten around to writing patches for).I think it's fair to say that when we normally add user-configurable stuff we do it as config, not as new env vars.
I disagree that not every configuration knob has to be a configuration variable, especially when core.* more or less becomes a dictionary that you can't really read anymore (unless you know the key to look for). But I see you're dead set on adding config vars. Go ahead.
Signature · 1 line
Duy
Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation
Quoted 12 lines · Duy Nguyen
Quoted 2 lines · Ævar Arnfjörð Bjarmason
I don't see how a new "abbreviatedOptions" is plausibly going to crowd out anything else, sounds pretty unambiguous to me.
By crowded I mean a lot of configuration variables under "core"
Quoted 5 lines
By my count out of the the existing GIT_* variables in git.txt around 1/3 are already configurable via config, another 1/3 (all the
GIT_TRACEstuff) is something we've wanted to have configurable in the past (but nobody's gotten around to writing patches for).
Quoted 7 lines
Quoted 2 lines
I think it's fair to say that when we normally add user-configurable stuff we do it as config, not as new env vars.
I disagree that not every configuration knob has to be a configuration variable, especially when core.* more or less becomes a dictionary that you can't really read anymore (unless you know the key to look for). But I see you're dead set on adding config vars. Go ahead.
I don't mean to pile on in this thread, and cards on the table: In terms of stuff I can be bothered to write a patch for and stuff I *really* want in git, this thing is maybe on a 2/10 out of a 1-10 scale.
I.e. I'd use it, and it would probably save my ass *somewhere* down the line, the motivation was being bitten in the past, so once I spotted Johannes's series I was reminded to clean it up & submit it. But I wouldn't care enough to carry a forked git with this (unlike some other stuff).
I just wanted to elaborate *in general* on the question of "why config and not env".
I don't think everything we find reason to add a knob for needs to be a config variable. E.g. I can't think of a reason for why the GIT_*_PATHSPECS variables should be. That seems like the sort of thing you'd only tweak for a *specific* git invocation. I wouldn't oppose it being config if someone had a compelling reason & patch, but can't think of one myself.
What I wanted to get across in the above replies to you/Eric, but probably did a bad job of is that separate & aside from the merits of any given new config variable there's the problem of being a "zookeeper" with large git installations.
I.e. you don't even pick the animals, or feed them, but if the zoo's on fire or overflowing with poop it's your problem.
So I have some servers where I'm in charge of maintaining/upgrading git, and it's on me if crap breaks in the aggregate, but I'm not writing anyone's individual scripts for them, and the runtime envs for those might be varied.
It's for those sorts of cases where having the flexibility of turning something on either in the env or via system-wide or user-wide config is handy. I.e. in this case I might say:
"You can run your scripts, but you'll be in 'strict mode' and need to spell out your options, because I'm not getting out of bed if your random job using git breaks because you were lazy and did a 'git init --ba'".
I don't think this is a use-case that's obvious, so it's worth elaborating on it. I suspect a lot of devs/users on this list have advanced git use-cases, but they're all ones where the git processes they care about/need to maintain run as a child of their login shell, or equivalent. In that case the config v.s. env var distinction mostly doesn't matter.
Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation
In reply to Eric SunshineQuoted 11 lines · Eric Sunshine
I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
exportGIT_TEST_ABBREVIATED_OPTIONSthan for it to muck about with git-config or use "git -c core.abbreviatedOptions=false ..." everywhere. The commit message doesn't do a good enough job of justifying the configuration option over the environment variable.
Absolutely.
One thing that big brotherly types would find config attractive is to install centrally managed /etc/gitconfig so that they can tell the tracing machinery to log all git command invocations centrally; with environment only system, it is not easy to arrange.
But this one under discussion is probably a use case that is at the other end of the extreme from that.
Users (1) may use scripts written by third-parties, (2) may also develop their own scripts, and (3) even may run Git commands interactively.
It is a laudable goal to introduce a mechanism to "notice use of abbreviated options that happen to be unique right now but may not stay unique forever and warn against it". But (2) among the above three uses is the only use case that wants the protection, which makes the configuration a poor fit for the purpose.
Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation
Hi Junio,
Quoted 14 lines · Junio C Hamano
Quoted 11 lines · Eric Sunshine
I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
exportGIT_TEST_ABBREVIATED_OPTIONSthan for it to muck about with git-config or use "git -c core.abbreviatedOptions=false ..." everywhere. The commit message doesn't do a good enough job of justifying the configuration option over the environment variable.
Quoted 6 lines
Absolutely.
One thing that big brotherly types would find config attractive is to install centrally managed
/etc/gitconfigso that they can tell the tracing machinery to log all git command invocations centrally; with environment only system, it is not easy to arrange.
I think that in this instance, we should use the fact that we know Ævar well, and refrain from characterizing him as a Big Brotherly type.
From my reading, it looks like Ævar just wants to avoid being woken up for a Live Site Incident that is caused by a violation of Postel's Law: be accepting in your input, but stringent in your output. And in this case, the scripts by their colleagues is the output that should be more stringent, and enforcing the stringency via a system-wide config variable is as legit as our instistence that user.name and user.email must be provided if you want to create a commit.
And I would not have the faintest problem with adding that patch to introduce the core.* setting to that end (which would be the right section, too, even if there are already so many core.* settings).
Having said that, I get the strong impression that there is a rather violent pushback against this (which I don't understand). Combined with the fact that it would protect only against a tiny fraction of "git upgrade problems", I'm getting more into the "well, then let's just not bother" camp.
Ciao,
Dscho
PATCH 2/2 parse-options: don't emit "ambiguous option" for aliases
In reply to Johannes Schindelin via GitGitGadgetChange the option parsing machinery so that e.g. "clone --recurs ..." doesn't error out because "clone" understands both "--recursive" and "--recurse-submodules" to mean the same thing.
Initially "clone" just understood --recursive until the --recurses-submodules alias was added in ccdd3da652 ("clone: Add the --recurse-submodules option as alias for --recursive", 2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to optionally take a pathspec", 2017-03-17) the longer form has been promoted to the default.
But due to the way the options parsing machinery works this resulted in the rather absurd situation of:
$ git clone --recurs [...]
error: ambiguous option: recurs (could be --recursive or --recurse-submodules)Let's re-use the PARSE_OPT_NOCOMPLETE flag to mean "this option doesn't contribute to abbreviation ambiguity". I was going to add a new PARSE_OPT_NOABBREV flag, but it makes sense just to re-use PARSE_OPT_NOCOMPLETE.
--- builtin/clone.c | 4 ++-- parse-options.c | 3 ++- parse-options.h | 2 ++ t/t0040-parse-options.sh | 5 +++++ 4 files changed, 11 insertions(+), 3 deletions(-)
builtin/clone.c +2 -2
@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = { N_("setup as shared repository")), {OPTION_CALLBACK ,0 ,"recursive" , &option_recurse_submodules,N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), -PARSE_OPT_OPTARG |PARSE_OPT_HIDDEN , recurse_submodules_cb, - (intptr_t)"." }, +PARSE_OPT_OPTARG |PARSE_OPT_HIDDEN |PARSE_OPT_NOCOMPLETE , + recurse_submodules_cb, (intptr_t)"." }, { OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
parse-options.c +2 -1
@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(if (!rest) {/* abbreviated? */ if (!(p->flags &PARSE_OPT_KEEP_UNKNOWN ) && - !strncmp (long_name, arg, arg_end - arg)) { + !strncmp (long_name, arg, arg_end - arg) && + !(options->flags &PARSE_OPT_NOCOMPLETE )) { is_abbreviated:if (abbrev_option) {/*
parse-options.h +2 -0
@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,* Useful for options with multiple parameters. * PARSE_OPT_NOCOMPLETE: by default all visible options are completable * by git-completion.bash. This option suppresses that. +* Will also skip this option when abbreviation is +* considered. See core.abbreviatedOptions. * PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to * complete an option as --name= not --name even if * the option takes optional argument.
t/t0040-parse-options.sh +5 -0
@@ -236,6 +236,11 @@ test_expect_success 'abbreviated options configured with core.abbreviatedOptions git -c core.abbreviatedOptions=true init --ba C ' +test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' ' + test_when_finished "rm -rf A" && + GIT_TEST_ABBREVIATED_OPTIONS=true git clone --recurs . A +' + cat >typo.err <<\EOF error: did you mean `--boolean` (with two dashes ?) EOF
Signature · 1 line
2.21.0.360.g471c308f928
PATCH v2 1/1 parse-options: don't emit "ambiguous option" for aliases
Change the option parsing machinery so that e.g. "clone --recurs ..." doesn't error out because "clone" understands both "--recursive" and "--recurse-submodules" to mean the same thing.
Initially "clone" just understood --recursive until the --recurses-submodules alias was added in ccdd3da652 ("clone: Add the --recurse-submodules option as alias for --recursive", 2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to optionally take a pathspec", 2017-03-17) the longer form has been promoted to the default.
But due to the way the options parsing machinery works this resulted in the rather absurd situation of:
$ git clone --recurs [...]
error: ambiguous option: recurs (could be --recursive or --recurse-submodules)Let's re-use the PARSE_OPT_NOCOMPLETE flag to mean "this option doesn't contribute to abbreviation ambiguity". I was going to add a new PARSE_OPT_NOABBREV flag, but it makes sense just to re-use PARSE_OPT_NOCOMPLETE.
---
See
https://public-inbox.org/git/[email protected]/
for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2 seems like a no-brainer bugfix.
It conflicted with some recently-landed stuff in 'master', but now cleanly applies to it and 'pu', and with pu's GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS.
builtin/clone.c | 4 ++-- parse-options.c | 3 ++- parse-options.h | 2 ++ t/t0040-parse-options.sh | 5 +++++ 4 files changed, 11 insertions(+), 3 deletions(-)
builtin/clone.c +2 -2
@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = { N_("setup as shared repository")), {OPTION_CALLBACK ,0 ,"recursive" , &option_recurse_submodules,N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), -PARSE_OPT_OPTARG |PARSE_OPT_HIDDEN , recurse_submodules_cb, - (intptr_t)"." }, +PARSE_OPT_OPTARG |PARSE_OPT_HIDDEN |PARSE_OPT_NOCOMPLETE , + recurse_submodules_cb, (intptr_t)"." }, { OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
parse-options.c +2 -1
@@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt(if (!rest) {/* abbreviated? */ if (!(p->flags &PARSE_OPT_KEEP_UNKNOWN ) && - !strncmp (long_name, arg, arg_end - arg)) { + !strncmp (long_name, arg, arg_end - arg) && + !(options->flags &PARSE_OPT_NOCOMPLETE )) { is_abbreviated:if (abbrev_option) {/*
parse-options.h +2 -0
@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,* Useful for options with multiple parameters. * PARSE_OPT_NOCOMPLETE: by default all visible options are completable * by git-completion.bash. This option suppresses that. +* Will also skip this option when abbreviation is +* considered. * PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to * complete an option as --name= not --name even if * the option takes optional argument.
t/t0040-parse-options.sh +5 -0
@@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' ' test-tool parse-options --expect="string: 123" --st 123 ' +test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' ' + test_when_finished "rm -rf A" && + git clone --recurs . A +' + cat >typo.err <<\EOF error: did you mean `--boolean` (with two dashes ?) EOF
Signature · 1 line
2.21.0.593.g511ec345e18
Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases
Quoted 46 lines · Ævar Arnfjörð Bjarmason
Change the option parsing machinery so that e.g. "clone --recurs ..." doesn't error out because "clone" understands both "--recursive" and "--recurse-submodules" to mean the same thing.
Initially "clone" just understood --recursive until the --recurses-submodules alias was added in
ccdd3da652("clone: Add the --recurse-submodules option as alias for --recursive", 2010-11-04). Sincebb62e0a99f("clone: teach --recurse-submodules to optionally take a pathspec", 2017-03-17) the longer form has been promoted to the default.But due to the way the options parsing machinery works this resulted in the rather absurd situation of:
$ git clone --recurs [...] error: ambiguous option: recurs (could be --recursive or --recurse-submodules)Let's re-use the
PARSE_OPT_NOCOMPLETEflag to mean "this option doesn't contribute to abbreviation ambiguity". I was going to add a newPARSE_OPT_NOABBREVflag, but it makes sense just to re-usePARSE_OPT_NOCOMPLETE.Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>---
See
https://public-inbox.org/git/[email protected]/
for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2 seems like a no-brainer bugfix.It conflicted with some recently-landed stuff in 'master', but now cleanly applies to it and 'pu', and with pu's
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS.builtin/clone.c | 4 ++-- parse-options.c | 3 ++- parse-options.h | 2 ++ t/t0040-parse-options.sh | 5 +++++ 4 files changed, 11 insertions(+), 3 deletions(-)builtin/clone.c +0
-0
builtin/clone.c static struct option builtin_clone_options[] = { · line 103
Quoted 6 lines
builtin/clone.c +1
-2@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = { N_("setup as shared repository")), {OPTION_CALLBACK ,0 ,"recursive" , &option_recurse_submodules,N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), -PARSE_OPT_OPTARG |PARSE_OPT_HIDDEN , recurse_submodules_cb, - (intptr_t)"." }, + PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
What happens if someone adds --recursive-hard? --recursi then resolving to --recursive-hard sounds wrong.
But on the other hand I can see it's a bit more work to teach parse-options OPT_ALIAS to say "--recursive is just an alias of --recurse-submodules" and chances of --recursive-hard coming up are probably very low.
So I don't know but I thought I should point out.
Quoted 50 lines
builtin/clone.c +1
-0+ recurse_submodules_cb, (intptr_t)"." }, {OPTION_CALLBACK ,0 ,"recurse-submodules" , &option_recurse_submodules,N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },parse-options.c +2
-1@@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt(if (!rest) {/* abbreviated? */ if (!(p->flags &PARSE_OPT_KEEP_UNKNOWN ) && - !strncmp (long_name, arg, arg_end - arg)) { + !strncmp (long_name, arg, arg_end - arg) && + !(options->flags &PARSE_OPT_NOCOMPLETE )) { is_abbreviated:if (abbrev_option) {/* parse-options.h +2
-0@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,* Useful for options with multiple parameters. * PARSE_OPT_NOCOMPLETE: by default all visible options are completable * by git-completion.bash. This option suppresses that. +* Will also skip this option when abbreviation is +* considered. * PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to * complete an option as --name= not --name even if * the option takes optional argument. t/t0040-parse-options.sh +5
-1@@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' ' test-tool parse-options --expect="string: 123" --st 123 ' +test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' ' + test_when_finished "rm -rf A" && + git clone --recurs . A +' + cat >typo.err <<\EOF error: did you mean `--boolean` (with two dashes ?) EOF --2.21.0.593.g511ec345e18
Signature · 1 line
Duy
Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases
Quoted 8 lines · Duy Nguyen
builtin/clone.c static struct option builtin_clone_options[] = { · line 103
Quoted 5 lines
{
OPTION_CALLBACK, 0, "recursive",&option_recurse_submodules,
N_("pathspec"),N_("initialize submodules in the clone"),builtin/clone.c +1
-2-PARSE_OPT_OPTARG |PARSE_OPT_HIDDEN , recurse_submodules_cb, - (intptr_t)"." }, + PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,What happens if someone adds --recursive-hard? --recursi then resolving to --recursive-hard sounds wrong.
That was exactly my initial reaction. Or "recurse-submodules" gets renamed away, and "recommend" gets added---now "--rec" is still not ambiguous as "recursive" is marked not to participate in the disambiguation (I think OPT_NOCOMPLETE should at least be renamed to OPT_NO_DISAMBIGUATION or something---if we were to use it for the purpose of marking an option as "not participating in disambiguation", but I am fairly negative on the approach).
And my initial reaction was followed by "don't we want a more explicit link only between recursive and recurse-submodules?", which exactly matches what you said below ;-)
Quoted 4 lines
But on the other hand I can see it's a bit more work to teach parse-options
OPT_ALIASto say "--recursive is just an alias of --recurse-submodules" and chances of --recursive-hard coming up are probably very low.
The "bit more work" is something that is worth doing in this case, I think.
Thanks.
Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases
Quoted 7 lines · Junio C Hamano
Quoted 4 lines
But on the other hand I can see it's a bit more work to teach parse-options
OPT_ALIASto say "--recursive is just an alias of --recurse-submodules" and chances of --recursive-hard coming up are probably very low.The "bit more work" is something that is worth doing in this case, I think.
I had a look at it. Linking two options together is not exactly easy because the way we organize option data. And I also had difficulty defining the exact semantics of this OPT_ALIAS.
So an alternative is simply outsource the ambiguity decision back to git-clone. If the same situation appears again elsewhere, we'll need to sit back and fix it for real. But this way we don't potentially introduce any new traps.
parse_options_extended() also makes it easier to extend parseopt functionality without changing all call sites. One thing I have in mind, which could maybe use this interface, is to remove parse_options_{start,step,end} from public API, avoid expose the parse_options_step internal result code. The caller just need to pass a callback to handle unknown options.
-- 8< --
builtin/clone.c +29 -2
@@ -145,6 +145,29 @@ static struct option builtin_clone_options[] = {OPT_END () }; +/* +* Avoid ambiguation error between --recursive and --recurse-submodule +* because they are the same. --recurs can be expanded to any of them +* and it still works. +*/ +static int is_abbrev_ambiguous (const struct option *prev, +const struct option *next) +{ +const struct option *opts[] = { prev, next }; +int i, found =0 ; + +for (i =0 ; i <ARRAY_SIZE (opts); i++) { +if (!opts[i]->long_name) +continue ; +if (!strcmp (opts[i]->long_name,"recursive" )) + found |=1 <<0 ; +if (!strcmp (opts[i]->long_name,"recurse-submodules" )) + found |=1 <<1 ; + } + +return found !=3 ; +} +static const char *get_repo_path_1 (struct strbuf *path,int *is_bundle) {static char *suffix[] = {"/.git" ,"" ,".git/.git" ,".git" };
@@ -905,14 +928,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)struct remote *remote;int err =0 , complete_refs_before_fetch =1 ;int submodule_progress; +struct parseopt_options parseopts = {0 };struct argv_array ref_prefixes =ARGV_ARRAY_INIT ; fetch_if_missing =0 ;packet_trace_identity ("clone" ); - argc =parse_options (argc, argv, prefix, builtin_clone_options, - builtin_clone_usage,0 ); + parseopts.usagestr = builtin_clone_usage; + parseopts.is_abbrev_ambiguous = is_abbrev_ambiguous; + argc =parse_options_extended (argc, argv, prefix, + builtin_clone_options, + &parseopts);if (argc >2 ) usage_msg_opt(_("Too many arguments."),
parse-options.c +27 -4
@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(if (!(p->flags &PARSE_OPT_KEEP_UNKNOWN ) && !strncmp (long_name, arg, arg_end - arg)) { is_abbreviated: -if (abbrev_option) { +if (abbrev_option && + p->is_abbrev_ambiguous (abbrev_option, options)) {/* * If this is abbreviated, it is * ambiguous. So when there is no
@@ -450,6 +451,12 @@ static void parse_options_check(const struct option *opts)exit (128 ); } +static int abbrev_is_always_ambiguous (const struct option *prev, +const struct option *next) +{ +return 1 ; +} +void parse_options_start (struct parse_opt_ctx_t *ctx,int argc,const char **argv,const char *prefix,const struct option *options,int flags)
@@ -466,6 +473,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx, ctx->prefix = prefix; ctx->cpidx = ((flags &PARSE_OPT_KEEP_ARGV0 ) !=0 ); ctx->flags = flags; + ctx->is_abbrev_ambiguous = abbrev_is_always_ambiguous;if ((flags &PARSE_OPT_KEEP_UNKNOWN ) && (flags &PARSE_OPT_STOP_AT_NON_OPTION ) && !(flags &PARSE_OPT_ONE_SHOT ))
@@ -702,13 +710,17 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)return ctx->cpidx + ctx->argc; } -int parse_options (int argc,const char **argv,const char *prefix, -const struct option *options,const char *const usagestr[], -int flags) +int parse_options_extended (int argc,const char **argv,const char *prefix, +const struct option *options, +const struct parseopt_options *parseopts) {struct parse_opt_ctx_t ctx; +const char *const * usagestr = parseopts->usagestr; +int flags = parseopts->flags;parse_options_start (&ctx, argc, argv, prefix, options, flags); +if (parseopts->is_abbrev_ambiguous) + ctx.is_abbrev_ambiguous = parseopts->is_abbrev_ambiguous;switch (parse_options_step (&ctx, options, usagestr)) {case PARSE_OPT_HELP :case PARSE_OPT_ERROR :
@@ -734,6 +746,17 @@ int parse_options(int argc, const char **argv, const char *prefix,return parse_options_end (&ctx); } +int parse_options (int argc,const char **argv,const char *prefix, +const struct option *options,const char *const usagestr[], +int flags) +{ +struct parseopt_options parseopts = {0 }; + + parseopts.flags = flags; + parseopts.usagestr = usagestr; +return parse_options_extended (argc, argv, prefix, options, &parseopts); +} +static int usage_argh (const struct option *opts,FILE *outfile) {const char *s;
parse-options.h +12 -0
@@ -198,6 +198,17 @@ int parse_options(int argc, const char **argv, const char *prefix,const struct option *options, const char * const usagestr[], int flags); +struct parseopt_options { +int flags; +const char *const * usagestr; +int (*is_abbrev_ambiguous)(const struct option *prev, +const struct option *next); +}; + +int parse_options_extended (int argc,const char **argv,const char *prefix, +const struct option *options, +const struct parseopt_options *parseopts); + NORETURN void usage_with_options(const char * const *usagestr,const struct option *options);
@@ -256,6 +267,7 @@ struct parse_opt_ctx_t {const char *opt;int flags;const char *prefix; +int (*is_abbrev_ambiguous)(const struct option *,const struct option *); }; void parse_options_start(struct parse_opt_ctx_t *ctx,
-- 8< --
Quoted 2 lines
Thanks.
Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases
Quoted 4 lines · Duy Nguyen
So an alternative is simply outsource the ambiguity decision back to git-clone. If the same situation appears again elsewhere, we'll need to sit back and fix it for real. But this way we don't potentially introduce any new traps.
Sounds like a sensibly safe approach.
builtin/clone.c static struct option builtin_clone_options[] = { · line 156
Quoted 9 lines
builtin/clone.c +9
-0+/* +* Avoid ambiguation error between --recursive and --recurse-submodule +* because they are the same. --recurs can be expanded to any of them +* and it still works. +*/ +static int is_abbrev_ambiguous (const struct option *prev, +const struct option *next) +{ +const struct option *opts[] = { prev, next };
By looking at its caller, I think the caller keeps track of the candidate it saw so far, and ask this function to see if the one it is looking at right now (i.e. "this one") should be flagged as conflicting with the other one (or "the other one"). So it probably makes more sense to call them <it, the_other> or <it, prev> [*1*].
Side note: *1* I would have used "this" instead of "it", but I vaguely recall there are those who want to use C++ aware static checkers and avoiding the identifier "this" is easy enough so ...
builtin/clone.c static struct option builtin_clone_options[] = { · line 169
Quoted 13 lines
builtin/clone.c +13
-0+int i, found =0 ; + +for (i =0 ; i <ARRAY_SIZE (opts); i++) { +if (!opts[i]->long_name) +continue ; +if (!strcmp (opts[i]->long_name,"recursive" )) + found |=1 <<0 ; +if (!strcmp (opts[i]->long_name,"recurse-submodules" )) + found |=1 <<1 ; + } + +return found !=3 ; +}
For any two options that share the prefix, unless they are "recursive" and "recurse-submodules" pair, we say "that's ambiguous". But when they are these two specifically singled out, we say it is OK. Makes sense.
The above may be sufficient for this purpose, but I would have expected that these groups of aliases would be the ones in the table, not the <it, the_other> pair. IOW, with helpers like these:
static const char *recurse_submodules[] = {
"recurse-submodules", "recursive", NULL,
};
static struct {
const char **aliases;
} disamb[] = {
recurse_submodules,
};
static int has_string(const char *it, const char **array)
{
while (*array)
if (!strcmp(it, *(array++)))
return 1;
return 0;
}the body would look something like:
int j;
for (j = 0; j < ARRAY_SIZE(disamb); j++) {
/* it and other are from the same family? */
if (it->long_name &&
has_string(it->long_name, disamb[j].aliases) &&
other->long_name &&
has_string(other->long_name, disamb[j].aliases))
return 0;
}
return 1;Perhaps?
I suspect that renaming the function (and the field in the context struct) to .is_alias() and reverse the polarity of its return value may make the flow of the logic at the caller side easier to follow?
parse-options.c static enum parse_opt_result parse_long_opt( · line 298
Quoted 11 lines
parse-options.c +2
-1@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(if (!(p->flags &PARSE_OPT_KEEP_UNKNOWN ) && !strncmp (long_name, arg, arg_end - arg)) { is_abbreviated: -if (abbrev_option) { +if (abbrev_option && + p->is_abbrev_ambiguous (abbrev_option, options)) {
The above suggestion would make the guard here to
if (abbrev_option &&
!p->is_alias(abbrev_option, options)) {That is, at this point, we know that the user may have used the given string to name the "abbrev_option" we earlier saw (and made sure it prefix matches the string), and now we are looking at another one, options[0], that also prefix matches the string. We usually flag this case as a problematic ambiguity inside the body of this if statement, but if one is an alias to the other, we do not do so.
Quoted 3 lines
parse-options.c +0
-0/* * If this is abbreviated, it is * ambiguous. So when there is no
PATCH 1/1 parse-options: don't emit "ambiguous option" for aliases
Change the option parsing machinery so that e.g. "clone --recurs ..." doesn't error out because "clone" understands both "--recursive" and "--recurse-submodules" to mean the same thing.
Initially "clone" just understood --recursive until the --recurses-submodules alias was added in ccdd3da652 ("clone: Add the --recurse-submodules option as alias for --recursive", 2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to optionally take a pathspec", 2017-03-17) the longer form has been promoted to the default.
But due to the way the options parsing machinery works this resulted in the rather absurd situation of:
$ git clone --recurs [...]
error: ambiguous option: recurs (could be --recursive or --recurse-submodules)Add OPT_ALIAS() to express this linke between two options and use it in git-clone.
A big chunk of code is actually from Junio C Hamano. The test case (and most of the commit message) is from Ævar Arnfjörð Bjarmason.
---
I'm not sure if Junio is an evil genius who lures me with his helpful suggestions. Either way I ended up spending a couple hours writing and rewriting and finally decided all other options were too ugly. And his suggestions were mostly there already.
So here's OPT_ALIAS(). I avoid rewriting all the option iteration code by simply making a copy of options[] array to use, then free it afterwards. Not the best, but at least it could be easily removed if we end up not want OPT_ALIAS() after all.
This probably needs signoff from both Ævar and Junio, if accepted.
builtin/clone.c | 5 +- parse-options.c | 130 +++++++++++++++++++++++++++++++++++++-- parse-options.h | 6 ++ t/t0040-parse-options.sh | 5 ++ 4 files changed, 136 insertions(+), 10 deletions(-)
builtin/clone.c +1 -4
@@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = { N_("don't use local hardlinks, always copy")),OPT_BOOL ('s' ,"shared" , &option_shared,N_ ("setup as shared repository" )), - { OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules, -N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), -PARSE_OPT_OPTARG |PARSE_OPT_HIDDEN , recurse_submodules_cb, - (intptr_t)"." }, +OPT_ALIAS (0 ,"recursive" ,"recurse-submodules" ), { OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
parse-options.c +124 -6
189 lines not on this page · raw message
parse-options.h +6 -0
@@ -7,6 +7,7 @@ enum parse_opt_type {OPTION_ARGUMENT ,OPTION_GROUP ,OPTION_NUMBER , +OPTION_ALIAS ,/* options with no arguments */ OPTION_BIT , OPTION_NEGBIT,
@@ -181,6 +182,9 @@ struct option {N_ ("no-op (backward compatibility)" ), \ PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb } +#define OPT_ALIAS (s, l, alias_source) \ + { OPTION_ALIAS, (s), (l), (alias_source) } +/* * parse_options() will filter out the processed options and leave the * non-option arguments in argv[]. argv0 is assumed program name and
@@ -256,6 +260,8 @@ struct parse_opt_ctx_t {const char *opt;int flags;const char *prefix; +const char **alias_groups;/* must be in groups of 3 elements! */ +struct option *updated_options; }; void parse_options_start(struct parse_opt_ctx_t *ctx,
t/t0040-parse-options.sh +5 -0
@@ -224,6 +224,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' ' test-tool parse-options --expect="string: 123" --st 123 ' +test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' ' + test_when_finished "rm -rf A" && + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false git clone --recurs . A +' + cat >typo.err <<\EOF error: did you mean `--boolean` (with two dashes ?) EOF
Signature · 1 line
2.21.0.854.ge34a79f761
Re: [PATCH] parse-options: don't emit "ambiguous option" for aliases
Quoted 44 lines · Nguyễn Thái Ngọc Duy
parse-options.c +40
-0@@ -574,6 +615,70 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,return PARSE_OPT_COMPLETE ; } +/* +* Scan and may produce a new option[] array, which should be used +* instead of the original 'options'. +* +* Right now this is only used to preprocess and substitue +* OPTION_ALIAS. +*/ +static struct option *preprocess_options (struct parse_opt_ctx_t *ctx, +const struct option *options) +{ +struct option *newopt; +int i, nr, alias; +int nr_aliases =0 ; + +for (nr =0 ; options[nr].type !=OPTION_END ; nr++) { +if (options[nr].type ==OPTION_ALIAS ) + nr_aliases++; + } + +if (!nr_aliases) +return NULL ; + +ALLOC_ARRAY (newopt, nr +1 ); +COPY_ARRAY (newopt, options, nr +1 ); + +/* each alias has two string pointers and NULL */ +CALLOC_ARRAY (ctx->alias_groups,3 * (nr_aliases +1 )); + +for (alias =0 , i =0 ; i < nr; i++) { +const char *source; +int j; + +if (newopt[i].type !=OPTION_ALIAS ) +continue ; + +if (!newopt[i].long_name) +BUG ("An alias must have long option name" ); + + source = newopt[i].value; +
parse-options.c static int show_gitcomp(struct parse_opt_ctx_t *ctx, · line 667
Quoted 10 lines
parse-options.c +10
-0+for (j =0 ; j < nr; j++) { +const char *name = options[j].long_name; + +if (!name ||strcmp (name, source)) +continue ; + +if (options[j].type ==OPTION_ALIAS ) +BUG ("No please. Nested aliases are not supported." ); + +memcpy (newopt + i, options + j,sizeof (*newopt));
Eck. I need to restore newopt[i].long_name and .short_name after this memcpy, or we'll just have two --recurse-submodules instead. Luckily it still works after this bug is gone.
Quoted 2 lines
(quoted diff) +2
-0+ break; + }
Signature · 1 line
Duy
PATCH v2 1/1 parse-options: don't emit "ambiguous option" for aliases
In reply to Nguyễn Thái Ngọc DuyChange the option parsing machinery so that e.g. "clone --recurs ..." doesn't error out because "clone" understands both "--recursive" and "--recurse-submodules" to mean the same thing.
Initially "clone" just understood --recursive until the --recurses-submodules alias was added in ccdd3da652 ("clone: Add the --recurse-submodules option as alias for --recursive", 2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to optionally take a pathspec", 2017-03-17) the longer form has been promoted to the default.
But due to the way the options parsing machinery works this resulted in the rather absurd situation of:
$ git clone --recurs [...]
error: ambiguous option: recurs (could be --recursive or --recurse-submodules)Add OPT_ALIAS() to express this link between two or more options and use it in git-clone. Multiple aliases of an option could be written as
OPT_ALIAS(0, "alias1", "original-name"),
OPT_ALIAS(0, "alias2", "original-name"),
...The current implementation is not exactly optimal in this case. But we can optimize it when it becomes a problem. So far we don't even have two aliases of any option.
A big chunk of code is actually from Junio C Hamano.
---
OK it's working for real this time. test-parse-options.c is also updated to for testing OPT_ALIAS.
builtin/clone.c | 5 +- parse-options.c | 143 ++++++++++++++++++++++++++++++++-- parse-options.h | 6 ++ t/helper/test-parse-options.c | 3 + t/t0040-parse-options.sh | 17 ++++ 5 files changed, 164 insertions(+), 10 deletions(-)
builtin/clone.c +1 -4
@@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = { N_("don't use local hardlinks, always copy")),OPT_BOOL ('s' ,"shared" , &option_shared,N_ ("setup as shared repository" )), - { OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules, -N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), -PARSE_OPT_OPTARG |PARSE_OPT_HIDDEN , recurse_submodules_cb, - (intptr_t)"." }, +OPT_ALIAS (0 ,"recursive" ,"recurse-submodules" ), { OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,N_ ("pathspec" ),N_ ("initialize submodules in the clone" ), PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
parse-options.c +137 -6
202 lines not on this page · raw message
parse-options.h +6 -0
@@ -7,6 +7,7 @@ enum parse_opt_type {OPTION_ARGUMENT ,OPTION_GROUP ,OPTION_NUMBER , +OPTION_ALIAS ,/* options with no arguments */ OPTION_BIT , OPTION_NEGBIT,
@@ -181,6 +182,9 @@ struct option {N_ ("no-op (backward compatibility)" ), \ PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb } +#define OPT_ALIAS (s, l, source_long_name) \ + { OPTION_ALIAS, (s), (l), (source_long_name) } +/* * parse_options() will filter out the processed options and leave the * non-option arguments in argv[]. argv0 is assumed program name and
@@ -256,6 +260,8 @@ struct parse_opt_ctx_t {const char *opt;int flags;const char *prefix; +const char **alias_groups;/* must be in groups of 3 elements! */ +struct option *updated_options; }; void parse_options_start(struct parse_opt_ctx_t *ctx,
t/helper/test-parse-options.c +3 -0
@@ -149,6 +149,9 @@ int cmd__parse_options(int argc, const char **argv)OPT_CALLBACK (0 ,"expect" , &expect,"string" ,"expected output in the variable dump" , collect_expect), +OPT_GROUP ("Alias" ), +OPT_STRING ('A' ,"alias-source" , &string,"string" ,"get a string" ), +OPT_ALIAS ('Z' ,"alias-target" ,"alias-source" ), OPT_END(), };int i;
t/t0040-parse-options.sh +17 -0
@@ -48,6 +48,12 @@ Standard options -q, --quiet be quiet --expect <string> expected output in the variable dump +Alias + -A, --alias-source <string> + get a string + -Z, --alias-target <string> + get a string + EOF test_expect_success 'test help' '
@@ -224,6 +230,17 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' ' test-tool parse-options --expect="string: 123" --st 123 ' +test_expect_success 'Alias options do not contribute to abbreviation' ' + test-tool parse-options --alias-source 123 >output && + grep "^string: 123" output && + test-tool parse-options --alias-target 123 >output && + grep "^string: 123" output && + test_must_fail test-tool parse-options --alias && + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool parse-options --alias 123 >output && + grep "^string: 123" output +' + cat >typo.err <<\EOF error: did you mean `--boolean` (with two dashes ?) EOF
Signature · 1 line
2.21.0.1110.g9614c01b33
Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases
Quoted 9 lines · Nguyễn Thái Ngọc Duy
But due to the way the options parsing machinery works this resulted in the rather absurd situation of:
$ git clone --recurs [...] error: ambiguous option: recurs (could be --recursive or --recurse-submodules)Add
OPT_ALIAS()to express this link between two or more options and use it in git-clone. Multiple aliases of an option could be written as
Quoted 7 lines
OPT_ALIAS(0, "alias1", "original-name"), OPT_ALIAS(0, "alias2", "original-name"), ...The current implementation is not exactly optimal in this case. But we can optimize it when it becomes a problem. So far we don't even have two aliases of any option.
Sounds good enough for any practical need I can forsee ;-) Thanks.
Quoted 5 lines
t/t0040-parse-options.sh +0
-0
t/t0040-parse-options.sh Standard options · line 57
Quoted 10 lines
t/t0040-parse-options.sh +6
-0@@ -48,6 +48,12 @@ Standard options -q, --quiet be quiet --expect <string> expected output in the variable dump +Alias + -A, --alias-source <string> + get a string + -Z, --alias-target <string> + get a string + EOF
This is not a new problem per-se, as there already is a line before the precontext of this hunk that shares the same issue, but to prevent future problems, I am very much tempted to apply the attached on top.
-- >8 --
Subject: t0040: protect lines that are indented by spaces
This block is byte-for-byte identical expected output, that contains a few lines that are indented in many spaces, which makes "git diff --check" unhappy and will break the test when "git am --whitespace=fix" is allowed to "correct" them.
Protect the left edge with a marker character, and strip it with sed, which is used as a standard way to deal with this issue in our test suite.
---
* Of course, if the right-edge need to be protected, we can do so as well.
t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------ 1 file changed, 47 insertions(+), 47 deletions(-)
t/t0040-parse-options.sh +47 -47
@@ -7,53 +7,53 @@ test_description='our own option parser' . ./test-lib.sh -cat >expect <<\EOF -usage: test-tool parse-options <options> - - A helper function for the parse-options API. - - --yes get a boolean - -D, --no-doubt begins with 'no-' - -B, --no-fear be brave - -b, --boolean increment by one - -4, --or4 bitwise-or boolean with ...0100 - --neg-or4 same as --no-or4 - - -i, --integer <n> get a integer - -j <n> get a integer, too - -m, --magnitude <n> get a magnitude - --set23 set integer to 23 - -L, --length <str> get length of <str> - -F, --file <file> set file to <file> - -String options - -s, --string <string> - get a string - --string2 <str> get another string - --st <st> get another string (pervert ordering) - -o <str> get another string - --list <str> add str to list - -Magic arguments - --quux means --quux - -NUM set integer to NUM - + same as -b - --ambiguous positive ambiguity - --no-ambiguous negative ambiguity - -Standard options - --abbrev[=<n>] use <n> digits to display SHA-1s - -v, --verbose be verbose - -n, --dry-run dry run - -q, --quiet be quiet - --expect <string> expected output in the variable dump - -Alias - -A, --alias-source <string> - get a string - -Z, --alias-target <string> - get a string - +sed -e 's/^|//' >expect <<\EOF +|usage: test-tool parse-options <options> +| +| A helper function for the parse-options API. +| +| --yes get a boolean +| -D, --no-doubt begins with 'no-' +| -B, --no-fear be brave +| -b, --boolean increment by one +| -4, --or4 bitwise-or boolean with ...0100 +| --neg-or4 same as --no-or4 +| +| -i, --integer <n> get a integer +| -j <n> get a integer, too +| -m, --magnitude <n> get a magnitude +| --set23 set integer to 23 +| -L, --length <str> get length of <str> +| -F, --file <file> set file to <file> +| +|String options +| -s, --string <string> +| get a string +| --string2 <str> get another string +| --st <st> get another string (pervert ordering) +| -o <str> get another string +| --list <str> add str to list +| +|Magic arguments +| --quux means --quux +| -NUM set integer to NUM +| + same as -b +| --ambiguous positive ambiguity +| --no-ambiguous negative ambiguity +| +|Standard options +| --abbrev[=<n>] use <n> digits to display SHA-1s +| -v, --verbose be verbose +| -n, --dry-run dry run +| -q, --quiet be quiet +| --expect <string> expected output in the variable dump +| +|Alias +| -A, --alias-source <string> +| get a string +| -Z, --alias-target <string> +| get a string +| EOF test_expect_success 'test help' '
Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases
Quoted 66 lines · Junio C Hamano
-- >8 --
Subject: t0040: protect lines that are indented by spacesThis block is byte-for-byte identical expected output, that contains a few lines that are indented in many spaces, which makes "git diff --check" unhappy and will break the test when "git am --whitespace=fix" is allowed to "correct" them.
Protect the left edge with a marker character, and strip it with sed, which is used as a standard way to deal with this issue in our test suite.
Signed-off-by Signed-off-by: Junio C Hamano <[email protected]>---
* Of course, if the right-edge need to be protected, we can do so as well.t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------ 1 file changed, 47 insertions(+), 47 deletions(-)t/t0040-parse-options.sh +0
-38@@ -7,53 +7,53 @@ test_description='our own option parser' . ./test-lib.sh -cat >expect <<\EOF -usage: test-tool parse-options <options> - - A helper function for the parse-options API. - - --yes get a boolean - -D, --no-doubt begins with 'no-' - -B, --no-fear be brave - -b, --boolean increment by one - -4, --or4 bitwise-or boolean with ...0100 - --neg-or4 same as --no-or4 - - -i, --integer <n> get a integer - -j <n> get a integer, too - -m, --magnitude <n> get a magnitude - --set23 set integer to 23 - -L, --length <str> get length of <str> - -F, --file <file> set file to <file> - -String options - -s, --string <string> - get a string - --string2 <str> get another string - --st <st> get another string (pervert ordering) - -o <str> get another string - --list <str> add str to list - -Magic arguments - --quux means --quux - -NUM set integer to NUM - + same as -b - --ambiguous positive ambiguity - --no-ambiguous negative ambiguity - -Standard options - --abbrev[=<n>] use <n> digits to display SHA-1s - -v, --verbose be verbose - -n, --dry-run dry run
t/t0040-parse-options.sh test_description='our own option parser' · line 10
Quoted 10 lines
t/t0040-parse-options.sh +1
-9- -q, --quiet be quiet - --expect <string> expected output in the variable dump - -Alias - -A, --alias-source <string> - get a string - -Z, --alias-target <string> - get a string - +sed -e 's/^|//' >expect <<\EOF
I think we already use qz_to_tab_space to protect leading spaces (t1450) or trailing ones (t4205). It's less strict than this though.
Anyway, if you go with 's/^|//', maybe make it a helper function too because I'm pretty sure we have more text like this in the test suite.
There's also the "tr -d Q" trick in t4038. But that's something to clean up if someone really have free time.
Quoted 49 lines
t/t0040-parse-options.sh +46
-0+|usage: test-tool parse-options <options> +| +| A helper function for the parse-options API. +| +| --yes get a boolean +| -D, --no-doubt begins with 'no-' +| -B, --no-fear be brave +| -b, --boolean increment by one +| -4, --or4 bitwise-or boolean with ...0100 +| --neg-or4 same as --no-or4 +| +| -i, --integer <n> get a integer +| -j <n> get a integer, too +| -m, --magnitude <n> get a magnitude +| --set23 set integer to 23 +| -L, --length <str> get length of <str> +| -F, --file <file> set file to <file> +| +|String options +| -s, --string <string> +| get a string +| --string2 <str> get another string +| --st <st> get another string (pervert ordering) +| -o <str> get another string +| --list <str> add str to list +| +|Magic arguments +| --quux means --quux +| -NUM set integer to NUM +| + same as -b +| --ambiguous positive ambiguity +| --no-ambiguous negative ambiguity +| +|Standard options +| --abbrev[=<n>] use <n> digits to display SHA-1s +| -v, --verbose be verbose +| -n, --dry-run dry run +| -q, --quiet be quiet +| --expect <string> expected output in the variable dump +| +|Alias +| -A, --alias-source <string> +| get a string +| -Z, --alias-target <string> +| get a string +| EOF test_expect_success 'test help' '
Signature · 1 line
Duy
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 3 lines · writes:
We do not want to have tests that need to be changed by completely unrelated patch series, just because abbreviations that used to be unique are being made non-unique by said patch series.
Makes sense.
If we wanted to make sure that options in abbreviated form work correctly, we should do so in dedicated parseopt-specific tests (and we do in t0040, I think), not in tests for random other commands that happens to feed options to them.
Will queue.
v2 cover Do not use abbreviated options in tests
In reply to Johannes Schindelin via GitGitGadgetWe do not want to have tests that need to be changed by completely unrelated patch series, just because abbreviations that used to be unique are being made non-unique by said patch series.
I stumbled over this while investigating the test failures in dl/rebase-i-keep-base
[https://dev.azure.com/gitgitgadget/git/_build/results?buildId=5482&view=ms.vss-test-web.build-test-results-tab]
: the new --keep-base option makes the --keep abbreviation of --keep-empty in t5407 non-unique, which causes the test suite to fail.
Changes since v1:
* Fixed a typo ("expaneded") in the last commit message.
* Added a regression test for the GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS handling.
* Adjusted the code in test-lib.sh that makes sure that this variable is
set: it is now consistent with the way we do similar things in that file already.
Johannes Schindelin (8):
tests (rebase): spell out the --keep-empty option
tests (rebase): spell out the --force-rebase option
t7810: do not abbreviate --no-exclude-standard nor --invert-match
t5531: avoid using an abbreviated option
tests (push): do not abbreviate the --follow-tags option tests (status): spell out the --find-renames option in full tests (pack-objects): use the full, unabbreviated --revs option
tests: disallow the use of abbreviated options (by default)
parse-options.c | 9 ++++++ t/README | 4 +++ t/t0040-parse-options.sh | 14 +++++++- t/t3415-rebase-autosquash.sh | 2 +- t/t3430-rebase-merges.sh | 4 +-- t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++------------- t/t5407-post-rewrite-hook.sh | 4 +-- t/t5516-fetch-push.sh | 4 +-- t/t5531-deep-submodule-push.sh | 2 +- t/t7525-status-rename.sh | 8 ++--- t/t7810-grep.sh | 16 +++++----- t/test-lib.sh | 7 ++++ 12 files changed, 75 insertions(+), 43 deletions(-)
base-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-167%2Fdscho%2Fdisallow-abbreviated-options-in-tests-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-167/dscho/disallow-abbreviated-options-in-tests-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/167
Range-diff vs v1:
1: a1b4b74b91 = 1: a1b4b74b91 tests (rebase): spell out the `--keep-empty` option
2: 017ec9a0c7 = 2: 017ec9a0c7 tests (rebase): spell out the `--force-rebase` option
3: 7d8f6a2ee6 = 3: 7d8f6a2ee6 t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`
4: f45c435e4a = 4: f45c435e4a t5531: avoid using an abbreviated option
5: 7372059922 = 5: 7372059922 tests (push): do not abbreviate the `--follow-tags` option
6: 531450c00d = 6: 531450c00d tests (status): spell out the `--find-renames` option in full
7: a722a065d2 = 7: a722a065d2 tests (pack-objects): use the full, unabbreviated `--revs` option
8: 04c36b1de9 ! 8: a27d316855 tests: disallow the use of abbreviated options (by default)
@@ -14,7 +14,7 @@
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` expaneded to `git init --bare`, that future
+ 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.
@@ -115,6 +115,18 @@
test-tool parse-options --no-ambig >output 2>output.err &&
test_must_be_empty output.err &&
test_cmp expect output
+@@
+ 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
--- a/t/test-lib.sh
@@ -124,10 +136,11 @@
export PERL_PATH SHELL_PATH
+# Disallow the use of abbreviated options in the test suite by default
-+test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
++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...Signature · 1 line
gitgitgadget
PATCH v2 1/8 tests (rebase): spell out the `--keep-empty` option
From: Johannes Schindelin <[email protected]>
This test wants to run git rebase with the --keep-empty option, but it really only spelled out --keep and trusted Git's option parsing to determine that this was a unique abbreviation of the real option.
However, Denton Liu contributed a patch series in
https://public-inbox.org/git/[email protected]/
that introduces a new git rebase option called --keep-base, which makes this previously unique abbreviation non-unique.
Whether this patch series is accepted or not, it is actually a bad practice to use abbreviated options in our test suite, because of the issue that those unique option names are not guaranteed to stay unique in the future.
So let's just not use abbreviated options in the test suite.
--- t/t5407-post-rewrite-hook.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t5407-post-rewrite-hook.sh +2 -2
@@ -131,7 +131,7 @@ test_expect_success 'git rebase -m --skip' ' test_expect_success 'git rebase with implicit use of interactive backend' ' git reset --hard D && clear_hook_input && - test_must_fail git rebase --keep --onto A B && + test_must_fail git rebase --keep-empty --onto A B && echo C > foo && git add foo && git rebase --continue &&
@@ -146,7 +146,7 @@ test_expect_success 'git rebase with implicit use of interactive backend' ' test_expect_success 'git rebase --skip with implicit use of interactive backend' ' git reset --hard D && clear_hook_input && - test_must_fail git rebase --keep --onto A B && + test_must_fail git rebase --keep-empty --onto A B && test_must_fail git rebase --skip && echo D > foo && git add foo &&
Signature · 1 line
gitgitgadget
PATCH v2 2/8 tests (rebase): spell out the `--force-rebase` option
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
In quite a few test cases, we were sloppy and used the abbreviation --force, but we really should be precise in what we want to test.
--- t/t3415-rebase-autosquash.sh | 2 +- t/t3430-rebase-merges.sh | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
t/t3415-rebase-autosquash.sh +1 -1
@@ -277,7 +277,7 @@ test_expect_success 'autosquash with empty custom instructionFormat' ' ( set_cat_todo_editor && test_must_fail git -c rebase.instructionFormat= \ - rebase --autosquash --force -i HEAD^ >actual && + rebase --autosquash --force-rebase -i HEAD^ >actual && git log -1 --format="pick %h %s" >expect && test_cmp expect actual )
t/t3430-rebase-merges.sh +2 -2
@@ -271,7 +271,7 @@ test_expect_success 'root commits' ' EOF test_config sequence.editor \""$PWD"/replace-editor.sh\" && test_tick && - git rebase -i --force --root -r && + git rebase -i --force-rebase --root -r && test "Parsnip" = "$(git show -s --format=%an HEAD^)" && test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) && test $(git rev-parse second-root:second-root.t) = \
@@ -364,7 +364,7 @@ test_expect_success 'octopus merges' ' test_cmp_rev HEAD $before && test_tick && - git rebase -i --force -r HEAD^^ && + git rebase -i --force-rebase -r HEAD^^ && test "Hank" = "$(git show -s --format=%an HEAD)" && test "$before" != $(git rev-parse HEAD) && test_cmp_graph HEAD^^.. <<-\EOF
Signature · 1 line
gitgitgadget
PATCH v2 3/8 t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
This script used abbreviated options, which is unnecessarily fragile.
--- t/t7810-grep.sh | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
t/t7810-grep.sh +8 -8
@@ -119,33 +119,33 @@ do test_cmp expected actual ' - test_expect_success "grep -w $L (with --column, --invert)" ' + test_expect_success "grep -w $L (with --column, --invert-match)" ' { echo ${HC}file:1:foo mmap bar echo ${HC}file:1:foo_mmap bar echo ${HC}file:1:foo_mmap bar mmap echo ${HC}file:1:foo mmap bar_mmap } >expected && - git grep --column --invert -w -e baz $H -- file >actual && + git grep --column --invert-match -w -e baz $H -- file >actual && test_cmp expected actual ' - test_expect_success "grep $L (with --column, --invert, extended OR)" ' + test_expect_success "grep $L (with --column, --invert-match, extended OR)" ' { echo ${HC}hello_world:6:HeLLo_world } >expected && - git grep --column --invert -e ll --or --not -e _ $H -- hello_world \ + git grep --column --invert-match -e ll --or --not -e _ $H -- hello_world \ >actual && test_cmp expected actual ' - test_expect_success "grep $L (with --column, --invert, extended AND)" ' + test_expect_success "grep $L (with --column, --invert-match, extended AND)" ' { echo ${HC}hello_world:3:Hello world echo ${HC}hello_world:3:Hello_world echo ${HC}hello_world:6:HeLLo_world } >expected && - git grep --column --invert --not -e _ --and --not -e ll $H -- hello_world \ + git grep --column --invert-match --not -e _ --and --not -e ll $H -- hello_world \ >actual && test_cmp expected actual '
@@ -1010,7 +1010,7 @@ test_expect_success 'outside of git repository' ' echo ".gitignore:.*o*" && cat ../expect.full } >../expect.with.ignored && - git grep --no-index --no-exclude o >../actual.full && + git grep --no-index --no-exclude-standard o >../actual.full && test_cmp ../expect.with.ignored ../actual.full ) '
@@ -1051,7 +1051,7 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' ' echo ".gitignore:.*o*" && cat ../expect.full } >../expect.with.ignored && - git -c grep.fallbackToNoIndex grep --no-exclude o >../actual.full && + git -c grep.fallbackToNoIndex grep --no-exclude-standard o >../actual.full && test_cmp ../expect.with.ignored ../actual.full ) '
Signature · 1 line
gitgitgadget
PATCH v2 4/8 t5531: avoid using an abbreviated option
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
It was probably just an oversight: the --recurse-submodules option puts the term "submodules" in the plural form, not the singular one.
To avoid future problems in case that another option is introduced that starts with the prefix --recurse-submodule, let's just fix this.
--- t/t5531-deep-submodule-push.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
t/t5531-deep-submodule-push.sh +1 -1
@@ -363,7 +363,7 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs ) && git add b && git commit -m "added submodule" && - git push --recurse-submodule=check origin master + git push --recurse-submodules=check origin master ) '
Signature · 1 line
gitgitgadget
PATCH v2 5/8 tests (push): do not abbreviate the `--follow-tags` option
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
We really want to spell out the option in the full form, to avoid any ambiguity that might be introduced by future patches.
--- t/t5516-fetch-push.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
t/t5516-fetch-push.sh +2 -2
@@ -1370,7 +1370,7 @@ test_expect_success 'push does not follow tags by default' ' test_cmp expect actual ' -test_expect_success 'push --follow-tag only pushes relevant tags' ' +test_expect_success 'push --follow-tags only pushes relevant tags' ' mk_test testrepo heads/master && rm -fr src dst && git init src &&
@@ -1384,7 +1384,7 @@ test_expect_success 'push --follow-tag only pushes relevant tags' ' git tag -m "future" future && git checkout master && git for-each-ref refs/heads/master refs/tags/tag >../expect && - git push --follow-tag ../dst master + git push --follow-tags ../dst master ) && ( cd dst &&
Signature · 1 line
gitgitgadget
PATCH v2 6/8 tests (status): spell out the `--find-renames` option in full
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
To avoid future ambiguities, we really want to use full option names in the test suite. t7525-status-rename.sh used an abbreviated form of the --find-renames option, though, so let's change that.
--- t/t7525-status-rename.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
t/t7525-status-rename.sh +4 -4
@@ -84,7 +84,7 @@ test_expect_success 'status score=100%' ' test_i18ngrep "deleted:" actual && test_i18ngrep "new file:" actual && - git status --find-rename=100% >actual && + git status --find-renames=100% >actual && test_i18ngrep "deleted:" actual && test_i18ngrep "new file:" actual '
@@ -93,11 +93,11 @@ test_expect_success 'status score=01%' ' git status -M=01% >actual && test_i18ngrep "renamed:" actual && - git status --find-rename=01% >actual && + git status --find-renames=01% >actual && test_i18ngrep "renamed:" actual ' -test_expect_success 'copies not overridden by find-rename' ' +test_expect_success 'copies not overridden by find-renames' ' cp renamed copy && git add copy &&
@@ -105,7 +105,7 @@ test_expect_success 'copies not overridden by find-rename' ' test_i18ngrep "copied:" actual && test_i18ngrep "renamed:" actual && - git -c status.renames=copies status --find-rename=01% >actual && + git -c status.renames=copies status --find-renames=01% >actual && test_i18ngrep "copied:" actual && test_i18ngrep "renamed:" actual '
Signature · 1 line
gitgitgadget
PATCH v2 7/8 tests (pack-objects): use the full, unabbreviated `--revs` option
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
To use the singular form of a word, when the option wants the plural form (and quietly expands it because it thinks it was abbreviated), is an easy mistake to make, and t5317 contains almost two dozen of them.
However, using abbreviated options in tests is a bit fragile, so we will disallow use of abbreviated options in our test suite.
In preparation for this change, let's fixt5317-pack-objects-filter-objects.sh.
--- t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++------------- 1 file changed, 22 insertions(+), 22 deletions(-)
t/t5317-pack-objects-filter-objects.sh +22 -22
@@ -25,7 +25,7 @@ test_expect_success 'verify blob count in normal packfile' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r1 pack-objects --rev --stdout >all.pack <<-EOF && + git -C r1 pack-objects --revs --stdout >all.pack <<-EOF && HEAD EOF git -C r1 index-pack ../all.pack &&
@@ -39,7 +39,7 @@ test_expect_success 'verify blob count in normal packfile' ' ' test_expect_success 'verify blob:none packfile has no blobs' ' - git -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF && + git -C r1 pack-objects --revs --stdout --filter=blob:none >filter.pack <<-EOF && HEAD EOF git -C r1 index-pack ../filter.pack &&
@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' ' git -C r5 commit -m "foo" && del=$(git -C r5 rev-parse HEAD^{tree} | sed "s|..|&/|") && rm r5/.git/objects/$del && - test_must_fail git -C r5 pack-objects --rev --stdout 2>bad_tree <<-EOF && + test_must_fail git -C r5 pack-objects --revs --stdout 2>bad_tree <<-EOF && HEAD EOF grep "bad tree object" bad_tree
@@ -88,7 +88,7 @@ test_expect_success 'setup for tests of tree:0' ' ' test_expect_success 'verify tree:0 packfile has no blobs or trees' ' - git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF && + git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF && HEAD EOF git -C r1 index-pack ../commitsonly.pack &&
@@ -98,7 +98,7 @@ test_expect_success 'verify tree:0 packfile has no blobs or trees' ' test_expect_success 'grab tree directly when using tree:0' ' # We should get the tree specified directly but not its blobs or subtrees. - git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF && + git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF && HEAD: EOF git -C r1 index-pack ../commitsonly.pack &&
@@ -128,7 +128,7 @@ test_expect_success 'verify blob count in normal packfile' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout >all.pack <<-EOF && + git -C r2 pack-objects --revs --stdout >all.pack <<-EOF && HEAD EOF git -C r2 index-pack ../all.pack &&
@@ -142,7 +142,7 @@ test_expect_success 'verify blob count in normal packfile' ' ' test_expect_success 'verify blob:limit=500 omits all blobs' ' - git -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=500 >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -157,7 +157,7 @@ test_expect_success 'verify blob:limit=500 omits all blobs' ' ' test_expect_success 'verify blob:limit=1000' ' - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1000 >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -176,7 +176,7 @@ test_expect_success 'verify blob:limit=1001' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -194,7 +194,7 @@ test_expect_success 'verify blob:limit=10001' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -212,7 +212,7 @@ test_expect_success 'verify blob:limit=1k' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -230,7 +230,7 @@ test_expect_success 'verify explicitly specifying oversized blob in input' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF && HEAD $(git -C r2 rev-parse HEAD:large.10000) EOF
@@ -249,7 +249,7 @@ test_expect_success 'verify blob:limit=1m' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF && + git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF && HEAD EOF git -C r2 index-pack ../filter.pack &&
@@ -302,7 +302,7 @@ test_expect_success 'verify blob count in normal packfile' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r3 pack-objects --rev --stdout >all.pack <<-EOF && + git -C r3 pack-objects --revs --stdout >all.pack <<-EOF && HEAD EOF git -C r3 index-pack ../all.pack &&
@@ -320,7 +320,7 @@ test_expect_success 'verify sparse:path=pattern1' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF && + git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF && HEAD EOF git -C r3 index-pack ../filter.pack &&
@@ -352,7 +352,7 @@ test_expect_success 'verify sparse:path=pattern2' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF && + git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF && HEAD EOF git -C r3 index-pack ../filter.pack &&
@@ -404,7 +404,7 @@ test_expect_success 'verify blob count in normal packfile' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r4 pack-objects --rev --stdout >all.pack <<-EOF && + git -C r4 pack-objects --revs --stdout >all.pack <<-EOF && HEAD EOF git -C r4 index-pack ../all.pack &&
@@ -423,7 +423,7 @@ test_expect_success 'verify sparse:oid=OID' ' sort >expected && oid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) && - git -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF && + git -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF && HEAD EOF git -C r4 index-pack ../filter.pack &&
@@ -441,7 +441,7 @@ test_expect_success 'verify sparse:oid=oid-ish' ' awk -f print_2.awk ls_files_result | sort >expected && - git -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF && + git -C r4 pack-objects --revs --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF && HEAD EOF git -C r4 index-pack ../filter.pack &&
@@ -470,19 +470,19 @@ test_expect_success 'setup r1 - delete loose blobs' ' ' test_expect_success 'verify pack-objects fails w/ missing objects' ' - test_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF + test_must_fail git -C r1 pack-objects --revs --stdout >miss.pack <<-EOF HEAD EOF ' test_expect_success 'verify pack-objects fails w/ --missing=error' ' - test_must_fail git -C r1 pack-objects --rev --stdout --missing=error >miss.pack <<-EOF + test_must_fail git -C r1 pack-objects --revs --stdout --missing=error >miss.pack <<-EOF HEAD EOF ' test_expect_success 'verify pack-objects w/ --missing=allow-any' ' - git -C r1 pack-objects --rev --stdout --missing=allow-any >miss.pack <<-EOF + git -C r1 pack-objects --revs --stdout --missing=allow-any >miss.pack <<-EOF HEAD EOF '
Signature · 1 line
gitgitgadget
PATCH v2 8/8 tests: disallow the use of abbreviated options (by default)
In reply to Johannes Schindelin via GitGitGadgetFrom: Johannes Schindelin <[email protected]>
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 modegit 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.
--- 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(-)
parse-options.c +9 -0
@@ -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 :
t/README +4 -0
@@ -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 ------------
t/t0040-parse-options.sh +13 -1
@@ -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
t/test-lib.sh +7 -0
@@ -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
Signature · 1 line
gitgitgadget
Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)
"Johannes Schindelin via GitGitGadget" <[email protected]>
parse-options.c int parse_options(int argc, const char **argv, const char *prefix, · line 718
Quoted 2 lines · writes:
parse-options.c +2
-0+ 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,
t/README GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the · line 404
Quoted 11 lines
t/README +3
-0@@ -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.
t/README GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
Quoted 6 lines
t/README +6
-0+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.
Quoted 5 lines
t/test-lib.sh +0
-0
t/test-lib.sh fi · line 65
Quoted 9 lines
t/test-lib.sh +6
-0@@ -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.
Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)
Quoted 3 lines · Junio C Hamano
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 <[email protected]>
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 modegit 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.
--- 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(-)
parse-options.c +9 -0
@@ -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 :
t/README +4 -0
@@ -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 ------------
t/t0040-parse-options.sh +13 -1
@@ -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
t/test-lib.sh +7 -0
@@ -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
Signature · 1 line
2.21.0-313-ge35b8cb8e2
Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)
Hi Junio,
Quoted 7 lines · Junio C Hamano
Quoted 3 lines · Junio C Hamano
Do you mean more like
...
I think I can agree with either of the two positions...
Quoted 6 lines
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
Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)
Quoted 14 lines · Johannes Schindelin
Hi Junio,
Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Junio C Hamano
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".
Quoted 6 lines
Quoted 3 lines
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
issetthing, 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 ;;
esacThis *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.
Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)
Hi Junio,
Quoted 37 lines · Junio C Hamano
Quoted 12 lines · Johannes Schindelin
Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Junio C Hamano
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".
Quoted 6 lines
Quoted 3 lines
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
issetthing, as it makes things more consistent, but I'll not fight for it.${var:+isset} is fine. Instead of
t/test-lib.sh +7
-0+# 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 ...
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 11 lines
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
Quoted 6 lines
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
Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)
In reply to Junio C HamanoHi Junio,
Quoted 26 lines · Junio C Hamano
"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
t/test-lib.sh +0
-0t/test-lib.sh fi · line 65
Quoted 9 lines
t/test-lib.sh +6
-0@@ -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 +fiIf 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.
Quoted 8 lines
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.
Quoted 4 lines
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.
Quoted 14 lines
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.
Quoted 2 lines
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