Do not use abbreviated options in tests

PATCH Superseded Johannes Schindelin via GitGitGadget 7 participants 52 messages Mar 25, 2019 – May 7, 2019 Documentation

lore

Share & tools

lore

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #0 lore raw

cover Do not use abbreviated options in tests

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

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

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

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

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

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #1 lore raw

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

From: Johannes Schindelin <[email protected]>

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

However, Denton Liu contributed a patch series in
https://public-inbox.org/git/[email protected]/
that introduces a new git rebase option called --keep-base, which makes this previously unique abbreviation non-unique.

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

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t5407-post-rewrite-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t5407-post-rewrite-hook.sh +2 -2

index a4a5903cba..7344253bfb 100755
@@ -131,7 +131,7 @@ test_expect_success 'git rebase -m --skip' '
 test_expect_success 'git rebase with implicit use of interactive backend' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --keep --onto A B &&
+	test_must_fail git rebase --keep-empty --onto A B &&
 	echo C > foo &&
 	git add foo &&
 	git rebase --continue &&

@@ -146,7 +146,7 @@ test_expect_success 'git rebase with implicit use of interactive backend' '
 test_expect_success 'git rebase --skip with implicit use of interactive backend' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --keep --onto A B &&
+	test_must_fail git rebase --keep-empty --onto A B &&
 	test_must_fail git rebase --skip &&
 	echo D > foo &&
 	git add foo &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #3 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

This script used abbreviated options, which is unnecessarily fragile.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t7810-grep.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

t/t7810-grep.sh +8 -8

index 43aa4161cf..2e1bb61b41 100755
@@ -119,33 +119,33 @@ do
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep -w $L (with --column, --invert)" '
+	test_expect_success "grep -w $L (with --column, --invert-match)" '
 		{
 			echo ${HC}file:1:foo mmap bar
 			echo ${HC}file:1:foo_mmap bar
 			echo ${HC}file:1:foo_mmap bar mmap
 			echo ${HC}file:1:foo mmap bar_mmap
 		} >expected &&
-		git grep --column --invert -w -e baz $H -- file >actual &&
+		git grep --column --invert-match -w -e baz $H -- file >actual &&
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep $L (with --column, --invert, extended OR)" '
+	test_expect_success "grep $L (with --column, --invert-match, extended OR)" '
 		{
 			echo ${HC}hello_world:6:HeLLo_world
 		} >expected &&
-		git grep --column --invert -e ll --or --not -e _ $H -- hello_world \
+		git grep --column --invert-match -e ll --or --not -e _ $H -- hello_world \
 			>actual &&
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep $L (with --column, --invert, extended AND)" '
+	test_expect_success "grep $L (with --column, --invert-match, extended AND)" '
 		{
 			echo ${HC}hello_world:3:Hello world
 			echo ${HC}hello_world:3:Hello_world
 			echo ${HC}hello_world:6:HeLLo_world
 		} >expected &&
-		git grep --column --invert --not -e _ --and --not -e ll $H -- hello_world \
+		git grep --column --invert-match --not -e _ --and --not -e ll $H -- hello_world \
 			>actual &&
 		test_cmp expected actual
 	'

@@ -1010,7 +1010,7 @@ test_expect_success 'outside of git repository' '
 			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git grep --no-index --no-exclude o >../actual.full &&
+		git grep --no-index --no-exclude-standard o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '

@@ -1051,7 +1051,7 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
 			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git -c grep.fallbackToNoIndex grep --no-exclude o >../actual.full &&
+		git -c grep.fallbackToNoIndex grep --no-exclude-standard o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #5 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t5516-fetch-push.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t5516-fetch-push.sh +2 -2

index 37e8e80893..db0b1db458 100755
@@ -1370,7 +1370,7 @@ test_expect_success 'push does not follow tags by default' '
 	test_cmp expect actual
 '
 
-test_expect_success 'push --follow-tag only pushes relevant tags' '
+test_expect_success 'push --follow-tags only pushes relevant tags' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
 	git init src &&

@@ -1384,7 +1384,7 @@ test_expect_success 'push --follow-tag only pushes relevant tags' '
 		git tag -m "future" future &&
 		git checkout master &&
 		git for-each-ref refs/heads/master refs/tags/tag >../expect &&
-		git push --follow-tag ../dst master
+		git push --follow-tags ../dst master
 	) &&
 	(
 		cd dst &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #4 lore raw

PATCH 4/8 t5531: avoid using an abbreviated option

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t5531-deep-submodule-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5531-deep-submodule-push.sh +1 -1

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

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #2 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3415-rebase-autosquash.sh | 2 +-
 t/t3430-rebase-merges.sh     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

t/t3415-rebase-autosquash.sh +1 -1

index 13f5688135..22d218698e 100755
@@ -277,7 +277,7 @@ test_expect_success 'autosquash with empty custom instructionFormat' '
 	(
 		set_cat_todo_editor &&
 		test_must_fail git -c rebase.instructionFormat= \
-			rebase --autosquash  --force -i HEAD^ >actual &&
+			rebase --autosquash  --force-rebase -i HEAD^ >actual &&
 		git log -1 --format="pick %h %s" >expect &&
 		test_cmp expect actual
 	)

t/t3430-rebase-merges.sh +2 -2

index 4c69255ee6..42ba5b9f09 100755
@@ -271,7 +271,7 @@ test_expect_success 'root commits' '
 	EOF
 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
 	test_tick &&
-	git rebase -i --force --root -r &&
+	git rebase -i --force-rebase --root -r &&
 	test "Parsnip" = "$(git show -s --format=%an HEAD^)" &&
 	test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
 	test $(git rev-parse second-root:second-root.t) = \

@@ -364,7 +364,7 @@ test_expect_success 'octopus merges' '
 	test_cmp_rev HEAD $before &&
 
 	test_tick &&
-	git rebase -i --force -r HEAD^^ &&
+	git rebase -i --force-rebase -r HEAD^^ &&
 	test "Hank" = "$(git show -s --format=%an HEAD)" &&
 	test "$before" != $(git rev-parse HEAD) &&
 	test_cmp_graph HEAD^^.. <<-\EOF

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #6 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t7525-status-rename.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

t/t7525-status-rename.sh +4 -4

index ef8b1b3078..a62736dce0 100755
@@ -84,7 +84,7 @@ test_expect_success 'status score=100%' '
 	test_i18ngrep "deleted:" actual &&
 	test_i18ngrep "new file:" actual &&
 
-	git status --find-rename=100% >actual &&
+	git status --find-renames=100% >actual &&
 	test_i18ngrep "deleted:" actual &&
 	test_i18ngrep "new file:" actual
 '

@@ -93,11 +93,11 @@ test_expect_success 'status score=01%' '
 	git status -M=01% >actual &&
 	test_i18ngrep "renamed:" actual &&
 
-	git status --find-rename=01% >actual &&
+	git status --find-renames=01% >actual &&
 	test_i18ngrep "renamed:" actual
 '
 
-test_expect_success 'copies not overridden by find-rename' '
+test_expect_success 'copies not overridden by find-renames' '
 	cp renamed copy &&
 	git add copy &&

@@ -105,7 +105,7 @@ test_expect_success 'copies not overridden by find-rename' '
 	test_i18ngrep "copied:" actual &&
 	test_i18ngrep "renamed:" actual &&
 
-	git -c status.renames=copies status --find-rename=01% >actual &&
+	git -c status.renames=copies status --find-renames=01% >actual &&
 	test_i18ngrep "copied:" actual &&
 	test_i18ngrep "renamed:" actual
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #8 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

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

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

For example, if a future contribution added a new 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 <[email protected]>

---
 parse-options.c          | 9 +++++++++
 t/README                 | 4 ++++
 t/t0040-parse-options.sh | 7 ++++++-
 t/test-lib.sh            | 6 ++++++
 4 files changed, 25 insertions(+), 1 deletion(-)

parse-options.c +9 -0

index cec74522e5..acc3a93660 100644
@@ -6,6 +6,8 @@
 #include "color.h"
 #include "utf8.h"
 
+static int disallow_abbreviated_options;
+
 #define OPT_SHORT 1
 #define OPT_UNSET 2

@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
+	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+		die("disallowed abbreviated or ambiguous option '%.*s'",
+		    (int)(arg_end - arg), arg);
+
 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),

@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
+	disallow_abbreviated_options =
+		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
+
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:

t/README +4 -0

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

t/t0040-parse-options.sh +6 -1

index b8f366c442..5f6a16336d 100755
@@ -203,20 +203,24 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 test-tool parse-options --strin 123
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="string: 123" --st 123
 '

@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output

t/test-lib.sh +6 -0

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

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-03-25 18:14 UTC #7 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

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

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------
 1 file changed, 22 insertions(+), 22 deletions(-)

t/t5317-pack-objects-filter-objects.sh +22 ~~-22~~

index 24541ea137..4c0201c34b 100755
@@ -25,7 +25,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../all.pack &&

@@ -39,7 +39,7 @@ test_expect_success 'verify blob count in normal packfile' '
 '
 
 test_expect_success 'verify blob:none packfile has no blobs' '
-	git -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=blob:none >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../filter.pack &&

@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' '
 	git -C r5 commit -m "foo" &&
 	del=$(git -C r5 rev-parse HEAD^{tree} | sed "s|..|&/|") &&
 	rm r5/.git/objects/$del &&
-	test_must_fail git -C r5 pack-objects --rev --stdout 2>bad_tree <<-EOF &&
+	test_must_fail git -C r5 pack-objects --revs --stdout 2>bad_tree <<-EOF &&
 	HEAD
 	EOF
 	grep "bad tree object" bad_tree

