{"thread":{"id":"50818","subject":"[PATCH 0/8] Do not use abbreviated options in tests","startedAt":"2019-03-25T18:14:21Z","lastAt":"2019-05-07T11:59:20Z","messageCount":52,"participants":["Johannes Schindelin via GitGitGadget","Denton Liu","Ævar Arnfjörð Bjarmason","Eric Sunshine","Duy Nguyen","Junio C Hamano","Johannes Schindelin","Nguyễn Thái Ngọc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"372444","messageId":"pull.167.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":null,"subject":"[PATCH 0/8] Do not use abbreviated options in tests","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:17Z","receivedAt":"2019-03-25T18:14:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We do not want to have tests that need to be changed by completely unrelated\npatch series, just because abbreviations that used to be unique are being\nmade non-unique by said patch series.\n\nI stumbled over this while investigating the test failures in\ndl/rebase-i-keep-base\n[https://dev.azure.com/gitgitgadget/git/_build/results?buildId=5482&view=ms.vss-test-web.build-test-results-tab]\n: the new --keep-base option makes the --keep abbreviation of --keep-empty \nin t5407 non-unique, which causes the test suite to fail.\n\nJohannes Schindelin (8):\n tests (rebase): spell out the `--keep-empty` option\n tests (rebase): spell out the `--force-rebase` option\n t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`\n t5531: avoid using an abbreviated option\n tests (push): do not abbreviate the `--follow-tags` option\n tests (status): spell out the `--find-renames` option in full\n tests (pack-objects): use the full, unabbreviated `--revs` option\n tests: disallow the use of abbreviated options (by default)\n\n parse-options.c | 9 ++++++\n t/README | 4 +++\n t/t0040-parse-options.sh | 7 +++-\n t/t3415-rebase-autosquash.sh | 2 +-\n t/t3430-rebase-merges.sh | 4 +--\n t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------\n t/t5407-post-rewrite-hook.sh | 4 +--\n t/t5516-fetch-push.sh | 4 +--\n t/t5531-deep-submodule-push.sh | 2 +-\n t/t7525-status-rename.sh | 8 ++---\n t/t7810-grep.sh | 16 +++++-----\n t/test-lib.sh | 6 ++++\n 12 files changed, 67 insertions(+), 43 deletions(-)\n\n\nbase-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-167%2Fdscho%2Fdisallow-abbreviated-options-in-tests-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-167/dscho/disallow-abbreviated-options-in-tests-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/167\n-- \ngitgitgadget\n"},{"id":"372445","messageId":"a1b4b74b9167e279dae4cd8c58fb28d8a714a66a.1553537656.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 1/8] tests (rebase): spell out the `--keep-empty` option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:18Z","receivedAt":"2019-03-25T18:14:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis test wants to run `git rebase` with the `--keep-empty` option, but\nit really only spelled out `--keep` and trusted Git's option parsing to\ndetermine that this was a unique abbreviation of the real option.\n\nHowever, Denton Liu contributed a patch series in\nhttps://public-inbox.org/git/cover.1553354374.git.liu.denton@gmail.com/\nthat introduces a new `git rebase` option called `--keep-base`, which\nmakes this previously unique abbreviation non-unique.\n\nWhether this patch series is accepted or not, it is actually a bad\npractice to use abbreviated options in our test suite, because of the\nissue that those unique option names are not guaranteed to stay unique\nin the future.\n\nSo let's just not use abbreviated options in the test suite.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5407-post-rewrite-hook.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex a4a5903cba..7344253bfb 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -131,7 +131,7 @@ test_expect_success 'git rebase -m --skip' '\n test_expect_success 'git rebase with implicit use of interactive backend' '\n \tgit reset --hard D &&\n \tclear_hook_input &&\n-\ttest_must_fail git rebase --keep --onto A B &&\n+\ttest_must_fail git rebase --keep-empty --onto A B &&\n \techo C > foo &&\n \tgit add foo &&\n \tgit rebase --continue &&\n@@ -146,7 +146,7 @@ test_expect_success 'git rebase with implicit use of interactive backend' '\n test_expect_success 'git rebase --skip with implicit use of interactive backend' '\n \tgit reset --hard D &&\n \tclear_hook_input &&\n-\ttest_must_fail git rebase --keep --onto A B &&\n+\ttest_must_fail git rebase --keep-empty --onto A B &&\n \ttest_must_fail git rebase --skip &&\n \techo D > foo &&\n \tgit add foo &&\n-- \ngitgitgadget\n\n"},{"id":"372446","messageId":"7d8f6a2ee6295b9817566853e1cfeca16b144b30.1553537656.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:19Z","receivedAt":"2019-03-25T18:14:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis script used abbreviated options, which is unnecessarily fragile.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7810-grep.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 43aa4161cf..2e1bb61b41 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -119,33 +119,33 @@ do\n \t\ttest_cmp expected actual\n \t'\n \n-\ttest_expect_success \"grep -w $L (with --column, --invert)\" '\n+\ttest_expect_success \"grep -w $L (with --column, --invert-match)\" '\n \t\t{\n \t\t\techo ${HC}file:1:foo mmap bar\n \t\t\techo ${HC}file:1:foo_mmap bar\n \t\t\techo ${HC}file:1:foo_mmap bar mmap\n \t\t\techo ${HC}file:1:foo mmap bar_mmap\n \t\t} >expected &&\n-\t\tgit grep --column --invert -w -e baz $H -- file >actual &&\n+\t\tgit grep --column --invert-match -w -e baz $H -- file >actual &&\n \t\ttest_cmp expected actual\n \t'\n \n-\ttest_expect_success \"grep $L (with --column, --invert, extended OR)\" '\n+\ttest_expect_success \"grep $L (with --column, --invert-match, extended OR)\" '\n \t\t{\n \t\t\techo ${HC}hello_world:6:HeLLo_world\n \t\t} >expected &&\n-\t\tgit grep --column --invert -e ll --or --not -e _ $H -- hello_world \\\n+\t\tgit grep --column --invert-match -e ll --or --not -e _ $H -- hello_world \\\n \t\t\t>actual &&\n \t\ttest_cmp expected actual\n \t'\n \n-\ttest_expect_success \"grep $L (with --column, --invert, extended AND)\" '\n+\ttest_expect_success \"grep $L (with --column, --invert-match, extended AND)\" '\n \t\t{\n \t\t\techo ${HC}hello_world:3:Hello world\n \t\t\techo ${HC}hello_world:3:Hello_world\n \t\t\techo ${HC}hello_world:6:HeLLo_world\n \t\t} >expected &&\n-\t\tgit grep --column --invert --not -e _ --and --not -e ll $H -- hello_world \\\n+\t\tgit grep --column --invert-match --not -e _ --and --not -e ll $H -- hello_world \\\n \t\t\t>actual &&\n \t\ttest_cmp expected actual\n \t'\n@@ -1010,7 +1010,7 @@ test_expect_success 'outside of git repository' '\n \t\t\techo \".gitignore:.*o*\" &&\n \t\t\tcat ../expect.full\n \t\t} >../expect.with.ignored &&\n-\t\tgit grep --no-index --no-exclude o >../actual.full &&\n+\t\tgit grep --no-index --no-exclude-standard o >../actual.full &&\n \t\ttest_cmp ../expect.with.ignored ../actual.full\n \t)\n '\n@@ -1051,7 +1051,7 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '\n \t\t\techo \".gitignore:.*o*\" &&\n \t\t\tcat ../expect.full\n \t\t} >../expect.with.ignored &&\n-\t\tgit -c grep.fallbackToNoIndex grep --no-exclude o >../actual.full &&\n+\t\tgit -c grep.fallbackToNoIndex grep --no-exclude-standard o >../actual.full &&\n \t\ttest_cmp ../expect.with.ignored ../actual.full\n \t)\n '\n-- \ngitgitgadget\n\n"},{"id":"372447","messageId":"7372059922710f392201cb32c5a7ac9cbdbe0616.1553537656.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 5/8] tests (push): do not abbreviate the `--follow-tags` option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:21Z","receivedAt":"2019-03-25T18:14:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe really want to spell out the option in the full form, to avoid any\nambiguity that might be introduced by future patches.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5516-fetch-push.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 37e8e80893..db0b1db458 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1370,7 +1370,7 @@ test_expect_success 'push does not follow tags by default' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'push --follow-tag only pushes relevant tags' '\n+test_expect_success 'push --follow-tags only pushes relevant tags' '\n \tmk_test testrepo heads/master &&\n \trm -fr src dst &&\n \tgit init src &&\n@@ -1384,7 +1384,7 @@ test_expect_success 'push --follow-tag only pushes relevant tags' '\n \t\tgit tag -m \"future\" future &&\n \t\tgit checkout master &&\n \t\tgit for-each-ref refs/heads/master refs/tags/tag >../expect &&\n-\t\tgit push --follow-tag ../dst master\n+\t\tgit push --follow-tags ../dst master\n \t) &&\n \t(\n \t\tcd dst &&\n-- \ngitgitgadget\n\n"},{"id":"372448","messageId":"f45c435e4acb765a68b83ab2c5127388c59dc0b1.1553537656.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 4/8] t5531: avoid using an abbreviated option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:20Z","receivedAt":"2019-03-25T18:14:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt was probably just an oversight: the `--recurse-submodules` option\nputs the term \"submodules\" in the plural form, not the singular one.\n\nTo avoid future problems in case that another option is introduced that\nstarts with the prefix `--recurse-submodule`, let's just fix this.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5531-deep-submodule-push.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex e2c37fd978..4ad059e6be 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -363,7 +363,7 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs\n \t\t) &&\n \t\tgit add b &&\n \t\tgit commit -m \"added submodule\" &&\n-\t\tgit push --recurse-submodule=check origin master\n+\t\tgit push --recurse-submodules=check origin master\n \t)\n '\n \n-- \ngitgitgadget\n\n"},{"id":"372449","messageId":"017ec9a0c74c770a42843eef65f49b7cc4397728.1553537656.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 2/8] tests (rebase): spell out the `--force-rebase` option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:19Z","receivedAt":"2019-03-25T18:14:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn quite a few test cases, we were sloppy and used the abbreviation\n`--force`, but we really should be precise in what we want to test.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3415-rebase-autosquash.sh | 2 +-\n t/t3430-rebase-merges.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 13f5688135..22d218698e 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -277,7 +277,7 @@ test_expect_success 'autosquash with empty custom instructionFormat' '\n \t(\n \t\tset_cat_todo_editor &&\n \t\ttest_must_fail git -c rebase.instructionFormat= \\\n-\t\t\trebase --autosquash --force -i HEAD^ >actual &&\n+\t\t\trebase --autosquash --force-rebase -i HEAD^ >actual &&\n \t\tgit log -1 --format=\"pick %h %s\" >expect &&\n \t\ttest_cmp expect actual\n \t)\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 4c69255ee6..42ba5b9f09 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -271,7 +271,7 @@ test_expect_success 'root commits' '\n \tEOF\n \ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n \ttest_tick &&\n-\tgit rebase -i --force --root -r &&\n+\tgit rebase -i --force-rebase --root -r &&\n \ttest \"Parsnip\" = \"$(git show -s --format=%an HEAD^)\" &&\n \ttest $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&\n \ttest $(git rev-parse second-root:second-root.t) = \\\n@@ -364,7 +364,7 @@ test_expect_success 'octopus merges' '\n \ttest_cmp_rev HEAD $before &&\n \n \ttest_tick &&\n-\tgit rebase -i --force -r HEAD^^ &&\n+\tgit rebase -i --force-rebase -r HEAD^^ &&\n \ttest \"Hank\" = \"$(git show -s --format=%an HEAD)\" &&\n \ttest \"$before\" != $(git rev-parse HEAD) &&\n \ttest_cmp_graph HEAD^^.. <<-\\EOF\n-- \ngitgitgadget\n\n"},{"id":"372450","messageId":"04c36b1de9f22d7e0c64bb118eb424c1f64bd223.1553537656.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:23Z","receivedAt":"2019-03-25T18:14:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nGit's command-line parsers support uniquely abbreviated options, e.g.\n`git init --ba` would automatically expand `--ba` to `--bare`.\n\nThis is a very convenient feature in every day life for Git users, in\nparticular when tab completion is not available.\n\nHowever, it is not a good idea to rely on that in Git's test suite, as\nsomething that is a unique abbreviation of a command line option today\nmight no longer be a unique abbreviation tomorrow.\n\nFor example, if a future contribution added a new mode\n`git init --babyproofing` and a previously-introduced test case used the\nfact that `git init --ba` expaneded to `git init --bare`, that future\ncontribution would now have to touch seemingly unrelated tests just to\nkeep the test suite from failing.\n\nSo let's disallow abbreviated options in the test suite by default.\n\nNote: for ease of implementation, this patch really only touches the\n`parse-options` machinery: more and more hand-rolled option parsers are\nconverted to use that internal API, and more and more scripts are\nconverted to built-ins (naturally using the parse-options API, too), so\nin practice this catches most issues, and is definitely the biggest bang\nfor the buck.\n\nSigned-off-by: Johannes Schindelin \n---\n parse-options.c | 9 +++++++++\n t/README | 4 ++++\n t/t0040-parse-options.sh | 7 ++++++-\n t/test-lib.sh | 6 ++++++\n 4 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex cec74522e5..acc3a93660 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -6,6 +6,8 @@\n #include \"color.h\"\n #include \"utf8.h\"\n \n+static int disallow_abbreviated_options;\n+\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(\n \t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n \t}\n \n+\tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n+\t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n+\t\t (int)(arg_end - arg), arg);\n+\n \tif (ambiguous_option) {\n \t\terror(_(\"ambiguous option: %s \"\n \t\t\t\"(could be --%s%s or --%s%s)\"),\n@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,\n {\n \tstruct parse_opt_ctx_t ctx;\n \n+\tdisallow_abbreviated_options =\n+\t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n+\n \tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\ndiff --git a/t/README b/t/README\nindex 656288edce..9ed3051a1c 100644\n--- a/t/README\n+++ b/t/README\n@@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n fetch-pack to not request sideband-all (even if the server advertises\n sideband-all).\n \n+GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=, when true (which is\n+the default when running tests), errors out when an abbreviated option\n+is used.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex b8f366c442..5f6a16336d 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -203,20 +203,24 @@ file: (not set)\n EOF\n \n test_expect_success 'unambiguously abbreviated option' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'unambiguously abbreviated option with \"=\"' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --expect=\"integer: 2\" --int=2\n '\n \n test_expect_success 'ambiguously abbreviated option' '\n-\ttest_expect_code 129 test-tool parse-options --strin 123\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest-tool parse-options --strin 123\n '\n \n test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --expect=\"string: 123\" --st 123\n '\n \n@@ -325,6 +329,7 @@ file: (not set)\n EOF\n \n test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --no-ambig >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 562c57e685..e550009411 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -57,6 +57,12 @@ fi\n . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n export PERL_PATH SHELL_PATH\n \n+# Disallow the use of abbreviated options in the test suite by default\n+test -n \"$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\" || {\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n+\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n+}\n+\n ################################################################\n # It appears that people try to run tests without building...\n \"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X\" >/dev/null\n-- \ngitgitgadget\n"},{"id":"372451","messageId":"531450c00dfe517748d9ad0bc77b9871c45042aa.1553537656.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 6/8] tests (status): spell out the `--find-renames` option in full","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:22Z","receivedAt":"2019-03-25T18:14:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTo avoid future ambiguities, we really want to use full option names in\nthe test suite. `t7525-status-rename.sh` used an abbreviated form of the\n`--find-renames` option, though, so let's change that.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7525-status-rename.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh\nindex ef8b1b3078..a62736dce0 100755\n--- a/t/t7525-status-rename.sh\n+++ b/t/t7525-status-rename.sh\n@@ -84,7 +84,7 @@ test_expect_success 'status score=100%' '\n \ttest_i18ngrep \"deleted:\" actual &&\n \ttest_i18ngrep \"new file:\" actual &&\n \n-\tgit status --find-rename=100% >actual &&\n+\tgit status --find-renames=100% >actual &&\n \ttest_i18ngrep \"deleted:\" actual &&\n \ttest_i18ngrep \"new file:\" actual\n '\n@@ -93,11 +93,11 @@ test_expect_success 'status score=01%' '\n \tgit status -M=01% >actual &&\n \ttest_i18ngrep \"renamed:\" actual &&\n \n-\tgit status --find-rename=01% >actual &&\n+\tgit status --find-renames=01% >actual &&\n \ttest_i18ngrep \"renamed:\" actual\n '\n \n-test_expect_success 'copies not overridden by find-rename' '\n+test_expect_success 'copies not overridden by find-renames' '\n \tcp renamed copy &&\n \tgit add copy &&\n \n@@ -105,7 +105,7 @@ test_expect_success 'copies not overridden by find-rename' '\n \ttest_i18ngrep \"copied:\" actual &&\n \ttest_i18ngrep \"renamed:\" actual &&\n \n-\tgit -c status.renames=copies status --find-rename=01% >actual &&\n+\tgit -c status.renames=copies status --find-renames=01% >actual &&\n \ttest_i18ngrep \"copied:\" actual &&\n \ttest_i18ngrep \"renamed:\" actual\n '\n-- \ngitgitgadget\n\n"},{"id":"372452","messageId":"a722a065d2c3249bddd987cca9a37779a81bb378.1553537656.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-03-25T18:14:23Z","receivedAt":"2019-03-25T18:14:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTo use the singular form of a word, when the option wants the plural\nform (and quietly expands it because it thinks it was abbreviated), is\nan easy mistake to make, and t5317 contains almost two dozen of them.\n\nHowever, using abbreviated options in tests is a bit fragile, so we will\ndisallow use of abbreviated options in our test suite.\n\nIn preparation for this change, let's fix\n`t5317-pack-objects-filter-objects.sh`.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------\n 1 file changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 24541ea137..4c0201c34b 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -25,7 +25,7 @@ test_expect_success 'verify blob count in normal packfile' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&\n+\tgit -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r1 index-pack ../all.pack &&\n@@ -39,7 +39,7 @@ test_expect_success 'verify blob count in normal packfile' '\n '\n \n test_expect_success 'verify blob:none packfile has no blobs' '\n-\tgit -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF &&\n+\tgit -C r1 pack-objects --revs --stdout --filter=blob:none >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r1 index-pack ../filter.pack &&\n@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' '\n \tgit -C r5 commit -m \"foo\" &&\n \tdel=$(git -C r5 rev-parse HEAD^{tree} | sed \"s|..|&/|\") &&\n \trm r5/.git/objects/$del &&\n-\ttest_must_fail git -C r5 pack-objects --rev --stdout 2>bad_tree <<-EOF &&\n+\ttest_must_fail git -C r5 pack-objects --revs --stdout 2>bad_tree <<-EOF &&\n \tHEAD\n \tEOF\n \tgrep \"bad tree object\" bad_tree\n@@ -88,7 +88,7 @@ test_expect_success 'setup for tests of tree:0' '\n '\n \n test_expect_success 'verify tree:0 packfile has no blobs or trees' '\n-\tgit -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&\n+\tgit -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r1 index-pack ../commitsonly.pack &&\n@@ -98,7 +98,7 @@ test_expect_success 'verify tree:0 packfile has no blobs or trees' '\n \n test_expect_success 'grab tree directly when using tree:0' '\n \t# We should get the tree specified directly but not its blobs or subtrees.\n-\tgit -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&\n+\tgit -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&\n \tHEAD:\n \tEOF\n \tgit -C r1 index-pack ../commitsonly.pack &&\n@@ -128,7 +128,7 @@ test_expect_success 'verify blob count in normal packfile' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout >all.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../all.pack &&\n@@ -142,7 +142,7 @@ test_expect_success 'verify blob count in normal packfile' '\n '\n \n test_expect_success 'verify blob:limit=500 omits all blobs' '\n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -157,7 +157,7 @@ test_expect_success 'verify blob:limit=500 omits all blobs' '\n '\n \n test_expect_success 'verify blob:limit=1000' '\n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -176,7 +176,7 @@ test_expect_success 'verify blob:limit=1001' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -194,7 +194,7 @@ test_expect_success 'verify blob:limit=10001' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -212,7 +212,7 @@ test_expect_success 'verify blob:limit=1k' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -230,7 +230,7 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n \tHEAD\n \t$(git -C r2 rev-parse HEAD:large.10000)\n \tEOF\n@@ -249,7 +249,7 @@ test_expect_success 'verify blob:limit=1m' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -302,7 +302,7 @@ test_expect_success 'verify blob count in normal packfile' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r3 pack-objects --rev --stdout >all.pack <<-EOF &&\n+\tgit -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r3 index-pack ../all.pack &&\n@@ -320,7 +320,7 @@ test_expect_success 'verify sparse:path=pattern1' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&\n+\tgit -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r3 index-pack ../filter.pack &&\n@@ -352,7 +352,7 @@ test_expect_success 'verify sparse:path=pattern2' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&\n+\tgit -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r3 index-pack ../filter.pack &&\n@@ -404,7 +404,7 @@ test_expect_success 'verify blob count in normal packfile' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r4 pack-objects --rev --stdout >all.pack <<-EOF &&\n+\tgit -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r4 index-pack ../all.pack &&\n@@ -423,7 +423,7 @@ test_expect_success 'verify sparse:oid=OID' '\n \tsort >expected &&\n \n \toid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) &&\n-\tgit -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&\n+\tgit -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r4 index-pack ../filter.pack &&\n@@ -441,7 +441,7 @@ test_expect_success 'verify sparse:oid=oid-ish' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&\n+\tgit -C r4 pack-objects --revs --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r4 index-pack ../filter.pack &&\n@@ -470,19 +470,19 @@ test_expect_success 'setup r1 - delete loose blobs' '\n '\n \n test_expect_success 'verify pack-objects fails w/ missing objects' '\n-\ttest_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF\n+\ttest_must_fail git -C r1 pack-objects --revs --stdout >miss.pack <<-EOF\n \tHEAD\n \tEOF\n '\n \n test_expect_success 'verify pack-objects fails w/ --missing=error' '\n-\ttest_must_fail git -C r1 pack-objects --rev --stdout --missing=error >miss.pack <<-EOF\n+\ttest_must_fail git -C r1 pack-objects --revs --stdout --missing=error >miss.pack <<-EOF\n \tHEAD\n \tEOF\n '\n \n test_expect_success 'verify pack-objects w/ --missing=allow-any' '\n-\tgit -C r1 pack-objects --rev --stdout --missing=allow-any >miss.pack <<-EOF\n+\tgit -C r1 pack-objects --revs --stdout --missing=allow-any >miss.pack <<-EOF\n \tHEAD\n \tEOF\n '\n-- \ngitgitgadget\n\n"},{"id":"372453","messageId":"20190325183505.GA28920@dev-l","threadId":"50818","inReplyTo":"04c36b1de9f22d7e0c64bb118eb424c1f64bd223.1553537656.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-03-25T18:35:05Z","receivedAt":"2019-03-25T18:35:10Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Johannes,\n\nThanks for catching this. Perhaps I should've been more diligent and ran\nthe entire test suite before submitting but I was running low on\nbatteries only ran the rebase-related tests.\n\nOn Mon, Mar 25, 2019 at 11:14:23AM -0700, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> Git's command-line parsers support uniquely abbreviated options, e.g.\n> `git init --ba` would automatically expand `--ba` to `--bare`.\n> \n> This is a very convenient feature in every day life for Git users, in\n> particular when tab completion is not available.\n> \n> However, it is not a good idea to rely on that in Git's test suite, as\n> something that is a unique abbreviation of a command line option today\n> might no longer be a unique abbreviation tomorrow.\n> \n> For example, if a future contribution added a new mode\n> `git init --babyproofing` and a previously-introduced test case used the\n> fact that `git init --ba` expaneded to `git init --bare`, that future\n\ns/expaneded/expanded/\n\n> contribution would now have to touch seemingly unrelated tests just to\n> keep the test suite from failing.\n> \n> So let's disallow abbreviated options in the test suite by default.\n> \n> Note: for ease of implementation, this patch really only touches the\n> `parse-options` machinery: more and more hand-rolled option parsers are\n> converted to use that internal API, and more and more scripts are\n> converted to built-ins (naturally using the parse-options API, too), so\n> in practice this catches most issues, and is definitely the biggest bang\n> for the buck.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> parse-options.c | 9 +++++++++\n> t/README | 4 ++++\n> t/t0040-parse-options.sh | 7 ++++++-\n> t/test-lib.sh | 6 ++++++\n> 4 files changed, 25 insertions(+), 1 deletion(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index cec74522e5..acc3a93660 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -6,6 +6,8 @@\n> #include \"color.h\"\n> #include \"utf8.h\"\n> \n> +static int disallow_abbreviated_options;\n> +\n> #define OPT_SHORT 1\n> #define OPT_UNSET 2\n> \n> @@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(\n> \t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n> \t}\n> \n> +\tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n> +\t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n> +\t\t (int)(arg_end - arg), arg);\n> +\n> \tif (ambiguous_option) {\n> \t\terror(_(\"ambiguous option: %s \"\n> \t\t\t\"(could be --%s%s or --%s%s)\"),\n> @@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,\n> {\n> \tstruct parse_opt_ctx_t ctx;\n> \n> +\tdisallow_abbreviated_options =\n> +\t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n> +\n> \tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n> \tswitch (parse_options_step(&ctx, options, usagestr)) {\n> \tcase PARSE_OPT_HELP:\n> diff --git a/t/README b/t/README\n> index 656288edce..9ed3051a1c 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n> fetch-pack to not request sideband-all (even if the server advertises\n> sideband-all).\n> \n> +GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=, when true (which is\n> +the default when running tests), errors out when an abbreviated option\n> +is used.\n> +\n> Naming Tests\n> ------------\n> \n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index b8f366c442..5f6a16336d 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -203,20 +203,24 @@ file: (not set)\n> EOF\n> \n> test_expect_success 'unambiguously abbreviated option' '\n> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> \ttest-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&\n> \ttest_must_be_empty output.err &&\n> \ttest_cmp expect output\n> '\n> \n> test_expect_success 'unambiguously abbreviated option with \"=\"' '\n> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> \ttest-tool parse-options --expect=\"integer: 2\" --int=2\n> '\n> \n> test_expect_success 'ambiguously abbreviated option' '\n> -\ttest_expect_code 129 test-tool parse-options --strin 123\n> +\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> +\ttest-tool parse-options --strin 123\n> '\n> \n> test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> \ttest-tool parse-options --expect=\"string: 123\" --st 123\n> '\n> \n> @@ -325,6 +329,7 @@ file: (not set)\n> EOF\n> \n> test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> \ttest-tool parse-options --no-ambig >output 2>output.err &&\n> \ttest_must_be_empty output.err &&\n> \ttest_cmp expect output\n\nWould it make sense to include a test case to ensure that\nGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?\n\nThanks,\n\nDenton\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 562c57e685..e550009411 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -57,6 +57,12 @@ fi\n> . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n> export PERL_PATH SHELL_PATH\n> \n> +# Disallow the use of abbreviated options in the test suite by default\n> +test -n \"$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\" || {\n> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n> +\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n> +}\n> +\n> ################################################################\n> # It appears that people try to run tests without building...\n> \"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X\" >/dev/null\n> -- \n> gitgitgadget\n"},{"id":"372463","messageId":"87sgvabutb.fsf@evledraar.gmail.com","threadId":"50818","inReplyTo":"04c36b1de9f22d7e0c64bb118eb424c1f64bd223.1553537656.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T19:47:12Z","receivedAt":"2019-03-25T19:47:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 25 2019, Johannes Schindelin via GitGitGadget wrote:\n\n> +# Disallow the use of abbreviated options in the test suite by default\n> +test -n \"$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\" || {\n> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n> +\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n> +}\n> +\n\nJust using the if test ...\\nthen\\nfi long-form would be consistent with\nour usual style & the rest of this file.\n"},{"id":"372464","messageId":"20190325202329.26033-1-avarab@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 0/2] allow for configuring option abbreviation + fix","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T20:23:27Z","receivedAt":"2019-03-25T20:23:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is based on Johannes's just-submitted\nhttps://public-inbox.org/git/pull.167.git.gitgitgadget@gmail.com/\n\nSome of this in my 1/2 could obviously be shortened if the\n\"GIT_TEST_ABBREVIATED_OPTIONS\" name was used by default, but I'll\nleave it up to discussion whether it's worth it to go for the route I\ntook here, and if so to rebase the whole thing or not.\n\nWhile I'm at it fix a bug I noticed a while ago in 2/2 related to the\noption abbreviation being silly and tripping over itself over an\naliased option.\n\nÆvar Arnfjörð Bjarmason (2):\n parse-options: allow for configuring option abbreviation\n parse-options: don't emit \"ambiguous option\" for aliases\n\n Documentation/config/core.txt | 12 ++++++++++++\n builtin/clone.c | 4 ++--\n parse-options.c | 22 +++++++++++++++++-----\n parse-options.h | 2 ++\n t/README | 4 ++--\n t/t0040-parse-options.sh | 27 ++++++++++++++++++++++-----\n t/test-lib.sh | 6 +++---\n 7 files changed, 60 insertions(+), 17 deletions(-)\n\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372465","messageId":"20190325202329.26033-2-avarab@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T20:23:28Z","receivedAt":"2019-03-25T20:23:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a new core.abbreviatedOptions which corresponds to the newly added\nGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS test mode. This allows\ne.g. script authors to enact the same sort of paranoia about option\nabbreviation as our own test suite now uses by default.\n\nI'm renaming GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS to\nGIT_TEST_ABBREVIATED_OPTIONS for consistency with the new\nconfiguration variable, and I've picked the shorter\ncore.abbreviatedOptions instead of core.disallowAbbreviatedOptions\nbecause it's easier to reason about a variable that's true by default\nthan about one that has a negation in its name.\n\nWhile I'm at it, factor out the check for the environment variable\ninto a parse_options_config() which similar to the code added in\n62c23938fa (\"tests: add a special setup where rebase.useBuiltin is\noff\", 2018-11-14) will first check the environment variable, and then\nthe configuration.\n\nUsing git_env_bool() means it's a bit of a pain to distinguish between\nthe case where GIT_TEST_ABBREVIATED_OPTIONS is set to \"true\" or\n\"false\" v.s. being empty, hence the\ntest_when_finished/export/sane_unset dance in the test that's being\nadded, but I think that's more sane than introducing a \"bool but not\nin the same way as the rest\" GIT_TEST_* variable.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config/core.txt | 12 ++++++++++++\n parse-options.c | 19 +++++++++++++++----\n t/README | 4 ++--\n t/t0040-parse-options.sh | 22 +++++++++++++++++-----\n t/test-lib.sh | 6 +++---\n 5 files changed, 49 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 7e9b6c8f4c..5a42ec3940 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -1,3 +1,15 @@\n+core.abbreviatedOptions::\n+\tDefaults to `true` which allows options to be abbreviated as\n+\tlong as they aren't ambiguous, e.g. for linkgit:git-init[1]\n+\tthe `--bare` option can be abbreviated as `--bar`, `--ba` or\n+\teven `--b` since no other option starts with those\n+\tprefixes. However, if such an option were added in the future\n+\tany use of these abbreviations would break.\n++\n+By setting this to false (e.g. in scripts) you can defend against such\n+future breakages by enforcing that options must always be fully\n+provided.\n+\n core.fileMode::\n \tTells Git if the executable bit of files in the working tree\n \tis to be honored.\ndiff --git a/parse-options.c b/parse-options.c\nindex acc3a93660..d6a291f705 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -6,7 +6,7 @@\n #include \"color.h\"\n #include \"utf8.h\"\n \n-static int disallow_abbreviated_options;\n+static int abbreviated_options = 1;\n \n #define OPT_SHORT 1\n #define OPT_UNSET 2\n@@ -346,7 +346,7 @@ static enum parse_opt_result parse_long_opt(\n \t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n \t}\n \n-\tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n+\tif (!abbreviated_options && (ambiguous_option || abbrev_option))\n \t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n \t\t (int)(arg_end - arg), arg);\n \n@@ -456,6 +456,17 @@ static void parse_options_check(const struct option *opts)\n \t\texit(128);\n }\n \n+static void parse_options_config(void)\n+{\n+\tint env = git_env_bool(\"GIT_TEST_ABBREVIATED_OPTIONS\", -1);\n+\tif (env != -1) {\n+\t\tabbreviated_options = env;\n+\t\treturn;\n+\t}\n+\tgit_config_get_bool(\"core.abbreviatedoptions\", &abbreviated_options);\n+\treturn;\n+}\n+\n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n \t\t\t const struct option *options, int flags)\n@@ -480,6 +491,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t (flags & PARSE_OPT_KEEP_ARGV0))\n \t\tBUG(\"Can't keep argv0 if you don't have it\");\n \tparse_options_check(options);\n+\tparse_options_config();\n }\n \n static void show_negated_gitcomp(const struct option *opts, int nr_noopts)\n@@ -714,9 +726,8 @@ int parse_options(int argc, const char **argv, const char *prefix,\n {\n \tstruct parse_opt_ctx_t ctx;\n \n-\tdisallow_abbreviated_options =\n-\t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n \n+\tparse_options_config();\n \tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\ndiff --git a/t/README b/t/README\nindex 9ed3051a1c..1d628baf31 100644\n--- a/t/README\n+++ b/t/README\n@@ -399,9 +399,9 @@ GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n fetch-pack to not request sideband-all (even if the server advertises\n sideband-all).\n \n-GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=, when true (which is\n+GIT_TEST_ABBREVIATED_OPTIONS=, when false (which is\n the default when running tests), errors out when an abbreviated option\n-is used.\n+is used. Overrides the core.abbreviatedOptions setting.\n \n Naming Tests\n ------------\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 5f6a16336d..19685d1582 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -203,27 +203,39 @@ file: (not set)\n EOF\n \n test_expect_success 'unambiguously abbreviated option' '\n-\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\tGIT_TEST_ABBREVIATED_OPTIONS=true \\\n \ttest-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'unambiguously abbreviated option with \"=\"' '\n-\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\tGIT_TEST_ABBREVIATED_OPTIONS=true \\\n \ttest-tool parse-options --expect=\"integer: 2\" --int=2\n '\n \n test_expect_success 'ambiguously abbreviated option' '\n-\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest_expect_code 129 env GIT_TEST_ABBREVIATED_OPTIONS=true \\\n \ttest-tool parse-options --strin 123\n '\n \n test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n-\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\tGIT_TEST_ABBREVIATED_OPTIONS=true \\\n \ttest-tool parse-options --expect=\"string: 123\" --st 123\n '\n \n+test_expect_success 'abbreviated options configured with core.abbreviatedOptions' '\n+\ttest_when_finished \"\n+\t\trm -rf A B C &&\n+\t\tGIT_TEST_ABBREVIATED_OPTIONS=$GIT_TEST_ABBREVIATED_OPTIONS &&\n+\t\texport GIT_TEST_ABBREVIATED_OPTIONS\n+\t\" &&\n+\tgit init --bare A &&\n+\ttest_must_fail git init --ba B &&\n+\tsane_unset GIT_TEST_ABBREVIATED_OPTIONS &&\n+\tgit -c core.abbreviatedOptions=true init --ba C\n+'\n+\n cat >typo.err <<\\EOF\n error: did you mean `--boolean` (with two dashes ?)\n EOF\n@@ -329,7 +341,7 @@ file: (not set)\n EOF\n \n test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n-\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\tGIT_TEST_ABBREVIATED_OPTIONS=true \\\n \ttest-tool parse-options --no-ambig >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e550009411..4b0dd8a1a9 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -58,9 +58,9 @@ fi\n export PERL_PATH SHELL_PATH\n \n # Disallow the use of abbreviated options in the test suite by default\n-test -n \"$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\" || {\n-\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n-\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n+test -n \"$GIT_TEST_ABBREVIATED_OPTIONS\" || {\n+\tGIT_TEST_ABBREVIATED_OPTIONS=false\n+\texport GIT_TEST_ABBREVIATED_OPTIONS\n }\n \n ################################################################\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372466","messageId":"20190325202329.26033-3-avarab@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T20:23:29Z","receivedAt":"2019-03-25T20:23:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the option parsing machinery so that e.g. \"clone --recurs ...\"\ndoesn't error out because \"clone\" understands both \"--recursive\" and\n\"--recurse-submodules\" to mean the same thing.\n\nInitially \"clone\" just understood --recursive until the\n--recurses-submodules alias was added in ccdd3da652 (\"clone: Add the\n--recurse-submodules option as alias for --recursive\",\n2010-11-04). Since bb62e0a99f (\"clone: teach --recurse-submodules to\noptionally take a pathspec\", 2017-03-17) the longer form has been\npromoted to the default.\n\nBut due to the way the options parsing machinery works this resulted\nin the rather absurd situation of:\n\n $ git clone --recurs [...]\n error: ambiguous option: recurs (could be --recursive or --recurse-submodules)\n\nLet's re-use the PARSE_OPT_NOCOMPLETE flag to mean \"this option\ndoesn't contribute to abbreviation ambiguity\". I was going to add a\nnew PARSE_OPT_NOABBREV flag, but it makes sense just to re-use\nPARSE_OPT_NOCOMPLETE.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/clone.c | 4 ++--\n parse-options.c | 3 ++-\n parse-options.h | 2 ++\n t/t0040-parse-options.sh | 5 +++++\n 4 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..4dc26969a7 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"setup as shared repository\")),\n \t{ OPTION_CALLBACK, 0, \"recursive\", &option_recurse_submodules,\n \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n-\t PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,\n-\t (intptr_t)\".\" },\n+\t PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,\n+\t recurse_submodules_cb, (intptr_t)\".\" },\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n \t PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\ndiff --git a/parse-options.c b/parse-options.c\nindex d6a291f705..84f3a2996f 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n \t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n-\t\t\t !strncmp(long_name, arg, arg_end - arg)) {\n+\t\t\t !strncmp(long_name, arg, arg_end - arg) &&\n+\t\t\t !(options->flags & PARSE_OPT_NOCOMPLETE)) {\n is_abbreviated:\n \t\t\t\tif (abbrev_option) {\n \t\t\t\t\t/*\ndiff --git a/parse-options.h b/parse-options.h\nindex 74cce4e7fc..9362a397ae 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n *\t\t\t\tUseful for options with multiple parameters.\n * PARSE_OPT_NOCOMPLETE: by default all visible options are completable\n *\t\t\t by git-completion.bash. This option suppresses that.\n+ *\t\t\t Will also skip this option when abbreviation is\n+ *\t\t\t considered. See core.abbreviatedOptions.\n * PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to\n *\t\t\t complete an option as --name= not --name even if\n *\t\t\t the option takes optional argument.\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 19685d1582..c1ea50aa85 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -236,6 +236,11 @@ test_expect_success 'abbreviated options configured with core.abbreviatedOptions\n \tgit -c core.abbreviatedOptions=true init --ba C\n '\n \n+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '\n+\ttest_when_finished \"rm -rf A\" &&\n+\tGIT_TEST_ABBREVIATED_OPTIONS=true git clone --recurs . A\n+'\n+\n cat >typo.err <<\\EOF\n error: did you mean `--boolean` (with two dashes ?)\n EOF\n-- \n2.21.0.360.g471c308f928\n\n"},{"id":"372467","messageId":"87r2aubsza.fsf@evledraar.gmail.com","threadId":"50818","inReplyTo":"20190325183505.GA28920@dev-l","subject":"Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T20:26:49Z","receivedAt":"2019-03-25T20:26:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 25 2019, Denton Liu wrote:\n\n> Hi Johannes,\n>\n> Thanks for catching this. Perhaps I should've been more diligent and ran\n> the entire test suite before submitting but I was running low on\n> batteries only ran the rebase-related tests.\n>\n> On Mon, Mar 25, 2019 at 11:14:23AM -0700, Johannes Schindelin via GitGitGadget wrote:\n>> From: Johannes Schindelin \n>> [...]\n>> @@ -325,6 +329,7 @@ file: (not set)\n>> EOF\n>>\n>> test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n>> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n>> \ttest-tool parse-options --no-ambig >output 2>output.err &&\n>> \ttest_must_be_empty output.err &&\n>> \ttest_cmp expect output\n>\n> Would it make sense to include a test case to ensure that\n> GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?\n\nTo elaborate on this since one might wonder \"but aren't these those\ntests?\". I think you mean if we shouldn't have a \"test_must_fail\" test\nthere that asserts that parsing the options will die with the default of\nGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true. Yeah, that makes sense, it's\ncurrently a blind spot that we just assume will keep working.\n"},{"id":"372473","messageId":"CAPig+cR0Ldt3EpQ683ZFNFXggfsTrdeZ3R-V6pDBZNA1N3c+xg@mail.gmail.com","threadId":"50818","inReplyTo":"20190325202329.26033-2-avarab@gmail.com","subject":"Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-25T21:23:25Z","receivedAt":"2019-03-25T21:23:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason\n wrote:\n> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> @@ -1,3 +1,15 @@\n> +core.abbreviatedOptions::\n> + Defaults to `true` which allows options to be abbreviated as\n> + long as they aren't ambiguous, e.g. for linkgit:git-init[1]\n> + the `--bare` option can be abbreviated as `--bar`, `--ba` or\n> + even `--b` since no other option starts with those\n> + prefixes. However, if such an option were added in the future\n> + any use of these abbreviations would break.\n> ++\n> +By setting this to false (e.g. in scripts) you can defend against such\n> +future breakages by enforcing that options must always be fully\n> +provided.\n\nI don't get why having a configuration option is better for defending\nscripts against this problem than a simple environment variable. It\nseems easier for the script prologue to contain:\n\n GIT_TEST_ABBREVIATED_OPTIONS=false\n export GIT_TEST_ABBREVIATED_OPTIONS\n\nthan for it to muck about with git-config or use \"git -c\ncore.abbreviatedOptions=false ...\" everywhere. The commit message\ndoesn't do a good enough job of justifying the configuration option\nover the environment variable.\n\nAlso, if this is now intended to be more general (aiding script\nwriters) than just being for our test suite, then dropping \"TEST\" from\nthe name seems warranted:\n\n GIT_ABBREVIATED_OPTIONS\n"},{"id":"372483","messageId":"87o95ybmgf.fsf@evledraar.gmail.com","threadId":"50818","inReplyTo":"CAPig+cR0Ldt3EpQ683ZFNFXggfsTrdeZ3R-V6pDBZNA1N3c+xg@mail.gmail.com","subject":"Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-25T22:47:44Z","receivedAt":"2019-03-25T22:47:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 25 2019, Eric Sunshine wrote:\n\n> On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n>> @@ -1,3 +1,15 @@\n>> +core.abbreviatedOptions::\n>> + Defaults to `true` which allows options to be abbreviated as\n>> + long as they aren't ambiguous, e.g. for linkgit:git-init[1]\n>> + the `--bare` option can be abbreviated as `--bar`, `--ba` or\n>> + even `--b` since no other option starts with those\n>> + prefixes. However, if such an option were added in the future\n>> + any use of these abbreviations would break.\n>> ++\n>> +By setting this to false (e.g. in scripts) you can defend against such\n>> +future breakages by enforcing that options must always be fully\n>> +provided.\n>\n> I don't get why having a configuration option is better for defending\n> scripts against this problem than a simple environment variable. It\n> seems easier for the script prologue to contain:\n>\n> GIT_TEST_ABBREVIATED_OPTIONS=false\n> export GIT_TEST_ABBREVIATED_OPTIONS\n>\n> than for it to muck about with git-config or use \"git -c\n> core.abbreviatedOptions=false ...\" everywhere. The commit message\n> doesn't do a good enough job of justifying the configuration option\n> over the environment variable.\n>\n> Also, if this is now intended to be more general (aiding script\n> writers) than just being for our test suite, then dropping \"TEST\" from\n> the name seems warranted:\n>\n> GIT_ABBREVIATED_OPTIONS\n\nIf we want to make something user-configurable we tend to add config\nvariables. The GIT_TEST_* variables are only intended for our own test\nsuite, see t/README.\n\nI don't mind documenting this, but it's a well-established pattern, so\nif we're going to describe how this works/why use one or the other it\nshould probably be some other series to t/README and/or git-config.txt\n\nWe traditionally *only* expose this sort of thing to users via config,\nand not via env variables.\n\nThe config system is more flexible in every way. You can set it\nsystem-wide, user-wide, repo-wide etc., and if you want exactly the\nscope of an env variable you can do that too, just start your script\nwith:\n\n # These \"''\" quotes are not a mistake, it needs to be like this\n export GIT_CONFIG_PARAMETERS=\"'core.abbreviatedOptions=false'\"\n git [...]\n\nSo the reason we have GIT_TEST_* is pretty much because we can't just\nwhitelist GIT_CONFIG_PARAMETERS for the test suite, and to make it\nobvious what test modes we have available.\n"},{"id":"372491","messageId":"CACsJy8Bk=Z8BaVeAhKzF4PWYLLG76cADooHKNw+Xy3EztZL1DQ@mail.gmail.com","threadId":"50818","inReplyTo":"87o95ybmgf.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-26T04:14:05Z","receivedAt":"2019-03-26T04:14:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 26, 2019 at 5:48 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Mon, Mar 25 2019, Eric Sunshine wrote:\n>\n> > On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason\n> > wrote:\n> >> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n> >> @@ -1,3 +1,15 @@\n> >> +core.abbreviatedOptions::\n> >> + Defaults to `true` which allows options to be abbreviated as\n> >> + long as they aren't ambiguous, e.g. for linkgit:git-init[1]\n> >> + the `--bare` option can be abbreviated as `--bar`, `--ba` or\n> >> + even `--b` since no other option starts with those\n> >> + prefixes. However, if such an option were added in the future\n> >> + any use of these abbreviations would break.\n> >> ++\n> >> +By setting this to false (e.g. in scripts) you can defend against such\n> >> +future breakages by enforcing that options must always be fully\n> >> +provided.\n> >\n> > I don't get why having a configuration option is better for defending\n> > scripts against this problem than a simple environment variable. It\n> > seems easier for the script prologue to contain:\n> >\n> > GIT_TEST_ABBREVIATED_OPTIONS=false\n> > export GIT_TEST_ABBREVIATED_OPTIONS\n> >\n> > than for it to muck about with git-config or use \"git -c\n> > core.abbreviatedOptions=false ...\" everywhere. The commit message\n> > doesn't do a good enough job of justifying the configuration option\n> > over the environment variable.\n> >\n> > Also, if this is now intended to be more general (aiding script\n> > writers) than just being for our test suite, then dropping \"TEST\" from\n> > the name seems warranted:\n> >\n> > GIT_ABBREVIATED_OPTIONS\n>\n> If we want to make something user-configurable we tend to add config\n> variables. The GIT_TEST_* variables are only intended for our own test\n> suite, see t/README.\n>\n> I don't mind documenting this, but it's a well-established pattern, so\n> if we're going to describe how this works/why use one or the other it\n> should probably be some other series to t/README and/or git-config.txt\n>\n> We traditionally *only* expose this sort of thing to users via config,\n> and not via env variables.\n\nIf this is mostly useful for scripts then I agree with Eric an\nenvironment variable is the way to go. A configuration variable does\nnot make it more convenient.\n\nAnd no we don't only export via config. There are a bunch of public\nenv variables in git.txt. \"core\" namespace is already very crowded. If\nthis one is only rarely used, I'd rather not add a new config\nvariable.\n-- \nDuy\n"},{"id":"372497","messageId":"87lg12b13p.fsf@evledraar.gmail.com","threadId":"50818","inReplyTo":"CACsJy8Bk=Z8BaVeAhKzF4PWYLLG76cADooHKNw+Xy3EztZL1DQ@mail.gmail.com","subject":"Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-26T06:28:58Z","receivedAt":"2019-03-26T06:29:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 26 2019, Duy Nguyen wrote:\n\n> On Tue, Mar 26, 2019 at 5:48 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>>\n>> On Mon, Mar 25 2019, Eric Sunshine wrote:\n>>\n>> > On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason\n>> > wrote:\n>> >> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\n>> >> @@ -1,3 +1,15 @@\n>> >> +core.abbreviatedOptions::\n>> >> + Defaults to `true` which allows options to be abbreviated as\n>> >> + long as they aren't ambiguous, e.g. for linkgit:git-init[1]\n>> >> + the `--bare` option can be abbreviated as `--bar`, `--ba` or\n>> >> + even `--b` since no other option starts with those\n>> >> + prefixes. However, if such an option were added in the future\n>> >> + any use of these abbreviations would break.\n>> >> ++\n>> >> +By setting this to false (e.g. in scripts) you can defend against such\n>> >> +future breakages by enforcing that options must always be fully\n>> >> +provided.\n>> >\n>> > I don't get why having a configuration option is better for defending\n>> > scripts against this problem than a simple environment variable. It\n>> > seems easier for the script prologue to contain:\n>> >\n>> > GIT_TEST_ABBREVIATED_OPTIONS=false\n>> > export GIT_TEST_ABBREVIATED_OPTIONS\n>> >\n>> > than for it to muck about with git-config or use \"git -c\n>> > core.abbreviatedOptions=false ...\" everywhere. The commit message\n>> > doesn't do a good enough job of justifying the configuration option\n>> > over the environment variable.\n>> >\n>> > Also, if this is now intended to be more general (aiding script\n>> > writers) than just being for our test suite, then dropping \"TEST\" from\n>> > the name seems warranted:\n>> >\n>> > GIT_ABBREVIATED_OPTIONS\n>>\n>> If we want to make something user-configurable we tend to add config\n>> variables. The GIT_TEST_* variables are only intended for our own test\n>> suite, see t/README.\n>>\n>> I don't mind documenting this, but it's a well-established pattern, so\n>> if we're going to describe how this works/why use one or the other it\n>> should probably be some other series to t/README and/or git-config.txt\n>>\n>> We traditionally *only* expose this sort of thing to users via config,\n>> and not via env variables.\n\nFWIW I replied so quickly to this with patches since I had something WIP\nto do this, it's annoyed me as a user in the past that I couldn't turn\nthis off, IIRC some upgrade of git broke my \"bad\" muscle memory /\nscripts.\n\n> If this is mostly useful for scripts then I agree with Eric an\n> environment variable is the way to go. A configuration variable does\n> not make it more convenient.\n\nI think both of you might be assuming that when you want to configure\nsomething it's as easy to tweak every run time environment (set an env\nvar) as it is to set a global config. See the trace2.* config discussion\nfor similar use-cases.\n\n> And no we don't only export via config. There are a bunch of public\n> env variables in git.txt. \"core\" namespace is already very crowded. If\n> this one is only rarely used, I'd rather not add a new config\n> variable.\n\nI don't see how a new \"abbreviatedOptions\" is plausibly going to crowd\nout anything else, sounds pretty unambiguous to me.\n\nBy my count out of the the existing GIT_* variables in git.txt around\n1/3 are already configurable via config, another 1/3 (all the GIT_TRACE\nstuff) is something we've wanted to have configurable in the past (but\nnobody's gotten around to writing patches for).\n\nI think it's fair to say that when we normally add user-configurable\nstuff we do it as config, not as new env vars.\n"},{"id":"372500","messageId":"CACsJy8BpToq2VAzFo61dz7Hco42Sbka5qQm3ACn_6aejUxAKOg@mail.gmail.com","threadId":"50818","inReplyTo":"87lg12b13p.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-26T07:13:26Z","receivedAt":"2019-03-26T07:13:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 26, 2019 at 1:29 PM Ævar Arnfjörð Bjarmason\n wrote:\n> I don't see how a new \"abbreviatedOptions\" is plausibly going to crowd\n> out anything else, sounds pretty unambiguous to me.\n\nBy crowded I mean a lot of configuration variables under \"core\"\n\n> By my count out of the the existing GIT_* variables in git.txt around\n> 1/3 are already configurable via config, another 1/3 (all the GIT_TRACE\n> stuff) is something we've wanted to have configurable in the past (but\n> nobody's gotten around to writing patches for).\n>\n> I think it's fair to say that when we normally add user-configurable\n> stuff we do it as config, not as new env vars.\n\nI disagree that not every configuration knob has to be a configuration\nvariable, especially when core.* more or less becomes a dictionary\nthat you can't really read anymore (unless you know the key to look\nfor). But I see you're dead set on adding config vars. Go ahead.\n-- \nDuy\n"},{"id":"372512","messageId":"87ef6tc33c.fsf@evledraar.gmail.com","threadId":"50818","inReplyTo":"CACsJy8BpToq2VAzFo61dz7Hco42Sbka5qQm3ACn_6aejUxAKOg@mail.gmail.com","subject":"Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-26T11:00:39Z","receivedAt":"2019-03-26T11:00:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 26 2019, Duy Nguyen wrote:\n\n> On Tue, Mar 26, 2019 at 1:29 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>> I don't see how a new \"abbreviatedOptions\" is plausibly going to crowd\n>> out anything else, sounds pretty unambiguous to me.\n>\n> By crowded I mean a lot of configuration variables under \"core\"\n>\n>> By my count out of the the existing GIT_* variables in git.txt around\n>> 1/3 are already configurable via config, another 1/3 (all the GIT_TRACE\n>> stuff) is something we've wanted to have configurable in the past (but\n>> nobody's gotten around to writing patches for).\n>>\n>> I think it's fair to say that when we normally add user-configurable\n>> stuff we do it as config, not as new env vars.\n>\n> I disagree that not every configuration knob has to be a configuration\n> variable, especially when core.* more or less becomes a dictionary\n> that you can't really read anymore (unless you know the key to look\n> for). But I see you're dead set on adding config vars. Go ahead.\n\nI don't mean to pile on in this thread, and cards on the table: In terms\nof stuff I can be bothered to write a patch for and stuff I *really*\nwant in git, this thing is maybe on a 2/10 out of a 1-10 scale.\n\nI.e. I'd use it, and it would probably save my ass *somewhere* down the\nline, the motivation was being bitten in the past, so once I spotted\nJohannes's series I was reminded to clean it up & submit it. But I\nwouldn't care enough to carry a forked git with this (unlike some other\nstuff).\n\nI just wanted to elaborate *in general* on the question of \"why config\nand not env\".\n\nI don't think everything we find reason to add a knob for needs to be a\nconfig variable. E.g. I can't think of a reason for why the\nGIT_*_PATHSPECS variables should be. That seems like the sort of thing\nyou'd only tweak for a *specific* git invocation. I wouldn't oppose it\nbeing config if someone had a compelling reason & patch, but can't think\nof one myself.\n\nWhat I wanted to get across in the above replies to you/Eric, but\nprobably did a bad job of is that separate & aside from the merits of\nany given new config variable there's the problem of being a \"zookeeper\"\nwith large git installations.\n\nI.e. you don't even pick the animals, or feed them, but if the zoo's on\nfire or overflowing with poop it's your problem.\n\nSo I have some servers where I'm in charge of maintaining/upgrading git,\nand it's on me if crap breaks in the aggregate, but I'm not writing\nanyone's individual scripts for them, and the runtime envs for those\nmight be varied.\n\nIt's for those sorts of cases where having the flexibility of turning\nsomething on either in the env or via system-wide or user-wide config is\nhandy. I.e. in this case I might say:\n\n \"You can run your scripts, but you'll be in 'strict mode' and need\n to spell out your options, because I'm not getting out of bed if\n your random job using git breaks because you were lazy and did a\n 'git init --ba'\".\n\nI don't think this is a use-case that's obvious, so it's worth\nelaborating on it. I suspect a lot of devs/users on this list have\nadvanced git use-cases, but they're all ones where the git processes\nthey care about/need to maintain run as a child of their login shell, or\nequivalent. In that case the config v.s. env var distinction mostly\ndoesn't matter.\n"},{"id":"372874","messageId":"xmqqtvfiuhn1.fsf@gitster-ct.c.googlers.com","threadId":"50818","inReplyTo":"CAPig+cR0Ldt3EpQ683ZFNFXggfsTrdeZ3R-V6pDBZNA1N3c+xg@mail.gmail.com","subject":"Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-01T10:47:14Z","receivedAt":"2019-04-01T10:47:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> I don't get why having a configuration option is better for defending\n> scripts against this problem than a simple environment variable. It\n> seems easier for the script prologue to contain:\n>\n> GIT_TEST_ABBREVIATED_OPTIONS=false\n> export GIT_TEST_ABBREVIATED_OPTIONS\n>\n> than for it to muck about with git-config or use \"git -c\n> core.abbreviatedOptions=false ...\" everywhere. The commit message\n> doesn't do a good enough job of justifying the configuration option\n> over the environment variable.\n\nAbsolutely.\n\nOne thing that big brotherly types would find config attractive is\nto install centrally managed /etc/gitconfig so that they can tell\nthe tracing machinery to log all git command invocations centrally;\nwith environment only system, it is not easy to arrange.\n\nBut this one under discussion is probably a use case that is at the\nother end of the extreme from that.\n\nUsers (1) may use scripts written by third-parties, (2) may also\ndevelop their own scripts, and (3) even may run Git commands\ninteractively.\n\nIt is a laudable goal to introduce a mechanism to \"notice use of\nabbreviated options that happen to be unique right now but may not\nstay unique forever and warn against it\". But (2) among the above\nthree uses is the only use case that wants the protection, which\nmakes the configuration a poor fit for the purpose.\n\n\n\n\n"},{"id":"372958","messageId":"xmqqy34tte92.fsf@gitster-ct.c.googlers.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/8] Do not use abbreviated options in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-02T00:58:01Z","receivedAt":"2019-04-02T00:58:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> We do not want to have tests that need to be changed by completely unrelated\n> patch series, just because abbreviations that used to be unique are being\n> made non-unique by said patch series.\n\nMakes sense.\n\nIf we wanted to make sure that options in abbreviated form work\ncorrectly, we should do so in dedicated parseopt-specific tests (and\nwe do in t0040, I think), not in tests for random other commands\nthat happens to feed options to them.\n\nWill queue.\n"},{"id":"373715","messageId":"nycvar.QRO.7.76.6.1904121048220.41@tvgsbejvaqbjf.bet","threadId":"50818","inReplyTo":"87r2aubsza.fsf@evledraar.gmail.com","subject":"Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-12T08:48:57Z","receivedAt":"2019-04-12T08:49:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n\nOn Mon, 25 Mar 2019, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Mon, Mar 25 2019, Denton Liu wrote:\n>\n> > Hi Johannes,\n> >\n> > Thanks for catching this. Perhaps I should've been more diligent and ran\n> > the entire test suite before submitting but I was running low on\n> > batteries only ran the rebase-related tests.\n> >\n> > On Mon, Mar 25, 2019 at 11:14:23AM -0700, Johannes Schindelin via GitGitGadget wrote:\n> >> From: Johannes Schindelin \n> >> [...]\n> >> @@ -325,6 +329,7 @@ file: (not set)\n> >> EOF\n> >>\n> >> test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n> >> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> >> \ttest-tool parse-options --no-ambig >output 2>output.err &&\n> >> \ttest_must_be_empty output.err &&\n> >> \ttest_cmp expect output\n> >\n> > Would it make sense to include a test case to ensure that\n> > GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?\n>\n> To elaborate on this since one might wonder \"but aren't these those\n> tests?\". I think you mean if we shouldn't have a \"test_must_fail\" test\n> there that asserts that parsing the options will die with the default of\n> GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true. Yeah, that makes sense, it's\n> currently a blind spot that we just assume will keep working.\n\nValid point. I added a test case in v2 (which I'll send out in a few\nmoments...)\n\nThanks,\nDscho\n"},{"id":"373716","messageId":"nycvar.QRO.7.76.6.1904121050140.41@tvgsbejvaqbjf.bet","threadId":"50818","inReplyTo":"20190325183505.GA28920@dev-l","subject":"Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-12T08:50:51Z","receivedAt":"2019-04-12T08:51:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Denton,\n\nOn Mon, 25 Mar 2019, Denton Liu wrote:\n\n> Thanks for catching this. Perhaps I should've been more diligent and ran\n> the entire test suite before submitting but I was running low on\n> batteries only ran the rebase-related tests.\n\nNo worries.\n\n> On Mon, Mar 25, 2019 at 11:14:23AM -0700, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > Git's command-line parsers support uniquely abbreviated options, e.g.\n> > `git init --ba` would automatically expand `--ba` to `--bare`.\n> >\n> > This is a very convenient feature in every day life for Git users, in\n> > particular when tab completion is not available.\n> >\n> > However, it is not a good idea to rely on that in Git's test suite, as\n> > something that is a unique abbreviation of a command line option today\n> > might no longer be a unique abbreviation tomorrow.\n> >\n> > For example, if a future contribution added a new mode\n> > `git init --babyproofing` and a previously-introduced test case used the\n> > fact that `git init --ba` expaneded to `git init --bare`, that future\n>\n> s/expaneded/expanded/\n\nThanks, fixed.\n\n> > @@ -325,6 +329,7 @@ file: (not set)\n> > EOF\n> >\n> > test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n> > +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> > \ttest-tool parse-options --no-ambig >output 2>output.err &&\n> > \ttest_must_be_empty output.err &&\n> > \ttest_cmp expect output\n>\n> Would it make sense to include a test case to ensure that\n> GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?\n\nYep, absolutely.\n\nThank you,\nDscho\n"},{"id":"373717","messageId":"nycvar.QRO.7.76.6.1904121058400.41@tvgsbejvaqbjf.bet","threadId":"50818","inReplyTo":"87sgvabutb.fsf@evledraar.gmail.com","subject":"Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-12T08:59:19Z","receivedAt":"2019-04-12T08:59:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ævar,\n\nOn Mon, 25 Mar 2019, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Mon, Mar 25 2019, Johannes Schindelin via GitGitGadget wrote:\n>\n> > +# Disallow the use of abbreviated options in the test suite by default\n> > +test -n \"$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\" || {\n> > +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n> > +\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n> > +}\n> > +\n>\n> Just using the if test ...\\nthen\\nfi long-form would be consistent with\n> our usual style & the rest of this file.\n\nYep, and we also use the `${...:+isset}` dance, so I imitated that, too.\n\nThanks for reminding me that when in Rome, do as the Romans do,\nDscho\n"},{"id":"373718","messageId":"nycvar.QRO.7.76.6.1904121100030.41@tvgsbejvaqbjf.bet","threadId":"50818","inReplyTo":"xmqqtvfiuhn1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-12T09:06:11Z","receivedAt":"2019-04-12T09:06:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 1 Apr 2019, Junio C Hamano wrote:\n\n> Eric Sunshine writes:\n>\n> > I don't get why having a configuration option is better for defending\n> > scripts against this problem than a simple environment variable. It\n> > seems easier for the script prologue to contain:\n> >\n> > GIT_TEST_ABBREVIATED_OPTIONS=false\n> > export GIT_TEST_ABBREVIATED_OPTIONS\n> >\n> > than for it to muck about with git-config or use \"git -c\n> > core.abbreviatedOptions=false ...\" everywhere. The commit message\n> > doesn't do a good enough job of justifying the configuration option\n> > over the environment variable.\n>\n> Absolutely.\n>\n> One thing that big brotherly types would find config attractive is\n> to install centrally managed /etc/gitconfig so that they can tell\n> the tracing machinery to log all git command invocations centrally;\n> with environment only system, it is not easy to arrange.\n\nI think that in this instance, we should use the fact that we know Ævar\nwell, and refrain from characterizing him as a Big Brotherly type.\n\nFrom my reading, it looks like Ævar just wants to avoid being woken up for\na Live Site Incident that is caused by a violation of Postel's Law: be\naccepting in your input, but stringent in your output. And in this case,\nthe scripts by their colleagues is the output that should be more\nstringent, and enforcing the stringency via a system-wide config variable\nis as legit as our instistence that `user.name` and `user.email` must be\nprovided if you want to create a commit.\n\nAnd I would not have the faintest problem with adding that patch to\nintroduce the `core.*` setting to that end (which would be the right\nsection, too, even if there are already so many `core.*` settings).\n\nHaving said that, I get the strong impression that there is a rather\nviolent pushback against this (which I don't understand). Combined with\nthe fact that it would protect only against a tiny fraction of \"git\nupgrade problems\", I'm getting more into the \"well, then let's just not\nbother\" camp.\n\nCiao,\nDscho\n"},{"id":"373724","messageId":"pull.167.v2.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/8] Do not use abbreviated options in tests","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:17Z","receivedAt":"2019-04-12T09:37:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"We do not want to have tests that need to be changed by completely unrelated\npatch series, just because abbreviations that used to be unique are being\nmade non-unique by said patch series.\n\nI stumbled over this while investigating the test failures in\ndl/rebase-i-keep-base\n[https://dev.azure.com/gitgitgadget/git/_build/results?buildId=5482&view=ms.vss-test-web.build-test-results-tab]\n: the new --keep-base option makes the --keep abbreviation of --keep-empty \nin t5407 non-unique, which causes the test suite to fail.\n\nChanges since v1:\n\n * Fixed a typo (\"expaneded\") in the last commit message.\n * Added a regression test for the GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS \n handling.\n * Adjusted the code in test-lib.sh that makes sure that this variable is\n set: it is now consistent with the way we do similar things in that file\n already.\n\nJohannes Schindelin (8):\n tests (rebase): spell out the `--keep-empty` option\n tests (rebase): spell out the `--force-rebase` option\n t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`\n t5531: avoid using an abbreviated option\n tests (push): do not abbreviate the `--follow-tags` option\n tests (status): spell out the `--find-renames` option in full\n tests (pack-objects): use the full, unabbreviated `--revs` option\n tests: disallow the use of abbreviated options (by default)\n\n parse-options.c | 9 ++++++\n t/README | 4 +++\n t/t0040-parse-options.sh | 14 +++++++-\n t/t3415-rebase-autosquash.sh | 2 +-\n t/t3430-rebase-merges.sh | 4 +--\n t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------\n t/t5407-post-rewrite-hook.sh | 4 +--\n t/t5516-fetch-push.sh | 4 +--\n t/t5531-deep-submodule-push.sh | 2 +-\n t/t7525-status-rename.sh | 8 ++---\n t/t7810-grep.sh | 16 +++++-----\n t/test-lib.sh | 7 ++++\n 12 files changed, 75 insertions(+), 43 deletions(-)\n\n\nbase-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-167%2Fdscho%2Fdisallow-abbreviated-options-in-tests-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-167/dscho/disallow-abbreviated-options-in-tests-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/167\n\nRange-diff vs v1:\n\n 1: a1b4b74b91 = 1: a1b4b74b91 tests (rebase): spell out the `--keep-empty` option\n 2: 017ec9a0c7 = 2: 017ec9a0c7 tests (rebase): spell out the `--force-rebase` option\n 3: 7d8f6a2ee6 = 3: 7d8f6a2ee6 t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`\n 4: f45c435e4a = 4: f45c435e4a t5531: avoid using an abbreviated option\n 5: 7372059922 = 5: 7372059922 tests (push): do not abbreviate the `--follow-tags` option\n 6: 531450c00d = 6: 531450c00d tests (status): spell out the `--find-renames` option in full\n 7: a722a065d2 = 7: a722a065d2 tests (pack-objects): use the full, unabbreviated `--revs` option\n 8: 04c36b1de9 ! 8: a27d316855 tests: disallow the use of abbreviated options (by default)\n @@ -14,7 +14,7 @@\n \n For example, if a future contribution added a new mode\n `git init --babyproofing` and a previously-introduced test case used the\n - fact that `git init --ba` expaneded to `git init --bare`, that future\n + fact that `git init --ba` expanded to `git init --bare`, that future\n contribution would now have to touch seemingly unrelated tests just to\n keep the test suite from failing.\n \n @@ -115,6 +115,18 @@\n \ttest-tool parse-options --no-ambig >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n +@@\n + \ttest-tool parse-options --expect=\"verbose: 0\" -v -v -v --no-verbose\n + '\n + \n ++test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '\n ++\tenv GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n ++\t\ttest-tool parse-options --ye &&\n ++\ttest_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \\\n ++\t\ttest-tool parse-options --ye\n ++'\n ++\n + test_done\n \n diff --git a/t/test-lib.sh b/t/test-lib.sh\n --- a/t/test-lib.sh\n @@ -124,10 +136,11 @@\n export PERL_PATH SHELL_PATH\n \n +# Disallow the use of abbreviated options in the test suite by default\n -+test -n \"$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\" || {\n ++if test -n \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}\"\n ++then\n +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n +\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n -+}\n ++fi\n +\n ################################################################\n # It appears that people try to run tests without building...\n\n-- \ngitgitgadget\n"},{"id":"373725","messageId":"017ec9a0c74c770a42843eef65f49b7cc4397728.1555061837.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/8] tests (rebase): spell out the `--force-rebase` option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:20Z","receivedAt":"2019-04-12T09:37:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn quite a few test cases, we were sloppy and used the abbreviation\n`--force`, but we really should be precise in what we want to test.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t3415-rebase-autosquash.sh | 2 +-\n t/t3430-rebase-merges.sh | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh\nindex 13f5688135..22d218698e 100755\n--- a/t/t3415-rebase-autosquash.sh\n+++ b/t/t3415-rebase-autosquash.sh\n@@ -277,7 +277,7 @@ test_expect_success 'autosquash with empty custom instructionFormat' '\n \t(\n \t\tset_cat_todo_editor &&\n \t\ttest_must_fail git -c rebase.instructionFormat= \\\n-\t\t\trebase --autosquash --force -i HEAD^ >actual &&\n+\t\t\trebase --autosquash --force-rebase -i HEAD^ >actual &&\n \t\tgit log -1 --format=\"pick %h %s\" >expect &&\n \t\ttest_cmp expect actual\n \t)\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 4c69255ee6..42ba5b9f09 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -271,7 +271,7 @@ test_expect_success 'root commits' '\n \tEOF\n \ttest_config sequence.editor \\\"\"$PWD\"/replace-editor.sh\\\" &&\n \ttest_tick &&\n-\tgit rebase -i --force --root -r &&\n+\tgit rebase -i --force-rebase --root -r &&\n \ttest \"Parsnip\" = \"$(git show -s --format=%an HEAD^)\" &&\n \ttest $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&\n \ttest $(git rev-parse second-root:second-root.t) = \\\n@@ -364,7 +364,7 @@ test_expect_success 'octopus merges' '\n \ttest_cmp_rev HEAD $before &&\n \n \ttest_tick &&\n-\tgit rebase -i --force -r HEAD^^ &&\n+\tgit rebase -i --force-rebase -r HEAD^^ &&\n \ttest \"Hank\" = \"$(git show -s --format=%an HEAD)\" &&\n \ttest \"$before\" != $(git rev-parse HEAD) &&\n \ttest_cmp_graph HEAD^^.. <<-\\EOF\n-- \ngitgitgadget\n\n"},{"id":"373726","messageId":"f45c435e4acb765a68b83ab2c5127388c59dc0b1.1555061837.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 4/8] t5531: avoid using an abbreviated option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:21Z","receivedAt":"2019-04-12T09:37:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt was probably just an oversight: the `--recurse-submodules` option\nputs the term \"submodules\" in the plural form, not the singular one.\n\nTo avoid future problems in case that another option is introduced that\nstarts with the prefix `--recurse-submodule`, let's just fix this.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5531-deep-submodule-push.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh\nindex e2c37fd978..4ad059e6be 100755\n--- a/t/t5531-deep-submodule-push.sh\n+++ b/t/t5531-deep-submodule-push.sh\n@@ -363,7 +363,7 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs\n \t\t) &&\n \t\tgit add b &&\n \t\tgit commit -m \"added submodule\" &&\n-\t\tgit push --recurse-submodule=check origin master\n+\t\tgit push --recurse-submodules=check origin master\n \t)\n '\n \n-- \ngitgitgadget\n\n"},{"id":"373727","messageId":"531450c00dfe517748d9ad0bc77b9871c45042aa.1555061837.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 6/8] tests (status): spell out the `--find-renames` option in full","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:23Z","receivedAt":"2019-04-12T09:37:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTo avoid future ambiguities, we really want to use full option names in\nthe test suite. `t7525-status-rename.sh` used an abbreviated form of the\n`--find-renames` option, though, so let's change that.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7525-status-rename.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh\nindex ef8b1b3078..a62736dce0 100755\n--- a/t/t7525-status-rename.sh\n+++ b/t/t7525-status-rename.sh\n@@ -84,7 +84,7 @@ test_expect_success 'status score=100%' '\n \ttest_i18ngrep \"deleted:\" actual &&\n \ttest_i18ngrep \"new file:\" actual &&\n \n-\tgit status --find-rename=100% >actual &&\n+\tgit status --find-renames=100% >actual &&\n \ttest_i18ngrep \"deleted:\" actual &&\n \ttest_i18ngrep \"new file:\" actual\n '\n@@ -93,11 +93,11 @@ test_expect_success 'status score=01%' '\n \tgit status -M=01% >actual &&\n \ttest_i18ngrep \"renamed:\" actual &&\n \n-\tgit status --find-rename=01% >actual &&\n+\tgit status --find-renames=01% >actual &&\n \ttest_i18ngrep \"renamed:\" actual\n '\n \n-test_expect_success 'copies not overridden by find-rename' '\n+test_expect_success 'copies not overridden by find-renames' '\n \tcp renamed copy &&\n \tgit add copy &&\n \n@@ -105,7 +105,7 @@ test_expect_success 'copies not overridden by find-rename' '\n \ttest_i18ngrep \"copied:\" actual &&\n \ttest_i18ngrep \"renamed:\" actual &&\n \n-\tgit -c status.renames=copies status --find-rename=01% >actual &&\n+\tgit -c status.renames=copies status --find-renames=01% >actual &&\n \ttest_i18ngrep \"copied:\" actual &&\n \ttest_i18ngrep \"renamed:\" actual\n '\n-- \ngitgitgadget\n\n"},{"id":"373728","messageId":"7372059922710f392201cb32c5a7ac9cbdbe0616.1555061837.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 5/8] tests (push): do not abbreviate the `--follow-tags` option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:22Z","receivedAt":"2019-04-12T09:37:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWe really want to spell out the option in the full form, to avoid any\nambiguity that might be introduced by future patches.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5516-fetch-push.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 37e8e80893..db0b1db458 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -1370,7 +1370,7 @@ test_expect_success 'push does not follow tags by default' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'push --follow-tag only pushes relevant tags' '\n+test_expect_success 'push --follow-tags only pushes relevant tags' '\n \tmk_test testrepo heads/master &&\n \trm -fr src dst &&\n \tgit init src &&\n@@ -1384,7 +1384,7 @@ test_expect_success 'push --follow-tag only pushes relevant tags' '\n \t\tgit tag -m \"future\" future &&\n \t\tgit checkout master &&\n \t\tgit for-each-ref refs/heads/master refs/tags/tag >../expect &&\n-\t\tgit push --follow-tag ../dst master\n+\t\tgit push --follow-tags ../dst master\n \t) &&\n \t(\n \t\tcd dst &&\n-- \ngitgitgadget\n\n"},{"id":"373729","messageId":"a1b4b74b9167e279dae4cd8c58fb28d8a714a66a.1555061837.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/8] tests (rebase): spell out the `--keep-empty` option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:19Z","receivedAt":"2019-04-12T09:37:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis test wants to run `git rebase` with the `--keep-empty` option, but\nit really only spelled out `--keep` and trusted Git's option parsing to\ndetermine that this was a unique abbreviation of the real option.\n\nHowever, Denton Liu contributed a patch series in\nhttps://public-inbox.org/git/cover.1553354374.git.liu.denton@gmail.com/\nthat introduces a new `git rebase` option called `--keep-base`, which\nmakes this previously unique abbreviation non-unique.\n\nWhether this patch series is accepted or not, it is actually a bad\npractice to use abbreviated options in our test suite, because of the\nissue that those unique option names are not guaranteed to stay unique\nin the future.\n\nSo let's just not use abbreviated options in the test suite.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5407-post-rewrite-hook.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh\nindex a4a5903cba..7344253bfb 100755\n--- a/t/t5407-post-rewrite-hook.sh\n+++ b/t/t5407-post-rewrite-hook.sh\n@@ -131,7 +131,7 @@ test_expect_success 'git rebase -m --skip' '\n test_expect_success 'git rebase with implicit use of interactive backend' '\n \tgit reset --hard D &&\n \tclear_hook_input &&\n-\ttest_must_fail git rebase --keep --onto A B &&\n+\ttest_must_fail git rebase --keep-empty --onto A B &&\n \techo C > foo &&\n \tgit add foo &&\n \tgit rebase --continue &&\n@@ -146,7 +146,7 @@ test_expect_success 'git rebase with implicit use of interactive backend' '\n test_expect_success 'git rebase --skip with implicit use of interactive backend' '\n \tgit reset --hard D &&\n \tclear_hook_input &&\n-\ttest_must_fail git rebase --keep --onto A B &&\n+\ttest_must_fail git rebase --keep-empty --onto A B &&\n \ttest_must_fail git rebase --skip &&\n \techo D > foo &&\n \tgit add foo &&\n-- \ngitgitgadget\n\n"},{"id":"373730","messageId":"7d8f6a2ee6295b9817566853e1cfeca16b144b30.1555061837.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:20Z","receivedAt":"2019-04-12T09:37:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis script used abbreviated options, which is unnecessarily fragile.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t7810-grep.sh | 16 ++++++++--------\n 1 file changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 43aa4161cf..2e1bb61b41 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -119,33 +119,33 @@ do\n \t\ttest_cmp expected actual\n \t'\n \n-\ttest_expect_success \"grep -w $L (with --column, --invert)\" '\n+\ttest_expect_success \"grep -w $L (with --column, --invert-match)\" '\n \t\t{\n \t\t\techo ${HC}file:1:foo mmap bar\n \t\t\techo ${HC}file:1:foo_mmap bar\n \t\t\techo ${HC}file:1:foo_mmap bar mmap\n \t\t\techo ${HC}file:1:foo mmap bar_mmap\n \t\t} >expected &&\n-\t\tgit grep --column --invert -w -e baz $H -- file >actual &&\n+\t\tgit grep --column --invert-match -w -e baz $H -- file >actual &&\n \t\ttest_cmp expected actual\n \t'\n \n-\ttest_expect_success \"grep $L (with --column, --invert, extended OR)\" '\n+\ttest_expect_success \"grep $L (with --column, --invert-match, extended OR)\" '\n \t\t{\n \t\t\techo ${HC}hello_world:6:HeLLo_world\n \t\t} >expected &&\n-\t\tgit grep --column --invert -e ll --or --not -e _ $H -- hello_world \\\n+\t\tgit grep --column --invert-match -e ll --or --not -e _ $H -- hello_world \\\n \t\t\t>actual &&\n \t\ttest_cmp expected actual\n \t'\n \n-\ttest_expect_success \"grep $L (with --column, --invert, extended AND)\" '\n+\ttest_expect_success \"grep $L (with --column, --invert-match, extended AND)\" '\n \t\t{\n \t\t\techo ${HC}hello_world:3:Hello world\n \t\t\techo ${HC}hello_world:3:Hello_world\n \t\t\techo ${HC}hello_world:6:HeLLo_world\n \t\t} >expected &&\n-\t\tgit grep --column --invert --not -e _ --and --not -e ll $H -- hello_world \\\n+\t\tgit grep --column --invert-match --not -e _ --and --not -e ll $H -- hello_world \\\n \t\t\t>actual &&\n \t\ttest_cmp expected actual\n \t'\n@@ -1010,7 +1010,7 @@ test_expect_success 'outside of git repository' '\n \t\t\techo \".gitignore:.*o*\" &&\n \t\t\tcat ../expect.full\n \t\t} >../expect.with.ignored &&\n-\t\tgit grep --no-index --no-exclude o >../actual.full &&\n+\t\tgit grep --no-index --no-exclude-standard o >../actual.full &&\n \t\ttest_cmp ../expect.with.ignored ../actual.full\n \t)\n '\n@@ -1051,7 +1051,7 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '\n \t\t\techo \".gitignore:.*o*\" &&\n \t\t\tcat ../expect.full\n \t\t} >../expect.with.ignored &&\n-\t\tgit -c grep.fallbackToNoIndex grep --no-exclude o >../actual.full &&\n+\t\tgit -c grep.fallbackToNoIndex grep --no-exclude-standard o >../actual.full &&\n \t\ttest_cmp ../expect.with.ignored ../actual.full\n \t)\n '\n-- \ngitgitgadget\n\n"},{"id":"373731","messageId":"a27d316855a833aa1726fc20c905dc40e41adf2f.1555061837.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:24Z","receivedAt":"2019-04-12T09:37:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nGit's command-line parsers support uniquely abbreviated options, e.g.\n`git init --ba` would automatically expand `--ba` to `--bare`.\n\nThis is a very convenient feature in every day life for Git users, in\nparticular when tab completion is not available.\n\nHowever, it is not a good idea to rely on that in Git's test suite, as\nsomething that is a unique abbreviation of a command line option today\nmight no longer be a unique abbreviation tomorrow.\n\nFor example, if a future contribution added a new mode\n`git init --babyproofing` and a previously-introduced test case used the\nfact that `git init --ba` expanded to `git init --bare`, that future\ncontribution would now have to touch seemingly unrelated tests just to\nkeep the test suite from failing.\n\nSo let's disallow abbreviated options in the test suite by default.\n\nNote: for ease of implementation, this patch really only touches the\n`parse-options` machinery: more and more hand-rolled option parsers are\nconverted to use that internal API, and more and more scripts are\nconverted to built-ins (naturally using the parse-options API, too), so\nin practice this catches most issues, and is definitely the biggest bang\nfor the buck.\n\nSigned-off-by: Johannes Schindelin \n---\n parse-options.c | 9 +++++++++\n t/README | 4 ++++\n t/t0040-parse-options.sh | 14 +++++++++++++-\n t/test-lib.sh | 7 +++++++\n 4 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex cec74522e5..acc3a93660 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -6,6 +6,8 @@\n #include \"color.h\"\n #include \"utf8.h\"\n \n+static int disallow_abbreviated_options;\n+\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(\n \t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n \t}\n \n+\tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n+\t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n+\t\t (int)(arg_end - arg), arg);\n+\n \tif (ambiguous_option) {\n \t\terror(_(\"ambiguous option: %s \"\n \t\t\t\"(could be --%s%s or --%s%s)\"),\n@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,\n {\n \tstruct parse_opt_ctx_t ctx;\n \n+\tdisallow_abbreviated_options =\n+\t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n+\n \tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\ndiff --git a/t/README b/t/README\nindex 656288edce..9ed3051a1c 100644\n--- a/t/README\n+++ b/t/README\n@@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n fetch-pack to not request sideband-all (even if the server advertises\n sideband-all).\n \n+GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=, when true (which is\n+the default when running tests), errors out when an abbreviated option\n+is used.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex b8f366c442..6f6f74bfe2 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -203,20 +203,24 @@ file: (not set)\n EOF\n \n test_expect_success 'unambiguously abbreviated option' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'unambiguously abbreviated option with \"=\"' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --expect=\"integer: 2\" --int=2\n '\n \n test_expect_success 'ambiguously abbreviated option' '\n-\ttest_expect_code 129 test-tool parse-options --strin 123\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest-tool parse-options --strin 123\n '\n \n test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --expect=\"string: 123\" --st 123\n '\n \n@@ -325,6 +329,7 @@ file: (not set)\n EOF\n \n test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --no-ambig >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n@@ -370,4 +375,11 @@ test_expect_success '--no-verbose resets multiple verbose to 0' '\n \ttest-tool parse-options --expect=\"verbose: 0\" -v -v -v --no-verbose\n '\n \n+test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '\n+\tenv GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\t\ttest-tool parse-options --ye &&\n+\ttest_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \\\n+\t\ttest-tool parse-options --ye\n+'\n+\n test_done\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 562c57e685..f1a0fea4e1 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -57,6 +57,13 @@ fi\n . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n export PERL_PATH SHELL_PATH\n \n+# Disallow the use of abbreviated options in the test suite by default\n+if test -n \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}\"\n+then\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n+\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n+fi\n+\n ################################################################\n # It appears that people try to run tests without building...\n \"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X\" >/dev/null\n-- \ngitgitgadget\n"},{"id":"373732","messageId":"a722a065d2c3249bddd987cca9a37779a81bb378.1555061837.git.gitgitgadget@gmail.com","threadId":"50818","inReplyTo":"pull.167.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-04-12T09:37:23Z","receivedAt":"2019-04-12T09:37:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nTo use the singular form of a word, when the option wants the plural\nform (and quietly expands it because it thinks it was abbreviated), is\nan easy mistake to make, and t5317 contains almost two dozen of them.\n\nHowever, using abbreviated options in tests is a bit fragile, so we will\ndisallow use of abbreviated options in our test suite.\n\nIn preparation for this change, let's fix\n`t5317-pack-objects-filter-objects.sh`.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------\n 1 file changed, 22 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh\nindex 24541ea137..4c0201c34b 100755\n--- a/t/t5317-pack-objects-filter-objects.sh\n+++ b/t/t5317-pack-objects-filter-objects.sh\n@@ -25,7 +25,7 @@ test_expect_success 'verify blob count in normal packfile' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&\n+\tgit -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r1 index-pack ../all.pack &&\n@@ -39,7 +39,7 @@ test_expect_success 'verify blob count in normal packfile' '\n '\n \n test_expect_success 'verify blob:none packfile has no blobs' '\n-\tgit -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF &&\n+\tgit -C r1 pack-objects --revs --stdout --filter=blob:none >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r1 index-pack ../filter.pack &&\n@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' '\n \tgit -C r5 commit -m \"foo\" &&\n \tdel=$(git -C r5 rev-parse HEAD^{tree} | sed \"s|..|&/|\") &&\n \trm r5/.git/objects/$del &&\n-\ttest_must_fail git -C r5 pack-objects --rev --stdout 2>bad_tree <<-EOF &&\n+\ttest_must_fail git -C r5 pack-objects --revs --stdout 2>bad_tree <<-EOF &&\n \tHEAD\n \tEOF\n \tgrep \"bad tree object\" bad_tree\n@@ -88,7 +88,7 @@ test_expect_success 'setup for tests of tree:0' '\n '\n \n test_expect_success 'verify tree:0 packfile has no blobs or trees' '\n-\tgit -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&\n+\tgit -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r1 index-pack ../commitsonly.pack &&\n@@ -98,7 +98,7 @@ test_expect_success 'verify tree:0 packfile has no blobs or trees' '\n \n test_expect_success 'grab tree directly when using tree:0' '\n \t# We should get the tree specified directly but not its blobs or subtrees.\n-\tgit -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&\n+\tgit -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&\n \tHEAD:\n \tEOF\n \tgit -C r1 index-pack ../commitsonly.pack &&\n@@ -128,7 +128,7 @@ test_expect_success 'verify blob count in normal packfile' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout >all.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../all.pack &&\n@@ -142,7 +142,7 @@ test_expect_success 'verify blob count in normal packfile' '\n '\n \n test_expect_success 'verify blob:limit=500 omits all blobs' '\n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -157,7 +157,7 @@ test_expect_success 'verify blob:limit=500 omits all blobs' '\n '\n \n test_expect_success 'verify blob:limit=1000' '\n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -176,7 +176,7 @@ test_expect_success 'verify blob:limit=1001' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -194,7 +194,7 @@ test_expect_success 'verify blob:limit=10001' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -212,7 +212,7 @@ test_expect_success 'verify blob:limit=1k' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -230,7 +230,7 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&\n \tHEAD\n \t$(git -C r2 rev-parse HEAD:large.10000)\n \tEOF\n@@ -249,7 +249,7 @@ test_expect_success 'verify blob:limit=1m' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&\n+\tgit -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r2 index-pack ../filter.pack &&\n@@ -302,7 +302,7 @@ test_expect_success 'verify blob count in normal packfile' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r3 pack-objects --rev --stdout >all.pack <<-EOF &&\n+\tgit -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r3 index-pack ../all.pack &&\n@@ -320,7 +320,7 @@ test_expect_success 'verify sparse:path=pattern1' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&\n+\tgit -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r3 index-pack ../filter.pack &&\n@@ -352,7 +352,7 @@ test_expect_success 'verify sparse:path=pattern2' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&\n+\tgit -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r3 index-pack ../filter.pack &&\n@@ -404,7 +404,7 @@ test_expect_success 'verify blob count in normal packfile' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r4 pack-objects --rev --stdout >all.pack <<-EOF &&\n+\tgit -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r4 index-pack ../all.pack &&\n@@ -423,7 +423,7 @@ test_expect_success 'verify sparse:oid=OID' '\n \tsort >expected &&\n \n \toid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) &&\n-\tgit -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&\n+\tgit -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r4 index-pack ../filter.pack &&\n@@ -441,7 +441,7 @@ test_expect_success 'verify sparse:oid=oid-ish' '\n \tawk -f print_2.awk ls_files_result |\n \tsort >expected &&\n \n-\tgit -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&\n+\tgit -C r4 pack-objects --revs --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&\n \tHEAD\n \tEOF\n \tgit -C r4 index-pack ../filter.pack &&\n@@ -470,19 +470,19 @@ test_expect_success 'setup r1 - delete loose blobs' '\n '\n \n test_expect_success 'verify pack-objects fails w/ missing objects' '\n-\ttest_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF\n+\ttest_must_fail git -C r1 pack-objects --revs --stdout >miss.pack <<-EOF\n \tHEAD\n \tEOF\n '\n \n test_expect_success 'verify pack-objects fails w/ --missing=error' '\n-\ttest_must_fail git -C r1 pack-objects --rev --stdout --missing=error >miss.pack <<-EOF\n+\ttest_must_fail git -C r1 pack-objects --revs --stdout --missing=error >miss.pack <<-EOF\n \tHEAD\n \tEOF\n '\n \n test_expect_success 'verify pack-objects w/ --missing=allow-any' '\n-\tgit -C r1 pack-objects --rev --stdout --missing=allow-any >miss.pack <<-EOF\n+\tgit -C r1 pack-objects --revs --stdout --missing=allow-any >miss.pack <<-EOF\n \tHEAD\n \tEOF\n '\n-- \ngitgitgadget\n\n"},{"id":"373808","messageId":"xmqqv9zhwbxt.fsf@gitster-ct.c.googlers.com","threadId":"50818","inReplyTo":"a27d316855a833aa1726fc20c905dc40e41adf2f.1555061837.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-14T02:35:58Z","receivedAt":"2019-04-14T02:36:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> +\tdisallow_abbreviated_options =\n> +\t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n\n... which means that the value of the environment variable follows\nthe usual \"true, yes and 1 all activate it\"; very good,\n\n> diff --git a/t/README b/t/README\n> index 656288edce..9ed3051a1c 100644\n> --- a/t/README\n> +++ b/t/README\n> @@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n> fetch-pack to not request sideband-all (even if the server advertises\n> sideband-all).\n> \n> +GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=, when true (which is\n> +the default when running tests), errors out when an abbreviated option\n> +is used.\n\nOK.\n\n> +test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '\n> +\tenv GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n> +\t\ttest-tool parse-options --ye &&\n> +\ttest_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \\\n> +\t\ttest-tool parse-options --ye\n> +'\n\nThe feature is activated for all tests unless otherwise noted, and\nthe above marked the places that need to be \"otherwise noted\" in a\nreasonable way. Good.\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 562c57e685..f1a0fea4e1 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -57,6 +57,13 @@ fi\n> . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n> export PERL_PATH SHELL_PATH\n> \n> +# Disallow the use of abbreviated options in the test suite by default\n> +if test -n \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}\"\n> +then\n> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n> +\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n> +fi\n\nIf the original environment has it as flase, as it is set, the\nsubstitution will yield \"isset\" which is not an empty string, so we\nassign true.\n\nIf the original environment is not set, or set to an empty, however,\nthe substitution will yield an empty string, so we won't touch the\nvariable.\n\nI am not sure in what situation the above behaviour becomes useful.\n\nDo you mean more like\n\n\tif test -z \"$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\"\n\tthen\n\t\t... assignment ...\n\tfi\n\nIOW, we'll take an explicit GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false\nas a sign that the developer for whatever readon wants the disambiguation\ncode in parse-options to kick in for all uses and allow a shortened option\nnames?\n\nIf on the other hand you are protecting our tests against those\nwho casually have the environment set to false, because they know\nsome of the scripts they use are sloppy *and* for whatever reason\nthey anticipate that someday we will start to disallow abbrevated\noptions by default? If so, an unconditional assignment of true\nwould be more appropriate.\n\nI think I can agree with either of the two positions (i.e. we let\nthose who explicitly want to decline do so, or we unconditionally\nmake sure we catch issues in our tests), and I do not think of a\nthird position that are different from these two and that would make\nsense. Between the two, I'd probably vote for the latter if I was\npressed, but even then that is not a very strong preference.\n\nThanks. I very much like the premise of this series, and the above\nhunk stood out in the range-diff in 0/8.\n\n\n"},{"id":"373839","messageId":"xmqqy34cugdj.fsf@gitster-ct.c.googlers.com","threadId":"50818","inReplyTo":"xmqqv9zhwbxt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-15T02:55:20Z","receivedAt":"2019-04-15T02:55:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n\n> Do you mean more like\n> ...\n> I think I can agree with either of the two positions...\n\nI am guessing from the earlier iteration that you wanted to say\n\"unless it is given explicitly, we turn it on\".\n\nAs this last-minute style update that was botched, and a typofix in\nthe proposed log message in 8/8, are the only differences, let me\nlocally fix 8/8 up and replace it.\n\nIt was very good that this round did not have to touch 1/8 which is\nshared with Denton's keep-base topic; otherwise replacing would have\nbeen a little bit messier.\n\nHere is what I'll use (unless it gets further replaced by v3 or\nlater, that is).\n\nThanks.\n\n-- >8 --\nFrom: Johannes Schindelin \nDate: Fri, 12 Apr 2019 02:37:24 -0700\nSubject: [PATCH v2bis 8/8] tests: disallow the use of abbreviated options (by default)\n\nGit's command-line parsers support uniquely abbreviated options, e.g.\n`git init --ba` would automatically expand `--ba` to `--bare`.\n\nThis is a very convenient feature in every day life for Git users, in\nparticular when tab completion is not available.\n\nHowever, it is not a good idea to rely on that in Git's test suite, as\nsomething that is a unique abbreviation of a command line option today\nmight no longer be a unique abbreviation tomorrow.\n\nFor example, if a future contribution added a new mode\n`git init --babyproofing` and a previously-introduced test case used the\nfact that `git init --ba` expanded to `git init --bare`, that future\ncontribution would now have to touch seemingly unrelated tests just to\nkeep the test suite from failing.\n\nSo let's disallow abbreviated options in the test suite by default.\n\nNote: for ease of implementation, this patch really only touches the\n`parse-options` machinery: more and more hand-rolled option parsers are\nconverted to use that internal API, and more and more scripts are\nconverted to built-ins (naturally using the parse-options API, too), so\nin practice this catches most issues, and is definitely the biggest bang\nfor the buck.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n parse-options.c | 9 +++++++++\n t/README | 4 ++++\n t/t0040-parse-options.sh | 14 +++++++++++++-\n t/test-lib.sh | 7 +++++++\n 4 files changed, 33 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex cec74522e5..acc3a93660 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -6,6 +6,8 @@\n #include \"color.h\"\n #include \"utf8.h\"\n \n+static int disallow_abbreviated_options;\n+\n #define OPT_SHORT 1\n #define OPT_UNSET 2\n \n@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(\n \t\treturn get_value(p, options, all_opts, flags ^ opt_flags);\n \t}\n \n+\tif (disallow_abbreviated_options && (ambiguous_option || abbrev_option))\n+\t\tdie(\"disallowed abbreviated or ambiguous option '%.*s'\",\n+\t\t (int)(arg_end - arg), arg);\n+\n \tif (ambiguous_option) {\n \t\terror(_(\"ambiguous option: %s \"\n \t\t\t\"(could be --%s%s or --%s%s)\"),\n@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,\n {\n \tstruct parse_opt_ctx_t ctx;\n \n+\tdisallow_abbreviated_options =\n+\t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n+\n \tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\ndiff --git a/t/README b/t/README\nindex 656288edce..9ed3051a1c 100644\n--- a/t/README\n+++ b/t/README\n@@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=, when true, overrides the\n fetch-pack to not request sideband-all (even if the server advertises\n sideband-all).\n \n+GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=, when true (which is\n+the default when running tests), errors out when an abbreviated option\n+is used.\n+\n Naming Tests\n ------------\n \ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex b8f366c442..800b3ea5f5 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -203,20 +203,24 @@ file: (not set)\n EOF\n \n test_expect_success 'unambiguously abbreviated option' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n '\n \n test_expect_success 'unambiguously abbreviated option with \"=\"' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --expect=\"integer: 2\" --int=2\n '\n \n test_expect_success 'ambiguously abbreviated option' '\n-\ttest_expect_code 129 test-tool parse-options --strin 123\n+\ttest_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest-tool parse-options --strin 123\n '\n \n test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --expect=\"string: 123\" --st 123\n '\n \n@@ -325,6 +329,7 @@ file: (not set)\n EOF\n \n test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n \ttest-tool parse-options --no-ambig >output 2>output.err &&\n \ttest_must_be_empty output.err &&\n \ttest_cmp expect output\n@@ -370,4 +375,11 @@ test_expect_success '--no-verbose resets multiple verbose to 0' '\n \ttest-tool parse-options --expect=\"verbose: 0\" -v -v -v --no-verbose\n '\n \n+test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\t\ttest-tool parse-options --ye &&\n+\ttest_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \\\n+\t\ttest-tool parse-options --ye\n+'\n+\n test_done\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 562c57e685..c14ebe68d3 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -57,6 +57,13 @@ fi\n . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n export PERL_PATH SHELL_PATH\n \n+# Disallow the use of abbreviated options in the test suite by default\n+if test -z \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}\"\n+then\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n+\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n+fi\n+\n ################################################################\n # It appears that people try to run tests without building...\n \"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X\" >/dev/null\n-- \n2.21.0-313-ge35b8cb8e2\n\n\n"},{"id":"373864","messageId":"nycvar.QRO.7.76.6.1904151503390.44@tvgsbejvaqbjf.bet","threadId":"50818","inReplyTo":"xmqqv9zhwbxt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-15T13:08:05Z","receivedAt":"2019-04-15T13:08:12Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 14 Apr 2019, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 562c57e685..f1a0fea4e1 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -57,6 +57,13 @@ fi\n> > . \"$GIT_BUILD_DIR\"/GIT-BUILD-OPTIONS\n> > export PERL_PATH SHELL_PATH\n> >\n> > +# Disallow the use of abbreviated options in the test suite by default\n> > +if test -n \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}\"\n> > +then\n> > +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n> > +\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n> > +fi\n>\n> If the original environment has it as flase, as it is set, the\n> substitution will yield \"isset\" which is not an empty string, so we\n> assign true.\n>\n> If the original environment is not set, or set to an empty, however,\n> the substitution will yield an empty string, so we won't touch the\n> variable.\n>\n> I am not sure in what situation the above behaviour becomes useful.\n>\n> Do you mean more like\n>\n> \tif test -z \"$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\"\n> \tthen\n> \t\t... assignment ...\n> \tfi\n\nÆvar rightfully pointed out that I introduced a new paradigm, so I\nswitched to imitating the exact way `test-lib.sh` handles similar cases.\nWhich is the `isset` method.\n\nSure, I could do something differently, like `test -z`. But why? I think\nit is better to stay consistent with the existing checks.\n\n> IOW, we'll take an explicit GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false\n> as a sign that the developer for whatever readon wants the disambiguation\n> code in parse-options to kick in for all uses and allow a shortened option\n> names?\n\nYes.\n\nLet's assume that there is a developer, or even a team (think e.g. VFS for\nGit), working on some long-running features that should be eventually\ncontributed to Git, but that are still in flux, and through a merge with\nthe current Git version, they get this change, and it breaks 50 of their\ntest scripts.\n\nThis is the scenario I wanted to help, however unlikely that is ;-)\n\nOf course, eventually they'd want to fix their test scripts. But maybe\nright now is not such a great time, so let's let them override the\nnew behavior.\n\n> If on the other hand you are protecting our tests against those\n> who casually have the environment set to false, because they know\n> some of the scripts they use are sloppy *and* for whatever reason\n> they anticipate that someday we will start to disallow abbrevated\n> options by default? If so, an unconditional assignment of true\n> would be more appropriate.\n>\n> I think I can agree with either of the two positions (i.e. we let\n> those who explicitly want to decline do so, or we unconditionally\n> make sure we catch issues in our tests), and I do not think of a\n> third position that are different from these two and that would make\n> sense. Between the two, I'd probably vote for the latter if I was\n> pressed, but even then that is not a very strong preference.\n>\n> Thanks. I very much like the premise of this series, and the above\n> hunk stood out in the range-diff in 0/8.\n\nThank you!\nDscho\n"},{"id":"373865","messageId":"nycvar.QRO.7.76.6.1904151509020.44@tvgsbejvaqbjf.bet","threadId":"50818","inReplyTo":"xmqqy34cugdj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-15T13:09:52Z","receivedAt":"2019-04-15T13:09:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 15 Apr 2019, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n>\n>\n> > Do you mean more like\n> > ...\n> > I think I can agree with either of the two positions...\n>\n> I am guessing from the earlier iteration that you wanted to say\n> \"unless it is given explicitly, we turn it on\".\n>\n> As this last-minute style update that was botched, and a typofix in\n> the proposed log message in 8/8, are the only differences, let me\n> locally fix 8/8 up and replace it.\n\nSure. I still would like the `isset` thing, as it makes things more\nconsistent, but I'll not fight for it.\n\nCiao,\nDscho\n"},{"id":"373871","messageId":"xmqqef63qt5e.fsf@gitster-ct.c.googlers.com","threadId":"50818","inReplyTo":"nycvar.QRO.7.76.6.1904151509020.44@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-15T13:45:17Z","receivedAt":"2019-04-15T13:45:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Mon, 15 Apr 2019, Junio C Hamano wrote:\n>\n>> Junio C Hamano writes:\n>>\n>>\n>> > Do you mean more like\n>> > ...\n>> > I think I can agree with either of the two positions...\n>>\n>> I am guessing from the earlier iteration that you wanted to say\n>> \"unless it is given explicitly, we turn it on\".\n>>\n>> As this last-minute style update that was botched, and a typofix in\n>> the proposed log message in 8/8, are the only differences, let me\n>> locally fix 8/8 up and replace it.\n>\n> Sure. I still would like the `isset` thing, as it makes things more\n> consistent, but I'll not fight for it.\n\n${var:+isset} is fine. Instead of\n\n+# Disallow the use of abbreviated options in the test suite by default\n+if test -z \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}\"\n+then\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n+\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n+fi\n+\n\nif you used\n\n\tif test -z \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}\"\n\tthen\n\t\t...\n\nI won't object. After all, I know I introduced :+isset pattern to\nour shell script codebase as there are cases where it makes the\nresult easier to follow.\n\nBut the thing is that your patch had the polarity inverted. Where\nyou must say \"if this thing is not set, assign this value\", you said\n\"if this thing is set, assign this value\", which was totally bogus.\nAs long as that is corrected, that's fine.\n\nHaving said that.\n\nWhen you check if the variable is set, use of the \":+isset\" pattern\nmakes the result often easier to follow by explicitly letting us\ncompare with an explicit \"isset\" token, e.g.\n\n\tcase \",${VAR1:+isset},${VAR2:+isset},\" in\n\t*,isset,*)\t: at least one is set ;;\n\t*)\t\t: neither is set ;;\n\tesac\n\nThis *does* make the code simpler and easier. But when checking for\n\"is it not set?\", you can compare with an explicit literal \"\" and\nthat comparison is plenty clear enough. You won't get as much\nbenefit as the \"is it set?\" test would out of the pattern. I would\nnot say that it is pointless to use the \":+isset\" pattern when\nchecking \"is it not set?\", but it is very close.\n\n"},{"id":"374031","messageId":"nycvar.QRO.7.76.6.1904171354270.46@tvgsbejvaqbjf.bet","threadId":"50818","inReplyTo":"xmqqef63qt5e.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-17T12:07:25Z","receivedAt":"2019-04-17T12:07:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 15 Apr 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > On Mon, 15 Apr 2019, Junio C Hamano wrote:\n> >\n> >> Junio C Hamano writes:\n> >>\n> >>\n> >> > Do you mean more like\n> >> > ...\n> >> > I think I can agree with either of the two positions...\n> >>\n> >> I am guessing from the earlier iteration that you wanted to say\n> >> \"unless it is given explicitly, we turn it on\".\n> >>\n> >> As this last-minute style update that was botched, and a typofix in\n> >> the proposed log message in 8/8, are the only differences, let me\n> >> locally fix 8/8 up and replace it.\n> >\n> > Sure. I still would like the `isset` thing, as it makes things more\n> > consistent, but I'll not fight for it.\n>\n> ${var:+isset} is fine. Instead of\n>\n> +# Disallow the use of abbreviated options in the test suite by default\n> +if test -z \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}\"\n> +then\n> +\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true\n> +\texport GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\n> +fi\n> +\n>\n> if you used\n>\n> \tif test -z \"${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}\"\n> \tthen\n> \t\t...\n>\n> I won't object. After all, I know I introduced :+isset pattern to\n> our shell script codebase as there are cases where it makes the\n> result easier to follow.\n>\n> But the thing is that your patch had the polarity inverted.\n\nAh! I finally understand what you are saying. Took me a while, sorry.\n\n> Where you must say \"if this thing is not set, assign this value\", you\n> said \"if this thing is set, assign this value\", which was totally bogus.\n> As long as that is corrected, that's fine.\n\nOf course! It should be `-z` instead of `-n` there.\n\n> Having said that.\n>\n> When you check if the variable is set, use of the \":+isset\" pattern\n> makes the result often easier to follow by explicitly letting us\n> compare with an explicit \"isset\" token, e.g.\n>\n> \tcase \",${VAR1:+isset},${VAR2:+isset},\" in\n> \t*,isset,*)\t: at least one is set ;;\n> \t*)\t\t: neither is set ;;\n> \tesac\n>\n> This *does* make the code simpler and easier. But when checking for\n> \"is it not set?\", you can compare with an explicit literal \"\" and\n> that comparison is plenty clear enough. You won't get as much\n> benefit as the \"is it set?\" test would out of the pattern. I would\n> not say that it is pointless to use the \":+isset\" pattern when\n> checking \"is it not set?\", but it is very close.\n\nWell, there is also the subtelty that somebody might *want* to set that\nenvironment variable to the empty string (and obviously they would not\ndeem it appropriate for us to override their choice).\n\nBut I do have bigger fish to fry, so if you're fine with the `isset` thing\n(fixed, of course), please go for it. If you'd prefer the version without\nit, that's fine enough with me, too.\n\nCiao,\nDscho\n"},{"id":"374033","messageId":"20190417124438.8191-1-avarab@gmail.com","threadId":"50818","inReplyTo":"20190325202329.26033-3-avarab@gmail.com","subject":"[PATCH v2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-04-17T12:44:38Z","receivedAt":"2019-04-17T12:44:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the option parsing machinery so that e.g. \"clone --recurs ...\"\ndoesn't error out because \"clone\" understands both \"--recursive\" and\n\"--recurse-submodules\" to mean the same thing.\n\nInitially \"clone\" just understood --recursive until the\n--recurses-submodules alias was added in ccdd3da652 (\"clone: Add the\n--recurse-submodules option as alias for --recursive\",\n2010-11-04). Since bb62e0a99f (\"clone: teach --recurse-submodules to\noptionally take a pathspec\", 2017-03-17) the longer form has been\npromoted to the default.\n\nBut due to the way the options parsing machinery works this resulted\nin the rather absurd situation of:\n\n $ git clone --recurs [...]\n error: ambiguous option: recurs (could be --recursive or --recurse-submodules)\n\nLet's re-use the PARSE_OPT_NOCOMPLETE flag to mean \"this option\ndoesn't contribute to abbreviation ambiguity\". I was going to add a\nnew PARSE_OPT_NOABBREV flag, but it makes sense just to re-use\nPARSE_OPT_NOCOMPLETE.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n\nSee\nhttps://public-inbox.org/git/20190325202329.26033-1-avarab@gmail.com/\nfor v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2\nseems like a no-brainer bugfix.\n\nIt conflicted with some recently-landed stuff in 'master', but now\ncleanly applies to it and 'pu', and with pu's\nGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS.\n\n builtin/clone.c | 4 ++--\n parse-options.c | 3 ++-\n parse-options.h | 2 ++\n t/t0040-parse-options.sh | 5 +++++\n 4 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..4dc26969a7 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"setup as shared repository\")),\n \t{ OPTION_CALLBACK, 0, \"recursive\", &option_recurse_submodules,\n \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n-\t PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,\n-\t (intptr_t)\".\" },\n+\t PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,\n+\t recurse_submodules_cb, (intptr_t)\".\" },\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n \t PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\ndiff --git a/parse-options.c b/parse-options.c\nindex cec74522e5..9899ce0171 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt(\n \t\tif (!rest) {\n \t\t\t/* abbreviated? */\n \t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n-\t\t\t !strncmp(long_name, arg, arg_end - arg)) {\n+\t\t\t !strncmp(long_name, arg, arg_end - arg) &&\n+\t\t\t !(options->flags & PARSE_OPT_NOCOMPLETE)) {\n is_abbreviated:\n \t\t\t\tif (abbrev_option) {\n \t\t\t\t\t/*\ndiff --git a/parse-options.h b/parse-options.h\nindex 74cce4e7fc..51c4b71ab0 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n *\t\t\t\tUseful for options with multiple parameters.\n * PARSE_OPT_NOCOMPLETE: by default all visible options are completable\n *\t\t\t by git-completion.bash. This option suppresses that.\n+ *\t\t\t Will also skip this option when abbreviation is\n+ *\t\t\t considered.\n * PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to\n *\t\t\t complete an option as --name= not --name even if\n *\t\t\t the option takes optional argument.\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex b8f366c442..e8f0371830 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n \ttest-tool parse-options --expect=\"string: 123\" --st 123\n '\n \n+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '\n+\ttest_when_finished \"rm -rf A\" &&\n+\tgit clone --recurs . A\n+'\n+\n cat >typo.err <<\\EOF\n error: did you mean `--boolean` (with two dashes ?)\n EOF\n-- \n2.21.0.593.g511ec345e18\n\n"},{"id":"374054","messageId":"CACsJy8D215hMvfCwz1G9mP2te-ZERVaMMRrnM=MK1_bc0oFsjw@mail.gmail.com","threadId":"50818","inReplyTo":"20190417124438.8191-1-avarab@gmail.com","subject":"Re: [PATCH v2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-17T16:04:59Z","receivedAt":"2019-04-17T16:05:28Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Apr 17, 2019 at 7:44 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> Change the option parsing machinery so that e.g. \"clone --recurs ...\"\n> doesn't error out because \"clone\" understands both \"--recursive\" and\n> \"--recurse-submodules\" to mean the same thing.\n>\n> Initially \"clone\" just understood --recursive until the\n> --recurses-submodules alias was added in ccdd3da652 (\"clone: Add the\n> --recurse-submodules option as alias for --recursive\",\n> 2010-11-04). Since bb62e0a99f (\"clone: teach --recurse-submodules to\n> optionally take a pathspec\", 2017-03-17) the longer form has been\n> promoted to the default.\n>\n> But due to the way the options parsing machinery works this resulted\n> in the rather absurd situation of:\n>\n> $ git clone --recurs [...]\n> error: ambiguous option: recurs (could be --recursive or --recurse-submodules)\n>\n> Let's re-use the PARSE_OPT_NOCOMPLETE flag to mean \"this option\n> doesn't contribute to abbreviation ambiguity\". I was going to add a\n> new PARSE_OPT_NOABBREV flag, but it makes sense just to re-use\n> PARSE_OPT_NOCOMPLETE.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n>\n> See\n> https://public-inbox.org/git/20190325202329.26033-1-avarab@gmail.com/\n> for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2\n> seems like a no-brainer bugfix.\n>\n> It conflicted with some recently-landed stuff in 'master', but now\n> cleanly applies to it and 'pu', and with pu's\n> GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS.\n>\n> builtin/clone.c | 4 ++--\n> parse-options.c | 3 ++-\n> parse-options.h | 2 ++\n> t/t0040-parse-options.sh | 5 +++++\n> 4 files changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 50bde99618..4dc26969a7 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {\n> N_(\"setup as shared repository\")),\n> { OPTION_CALLBACK, 0, \"recursive\", &option_recurse_submodules,\n> N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n> - PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,\n> - (intptr_t)\".\" },\n> + PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,\n\nWhat happens if someone adds --recursive-hard? --recursi then\nresolving to --recursive-hard sounds wrong.\n\nBut on the other hand I can see it's a bit more work to teach\nparse-options OPT_ALIAS to say \"--recursive is just an alias of\n--recurse-submodules\" and chances of --recursive-hard coming up are\nprobably very low.\n\nSo I don't know but I thought I should point out.\n\n> + recurse_submodules_cb, (intptr_t)\".\" },\n> { OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n> N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n> PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\n> diff --git a/parse-options.c b/parse-options.c\n> index cec74522e5..9899ce0171 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt(\n> if (!rest) {\n> /* abbreviated? */\n> if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n> - !strncmp(long_name, arg, arg_end - arg)) {\n> + !strncmp(long_name, arg, arg_end - arg) &&\n> + !(options->flags & PARSE_OPT_NOCOMPLETE)) {\n> is_abbreviated:\n> if (abbrev_option) {\n> /*\n> diff --git a/parse-options.h b/parse-options.h\n> index 74cce4e7fc..51c4b71ab0 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n> * Useful for options with multiple parameters.\n> * PARSE_OPT_NOCOMPLETE: by default all visible options are completable\n> * by git-completion.bash. This option suppresses that.\n> + * Will also skip this option when abbreviation is\n> + * considered.\n> * PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to\n> * complete an option as --name= not --name even if\n> * the option takes optional argument.\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index b8f366c442..e8f0371830 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n> test-tool parse-options --expect=\"string: 123\" --st 123\n> '\n>\n> +test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '\n> + test_when_finished \"rm -rf A\" &&\n> + git clone --recurs . A\n> +'\n> +\n> cat >typo.err <<\\EOF\n> error: did you mean `--boolean` (with two dashes ?)\n> EOF\n> --\n> 2.21.0.593.g511ec345e18\n>\n\n\n-- \nDuy\n"},{"id":"374079","messageId":"xmqq4l6wjfyw.fsf@gitster-ct.c.googlers.com","threadId":"50818","inReplyTo":"CACsJy8D215hMvfCwz1G9mP2te-ZERVaMMRrnM=MK1_bc0oFsjw@mail.gmail.com","subject":"Re: [PATCH v2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-18T00:48:39Z","receivedAt":"2019-04-18T00:49:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> { OPTION_CALLBACK, 0, \"recursive\", &option_recurse_submodules,\n>> N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n>> - PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,\n>> - (intptr_t)\".\" },\n>> + PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,\n>\n> What happens if someone adds --recursive-hard? --recursi then\n> resolving to --recursive-hard sounds wrong.\n\nThat was exactly my initial reaction. Or \"recurse-submodules\" gets\nrenamed away, and \"recommend\" gets added---now \"--rec\" is still not\nambiguous as \"recursive\" is marked not to participate in the\ndisambiguation (I think OPT_NOCOMPLETE should at least be renamed to\nOPT_NO_DISAMBIGUATION or something---if we were to use it for the\npurpose of marking an option as \"not participating in\ndisambiguation\", but I am fairly negative on the approach).\n\nAnd my initial reaction was followed by \"don't we want a more\nexplicit link only between recursive and recurse-submodules?\",\nwhich exactly matches what you said below ;-)\n\n> But on the other hand I can see it's a bit more work to teach\n> parse-options OPT_ALIAS to say \"--recursive is just an alias of\n> --recurse-submodules\" and chances of --recursive-hard coming up are\n> probably very low.\n\nThe \"bit more work\" is something that is worth doing in this case, I\nthink.\n\nThanks.\n"},{"id":"374094","messageId":"20190418092940.GA13484@ash","threadId":"50818","inReplyTo":"xmqq4l6wjfyw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-18T09:29:41Z","receivedAt":"2019-04-18T09:29:50Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Apr 18, 2019 at 09:48:39AM +0900, Junio C Hamano wrote:\n> > But on the other hand I can see it's a bit more work to teach\n> > parse-options OPT_ALIAS to say \"--recursive is just an alias of\n> > --recurse-submodules\" and chances of --recursive-hard coming up are\n> > probably very low.\n> \n> The \"bit more work\" is something that is worth doing in this case, I\n> think.\n\nI had a look at it. Linking two options together is not exactly easy\nbecause the way we organize option data. And I also had difficulty\ndefining the exact semantics of this OPT_ALIAS.\n\nSo an alternative is simply outsource the ambiguity decision back to\ngit-clone. If the same situation appears again elsewhere, we'll need\nto sit back and fix it for real. But this way we don't potentially\nintroduce any new traps.\n\nparse_options_extended() also makes it easier to extend parseopt\nfunctionality without changing all call sites. One thing I have in\nmind, which could maybe use this interface, is to remove\nparse_options_{start,step,end} from public API, avoid expose the\nparse_options_step internal result code. The caller just need to pass\na callback to handle unknown options.\n\n-- 8< --\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..041cd43ddc 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -145,6 +145,29 @@ static struct option builtin_clone_options[] = {\n \tOPT_END()\n };\n \n+/*\n+ * Avoid ambiguation error between --recursive and --recurse-submodule\n+ * because they are the same. --recurs can be expanded to any of them\n+ * and it still works.\n+ */\n+static int is_abbrev_ambiguous(const struct option *prev,\n+\t\t\t const struct option *next)\n+{\n+\tconst struct option *opts[] = { prev, next };\n+\tint i, found = 0;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(opts); i++) {\n+\t\tif (!opts[i]->long_name)\n+\t\t\tcontinue;\n+\t\tif (!strcmp(opts[i]->long_name, \"recursive\"))\n+\t\t\tfound |= 1 << 0;\n+\t\tif (!strcmp(opts[i]->long_name, \"recurse-submodules\"))\n+\t\t\tfound |= 1 << 1;\n+\t}\n+\n+\treturn found != 3;\n+}\n+\n static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)\n {\n \tstatic char *suffix[] = { \"/.git\", \"\", \".git/.git\", \".git\" };\n@@ -905,14 +928,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tstruct remote *remote;\n \tint err = 0, complete_refs_before_fetch = 1;\n \tint submodule_progress;\n+\tstruct parseopt_options parseopts = { 0 };\n \n \tstruct argv_array ref_prefixes = ARGV_ARRAY_INIT;\n \n \tfetch_if_missing = 0;\n \n \tpacket_trace_identity(\"clone\");\n-\targc = parse_options(argc, argv, prefix, builtin_clone_options,\n-\t\t\t builtin_clone_usage, 0);\n+\tparseopts.usagestr = builtin_clone_usage;\n+\tparseopts.is_abbrev_ambiguous = is_abbrev_ambiguous;\n+\targc = parse_options_extended(argc, argv, prefix,\n+\t\t\t\t builtin_clone_options,\n+\t\t\t\t &parseopts);\n \n \tif (argc > 2)\n \t\tusage_msg_opt(_(\"Too many arguments.\"),\ndiff --git a/parse-options.c b/parse-options.c\nindex cec74522e5..c0354e5a92 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(\n \t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n \t\t\t !strncmp(long_name, arg, arg_end - arg)) {\n is_abbreviated:\n-\t\t\t\tif (abbrev_option) {\n+\t\t\t\tif (abbrev_option &&\n+\t\t\t\t p->is_abbrev_ambiguous(abbrev_option, options)) {\n \t\t\t\t\t/*\n \t\t\t\t\t * If this is abbreviated, it is\n \t\t\t\t\t * ambiguous. So when there is no\n@@ -450,6 +451,12 @@ static void parse_options_check(const struct option *opts)\n \t\texit(128);\n }\n \n+static int abbrev_is_always_ambiguous(const struct option *prev,\n+\t\t\t\t const struct option *next)\n+{\n+\treturn 1;\n+}\n+\n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n \t\t\t const struct option *options, int flags)\n@@ -466,6 +473,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tctx->prefix = prefix;\n \tctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);\n \tctx->flags = flags;\n+\tctx->is_abbrev_ambiguous = abbrev_is_always_ambiguous;\n \tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n \t (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&\n \t !(flags & PARSE_OPT_ONE_SHOT))\n@@ -702,13 +710,17 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)\n \treturn ctx->cpidx + ctx->argc;\n }\n \n-int parse_options(int argc, const char **argv, const char *prefix,\n-\t\t const struct option *options, const char * const usagestr[],\n-\t\t int flags)\n+int parse_options_extended(int argc, const char **argv, const char *prefix,\n+\t\t\t const struct option *options,\n+\t\t\t const struct parseopt_options *parseopts)\n {\n \tstruct parse_opt_ctx_t ctx;\n+\tconst char * const * usagestr = parseopts->usagestr;\n+\tint flags = parseopts->flags;\n \n \tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n+\tif (parseopts->is_abbrev_ambiguous)\n+\t\tctx.is_abbrev_ambiguous = parseopts->is_abbrev_ambiguous;\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \tcase PARSE_OPT_ERROR:\n@@ -734,6 +746,17 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \treturn parse_options_end(&ctx);\n }\n \n+int parse_options(int argc, const char **argv, const char *prefix,\n+\t\t const struct option *options, const char * const usagestr[],\n+\t\t int flags)\n+{\n+\tstruct parseopt_options parseopts = {0};\n+\n+\tparseopts.flags = flags;\n+\tparseopts.usagestr = usagestr;\n+\treturn parse_options_extended(argc, argv, prefix, options, &parseopts);\n+}\n+\n static int usage_argh(const struct option *opts, FILE *outfile)\n {\n \tconst char *s;\ndiff --git a/parse-options.h b/parse-options.h\nindex 74cce4e7fc..31ebf4572c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -198,6 +198,17 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \t\t const struct option *options,\n \t\t const char * const usagestr[], int flags);\n \n+struct parseopt_options {\n+\tint flags;\n+\tconst char * const * usagestr;\n+\tint (*is_abbrev_ambiguous)(const struct option *prev,\n+\t\t\t\t const struct option *next);\n+};\n+\n+int parse_options_extended(int argc, const char **argv, const char *prefix,\n+\t\t\t const struct option *options,\n+\t\t\t const struct parseopt_options *parseopts);\n+\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n@@ -256,6 +267,7 @@ struct parse_opt_ctx_t {\n \tconst char *opt;\n \tint flags;\n \tconst char *prefix;\n+\tint (*is_abbrev_ambiguous)(const struct option *, const struct option *);\n };\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\n-- 8< --\n\n> \n> Thanks.\n"},{"id":"374160","messageId":"xmqq1s1yfw24.fsf@gitster-ct.c.googlers.com","threadId":"50818","inReplyTo":"20190418092940.GA13484@ash","subject":"Re: [PATCH v2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-19T04:39:15Z","receivedAt":"2019-04-19T19:48:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> So an alternative is simply outsource the ambiguity decision back to\n> git-clone. If the same situation appears again elsewhere, we'll need\n> to sit back and fix it for real. But this way we don't potentially\n> introduce any new traps.\n\nSounds like a sensibly safe approach.\n\n> +/*\n> + * Avoid ambiguation error between --recursive and --recurse-submodule\n> + * because they are the same. --recurs can be expanded to any of them\n> + * and it still works.\n> + */\n> +static int is_abbrev_ambiguous(const struct option *prev,\n> +\t\t\t const struct option *next)\n> +{\n> +\tconst struct option *opts[] = { prev, next };\n\nBy looking at its caller, I think the caller keeps track of the\ncandidate it saw so far, and ask this function to see if the one it\nis looking at right now (i.e. \"this one\") should be flagged as\nconflicting with the other one (or \"the other one\"). So it probably\nmakes more sense to call them or [*1*].\n\n\tSide note: *1* I would have used \"this\" instead of \"it\", but\n\tI vaguely recall there are those who want to use C++ aware\n\tstatic checkers and avoiding the identifier \"this\" is easy\n\tenough so ...\n\n> +\tint i, found = 0;\n> +\n> +\tfor (i = 0; i < ARRAY_SIZE(opts); i++) {\n> +\t\tif (!opts[i]->long_name)\n> +\t\t\tcontinue;\n> +\t\tif (!strcmp(opts[i]->long_name, \"recursive\"))\n> +\t\t\tfound |= 1 << 0;\n> +\t\tif (!strcmp(opts[i]->long_name, \"recurse-submodules\"))\n> +\t\t\tfound |= 1 << 1;\n> +\t}\n> +\n> +\treturn found != 3;\n> +}\n\nFor any two options that share the prefix, unless they are\n\"recursive\" and \"recurse-submodules\" pair, we say \"that's\nambiguous\". But when they are these two specifically singled out,\nwe say it is OK. Makes sense.\n\nThe above may be sufficient for this purpose, but I would have\nexpected that these groups of aliases would be the ones in the\ntable, not the pair. IOW, with helpers like these:\n\n\tstatic const char *recurse_submodules[] = {\n\t\t\"recurse-submodules\", \"recursive\", NULL,\n\t};\n\n\tstatic struct {\n\t\tconst char **aliases;\n\t} disamb[] = {\n\t\trecurse_submodules,\n\t};\n\t\t\n\tstatic int has_string(const char *it, const char **array)\n\t{\n\t\twhile (*array)\n\t\t\tif (!strcmp(it, *(array++)))\n\t\t\t\treturn 1;\n\t\treturn 0;\n\t}\n\nthe body would look something like:\n\n\tint j;\n\n\tfor (j = 0; j < ARRAY_SIZE(disamb); j++) {\n\t\t/* it and other are from the same family? */\n\t\tif (it->long_name &&\n\t\t has_string(it->long_name, disamb[j].aliases) &&\n\t\t other->long_name &&\n\t\t has_string(other->long_name, disamb[j].aliases))\n\t\t\treturn 0;\n\t}\n\treturn 1;\n\nPerhaps?\n\nI suspect that renaming the function (and the field in the context\nstruct) to .is_alias() and reverse the polarity of its return value\nmay make the flow of the logic at the caller side easier to follow?\n\n> diff --git a/parse-options.c b/parse-options.c\n> index cec74522e5..c0354e5a92 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(\n> \t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n> \t\t\t !strncmp(long_name, arg, arg_end - arg)) {\n> is_abbreviated:\n> -\t\t\t\tif (abbrev_option) {\n> +\t\t\t\tif (abbrev_option &&\n> +\t\t\t\t p->is_abbrev_ambiguous(abbrev_option, options)) {\n\nThe above suggestion would make the guard here to\n\n\t\t\tif (abbrev_option &&\n\t\t\t !p->is_alias(abbrev_option, options)) {\n\nThat is, at this point, we know that the user may have used the\ngiven string to name the \"abbrev_option\" we earlier saw (and made\nsure it prefix matches the string), and now we are looking at\nanother one, options[0], that also prefix matches the string. We\nusually flag this case as a problematic ambiguity inside the body of\nthis if statement, but if one is an alias to the other, we do not do\nso.\n\n> \t\t\t\t\t/*\n> \t\t\t\t\t * If this is abbreviated, it is\n> \t\t\t\t\t * ambiguous. So when there is no\n"},{"id":"374239","messageId":"20190422122250.15248-1-pclouds@gmail.com","threadId":"50818","inReplyTo":"xmqq1s1yfw24.fsf@gitster-ct.c.googlers.com","subject":"[PATCH] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-22T12:22:50Z","receivedAt":"2019-04-22T12:23:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Change the option parsing machinery so that e.g. \"clone --recurs ...\"\ndoesn't error out because \"clone\" understands both \"--recursive\" and\n\"--recurse-submodules\" to mean the same thing.\n\nInitially \"clone\" just understood --recursive until the\n--recurses-submodules alias was added in ccdd3da652 (\"clone: Add the\n--recurse-submodules option as alias for --recursive\",\n2010-11-04). Since bb62e0a99f (\"clone: teach --recurse-submodules to\noptionally take a pathspec\", 2017-03-17) the longer form has been\npromoted to the default.\n\nBut due to the way the options parsing machinery works this resulted\nin the rather absurd situation of:\n\n $ git clone --recurs [...]\n error: ambiguous option: recurs (could be --recursive or --recurse-submodules)\n\nAdd OPT_ALIAS() to express this linke between two options and use it in\ngit-clone.\n\nA big chunk of code is actually from Junio C Hamano. The test case (and\nmost of the commit message) is from Ævar Arnfjörð Bjarmason.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I'm not sure if Junio is an evil genius who lures me with his helpful\n suggestions. Either way I ended up spending a couple hours writing and\n rewriting and finally decided all other options were too ugly. And his\n suggestions were mostly there already.\n \n So here's OPT_ALIAS(). I avoid rewriting all the option iteration code\n by simply making a copy of options[] array to use, then free it\n afterwards. Not the best, but at least it could be easily removed if we\n end up not want OPT_ALIAS() after all.\n\n This probably needs signoff from both Ævar and Junio, if accepted.\n\n builtin/clone.c | 5 +-\n parse-options.c | 130 +++++++++++++++++++++++++++++++++++++--\n parse-options.h | 6 ++\n t/t0040-parse-options.sh | 5 ++\n 4 files changed, 136 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..703b7247ad 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"don't use local hardlinks, always copy\")),\n \tOPT_BOOL('s', \"shared\", &option_shared,\n \t\t N_(\"setup as shared repository\")),\n-\t{ OPTION_CALLBACK, 0, \"recursive\", &option_recurse_submodules,\n-\t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n-\t PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,\n-\t (intptr_t)\".\" },\n+\tOPT_ALIAS(0, \"recursive\", \"recurse-submodules\"),\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n \t PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\ndiff --git a/parse-options.c b/parse-options.c\nindex acc3a93660..7ec768e376 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -261,6 +261,35 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,\n \treturn PARSE_OPT_UNKNOWN;\n }\n \n+static int has_string(const char *it, const char **array)\n+{\n+\twhile (*array)\n+\t\tif (!strcmp(it, *(array++)))\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static int is_alias(struct parse_opt_ctx_t *ctx,\n+\t\t const struct option *one_opt,\n+\t\t const struct option *another_opt)\n+{\n+\tconst char **group;\n+\n+\tif (!ctx->alias_groups)\n+\t\treturn 0;\n+\n+\tif (!one_opt->long_name || !another_opt->long_name)\n+\t\treturn 0;\n+\n+\tfor (group = ctx->alias_groups; *group; group += 3) {\n+\t\t/* it and other are from the same family? */\n+\t\tif (has_string(one_opt->long_name, group) &&\n+\t\t has_string(another_opt->long_name, group))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static enum parse_opt_result parse_long_opt(\n \tstruct parse_opt_ctx_t *p, const char *arg,\n \tconst struct option *options)\n@@ -296,7 +325,8 @@ static enum parse_opt_result parse_long_opt(\n \t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n \t\t\t !strncmp(long_name, arg, arg_end - arg)) {\n is_abbreviated:\n-\t\t\t\tif (abbrev_option) {\n+\t\t\t\tif (abbrev_option &&\n+\t\t\t\t !is_alias(p, abbrev_option, options)) {\n \t\t\t\t\t/*\n \t\t\t\t\t * If this is abbreviated, it is\n \t\t\t\t\t * ambiguous. So when there is no\n@@ -445,6 +475,10 @@ static void parse_options_check(const struct option *opts)\n \t\t\tif (opts->callback)\n \t\t\t\tBUG(\"OPTION_LOWLEVEL_CALLBACK needs no high level callback\");\n \t\t\tbreak;\n+\t\tcase OPTION_ALIAS:\n+\t\t\tBUG(\"OPT_ALIAS() should not remain at this point. \"\n+\t\t\t \"Are you using parse_options_step() directly?\\n\"\n+\t\t\t \"That case is not supported yet.\");\n \t\tdefault:\n \t\t\t; /* ok. (usually accepts an argument) */\n \t\t}\n@@ -456,11 +490,10 @@ static void parse_options_check(const struct option *opts)\n \t\texit(128);\n }\n \n-void parse_options_start(struct parse_opt_ctx_t *ctx,\n-\t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t const struct option *options, int flags)\n+static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t int argc, const char **argv, const char *prefix,\n+\t\t\t\t const struct option *options, int flags)\n {\n-\tmemset(ctx, 0, sizeof(*ctx));\n \tctx->argc = argc;\n \tctx->argv = argv;\n \tif (!(flags & PARSE_OPT_ONE_SHOT)) {\n@@ -482,6 +515,14 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tparse_options_check(options);\n }\n \n+void parse_options_start(struct parse_opt_ctx_t *ctx,\n+\t\t\t int argc, const char **argv, const char *prefix,\n+\t\t\t const struct option *options, int flags)\n+{\n+\tmemset(ctx, 0, sizeof(*ctx));\n+\tparse_options_start_1(ctx, argc, argv, prefix, options, flags);\n+}\n+\n static void show_negated_gitcomp(const struct option *opts, int nr_noopts)\n {\n \tint printed_dashdash = 0;\n@@ -574,6 +615,70 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,\n \treturn PARSE_OPT_COMPLETE;\n }\n \n+/*\n+ * Scan and may produce a new option[] array, which should be used\n+ * instead of the original 'options'.\n+ *\n+ * Right now this is only used to preprocess and substitue\n+ * OPTION_ALIAS.\n+ */\n+static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *options)\n+{\n+\tstruct option *newopt;\n+\tint i, nr, alias;\n+\tint nr_aliases = 0;\n+\n+\tfor (nr = 0; options[nr].type != OPTION_END; nr++) {\n+\t\tif (options[nr].type == OPTION_ALIAS)\n+\t\t\tnr_aliases++;\n+\t}\n+\n+\tif (!nr_aliases)\n+\t\treturn NULL;\n+\n+\tALLOC_ARRAY(newopt, nr + 1);\n+\tCOPY_ARRAY(newopt, options, nr + 1);\n+\n+\t/* each alias has two string pointers and NULL */\n+\tCALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));\n+\n+\tfor (alias = 0, i = 0; i < nr; i++) {\n+\t\tconst char *source;\n+\t\tint j;\n+\n+\t\tif (newopt[i].type != OPTION_ALIAS)\n+\t\t\tcontinue;\n+\n+\t\tif (!newopt[i].long_name)\n+\t\t\tBUG(\"An alias must have long option name\");\n+\n+\t\tsource = newopt[i].value;\n+\n+\t\tfor (j = 0; j < nr; j++) {\n+\t\t\tconst char *name = options[j].long_name;\n+\n+\t\t\tif (!name || strcmp(name, source))\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (options[j].type == OPTION_ALIAS)\n+\t\t\t\tBUG(\"No please. Nested aliases are not supported.\");\n+\n+\t\t\tmemcpy(newopt + i, options + j, sizeof(*newopt));\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tif (j == nr)\n+\t\t\tBUG(\"could not find source option '%s' of alias '%s'\",\n+\t\t\t source, newopt[i].long_name);\n+\t\tctx->alias_groups[alias * 3 + 0] = newopt[i].long_name;\n+\t\tctx->alias_groups[alias * 3 + 1] = options[j].long_name;\n+\t\talias++;\n+\t}\n+\n+\treturn newopt;\n+}\n+\n static int usage_with_options_internal(struct parse_opt_ctx_t *,\n \t\t\t\t const char * const *,\n \t\t\t\t const struct option *, int, int);\n@@ -713,11 +818,16 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \t\t int flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n+\tstruct option *real_options;\n \n \tdisallow_abbreviated_options =\n \t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n \n-\tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n+\tmemset(&ctx, 0, sizeof(ctx));\n+\treal_options = preprocess_options(&ctx, options);\n+\tif (real_options)\n+\t\toptions = real_options;\n+\tparse_options_start_1(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \tcase PARSE_OPT_ERROR:\n@@ -740,6 +850,8 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \t}\n \n \tprecompose_argv(argc, argv);\n+\tfree(real_options);\n+\tfree(ctx.alias_groups);\n \treturn parse_options_end(&ctx);\n }\n \n@@ -834,6 +946,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\t\tfputc('\\n', outfile);\n \t\t\tpad = USAGE_OPTS_WIDTH;\n \t\t}\n+\t\tif (opts->type == OPTION_ALIAS) {\n+\t\t\tfprintf(outfile, \"%*s\", pad + USAGE_GAP, \"\");\n+\t\t\tfprintf_ln(outfile, _(\"alias of --%s\"),\n+\t\t\t\t (const char *)opts->value);\n+\t\t\tcontinue;\n+\t\t}\n \t\tfprintf(outfile, \"%*s%s\\n\", pad + USAGE_GAP, \"\", _(opts->help));\n \t}\n \tfputc('\\n', outfile);\ndiff --git a/parse-options.h b/parse-options.h\nindex 74cce4e7fc..e6b4a8d1c7 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -7,6 +7,7 @@ enum parse_opt_type {\n \tOPTION_ARGUMENT,\n \tOPTION_GROUP,\n \tOPTION_NUMBER,\n+\tOPTION_ALIAS,\n \t/* options with no arguments */\n \tOPTION_BIT,\n \tOPTION_NEGBIT,\n@@ -181,6 +182,9 @@ struct option {\n \t N_(\"no-op (backward compatibility)\"),\t\t\\\n \t PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }\n \n+#define OPT_ALIAS(s, l, alias_source) \\\n+\t{ OPTION_ALIAS, (s), (l), (alias_source) }\n+\n /*\n * parse_options() will filter out the processed options and leave the\n * non-option arguments in argv[]. argv0 is assumed program name and\n@@ -256,6 +260,8 @@ struct parse_opt_ctx_t {\n \tconst char *opt;\n \tint flags;\n \tconst char *prefix;\n+\tconst char **alias_groups; /* must be in groups of 3 elements! */\n+\tstruct option *updated_options;\n };\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 800b3ea5f5..14e6664240 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -224,6 +224,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n \ttest-tool parse-options --expect=\"string: 123\" --st 123\n '\n \n+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '\n+\ttest_when_finished \"rm -rf A\" &&\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false git clone --recurs . A\n+'\n+\n cat >typo.err <<\\EOF\n error: did you mean `--boolean` (with two dashes ?)\n EOF\n-- \n2.21.0.854.ge34a79f761\n\n"},{"id":"374240","messageId":"CACsJy8ByLxUn8vgSmkPJQvO5vV-=vSwyP=ug3QKJL7hhD11-KA@mail.gmail.com","threadId":"50818","inReplyTo":"20190422122250.15248-1-pclouds@gmail.com","subject":"Re: [PATCH] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-22T12:34:40Z","receivedAt":"2019-04-22T12:35:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Apr 22, 2019 at 7:23 PM Nguyễn Thái Ngọc Duy wrote:\n> @@ -574,6 +615,70 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,\n> return PARSE_OPT_COMPLETE;\n> }\n>\n> +/*\n> + * Scan and may produce a new option[] array, which should be used\n> + * instead of the original 'options'.\n> + *\n> + * Right now this is only used to preprocess and substitue\n> + * OPTION_ALIAS.\n> + */\n> +static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,\n> + const struct option *options)\n> +{\n> + struct option *newopt;\n> + int i, nr, alias;\n> + int nr_aliases = 0;\n> +\n> + for (nr = 0; options[nr].type != OPTION_END; nr++) {\n> + if (options[nr].type == OPTION_ALIAS)\n> + nr_aliases++;\n> + }\n> +\n> + if (!nr_aliases)\n> + return NULL;\n> +\n> + ALLOC_ARRAY(newopt, nr + 1);\n> + COPY_ARRAY(newopt, options, nr + 1);\n> +\n> + /* each alias has two string pointers and NULL */\n> + CALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));\n> +\n> + for (alias = 0, i = 0; i < nr; i++) {\n> + const char *source;\n> + int j;\n> +\n> + if (newopt[i].type != OPTION_ALIAS)\n> + continue;\n> +\n> + if (!newopt[i].long_name)\n> + BUG(\"An alias must have long option name\");\n> +\n> + source = newopt[i].value;\n> +\n> + for (j = 0; j < nr; j++) {\n> + const char *name = options[j].long_name;\n> +\n> + if (!name || strcmp(name, source))\n> + continue;\n> +\n> + if (options[j].type == OPTION_ALIAS)\n> + BUG(\"No please. Nested aliases are not supported.\");\n> +\n> + memcpy(newopt + i, options + j, sizeof(*newopt));\n\nEck. I need to restore newopt[i].long_name and .short_name after this\nmemcpy, or we'll just have two --recurse-submodules instead. Luckily\nit still works after this bug is gone.\n\n> + break;\n> + }\n-- \nDuy\n"},{"id":"374634","messageId":"20190429100525.32045-1-pclouds@gmail.com","threadId":"50818","inReplyTo":"20190422122250.15248-1-pclouds@gmail.com","subject":"[PATCH v2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-29T10:05:25Z","receivedAt":"2019-04-29T10:05:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Change the option parsing machinery so that e.g. \"clone --recurs ...\"\ndoesn't error out because \"clone\" understands both \"--recursive\" and\n\"--recurse-submodules\" to mean the same thing.\n\nInitially \"clone\" just understood --recursive until the\n--recurses-submodules alias was added in ccdd3da652 (\"clone: Add the\n--recurse-submodules option as alias for --recursive\",\n2010-11-04). Since bb62e0a99f (\"clone: teach --recurse-submodules to\noptionally take a pathspec\", 2017-03-17) the longer form has been\npromoted to the default.\n\nBut due to the way the options parsing machinery works this resulted\nin the rather absurd situation of:\n\n $ git clone --recurs [...]\n error: ambiguous option: recurs (could be --recursive or --recurse-submodules)\n\nAdd OPT_ALIAS() to express this link between two or more options and use\nit in git-clone. Multiple aliases of an option could be written as\n\n OPT_ALIAS(0, \"alias1\", \"original-name\"),\n OPT_ALIAS(0, \"alias2\", \"original-name\"),\n ...\n\nThe current implementation is not exactly optimal in this case. But we\ncan optimize it when it becomes a problem. So far we don't even have two\naliases of any option.\n\nA big chunk of code is actually from Junio C Hamano.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n OK it's working for real this time. test-parse-options.c is also\n updated to for testing OPT_ALIAS.\n\n builtin/clone.c | 5 +-\n parse-options.c | 143 ++++++++++++++++++++++++++++++++--\n parse-options.h | 6 ++\n t/helper/test-parse-options.c | 3 +\n t/t0040-parse-options.sh | 17 ++++\n 5 files changed, 164 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 50bde99618..703b7247ad 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = {\n \t\t N_(\"don't use local hardlinks, always copy\")),\n \tOPT_BOOL('s', \"shared\", &option_shared,\n \t\t N_(\"setup as shared repository\")),\n-\t{ OPTION_CALLBACK, 0, \"recursive\", &option_recurse_submodules,\n-\t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n-\t PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,\n-\t (intptr_t)\".\" },\n+\tOPT_ALIAS(0, \"recursive\", \"recurse-submodules\"),\n \t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &option_recurse_submodules,\n \t N_(\"pathspec\"), N_(\"initialize submodules in the clone\"),\n \t PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)\".\" },\ndiff --git a/parse-options.c b/parse-options.c\nindex acc3a93660..1b1cc2add7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -261,6 +261,35 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,\n \treturn PARSE_OPT_UNKNOWN;\n }\n \n+static int has_string(const char *it, const char **array)\n+{\n+\twhile (*array)\n+\t\tif (!strcmp(it, *(array++)))\n+\t\t\treturn 1;\n+\treturn 0;\n+}\n+\n+static int is_alias(struct parse_opt_ctx_t *ctx,\n+\t\t const struct option *one_opt,\n+\t\t const struct option *another_opt)\n+{\n+\tconst char **group;\n+\n+\tif (!ctx->alias_groups)\n+\t\treturn 0;\n+\n+\tif (!one_opt->long_name || !another_opt->long_name)\n+\t\treturn 0;\n+\n+\tfor (group = ctx->alias_groups; *group; group += 3) {\n+\t\t/* it and other are from the same family? */\n+\t\tif (has_string(one_opt->long_name, group) &&\n+\t\t has_string(another_opt->long_name, group))\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static enum parse_opt_result parse_long_opt(\n \tstruct parse_opt_ctx_t *p, const char *arg,\n \tconst struct option *options)\n@@ -296,7 +325,8 @@ static enum parse_opt_result parse_long_opt(\n \t\t\tif (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&\n \t\t\t !strncmp(long_name, arg, arg_end - arg)) {\n is_abbreviated:\n-\t\t\t\tif (abbrev_option) {\n+\t\t\t\tif (abbrev_option &&\n+\t\t\t\t !is_alias(p, abbrev_option, options)) {\n \t\t\t\t\t/*\n \t\t\t\t\t * If this is abbreviated, it is\n \t\t\t\t\t * ambiguous. So when there is no\n@@ -445,6 +475,10 @@ static void parse_options_check(const struct option *opts)\n \t\t\tif (opts->callback)\n \t\t\t\tBUG(\"OPTION_LOWLEVEL_CALLBACK needs no high level callback\");\n \t\t\tbreak;\n+\t\tcase OPTION_ALIAS:\n+\t\t\tBUG(\"OPT_ALIAS() should not remain at this point. \"\n+\t\t\t \"Are you using parse_options_step() directly?\\n\"\n+\t\t\t \"That case is not supported yet.\");\n \t\tdefault:\n \t\t\t; /* ok. (usually accepts an argument) */\n \t\t}\n@@ -456,11 +490,10 @@ static void parse_options_check(const struct option *opts)\n \t\texit(128);\n }\n \n-void parse_options_start(struct parse_opt_ctx_t *ctx,\n-\t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t const struct option *options, int flags)\n+static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t int argc, const char **argv, const char *prefix,\n+\t\t\t\t const struct option *options, int flags)\n {\n-\tmemset(ctx, 0, sizeof(*ctx));\n \tctx->argc = argc;\n \tctx->argv = argv;\n \tif (!(flags & PARSE_OPT_ONE_SHOT)) {\n@@ -482,6 +515,14 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tparse_options_check(options);\n }\n \n+void parse_options_start(struct parse_opt_ctx_t *ctx,\n+\t\t\t int argc, const char **argv, const char *prefix,\n+\t\t\t const struct option *options, int flags)\n+{\n+\tmemset(ctx, 0, sizeof(*ctx));\n+\tparse_options_start_1(ctx, argc, argv, prefix, options, flags);\n+}\n+\n static void show_negated_gitcomp(const struct option *opts, int nr_noopts)\n {\n \tint printed_dashdash = 0;\n@@ -574,6 +615,83 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,\n \treturn PARSE_OPT_COMPLETE;\n }\n \n+/*\n+ * Scan and may produce a new option[] array, which should be used\n+ * instead of the original 'options'.\n+ *\n+ * Right now this is only used to preprocess and substitue\n+ * OPTION_ALIAS.\n+ */\n+static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,\n+\t\t\t\t\t const struct option *options)\n+{\n+\tstruct option *newopt;\n+\tint i, nr, alias;\n+\tint nr_aliases = 0;\n+\n+\tfor (nr = 0; options[nr].type != OPTION_END; nr++) {\n+\t\tif (options[nr].type == OPTION_ALIAS)\n+\t\t\tnr_aliases++;\n+\t}\n+\n+\tif (!nr_aliases)\n+\t\treturn NULL;\n+\n+\tALLOC_ARRAY(newopt, nr + 1);\n+\tCOPY_ARRAY(newopt, options, nr + 1);\n+\n+\t/* each alias has two string pointers and NULL */\n+\tCALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));\n+\n+\tfor (alias = 0, i = 0; i < nr; i++) {\n+\t\tint short_name;\n+\t\tconst char *long_name;\n+\t\tconst char *source;\n+\t\tint j;\n+\n+\t\tif (newopt[i].type != OPTION_ALIAS)\n+\t\t\tcontinue;\n+\n+\t\tshort_name = newopt[i].short_name;\n+\t\tlong_name = newopt[i].long_name;\n+\t\tsource = newopt[i].value;\n+\n+\t\tif (!long_name)\n+\t\t\tBUG(\"An alias must have long option name\");\n+\n+\t\tfor (j = 0; j < nr; j++) {\n+\t\t\tconst char *name = options[j].long_name;\n+\n+\t\t\tif (!name || strcmp(name, source))\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (options[j].type == OPTION_ALIAS)\n+\t\t\t\tBUG(\"No please. Nested aliases are not supported.\");\n+\n+\t\t\t/*\n+\t\t\t * NEEDSWORK: this is a bit inconsistent because\n+\t\t\t * usage_with_options() on the original options[] will print\n+\t\t\t * help string as \"alias of %s\" but \"git cmd -h\" will\n+\t\t\t * print the original help string.\n+\t\t\t */\n+\t\t\tmemcpy(newopt + i, options + j, sizeof(*newopt));\n+\t\t\tnewopt[i].short_name = short_name;\n+\t\t\tnewopt[i].long_name = long_name;\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tif (j == nr)\n+\t\t\tBUG(\"could not find source option '%s' of alias '%s'\",\n+\t\t\t source, newopt[i].long_name);\n+\t\tctx->alias_groups[alias * 3 + 0] = newopt[i].long_name;\n+\t\tctx->alias_groups[alias * 3 + 1] = options[j].long_name;\n+\t\tctx->alias_groups[alias * 3 + 2] = NULL;\n+\t\talias++;\n+\t}\n+\n+\treturn newopt;\n+}\n+\n static int usage_with_options_internal(struct parse_opt_ctx_t *,\n \t\t\t\t const char * const *,\n \t\t\t\t const struct option *, int, int);\n@@ -713,11 +831,16 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \t\t int flags)\n {\n \tstruct parse_opt_ctx_t ctx;\n+\tstruct option *real_options;\n \n \tdisallow_abbreviated_options =\n \t\tgit_env_bool(\"GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS\", 0);\n \n-\tparse_options_start(&ctx, argc, argv, prefix, options, flags);\n+\tmemset(&ctx, 0, sizeof(ctx));\n+\treal_options = preprocess_options(&ctx, options);\n+\tif (real_options)\n+\t\toptions = real_options;\n+\tparse_options_start_1(&ctx, argc, argv, prefix, options, flags);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \tcase PARSE_OPT_ERROR:\n@@ -740,6 +863,8 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \t}\n \n \tprecompose_argv(argc, argv);\n+\tfree(real_options);\n+\tfree(ctx.alias_groups);\n \treturn parse_options_end(&ctx);\n }\n \n@@ -834,6 +959,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\t\tfputc('\\n', outfile);\n \t\t\tpad = USAGE_OPTS_WIDTH;\n \t\t}\n+\t\tif (opts->type == OPTION_ALIAS) {\n+\t\t\tfprintf(outfile, \"%*s\", pad + USAGE_GAP, \"\");\n+\t\t\tfprintf_ln(outfile, _(\"alias of --%s\"),\n+\t\t\t\t (const char *)opts->value);\n+\t\t\tcontinue;\n+\t\t}\n \t\tfprintf(outfile, \"%*s%s\\n\", pad + USAGE_GAP, \"\", _(opts->help));\n \t}\n \tfputc('\\n', outfile);\ndiff --git a/parse-options.h b/parse-options.h\nindex 74cce4e7fc..9ed479f41d 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -7,6 +7,7 @@ enum parse_opt_type {\n \tOPTION_ARGUMENT,\n \tOPTION_GROUP,\n \tOPTION_NUMBER,\n+\tOPTION_ALIAS,\n \t/* options with no arguments */\n \tOPTION_BIT,\n \tOPTION_NEGBIT,\n@@ -181,6 +182,9 @@ struct option {\n \t N_(\"no-op (backward compatibility)\"),\t\t\\\n \t PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }\n \n+#define OPT_ALIAS(s, l, source_long_name) \\\n+\t{ OPTION_ALIAS, (s), (l), (source_long_name) }\n+\n /*\n * parse_options() will filter out the processed options and leave the\n * non-option arguments in argv[]. argv0 is assumed program name and\n@@ -256,6 +260,8 @@ struct parse_opt_ctx_t {\n \tconst char *opt;\n \tint flags;\n \tconst char *prefix;\n+\tconst char **alias_groups; /* must be in groups of 3 elements! */\n+\tstruct option *updated_options;\n };\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex cc88fba057..76cfb87588 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -149,6 +149,9 @@ int cmd__parse_options(int argc, const char **argv)\n \t\tOPT_CALLBACK(0, \"expect\", &expect, \"string\",\n \t\t\t \"expected output in the variable dump\",\n \t\t\t collect_expect),\n+\t\tOPT_GROUP(\"Alias\"),\n+\t\tOPT_STRING('A', \"alias-source\", &string, \"string\", \"get a string\"),\n+\t\tOPT_ALIAS('Z', \"alias-target\", \"alias-source\"),\n \t\tOPT_END(),\n \t};\n \tint i;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 800b3ea5f5..cebc77fab0 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -48,6 +48,12 @@ Standard options\n -q, --quiet be quiet\n --expect expected output in the variable dump\n \n+Alias\n+ -A, --alias-source \n+ get a string\n+ -Z, --alias-target \n+ get a string\n+\n EOF\n \n test_expect_success 'test help' '\n@@ -224,6 +230,17 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '\n \ttest-tool parse-options --expect=\"string: 123\" --st 123\n '\n \n+test_expect_success 'Alias options do not contribute to abbreviation' '\n+\ttest-tool parse-options --alias-source 123 >output &&\n+\tgrep \"^string: 123\" output &&\n+\ttest-tool parse-options --alias-target 123 >output &&\n+\tgrep \"^string: 123\" output &&\n+\ttest_must_fail test-tool parse-options --alias &&\n+\tGIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \\\n+\ttest-tool parse-options --alias 123 >output &&\n+\tgrep \"^string: 123\" output\n+'\n+\n cat >typo.err <<\\EOF\n error: did you mean `--boolean` (with two dashes ?)\n EOF\n-- \n2.21.0.1110.g9614c01b33\n\n"},{"id":"374981","messageId":"xmqq1s1bnd4q.fsf@gitster-ct.c.googlers.com","threadId":"50818","inReplyTo":"20190429100525.32045-1-pclouds@gmail.com","subject":"Re: [PATCH v2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-07T03:43:01Z","receivedAt":"2019-05-07T03:43:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> But due to the way the options parsing machinery works this resulted\n> in the rather absurd situation of:\n>\n> $ git clone --recurs [...]\n> error: ambiguous option: recurs (could be --recursive or --recurse-submodules)\n>\n> Add OPT_ALIAS() to express this link between two or more options and use\n> it in git-clone. Multiple aliases of an option could be written as\n>\n> OPT_ALIAS(0, \"alias1\", \"original-name\"),\n> OPT_ALIAS(0, \"alias2\", \"original-name\"),\n> ...\n>\n> The current implementation is not exactly optimal in this case. But we\n> can optimize it when it becomes a problem. So far we don't even have two\n> aliases of any option.\n\nSounds good enough for any practical need I can forsee ;-) Thanks.\n\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index 800b3ea5f5..cebc77fab0 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -48,6 +48,12 @@ Standard options\n> -q, --quiet be quiet\n> --expect expected output in the variable dump\n> \n> +Alias\n> + -A, --alias-source \n> + get a string\n> + -Z, --alias-target \n> + get a string\n> +\n> EOF\n\nThis is not a new problem per-se, as there already is a line before\nthe precontext of this hunk that shares the same issue, but to\nprevent future problems, I am very much tempted to apply the\nattached on top.\n\n-- >8 --\nSubject: t0040: protect lines that are indented by spaces\n\nThis block is byte-for-byte identical expected output, that contains a\nfew lines that are indented in many spaces, which makes \"git diff --check\"\nunhappy and will break the test when \"git am --whitespace=fix\" is\nallowed to \"correct\" them.\n\nProtect the left edge with a marker character, and strip it with\nsed, which is used as a standard way to deal with this issue in our\ntest suite.\n\nSigned-off-by: Signed-off-by: Junio C Hamano \n---\n * Of course, if the right-edge need to be protected, we can do so\n as well.\n\n t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------\n 1 file changed, 47 insertions(+), 47 deletions(-)\n\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex cebc77fab0..26373b5b72 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -7,53 +7,53 @@ test_description='our own option parser'\n \n . ./test-lib.sh\n \n-cat >expect <<\\EOF\n-usage: test-tool parse-options \n-\n- A helper function for the parse-options API.\n-\n- --yes get a boolean\n- -D, --no-doubt begins with 'no-'\n- -B, --no-fear be brave\n- -b, --boolean increment by one\n- -4, --or4 bitwise-or boolean with ...0100\n- --neg-or4 same as --no-or4\n-\n- -i, --integer get a integer\n- -j get a integer, too\n- -m, --magnitude get a magnitude\n- --set23 set integer to 23\n- -L, --length get length of \n- -F, --file set file to \n-\n-String options\n- -s, --string \n- get a string\n- --string2 get another string\n- --st get another string (pervert ordering)\n- -o get another string\n- --list add str to list\n-\n-Magic arguments\n- --quux means --quux\n- -NUM set integer to NUM\n- + same as -b\n- --ambiguous positive ambiguity\n- --no-ambiguous negative ambiguity\n-\n-Standard options\n- --abbrev[=] use digits to display SHA-1s\n- -v, --verbose be verbose\n- -n, --dry-run dry run\n- -q, --quiet be quiet\n- --expect expected output in the variable dump\n-\n-Alias\n- -A, --alias-source \n- get a string\n- -Z, --alias-target \n- get a string\n-\n+sed -e 's/^|//' >expect <<\\EOF\n+|usage: test-tool parse-options \n+|\n+| A helper function for the parse-options API.\n+|\n+| --yes get a boolean\n+| -D, --no-doubt begins with 'no-'\n+| -B, --no-fear be brave\n+| -b, --boolean increment by one\n+| -4, --or4 bitwise-or boolean with ...0100\n+| --neg-or4 same as --no-or4\n+|\n+| -i, --integer get a integer\n+| -j get a integer, too\n+| -m, --magnitude get a magnitude\n+| --set23 set integer to 23\n+| -L, --length get length of \n+| -F, --file set file to \n+|\n+|String options\n+| -s, --string \n+| get a string\n+| --string2 get another string\n+| --st get another string (pervert ordering)\n+| -o get another string\n+| --list add str to list\n+|\n+|Magic arguments\n+| --quux means --quux\n+| -NUM set integer to NUM\n+| + same as -b\n+| --ambiguous positive ambiguity\n+| --no-ambiguous negative ambiguity\n+|\n+|Standard options\n+| --abbrev[=] use digits to display SHA-1s\n+| -v, --verbose be verbose\n+| -n, --dry-run dry run\n+| -q, --quiet be quiet\n+| --expect expected output in the variable dump\n+|\n+|Alias\n+| -A, --alias-source \n+| get a string\n+| -Z, --alias-target \n+| get a string\n+|\n EOF\n \n test_expect_success 'test help' '\n"},{"id":"375025","messageId":"CACsJy8ARjTAWqB803WWSk15Y-66SWVPiz=+M7NEQezTPWbpJwA@mail.gmail.com","threadId":"50818","inReplyTo":"xmqq1s1bnd4q.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] parse-options: don't emit \"ambiguous option\" for aliases","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-05-07T11:58:51Z","receivedAt":"2019-05-07T11:59:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 7, 2019 at 10:43 AM Junio C Hamano wrote:\n> -- >8 --\n> Subject: t0040: protect lines that are indented by spaces\n>\n> This block is byte-for-byte identical expected output, that contains a\n> few lines that are indented in many spaces, which makes \"git diff --check\"\n> unhappy and will break the test when \"git am --whitespace=fix\" is\n> allowed to \"correct\" them.\n>\n> Protect the left edge with a marker character, and strip it with\n> sed, which is used as a standard way to deal with this issue in our\n> test suite.\n>\n> Signed-off-by: Signed-off-by: Junio C Hamano \n> ---\n> * Of course, if the right-edge need to be protected, we can do so\n> as well.\n>\n> t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------\n> 1 file changed, 47 insertions(+), 47 deletions(-)\n>\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index cebc77fab0..26373b5b72 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -7,53 +7,53 @@ test_description='our own option parser'\n>\n> . ./test-lib.sh\n>\n> -cat >expect <<\\EOF\n> -usage: test-tool parse-options \n> -\n> - A helper function for the parse-options API.\n> -\n> - --yes get a boolean\n> - -D, --no-doubt begins with 'no-'\n> - -B, --no-fear be brave\n> - -b, --boolean increment by one\n> - -4, --or4 bitwise-or boolean with ...0100\n> - --neg-or4 same as --no-or4\n> -\n> - -i, --integer get a integer\n> - -j get a integer, too\n> - -m, --magnitude get a magnitude\n> - --set23 set integer to 23\n> - -L, --length get length of \n> - -F, --file set file to \n> -\n> -String options\n> - -s, --string \n> - get a string\n> - --string2 get another string\n> - --st get another string (pervert ordering)\n> - -o get another string\n> - --list add str to list\n> -\n> -Magic arguments\n> - --quux means --quux\n> - -NUM set integer to NUM\n> - + same as -b\n> - --ambiguous positive ambiguity\n> - --no-ambiguous negative ambiguity\n> -\n> -Standard options\n> - --abbrev[=] use digits to display SHA-1s\n> - -v, --verbose be verbose\n> - -n, --dry-run dry run\n> - -q, --quiet be quiet\n> - --expect expected output in the variable dump\n> -\n> -Alias\n> - -A, --alias-source \n> - get a string\n> - -Z, --alias-target \n> - get a string\n> -\n> +sed -e 's/^|//' >expect <<\\EOF\n\nI think we already use qz_to_tab_space to protect leading spaces\n(t1450) or trailing ones (t4205). It's less strict than this though.\n\nAnyway, if you go with 's/^|//', maybe make it a helper function too\nbecause I'm pretty sure we have more text like this in the test suite.\n\nThere's also the \"tr -d Q\" trick in t4038. But that's something to\nclean up if someone really have free time.\n\n> +|usage: test-tool parse-options \n> +|\n> +| A helper function for the parse-options API.\n> +|\n> +| --yes get a boolean\n> +| -D, --no-doubt begins with 'no-'\n> +| -B, --no-fear be brave\n> +| -b, --boolean increment by one\n> +| -4, --or4 bitwise-or boolean with ...0100\n> +| --neg-or4 same as --no-or4\n> +|\n> +| -i, --integer get a integer\n> +| -j get a integer, too\n> +| -m, --magnitude get a magnitude\n> +| --set23 set integer to 23\n> +| -L, --length get length of \n> +| -F, --file set file to \n> +|\n> +|String options\n> +| -s, --string \n> +| get a string\n> +| --string2 get another string\n> +| --st get another string (pervert ordering)\n> +| -o get another string\n> +| --list add str to list\n> +|\n> +|Magic arguments\n> +| --quux means --quux\n> +| -NUM set integer to NUM\n> +| + same as -b\n> +| --ambiguous positive ambiguity\n> +| --no-ambiguous negative ambiguity\n> +|\n> +|Standard options\n> +| --abbrev[=] use digits to display SHA-1s\n> +| -v, --verbose be verbose\n> +| -n, --dry-run dry run\n> +| -q, --quiet be quiet\n> +| --expect expected output in the variable dump\n> +|\n> +|Alias\n> +| -A, --alias-source \n> +| get a string\n> +| -Z, --alias-target \n> +| get a string\n> +|\n> EOF\n>\n> test_expect_success 'test help' '\n\n\n\n-- \nDuy\n"}]}