[PATCH 0/8] Do not use abbreviated options in tests

[PATCH 0/8] Do not use abbreviated options in tests

[Johannes Schindelin via GitGitGadget]

We do not want to have tests that need to be changed by completely unrelated
patch series, just because abbreviations that used to be unique are being
made non-unique by said patch series.

I stumbled over this while investigating the test failures in
dl/rebase-i-keep-base
[https://dev.azure.com/gitgitgadget/git/_build/results?buildId=5482&view=ms.vss-test-web.build-test-results-tab]
: the new --keep-base option makes the --keep abbreviation of --keep-empty 
in t5407 non-unique, which causes the test suite to fail.

Johannes Schindelin (8):
  tests (rebase): spell out the `--keep-empty` option
  tests (rebase): spell out the `--force-rebase` option
  t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`
  t5531: avoid using an abbreviated option
  tests (push): do not abbreviate the `--follow-tags` option
  tests (status): spell out the `--find-renames` option in full
  tests (pack-objects): use the full, unabbreviated `--revs` option
  tests: disallow the use of abbreviated options (by default)

 parse-options.c                        |  9 ++++++
 t/README                               |  4 +++
 t/t0040-parse-options.sh               |  7 +++-
 t/t3415-rebase-autosquash.sh           |  2 +-
 t/t3430-rebase-merges.sh               |  4 +--
 t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------
 t/t5407-post-rewrite-hook.sh           |  4 +--
 t/t5516-fetch-push.sh                  |  4 +--
 t/t5531-deep-submodule-push.sh         |  2 +-
 t/t7525-status-rename.sh               |  8 ++---
 t/t7810-grep.sh                        | 16 +++++-----
 t/test-lib.sh                          |  6 ++++
 12 files changed, 67 insertions(+), 43 deletions(-)


base-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-167%2Fdscho%2Fdisallow-abbreviated-options-in-tests-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-167/dscho/disallow-abbreviated-options-in-tests-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/167
-- 
gitgitgadget

[PATCH 1/8] tests (rebase): spell out the `--keep-empty` option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This test wants to run `git rebase` with the `--keep-empty` option, but
it really only spelled out `--keep` and trusted Git's option parsing to
determine that this was a unique abbreviation of the real option.

However, Denton Liu contributed a patch series in
https://public-inbox.org/git/cover.1553354374.git.liu.denton@gmail.com/
that introduces a new `git rebase` option called `--keep-base`, which
makes this previously unique abbreviation non-unique.

Whether this patch series is accepted or not, it is actually a bad
practice to use abbreviated options in our test suite, because of the
issue that those unique option names are not guaranteed to stay unique
in the future.

So let's just not use abbreviated options in the test suite.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5407-post-rewrite-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index a4a5903cba..7344253bfb 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -131,7 +131,7 @@ test_expect_success 'git rebase -m --skip' '
 test_expect_success 'git rebase with implicit use of interactive backend' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --keep --onto A B &&
+	test_must_fail git rebase --keep-empty --onto A B &&
 	echo C > foo &&
 	git add foo &&
 	git rebase --continue &&
@@ -146,7 +146,7 @@ test_expect_success 'git rebase with implicit use of interactive backend' '
 test_expect_success 'git rebase --skip with implicit use of interactive backend' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --keep --onto A B &&
+	test_must_fail git rebase --keep-empty --onto A B &&
 	test_must_fail git rebase --skip &&
 	echo D > foo &&
 	git add foo &&
-- 
gitgitgadget

[PATCH 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This script used abbreviated options, which is unnecessarily fragile.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7810-grep.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 43aa4161cf..2e1bb61b41 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -119,33 +119,33 @@ do
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep -w $L (with --column, --invert)" '
+	test_expect_success "grep -w $L (with --column, --invert-match)" '
 		{
 			echo ${HC}file:1:foo mmap bar
 			echo ${HC}file:1:foo_mmap bar
 			echo ${HC}file:1:foo_mmap bar mmap
 			echo ${HC}file:1:foo mmap bar_mmap
 		} >expected &&
-		git grep --column --invert -w -e baz $H -- file >actual &&
+		git grep --column --invert-match -w -e baz $H -- file >actual &&
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep $L (with --column, --invert, extended OR)" '
+	test_expect_success "grep $L (with --column, --invert-match, extended OR)" '
 		{
 			echo ${HC}hello_world:6:HeLLo_world
 		} >expected &&
-		git grep --column --invert -e ll --or --not -e _ $H -- hello_world \
+		git grep --column --invert-match -e ll --or --not -e _ $H -- hello_world \
 			>actual &&
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep $L (with --column, --invert, extended AND)" '
+	test_expect_success "grep $L (with --column, --invert-match, extended AND)" '
 		{
 			echo ${HC}hello_world:3:Hello world
 			echo ${HC}hello_world:3:Hello_world
 			echo ${HC}hello_world:6:HeLLo_world
 		} >expected &&
-		git grep --column --invert --not -e _ --and --not -e ll $H -- hello_world \
+		git grep --column --invert-match --not -e _ --and --not -e ll $H -- hello_world \
 			>actual &&
 		test_cmp expected actual
 	'
@@ -1010,7 +1010,7 @@ test_expect_success 'outside of git repository' '
 			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git grep --no-index --no-exclude o >../actual.full &&
+		git grep --no-index --no-exclude-standard o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '
@@ -1051,7 +1051,7 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
 			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git -c grep.fallbackToNoIndex grep --no-exclude o >../actual.full &&
+		git -c grep.fallbackToNoIndex grep --no-exclude-standard o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '
-- 
gitgitgadget

[PATCH 2/8] tests (rebase): spell out the `--force-rebase` option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In quite a few test cases, we were sloppy and used the abbreviation
`--force`, but we really should be precise in what we want to test.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3415-rebase-autosquash.sh | 2 +-
 t/t3430-rebase-merges.sh     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 13f5688135..22d218698e 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -277,7 +277,7 @@ test_expect_success 'autosquash with empty custom instructionFormat' '
 	(
 		set_cat_todo_editor &&
 		test_must_fail git -c rebase.instructionFormat= \
-			rebase --autosquash  --force -i HEAD^ >actual &&
+			rebase --autosquash  --force-rebase -i HEAD^ >actual &&
 		git log -1 --format="pick %h %s" >expect &&
 		test_cmp expect actual
 	)
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 4c69255ee6..42ba5b9f09 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -271,7 +271,7 @@ test_expect_success 'root commits' '
 	EOF
 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
 	test_tick &&
-	git rebase -i --force --root -r &&
+	git rebase -i --force-rebase --root -r &&
 	test "Parsnip" = "$(git show -s --format=%an HEAD^)" &&
 	test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
 	test $(git rev-parse second-root:second-root.t) = \
@@ -364,7 +364,7 @@ test_expect_success 'octopus merges' '
 	test_cmp_rev HEAD $before &&
 
 	test_tick &&
-	git rebase -i --force -r HEAD^^ &&
+	git rebase -i --force-rebase -r HEAD^^ &&
 	test "Hank" = "$(git show -s --format=%an HEAD)" &&
 	test "$before" != $(git rev-parse HEAD) &&
 	test_cmp_graph HEAD^^.. <<-\EOF
-- 
gitgitgadget

[PATCH 4/8] t5531: avoid using an abbreviated option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It was probably just an oversight: the `--recurse-submodules` option
puts the term "submodules" in the plural form, not the singular one.

To avoid future problems in case that another option is introduced that
starts with the prefix `--recurse-submodule`, let's just fix this.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5531-deep-submodule-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index e2c37fd978..4ad059e6be 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -363,7 +363,7 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs
 		) &&
 		git add b &&
 		git commit -m "added submodule" &&
-		git push --recurse-submodule=check origin master
+		git push --recurse-submodules=check origin master
 	)
 '
 
-- 
gitgitgadget

[PATCH 5/8] tests (push): do not abbreviate the `--follow-tags` option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We really want to spell out the option in the full form, to avoid any
ambiguity that might be introduced by future patches.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5516-fetch-push.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893..db0b1db458 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1370,7 +1370,7 @@ test_expect_success 'push does not follow tags by default' '
 	test_cmp expect actual
 '
 
-test_expect_success 'push --follow-tag only pushes relevant tags' '
+test_expect_success 'push --follow-tags only pushes relevant tags' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
 	git init src &&
@@ -1384,7 +1384,7 @@ test_expect_success 'push --follow-tag only pushes relevant tags' '
 		git tag -m "future" future &&
 		git checkout master &&
 		git for-each-ref refs/heads/master refs/tags/tag >../expect &&
-		git push --follow-tag ../dst master
+		git push --follow-tags ../dst master
 	) &&
 	(
 		cd dst &&
-- 
gitgitgadget

[PATCH 6/8] tests (status): spell out the `--find-renames` option in full

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

To avoid future ambiguities, we really want to use full option names in
the test suite. `t7525-status-rename.sh` used an abbreviated form of the
`--find-renames` option, though, so let's change that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7525-status-rename.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh
index ef8b1b3078..a62736dce0 100755
--- a/t/t7525-status-rename.sh
+++ b/t/t7525-status-rename.sh
@@ -84,7 +84,7 @@ test_expect_success 'status score=100%' '
 	test_i18ngrep "deleted:" actual &&
 	test_i18ngrep "new file:" actual &&
 
-	git status --find-rename=100% >actual &&
+	git status --find-renames=100% >actual &&
 	test_i18ngrep "deleted:" actual &&
 	test_i18ngrep "new file:" actual
 '
@@ -93,11 +93,11 @@ test_expect_success 'status score=01%' '
 	git status -M=01% >actual &&
 	test_i18ngrep "renamed:" actual &&
 
-	git status --find-rename=01% >actual &&
+	git status --find-renames=01% >actual &&
 	test_i18ngrep "renamed:" actual
 '
 
-test_expect_success 'copies not overridden by find-rename' '
+test_expect_success 'copies not overridden by find-renames' '
 	cp renamed copy &&
 	git add copy &&
 
@@ -105,7 +105,7 @@ test_expect_success 'copies not overridden by find-rename' '
 	test_i18ngrep "copied:" actual &&
 	test_i18ngrep "renamed:" actual &&
 
-	git -c status.renames=copies status --find-rename=01% >actual &&
+	git -c status.renames=copies status --find-renames=01% >actual &&
 	test_i18ngrep "copied:" actual &&
 	test_i18ngrep "renamed:" actual
 '
-- 
gitgitgadget

[PATCH 8/8] tests: disallow the use of abbreviated options (by default)

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Git's command-line parsers support uniquely abbreviated options, e.g.
`git init --ba` would automatically expand `--ba` to `--bare`.

This is a very convenient feature in every day life for Git users, in
particular when tab completion is not available.

However, it is not a good idea to rely on that in Git's test suite, as
something that is a unique abbreviation of a command line option today
might no longer be a unique abbreviation tomorrow.

For example, if a future contribution added a new mode
`git init --babyproofing` and a previously-introduced test case used the
fact that `git init --ba` expaneded to `git init --bare`, that future
contribution would now have to touch seemingly unrelated tests just to
keep the test suite from failing.

So let's disallow abbreviated options in the test suite by default.

Note: for ease of implementation, this patch really only touches the
`parse-options` machinery: more and more hand-rolled option parsers are
converted to use that internal API, and more and more scripts are
converted to built-ins (naturally using the parse-options API, too), so
in practice this catches most issues, and is definitely the biggest bang
for the buck.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 parse-options.c          | 9 +++++++++
 t/README                 | 4 ++++
 t/t0040-parse-options.sh | 7 ++++++-
 t/test-lib.sh            | 6 ++++++
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index cec74522e5..acc3a93660 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -6,6 +6,8 @@
 #include "color.h"
 #include "utf8.h"
 
+static int disallow_abbreviated_options;
+
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
+	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+		die("disallowed abbreviated or ambiguous option '%.*s'",
+		    (int)(arg_end - arg), arg);
+
 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),
@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
+	disallow_abbreviated_options =
+		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
+
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
diff --git a/t/README b/t/README
index 656288edce..9ed3051a1c 100644
--- a/t/README
+++ b/t/README
@@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
 fetch-pack to not request sideband-all (even if the server advertises
 sideband-all).
 
+GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
+the default when running tests), errors out when an abbreviated option
+is used.
+
 Naming Tests
 ------------
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index b8f366c442..5f6a16336d 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -203,20 +203,24 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 test-tool parse-options --strin 123
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 562c57e685..e550009411 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -57,6 +57,12 @@ fi
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH
 
+# Disallow the use of abbreviated options in the test suite by default
+test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+}
+
 ################################################################
 # It appears that people try to run tests without building...
 "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
-- 
gitgitgadget

[PATCH 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

To use the singular form of a word, when the option wants the plural
form (and quietly expands it because it thinks it was abbreviated), is
an easy mistake to make, and t5317 contains almost two dozen of them.

However, using abbreviated options in tests is a bit fragile, so we will
disallow use of abbreviated options in our test suite.

In preparation for this change, let's fix
`t5317-pack-objects-filter-objects.sh`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
index 24541ea137..4c0201c34b 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -25,7 +25,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../all.pack &&
@@ -39,7 +39,7 @@ test_expect_success 'verify blob count in normal packfile' '
 '
 
 test_expect_success 'verify blob:none packfile has no blobs' '
-	git -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=blob:none >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../filter.pack &&
@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' '
 	git -C r5 commit -m "foo" &&
 	del=$(git -C r5 rev-parse HEAD^{tree} | sed "s|..|&/|") &&
 	rm r5/.git/objects/$del &&
-	test_must_fail git -C r5 pack-objects --rev --stdout 2>bad_tree <<-EOF &&
+	test_must_fail git -C r5 pack-objects --revs --stdout 2>bad_tree <<-EOF &&
 	HEAD
 	EOF
 	grep "bad tree object" bad_tree
@@ -88,7 +88,7 @@ test_expect_success 'setup for tests of tree:0' '
 '
 
 test_expect_success 'verify tree:0 packfile has no blobs or trees' '
-	git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../commitsonly.pack &&
@@ -98,7 +98,7 @@ test_expect_success 'verify tree:0 packfile has no blobs or trees' '
 
 test_expect_success 'grab tree directly when using tree:0' '
 	# We should get the tree specified directly but not its blobs or subtrees.
-	git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
 	HEAD:
 	EOF
 	git -C r1 index-pack ../commitsonly.pack &&
@@ -128,7 +128,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../all.pack &&
@@ -142,7 +142,7 @@ test_expect_success 'verify blob count in normal packfile' '
 '
 
 test_expect_success 'verify blob:limit=500 omits all blobs' '
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -157,7 +157,7 @@ test_expect_success 'verify blob:limit=500 omits all blobs' '
 '
 
 test_expect_success 'verify blob:limit=1000' '
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -176,7 +176,7 @@ test_expect_success 'verify blob:limit=1001' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -194,7 +194,7 @@ test_expect_success 'verify blob:limit=10001' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -212,7 +212,7 @@ test_expect_success 'verify blob:limit=1k' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -230,7 +230,7 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
 	HEAD
 	$(git -C r2 rev-parse HEAD:large.10000)
 	EOF
@@ -249,7 +249,7 @@ test_expect_success 'verify blob:limit=1m' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -302,7 +302,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../all.pack &&
@@ -320,7 +320,7 @@ test_expect_success 'verify sparse:path=pattern1' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../filter.pack &&
@@ -352,7 +352,7 @@ test_expect_success 'verify sparse:path=pattern2' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../filter.pack &&
@@ -404,7 +404,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r4 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../all.pack &&
@@ -423,7 +423,7 @@ test_expect_success 'verify sparse:oid=OID' '
 	sort >expected &&
 
 	oid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) &&
-	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../filter.pack &&
@@ -441,7 +441,7 @@ test_expect_success 'verify sparse:oid=oid-ish' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../filter.pack &&
@@ -470,19 +470,19 @@ test_expect_success 'setup r1 - delete loose blobs' '
 '
 
 test_expect_success 'verify pack-objects fails w/ missing objects' '
-	test_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF
+	test_must_fail git -C r1 pack-objects --revs --stdout >miss.pack <<-EOF
 	HEAD
 	EOF
 '
 
 test_expect_success 'verify pack-objects fails w/ --missing=error' '
-	test_must_fail git -C r1 pack-objects --rev --stdout --missing=error >miss.pack <<-EOF
+	test_must_fail git -C r1 pack-objects --revs --stdout --missing=error >miss.pack <<-EOF
 	HEAD
 	EOF
 '
 
 test_expect_success 'verify pack-objects w/ --missing=allow-any' '
-	git -C r1 pack-objects --rev --stdout --missing=allow-any >miss.pack <<-EOF
+	git -C r1 pack-objects --revs --stdout --missing=allow-any >miss.pack <<-EOF
 	HEAD
 	EOF
 '
-- 
gitgitgadget

Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)

[Denton Liu]

Hi Johannes,

Thanks for catching this. Perhaps I should've been more diligent and ran
the entire test suite before submitting but I was running low on
batteries only ran the rebase-related tests.

On Mon, Mar 25, 2019 at 11:14:23AM -0700, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Git's command-line parsers support uniquely abbreviated options, e.g.
> `git init --ba` would automatically expand `--ba` to `--bare`.
> 
> This is a very convenient feature in every day life for Git users, in
> particular when tab completion is not available.
> 
> However, it is not a good idea to rely on that in Git's test suite, as
> something that is a unique abbreviation of a command line option today
> might no longer be a unique abbreviation tomorrow.
> 
> For example, if a future contribution added a new mode
> `git init --babyproofing` and a previously-introduced test case used the
> fact that `git init --ba` expaneded to `git init --bare`, that future

s/expaneded/expanded/

> contribution would now have to touch seemingly unrelated tests just to
> keep the test suite from failing.
> 
> So let's disallow abbreviated options in the test suite by default.
> 
> Note: for ease of implementation, this patch really only touches the
> `parse-options` machinery: more and more hand-rolled option parsers are
> converted to use that internal API, and more and more scripts are
> converted to built-ins (naturally using the parse-options API, too), so
> in practice this catches most issues, and is definitely the biggest bang
> for the buck.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  parse-options.c          | 9 +++++++++
>  t/README                 | 4 ++++
>  t/t0040-parse-options.sh | 7 ++++++-
>  t/test-lib.sh            | 6 ++++++
>  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index cec74522e5..acc3a93660 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -6,6 +6,8 @@
>  #include "color.h"
>  #include "utf8.h"
>  
> +static int disallow_abbreviated_options;
> +
>  #define OPT_SHORT 1
>  #define OPT_UNSET 2
>  
> @@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(
>  		return get_value(p, options, all_opts, flags ^ opt_flags);
>  	}
>  
> +	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
> +		die("disallowed abbreviated or ambiguous option '%.*s'",
> +		    (int)(arg_end - arg), arg);
> +
>  	if (ambiguous_option) {
>  		error(_("ambiguous option: %s "
>  			"(could be --%s%s or --%s%s)"),
> @@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,
>  {
>  	struct parse_opt_ctx_t ctx;
>  
> +	disallow_abbreviated_options =
> +		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
> +
>  	parse_options_start(&ctx, argc, argv, prefix, options, flags);
>  	switch (parse_options_step(&ctx, options, usagestr)) {
>  	case PARSE_OPT_HELP:
> diff --git a/t/README b/t/README
> index 656288edce..9ed3051a1c 100644
> --- a/t/README
> +++ b/t/README
> @@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
>  fetch-pack to not request sideband-all (even if the server advertises
>  sideband-all).
>  
> +GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
> +the default when running tests), errors out when an abbreviated option
> +is used.
> +
>  Naming Tests
>  ------------
>  
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index b8f366c442..5f6a16336d 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -203,20 +203,24 @@ file: (not set)
>  EOF
>  
>  test_expect_success 'unambiguously abbreviated option' '
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
>  	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
>  	test_must_be_empty output.err &&
>  	test_cmp expect output
>  '
>  
>  test_expect_success 'unambiguously abbreviated option with "="' '
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
>  	test-tool parse-options --expect="integer: 2" --int=2
>  '
>  
>  test_expect_success 'ambiguously abbreviated option' '
> -	test_expect_code 129 test-tool parse-options --strin 123
> +	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
> +	test-tool parse-options --strin 123
>  '
>  
>  test_expect_success 'non ambiguous option (after two options it abbreviates)' '
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
>  	test-tool parse-options --expect="string: 123" --st 123
>  '
>  
> @@ -325,6 +329,7 @@ file: (not set)
>  EOF
>  
>  test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
>  	test-tool parse-options --no-ambig >output 2>output.err &&
>  	test_must_be_empty output.err &&
>  	test_cmp expect output

Would it make sense to include a test case to ensure that
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?

Thanks,

Denton

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 562c57e685..e550009411 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -57,6 +57,12 @@ fi
>  . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
>  export PERL_PATH SHELL_PATH
>  
> +# Disallow the use of abbreviated options in the test suite by default
> +test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
> +	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
> +}
> +
>  ################################################################
>  # It appears that people try to run tests without building...
>  "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
> -- 
> gitgitgadget

Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 25 2019, Johannes Schindelin via GitGitGadget wrote:

> +# Disallow the use of abbreviated options in the test suite by default
> +test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
> +	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
> +}
> +

Just using the if test ...\nthen\nfi long-form would be consistent with
our usual style & the rest of this file.

[PATCH 0/2] allow for configuring option abbreviation + fix

[Ævar Arnfjörð Bjarmason]

This is based on Johannes's just-submitted
https://public-inbox.org/git/pull.167.git.gitgitgadget@gmail.com/

Some of this in my 1/2 could obviously be shortened if the
"GIT_TEST_ABBREVIATED_OPTIONS" name was used by default, but I'll
leave it up to discussion whether it's worth it to go for the route I
took here, and if so to rebase the whole thing or not.

While I'm at it fix a bug I noticed a while ago in 2/2 related to the
option abbreviation being silly and tripping over itself over an
aliased option.

Ævar Arnfjörð Bjarmason (2):
  parse-options: allow for configuring option abbreviation
  parse-options: don't emit "ambiguous option" for aliases

 Documentation/config/core.txt | 12 ++++++++++++
 builtin/clone.c               |  4 ++--
 parse-options.c               | 22 +++++++++++++++++-----
 parse-options.h               |  2 ++
 t/README                      |  4 ++--
 t/t0040-parse-options.sh      | 27 ++++++++++++++++++++++-----
 t/test-lib.sh                 |  6 +++---
 7 files changed, 60 insertions(+), 17 deletions(-)

-- 
2.21.0.360.g471c308f928

[PATCH 1/2] parse-options: allow for configuring option abbreviation

[Ævar Arnfjörð Bjarmason]

Add a new core.abbreviatedOptions which corresponds to the newly added
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS test mode. This allows
e.g. script authors to enact the same sort of paranoia about option
abbreviation as our own test suite now uses by default.

I'm renaming GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS to
GIT_TEST_ABBREVIATED_OPTIONS for consistency with the new
configuration variable, and I've picked the shorter
core.abbreviatedOptions instead of core.disallowAbbreviatedOptions
because it's easier to reason about a variable that's true by default
than about one that has a negation in its name.

While I'm at it, factor out the check for the environment variable
into a parse_options_config() which similar to the code added in
62c23938fa ("tests: add a special setup where rebase.useBuiltin is
off", 2018-11-14) will first check the environment variable, and then
the configuration.

Using git_env_bool() means it's a bit of a pain to distinguish between
the case where GIT_TEST_ABBREVIATED_OPTIONS is set to "true" or
"false" v.s. being empty, hence the
test_when_finished/export/sane_unset dance in the test that's being
added, but I think that's more sane than introducing a "bool but not
in the same way as the rest" GIT_TEST_* variable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config/core.txt | 12 ++++++++++++
 parse-options.c               | 19 +++++++++++++++----
 t/README                      |  4 ++--
 t/t0040-parse-options.sh      | 22 +++++++++++++++++-----
 t/test-lib.sh                 |  6 +++---
 5 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
index 7e9b6c8f4c..5a42ec3940 100644
--- a/Documentation/config/core.txt
+++ b/Documentation/config/core.txt
@@ -1,3 +1,15 @@
+core.abbreviatedOptions::
+	Defaults to `true` which allows options to be abbreviated as
+	long as they aren't ambiguous, e.g. for linkgit:git-init[1]
+	the `--bare` option can be abbreviated as `--bar`, `--ba` or
+	even `--b` since no other option starts with those
+	prefixes. However, if such an option were added in the future
+	any use of these abbreviations would break.
++
+By setting this to false (e.g. in scripts) you can defend against such
+future breakages by enforcing that options must always be fully
+provided.
+
 core.fileMode::
 	Tells Git if the executable bit of files in the working tree
 	is to be honored.
diff --git a/parse-options.c b/parse-options.c
index acc3a93660..d6a291f705 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -6,7 +6,7 @@
 #include "color.h"
 #include "utf8.h"
 
-static int disallow_abbreviated_options;
+static int abbreviated_options = 1;
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -346,7 +346,7 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
-	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+	if (!abbreviated_options && (ambiguous_option || abbrev_option))
 		die("disallowed abbreviated or ambiguous option '%.*s'",
 		    (int)(arg_end - arg), arg);
 
@@ -456,6 +456,17 @@ static void parse_options_check(const struct option *opts)
 		exit(128);
 }
 
+static void parse_options_config(void)
+{
+	int env = git_env_bool("GIT_TEST_ABBREVIATED_OPTIONS", -1);
+	if (env != -1) {
+		abbreviated_options = env;
+		return;
+	}
+	git_config_get_bool("core.abbreviatedoptions", &abbreviated_options);
+	return;
+}
+
 void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 int argc, const char **argv, const char *prefix,
 			 const struct option *options, int flags)
@@ -480,6 +491,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	    (flags & PARSE_OPT_KEEP_ARGV0))
 		BUG("Can't keep argv0 if you don't have it");
 	parse_options_check(options);
+	parse_options_config();
 }
 
 static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
@@ -714,9 +726,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
-	disallow_abbreviated_options =
-		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
 
+	parse_options_config();
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
diff --git a/t/README b/t/README
index 9ed3051a1c..1d628baf31 100644
--- a/t/README
+++ b/t/README
@@ -399,9 +399,9 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
 fetch-pack to not request sideband-all (even if the server advertises
 sideband-all).
 
-GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
+GIT_TEST_ABBREVIATED_OPTIONS=<boolean>, when false (which is
 the default when running tests), errors out when an abbreviated option
-is used.
+is used. Overrides the core.abbreviatedOptions setting.
 
 Naming Tests
 ------------
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 5f6a16336d..19685d1582 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -203,27 +203,39 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test_expect_code 129 env GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'abbreviated options configured with core.abbreviatedOptions' '
+	test_when_finished "
+		rm -rf A B C &&
+		GIT_TEST_ABBREVIATED_OPTIONS=$GIT_TEST_ABBREVIATED_OPTIONS &&
+		export GIT_TEST_ABBREVIATED_OPTIONS
+	" &&
+	git init --bare A &&
+	test_must_fail git init --ba B &&
+	sane_unset GIT_TEST_ABBREVIATED_OPTIONS &&
+	git -c core.abbreviatedOptions=true init --ba C
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF
@@ -329,7 +341,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e550009411..4b0dd8a1a9 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -58,9 +58,9 @@ fi
 export PERL_PATH SHELL_PATH
 
 # Disallow the use of abbreviated options in the test suite by default
-test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
-	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+test -n "$GIT_TEST_ABBREVIATED_OPTIONS" || {
+	GIT_TEST_ABBREVIATED_OPTIONS=false
+	export GIT_TEST_ABBREVIATED_OPTIONS
 }
 
 ################################################################
-- 
2.21.0.360.g471c308f928

[PATCH 2/2] parse-options: don't emit "ambiguous option" for aliases

[Ævar Arnfjörð Bjarmason]

Change the option parsing machinery so that e.g. "clone --recurs ..."
doesn't error out because "clone" understands both "--recursive" and
"--recurse-submodules" to mean the same thing.

Initially "clone" just understood --recursive until the
--recurses-submodules alias was added in ccdd3da652 ("clone: Add the
--recurse-submodules option as alias for --recursive",
2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to
optionally take a pathspec", 2017-03-17) the longer form has been
promoted to the default.

But due to the way the options parsing machinery works this resulted
in the rather absurd situation of:

    $ git clone --recurs [...]
    error: ambiguous option: recurs (could be --recursive or --recurse-submodules)

Let's re-use the PARSE_OPT_NOCOMPLETE flag to mean "this option
doesn't contribute to abbreviation ambiguity". I was going to add a
new PARSE_OPT_NOABBREV flag, but it makes sense just to re-use
PARSE_OPT_NOCOMPLETE.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/clone.c          | 4 ++--
 parse-options.c          | 3 ++-
 parse-options.h          | 2 ++
 t/t0040-parse-options.sh | 5 +++++
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 50bde99618..4dc26969a7 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {
 		    N_("setup as shared repository")),
 	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
+	  recurse_submodules_cb, (intptr_t)"." },
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
diff --git a/parse-options.c b/parse-options.c
index d6a291f705..84f3a2996f 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(
 		if (!rest) {
 			/* abbreviated? */
 			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
-			    !strncmp(long_name, arg, arg_end - arg)) {
+			    !strncmp(long_name, arg, arg_end - arg) &&
+			    !(options->flags & PARSE_OPT_NOCOMPLETE)) {
 is_abbreviated:
 				if (abbrev_option) {
 					/*
diff --git a/parse-options.h b/parse-options.h
index 74cce4e7fc..9362a397ae 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *				Useful for options with multiple parameters.
  *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
  *			   by git-completion.bash. This option suppresses that.
+ *			   Will also skip this option when abbreviation is
+ *			   considered. See core.abbreviatedOptions.
  *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
  *			 complete an option as --name= not --name even if
  *			 the option takes optional argument.
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 19685d1582..c1ea50aa85 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -236,6 +236,11 @@ test_expect_success 'abbreviated options configured with core.abbreviatedOptions
 	git -c core.abbreviatedOptions=true init --ba C
 '
 
+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '
+	test_when_finished "rm -rf A" &&
+	GIT_TEST_ABBREVIATED_OPTIONS=true git clone --recurs . A
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF
-- 
2.21.0.360.g471c308f928

Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 25 2019, Denton Liu wrote:

> Hi Johannes,
>
> Thanks for catching this. Perhaps I should've been more diligent and ran
> the entire test suite before submitting but I was running low on
> batteries only ran the rebase-related tests.
>
> On Mon, Mar 25, 2019 at 11:14:23AM -0700, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>> [...]
>> @@ -325,6 +329,7 @@ file: (not set)
>>  EOF
>>
>>  test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
>> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
>>  	test-tool parse-options --no-ambig >output 2>output.err &&
>>  	test_must_be_empty output.err &&
>>  	test_cmp expect output
>
> Would it make sense to include a test case to ensure that
> GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?

To elaborate on this since one might wonder "but aren't these those
tests?". I think you mean if we shouldn't have a "test_must_fail" test
there that asserts that parsing the options will die with the default of
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true. Yeah, that makes sense, it's
currently a blind spot that we just assume will keep working.

Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

[Eric Sunshine]

On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> @@ -1,3 +1,15 @@
> +core.abbreviatedOptions::
> +       Defaults to `true` which allows options to be abbreviated as
> +       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
> +       the `--bare` option can be abbreviated as `--bar`, `--ba` or
> +       even `--b` since no other option starts with those
> +       prefixes. However, if such an option were added in the future
> +       any use of these abbreviations would break.
> ++
> +By setting this to false (e.g. in scripts) you can defend against such
> +future breakages by enforcing that options must always be fully
> +provided.

I don't get why having a configuration option is better for defending
scripts against this problem than a simple environment variable. It
seems easier for the script prologue to contain:

    GIT_TEST_ABBREVIATED_OPTIONS=false
    export GIT_TEST_ABBREVIATED_OPTIONS

than for it to muck about with git-config or use "git -c
core.abbreviatedOptions=false ..." everywhere. The commit message
doesn't do a good enough job of justifying the configuration option
over the environment variable.

Also, if this is now intended to be more general (aiding script
writers) than just being for our test suite, then dropping "TEST" from
the name seems warranted:

    GIT_ABBREVIATED_OPTIONS

Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 25 2019, Eric Sunshine wrote:

> On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
>> @@ -1,3 +1,15 @@
>> +core.abbreviatedOptions::
>> +       Defaults to `true` which allows options to be abbreviated as
>> +       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
>> +       the `--bare` option can be abbreviated as `--bar`, `--ba` or
>> +       even `--b` since no other option starts with those
>> +       prefixes. However, if such an option were added in the future
>> +       any use of these abbreviations would break.
>> ++
>> +By setting this to false (e.g. in scripts) you can defend against such
>> +future breakages by enforcing that options must always be fully
>> +provided.
>
> I don't get why having a configuration option is better for defending
> scripts against this problem than a simple environment variable. It
> seems easier for the script prologue to contain:
>
>     GIT_TEST_ABBREVIATED_OPTIONS=false
>     export GIT_TEST_ABBREVIATED_OPTIONS
>
> than for it to muck about with git-config or use "git -c
> core.abbreviatedOptions=false ..." everywhere. The commit message
> doesn't do a good enough job of justifying the configuration option
> over the environment variable.
>
> Also, if this is now intended to be more general (aiding script
> writers) than just being for our test suite, then dropping "TEST" from
> the name seems warranted:
>
>     GIT_ABBREVIATED_OPTIONS

If we want to make something user-configurable we tend to add config
variables. The GIT_TEST_* variables are only intended for our own test
suite, see t/README.

I don't mind documenting this, but it's a well-established pattern, so
if we're going to describe how this works/why use one or the other it
should probably be some other series to t/README and/or git-config.txt

We traditionally *only* expose this sort of thing to users via config,
and not via env variables.

The config system is more flexible in every way. You can set it
system-wide, user-wide, repo-wide etc., and if you want exactly the
scope of an env variable you can do that too, just start your script
with:

    # These "''" quotes are not a mistake, it needs to be like this
    export GIT_CONFIG_PARAMETERS="'core.abbreviatedOptions=false'"
    git <some-cmd> [...]

So the reason we have GIT_TEST_* is pretty much because we can't just
whitelist GIT_CONFIG_PARAMETERS for the test suite, and to make it
obvious what test modes we have available.

Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

[Duy Nguyen]

On Tue, Mar 26, 2019 at 5:48 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Mon, Mar 25 2019, Eric Sunshine wrote:
>
> > On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
> >> @@ -1,3 +1,15 @@
> >> +core.abbreviatedOptions::
> >> +       Defaults to `true` which allows options to be abbreviated as
> >> +       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
> >> +       the `--bare` option can be abbreviated as `--bar`, `--ba` or
> >> +       even `--b` since no other option starts with those
> >> +       prefixes. However, if such an option were added in the future
> >> +       any use of these abbreviations would break.
> >> ++
> >> +By setting this to false (e.g. in scripts) you can defend against such
> >> +future breakages by enforcing that options must always be fully
> >> +provided.
> >
> > I don't get why having a configuration option is better for defending
> > scripts against this problem than a simple environment variable. It
> > seems easier for the script prologue to contain:
> >
> >     GIT_TEST_ABBREVIATED_OPTIONS=false
> >     export GIT_TEST_ABBREVIATED_OPTIONS
> >
> > than for it to muck about with git-config or use "git -c
> > core.abbreviatedOptions=false ..." everywhere. The commit message
> > doesn't do a good enough job of justifying the configuration option
> > over the environment variable.
> >
> > Also, if this is now intended to be more general (aiding script
> > writers) than just being for our test suite, then dropping "TEST" from
> > the name seems warranted:
> >
> >     GIT_ABBREVIATED_OPTIONS
>
> If we want to make something user-configurable we tend to add config
> variables. The GIT_TEST_* variables are only intended for our own test
> suite, see t/README.
>
> I don't mind documenting this, but it's a well-established pattern, so
> if we're going to describe how this works/why use one or the other it
> should probably be some other series to t/README and/or git-config.txt
>
> We traditionally *only* expose this sort of thing to users via config,
> and not via env variables.

If this is mostly useful for scripts then I agree with Eric an
environment variable is the way to go. A configuration variable does
not make it more convenient.

And no we don't only export via config. There are a bunch of public
env variables in git.txt. "core" namespace is already very crowded. If
this one is only rarely used, I'd rather not add a new config
variable.
-- 
Duy

Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 26 2019, Duy Nguyen wrote:

> On Tue, Mar 26, 2019 at 5:48 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Mon, Mar 25 2019, Eric Sunshine wrote:
>>
>> > On Mon, Mar 25, 2019 at 4:23 PM Ævar Arnfjörð Bjarmason
>> > <avarab@gmail.com> wrote:
>> >> diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt
>> >> @@ -1,3 +1,15 @@
>> >> +core.abbreviatedOptions::
>> >> +       Defaults to `true` which allows options to be abbreviated as
>> >> +       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
>> >> +       the `--bare` option can be abbreviated as `--bar`, `--ba` or
>> >> +       even `--b` since no other option starts with those
>> >> +       prefixes. However, if such an option were added in the future
>> >> +       any use of these abbreviations would break.
>> >> ++
>> >> +By setting this to false (e.g. in scripts) you can defend against such
>> >> +future breakages by enforcing that options must always be fully
>> >> +provided.
>> >
>> > I don't get why having a configuration option is better for defending
>> > scripts against this problem than a simple environment variable. It
>> > seems easier for the script prologue to contain:
>> >
>> >     GIT_TEST_ABBREVIATED_OPTIONS=false
>> >     export GIT_TEST_ABBREVIATED_OPTIONS
>> >
>> > than for it to muck about with git-config or use "git -c
>> > core.abbreviatedOptions=false ..." everywhere. The commit message
>> > doesn't do a good enough job of justifying the configuration option
>> > over the environment variable.
>> >
>> > Also, if this is now intended to be more general (aiding script
>> > writers) than just being for our test suite, then dropping "TEST" from
>> > the name seems warranted:
>> >
>> >     GIT_ABBREVIATED_OPTIONS
>>
>> If we want to make something user-configurable we tend to add config
>> variables. The GIT_TEST_* variables are only intended for our own test
>> suite, see t/README.
>>
>> I don't mind documenting this, but it's a well-established pattern, so
>> if we're going to describe how this works/why use one or the other it
>> should probably be some other series to t/README and/or git-config.txt
>>
>> We traditionally *only* expose this sort of thing to users via config,
>> and not via env variables.

FWIW I replied so quickly to this with patches since I had something WIP
to do this, it's annoyed me as a user in the past that I couldn't turn
this off, IIRC some upgrade of git broke my "bad" muscle memory /
scripts.

> If this is mostly useful for scripts then I agree with Eric an
> environment variable is the way to go. A configuration variable does
> not make it more convenient.

I think both of you might be assuming that when you want to configure
something it's as easy to tweak every run time environment (set an env
var) as it is to set a global config. See the trace2.* config discussion
for similar use-cases.

> And no we don't only export via config. There are a bunch of public
> env variables in git.txt. "core" namespace is already very crowded. If
> this one is only rarely used, I'd rather not add a new config
> variable.

I don't see how a new "abbreviatedOptions" is plausibly going to crowd
out anything else, sounds pretty unambiguous to me.

By my count out of the the existing GIT_* variables in git.txt around
1/3 are already configurable via config, another 1/3 (all the GIT_TRACE
stuff) is something we've wanted to have configurable in the past (but
nobody's gotten around to writing patches for).

I think it's fair to say that when we normally add user-configurable
stuff we do it as config, not as new env vars.

Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

[Duy Nguyen]

On Tue, Mar 26, 2019 at 1:29 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> I don't see how a new "abbreviatedOptions" is plausibly going to crowd
> out anything else, sounds pretty unambiguous to me.

By crowded I mean a lot of configuration variables under "core"

> By my count out of the the existing GIT_* variables in git.txt around
> 1/3 are already configurable via config, another 1/3 (all the GIT_TRACE
> stuff) is something we've wanted to have configurable in the past (but
> nobody's gotten around to writing patches for).
>
> I think it's fair to say that when we normally add user-configurable
> stuff we do it as config, not as new env vars.

I disagree that not every configuration knob has to be a configuration
variable, especially when core.* more or less becomes a dictionary
that you can't really read anymore (unless you know the key to look
for). But I see you're dead set on adding config vars. Go ahead.
-- 
Duy

Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 26 2019, Duy Nguyen wrote:

> On Tue, Mar 26, 2019 at 1:29 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> I don't see how a new "abbreviatedOptions" is plausibly going to crowd
>> out anything else, sounds pretty unambiguous to me.
>
> By crowded I mean a lot of configuration variables under "core"
>
>> By my count out of the the existing GIT_* variables in git.txt around
>> 1/3 are already configurable via config, another 1/3 (all the GIT_TRACE
>> stuff) is something we've wanted to have configurable in the past (but
>> nobody's gotten around to writing patches for).
>>
>> I think it's fair to say that when we normally add user-configurable
>> stuff we do it as config, not as new env vars.
>
> I disagree that not every configuration knob has to be a configuration
> variable, especially when core.* more or less becomes a dictionary
> that you can't really read anymore (unless you know the key to look
> for). But I see you're dead set on adding config vars. Go ahead.

I don't mean to pile on in this thread, and cards on the table: In terms
of stuff I can be bothered to write a patch for and stuff I *really*
want in git, this thing is maybe on a 2/10 out of a 1-10 scale.

I.e. I'd use it, and it would probably save my ass *somewhere* down the
line, the motivation was being bitten in the past, so once I spotted
Johannes's series I was reminded to clean it up & submit it. But I
wouldn't care enough to carry a forked git with this (unlike some other
stuff).

I just wanted to elaborate *in general* on the question of "why config
and not env".

I don't think everything we find reason to add a knob for needs to be a
config variable. E.g. I can't think of a reason for why the
GIT_*_PATHSPECS variables should be. That seems like the sort of thing
you'd only tweak for a *specific* git invocation. I wouldn't oppose it
being config if someone had a compelling reason & patch, but can't think
of one myself.

What I wanted to get across in the above replies to you/Eric, but
probably did a bad job of is that separate & aside from the merits of
any given new config variable there's the problem of being a "zookeeper"
with large git installations.

I.e. you don't even pick the animals, or feed them, but if the zoo's on
fire or overflowing with poop it's your problem.

So I have some servers where I'm in charge of maintaining/upgrading git,
and it's on me if crap breaks in the aggregate, but I'm not writing
anyone's individual scripts for them, and the runtime envs for those
might be varied.

It's for those sorts of cases where having the flexibility of turning
something on either in the env or via system-wide or user-wide config is
handy. I.e. in this case I might say:

    "You can run your scripts, but you'll be in 'strict mode' and need
    to spell out your options, because I'm not getting out of bed if
    your random job using git breaks because you were lazy and did a
    'git init --ba'".

I don't think this is a use-case that's obvious, so it's worth
elaborating on it. I suspect a lot of devs/users on this list have
advanced git use-cases, but they're all ones where the git processes
they care about/need to maintain run as a child of their login shell, or
equivalent. In that case the config v.s. env var distinction mostly
doesn't matter.

Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> I don't get why having a configuration option is better for defending
> scripts against this problem than a simple environment variable. It
> seems easier for the script prologue to contain:
>
>     GIT_TEST_ABBREVIATED_OPTIONS=false
>     export GIT_TEST_ABBREVIATED_OPTIONS
>
> than for it to muck about with git-config or use "git -c
> core.abbreviatedOptions=false ..." everywhere. The commit message
> doesn't do a good enough job of justifying the configuration option
> over the environment variable.

Absolutely.

One thing that big brotherly types would find config attractive is
to install centrally managed /etc/gitconfig so that they can tell
the tracing machinery to log all git command invocations centrally;
with environment only system, it is not easy to arrange.

But this one under discussion is probably a use case that is at the
other end of the extreme from that.

Users (1) may use scripts written by third-parties, (2) may also
develop their own scripts, and (3) even may run Git commands
interactively.

It is a laudable goal to introduce a mechanism to "notice use of
abbreviated options that happen to be unique right now but may not
stay unique forever and warn against it".  But (2) among the above
three uses is the only use case that wants the protection, which
makes the configuration a poor fit for the purpose.

Re: [PATCH 0/8] Do not use abbreviated options in tests

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> We do not want to have tests that need to be changed by completely unrelated
> patch series, just because abbreviations that used to be unique are being
> made non-unique by said patch series.

Makes sense.

If we wanted to make sure that options in abbreviated form work
correctly, we should do so in dedicated parseopt-specific tests (and
we do in t0040, I think), not in tests for random other commands
that happens to feed options to them.

Will queue.

Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1433 bytes --]

Hi,


On Mon, 25 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

>
> On Mon, Mar 25 2019, Denton Liu wrote:
>
> > Hi Johannes,
> >
> > Thanks for catching this. Perhaps I should've been more diligent and ran
> > the entire test suite before submitting but I was running low on
> > batteries only ran the rebase-related tests.
> >
> > On Mon, Mar 25, 2019 at 11:14:23AM -0700, Johannes Schindelin via GitGitGadget wrote:
> >> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >> [...]
> >> @@ -325,6 +329,7 @@ file: (not set)
> >>  EOF
> >>
> >>  test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
> >> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
> >>  	test-tool parse-options --no-ambig >output 2>output.err &&
> >>  	test_must_be_empty output.err &&
> >>  	test_cmp expect output
> >
> > Would it make sense to include a test case to ensure that
> > GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?
>
> To elaborate on this since one might wonder "but aren't these those
> tests?". I think you mean if we shouldn't have a "test_must_fail" test
> there that asserts that parsing the options will die with the default of
> GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true. Yeah, that makes sense, it's
> currently a blind spot that we just assume will keep working.

Valid point. I added a test case in v2 (which I'll send out in a few
moments...)

Thanks,
Dscho

Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)

[Johannes Schindelin]

Hi Denton,

On Mon, 25 Mar 2019, Denton Liu wrote:

> Thanks for catching this. Perhaps I should've been more diligent and ran
> the entire test suite before submitting but I was running low on
> batteries only ran the rebase-related tests.

No worries.

> On Mon, Mar 25, 2019 at 11:14:23AM -0700, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > Git's command-line parsers support uniquely abbreviated options, e.g.
> > `git init --ba` would automatically expand `--ba` to `--bare`.
> >
> > This is a very convenient feature in every day life for Git users, in
> > particular when tab completion is not available.
> >
> > However, it is not a good idea to rely on that in Git's test suite, as
> > something that is a unique abbreviation of a command line option today
> > might no longer be a unique abbreviation tomorrow.
> >
> > For example, if a future contribution added a new mode
> > `git init --babyproofing` and a previously-introduced test case used the
> > fact that `git init --ba` expaneded to `git init --bare`, that future
>
> s/expaneded/expanded/

Thanks, fixed.

> > @@ -325,6 +329,7 @@ file: (not set)
> >  EOF
> >
> >  test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
> > +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
> >  	test-tool parse-options --no-ambig >output 2>output.err &&
> >  	test_must_be_empty output.err &&
> >  	test_cmp expect output
>
> Would it make sense to include a test case to ensure that
> GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is working properly?

Yep, absolutely.

Thank you,
Dscho

Re: [PATCH 8/8] tests: disallow the use of abbreviated options (by default)

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

Hi Ævar,

On Mon, 25 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

>
> On Mon, Mar 25 2019, Johannes Schindelin via GitGitGadget wrote:
>
> > +# Disallow the use of abbreviated options in the test suite by default
> > +test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
> > +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
> > +	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
> > +}
> > +
>
> Just using the if test ...\nthen\nfi long-form would be consistent with
> our usual style & the rest of this file.

Yep, and we also use the `${...:+isset}` dance, so I imitated that, too.

Thanks for reminding me that when in Rome, do as the Romans do,
Dscho

Re: [PATCH 1/2] parse-options: allow for configuring option abbreviation

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2101 bytes --]

Hi Junio,

On Mon, 1 Apr 2019, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > I don't get why having a configuration option is better for defending
> > scripts against this problem than a simple environment variable. It
> > seems easier for the script prologue to contain:
> >
> >     GIT_TEST_ABBREVIATED_OPTIONS=false
> >     export GIT_TEST_ABBREVIATED_OPTIONS
> >
> > than for it to muck about with git-config or use "git -c
> > core.abbreviatedOptions=false ..." everywhere. The commit message
> > doesn't do a good enough job of justifying the configuration option
> > over the environment variable.
>
> Absolutely.
>
> One thing that big brotherly types would find config attractive is
> to install centrally managed /etc/gitconfig so that they can tell
> the tracing machinery to log all git command invocations centrally;
> with environment only system, it is not easy to arrange.

I think that in this instance, we should use the fact that we know Ævar
well, and refrain from characterizing him as a Big Brotherly type.

From my reading, it looks like Ævar just wants to avoid being woken up for
a Live Site Incident that is caused by a violation of Postel's Law: be
accepting in your input, but stringent in your output. And in this case,
the scripts by their colleagues is the output that should be more
stringent, and enforcing the stringency via a system-wide config variable
is as legit as our instistence that `user.name` and `user.email` must be
provided if you want to create a commit.

And I would not have the faintest problem with adding that patch to
introduce the `core.*` setting to that end (which would be the right
section, too, even if there are already so many `core.*` settings).

Having said that, I get the strong impression that there is a rather
violent pushback against this (which I don't understand). Combined with
the fact that it would protect only against a tiny fraction of "git
upgrade problems", I'm getting more into the "well, then let's just not
bother" camp.

Ciao,
Dscho

[PATCH v2 0/8] Do not use abbreviated options in tests

[Johannes Schindelin via GitGitGadget]

We do not want to have tests that need to be changed by completely unrelated
patch series, just because abbreviations that used to be unique are being
made non-unique by said patch series.

I stumbled over this while investigating the test failures in
dl/rebase-i-keep-base
[https://dev.azure.com/gitgitgadget/git/_build/results?buildId=5482&view=ms.vss-test-web.build-test-results-tab]
: the new --keep-base option makes the --keep abbreviation of --keep-empty 
in t5407 non-unique, which causes the test suite to fail.

Changes since v1:

 * Fixed a typo ("expaneded") in the last commit message.
 * Added a regression test for the GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS 
   handling.
 * Adjusted the code in test-lib.sh that makes sure that this variable is
   set: it is now consistent with the way we do similar things in that file
   already.

Johannes Schindelin (8):
  tests (rebase): spell out the `--keep-empty` option
  tests (rebase): spell out the `--force-rebase` option
  t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`
  t5531: avoid using an abbreviated option
  tests (push): do not abbreviate the `--follow-tags` option
  tests (status): spell out the `--find-renames` option in full
  tests (pack-objects): use the full, unabbreviated `--revs` option
  tests: disallow the use of abbreviated options (by default)

 parse-options.c                        |  9 ++++++
 t/README                               |  4 +++
 t/t0040-parse-options.sh               | 14 +++++++-
 t/t3415-rebase-autosquash.sh           |  2 +-
 t/t3430-rebase-merges.sh               |  4 +--
 t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------
 t/t5407-post-rewrite-hook.sh           |  4 +--
 t/t5516-fetch-push.sh                  |  4 +--
 t/t5531-deep-submodule-push.sh         |  2 +-
 t/t7525-status-rename.sh               |  8 ++---
 t/t7810-grep.sh                        | 16 +++++-----
 t/test-lib.sh                          |  7 ++++
 12 files changed, 75 insertions(+), 43 deletions(-)


base-commit: 041f5ea1cf987a4068ef5f39ba0a09be85952064
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-167%2Fdscho%2Fdisallow-abbreviated-options-in-tests-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-167/dscho/disallow-abbreviated-options-in-tests-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/167

Range-diff vs v1:

 1:  a1b4b74b91 = 1:  a1b4b74b91 tests (rebase): spell out the `--keep-empty` option
 2:  017ec9a0c7 = 2:  017ec9a0c7 tests (rebase): spell out the `--force-rebase` option
 3:  7d8f6a2ee6 = 3:  7d8f6a2ee6 t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`
 4:  f45c435e4a = 4:  f45c435e4a t5531: avoid using an abbreviated option
 5:  7372059922 = 5:  7372059922 tests (push): do not abbreviate the `--follow-tags` option
 6:  531450c00d = 6:  531450c00d tests (status): spell out the `--find-renames` option in full
 7:  a722a065d2 = 7:  a722a065d2 tests (pack-objects): use the full, unabbreviated `--revs` option
 8:  04c36b1de9 ! 8:  a27d316855 tests: disallow the use of abbreviated options (by default)
     @@ -14,7 +14,7 @@
      
          For example, if a future contribution added a new mode
          `git init --babyproofing` and a previously-introduced test case used the
     -    fact that `git init --ba` expaneded to `git init --bare`, that future
     +    fact that `git init --ba` expanded to `git init --bare`, that future
          contribution would now have to touch seemingly unrelated tests just to
          keep the test suite from failing.
      
     @@ -115,6 +115,18 @@
       	test-tool parse-options --no-ambig >output 2>output.err &&
       	test_must_be_empty output.err &&
       	test_cmp expect output
     +@@
     + 	test-tool parse-options --expect="verbose: 0" -v -v -v --no-verbose
     + '
     + 
     ++test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '
     ++	env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
     ++		test-tool parse-options --ye &&
     ++	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \
     ++		test-tool parse-options --ye
     ++'
     ++
     + test_done
      
       diff --git a/t/test-lib.sh b/t/test-lib.sh
       --- a/t/test-lib.sh
     @@ -124,10 +136,11 @@
       export PERL_PATH SHELL_PATH
       
      +# Disallow the use of abbreviated options in the test suite by default
     -+test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
     ++if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
     ++then
      +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
      +	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
     -+}
     ++fi
      +
       ################################################################
       # It appears that people try to run tests without building...

-- 
gitgitgadget

[PATCH v2 1/8] tests (rebase): spell out the `--keep-empty` option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This test wants to run `git rebase` with the `--keep-empty` option, but
it really only spelled out `--keep` and trusted Git's option parsing to
determine that this was a unique abbreviation of the real option.

However, Denton Liu contributed a patch series in
https://public-inbox.org/git/cover.1553354374.git.liu.denton@gmail.com/
that introduces a new `git rebase` option called `--keep-base`, which
makes this previously unique abbreviation non-unique.

Whether this patch series is accepted or not, it is actually a bad
practice to use abbreviated options in our test suite, because of the
issue that those unique option names are not guaranteed to stay unique
in the future.

So let's just not use abbreviated options in the test suite.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5407-post-rewrite-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
index a4a5903cba..7344253bfb 100755
--- a/t/t5407-post-rewrite-hook.sh
+++ b/t/t5407-post-rewrite-hook.sh
@@ -131,7 +131,7 @@ test_expect_success 'git rebase -m --skip' '
 test_expect_success 'git rebase with implicit use of interactive backend' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --keep --onto A B &&
+	test_must_fail git rebase --keep-empty --onto A B &&
 	echo C > foo &&
 	git add foo &&
 	git rebase --continue &&
@@ -146,7 +146,7 @@ test_expect_success 'git rebase with implicit use of interactive backend' '
 test_expect_success 'git rebase --skip with implicit use of interactive backend' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --keep --onto A B &&
+	test_must_fail git rebase --keep-empty --onto A B &&
 	test_must_fail git rebase --skip &&
 	echo D > foo &&
 	git add foo &&
-- 
gitgitgadget

[PATCH v2 2/8] tests (rebase): spell out the `--force-rebase` option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In quite a few test cases, we were sloppy and used the abbreviation
`--force`, but we really should be precise in what we want to test.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t3415-rebase-autosquash.sh | 2 +-
 t/t3430-rebase-merges.sh     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
index 13f5688135..22d218698e 100755
--- a/t/t3415-rebase-autosquash.sh
+++ b/t/t3415-rebase-autosquash.sh
@@ -277,7 +277,7 @@ test_expect_success 'autosquash with empty custom instructionFormat' '
 	(
 		set_cat_todo_editor &&
 		test_must_fail git -c rebase.instructionFormat= \
-			rebase --autosquash  --force -i HEAD^ >actual &&
+			rebase --autosquash  --force-rebase -i HEAD^ >actual &&
 		git log -1 --format="pick %h %s" >expect &&
 		test_cmp expect actual
 	)
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 4c69255ee6..42ba5b9f09 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -271,7 +271,7 @@ test_expect_success 'root commits' '
 	EOF
 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
 	test_tick &&
-	git rebase -i --force --root -r &&
+	git rebase -i --force-rebase --root -r &&
 	test "Parsnip" = "$(git show -s --format=%an HEAD^)" &&
 	test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
 	test $(git rev-parse second-root:second-root.t) = \
@@ -364,7 +364,7 @@ test_expect_success 'octopus merges' '
 	test_cmp_rev HEAD $before &&
 
 	test_tick &&
-	git rebase -i --force -r HEAD^^ &&
+	git rebase -i --force-rebase -r HEAD^^ &&
 	test "Hank" = "$(git show -s --format=%an HEAD)" &&
 	test "$before" != $(git rev-parse HEAD) &&
 	test_cmp_graph HEAD^^.. <<-\EOF
-- 
gitgitgadget

[PATCH v2 3/8] t7810: do not abbreviate `--no-exclude-standard` nor `--invert-match`

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

This script used abbreviated options, which is unnecessarily fragile.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7810-grep.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 43aa4161cf..2e1bb61b41 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -119,33 +119,33 @@ do
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep -w $L (with --column, --invert)" '
+	test_expect_success "grep -w $L (with --column, --invert-match)" '
 		{
 			echo ${HC}file:1:foo mmap bar
 			echo ${HC}file:1:foo_mmap bar
 			echo ${HC}file:1:foo_mmap bar mmap
 			echo ${HC}file:1:foo mmap bar_mmap
 		} >expected &&
-		git grep --column --invert -w -e baz $H -- file >actual &&
+		git grep --column --invert-match -w -e baz $H -- file >actual &&
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep $L (with --column, --invert, extended OR)" '
+	test_expect_success "grep $L (with --column, --invert-match, extended OR)" '
 		{
 			echo ${HC}hello_world:6:HeLLo_world
 		} >expected &&
-		git grep --column --invert -e ll --or --not -e _ $H -- hello_world \
+		git grep --column --invert-match -e ll --or --not -e _ $H -- hello_world \
 			>actual &&
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep $L (with --column, --invert, extended AND)" '
+	test_expect_success "grep $L (with --column, --invert-match, extended AND)" '
 		{
 			echo ${HC}hello_world:3:Hello world
 			echo ${HC}hello_world:3:Hello_world
 			echo ${HC}hello_world:6:HeLLo_world
 		} >expected &&
-		git grep --column --invert --not -e _ --and --not -e ll $H -- hello_world \
+		git grep --column --invert-match --not -e _ --and --not -e ll $H -- hello_world \
 			>actual &&
 		test_cmp expected actual
 	'
@@ -1010,7 +1010,7 @@ test_expect_success 'outside of git repository' '
 			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git grep --no-index --no-exclude o >../actual.full &&
+		git grep --no-index --no-exclude-standard o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '
@@ -1051,7 +1051,7 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
 			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git -c grep.fallbackToNoIndex grep --no-exclude o >../actual.full &&
+		git -c grep.fallbackToNoIndex grep --no-exclude-standard o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '
-- 
gitgitgadget

[PATCH v2 4/8] t5531: avoid using an abbreviated option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It was probably just an oversight: the `--recurse-submodules` option
puts the term "submodules" in the plural form, not the singular one.

To avoid future problems in case that another option is introduced that
starts with the prefix `--recurse-submodule`, let's just fix this.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5531-deep-submodule-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
index e2c37fd978..4ad059e6be 100755
--- a/t/t5531-deep-submodule-push.sh
+++ b/t/t5531-deep-submodule-push.sh
@@ -363,7 +363,7 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs
 		) &&
 		git add b &&
 		git commit -m "added submodule" &&
-		git push --recurse-submodule=check origin master
+		git push --recurse-submodules=check origin master
 	)
 '
 
-- 
gitgitgadget

[PATCH v2 5/8] tests (push): do not abbreviate the `--follow-tags` option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

We really want to spell out the option in the full form, to avoid any
ambiguity that might be introduced by future patches.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5516-fetch-push.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 37e8e80893..db0b1db458 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1370,7 +1370,7 @@ test_expect_success 'push does not follow tags by default' '
 	test_cmp expect actual
 '
 
-test_expect_success 'push --follow-tag only pushes relevant tags' '
+test_expect_success 'push --follow-tags only pushes relevant tags' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
 	git init src &&
@@ -1384,7 +1384,7 @@ test_expect_success 'push --follow-tag only pushes relevant tags' '
 		git tag -m "future" future &&
 		git checkout master &&
 		git for-each-ref refs/heads/master refs/tags/tag >../expect &&
-		git push --follow-tag ../dst master
+		git push --follow-tags ../dst master
 	) &&
 	(
 		cd dst &&
-- 
gitgitgadget

[PATCH v2 6/8] tests (status): spell out the `--find-renames` option in full

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

To avoid future ambiguities, we really want to use full option names in
the test suite. `t7525-status-rename.sh` used an abbreviated form of the
`--find-renames` option, though, so let's change that.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t7525-status-rename.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7525-status-rename.sh b/t/t7525-status-rename.sh
index ef8b1b3078..a62736dce0 100755
--- a/t/t7525-status-rename.sh
+++ b/t/t7525-status-rename.sh
@@ -84,7 +84,7 @@ test_expect_success 'status score=100%' '
 	test_i18ngrep "deleted:" actual &&
 	test_i18ngrep "new file:" actual &&
 
-	git status --find-rename=100% >actual &&
+	git status --find-renames=100% >actual &&
 	test_i18ngrep "deleted:" actual &&
 	test_i18ngrep "new file:" actual
 '
@@ -93,11 +93,11 @@ test_expect_success 'status score=01%' '
 	git status -M=01% >actual &&
 	test_i18ngrep "renamed:" actual &&
 
-	git status --find-rename=01% >actual &&
+	git status --find-renames=01% >actual &&
 	test_i18ngrep "renamed:" actual
 '
 
-test_expect_success 'copies not overridden by find-rename' '
+test_expect_success 'copies not overridden by find-renames' '
 	cp renamed copy &&
 	git add copy &&
 
@@ -105,7 +105,7 @@ test_expect_success 'copies not overridden by find-rename' '
 	test_i18ngrep "copied:" actual &&
 	test_i18ngrep "renamed:" actual &&
 
-	git -c status.renames=copies status --find-rename=01% >actual &&
+	git -c status.renames=copies status --find-renames=01% >actual &&
 	test_i18ngrep "copied:" actual &&
 	test_i18ngrep "renamed:" actual
 '
-- 
gitgitgadget

[PATCH v2 7/8] tests (pack-objects): use the full, unabbreviated `--revs` option

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

To use the singular form of a word, when the option wants the plural
form (and quietly expands it because it thinks it was abbreviated), is
an easy mistake to make, and t5317 contains almost two dozen of them.

However, using abbreviated options in tests is a bit fragile, so we will
disallow use of abbreviated options in our test suite.

In preparation for this change, let's fix
`t5317-pack-objects-filter-objects.sh`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/t/t5317-pack-objects-filter-objects.sh b/t/t5317-pack-objects-filter-objects.sh
index 24541ea137..4c0201c34b 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -25,7 +25,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../all.pack &&
@@ -39,7 +39,7 @@ test_expect_success 'verify blob count in normal packfile' '
 '
 
 test_expect_success 'verify blob:none packfile has no blobs' '
-	git -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=blob:none >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../filter.pack &&
@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' '
 	git -C r5 commit -m "foo" &&
 	del=$(git -C r5 rev-parse HEAD^{tree} | sed "s|..|&/|") &&
 	rm r5/.git/objects/$del &&
-	test_must_fail git -C r5 pack-objects --rev --stdout 2>bad_tree <<-EOF &&
+	test_must_fail git -C r5 pack-objects --revs --stdout 2>bad_tree <<-EOF &&
 	HEAD
 	EOF
 	grep "bad tree object" bad_tree
@@ -88,7 +88,7 @@ test_expect_success 'setup for tests of tree:0' '
 '
 
 test_expect_success 'verify tree:0 packfile has no blobs or trees' '
-	git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../commitsonly.pack &&
@@ -98,7 +98,7 @@ test_expect_success 'verify tree:0 packfile has no blobs or trees' '
 
 test_expect_success 'grab tree directly when using tree:0' '
 	# We should get the tree specified directly but not its blobs or subtrees.
-	git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
 	HEAD:
 	EOF
 	git -C r1 index-pack ../commitsonly.pack &&
@@ -128,7 +128,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../all.pack &&
@@ -142,7 +142,7 @@ test_expect_success 'verify blob count in normal packfile' '
 '
 
 test_expect_success 'verify blob:limit=500 omits all blobs' '
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -157,7 +157,7 @@ test_expect_success 'verify blob:limit=500 omits all blobs' '
 '
 
 test_expect_success 'verify blob:limit=1000' '
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -176,7 +176,7 @@ test_expect_success 'verify blob:limit=1001' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -194,7 +194,7 @@ test_expect_success 'verify blob:limit=10001' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -212,7 +212,7 @@ test_expect_success 'verify blob:limit=1k' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -230,7 +230,7 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
 	HEAD
 	$(git -C r2 rev-parse HEAD:large.10000)
 	EOF
@@ -249,7 +249,7 @@ test_expect_success 'verify blob:limit=1m' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&
@@ -302,7 +302,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../all.pack &&
@@ -320,7 +320,7 @@ test_expect_success 'verify sparse:path=pattern1' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../filter.pack &&
@@ -352,7 +352,7 @@ test_expect_success 'verify sparse:path=pattern2' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../filter.pack &&
@@ -404,7 +404,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r4 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../all.pack &&
@@ -423,7 +423,7 @@ test_expect_success 'verify sparse:oid=OID' '
 	sort >expected &&
 
 	oid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) &&
-	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../filter.pack &&
@@ -441,7 +441,7 @@ test_expect_success 'verify sparse:oid=oid-ish' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../filter.pack &&
@@ -470,19 +470,19 @@ test_expect_success 'setup r1 - delete loose blobs' '
 '
 
 test_expect_success 'verify pack-objects fails w/ missing objects' '
-	test_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF
+	test_must_fail git -C r1 pack-objects --revs --stdout >miss.pack <<-EOF
 	HEAD
 	EOF
 '
 
 test_expect_success 'verify pack-objects fails w/ --missing=error' '
-	test_must_fail git -C r1 pack-objects --rev --stdout --missing=error >miss.pack <<-EOF
+	test_must_fail git -C r1 pack-objects --revs --stdout --missing=error >miss.pack <<-EOF
 	HEAD
 	EOF
 '
 
 test_expect_success 'verify pack-objects w/ --missing=allow-any' '
-	git -C r1 pack-objects --rev --stdout --missing=allow-any >miss.pack <<-EOF
+	git -C r1 pack-objects --revs --stdout --missing=allow-any >miss.pack <<-EOF
 	HEAD
 	EOF
 '
-- 
gitgitgadget

[PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Git's command-line parsers support uniquely abbreviated options, e.g.
`git init --ba` would automatically expand `--ba` to `--bare`.

This is a very convenient feature in every day life for Git users, in
particular when tab completion is not available.

However, it is not a good idea to rely on that in Git's test suite, as
something that is a unique abbreviation of a command line option today
might no longer be a unique abbreviation tomorrow.

For example, if a future contribution added a new mode
`git init --babyproofing` and a previously-introduced test case used the
fact that `git init --ba` expanded to `git init --bare`, that future
contribution would now have to touch seemingly unrelated tests just to
keep the test suite from failing.

So let's disallow abbreviated options in the test suite by default.

Note: for ease of implementation, this patch really only touches the
`parse-options` machinery: more and more hand-rolled option parsers are
converted to use that internal API, and more and more scripts are
converted to built-ins (naturally using the parse-options API, too), so
in practice this catches most issues, and is definitely the biggest bang
for the buck.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 parse-options.c          |  9 +++++++++
 t/README                 |  4 ++++
 t/t0040-parse-options.sh | 14 +++++++++++++-
 t/test-lib.sh            |  7 +++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index cec74522e5..acc3a93660 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -6,6 +6,8 @@
 #include "color.h"
 #include "utf8.h"
 
+static int disallow_abbreviated_options;
+
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
+	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+		die("disallowed abbreviated or ambiguous option '%.*s'",
+		    (int)(arg_end - arg), arg);
+
 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),
@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
+	disallow_abbreviated_options =
+		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
+
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
diff --git a/t/README b/t/README
index 656288edce..9ed3051a1c 100644
--- a/t/README
+++ b/t/README
@@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
 fetch-pack to not request sideband-all (even if the server advertises
 sideband-all).
 
+GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
+the default when running tests), errors out when an abbreviated option
+is used.
+
 Naming Tests
 ------------
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index b8f366c442..6f6f74bfe2 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -203,20 +203,24 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 test-tool parse-options --strin 123
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
@@ -370,4 +375,11 @@ test_expect_success '--no-verbose resets multiple verbose to 0' '
 	test-tool parse-options --expect="verbose: 0" -v -v -v --no-verbose
 '
 
+test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '
+	env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+		test-tool parse-options --ye &&
+	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \
+		test-tool parse-options --ye
+'
+
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 562c57e685..f1a0fea4e1 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -57,6 +57,13 @@ fi
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH
 
+# Disallow the use of abbreviated options in the test suite by default
+if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
+then
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+fi
+
 ################################################################
 # It appears that people try to run tests without building...
 "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
-- 
gitgitgadget

Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +	disallow_abbreviated_options =
> +		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);

... which means that the value of the environment variable follows
the usual "true, yes and 1 all activate it"; very good,

> diff --git a/t/README b/t/README
> index 656288edce..9ed3051a1c 100644
> --- a/t/README
> +++ b/t/README
> @@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
>  fetch-pack to not request sideband-all (even if the server advertises
>  sideband-all).
>  
> +GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
> +the default when running tests), errors out when an abbreviated option
> +is used.

OK.

> +test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '
> +	env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
> +		test-tool parse-options --ye &&
> +	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \
> +		test-tool parse-options --ye
> +'

The feature is activated for all tests unless otherwise noted, and
the above marked the places that need to be "otherwise noted" in a
reasonable way.  Good.

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 562c57e685..f1a0fea4e1 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -57,6 +57,13 @@ fi
>  . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
>  export PERL_PATH SHELL_PATH
>  
> +# Disallow the use of abbreviated options in the test suite by default
> +if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
> +then
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
> +	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
> +fi

If the original environment has it as flase, as it is set, the
substitution will yield "isset" which is not an empty string, so we
assign true.

If the original environment is not set, or set to an empty, however,
the substitution will yield an empty string, so we won't touch the
variable.

I am not sure in what situation the above behaviour becomes useful.

Do you mean more like

	if test -z "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS"
	then
		... assignment ...
	fi

IOW, we'll take an explicit GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false
as a sign that the developer for whatever readon wants the disambiguation
code in parse-options to kick in for all uses and allow a shortened option
names?

If on the other hand you are protecting our tests against those
who casually have the environment set to false, because they know
some of the scripts they use are sloppy *and* for whatever reason
they anticipate that someday we will start to disallow abbrevated
options by default?  If so, an unconditional assignment of true
would be more appropriate.

I think I can agree with either of the two positions (i.e. we let
those who explicitly want to decline do so, or we unconditionally
make sure we catch issues in our tests), and I do not think of a
third position that are different from these two and that would make
sense.  Between the two, I'd probably vote for the latter if I was
pressed, but even then that is not a very strong preference.

Thanks.  I very much like the premise of this series, and the above
hunk stood out in the range-diff in 0/8.

Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:


> Do you mean more like
> ...
> I think I can agree with either of the two positions...

I am guessing from the earlier iteration that you wanted to say
"unless it is given explicitly, we turn it on".

As this last-minute style update that was botched, and a typofix in
the proposed log message in 8/8, are the only differences, let me
locally fix 8/8 up and replace it.

It was very good that this round did not have to touch 1/8 which is
shared with Denton's keep-base topic; otherwise replacing would have
been a little bit messier.

Here is what I'll use (unless it gets further replaced by v3 or
later, that is).

Thanks.

-- >8 --
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Fri, 12 Apr 2019 02:37:24 -0700
Subject: [PATCH v2bis 8/8] tests: disallow the use of abbreviated options (by default)

Git's command-line parsers support uniquely abbreviated options, e.g.
`git init --ba` would automatically expand `--ba` to `--bare`.

This is a very convenient feature in every day life for Git users, in
particular when tab completion is not available.

However, it is not a good idea to rely on that in Git's test suite, as
something that is a unique abbreviation of a command line option today
might no longer be a unique abbreviation tomorrow.

For example, if a future contribution added a new mode
`git init --babyproofing` and a previously-introduced test case used the
fact that `git init --ba` expanded to `git init --bare`, that future
contribution would now have to touch seemingly unrelated tests just to
keep the test suite from failing.

So let's disallow abbreviated options in the test suite by default.

Note: for ease of implementation, this patch really only touches the
`parse-options` machinery: more and more hand-rolled option parsers are
converted to use that internal API, and more and more scripts are
converted to built-ins (naturally using the parse-options API, too), so
in practice this catches most issues, and is definitely the biggest bang
for the buck.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c          |  9 +++++++++
 t/README                 |  4 ++++
 t/t0040-parse-options.sh | 14 +++++++++++++-
 t/test-lib.sh            |  7 +++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index cec74522e5..acc3a93660 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -6,6 +6,8 @@
 #include "color.h"
 #include "utf8.h"
 
+static int disallow_abbreviated_options;
+
 #define OPT_SHORT 1
 #define OPT_UNSET 2
 
@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
+	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+		die("disallowed abbreviated or ambiguous option '%.*s'",
+		    (int)(arg_end - arg), arg);
+
 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),
@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
+	disallow_abbreviated_options =
+		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
+
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
diff --git a/t/README b/t/README
index 656288edce..9ed3051a1c 100644
--- a/t/README
+++ b/t/README
@@ -399,6 +399,10 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
 fetch-pack to not request sideband-all (even if the server advertises
 sideband-all).
 
+GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
+the default when running tests), errors out when an abbreviated option
+is used.
+
 Naming Tests
 ------------
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index b8f366c442..800b3ea5f5 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -203,20 +203,24 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 test-tool parse-options --strin 123
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
@@ -370,4 +375,11 @@ test_expect_success '--no-verbose resets multiple verbose to 0' '
 	test-tool parse-options --expect="verbose: 0" -v -v -v --no-verbose
 '
 
+test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+		test-tool parse-options --ye &&
+	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \
+		test-tool parse-options --ye
+'
+
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 562c57e685..c14ebe68d3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -57,6 +57,13 @@ fi
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH
 
+# Disallow the use of abbreviated options in the test suite by default
+if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}"
+then
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+fi
+
 ################################################################
 # It appears that people try to run tests without building...
 "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null
-- 
2.21.0-313-ge35b8cb8e2

Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3196 bytes --]

Hi Junio,

On Sun, 14 Apr 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 562c57e685..f1a0fea4e1 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -57,6 +57,13 @@ fi
> >  . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> >  export PERL_PATH SHELL_PATH
> >
> > +# Disallow the use of abbreviated options in the test suite by default
> > +if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
> > +then
> > +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
> > +	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
> > +fi
>
> If the original environment has it as flase, as it is set, the
> substitution will yield "isset" which is not an empty string, so we
> assign true.
>
> If the original environment is not set, or set to an empty, however,
> the substitution will yield an empty string, so we won't touch the
> variable.
>
> I am not sure in what situation the above behaviour becomes useful.
>
> Do you mean more like
>
> 	if test -z "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS"
> 	then
> 		... assignment ...
> 	fi

Ævar rightfully pointed out that I introduced a new paradigm, so I
switched to imitating the exact way `test-lib.sh` handles similar cases.
Which is the `isset` method.

Sure, I could do something differently, like `test -z`. But why? I think
it is better to stay consistent with the existing checks.

> IOW, we'll take an explicit GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false
> as a sign that the developer for whatever readon wants the disambiguation
> code in parse-options to kick in for all uses and allow a shortened option
> names?

Yes.

Let's assume that there is a developer, or even a team (think e.g. VFS for
Git), working on some long-running features that should be eventually
contributed to Git, but that are still in flux, and through a merge with
the current Git version, they get this change, and it breaks 50 of their
test scripts.

This is the scenario I wanted to help, however unlikely that is ;-)

Of course, eventually they'd want to fix their test scripts. But maybe
right now is not such a great time, so let's let them override the
new behavior.

> If on the other hand you are protecting our tests against those
> who casually have the environment set to false, because they know
> some of the scripts they use are sloppy *and* for whatever reason
> they anticipate that someday we will start to disallow abbrevated
> options by default?  If so, an unconditional assignment of true
> would be more appropriate.
>
> I think I can agree with either of the two positions (i.e. we let
> those who explicitly want to decline do so, or we unconditionally
> make sure we catch issues in our tests), and I do not think of a
> third position that are different from these two and that would make
> sense.  Between the two, I'd probably vote for the latter if I was
> pressed, but even then that is not a very strong preference.
>
> Thanks.  I very much like the premise of this series, and the above
> hunk stood out in the range-diff in 0/8.

Thank you!
Dscho

Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)

[Johannes Schindelin]

Hi Junio,

On Mon, 15 Apr 2019, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>
> > Do you mean more like
> > ...
> > I think I can agree with either of the two positions...
>
> I am guessing from the earlier iteration that you wanted to say
> "unless it is given explicitly, we turn it on".
>
> As this last-minute style update that was botched, and a typofix in
> the proposed log message in 8/8, are the only differences, let me
> locally fix 8/8 up and replace it.

Sure. I still would like the `isset` thing, as it makes things more
consistent, but I'll not fight for it.

Ciao,
Dscho

Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Mon, 15 Apr 2019, Junio C Hamano wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>
>> > Do you mean more like
>> > ...
>> > I think I can agree with either of the two positions...
>>
>> I am guessing from the earlier iteration that you wanted to say
>> "unless it is given explicitly, we turn it on".
>>
>> As this last-minute style update that was botched, and a typofix in
>> the proposed log message in 8/8, are the only differences, let me
>> locally fix 8/8 up and replace it.
>
> Sure. I still would like the `isset` thing, as it makes things more
> consistent, but I'll not fight for it.

${var:+isset} is fine.  Instead of

+# Disallow the use of abbreviated options in the test suite by default
+if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}"
+then
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+fi
+

if you used

	if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
	then
		...

I won't object.  After all, I know I introduced :+isset pattern to
our shell script codebase as there are cases where it makes the
result easier to follow.

But the thing is that your patch had the polarity inverted.  Where
you must say "if this thing is not set, assign this value", you said
"if this thing is set, assign this value", which was totally bogus.
As long as that is corrected, that's fine.

Having said that.

When you check if the variable is set, use of the ":+isset" pattern
makes the result often easier to follow by explicitly letting us
compare with an explicit "isset" token, e.g.

	case ",${VAR1:+isset},${VAR2:+isset}," in
	*,isset,*)	: at least one is set ;;
	*)		: neither is set ;;
	esac

This *does* make the code simpler and easier.  But when checking for
"is it not set?", you can compare with an explicit literal "" and
that comparison is plenty clear enough.  You won't get as much
benefit as the "is it set?" test would out of the pattern.  I would
not say that it is pointless to use the ":+isset" pattern when
checking "is it not set?", but it is very close.

Re: [PATCH v2 8/8] tests: disallow the use of abbreviated options (by default)

[Johannes Schindelin]

Hi Junio,

On Mon, 15 Apr 2019, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Mon, 15 Apr 2019, Junio C Hamano wrote:
> >
> >> Junio C Hamano <gitster@pobox.com> writes:
> >>
> >>
> >> > Do you mean more like
> >> > ...
> >> > I think I can agree with either of the two positions...
> >>
> >> I am guessing from the earlier iteration that you wanted to say
> >> "unless it is given explicitly, we turn it on".
> >>
> >> As this last-minute style update that was botched, and a typofix in
> >> the proposed log message in 8/8, are the only differences, let me
> >> locally fix 8/8 up and replace it.
> >
> > Sure. I still would like the `isset` thing, as it makes things more
> > consistent, but I'll not fight for it.
>
> ${var:+isset} is fine.  Instead of
>
> +# Disallow the use of abbreviated options in the test suite by default
> +if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}"
> +then
> +	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
> +	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
> +fi
> +
>
> if you used
>
> 	if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
> 	then
> 		...
>
> I won't object.  After all, I know I introduced :+isset pattern to
> our shell script codebase as there are cases where it makes the
> result easier to follow.
>
> But the thing is that your patch had the polarity inverted.

Ah! I finally understand what you are saying. Took me a while, sorry.

> Where you must say "if this thing is not set, assign this value", you
> said "if this thing is set, assign this value", which was totally bogus.
> As long as that is corrected, that's fine.

Of course! It should be `-z` instead of `-n` there.

> Having said that.
>
> When you check if the variable is set, use of the ":+isset" pattern
> makes the result often easier to follow by explicitly letting us
> compare with an explicit "isset" token, e.g.
>
> 	case ",${VAR1:+isset},${VAR2:+isset}," in
> 	*,isset,*)	: at least one is set ;;
> 	*)		: neither is set ;;
> 	esac
>
> This *does* make the code simpler and easier.  But when checking for
> "is it not set?", you can compare with an explicit literal "" and
> that comparison is plenty clear enough.  You won't get as much
> benefit as the "is it set?" test would out of the pattern.  I would
> not say that it is pointless to use the ":+isset" pattern when
> checking "is it not set?", but it is very close.

Well, there is also the subtelty that somebody might *want* to set that
environment variable to the empty string (and obviously they would not
deem it appropriate for us to override their choice).

But I do have bigger fish to fry, so if you're fine with the `isset` thing
(fixed, of course), please go for it. If you'd prefer the version without
it, that's fine enough with me, too.

Ciao,
Dscho

[PATCH v2] parse-options: don't emit "ambiguous option" for aliases

[Ævar Arnfjörð Bjarmason]

Change the option parsing machinery so that e.g. "clone --recurs ..."
doesn't error out because "clone" understands both "--recursive" and
"--recurse-submodules" to mean the same thing.

Initially "clone" just understood --recursive until the
--recurses-submodules alias was added in ccdd3da652 ("clone: Add the
--recurse-submodules option as alias for --recursive",
2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to
optionally take a pathspec", 2017-03-17) the longer form has been
promoted to the default.

But due to the way the options parsing machinery works this resulted
in the rather absurd situation of:

    $ git clone --recurs [...]
    error: ambiguous option: recurs (could be --recursive or --recurse-submodules)

Let's re-use the PARSE_OPT_NOCOMPLETE flag to mean "this option
doesn't contribute to abbreviation ambiguity". I was going to add a
new PARSE_OPT_NOABBREV flag, but it makes sense just to re-use
PARSE_OPT_NOCOMPLETE.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

See
https://public-inbox.org/git/20190325202329.26033-1-avarab@gmail.com/
for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2
seems like a no-brainer bugfix.

It conflicted with some recently-landed stuff in 'master', but now
cleanly applies to it and 'pu', and with pu's
GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS.

 builtin/clone.c          | 4 ++--
 parse-options.c          | 3 ++-
 parse-options.h          | 2 ++
 t/t0040-parse-options.sh | 5 +++++
 4 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 50bde99618..4dc26969a7 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {
 		    N_("setup as shared repository")),
 	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
+	  recurse_submodules_cb, (intptr_t)"." },
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
diff --git a/parse-options.c b/parse-options.c
index cec74522e5..9899ce0171 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt(
 		if (!rest) {
 			/* abbreviated? */
 			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
-			    !strncmp(long_name, arg, arg_end - arg)) {
+			    !strncmp(long_name, arg, arg_end - arg) &&
+			    !(options->flags & PARSE_OPT_NOCOMPLETE)) {
 is_abbreviated:
 				if (abbrev_option) {
 					/*
diff --git a/parse-options.h b/parse-options.h
index 74cce4e7fc..51c4b71ab0 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *				Useful for options with multiple parameters.
  *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
  *			   by git-completion.bash. This option suppresses that.
+ *			   Will also skip this option when abbreviation is
+ *			   considered.
  *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
  *			 complete an option as --name= not --name even if
  *			 the option takes optional argument.
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index b8f366c442..e8f0371830 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '
+	test_when_finished "rm -rf A" &&
+	git clone --recurs . A
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF
-- 
2.21.0.593.g511ec345e18

Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases

[Duy Nguyen]

On Wed, Apr 17, 2019 at 7:44 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Change the option parsing machinery so that e.g. "clone --recurs ..."
> doesn't error out because "clone" understands both "--recursive" and
> "--recurse-submodules" to mean the same thing.
>
> Initially "clone" just understood --recursive until the
> --recurses-submodules alias was added in ccdd3da652 ("clone: Add the
> --recurse-submodules option as alias for --recursive",
> 2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to
> optionally take a pathspec", 2017-03-17) the longer form has been
> promoted to the default.
>
> But due to the way the options parsing machinery works this resulted
> in the rather absurd situation of:
>
>     $ git clone --recurs [...]
>     error: ambiguous option: recurs (could be --recursive or --recurse-submodules)
>
> Let's re-use the PARSE_OPT_NOCOMPLETE flag to mean "this option
> doesn't contribute to abbreviation ambiguity". I was going to add a
> new PARSE_OPT_NOABBREV flag, but it makes sense just to re-use
> PARSE_OPT_NOCOMPLETE.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>
> See
> https://public-inbox.org/git/20190325202329.26033-1-avarab@gmail.com/
> for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2
> seems like a no-brainer bugfix.
>
> It conflicted with some recently-landed stuff in 'master', but now
> cleanly applies to it and 'pu', and with pu's
> GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS.
>
>  builtin/clone.c          | 4 ++--
>  parse-options.c          | 3 ++-
>  parse-options.h          | 2 ++
>  t/t0040-parse-options.sh | 5 +++++
>  4 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 50bde99618..4dc26969a7 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {
>                     N_("setup as shared repository")),
>         { OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
>           N_("pathspec"), N_("initialize submodules in the clone"),
> -         PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
> -         (intptr_t)"." },
> +         PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,

What happens if someone adds --recursive-hard? --recursi then
resolving to --recursive-hard sounds wrong.

But on the other hand I can see it's a bit more work to teach
parse-options OPT_ALIAS to say "--recursive is just an alias of
--recurse-submodules" and chances of --recursive-hard coming up are
probably very low.

So I don't know but I thought I should point out.

> +         recurse_submodules_cb, (intptr_t)"." },
>         { OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
>           N_("pathspec"), N_("initialize submodules in the clone"),
>           PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
> diff --git a/parse-options.c b/parse-options.c
> index cec74522e5..9899ce0171 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt(
>                 if (!rest) {
>                         /* abbreviated? */
>                         if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
> -                           !strncmp(long_name, arg, arg_end - arg)) {
> +                           !strncmp(long_name, arg, arg_end - arg) &&
> +                           !(options->flags & PARSE_OPT_NOCOMPLETE)) {
>  is_abbreviated:
>                                 if (abbrev_option) {
>                                         /*
> diff --git a/parse-options.h b/parse-options.h
> index 74cce4e7fc..51c4b71ab0 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
>   *                             Useful for options with multiple parameters.
>   *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
>   *                        by git-completion.bash. This option suppresses that.
> + *                        Will also skip this option when abbreviation is
> + *                        considered.
>   *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
>   *                      complete an option as --name= not --name even if
>   *                      the option takes optional argument.
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index b8f366c442..e8f0371830 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '
>         test-tool parse-options --expect="string: 123" --st 123
>  '
>
> +test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '
> +       test_when_finished "rm -rf A" &&
> +       git clone --recurs . A
> +'
> +
>  cat >typo.err <<\EOF
>  error: did you mean `--boolean` (with two dashes ?)
>  EOF
> --
> 2.21.0.593.g511ec345e18
>


-- 
Duy

Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>>         { OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
>>           N_("pathspec"), N_("initialize submodules in the clone"),
>> -         PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
>> -         (intptr_t)"." },
>> +         PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
>
> What happens if someone adds --recursive-hard? --recursi then
> resolving to --recursive-hard sounds wrong.

That was exactly my initial reaction.  Or "recurse-submodules" gets
renamed away, and "recommend" gets added---now "--rec" is still not
ambiguous as "recursive" is marked not to participate in the
disambiguation (I think OPT_NOCOMPLETE should at least be renamed to
OPT_NO_DISAMBIGUATION or something---if we were to use it for the
purpose of marking an option as "not participating in
disambiguation", but I am fairly negative on the approach).

And my initial reaction was followed by "don't we want a more
explicit link only between recursive and recurse-submodules?",
which exactly matches what you said below ;-)

> But on the other hand I can see it's a bit more work to teach
> parse-options OPT_ALIAS to say "--recursive is just an alias of
> --recurse-submodules" and chances of --recursive-hard coming up are
> probably very low.

The "bit more work" is something that is worth doing in this case, I
think.

Thanks.

Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases

[Duy Nguyen]

On Thu, Apr 18, 2019 at 09:48:39AM +0900, Junio C Hamano wrote:
> > But on the other hand I can see it's a bit more work to teach
> > parse-options OPT_ALIAS to say "--recursive is just an alias of
> > --recurse-submodules" and chances of --recursive-hard coming up are
> > probably very low.
> 
> The "bit more work" is something that is worth doing in this case, I
> think.

I had a look at it. Linking two options together is not exactly easy
because the way we organize option data. And I also had difficulty
defining the exact semantics of this OPT_ALIAS.

So an alternative is simply outsource the ambiguity decision back to
git-clone. If the same situation appears again elsewhere, we'll need
to sit back and fix it for real. But this way we don't potentially
introduce any new traps.

parse_options_extended() also makes it easier to extend parseopt
functionality without changing all call sites. One thing I have in
mind, which could maybe use this interface, is to remove
parse_options_{start,step,end} from public API, avoid expose the
parse_options_step internal result code. The caller just need to pass
a callback to handle unknown options.

-- 8< --
diff --git a/builtin/clone.c b/builtin/clone.c
index 50bde99618..041cd43ddc 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -145,6 +145,29 @@ static struct option builtin_clone_options[] = {
 	OPT_END()
 };
 
+/*
+ * Avoid ambiguation error between --recursive and --recurse-submodule
+ * because they are the same. --recurs can be expanded to any of them
+ * and it still works.
+ */
+static int is_abbrev_ambiguous(const struct option *prev,
+			       const struct option *next)
+{
+	const struct option *opts[] = { prev, next };
+	int i, found = 0;
+
+	for (i = 0; i < ARRAY_SIZE(opts); i++) {
+		if (!opts[i]->long_name)
+			continue;
+		if (!strcmp(opts[i]->long_name, "recursive"))
+			found |= 1 << 0;
+		if (!strcmp(opts[i]->long_name, "recurse-submodules"))
+			found |= 1 << 1;
+	}
+
+	return found != 3;
+}
+
 static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 {
 	static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -905,14 +928,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct remote *remote;
 	int err = 0, complete_refs_before_fetch = 1;
 	int submodule_progress;
+	struct parseopt_options parseopts = { 0 };
 
 	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 
 	fetch_if_missing = 0;
 
 	packet_trace_identity("clone");
-	argc = parse_options(argc, argv, prefix, builtin_clone_options,
-			     builtin_clone_usage, 0);
+	parseopts.usagestr = builtin_clone_usage;
+	parseopts.is_abbrev_ambiguous = is_abbrev_ambiguous;
+	argc = parse_options_extended(argc, argv, prefix,
+				      builtin_clone_options,
+				      &parseopts);
 
 	if (argc > 2)
 		usage_msg_opt(_("Too many arguments."),
diff --git a/parse-options.c b/parse-options.c
index cec74522e5..c0354e5a92 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(
 			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
 			    !strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
-				if (abbrev_option) {
+				if (abbrev_option &&
+				    p->is_abbrev_ambiguous(abbrev_option, options)) {
 					/*
 					 * If this is abbreviated, it is
 					 * ambiguous. So when there is no
@@ -450,6 +451,12 @@ static void parse_options_check(const struct option *opts)
 		exit(128);
 }
 
+static int abbrev_is_always_ambiguous(const struct option *prev,
+				      const struct option *next)
+{
+	return 1;
+}
+
 void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 int argc, const char **argv, const char *prefix,
 			 const struct option *options, int flags)
@@ -466,6 +473,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->prefix = prefix;
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
+	ctx->is_abbrev_ambiguous = abbrev_is_always_ambiguous;
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
 	    (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&
 	    !(flags & PARSE_OPT_ONE_SHOT))
@@ -702,13 +710,17 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 	return ctx->cpidx + ctx->argc;
 }
 
-int parse_options(int argc, const char **argv, const char *prefix,
-		  const struct option *options, const char * const usagestr[],
-		  int flags)
+int parse_options_extended(int argc, const char **argv, const char *prefix,
+			   const struct option *options,
+			   const struct parseopt_options *parseopts)
 {
 	struct parse_opt_ctx_t ctx;
+	const char * const * usagestr = parseopts->usagestr;
+	int flags = parseopts->flags;
 
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
+	if (parseopts->is_abbrev_ambiguous)
+		ctx.is_abbrev_ambiguous = parseopts->is_abbrev_ambiguous;
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 	case PARSE_OPT_ERROR:
@@ -734,6 +746,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	return parse_options_end(&ctx);
 }
 
+int parse_options(int argc, const char **argv, const char *prefix,
+		  const struct option *options, const char * const usagestr[],
+		  int flags)
+{
+	struct parseopt_options parseopts = {0};
+
+	parseopts.flags = flags;
+	parseopts.usagestr = usagestr;
+	return parse_options_extended(argc, argv, prefix, options, &parseopts);
+}
+
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
diff --git a/parse-options.h b/parse-options.h
index 74cce4e7fc..31ebf4572c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -198,6 +198,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
 		  const struct option *options,
 		  const char * const usagestr[], int flags);
 
+struct parseopt_options {
+	int flags;
+	const char * const * usagestr;
+	int (*is_abbrev_ambiguous)(const struct option *prev,
+				   const struct option *next);
+};
+
+int parse_options_extended(int argc, const char **argv, const char *prefix,
+			   const struct option *options,
+			   const struct parseopt_options *parseopts);
+
 NORETURN void usage_with_options(const char * const *usagestr,
 				 const struct option *options);
 
@@ -256,6 +267,7 @@ struct parse_opt_ctx_t {
 	const char *opt;
 	int flags;
 	const char *prefix;
+	int (*is_abbrev_ambiguous)(const struct option *, const struct option *);
 };
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,
-- 8< --

> 
> Thanks.

Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> So an alternative is simply outsource the ambiguity decision back to
> git-clone. If the same situation appears again elsewhere, we'll need
> to sit back and fix it for real. But this way we don't potentially
> introduce any new traps.

Sounds like a sensibly safe approach.

> +/*
> + * Avoid ambiguation error between --recursive and --recurse-submodule
> + * because they are the same. --recurs can be expanded to any of them
> + * and it still works.
> + */
> +static int is_abbrev_ambiguous(const struct option *prev,
> +			       const struct option *next)
> +{
> +	const struct option *opts[] = { prev, next };

By looking at its caller, I think the caller keeps track of the
candidate it saw so far, and ask this function to see if the one it
is looking at right now (i.e. "this one") should be flagged as
conflicting with the other one (or "the other one").  So it probably
makes more sense to call them <it, the_other> or <it, prev> [*1*].

	Side note: *1* I would have used "this" instead of "it", but
	I vaguely recall there are those who want to use C++ aware
	static checkers and avoiding the identifier "this" is easy
	enough so ...

> +	int i, found = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(opts); i++) {
> +		if (!opts[i]->long_name)
> +			continue;
> +		if (!strcmp(opts[i]->long_name, "recursive"))
> +			found |= 1 << 0;
> +		if (!strcmp(opts[i]->long_name, "recurse-submodules"))
> +			found |= 1 << 1;
> +	}
> +
> +	return found != 3;
> +}

For any two options that share the prefix, unless they are
"recursive" and "recurse-submodules" pair, we say "that's
ambiguous".  But when they are these two specifically singled out,
we say it is OK.  Makes sense.

The above may be sufficient for this purpose, but I would have
expected that these groups of aliases would be the ones in the
table, not the <it, the_other> pair.  IOW, with helpers like these:

	static const char *recurse_submodules[] = {
		"recurse-submodules", "recursive", NULL,
	};

	static struct {
		const char **aliases;
	} disamb[] = {
		recurse_submodules,
	};
		
	static int has_string(const char *it, const char **array)
	{
		while (*array)
			if (!strcmp(it, *(array++)))
				return 1;
		return 0;
	}

the body would look something like:

	int j;

	for (j = 0; j < ARRAY_SIZE(disamb); j++) {
		/* it and other are from the same family? */
		if (it->long_name &&
		    has_string(it->long_name, disamb[j].aliases) &&
		    other->long_name &&
		    has_string(other->long_name, disamb[j].aliases))
			return 0;
	}
	return 1;

Perhaps?

I suspect that renaming the function (and the field in the context
struct) to .is_alias() and reverse the polarity of its return value
may make the flow of the logic at the caller side easier to follow?

> diff --git a/parse-options.c b/parse-options.c
> index cec74522e5..c0354e5a92 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(
>  			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
>  			    !strncmp(long_name, arg, arg_end - arg)) {
>  is_abbreviated:
> -				if (abbrev_option) {
> +				if (abbrev_option &&
> +				    p->is_abbrev_ambiguous(abbrev_option, options)) {

The above suggestion would make the guard here to

			if (abbrev_option &&
			    !p->is_alias(abbrev_option, options)) {

That is, at this point, we know that the user may have used the
given string to name the "abbrev_option" we earlier saw (and made
sure it prefix matches the string), and now we are looking at
another one, options[0], that also prefix matches the string.  We
usually flag this case as a problematic ambiguity inside the body of
this if statement, but if one is an alias to the other, we do not do
so.

>  					/*
>  					 * If this is abbreviated, it is
>  					 * ambiguous. So when there is no

[PATCH] parse-options: don't emit "ambiguous option" for aliases

[Nguyễn Thái Ngọc Duy]

Change the option parsing machinery so that e.g. "clone --recurs ..."
doesn't error out because "clone" understands both "--recursive" and
"--recurse-submodules" to mean the same thing.

Initially "clone" just understood --recursive until the
--recurses-submodules alias was added in ccdd3da652 ("clone: Add the
--recurse-submodules option as alias for --recursive",
2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to
optionally take a pathspec", 2017-03-17) the longer form has been
promoted to the default.

But due to the way the options parsing machinery works this resulted
in the rather absurd situation of:

    $ git clone --recurs [...]
    error: ambiguous option: recurs (could be --recursive or --recurse-submodules)

Add OPT_ALIAS() to express this linke between two options and use it in
git-clone.

A big chunk of code is actually from Junio C Hamano. The test case (and
most of the commit message) is from Ævar Arnfjörð Bjarmason.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I'm not sure if Junio is an evil genius who lures me with his helpful
 suggestions. Either way I ended up spending a couple hours writing and
 rewriting and finally decided all other options were too ugly. And his
 suggestions were mostly there already.
 
 So here's OPT_ALIAS(). I avoid rewriting all the option iteration code
 by simply making a copy of options[] array to use, then free it
 afterwards. Not the best, but at least it could be easily removed if we
 end up not want OPT_ALIAS() after all.

 This probably needs signoff from both Ævar and Junio, if accepted.

 builtin/clone.c          |   5 +-
 parse-options.c          | 130 +++++++++++++++++++++++++++++++++++++--
 parse-options.h          |   6 ++
 t/t0040-parse-options.sh |   5 ++
 4 files changed, 136 insertions(+), 10 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 50bde99618..703b7247ad 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = {
 		    N_("don't use local hardlinks, always copy")),
 	OPT_BOOL('s', "shared", &option_shared,
 		    N_("setup as shared repository")),
-	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
-	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	OPT_ALIAS(0, "recursive", "recurse-submodules"),
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
diff --git a/parse-options.c b/parse-options.c
index acc3a93660..7ec768e376 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -261,6 +261,35 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
 	return PARSE_OPT_UNKNOWN;
 }
 
+static int has_string(const char *it, const char **array)
+{
+	while (*array)
+		if (!strcmp(it, *(array++)))
+			return 1;
+	return 0;
+}
+
+static int is_alias(struct parse_opt_ctx_t *ctx,
+		    const struct option *one_opt,
+		    const struct option *another_opt)
+{
+	const char **group;
+
+	if (!ctx->alias_groups)
+		return 0;
+
+	if (!one_opt->long_name || !another_opt->long_name)
+		return 0;
+
+	for (group = ctx->alias_groups; *group; group += 3) {
+		/* it and other are from the same family? */
+		if (has_string(one_opt->long_name, group) &&
+		    has_string(another_opt->long_name, group))
+			return 1;
+	}
+	return 0;
+}
+
 static enum parse_opt_result parse_long_opt(
 	struct parse_opt_ctx_t *p, const char *arg,
 	const struct option *options)
@@ -296,7 +325,8 @@ static enum parse_opt_result parse_long_opt(
 			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
 			    !strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
-				if (abbrev_option) {
+				if (abbrev_option &&
+				    !is_alias(p, abbrev_option, options)) {
 					/*
 					 * If this is abbreviated, it is
 					 * ambiguous. So when there is no
@@ -445,6 +475,10 @@ static void parse_options_check(const struct option *opts)
 			if (opts->callback)
 				BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
 			break;
+		case OPTION_ALIAS:
+			BUG("OPT_ALIAS() should not remain at this point. "
+			    "Are you using parse_options_step() directly?\n"
+			    "That case is not supported yet.");
 		default:
 			; /* ok. (usually accepts an argument) */
 		}
@@ -456,11 +490,10 @@ static void parse_options_check(const struct option *opts)
 		exit(128);
 }
 
-void parse_options_start(struct parse_opt_ctx_t *ctx,
-			 int argc, const char **argv, const char *prefix,
-			 const struct option *options, int flags)
+static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
+				  int argc, const char **argv, const char *prefix,
+				  const struct option *options, int flags)
 {
-	memset(ctx, 0, sizeof(*ctx));
 	ctx->argc = argc;
 	ctx->argv = argv;
 	if (!(flags & PARSE_OPT_ONE_SHOT)) {
@@ -482,6 +515,14 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	parse_options_check(options);
 }
 
+void parse_options_start(struct parse_opt_ctx_t *ctx,
+			 int argc, const char **argv, const char *prefix,
+			 const struct option *options, int flags)
+{
+	memset(ctx, 0, sizeof(*ctx));
+	parse_options_start_1(ctx, argc, argv, prefix, options, flags);
+}
+
 static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
 {
 	int printed_dashdash = 0;
@@ -574,6 +615,70 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,
 	return PARSE_OPT_COMPLETE;
 }
 
+/*
+ * Scan and may produce a new option[] array, which should be used
+ * instead of the original 'options'.
+ *
+ * Right now this is only used to preprocess and substitue
+ * OPTION_ALIAS.
+ */
+static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
+					 const struct option *options)
+{
+	struct option *newopt;
+	int i, nr, alias;
+	int nr_aliases = 0;
+
+	for (nr = 0; options[nr].type != OPTION_END; nr++) {
+		if (options[nr].type == OPTION_ALIAS)
+			nr_aliases++;
+	}
+
+	if (!nr_aliases)
+		return NULL;
+
+	ALLOC_ARRAY(newopt, nr + 1);
+	COPY_ARRAY(newopt, options, nr + 1);
+
+	/* each alias has two string pointers and NULL */
+	CALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));
+
+	for (alias = 0, i = 0; i < nr; i++) {
+		const char *source;
+		int j;
+
+		if (newopt[i].type != OPTION_ALIAS)
+			continue;
+
+		if (!newopt[i].long_name)
+			BUG("An alias must have long option name");
+
+		source = newopt[i].value;
+
+		for (j = 0; j < nr; j++) {
+			const char *name = options[j].long_name;
+
+			if (!name || strcmp(name, source))
+				continue;
+
+			if (options[j].type == OPTION_ALIAS)
+				BUG("No please. Nested aliases are not supported.");
+
+			memcpy(newopt + i, options + j, sizeof(*newopt));
+			break;
+		}
+
+		if (j == nr)
+			BUG("could not find source option '%s' of alias '%s'",
+			    source, newopt[i].long_name);
+		ctx->alias_groups[alias * 3 + 0] = newopt[i].long_name;
+		ctx->alias_groups[alias * 3 + 1] = options[j].long_name;
+		alias++;
+	}
+
+	return newopt;
+}
+
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
 				       const char * const *,
 				       const struct option *, int, int);
@@ -713,11 +818,16 @@ int parse_options(int argc, const char **argv, const char *prefix,
 		  int flags)
 {
 	struct parse_opt_ctx_t ctx;
+	struct option *real_options;
 
 	disallow_abbreviated_options =
 		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
 
-	parse_options_start(&ctx, argc, argv, prefix, options, flags);
+	memset(&ctx, 0, sizeof(ctx));
+	real_options = preprocess_options(&ctx, options);
+	if (real_options)
+		options = real_options;
+	parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 	case PARSE_OPT_ERROR:
@@ -740,6 +850,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	}
 
 	precompose_argv(argc, argv);
+	free(real_options);
+	free(ctx.alias_groups);
 	return parse_options_end(&ctx);
 }
 
@@ -834,6 +946,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 			fputc('\n', outfile);
 			pad = USAGE_OPTS_WIDTH;
 		}
+		if (opts->type == OPTION_ALIAS) {
+			fprintf(outfile, "%*s", pad + USAGE_GAP, "");
+			fprintf_ln(outfile, _("alias of --%s"),
+				   (const char *)opts->value);
+			continue;
+		}
 		fprintf(outfile, "%*s%s\n", pad + USAGE_GAP, "", _(opts->help));
 	}
 	fputc('\n', outfile);
diff --git a/parse-options.h b/parse-options.h
index 74cce4e7fc..e6b4a8d1c7 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -7,6 +7,7 @@ enum parse_opt_type {
 	OPTION_ARGUMENT,
 	OPTION_GROUP,
 	OPTION_NUMBER,
+	OPTION_ALIAS,
 	/* options with no arguments */
 	OPTION_BIT,
 	OPTION_NEGBIT,
@@ -181,6 +182,9 @@ struct option {
 	  N_("no-op (backward compatibility)"),		\
 	  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }
 
+#define OPT_ALIAS(s, l, alias_source) \
+	{ OPTION_ALIAS, (s), (l), (alias_source) }
+
 /*
  * parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[]. argv0 is assumed program name and
@@ -256,6 +260,8 @@ struct parse_opt_ctx_t {
 	const char *opt;
 	int flags;
 	const char *prefix;
+	const char **alias_groups; /* must be in groups of 3 elements! */
+	struct option *updated_options;
 };
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 800b3ea5f5..14e6664240 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -224,6 +224,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '
+	test_when_finished "rm -rf A" &&
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false git clone --recurs . A
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF
-- 
2.21.0.854.ge34a79f761

Re: [PATCH] parse-options: don't emit "ambiguous option" for aliases

[Duy Nguyen]

On Mon, Apr 22, 2019 at 7:23 PM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> @@ -574,6 +615,70 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,
>         return PARSE_OPT_COMPLETE;
>  }
>
> +/*
> + * Scan and may produce a new option[] array, which should be used
> + * instead of the original 'options'.
> + *
> + * Right now this is only used to preprocess and substitue
> + * OPTION_ALIAS.
> + */
> +static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
> +                                        const struct option *options)
> +{
> +       struct option *newopt;
> +       int i, nr, alias;
> +       int nr_aliases = 0;
> +
> +       for (nr = 0; options[nr].type != OPTION_END; nr++) {
> +               if (options[nr].type == OPTION_ALIAS)
> +                       nr_aliases++;
> +       }
> +
> +       if (!nr_aliases)
> +               return NULL;
> +
> +       ALLOC_ARRAY(newopt, nr + 1);
> +       COPY_ARRAY(newopt, options, nr + 1);
> +
> +       /* each alias has two string pointers and NULL */
> +       CALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));
> +
> +       for (alias = 0, i = 0; i < nr; i++) {
> +               const char *source;
> +               int j;
> +
> +               if (newopt[i].type != OPTION_ALIAS)
> +                       continue;
> +
> +               if (!newopt[i].long_name)
> +                       BUG("An alias must have long option name");
> +
> +               source = newopt[i].value;
> +
> +               for (j = 0; j < nr; j++) {
> +                       const char *name = options[j].long_name;
> +
> +                       if (!name || strcmp(name, source))
> +                               continue;
> +
> +                       if (options[j].type == OPTION_ALIAS)
> +                               BUG("No please. Nested aliases are not supported.");
> +
> +                       memcpy(newopt + i, options + j, sizeof(*newopt));

Eck. I need to restore newopt[i].long_name and .short_name after this
memcpy, or we'll just have two --recurse-submodules instead. Luckily
it still works after this bug is gone.

> +                       break;
> +               }
-- 
Duy

[PATCH v2] parse-options: don't emit "ambiguous option" for aliases

[Nguyễn Thái Ngọc Duy]

Change the option parsing machinery so that e.g. "clone --recurs ..."
doesn't error out because "clone" understands both "--recursive" and
"--recurse-submodules" to mean the same thing.

Initially "clone" just understood --recursive until the
--recurses-submodules alias was added in ccdd3da652 ("clone: Add the
--recurse-submodules option as alias for --recursive",
2010-11-04). Since bb62e0a99f ("clone: teach --recurse-submodules to
optionally take a pathspec", 2017-03-17) the longer form has been
promoted to the default.

But due to the way the options parsing machinery works this resulted
in the rather absurd situation of:

    $ git clone --recurs [...]
    error: ambiguous option: recurs (could be --recursive or --recurse-submodules)

Add OPT_ALIAS() to express this link between two or more options and use
it in git-clone. Multiple aliases of an option could be written as

    OPT_ALIAS(0, "alias1", "original-name"),
    OPT_ALIAS(0, "alias2", "original-name"),
    ...

The current implementation is not exactly optimal in this case. But we
can optimize it when it becomes a problem. So far we don't even have two
aliases of any option.

A big chunk of code is actually from Junio C Hamano.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 OK it's working for real this time. test-parse-options.c is also
 updated to for testing OPT_ALIAS.

 builtin/clone.c               |   5 +-
 parse-options.c               | 143 ++++++++++++++++++++++++++++++++--
 parse-options.h               |   6 ++
 t/helper/test-parse-options.c |   3 +
 t/t0040-parse-options.sh      |  17 ++++
 5 files changed, 164 insertions(+), 10 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 50bde99618..703b7247ad 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = {
 		    N_("don't use local hardlinks, always copy")),
 	OPT_BOOL('s', "shared", &option_shared,
 		    N_("setup as shared repository")),
-	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
-	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	OPT_ALIAS(0, "recursive", "recurse-submodules"),
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },
diff --git a/parse-options.c b/parse-options.c
index acc3a93660..1b1cc2add7 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -261,6 +261,35 @@ static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p,
 	return PARSE_OPT_UNKNOWN;
 }
 
+static int has_string(const char *it, const char **array)
+{
+	while (*array)
+		if (!strcmp(it, *(array++)))
+			return 1;
+	return 0;
+}
+
+static int is_alias(struct parse_opt_ctx_t *ctx,
+		    const struct option *one_opt,
+		    const struct option *another_opt)
+{
+	const char **group;
+
+	if (!ctx->alias_groups)
+		return 0;
+
+	if (!one_opt->long_name || !another_opt->long_name)
+		return 0;
+
+	for (group = ctx->alias_groups; *group; group += 3) {
+		/* it and other are from the same family? */
+		if (has_string(one_opt->long_name, group) &&
+		    has_string(another_opt->long_name, group))
+			return 1;
+	}
+	return 0;
+}
+
 static enum parse_opt_result parse_long_opt(
 	struct parse_opt_ctx_t *p, const char *arg,
 	const struct option *options)
@@ -296,7 +325,8 @@ static enum parse_opt_result parse_long_opt(
 			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
 			    !strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
-				if (abbrev_option) {
+				if (abbrev_option &&
+				    !is_alias(p, abbrev_option, options)) {
 					/*
 					 * If this is abbreviated, it is
 					 * ambiguous. So when there is no
@@ -445,6 +475,10 @@ static void parse_options_check(const struct option *opts)
 			if (opts->callback)
 				BUG("OPTION_LOWLEVEL_CALLBACK needs no high level callback");
 			break;
+		case OPTION_ALIAS:
+			BUG("OPT_ALIAS() should not remain at this point. "
+			    "Are you using parse_options_step() directly?\n"
+			    "That case is not supported yet.");
 		default:
 			; /* ok. (usually accepts an argument) */
 		}
@@ -456,11 +490,10 @@ static void parse_options_check(const struct option *opts)
 		exit(128);
 }
 
-void parse_options_start(struct parse_opt_ctx_t *ctx,
-			 int argc, const char **argv, const char *prefix,
-			 const struct option *options, int flags)
+static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
+				  int argc, const char **argv, const char *prefix,
+				  const struct option *options, int flags)
 {
-	memset(ctx, 0, sizeof(*ctx));
 	ctx->argc = argc;
 	ctx->argv = argv;
 	if (!(flags & PARSE_OPT_ONE_SHOT)) {
@@ -482,6 +515,14 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	parse_options_check(options);
 }
 
+void parse_options_start(struct parse_opt_ctx_t *ctx,
+			 int argc, const char **argv, const char *prefix,
+			 const struct option *options, int flags)
+{
+	memset(ctx, 0, sizeof(*ctx));
+	parse_options_start_1(ctx, argc, argv, prefix, options, flags);
+}
+
 static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
 {
 	int printed_dashdash = 0;
@@ -574,6 +615,83 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,
 	return PARSE_OPT_COMPLETE;
 }
 
+/*
+ * Scan and may produce a new option[] array, which should be used
+ * instead of the original 'options'.
+ *
+ * Right now this is only used to preprocess and substitue
+ * OPTION_ALIAS.
+ */
+static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
+					 const struct option *options)
+{
+	struct option *newopt;
+	int i, nr, alias;
+	int nr_aliases = 0;
+
+	for (nr = 0; options[nr].type != OPTION_END; nr++) {
+		if (options[nr].type == OPTION_ALIAS)
+			nr_aliases++;
+	}
+
+	if (!nr_aliases)
+		return NULL;
+
+	ALLOC_ARRAY(newopt, nr + 1);
+	COPY_ARRAY(newopt, options, nr + 1);
+
+	/* each alias has two string pointers and NULL */
+	CALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));
+
+	for (alias = 0, i = 0; i < nr; i++) {
+		int short_name;
+		const char *long_name;
+		const char *source;
+		int j;
+
+		if (newopt[i].type != OPTION_ALIAS)
+			continue;
+
+		short_name = newopt[i].short_name;
+		long_name = newopt[i].long_name;
+		source = newopt[i].value;
+
+		if (!long_name)
+			BUG("An alias must have long option name");
+
+		for (j = 0; j < nr; j++) {
+			const char *name = options[j].long_name;
+
+			if (!name || strcmp(name, source))
+				continue;
+
+			if (options[j].type == OPTION_ALIAS)
+				BUG("No please. Nested aliases are not supported.");
+
+			/*
+			 * NEEDSWORK: this is a bit inconsistent because
+			 * usage_with_options() on the original options[] will print
+			 * help string as "alias of %s" but "git cmd -h" will
+			 * print the original help string.
+			 */
+			memcpy(newopt + i, options + j, sizeof(*newopt));
+			newopt[i].short_name = short_name;
+			newopt[i].long_name = long_name;
+			break;
+		}
+
+		if (j == nr)
+			BUG("could not find source option '%s' of alias '%s'",
+			    source, newopt[i].long_name);
+		ctx->alias_groups[alias * 3 + 0] = newopt[i].long_name;
+		ctx->alias_groups[alias * 3 + 1] = options[j].long_name;
+		ctx->alias_groups[alias * 3 + 2] = NULL;
+		alias++;
+	}
+
+	return newopt;
+}
+
 static int usage_with_options_internal(struct parse_opt_ctx_t *,
 				       const char * const *,
 				       const struct option *, int, int);
@@ -713,11 +831,16 @@ int parse_options(int argc, const char **argv, const char *prefix,
 		  int flags)
 {
 	struct parse_opt_ctx_t ctx;
+	struct option *real_options;
 
 	disallow_abbreviated_options =
 		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
 
-	parse_options_start(&ctx, argc, argv, prefix, options, flags);
+	memset(&ctx, 0, sizeof(ctx));
+	real_options = preprocess_options(&ctx, options);
+	if (real_options)
+		options = real_options;
+	parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 	case PARSE_OPT_ERROR:
@@ -740,6 +863,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	}
 
 	precompose_argv(argc, argv);
+	free(real_options);
+	free(ctx.alias_groups);
 	return parse_options_end(&ctx);
 }
 
@@ -834,6 +959,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 			fputc('\n', outfile);
 			pad = USAGE_OPTS_WIDTH;
 		}
+		if (opts->type == OPTION_ALIAS) {
+			fprintf(outfile, "%*s", pad + USAGE_GAP, "");
+			fprintf_ln(outfile, _("alias of --%s"),
+				   (const char *)opts->value);
+			continue;
+		}
 		fprintf(outfile, "%*s%s\n", pad + USAGE_GAP, "", _(opts->help));
 	}
 	fputc('\n', outfile);
diff --git a/parse-options.h b/parse-options.h
index 74cce4e7fc..9ed479f41d 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -7,6 +7,7 @@ enum parse_opt_type {
 	OPTION_ARGUMENT,
 	OPTION_GROUP,
 	OPTION_NUMBER,
+	OPTION_ALIAS,
 	/* options with no arguments */
 	OPTION_BIT,
 	OPTION_NEGBIT,
@@ -181,6 +182,9 @@ struct option {
 	  N_("no-op (backward compatibility)"),		\
 	  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }
 
+#define OPT_ALIAS(s, l, source_long_name) \
+	{ OPTION_ALIAS, (s), (l), (source_long_name) }
+
 /*
  * parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[]. argv0 is assumed program name and
@@ -256,6 +260,8 @@ struct parse_opt_ctx_t {
 	const char *opt;
 	int flags;
 	const char *prefix;
+	const char **alias_groups; /* must be in groups of 3 elements! */
+	struct option *updated_options;
 };
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index cc88fba057..76cfb87588 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -149,6 +149,9 @@ int cmd__parse_options(int argc, const char **argv)
 		OPT_CALLBACK(0, "expect", &expect, "string",
 			     "expected output in the variable dump",
 			     collect_expect),
+		OPT_GROUP("Alias"),
+		OPT_STRING('A', "alias-source", &string, "string", "get a string"),
+		OPT_ALIAS('Z', "alias-target", "alias-source"),
 		OPT_END(),
 	};
 	int i;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 800b3ea5f5..cebc77fab0 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -48,6 +48,12 @@ Standard options
     -q, --quiet           be quiet
     --expect <string>     expected output in the variable dump
 
+Alias
+    -A, --alias-source <string>
+                          get a string
+    -Z, --alias-target <string>
+                          get a string
+
 EOF
 
 test_expect_success 'test help' '
@@ -224,6 +230,17 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'Alias options do not contribute to abbreviation' '
+	test-tool parse-options --alias-source 123 >output &&
+	grep "^string: 123" output &&
+	test-tool parse-options --alias-target 123 >output &&
+	grep "^string: 123" output &&
+	test_must_fail test-tool parse-options --alias &&
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --alias 123 >output &&
+	grep "^string: 123" output
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF
-- 
2.21.0.1110.g9614c01b33

Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> But due to the way the options parsing machinery works this resulted
> in the rather absurd situation of:
>
>     $ git clone --recurs [...]
>     error: ambiguous option: recurs (could be --recursive or --recurse-submodules)
>
> Add OPT_ALIAS() to express this link between two or more options and use
> it in git-clone. Multiple aliases of an option could be written as
>
>     OPT_ALIAS(0, "alias1", "original-name"),
>     OPT_ALIAS(0, "alias2", "original-name"),
>     ...
>
> The current implementation is not exactly optimal in this case. But we
> can optimize it when it becomes a problem. So far we don't even have two
> aliases of any option.

Sounds good enough for any practical need I can forsee ;-)  Thanks.

> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index 800b3ea5f5..cebc77fab0 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -48,6 +48,12 @@ Standard options
>      -q, --quiet           be quiet
>      --expect <string>     expected output in the variable dump
>  
> +Alias
> +    -A, --alias-source <string>
> +                          get a string
> +    -Z, --alias-target <string>
> +                          get a string
> +
>  EOF

This is not a new problem per-se, as there already is a line before
the precontext of this hunk that shares the same issue, but to
prevent future problems, I am very much tempted to apply the
attached on top.

-- >8 --
Subject: t0040: protect lines that are indented by spaces

This block is byte-for-byte identical expected output, that contains a
few lines that are indented in many spaces, which makes "git diff --check"
unhappy and will break the test when "git am --whitespace=fix" is
allowed to "correct" them.

Protect the left edge with a marker character, and strip it with
sed, which is used as a standard way to deal with this issue in our
test suite.

Signed-off-by: Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Of course, if the right-edge need to be protected, we can do so
   as well.

 t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------
 1 file changed, 47 insertions(+), 47 deletions(-)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index cebc77fab0..26373b5b72 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -7,53 +7,53 @@ test_description='our own option parser'
 
 . ./test-lib.sh
 
-cat >expect <<\EOF
-usage: test-tool parse-options <options>
-
-    A helper function for the parse-options API.
-
-    --yes                 get a boolean
-    -D, --no-doubt        begins with 'no-'
-    -B, --no-fear         be brave
-    -b, --boolean         increment by one
-    -4, --or4             bitwise-or boolean with ...0100
-    --neg-or4             same as --no-or4
-
-    -i, --integer <n>     get a integer
-    -j <n>                get a integer, too
-    -m, --magnitude <n>   get a magnitude
-    --set23               set integer to 23
-    -L, --length <str>    get length of <str>
-    -F, --file <file>     set file to <file>
-
-String options
-    -s, --string <string>
-                          get a string
-    --string2 <str>       get another string
-    --st <st>             get another string (pervert ordering)
-    -o <str>              get another string
-    --list <str>          add str to list
-
-Magic arguments
-    --quux                means --quux
-    -NUM                  set integer to NUM
-    +                     same as -b
-    --ambiguous           positive ambiguity
-    --no-ambiguous        negative ambiguity
-
-Standard options
-    --abbrev[=<n>]        use <n> digits to display SHA-1s
-    -v, --verbose         be verbose
-    -n, --dry-run         dry run
-    -q, --quiet           be quiet
-    --expect <string>     expected output in the variable dump
-
-Alias
-    -A, --alias-source <string>
-                          get a string
-    -Z, --alias-target <string>
-                          get a string
-
+sed -e 's/^|//' >expect <<\EOF
+|usage: test-tool parse-options <options>
+|
+|    A helper function for the parse-options API.
+|
+|    --yes                 get a boolean
+|    -D, --no-doubt        begins with 'no-'
+|    -B, --no-fear         be brave
+|    -b, --boolean         increment by one
+|    -4, --or4             bitwise-or boolean with ...0100
+|    --neg-or4             same as --no-or4
+|
+|    -i, --integer <n>     get a integer
+|    -j <n>                get a integer, too
+|    -m, --magnitude <n>   get a magnitude
+|    --set23               set integer to 23
+|    -L, --length <str>    get length of <str>
+|    -F, --file <file>     set file to <file>
+|
+|String options
+|    -s, --string <string>
+|                          get a string
+|    --string2 <str>       get another string
+|    --st <st>             get another string (pervert ordering)
+|    -o <str>              get another string
+|    --list <str>          add str to list
+|
+|Magic arguments
+|    --quux                means --quux
+|    -NUM                  set integer to NUM
+|    +                     same as -b
+|    --ambiguous           positive ambiguity
+|    --no-ambiguous        negative ambiguity
+|
+|Standard options
+|    --abbrev[=<n>]        use <n> digits to display SHA-1s
+|    -v, --verbose         be verbose
+|    -n, --dry-run         dry run
+|    -q, --quiet           be quiet
+|    --expect <string>     expected output in the variable dump
+|
+|Alias
+|    -A, --alias-source <string>
+|                          get a string
+|    -Z, --alias-target <string>
+|                          get a string
+|
 EOF
 
 test_expect_success 'test help' '

Re: [PATCH v2] parse-options: don't emit "ambiguous option" for aliases

[Duy Nguyen]

On Tue, May 7, 2019 at 10:43 AM Junio C Hamano <gitster@pobox.com> wrote:
> -- >8 --
> Subject: t0040: protect lines that are indented by spaces
>
> This block is byte-for-byte identical expected output, that contains a
> few lines that are indented in many spaces, which makes "git diff --check"
> unhappy and will break the test when "git am --whitespace=fix" is
> allowed to "correct" them.
>
> Protect the left edge with a marker character, and strip it with
> sed, which is used as a standard way to deal with this issue in our
> test suite.
>
> Signed-off-by: Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  * Of course, if the right-edge need to be protected, we can do so
>    as well.
>
>  t/t0040-parse-options.sh | 94 ++++++++++++++++++++++++------------------------
>  1 file changed, 47 insertions(+), 47 deletions(-)
>
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index cebc77fab0..26373b5b72 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -7,53 +7,53 @@ test_description='our own option parser'
>
>  . ./test-lib.sh
>
> -cat >expect <<\EOF
> -usage: test-tool parse-options <options>
> -
> -    A helper function for the parse-options API.
> -
> -    --yes                 get a boolean
> -    -D, --no-doubt        begins with 'no-'
> -    -B, --no-fear         be brave
> -    -b, --boolean         increment by one
> -    -4, --or4             bitwise-or boolean with ...0100
> -    --neg-or4             same as --no-or4
> -
> -    -i, --integer <n>     get a integer
> -    -j <n>                get a integer, too
> -    -m, --magnitude <n>   get a magnitude
> -    --set23               set integer to 23
> -    -L, --length <str>    get length of <str>
> -    -F, --file <file>     set file to <file>
> -
> -String options
> -    -s, --string <string>
> -                          get a string
> -    --string2 <str>       get another string
> -    --st <st>             get another string (pervert ordering)
> -    -o <str>              get another string
> -    --list <str>          add str to list
> -
> -Magic arguments
> -    --quux                means --quux
> -    -NUM                  set integer to NUM
> -    +                     same as -b
> -    --ambiguous           positive ambiguity
> -    --no-ambiguous        negative ambiguity
> -
> -Standard options
> -    --abbrev[=<n>]        use <n> digits to display SHA-1s
> -    -v, --verbose         be verbose
> -    -n, --dry-run         dry run
> -    -q, --quiet           be quiet
> -    --expect <string>     expected output in the variable dump
> -
> -Alias
> -    -A, --alias-source <string>
> -                          get a string
> -    -Z, --alias-target <string>
> -                          get a string
> -
> +sed -e 's/^|//' >expect <<\EOF

I think we already use qz_to_tab_space to protect leading spaces
(t1450) or trailing ones (t4205). It's less strict than this though.

Anyway, if you go with 's/^|//', maybe make it a helper function too
because I'm pretty sure we have more text like this in the test suite.

There's also the "tr -d Q" trick in t4038. But that's something to
clean up if someone really have free time.

> +|usage: test-tool parse-options <options>
> +|
> +|    A helper function for the parse-options API.
> +|
> +|    --yes                 get a boolean
> +|    -D, --no-doubt        begins with 'no-'
> +|    -B, --no-fear         be brave
> +|    -b, --boolean         increment by one
> +|    -4, --or4             bitwise-or boolean with ...0100
> +|    --neg-or4             same as --no-or4
> +|
> +|    -i, --integer <n>     get a integer
> +|    -j <n>                get a integer, too
> +|    -m, --magnitude <n>   get a magnitude
> +|    --set23               set integer to 23
> +|    -L, --length <str>    get length of <str>
> +|    -F, --file <file>     set file to <file>
> +|
> +|String options
> +|    -s, --string <string>
> +|                          get a string
> +|    --string2 <str>       get another string
> +|    --st <st>             get another string (pervert ordering)
> +|    -o <str>              get another string
> +|    --list <str>          add str to list
> +|
> +|Magic arguments
> +|    --quux                means --quux
> +|    -NUM                  set integer to NUM
> +|    +                     same as -b
> +|    --ambiguous           positive ambiguity
> +|    --no-ambiguous        negative ambiguity
> +|
> +|Standard options
> +|    --abbrev[=<n>]        use <n> digits to display SHA-1s
> +|    -v, --verbose         be verbose
> +|    -n, --dry-run         dry run
> +|    -q, --quiet           be quiet
> +|    --expect <string>     expected output in the variable dump
> +|
> +|Alias
> +|    -A, --alias-source <string>
> +|                          get a string
> +|    -Z, --alias-target <string>
> +|                          get a string
> +|
>  EOF
>
>  test_expect_success 'test help' '



-- 
Duy