@@ -88,7 +88,7 @@ test_expect_success 'setup for tests of tree:0' '
 '
 
 test_expect_success 'verify tree:0 packfile has no blobs or trees' '
-	git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../commitsonly.pack &&

@@ -98,7 +98,7 @@ test_expect_success 'verify tree:0 packfile has no blobs or trees' '
 
 test_expect_success 'grab tree directly when using tree:0' '
 	# We should get the tree specified directly but not its blobs or subtrees.
-	git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
 	HEAD:
 	EOF
 	git -C r1 index-pack ../commitsonly.pack &&

@@ -128,7 +128,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../all.pack &&

@@ -142,7 +142,7 @@ test_expect_success 'verify blob count in normal packfile' '
 '
 
 test_expect_success 'verify blob:limit=500 omits all blobs' '
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -157,7 +157,7 @@ test_expect_success 'verify blob:limit=500 omits all blobs' '
 '
 
 test_expect_success 'verify blob:limit=1000' '
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -176,7 +176,7 @@ test_expect_success 'verify blob:limit=1001' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -194,7 +194,7 @@ test_expect_success 'verify blob:limit=10001' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -212,7 +212,7 @@ test_expect_success 'verify blob:limit=1k' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -230,7 +230,7 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
 	HEAD
 	$(git -C r2 rev-parse HEAD:large.10000)
 	EOF

@@ -249,7 +249,7 @@ test_expect_success 'verify blob:limit=1m' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -302,7 +302,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../all.pack &&

@@ -320,7 +320,7 @@ test_expect_success 'verify sparse:path=pattern1' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../filter.pack &&

@@ -352,7 +352,7 @@ test_expect_success 'verify sparse:path=pattern2' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../filter.pack &&

@@ -404,7 +404,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r4 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../all.pack &&

@@ -423,7 +423,7 @@ test_expect_success 'verify sparse:oid=OID' '
 	sort >expected &&
 
 	oid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) &&
-	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../filter.pack &&

@@ -441,7 +441,7 @@ test_expect_success 'verify sparse:oid=oid-ish' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../filter.pack &&

@@ -470,19 +470,19 @@ test_expect_success 'setup r1 - delete loose blobs' '
 '
 
 test_expect_success 'verify pack-objects fails w/ missing objects' '
-	test_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF
+	test_must_fail git -C r1 pack-objects --revs --stdout >miss.pack <<-EOF
 	HEAD
 	EOF
 '
 
 test_expect_success 'verify pack-objects fails w/ --missing=error' '
-	test_must_fail git -C r1 pack-objects --rev --stdout --missing=error >miss.pack <<-EOF
+	test_must_fail git -C r1 pack-objects --revs --stdout --missing=error >miss.pack <<-EOF
 	HEAD
 	EOF
 '
 
 test_expect_success 'verify pack-objects w/ --missing=allow-any' '
-	git -C r1 pack-objects --rev --stdout --missing=allow-any >miss.pack <<-EOF
+	git -C r1 pack-objects --revs --stdout --missing=allow-any >miss.pack <<-EOF
 	HEAD
 	EOF
 '

Signature · 1 line

gitgitgadget

Denton Liu

2019-03-25 18:35 UTC #9 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

Hi Johannes,

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

Quoted 8 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>
Git's command-line parsers support uniquely abbreviated options, e.g. git init --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.

Quoted 7 lines

However, it is not a good idea to rely on that in Git's test suite, as something that is a unique abbreviation of a command line option today might no longer be a unique abbreviation tomorrow.
For example, if a future contribution added a new mode
git init --babyproofing and a previously-introduced test case used the fact that git init --ba expaneded to git init --bare, that future

s/expaneded/expanded/

Quoted 100 lines

contribution would now have to touch seemingly unrelated tests just to keep the test suite from failing.

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 parse-options.c          | 9 +++++++++
 t/README                 | 4 ++++
 t/t0040-parse-options.sh | 7 ++++++-
 t/test-lib.sh            | 6 ++++++
 4 files changed, 25 insertions(+), 1 deletion(-)

parse-options.c +9 -0

index cec74522e5..acc3a93660 100644
@@ -6,6 +6,8 @@
 #include "color.h"
 #include "utf8.h"
 
+static int disallow_abbreviated_options;
+
 #define OPT_SHORT 1
 #define OPT_UNSET 2

@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
+	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+		die("disallowed abbreviated or ambiguous option '%.*s'",
+		    (int)(arg_end - arg), arg);
+
 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),

@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
+	disallow_abbreviated_options =
+		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
+
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:

t/README +4 -0

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

t/t0040-parse-options.sh +5 -1

index b8f366c442..5f6a16336d 100755
@@ -203,20 +203,24 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 test-tool parse-options --strin 123
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="string: 123" --st 123
 '

t/t0040-parse-options.sh file: (not set) · line 335

Quoted 7 lines

t/t0040-parse-options.sh +1 -0

@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output

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

Thanks,

Denton

Quoted 19 lines

t/test-lib.sh +6 -0

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

Signature · 1 line

gitgitgadget

Ævar Arnfjörð Bjarmason

2019-03-25 19:47 UTC #13 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

t/test-lib.sh fi · line 65

Quoted 6 lines · Johannes Schindelin via GitGitGadget

t/test-lib.sh +6 -0

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

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

Ævar Arnfjörð Bjarmason

2019-03-25 20:23 UTC #15 lore raw

cover allow for configuring option abbreviation + fix

In reply to Johannes Schindelin via GitGitGadget

This is based on Johannes's just-submitted
https://public-inbox.org/git/[email protected]/

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

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

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

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

Signature · 1 line

2.21.0.360.g471c308f928

Ævar Arnfjörð Bjarmason

2019-03-25 20:23 UTC #16 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

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 <[email protected]>

---
 Documentation/config/core.txt | 12 ++++++++++++
 parse-options.c               | 19 +++++++++++++++----
 t/README                      |  4 ++--
 t/t0040-parse-options.sh      | 22 +++++++++++++++++-----
 t/test-lib.sh                 |  6 +++---
 5 files changed, 49 insertions(+), 14 deletions(-)

Documentation/config/core.txt +12 -0

index 7e9b6c8f4c..5a42ec3940 100644
@@ -1,3 +1,15 @@
+core.abbreviatedOptions::
+	Defaults to `true` which allows options to be abbreviated as
+	long as they aren't ambiguous, e.g. for linkgit:git-init[1]
+	the `--bare` option can be abbreviated as `--bar`, `--ba` or
+	even `--b` since no other option starts with those
+	prefixes. However, if such an option were added in the future
+	any use of these abbreviations would break.
++
+By setting this to false (e.g. in scripts) you can defend against such
+future breakages by enforcing that options must always be fully
+provided.
+
 core.fileMode::
 	Tells Git if the executable bit of files in the working tree
 	is to be honored.

parse-options.c +15 -4

index acc3a93660..d6a291f705 100644
@@ -6,7 +6,7 @@
 #include "color.h"
 #include "utf8.h"
 
-static int disallow_abbreviated_options;
+static int abbreviated_options = 1;
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2

@@ -346,7 +346,7 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
-	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+	if (!abbreviated_options && (ambiguous_option || abbrev_option))
 		die("disallowed abbreviated or ambiguous option '%.*s'",
 		    (int)(arg_end - arg), arg);

@@ -456,6 +456,17 @@ static void parse_options_check(const struct option *opts)
 		exit(128);
 }
 
+static void parse_options_config(void)
+{
+	int env = git_env_bool("GIT_TEST_ABBREVIATED_OPTIONS", -1);
+	if (env != -1) {
+		abbreviated_options = env;
+		return;
+	}
+	git_config_get_bool("core.abbreviatedoptions", &abbreviated_options);
+	return;
+}
+
 void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 int argc, const char **argv, const char *prefix,
 			 const struct option *options, int flags)

@@ -480,6 +491,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	    (flags & PARSE_OPT_KEEP_ARGV0))
 		BUG("Can't keep argv0 if you don't have it");
 	parse_options_check(options);
+	parse_options_config();
 }
 
 static void show_negated_gitcomp(const struct option *opts, int nr_noopts)

@@ -714,9 +726,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
-	disallow_abbreviated_options =
-		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
 
+	parse_options_config();
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:

t/README +2 -2

index 9ed3051a1c..1d628baf31 100644
@@ -399,9 +399,9 @@ GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the
 fetch-pack to not request sideband-all (even if the server advertises
 sideband-all).
 
-GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=<boolean>, when true (which is
+GIT_TEST_ABBREVIATED_OPTIONS=<boolean>, when false (which is
 the default when running tests), errors out when an abbreviated option
-is used.
+is used. Overrides the core.abbreviatedOptions setting.
 
 Naming Tests
 ------------

t/t0040-parse-options.sh +17 -5

index 5f6a16336d..19685d1582 100755
@@ -203,27 +203,39 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test_expect_code 129 env GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'abbreviated options configured with core.abbreviatedOptions' '
+	test_when_finished "
+		rm -rf A B C &&
+		GIT_TEST_ABBREVIATED_OPTIONS=$GIT_TEST_ABBREVIATED_OPTIONS &&
+		export GIT_TEST_ABBREVIATED_OPTIONS
+	" &&
+	git init --bare A &&
+	test_must_fail git init --ba B &&
+	sane_unset GIT_TEST_ABBREVIATED_OPTIONS &&
+	git -c core.abbreviatedOptions=true init --ba C
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF

@@ -329,7 +341,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	GIT_TEST_ABBREVIATED_OPTIONS=true \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output

t/test-lib.sh +3 -3

index e550009411..4b0dd8a1a9 100644
@@ -58,9 +58,9 @@ fi
 export PERL_PATH SHELL_PATH
 
 # Disallow the use of abbreviated options in the test suite by default
-test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
-	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
-	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+test -n "$GIT_TEST_ABBREVIATED_OPTIONS" || {
+	GIT_TEST_ABBREVIATED_OPTIONS=false
+	export GIT_TEST_ABBREVIATED_OPTIONS
 }
 
 ################################################################

Signature · 1 line

2.21.0.360.g471c308f928

Ævar Arnfjörð Bjarmason

2019-03-25 20:23 UTC #25 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

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 <[email protected]>

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

builtin/clone.c +2 -2

index 50bde99618..4dc26969a7 100644
@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {
 		    N_("setup as shared repository")),
 	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
+	  recurse_submodules_cb, (intptr_t)"." },
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },

