[v2,00/12] test-lib.sh: new test_commit args, simplification & fixes

Message

Ævar Arnfjörð Bjarmason April 17, 2021, 12:52 p.m. UTC

This is a "various small fixes" series to test-lib.sh,
test-lib-functions.sh.

I've addressed the feedback on v1, the "test_create_repo" function is
now purely a dumb "git init" wrapper without any arguments, thanks to
SZEDER's observation.

The trickery with "test" arguments and "$@" later in the series seemed
more controversial, so I've peeled those patches off. I'll submit them
as another series, I've got various other serieses waiting on the
"test_commit" functions here so I'd like to get this in sooner than
later, rather than having it hang on the discussion around that.

Ævar Arnfjörð Bjarmason (12):
  check-non-portable-shell: check for "test <cond> -a/-o <cond>"
  test-lib: bring $remove_trash out of retirement
  test-lib tests: remove dead GIT_TEST_FRAMEWORK_SELFTEST variable
  test-lib-functions: reword "test_commit --append" docs
  test-lib-functions: document test_commit --no-tag
  test-lib functions: add an --annotated option to "test_commit"
  describe tests: convert setup to use test_commit
  test-lib functions: add --printf option to test_commit
  submodule tests: use symbolic-ref --short to discover branch name
  test-lib: reformat argument list in test_create_repo()
  test-lib: do not show advice about init.defaultBranch under --verbose
  test-lib: split up and deprecate test_create_repo()

 t/check-non-portable-shell.pl       |  2 +
 t/lib-submodule-update.sh           |  3 +-
 t/t0000-basic.sh                    |  4 --
 t/t1307-config-blob.sh              |  4 +-
 t/t1403-show-ref.sh                 |  6 +--
 t/t2030-unresolve-info.sh           |  3 +-
 t/t4006-diff-mode.sh                |  6 +--
 t/t4030-diff-textconv.sh            |  8 +---
 t/t5406-remote-rejects.sh           |  1 -
 t/t5407-post-rewrite-hook.sh        |  2 -
 t/t5409-colorize-remote-messages.sh |  1 -
 t/t5520-pull.sh                     | 10 +----
 t/t6120-describe.sh                 | 58 +++++++---------------------
 t/test-lib-functions.sh             | 60 ++++++++++++++++++-----------
 t/test-lib.sh                       | 41 ++++++++++++--------
 15 files changed, 90 insertions(+), 119 deletions(-)

Range-diff against v1:
 1:  8e4b4a2a216 !  1:  a8b483bc771 check-non-portable-shell: complain about "test" a/-o instead of &&/||
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    check-non-portable-shell: complain about "test" a/-o instead of &&/||
    +    check-non-portable-shell: check for "test <cond> -a/-o <cond>"
     
    -    Expand the t/check-non-portable-shell.pl checks to complain about the
    -    use of "-a" and "-o" to the "test" shell built-in to to mean "and" and
    -    "or", as opposed to using two "test" invocations with "&&" or "||".
    +    Add a check for -a/-o in "test", as a follow-up to the
    +    CodingGuidelines having recommended against their use since
    +    897f964c0dc (CodingGuidelines: avoid "test <cond> -a/-o <cond>",
    +    2014-05-20).
     
    -    There aren't any portability issues with using that construct that I
    -    know of, but since Junio expressed a dislike of it in [1] and we've
    -    currently got no such constructs let's add it to the lint checking. I
    -    had various in-flight and WIP patches that used this construct.
    +    These constructs are considered obsolescent by POSIX[1]. GNU has
    +    likewise warned against them[2] for a few decades.
     
    -    1. https://lore.kernel.org/git/xmqqa6qkb5fi.fsf@gitster.g/
    +    These will only match the simplistic forms of `test -X blah` (where
    +    "-X" is some single letter option), but will miss expressions such as
    +    `test "$foo" = bar`. We stop at "&" or "|" to try not to overmatch
    +    things like:
    +
    +        test whatever && ls -a foo
    +        test whatever && foo -o outfile
    +
    +    1. https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html#tag_20_128_16
    +    2. https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.70/html_node/Limitations-of-Builtins.html
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## t/check-non-portable-shell.pl ##
     @@ t/check-non-portable-shell.pl: sub err {
    - 	next if $line =~ s/\\$//;
    - 
    - 	$_ = $line;
    -+	# Portability issues
    - 	/\bcp\s+-a/ and err 'cp -a is not portable';
    - 	/\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)';
    - 	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
    -@@ t/check-non-portable-shell.pl: sub err {
    - 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
    - 	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
    - 		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
    -+	# Coding style preferences
    -+	/\btest\s+-[a-z]\s+.*?\s+-a\s+/ and err '"test A && test B" preferred to "test A -a B"';
    -+	/\btest\s+-[a-z]\s+.*?\s+-o\s+/ and err '"test A || test B" preferred to "test A -o B"';
    - 	$line = '';
    - 	# this resets our $. for each file
    - 	close ARGV if eof;
    + 	/^\s*declare\s+/ and err 'arrays/declare not portable';
    + 	/^\s*[^#]\s*which\s/ and err 'which is not portable (use type)';
    + 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (use =)';
    ++	/\btest\s+-[a-z]\s+[^&|]+\s+-a\s+/ and err '"test A && test B" preferred to "test A -a B"';
    ++	/\btest\s+-[a-z]\s+[^&|]+\s+-o\s+/ and err '"test A || test B" preferred to "test A -o B"';
    + 	/\bwc -l.*"\s*=/ and err '`"$(wc -l)"` is not portable (use test_line_count)';
    + 	/\bhead\s+-c\b/ and err 'head -c is not portable (use test_copy_bytes BYTES <file >out)';
    + 	/(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)';
 2:  44223ae777e !  2:  39759d00adf test-lib: bring $remove_trash out of retirement
    @@ Commit message
         test-lib: bring $remove_trash out of retirement
     
         There's no point in creating a repository or directory only to decide
    -    right afterwards that we're skipping all the tests.
    +    right afterwards that we're skipping all the tests. We can save
    +    ourselves the redundant "git init" or "mkdir" and "rm -rf" in this
    +    case.
    +
    +    We carry around the "$remove_trash" variable because if the directory
    +    is unexpectedly gone at test_done time we'll hit the error about it
    +    being unexpectedly gone added in df4c0d1a792 (test-lib: abort when
    +    can't remove trash directory, 2017-04-20).
     
         So let's partially revert 06478dab4c (test-lib: retire $remove_trash
         variable, 2017-04-23) and move the decision about whether to skip all
 3:  3d04553d710 =  3:  d669ce31961 test-lib tests: remove dead GIT_TEST_FRAMEWORK_SELFTEST variable
 4:  4ecce153b99 =  4:  9313d35bf86 test-lib-functions: reword "test_commit --append" docs
 5:  6df03776940 !  5:  c916d648e49 test-lib-functions: document test_commit --no-tag
    @@ t/test-lib-functions.sh: debug () {
      #   --author <author>
      #	Invoke "git commit" with --author <author>
     +#   --no-tag
    -+#	Do not tag the resulting commit, if supplied giving the
    -+#	optional "<tag>" argument is an error.
    ++#	Do not tag the resulting commit
      #
      # This will commit a file with the given contents and the given commit
      # message, and tag the resulting commit with the given tag name.
 6:  8d43fdd5865 !  6:  9b6bf65ce1c test-lib functions: add an --annotated-tag option to "test_commit"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    test-lib functions: add an --annotated-tag option to "test_commit"
    +    test-lib functions: add an --annotated option to "test_commit"
     
    -    Add an --annotated-tag option to test_commit. The tag will share the
    -    same message as the commit, and we'll call test_tick before creating
    -    it (unless --notick) is provided.
    +    Add an --annotated option to test_commit to create annotated tags. The
    +    tag will share the same message as the commit, and we'll call
    +    test_tick before creating it (unless --notick) is provided.
     
         There's quite a few tests that could be simplified with this
         construct. I've picked one to convert in this change as a
    @@ t/t1403-show-ref.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     
      ## t/test-lib-functions.sh ##
     @@ t/test-lib-functions.sh: debug () {
    + #	Invoke "git commit" with --author <author>
      #   --no-tag
    - #	Do not tag the resulting commit, if supplied giving the
    - #	optional "<tag>" argument is an error.
    + #	Do not tag the resulting commit
     +#   --annotate
     +#	Create an annotated tag with "--annotate -m <message>". Calls
    -+#	test_tick between making the commit and tag unless --notick is
    -+#	given.
    ++#	test_tick between making the commit and tag, unless --notick
    ++#	is given.
      #
      # This will commit a file with the given contents and the given commit
      # message, and tag the resulting commit with the given tag name.
    @@ t/test-lib-functions.sh: test_commit () {
     -	fi
     +		;;
     +	annotate)
    -+		if test "$tag$notick" = "annotate"
    ++		if test -z "$notick"
     +		then
     +			test_tick
     +		fi &&
 7:  ec4809e7466 =  7:  683b3ba3dd9 describe tests: convert setup to use test_commit
 8:  352eeff41c9 =  8:  dc0a863db72 test-lib functions: add --printf option to test_commit
 9:  e7432294215 =  9:  90bf55d2d42 submodule tests: use symbolic-ref --short to discover branch name
10:  634038c3a8d = 10:  e0a1e2fd529 test-lib: reformat argument list in test_create_repo()
11:  cdbff4df362 = 11:  cedf5d383b0 test-lib: do not show advice about init.defaultBranch under --verbose
12:  424caad189f ! 12:  a3e20ef18f7 test-lib: modernize test_create_repo() function
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    test-lib: modernize test_create_repo() function
    +    test-lib: split up and deprecate test_create_repo()
     
    -    Remove redundant "mkdir -p", argument number checking', test
    -    environment sanity checking, and disabling of hooks from
    -    test_create_repo(). As we'll see below these were all either redundant
    -    to other test other framework code, or to changes in git itself.
    +    Remove various redundant or obsolete code from the test_create_repo()
    +    function, and split up its use in test-lib.sh from what tests need
    +    from it, leaving us with a pass-through wrapper for "git init" in
    +    test-lib-functions.sh
     
    -    Respectively:
    +    Reasons for why we can remove various code from test_create_repo():
     
          1. "mkdir -p" isn't needed because "git init" itself will create
             leading directories if needed.
     
    -     2. We don't need to check the number of arguments anymore, instead
    -        we'll feed "git init" with "$@". It will die if given too many
    -        arguments.
    +     2. Since we're now a simple wrapper for "git init" we don't need to
    +        check that we have only one argument. If someone wants to run
    +        "test_create_repo --bare x" that's OK.
     
          3. We won't ever hit that "Cannot setup test environment"
             error.
    @@ Commit message
          5. Since we don't need to move the .git/hooks directory we don't need
             the subshell here either.
     
    +        That wasn't really needed for the .git/hooks either, but was being
    +        done for the convenience of not having to quote the path to the
    +        repository as we moved the hooks.
    +
    +     6. We can drop the --template argument and instead rely on the
    +        GIT_TEMPLATE_DIR set to the same path earlier in test-lib.sh. See
    +        8683a45d669 (Introduce GIT_TEMPLATE_DIR, 2006-12-19)
    +
    +     7. We only needed that ">&3 2>&4" redirection when invoked from
    +        test-lib.sh, and the same goes for needing the full path to "git".
    +
    +        Let's move that special behavior into test-lib.sh itself.
    +
         In the end it turns out that all we needed was a plain "git init"
    -    invocation with a custom --template directory.
    +    invocation.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
    @@ t/t5409-colorize-remote-messages.sh: test_description='remote messages are color
     
      ## t/test-lib-functions.sh ##
     @@ t/test-lib-functions.sh: test_atexit () {
    - # Most tests can use the created repository, but some may need to create more.
    + 		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_atexit_cleanup"
    + }
    + 
    +-# Most tests can use the created repository, but some may need to create more.
    ++# Deprecated wrapper for "git init", use "git init" directly instead
      # Usage: test_create_repo <directory>
      test_create_repo () {
     -	test "$#" = 1 ||
    @@ t/test-lib-functions.sh: test_atexit () {
     -		error "cannot run git init -- have you built things yet?"
     -		mv .git/hooks .git/hooks-disabled
     -	) || exit
    -+	"${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" \
    -+		init \
    -+		"--template=$GIT_BUILD_DIR/templates/blt/" "$@" >&3 2>&4
    ++	git init "$@"
      }
      
      # This function helps on symlink challenged file systems when it is not
    +
    + ## t/test-lib.sh ##
    +@@ t/test-lib.sh: rm -fr "$TRASH_DIRECTORY" || {
    + remove_trash=t
    + if test -z "$TEST_NO_CREATE_REPO"
    + then
    +-	test_create_repo "$TRASH_DIRECTORY"
    ++	"${GIT_TEST_INSTALLED:-$GIT_BUILD_DIR}/git$X" \
    ++		init \
    ++		"$TRASH_DIRECTORY" >&3 2>&4 ||
    ++	error "cannot run git init"
    + else
    + 	mkdir -p "$TRASH_DIRECTORY"
    + fi
13:  0813aa8e34e <  -:  ----------- test-lib-functions: normalize test_path_is_missing() debugging
14:  b6e9d971b40 <  -:  ----------- test-lib-functions: use "return 1" instead of "false"
15:  0cd511206c4 <  -:  ----------- Revert and amend "test-lib-functions: assert correct parameter count"
16:  b4a018a63f3 <  -:  ----------- test-lib-functions: remove last two parameter count assertions