parse-options.c +2 -1

index d6a291f705..84f3a2996f 100644
@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(
 		if (!rest) {
 			/* abbreviated? */
 			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
-			    !strncmp(long_name, arg, arg_end - arg)) {
+			    !strncmp(long_name, arg, arg_end - arg) &&
+			    !(options->flags & PARSE_OPT_NOCOMPLETE)) {
 is_abbreviated:
 				if (abbrev_option) {
 					/*

parse-options.h +2 -0

index 74cce4e7fc..9362a397ae 100644
@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *				Useful for options with multiple parameters.
  *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
  *			   by git-completion.bash. This option suppresses that.
+ *			   Will also skip this option when abbreviation is
+ *			   considered. See core.abbreviatedOptions.
  *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
  *			 complete an option as --name= not --name even if
  *			 the option takes optional argument.

t/t0040-parse-options.sh +5 -0

index 19685d1582..c1ea50aa85 100755
@@ -236,6 +236,11 @@ test_expect_success 'abbreviated options configured with core.abbreviatedOptions
 	git -c core.abbreviatedOptions=true init --ba C
 '
 
+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '
+	test_when_finished "rm -rf A" &&
+	GIT_TEST_ABBREVIATED_OPTIONS=true git clone --recurs . A
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF

Signature · 1 line

2.21.0.360.g471c308f928

Ævar Arnfjörð Bjarmason

2019-03-25 20:26 UTC #10 lore raw

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

In reply to Denton Liu

Quoted 12 lines · Denton Liu

Hi Johannes,
Thanks for catching this. Perhaps I should've been more diligent and ran the entire test suite before submitting but I was running low on batteries only ran the rebase-related tests.
Quoted 5 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]> [...]
(quoted diff) +0 -0
@@ -325,6 +329,7 @@ file: (not set)
 EOF
 

Quoted 8 lines

t/t0040-parse-options.sh file: (not set) · line 335
Quoted 5 lines
t/t0040-parse-options.sh +1 -0
 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.

Eric Sunshine

2019-03-25 21:23 UTC #17 lore raw

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

In reply to Ævar Arnfjörð Bjarmason

Documentation/config/core.txt line 11

Quoted 13 lines · Ævar Arnfjörð Bjarmason

Documentation/config/core.txt +11 -0

@@ -1,3 +1,15 @@
+core.abbreviatedOptions::
+       Defaults to `true` which allows options to be abbreviated as
+       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
+       the `--bare` option can be abbreviated as `--bar`, `--ba` or
+       even `--b` since no other option starts with those
+       prefixes. However, if such an option were added in the future
+       any use of these abbreviations would break.
++
+By setting this to false (e.g. in scripts) you can defend against such
+future breakages by enforcing that options must always be fully
+provided.

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

GIT_TEST_ABBREVIATED_OPTIONS=false
export GIT_TEST_ABBREVIATED_OPTIONS

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

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

GIT_ABBREVIATED_OPTIONS

Ævar Arnfjörð Bjarmason

2019-03-25 22:47 UTC #18 lore raw

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

Quoted 28 lines · Eric Sunshine

Documentation/config/core.txt line 11
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Documentation/config/core.txt +11 -0
@@ -1,3 +1,15 @@
+core.abbreviatedOptions::
+       Defaults to `true` which allows options to be abbreviated as
+       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
+       the `--bare` option can be abbreviated as `--bar`, `--ba` or
+       even `--b` since no other option starts with those
+       prefixes. However, if such an option were added in the future
+       any use of these abbreviations would break.
++
+By setting this to false (e.g. in scripts) you can defend against such
+future breakages by enforcing that options must always be fully
+provided.
I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
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.

Quoted 5 lines

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

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

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

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

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

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

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

Duy Nguyen

2019-03-26 04:14 UTC #19 lore raw

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

Quoted 42 lines · Ævar Arnfjörð Bjarmason

Quoted 28 lines · Eric Sunshine
Documentation/config/core.txt line 11
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Documentation/config/core.txt +11 -0
@@ -1,3 +1,15 @@
+core.abbreviatedOptions::
+       Defaults to `true` which allows options to be abbreviated as
+       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
+       the `--bare` option can be abbreviated as `--bar`, `--ba` or
+       even `--b` since no other option starts with those
+       prefixes. However, if such an option were added in the future
+       any use of these abbreviations would break.
++
+By setting this to false (e.g. in scripts) you can defend against such
+future breakages by enforcing that options must always be fully
+provided.
I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
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.
Quoted 5 lines
Also, if this is now intended to be more general (aiding script writers) than just being for our test suite, then dropping "TEST" from the name seems warranted:
GIT_ABBREVIATED_OPTIONS
If we want to make something user-configurable we tend to add config variables. The GIT_TEST_* variables are only intended for our own test suite, see t/README.

Quoted 6 lines

I don't mind documenting this, but it's a well-established pattern, so if we're going to describe how this works/why use one or the other it should probably be some other series to t/README and/or git-config.txt
We traditionally *only* expose this sort of thing to users via config, and not via env variables.

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

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

Signature · 1 line

Duy

Ævar Arnfjörð Bjarmason

2019-03-26 06:29 UTC #20 lore raw

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

Quoted 44 lines · Duy Nguyen

Quoted 42 lines · Ævar Arnfjörð Bjarmason
Quoted 28 lines · Eric Sunshine
Documentation/config/core.txt line 11
Quoted 13 lines · Ævar Arnfjörð Bjarmason
Documentation/config/core.txt +11 -0
@@ -1,3 +1,15 @@
+core.abbreviatedOptions::
+       Defaults to `true` which allows options to be abbreviated as
+       long as they aren't ambiguous, e.g. for linkgit:git-init[1]
+       the `--bare` option can be abbreviated as `--bar`, `--ba` or
+       even `--b` since no other option starts with those
+       prefixes. However, if such an option were added in the future
+       any use of these abbreviations would break.
++
+By setting this to false (e.g. in scripts) you can defend against such
+future breakages by enforcing that options must always be fully
+provided.
I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
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.
Quoted 5 lines
Also, if this is now intended to be more general (aiding script writers) than just being for our test suite, then dropping "TEST" from the name seems warranted:
GIT_ABBREVIATED_OPTIONS
If we want to make something user-configurable we tend to add config variables. The GIT_TEST_* variables are only intended for our own test suite, see t/README.

Quoted 6 lines

Quoted 6 lines
I don't mind documenting this, but it's a well-established pattern, so if we're going to describe how this works/why use one or the other it should probably be some other series to t/README and/or git-config.txt
We traditionally *only* expose this sort of thing to users via config, and not via env variables.

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

Quoted 3 lines

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

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

Quoted 4 lines

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

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

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

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

Duy Nguyen

2019-03-26 07:13 UTC #21 lore raw

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

Quoted 2 lines · Ævar Arnfjörð Bjarmason

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

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

Quoted 7 lines

By my count out of the the existing GIT_* variables in git.txt around 1/3 are already configurable via config, another 1/3 (all the GIT_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.

Signature · 1 line

Duy

Ævar Arnfjörð Bjarmason

2019-03-26 11:00 UTC #22 lore raw

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

Quoted 12 lines · Duy Nguyen

Quoted 2 lines · Ævar Arnfjörð Bjarmason
I don't see how a new "abbreviatedOptions" is plausibly going to crowd out anything else, sounds pretty unambiguous to me.
By crowded I mean a lot of configuration variables under "core"
Quoted 5 lines
By my count out of the the existing GIT_* variables in git.txt around 1/3 are already configurable via config, another 1/3 (all the GIT_TRACE stuff) is something we've wanted to have configurable in the past (but nobody's gotten around to writing patches for).

Quoted 7 lines

Quoted 2 lines
I think it's fair to say that when we normally add user-configurable stuff we do it as config, not as new env vars.
I disagree that not every configuration knob has to be a configuration variable, especially when core.* more or less becomes a dictionary that you can't really read anymore (unless you know the key to look for). But I see you're dead set on adding config vars. Go ahead.

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

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

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

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

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

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

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

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

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

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

Junio C Hamano

2019-04-01 10:47 UTC #23 lore raw

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

In reply to Eric Sunshine

Quoted 11 lines · Eric Sunshine

I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
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.

Junio C Hamano

2019-04-02 00:58 UTC #36 lore raw

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 3 lines · writes:

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

Makes sense.

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

Will queue.

Johannes Schindelin

2019-04-12 08:49 UTC #11 lore raw

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

In reply to Ævar Arnfjörð Bjarmason

Hi,

Quoted 24 lines · Ævar Arnfjörð Bjarmason

Quoted 12 lines · Denton Liu
Hi Johannes,
Thanks for catching this. Perhaps I should've been more diligent and ran the entire test suite before submitting but I was running low on batteries only ran the rebase-related tests.
Quoted 5 lines · Johannes Schindelin via GitGitGadget
From: Johannes Schindelin <[email protected]> [...]
(quoted diff) +0 -0
@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
Quoted 8 lines
t/t0040-parse-options.sh file: (not set) · line 335
Quoted 5 lines
t/t0040-parse-options.sh +1 -0
 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?

Quoted 5 lines

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

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

Thanks,
Dscho

Johannes Schindelin

2019-04-12 08:51 UTC #12 lore raw

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

In reply to Denton Liu

Hi Denton,

Quoted 3 lines · Denton Liu

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

No worries.

Quoted 13 lines · Johannes Schindelin via GitGitGadget

Quoted 12 lines
From: Johannes Schindelin <[email protected]>
Git's command-line parsers support uniquely abbreviated options, e.g. git init --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.

Quoted 5 lines

Quoted 3 lines
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.

Quoted 11 lines

t/t0040-parse-options.sh file: (not set) · line 335
Quoted 8 lines
t/t0040-parse-options.sh +1 -0
@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect 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

Johannes Schindelin

2019-04-12 08:59 UTC #14 lore raw

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

In reply to Ævar Arnfjörð Bjarmason

Hi Ævar,

Quoted 12 lines · Ævar Arnfjörð Bjarmason

t/test-lib.sh fi · line 65
Quoted 6 lines · Johannes Schindelin via GitGitGadget
t/test-lib.sh +6 -0
+# Disallow the use of abbreviated options in the test suite by default
+test -n "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS" || {
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+}
+
Just using the if test ...\nthen\nfi long-form would be consistent with our usual style & the rest of this file.

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

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

Johannes Schindelin

2019-04-12 09:06 UTC #24 lore raw

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

In reply to Junio C Hamano

Hi Junio,

Quoted 14 lines · Junio C Hamano

Quoted 11 lines · Eric Sunshine
I don't get why having a configuration option is better for defending scripts against this problem than a simple environment variable. It seems easier for the script prologue to contain:
GIT_TEST_ABBREVIATED_OPTIONS=false
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.

Quoted 6 lines

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

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #37 lore raw

v2 cover Do not use abbreviated options in tests

In reply to 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.

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.

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

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

Range-diff vs v1:

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

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #39 lore raw

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

From: Johannes Schindelin <[email protected]>

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t3415-rebase-autosquash.sh | 2 +-
 t/t3430-rebase-merges.sh     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

t/t3415-rebase-autosquash.sh +1 -1

index 13f5688135..22d218698e 100755
@@ -277,7 +277,7 @@ test_expect_success 'autosquash with empty custom instructionFormat' '
 	(
 		set_cat_todo_editor &&
 		test_must_fail git -c rebase.instructionFormat= \
-			rebase --autosquash  --force -i HEAD^ >actual &&
+			rebase --autosquash  --force-rebase -i HEAD^ >actual &&
 		git log -1 --format="pick %h %s" >expect &&
 		test_cmp expect actual
 	)

t/t3430-rebase-merges.sh +2 -2

index 4c69255ee6..42ba5b9f09 100755
@@ -271,7 +271,7 @@ test_expect_success 'root commits' '
 	EOF
 	test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
 	test_tick &&
-	git rebase -i --force --root -r &&
+	git rebase -i --force-rebase --root -r &&
 	test "Parsnip" = "$(git show -s --format=%an HEAD^)" &&
 	test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
 	test $(git rev-parse second-root:second-root.t) = \

@@ -364,7 +364,7 @@ test_expect_success 'octopus merges' '
 	test_cmp_rev HEAD $before &&
 
 	test_tick &&
-	git rebase -i --force -r HEAD^^ &&
+	git rebase -i --force-rebase -r HEAD^^ &&
 	test "Hank" = "$(git show -s --format=%an HEAD)" &&
 	test "$before" != $(git rev-parse HEAD) &&
 	test_cmp_graph HEAD^^.. <<-\EOF

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #41 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t5531-deep-submodule-push.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t5531-deep-submodule-push.sh +1 -1

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

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #43 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t7525-status-rename.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

t/t7525-status-rename.sh +4 -4

index ef8b1b3078..a62736dce0 100755
@@ -84,7 +84,7 @@ test_expect_success 'status score=100%' '
 	test_i18ngrep "deleted:" actual &&
 	test_i18ngrep "new file:" actual &&
 
-	git status --find-rename=100% >actual &&
+	git status --find-renames=100% >actual &&
 	test_i18ngrep "deleted:" actual &&
 	test_i18ngrep "new file:" actual
 '

@@ -93,11 +93,11 @@ test_expect_success 'status score=01%' '
 	git status -M=01% >actual &&
 	test_i18ngrep "renamed:" actual &&
 
-	git status --find-rename=01% >actual &&
+	git status --find-renames=01% >actual &&
 	test_i18ngrep "renamed:" actual
 '
 
-test_expect_success 'copies not overridden by find-rename' '
+test_expect_success 'copies not overridden by find-renames' '
 	cp renamed copy &&
 	git add copy &&

@@ -105,7 +105,7 @@ test_expect_success 'copies not overridden by find-rename' '
 	test_i18ngrep "copied:" actual &&
 	test_i18ngrep "renamed:" actual &&
 
-	git -c status.renames=copies status --find-rename=01% >actual &&
+	git -c status.renames=copies status --find-renames=01% >actual &&
 	test_i18ngrep "copied:" actual &&
 	test_i18ngrep "renamed:" actual
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #42 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t5516-fetch-push.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t5516-fetch-push.sh +2 -2

index 37e8e80893..db0b1db458 100755
@@ -1370,7 +1370,7 @@ test_expect_success 'push does not follow tags by default' '
 	test_cmp expect actual
 '
 
-test_expect_success 'push --follow-tag only pushes relevant tags' '
+test_expect_success 'push --follow-tags only pushes relevant tags' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
 	git init src &&

@@ -1384,7 +1384,7 @@ test_expect_success 'push --follow-tag only pushes relevant tags' '
 		git tag -m "future" future &&
 		git checkout master &&
 		git for-each-ref refs/heads/master refs/tags/tag >../expect &&
-		git push --follow-tag ../dst master
+		git push --follow-tags ../dst master
 	) &&
 	(
 		cd dst &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #38 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t5407-post-rewrite-hook.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

t/t5407-post-rewrite-hook.sh +2 -2

index a4a5903cba..7344253bfb 100755
@@ -131,7 +131,7 @@ test_expect_success 'git rebase -m --skip' '
 test_expect_success 'git rebase with implicit use of interactive backend' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --keep --onto A B &&
+	test_must_fail git rebase --keep-empty --onto A B &&
 	echo C > foo &&
 	git add foo &&
 	git rebase --continue &&

@@ -146,7 +146,7 @@ test_expect_success 'git rebase with implicit use of interactive backend' '
 test_expect_success 'git rebase --skip with implicit use of interactive backend' '
 	git reset --hard D &&
 	clear_hook_input &&
-	test_must_fail git rebase --keep --onto A B &&
+	test_must_fail git rebase --keep-empty --onto A B &&
 	test_must_fail git rebase --skip &&
 	echo D > foo &&
 	git add foo &&

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #40 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

This script used abbreviated options, which is unnecessarily fragile.

Signed-off-by Johannes Schindelin <[email protected]>

---
 t/t7810-grep.sh | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

t/t7810-grep.sh +8 -8

index 43aa4161cf..2e1bb61b41 100755
@@ -119,33 +119,33 @@ do
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep -w $L (with --column, --invert)" '
+	test_expect_success "grep -w $L (with --column, --invert-match)" '
 		{
 			echo ${HC}file:1:foo mmap bar
 			echo ${HC}file:1:foo_mmap bar
 			echo ${HC}file:1:foo_mmap bar mmap
 			echo ${HC}file:1:foo mmap bar_mmap
 		} >expected &&
-		git grep --column --invert -w -e baz $H -- file >actual &&
+		git grep --column --invert-match -w -e baz $H -- file >actual &&
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep $L (with --column, --invert, extended OR)" '
+	test_expect_success "grep $L (with --column, --invert-match, extended OR)" '
 		{
 			echo ${HC}hello_world:6:HeLLo_world
 		} >expected &&
-		git grep --column --invert -e ll --or --not -e _ $H -- hello_world \
+		git grep --column --invert-match -e ll --or --not -e _ $H -- hello_world \
 			>actual &&
 		test_cmp expected actual
 	'
 
-	test_expect_success "grep $L (with --column, --invert, extended AND)" '
+	test_expect_success "grep $L (with --column, --invert-match, extended AND)" '
 		{
 			echo ${HC}hello_world:3:Hello world
 			echo ${HC}hello_world:3:Hello_world
 			echo ${HC}hello_world:6:HeLLo_world
 		} >expected &&
-		git grep --column --invert --not -e _ --and --not -e ll $H -- hello_world \
+		git grep --column --invert-match --not -e _ --and --not -e ll $H -- hello_world \
 			>actual &&
 		test_cmp expected actual
 	'

@@ -1010,7 +1010,7 @@ test_expect_success 'outside of git repository' '
 			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git grep --no-index --no-exclude o >../actual.full &&
+		git grep --no-index --no-exclude-standard o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '

@@ -1051,7 +1051,7 @@ test_expect_success 'outside of git repository with fallbackToNoIndex' '
 			echo ".gitignore:.*o*" &&
 			cat ../expect.full
 		} >../expect.with.ignored &&
-		git -c grep.fallbackToNoIndex grep --no-exclude o >../actual.full &&
+		git -c grep.fallbackToNoIndex grep --no-exclude-standard o >../actual.full &&
 		test_cmp ../expect.with.ignored ../actual.full
 	)
 '

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #45 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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

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

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

For example, if a future contribution added a new 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.

Signed-off-by Johannes Schindelin <[email protected]>

---
 parse-options.c          |  9 +++++++++
 t/README                 |  4 ++++
 t/t0040-parse-options.sh | 14 +++++++++++++-
 t/test-lib.sh            |  7 +++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

parse-options.c +9 -0

index cec74522e5..acc3a93660 100644
@@ -6,6 +6,8 @@
 #include "color.h"
 #include "utf8.h"
 
+static int disallow_abbreviated_options;
+
 #define OPT_SHORT 1
 #define OPT_UNSET 2

@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
+	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+		die("disallowed abbreviated or ambiguous option '%.*s'",
+		    (int)(arg_end - arg), arg);
+
 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),

@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
+	disallow_abbreviated_options =
+		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
+
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:

t/README +4 -0

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

t/t0040-parse-options.sh +13 -1

index b8f366c442..6f6f74bfe2 100755
@@ -203,20 +203,24 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 test-tool parse-options --strin 123
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="string: 123" --st 123
 '

@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output

@@ -370,4 +375,11 @@ test_expect_success '--no-verbose resets multiple verbose to 0' '
 	test-tool parse-options --expect="verbose: 0" -v -v -v --no-verbose
 '
 
+test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '
+	env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+		test-tool parse-options --ye &&
+	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \
+		test-tool parse-options --ye
+'
+
 test_done

t/test-lib.sh +7 -0

index 562c57e685..f1a0fea4e1 100644
@@ -57,6 +57,13 @@ fi
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH
 
+# Disallow the use of abbreviated options in the test suite by default
+if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
+then
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+fi
+
 ################################################################
 # It appears that people try to run tests without building...
 "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null

Signature · 1 line

gitgitgadget

Johannes Schindelin via GitGitGadget

author

2019-04-12 09:37 UTC #44 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

---
 t/t5317-pack-objects-filter-objects.sh | 44 +++++++++++++-------------
 1 file changed, 22 insertions(+), 22 deletions(-)

t/t5317-pack-objects-filter-objects.sh +22 ~~-22~~

index 24541ea137..4c0201c34b 100755
@@ -25,7 +25,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r1 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../all.pack &&

@@ -39,7 +39,7 @@ test_expect_success 'verify blob count in normal packfile' '
 '
 
 test_expect_success 'verify blob:none packfile has no blobs' '
-	git -C r1 pack-objects --rev --stdout --filter=blob:none >filter.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=blob:none >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../filter.pack &&

@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' '
 	git -C r5 commit -m "foo" &&
 	del=$(git -C r5 rev-parse HEAD^{tree} | sed "s|..|&/|") &&
 	rm r5/.git/objects/$del &&
-	test_must_fail git -C r5 pack-objects --rev --stdout 2>bad_tree <<-EOF &&
+	test_must_fail git -C r5 pack-objects --revs --stdout 2>bad_tree <<-EOF &&
 	HEAD
 	EOF
 	grep "bad tree object" bad_tree

@@ -88,7 +88,7 @@ test_expect_success 'setup for tests of tree:0' '
 '
 
 test_expect_success 'verify tree:0 packfile has no blobs or trees' '
-	git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r1 index-pack ../commitsonly.pack &&

@@ -98,7 +98,7 @@ test_expect_success 'verify tree:0 packfile has no blobs or trees' '
 
 test_expect_success 'grab tree directly when using tree:0' '
 	# We should get the tree specified directly but not its blobs or subtrees.
-	git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
+	git -C r1 pack-objects --revs --stdout --filter=tree:0 >commitsonly.pack <<-EOF &&
 	HEAD:
 	EOF
 	git -C r1 index-pack ../commitsonly.pack &&

@@ -128,7 +128,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../all.pack &&

@@ -142,7 +142,7 @@ test_expect_success 'verify blob count in normal packfile' '
 '
 
 test_expect_success 'verify blob:limit=500 omits all blobs' '
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=500 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -157,7 +157,7 @@ test_expect_success 'verify blob:limit=500 omits all blobs' '
 '
 
 test_expect_success 'verify blob:limit=1000' '
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1000 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -176,7 +176,7 @@ test_expect_success 'verify blob:limit=1001' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1001 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -194,7 +194,7 @@ test_expect_success 'verify blob:limit=10001' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=10001 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -212,7 +212,7 @@ test_expect_success 'verify blob:limit=1k' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -230,7 +230,7 @@ test_expect_success 'verify explicitly specifying oversized blob in input' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1k >filter.pack <<-EOF &&
 	HEAD
 	$(git -C r2 rev-parse HEAD:large.10000)
 	EOF

@@ -249,7 +249,7 @@ test_expect_success 'verify blob:limit=1m' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r2 pack-objects --rev --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
+	git -C r2 pack-objects --revs --stdout --filter=blob:limit=1m >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r2 index-pack ../filter.pack &&

@@ -302,7 +302,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../all.pack &&

@@ -320,7 +320,7 @@ test_expect_success 'verify sparse:path=pattern1' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern1 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../filter.pack &&

@@ -352,7 +352,7 @@ test_expect_success 'verify sparse:path=pattern2' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r3 pack-objects --rev --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
+	git -C r3 pack-objects --revs --stdout --filter=sparse:path=../pattern2 >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r3 index-pack ../filter.pack &&

@@ -404,7 +404,7 @@ test_expect_success 'verify blob count in normal packfile' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r4 pack-objects --rev --stdout >all.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout >all.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../all.pack &&

@@ -423,7 +423,7 @@ test_expect_success 'verify sparse:oid=OID' '
 	sort >expected &&
 
 	oid=$(git -C r4 ls-files -s pattern | awk -f print_2.awk) &&
-	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=$oid >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../filter.pack &&

@@ -441,7 +441,7 @@ test_expect_success 'verify sparse:oid=oid-ish' '
 	awk -f print_2.awk ls_files_result |
 	sort >expected &&
 
-	git -C r4 pack-objects --rev --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
+	git -C r4 pack-objects --revs --stdout --filter=sparse:oid=master:pattern >filter.pack <<-EOF &&
 	HEAD
 	EOF
 	git -C r4 index-pack ../filter.pack &&

@@ -470,19 +470,19 @@ test_expect_success 'setup r1 - delete loose blobs' '
 '
 
 test_expect_success 'verify pack-objects fails w/ missing objects' '
-	test_must_fail git -C r1 pack-objects --rev --stdout >miss.pack <<-EOF
+	test_must_fail git -C r1 pack-objects --revs --stdout >miss.pack <<-EOF
 	HEAD
 	EOF
 '
 
 test_expect_success 'verify pack-objects fails w/ --missing=error' '
-	test_must_fail git -C r1 pack-objects --rev --stdout --missing=error >miss.pack <<-EOF
+	test_must_fail git -C r1 pack-objects --revs --stdout --missing=error >miss.pack <<-EOF
 	HEAD
 	EOF
 '
 
 test_expect_success 'verify pack-objects w/ --missing=allow-any' '
-	git -C r1 pack-objects --rev --stdout --missing=allow-any >miss.pack <<-EOF
+	git -C r1 pack-objects --revs --stdout --missing=allow-any >miss.pack <<-EOF
 	HEAD
 	EOF
 '

Signature · 1 line

gitgitgadget

Junio C Hamano

2019-04-14 02:36 UTC #46 lore raw

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

In reply to Johannes Schindelin via GitGitGadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

parse-options.c int parse_options(int argc, const char **argv, const char *prefix, · line 718

Quoted 2 lines · writes:

parse-options.c +2 -0

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

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

t/README GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the · line 404

Quoted 11 lines

t/README +3 -0

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

OK.

t/README GIT_TEST_SIDEBAND_ALL=<boolean>, when true, overrides the

Quoted 6 lines

t/README +6 -0

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

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

Quoted 5 lines

t/test-lib.sh +0 -0
index 562c57e685..f1a0fea4e1 100644

t/test-lib.sh fi · line 65

Quoted 9 lines

t/test-lib.sh +6 -0

@@ -57,6 +57,13 @@ fi
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH
 
+# Disallow the use of abbreviated options in the test suite by default
+if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
+then
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+fi

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

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

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

Do you mean more like

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

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

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

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

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

Junio C Hamano

2019-04-15 02:55 UTC #47 lore raw

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

Quoted 3 lines · Junio C Hamano

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

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

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

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

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

Thanks.

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

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

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

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

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

Signed-off-by Johannes Schindelin <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 parse-options.c          |  9 +++++++++
 t/README                 |  4 ++++
 t/t0040-parse-options.sh | 14 +++++++++++++-
 t/test-lib.sh            |  7 +++++++
 4 files changed, 33 insertions(+), 1 deletion(-)

parse-options.c +9 -0

index cec74522e5..acc3a93660 100644
@@ -6,6 +6,8 @@
 #include "color.h"
 #include "utf8.h"
 
+static int disallow_abbreviated_options;
+
 #define OPT_SHORT 1
 #define OPT_UNSET 2

@@ -344,6 +346,10 @@ static enum parse_opt_result parse_long_opt(
 		return get_value(p, options, all_opts, flags ^ opt_flags);
 	}
 
+	if (disallow_abbreviated_options && (ambiguous_option || abbrev_option))
+		die("disallowed abbreviated or ambiguous option '%.*s'",
+		    (int)(arg_end - arg), arg);
+
 	if (ambiguous_option) {
 		error(_("ambiguous option: %s "
 			"(could be --%s%s or --%s%s)"),

@@ -708,6 +714,9 @@ int parse_options(int argc, const char **argv, const char *prefix,
 {
 	struct parse_opt_ctx_t ctx;
 
+	disallow_abbreviated_options =
+		git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0);
+
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:

t/README +4 -0

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

t/t0040-parse-options.sh +13 -1

index b8f366c442..800b3ea5f5 100755
@@ -203,20 +203,24 @@ file: (not set)
 EOF
 
 test_expect_success 'unambiguously abbreviated option' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --int 2 --boolean --no-bo >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
 
 test_expect_success 'unambiguously abbreviated option with "="' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="integer: 2" --int=2
 '
 
 test_expect_success 'ambiguously abbreviated option' '
-	test_expect_code 129 test-tool parse-options --strin 123
+	test_expect_code 129 env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --strin 123
 '
 
 test_expect_success 'non ambiguous option (after two options it abbreviates)' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --expect="string: 123" --st 123
 '

@@ -325,6 +329,7 @@ file: (not set)
 EOF
 
 test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
 	test-tool parse-options --no-ambig >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output

@@ -370,4 +375,11 @@ test_expect_success '--no-verbose resets multiple verbose to 0' '
 	test-tool parse-options --expect="verbose: 0" -v -v -v --no-verbose
 '
 
+test_expect_success 'GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS works' '
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+		test-tool parse-options --ye &&
+	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \
+		test-tool parse-options --ye
+'
+
 test_done

t/test-lib.sh +7 -0

index 562c57e685..c14ebe68d3 100644
@@ -57,6 +57,13 @@ fi
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH
 
+# Disallow the use of abbreviated options in the test suite by default
+if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}"
+then
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+fi
+
 ################################################################
 # It appears that people try to run tests without building...
 "${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" >/dev/null

Signature · 1 line

2.21.0-313-ge35b8cb8e2

Johannes Schindelin

2019-04-15 13:08 UTC #51 lore raw

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

In reply to Junio C Hamano

Hi Junio,

Quoted 26 lines · Junio C Hamano

"Johannes Schindelin via GitGitGadget" <[email protected]>
Quoted 5 lines · writes:
t/test-lib.sh +0 -0
index 562c57e685..f1a0fea4e1 100644
t/test-lib.sh fi · line 65
Quoted 9 lines
t/test-lib.sh +6 -0
@@ -57,6 +57,13 @@ fi
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
 export PERL_PATH SHELL_PATH
 
+# Disallow the use of abbreviated options in the test suite by default
+if test -n "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
+then
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+fi
If the original environment has it as flase, as it is set, the substitution will yield "isset" which is not an empty string, so we assign true.
If the original environment is not set, or set to an empty, however, the substitution will yield an empty string, so we won't touch the variable.

Quoted 8 lines

I am not sure in what situation the above behaviour becomes useful.
Do you mean more like
	if test -z "$GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS"
	then
		... assignment ...
	fi

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

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

Quoted 4 lines

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

Yes.

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

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

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

Quoted 14 lines

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

Quoted 2 lines

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

Thank you!
Dscho

Johannes Schindelin

2019-04-15 13:09 UTC #48 lore raw

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

In reply to Junio C Hamano

Hi Junio,

Quoted 7 lines · Junio C Hamano

Quoted 3 lines · Junio C Hamano
Do you mean more like
...
I think I can agree with either of the two positions...

Quoted 6 lines

I am guessing from the earlier iteration that you wanted to say "unless it is given explicitly, we turn it on".
As this last-minute style update that was botched, and a typofix in the proposed log message in 8/8, are the only differences, let me locally fix 8/8 up and replace it.

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

Ciao,
Dscho

Junio C Hamano

2019-04-15 13:45 UTC #49 lore raw

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

Quoted 14 lines · Johannes Schindelin

Hi Junio,
Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Junio C Hamano
Do you mean more like
...
I think I can agree with either of the two positions...
I am guessing from the earlier iteration that you wanted to say "unless it is given explicitly, we turn it on".

Quoted 6 lines

Quoted 3 lines
As this last-minute style update that was botched, and a typofix in the proposed log message in 8/8, are the only differences, let me locally fix 8/8 up and replace it.
Sure. I still would like the 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.

Johannes Schindelin

2019-04-17 12:07 UTC #50 lore raw

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

Hi Junio,

Quoted 37 lines · Junio C Hamano

Quoted 12 lines · Johannes Schindelin
Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Junio C Hamano
Do you mean more like
...
I think I can agree with either of the two positions...
I am guessing from the earlier iteration that you wanted to say "unless it is given explicitly, we turn it on".
Quoted 6 lines
Quoted 3 lines
As this last-minute style update that was botched, and a typofix in the proposed log message in 8/8, are the only differences, let me locally fix 8/8 up and replace it.
Sure. I still would like the isset thing, as it makes things more consistent, but I'll not fight for it.
${var:+isset} is fine. Instead of
t/test-lib.sh +7 -0
+# Disallow the use of abbreviated options in the test suite by default
+if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS}"
+then
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true
+	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
+fi
+
if you used
	if test -z "${GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS:+isset}"
	then
		...

Quoted 5 lines

I won't object. After all, I know I introduced :+isset pattern to our shell script codebase as there are cases where it makes the result easier to follow.
But the thing is that your patch had the polarity inverted.

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

Quoted 3 lines

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

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

Quoted 11 lines

Having said that.
When you check if the variable is set, use of the ":+isset" pattern makes the result often easier to follow by explicitly letting us compare with an explicit "isset" token, e.g.
	case ",${VAR1:+isset},${VAR2:+isset}," in
	*,isset,*)	: at least one is set ;;
	*)		: neither is set ;;
	esac

Quoted 6 lines

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

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

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

Ciao,
Dscho

Ævar Arnfjörð Bjarmason

2019-04-17 12:44 UTC #26 lore raw

PATCH v2 1/1 parse-options: don't emit "ambiguous option" for aliases

In reply to Æ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.

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)

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

---

See
https://public-inbox.org/git/[email protected]/
for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2 seems like a no-brainer bugfix.

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

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

builtin/clone.c +2 -2

index 50bde99618..4dc26969a7 100644
@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {
 		    N_("setup as shared repository")),
 	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
+	  recurse_submodules_cb, (intptr_t)"." },
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },

parse-options.c +2 -1

index cec74522e5..9899ce0171 100644
@@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt(
 		if (!rest) {
 			/* abbreviated? */
 			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
-			    !strncmp(long_name, arg, arg_end - arg)) {
+			    !strncmp(long_name, arg, arg_end - arg) &&
+			    !(options->flags & PARSE_OPT_NOCOMPLETE)) {
 is_abbreviated:
 				if (abbrev_option) {
 					/*

parse-options.h +2 -0

index 74cce4e7fc..51c4b71ab0 100644
@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *				Useful for options with multiple parameters.
  *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
  *			   by git-completion.bash. This option suppresses that.
+ *			   Will also skip this option when abbreviation is
+ *			   considered.
  *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
  *			 complete an option as --name= not --name even if
  *			 the option takes optional argument.

t/t0040-parse-options.sh +5 -0

index b8f366c442..e8f0371830 100755
@@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '
+	test_when_finished "rm -rf A" &&
+	git clone --recurs . A
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF

Signature · 1 line

2.21.0.593.g511ec345e18

Duy Nguyen

2019-04-17 16:05 UTC #27 lore raw

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

Quoted 46 lines · Ævar Arnfjörð Bjarmason

Change the option parsing machinery so that e.g. "clone --recurs ..." doesn't error out because "clone" understands both "--recursive" and "--recurse-submodules" to mean the same thing.
Initially "clone" just understood --recursive until the --recurses-submodules alias was added in ccdd3da652 ("clone: Add the --recurse-submodules option as alias for --recursive", 2010-11-04). 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 <[email protected]>
---
See
https://public-inbox.org/git/[email protected]/
for v1. There wasn't consensus for 1/2 there, but this used-to-be 2/2 seems like a no-brainer bugfix.
It conflicted with some recently-landed stuff in 'master', but now cleanly applies to it and 'pu', and with pu's GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS.
 builtin/clone.c          | 4 ++--
 parse-options.c          | 3 ++-
 parse-options.h          | 2 ++
 t/t0040-parse-options.sh | 5 +++++
 4 files changed, 11 insertions(+), 3 deletions(-)
builtin/clone.c +0 -0
index 50bde99618..4dc26969a7 100644

builtin/clone.c static struct option builtin_clone_options[] = { · line 103

Quoted 6 lines

builtin/clone.c +1 -2

@@ -100,8 +100,8 @@ static struct option builtin_clone_options[] = {
                    N_("setup as shared repository")),
        { OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
          N_("pathspec"), N_("initialize submodules in the clone"),
-         PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-         (intptr_t)"." },
+         PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,

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

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

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

Quoted 50 lines

builtin/clone.c +1 -0

+         recurse_submodules_cb, (intptr_t)"." },
        { OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
          N_("pathspec"), N_("initialize submodules in the clone"),
          PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },

parse-options.c +2 -1

index cec74522e5..9899ce0171 100644
@@ -292,7 +292,8 @@ static enum parse_opt_result parse_long_opt(
                if (!rest) {
                        /* abbreviated? */
                        if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
-                           !strncmp(long_name, arg, arg_end - arg)) {
+                           !strncmp(long_name, arg, arg_end - arg) &&
+                           !(options->flags & PARSE_OPT_NOCOMPLETE)) {
 is_abbreviated:
                                if (abbrev_option) {
                                        /*

parse-options.h +2 -0

index 74cce4e7fc..51c4b71ab0 100644
@@ -96,6 +96,8 @@ typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
  *                             Useful for options with multiple parameters.
  *   PARSE_OPT_NOCOMPLETE: by default all visible options are completable
  *                        by git-completion.bash. This option suppresses that.
+ *                        Will also skip this option when abbreviation is
+ *                        considered.
  *   PARSE_OPT_COMP_ARG: this option forces to git-completion.bash to
  *                      complete an option as --name= not --name even if
  *                      the option takes optional argument.

t/t0040-parse-options.sh +5 -1

index b8f366c442..e8f0371830 100755
@@ -220,6 +220,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '
        test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '
+       test_when_finished "rm -rf A" &&
+       git clone --recurs . A
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF
--

2.21.0.593.g511ec345e18

Signature · 1 line

Duy

Junio C Hamano

2019-04-18 00:49 UTC #28 lore raw

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

Quoted 8 lines · Duy Nguyen

builtin/clone.c static struct option builtin_clone_options[] = { · line 103
Quoted 5 lines
{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
N_("pathspec"), N_("initialize submodules in the clone"),
builtin/clone.c +1 -2
-         PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-         (intptr_t)"." },
+         PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE,
What happens if someone adds --recursive-hard? --recursi then resolving to --recursive-hard sounds wrong.

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

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

Quoted 4 lines

But on the other hand I can see it's a bit more work to teach parse-options OPT_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.

Duy Nguyen

2019-04-18 09:29 UTC #29 lore raw

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

Quoted 7 lines · Junio C Hamano

Quoted 4 lines
But on the other hand I can see it's a bit more work to teach parse-options OPT_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< --

builtin/clone.c +29 -2

index 50bde99618..041cd43ddc 100644
@@ -145,6 +145,29 @@ static struct option builtin_clone_options[] = {
 	OPT_END()
 };
 
+/*
+ * Avoid ambiguation error between --recursive and --recurse-submodule
+ * because they are the same. --recurs can be expanded to any of them
+ * and it still works.
+ */
+static int is_abbrev_ambiguous(const struct option *prev,
+			       const struct option *next)
+{
+	const struct option *opts[] = { prev, next };
+	int i, found = 0;
+
+	for (i = 0; i < ARRAY_SIZE(opts); i++) {
+		if (!opts[i]->long_name)
+			continue;
+		if (!strcmp(opts[i]->long_name, "recursive"))
+			found |= 1 << 0;
+		if (!strcmp(opts[i]->long_name, "recurse-submodules"))
+			found |= 1 << 1;
+	}
+
+	return found != 3;
+}
+
 static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 {
 	static char *suffix[] = { "/.git", "", ".git/.git", ".git" };

@@ -905,14 +928,18 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct remote *remote;
 	int err = 0, complete_refs_before_fetch = 1;
 	int submodule_progress;
+	struct parseopt_options parseopts = { 0 };
 
 	struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 
 	fetch_if_missing = 0;
 
 	packet_trace_identity("clone");
-	argc = parse_options(argc, argv, prefix, builtin_clone_options,
-			     builtin_clone_usage, 0);
+	parseopts.usagestr = builtin_clone_usage;
+	parseopts.is_abbrev_ambiguous = is_abbrev_ambiguous;
+	argc = parse_options_extended(argc, argv, prefix,
+				      builtin_clone_options,
+				      &parseopts);
 
 	if (argc > 2)
 		usage_msg_opt(_("Too many arguments."),

parse-options.c +27 -4

index cec74522e5..c0354e5a92 100644
@@ -294,7 +294,8 @@ static enum parse_opt_result parse_long_opt(
 			if (!(p->flags & PARSE_OPT_KEEP_UNKNOWN) &&
 			    !strncmp(long_name, arg, arg_end - arg)) {
 is_abbreviated:
-				if (abbrev_option) {
+				if (abbrev_option &&
+				    p->is_abbrev_ambiguous(abbrev_option, options)) {
 					/*
 					 * If this is abbreviated, it is
 					 * ambiguous. So when there is no

@@ -450,6 +451,12 @@ static void parse_options_check(const struct option *opts)
 		exit(128);
 }
 
+static int abbrev_is_always_ambiguous(const struct option *prev,
+				      const struct option *next)
+{
+	return 1;
+}
+
 void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 int argc, const char **argv, const char *prefix,
 			 const struct option *options, int flags)

@@ -466,6 +473,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	ctx->prefix = prefix;
 	ctx->cpidx = ((flags & PARSE_OPT_KEEP_ARGV0) != 0);
 	ctx->flags = flags;
+	ctx->is_abbrev_ambiguous = abbrev_is_always_ambiguous;
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
 	    (flags & PARSE_OPT_STOP_AT_NON_OPTION) &&
 	    !(flags & PARSE_OPT_ONE_SHOT))

@@ -702,13 +710,17 @@ int parse_options_end(struct parse_opt_ctx_t *ctx)
 	return ctx->cpidx + ctx->argc;
 }
 
-int parse_options(int argc, const char **argv, const char *prefix,
-		  const struct option *options, const char * const usagestr[],
-		  int flags)
+int parse_options_extended(int argc, const char **argv, const char *prefix,
+			   const struct option *options,
+			   const struct parseopt_options *parseopts)
 {
 	struct parse_opt_ctx_t ctx;
+	const char * const * usagestr = parseopts->usagestr;
+	int flags = parseopts->flags;
 
 	parse_options_start(&ctx, argc, argv, prefix, options, flags);
+	if (parseopts->is_abbrev_ambiguous)
+		ctx.is_abbrev_ambiguous = parseopts->is_abbrev_ambiguous;
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 	case PARSE_OPT_ERROR:

@@ -734,6 +746,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	return parse_options_end(&ctx);
 }
 
+int parse_options(int argc, const char **argv, const char *prefix,
+		  const struct option *options, const char * const usagestr[],
+		  int flags)
+{
+	struct parseopt_options parseopts = {0};
+
+	parseopts.flags = flags;
+	parseopts.usagestr = usagestr;
+	return parse_options_extended(argc, argv, prefix, options, &parseopts);
+}
+
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;

parse-options.h +12 -0

index 74cce4e7fc..31ebf4572c 100644
@@ -198,6 +198,17 @@ int parse_options(int argc, const char **argv, const char *prefix,
 		  const struct option *options,
 		  const char * const usagestr[], int flags);
 
+struct parseopt_options {
+	int flags;
+	const char * const * usagestr;
+	int (*is_abbrev_ambiguous)(const struct option *prev,
+				   const struct option *next);
+};
+
+int parse_options_extended(int argc, const char **argv, const char *prefix,
+			   const struct option *options,
+			   const struct parseopt_options *parseopts);
+
 NORETURN void usage_with_options(const char * const *usagestr,
 				 const struct option *options);

@@ -256,6 +267,7 @@ struct parse_opt_ctx_t {
 	const char *opt;
 	int flags;
 	const char *prefix;
+	int (*is_abbrev_ambiguous)(const struct option *, const struct option *);
 };
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,

-- 8< --

Quoted 2 lines

Thanks.

Junio C Hamano

2019-04-19 19:48 UTC #30 lore raw

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

Quoted 4 lines · Duy Nguyen

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

Sounds like a sensibly safe approach.

builtin/clone.c static struct option builtin_clone_options[] = { · line 156

Quoted 9 lines

builtin/clone.c +9 -0

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

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

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

builtin/clone.c static struct option builtin_clone_options[] = { · line 169

Quoted 13 lines

builtin/clone.c +13 -0

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

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

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

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

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

the body would look something like:

	int j;

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

Perhaps?

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

parse-options.c static enum parse_opt_result parse_long_opt( · line 298

Quoted 11 lines

parse-options.c +2 -1

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

The above suggestion would make the guard here to

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

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

Quoted 3 lines

parse-options.c +0 -0

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

Nguyễn Thái Ngọc Duy

2019-04-22 12:23 UTC #31 lore raw

PATCH 1/1 parse-options: don't emit "ambiguous option" for aliases

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

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 <[email protected]>

---
I'm not sure if Junio is an evil genius who lures me with his helpful suggestions. Either way I ended up spending a couple hours writing and rewriting and finally decided all other options were too ugly. And his suggestions were mostly there already.

So here's OPT_ALIAS(). I avoid rewriting all the option iteration code by simply making a copy of options[] array to use, then free it afterwards. Not the best, but at least it could be easily removed if we end up not want OPT_ALIAS() after all.

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

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

builtin/clone.c +1 -4

index 50bde99618..703b7247ad 100644
@@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = {
 		    N_("don't use local hardlinks, always copy")),
 	OPT_BOOL('s', "shared", &option_shared,
 		    N_("setup as shared repository")),
-	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
-	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	OPT_ALIAS(0, "recursive", "recurse-submodules"),
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },

parse-options.c +124 -6

189 lines not on this page · raw message

parse-options.h +6 -0

index 74cce4e7fc..e6b4a8d1c7 100644
@@ -7,6 +7,7 @@ enum parse_opt_type {
 	OPTION_ARGUMENT,
 	OPTION_GROUP,
 	OPTION_NUMBER,
+	OPTION_ALIAS,
 	/* options with no arguments */
 	OPTION_BIT,
 	OPTION_NEGBIT,

@@ -181,6 +182,9 @@ struct option {
 	  N_("no-op (backward compatibility)"),		\
 	  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }
 
+#define OPT_ALIAS(s, l, alias_source) \
+	{ OPTION_ALIAS, (s), (l), (alias_source) }
+
 /*
  * parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[]. argv0 is assumed program name and

@@ -256,6 +260,8 @@ struct parse_opt_ctx_t {
 	const char *opt;
 	int flags;
 	const char *prefix;
+	const char **alias_groups; /* must be in groups of 3 elements! */
+	struct option *updated_options;
 };
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,

t/t0040-parse-options.sh +5 -0

index 800b3ea5f5..14e6664240 100755
@@ -224,6 +224,11 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'NOCOMPLETE options do not contribute to abbreviation' '
+	test_when_finished "rm -rf A" &&
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false git clone --recurs . A
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF

Signature · 1 line

2.21.0.854.ge34a79f761

Duy Nguyen

2019-04-22 12:35 UTC #32 lore raw

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

Quoted 44 lines · Nguyễn Thái Ngọc Duy

parse-options.c +40 -0

@@ -574,6 +615,70 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx,
        return PARSE_OPT_COMPLETE;
 }
 
+/*
+ * Scan and may produce a new option[] array, which should be used
+ * instead of the original 'options'.
+ *
+ * Right now this is only used to preprocess and substitue
+ * OPTION_ALIAS.
+ */
+static struct option *preprocess_options(struct parse_opt_ctx_t *ctx,
+                                        const struct option *options)
+{
+       struct option *newopt;
+       int i, nr, alias;
+       int nr_aliases = 0;
+
+       for (nr = 0; options[nr].type != OPTION_END; nr++) {
+               if (options[nr].type == OPTION_ALIAS)
+                       nr_aliases++;
+       }
+
+       if (!nr_aliases)
+               return NULL;
+
+       ALLOC_ARRAY(newopt, nr + 1);
+       COPY_ARRAY(newopt, options, nr + 1);
+
+       /* each alias has two string pointers and NULL */
+       CALLOC_ARRAY(ctx->alias_groups, 3 * (nr_aliases + 1));
+
+       for (alias = 0, i = 0; i < nr; i++) {
+               const char *source;
+               int j;
+
+               if (newopt[i].type != OPTION_ALIAS)
+                       continue;
+
+               if (!newopt[i].long_name)
+                       BUG("An alias must have long option name");
+
+               source = newopt[i].value;
+

parse-options.c static int show_gitcomp(struct parse_opt_ctx_t *ctx, · line 667

Quoted 10 lines

parse-options.c +10 -0

+               for (j = 0; j < nr; j++) {
+                       const char *name = options[j].long_name;
+
+                       if (!name || strcmp(name, source))
+                               continue;
+
+                       if (options[j].type == OPTION_ALIAS)
+                               BUG("No please. Nested aliases are not supported.");
+
+                       memcpy(newopt + i, options + j, sizeof(*newopt));

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

Quoted 2 lines

(quoted diff) +2 -0

+                       break;
+               }

Signature · 1 line

Duy

Nguyễn Thái Ngọc Duy

2019-04-29 10:05 UTC #33 lore raw

PATCH v2 1/1 parse-options: don't emit "ambiguous option" for aliases

In reply to 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.

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 <[email protected]>

---
OK it's working for real this time. test-parse-options.c is also updated to for testing OPT_ALIAS.

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

builtin/clone.c +1 -4

index 50bde99618..703b7247ad 100644
@@ -98,10 +98,7 @@ static struct option builtin_clone_options[] = {
 		    N_("don't use local hardlinks, always copy")),
 	OPT_BOOL('s', "shared", &option_shared,
 		    N_("setup as shared repository")),
-	{ OPTION_CALLBACK, 0, "recursive", &option_recurse_submodules,
-	  N_("pathspec"), N_("initialize submodules in the clone"),
-	  PARSE_OPT_OPTARG | PARSE_OPT_HIDDEN, recurse_submodules_cb,
-	  (intptr_t)"." },
+	OPT_ALIAS(0, "recursive", "recurse-submodules"),
 	{ OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules,
 	  N_("pathspec"), N_("initialize submodules in the clone"),
 	  PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." },

parse-options.c +137 -6

202 lines not on this page · raw message

parse-options.h +6 -0

index 74cce4e7fc..9ed479f41d 100644
@@ -7,6 +7,7 @@ enum parse_opt_type {
 	OPTION_ARGUMENT,
 	OPTION_GROUP,
 	OPTION_NUMBER,
+	OPTION_ALIAS,
 	/* options with no arguments */
 	OPTION_BIT,
 	OPTION_NEGBIT,

@@ -181,6 +182,9 @@ struct option {
 	  N_("no-op (backward compatibility)"),		\
 	  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, parse_opt_noop_cb }
 
+#define OPT_ALIAS(s, l, source_long_name) \
+	{ OPTION_ALIAS, (s), (l), (source_long_name) }
+
 /*
  * parse_options() will filter out the processed options and leave the
  * non-option arguments in argv[]. argv0 is assumed program name and

@@ -256,6 +260,8 @@ struct parse_opt_ctx_t {
 	const char *opt;
 	int flags;
 	const char *prefix;
+	const char **alias_groups; /* must be in groups of 3 elements! */
+	struct option *updated_options;
 };
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,

t/helper/test-parse-options.c +3 -0

index cc88fba057..76cfb87588 100644
@@ -149,6 +149,9 @@ int cmd__parse_options(int argc, const char **argv)
 		OPT_CALLBACK(0, "expect", &expect, "string",
 			     "expected output in the variable dump",
 			     collect_expect),
+		OPT_GROUP("Alias"),
+		OPT_STRING('A', "alias-source", &string, "string", "get a string"),
+		OPT_ALIAS('Z', "alias-target", "alias-source"),
 		OPT_END(),
 	};
 	int i;

t/t0040-parse-options.sh +17 -0

index 800b3ea5f5..cebc77fab0 100755
@@ -48,6 +48,12 @@ Standard options
     -q, --quiet           be quiet
     --expect <string>     expected output in the variable dump
 
+Alias
+    -A, --alias-source <string>
+                          get a string
+    -Z, --alias-target <string>
+                          get a string
+
 EOF
 
 test_expect_success 'test help' '

@@ -224,6 +230,17 @@ test_expect_success 'non ambiguous option (after two options it abbreviates)' '
 	test-tool parse-options --expect="string: 123" --st 123
 '
 
+test_expect_success 'Alias options do not contribute to abbreviation' '
+	test-tool parse-options --alias-source 123 >output &&
+	grep "^string: 123" output &&
+	test-tool parse-options --alias-target 123 >output &&
+	grep "^string: 123" output &&
+	test_must_fail test-tool parse-options --alias &&
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	test-tool parse-options --alias 123 >output &&
+	grep "^string: 123" output
+'
+
 cat >typo.err <<\EOF
 error: did you mean `--boolean` (with two dashes ?)
 EOF

Signature · 1 line

2.21.0.1110.g9614c01b33

Junio C Hamano

2019-05-07 03:43 UTC #34 lore raw

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

Quoted 9 lines · Nguyễn Thái Ngọc Duy

But due to the way the options parsing machinery works this resulted in the rather absurd situation of:
    $ git clone --recurs [...]
    error: ambiguous option: recurs (could be --recursive or --recurse-submodules)
Add OPT_ALIAS() to express this link between two or more options and use it in git-clone. Multiple aliases of an option could be written as

Quoted 7 lines

    OPT_ALIAS(0, "alias1", "original-name"),
    OPT_ALIAS(0, "alias2", "original-name"),
    ...
The current implementation is not exactly optimal in this case. But we can optimize it when it becomes a problem. So far we don't even have two aliases of any option.

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

Quoted 5 lines

t/t0040-parse-options.sh +0 -0
index 800b3ea5f5..cebc77fab0 100755

t/t0040-parse-options.sh Standard options · line 57

Quoted 10 lines

t/t0040-parse-options.sh +6 -0

@@ -48,6 +48,12 @@ Standard options
     -q, --quiet           be quiet
     --expect <string>     expected output in the variable dump
 
+Alias
+    -A, --alias-source <string>
+                          get a string
+    -Z, --alias-target <string>
+                          get a string
+
 EOF

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

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

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

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

Signed-off-by Signed-off-by: Junio C Hamano <[email protected]>

---
* Of course, if the right-edge need to be protected, we can do so as well.

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

t/t0040-parse-options.sh +47 ~~-47~~

index cebc77fab0..26373b5b72 100755
@@ -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' '

Duy Nguyen

2019-05-07 11:59 UTC #35 lore raw

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

Quoted 66 lines · Junio C Hamano

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

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

Signed-off-by Signed-off-by: Junio C Hamano <[email protected]>

---
* Of course, if the right-edge need to be protected, we can do so as well.

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

t/t0040-parse-options.sh +0 ~~-38~~

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

t/t0040-parse-options.sh test_description='our own option parser' · line 10

Quoted 10 lines

t/t0040-parse-options.sh +1 -9

-    -q, --quiet           be quiet
-    --expect <string>     expected output in the variable dump
-
-Alias
-    -A, --alias-source <string>
-                          get a string
-    -Z, --alias-target <string>
-                          get a string
-
+sed -e 's/^|//' >expect <<\EOF

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

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

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

Quoted 49 lines

t/t0040-parse-options.sh +46 -0

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

Signature · 1 line

Duy