patch, 16 partstest-lib.sh: new test_commit args, simplification & fixes

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

I have a couple of outstanding patch serieses that I've pulled this from (ab/describe-tests-fix, ab/pickaxe-pcre2), as well as some other local work I've been having a chicken and egg problem with.

I think the approach of pulling out these various miscellaneous test fixes into their own topic makes the most sense, I'll then submit re-rolled versions of other topics that e.g. need these new test_commit arguments on top of this.

Ævar Arnfjörð Bjarmason (16):
  check-non-portable-shell: complain about "test" a/-o instead of &&/||
  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-tag 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: modernize test_create_repo() function
  test-lib-functions: normalize test_path_is_missing() debugging
  test-lib-functions: use "return 1" instead of "false"
  Revert and amend "test-lib-functions: assert correct parameter count"
  test-lib-functions: remove last two parameter count assertions

 t/check-non-portable-shell.pl       |   4 +
 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             | 125 ++++++++++++++--------------
 t/test-lib.sh                       |  36 ++++----
 15 files changed, 113 insertions(+), 158 deletions(-)

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 "||".

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.

1. https://lore.kernel.org/git/xmqqa6qkb5fi.fsf@gitster.g/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 4 ++++
 1 file changed, 4 insertions(+)

On Mon, Apr 12, 2021 at 7:09 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

It's not only Junio's dislike of `-a` and `-o` but also that they have long been considered obsolescent by POSIX[1]. GNU has likewise warned against it[2] for a few decades.

[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

Given that these flags are considered obsolescent, thus may someday be dropped from some implementations, these seem a better fit for the "portability issues" section than the "coding style preferences" section. (I'd probably just drop those section titles, though.)

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`. Crafting a regex to match more generally would be non-trivial, so this simpler match is a reasonable start. Okay.

"Eric Sunshine" <sunshine@sunshineco.com> wrote:

And also in Git's Documentation/CodingGuidelines, which motivates the advice with an example:

     test -n "$x" -a "$a" = "$b"

   is buggy and breaks when $x is "=", but

     test -n "$x" && test "$a" = "$b"

   does not have such a problem.

Matthieu Moy <git@matthieu-moy.fr> writes:

Good advice to cite Documentation/CodingGuidelines in the log message.

Thanks (and nice to hear from you again ;-).

Eric Sunshine <sunshine@sunshineco.com> writes:

Would it be a trivial improvement to do

    'test', followed by anything other than '&' or '|', and then
    followed by '-a' or '-o'

instead?

On Mon, Apr 12, 2021 at 2:38 PM Junio C Hamano <gitster@pobox.com> wrote:

That seems plausible and trivial enough. In fact, I think it eliminates a source of false-positives that Ævar's pattern can report, such as:

    test whatever && ls -a foo

and

    test whatever && foo -o outfile

among others.

There's no point in creating a repository or directory only to decide right afterwards that we're skipping all the tests.

So let's partially revert 06478dab4c (test-lib: retire $remove_trash variable, 2017-04-23) and move the decision about whether to skip all tests earlier.

I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf under --debug at the end of the test, 2017-04-24) for a bug we don't want to re-introduce.

While I'm at it let's move the HOME assignment to just before test_create_repo, it could be lower, but it seems better to set it before calling anything in test-lib-functions.sh

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I am not clear what the reasoning behind this change is. Is it correct to say that the idea flows like the following?

 0. If we do the $GIT_SKIP_TESTS check early, specifically, before
    we create the trash directory to run the tests in, we do not
    have to create and then remove it.

 1. If we naïvely do the above, test_done called in the "ah, the
    entirety of test is skipped" case will try to see if the trash
    directory exists and complain, so we need to add more code in
    that test_done codepath to avoid complaints.

 2. As a part of that "more code", we mark the fact that we created
    a new repository with $remove_trash variable.

Assuming that the above flow of thought is what is behind this patch, I think the patch is mostly sensible.

remove_trash needs to be cleared at the beginning, perhaps where store_arg_to and opt_required_arg are cleared, though, to protect us from a stray environment variable.

Thanks.

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

By the way, I did wonder if the call to test_done is even needed, or if it is even ready to be called (in other words, the machineries it tries to finalize like junit and HARNESS_ACTIVE are even initialized yet). I verified that calling test_done this early is fine with respect to the call it makes to finalize_junit_xml, though.

Reword the documentation for "test_commit --append" added in my 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

A follow-up commit will make the "echo" part of this configurable, and in any case saying "echo >>" rather than ">>" was redundant.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Stop setting the GIT_TEST_FRAMEWORK_SELFTEST variable. This was originally needed back in 4231d1ba99 (t0000: do not get self-test disrupted by environment warnings, 2018-09-20).

It hasn't been needed since I deleted the relevant code in test-lib.sh in c0eedbc009 (test-lib: remove check_var_migration, 2021-02-09), I just didn't notice that it was set here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0000-basic.sh | 4 ----
 1 file changed, 4 deletions(-)

In 76b8b8d05c (test-lib functions: document arguments to test_commit, 2021-01-12) I added missing documentation to test_commit, but in less than a month later in 3803a3a099 (t: add --no-tag option to test_commit, 2021-02-09) we got another undocumented option. Let's fix that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 3 +++
 1 file changed, 3 insertions(+)

On Mon, Apr 12, 2021 at 7:09 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

This is difficult to understand for a native English speaker/reader due to the comma-splice[1]. A period or semicolon in place of the comma would fix it.

[1]: https://lore.kernel.org/git/CAPx1GvfFPWvJsj+uJV7RZrv1rgEpio=pk6rKF2UrjHebVY=LPA@mail.gmail.com/

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.

There's quite a few tests that could be simplified with this construct. I've picked one to convert in this change as a demonstration.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1403-show-ref.sh     |  6 ++----
 t/test-lib-functions.sh | 27 ++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 9 deletions(-)

On 2021-04-12 13:08:55+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

This comment applied to previous patch, but I think we need this patch to justify for optional "<tag>" argument is an error for "--no-tag"

----8<----

Convert the setup of the describe tests to use test_commit when possible. This makes use of the new --annotated-tag option to test_commit.

Some of the setup here could simply be removed since the data being created wasn't important to any of the subsequent tests, so I've done so. E.g. assigning to the "one" variable was always useless, and just checking that we can describe HEAD after the first commit wasn't useful.

In the case of the "two" variable we could instead use the tag we just created. See 5312ab11fbf (Add describe test., 2007-01-13) for the initial version of this code. There's other cases here like redundant "test_tick" invocations, or the simplification of not echoing "X" to a file we're about to tag as "x", now we just use "x" in both cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 58 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

Add a --printf option to test_commit to allow writing to the file with "printf" instead of "echo".

This is useful for writing "\n", "\0" etc., in particular in combination with the --append option added in 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

I'm converting a few tests to use the new option rather than a manual printf/add/commit combination to demonstrate its usefulness. While I'm at it use "test_create_repo" where appropriate, and give the first/second commit a meaningful/more conventional log message in cases where no test cared about that message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1307-config-blob.sh    |  4 +---
 t/t2030-unresolve-info.sh |  3 +--
 t/t4006-diff-mode.sh      |  6 ++----
 t/t4030-diff-textconv.sh  |  8 ++------
 t/t5520-pull.sh           | 10 ++--------
 t/test-lib-functions.sh   | 12 ++++++++++--
 6 files changed, 18 insertions(+), 25 deletions(-)

On Mon, Apr 12, 2021 at 7:09 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Just a bit of pure bikeshedding...

The name "printf" has such strong association in programmer's minds with "%" and argument consumption that the name of this option alone almost begs people to take advantage of argument interpolation even though it's documented here as not allowing it. Taking into consideration that people often do not read documentation, `--printf` as the name of the option may be an unfortunate one. Perhaps it could be called `--raw` or something less likely to suggest argument interpolation.

This could be slightly confusing. I wonder if naming this variable `emit` would be clearer.

Eric Sunshine <sunshine@sunshineco.com> writes:

The reason we want to use 'printf' instead of 'echo' is because only some implementations of 'echo' honors '\t\n\r' etc., and 'echo' by others show these literally. Using printf(1) allows us to write these backslashed special characters universally.

So, I find 'raw' equally confusing, if not more.

Perhaps.

On Mon, Apr 12, 2021 at 5:27 PM Junio C Hamano <gitster@pobox.com> wrote:

I don't care for `--raw` either but couldn't think of anything better at the time. But perhaps a name such as `--allow-escapes` would be clearer, or perhaps not. `--c-style-escapes`?

Anyhow, it's pure bikeshedding as I mentioned in my original email, so not a big deal. I brought it up only because the very first thought that popped into my head when reading the commit message saying it was adding `--printf` was "oh, interesting; how do I specify the arguments to interpolate into the `printf` format string?".

Eric Sunshine <sunshine@sunshineco.com> writes:

It's printf(1) style escapes ;-)

On Tue, Apr 13 2021, Junio C Hamano wrote:

So, the conclusion of this thread is let's keep it as --printf?

On Thu, Apr 15, 2021 at 7:33 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

It was bikeshedding on my part, so I don't feel strongly. As mentioned, I only brought it up because my first thought was to wonder how interpolation would work. One might suggest --printf-escapes or --string-escapes but the audience for this is so narrow (Git developers) that the short and concise --printf is probably preferable.

Eric Sunshine <sunshine@sunshineco.com> writes:

After seeing you raised the issue, I wish we had a better option name than --printf to be even more clear, but the wish is not strong enough to make me say "let's stop until we come up with the perfect name". I am OK with --printf

Thanks.

Change a use of $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME added in 704fed9ea22 (tests: start moving to a different default main branch name, 2020-10-23) to simply discover the initial branch name of a repository set up in this function with "symbolic-ref --short".

That's something done in another test in 704fed9ea22, so doing it like this seems like an omission, or rather an overly eager search/replacement instead of fixing the test logic.

There are only three uses of the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable in the test suite, this gets rid of one of those.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Reformat an argument list changed in 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) to have the "-c" on the same line as the argument it sets. This whitespace-only change makes it easier to review a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Arrange for the advice about naming the initial branch not to be shown in the --verbose output of the test suite.

Since 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) some tests have been very chatty with repeated occurrences of this multi-line advice. Having it be this verbose isn't helpful for anyone in the context of git's own test suite, and it makes debugging tests that use their own "git init" invocations needlessly distracting.

By setting the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable early in test-lib.sh itself we'll squash the warning not only for test_create_repo(), as 675704c74dd explicitly intended, but also for other "git init" invocations.

And once we'd like to have this configuration set for all "git init" invocations in the test suite we can get rid of the init.defaultBranch configuration setting in test_create_repo(), as repo_default_branch_name() in refs.c will take the GIT_TEST_* variable over it being set.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 1 -
 t/test-lib.sh           | 5 +++++
 2 files changed, 5 insertions(+), 1 deletion(-)

On Mon, Apr 12, 2021 at 01:09:00PM +0200, Ævar Arnfjörð Bjarmason wrote:

This change got my hopes up that

  GIT_TEST_INSTALLED=/.../v1.6.0/bin ./t9999-foo.sh

will work again (it didn't yet support 'git init -c <...>'), but, alas, already the next patch will turn this into 'git init ... <trash-dir>', and v1.6.0 didn't support that, either, so it will remain broken... Oh, well.

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.

Respectively:

 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.

 3. We won't ever hit that "Cannot setup test environment"
    error.

    Checking the test environment sanity when doing "git init" dates
    back to eea420693be (t0000: catch trivial pilot errors.,
    2005-12-10) and 2ccd2027b01 (trivial: check, if t/trash directory
    was successfully created, 2006-01-05).

    We can also see it in another form a bit later in my own
    0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd
    $old, 2010-08-30).

    But since 2006f0adaee (t/test-lib: make sure Git has already been
    built, 2012-09-17) we already check if we have a built git
    earlier.

    The one thing this was testing after that 2012 change was that
    we'd just built "git", but not "git-init", but since
    3af4c7156c4 (tests: respect GIT_TEST_INSTALLED when initializing
    repositories, 2018-11-12) we invoke "git", not "git-init".

    So all of that's been checked already, and we don't need to
    re-check it here.

 4. We don't need to move .git/hooks out of the way.

    That dates back to c09a69a83e3 (Disable hooks during tests.,
    2005-10-16), since then hooks became disabled by default in
    f98f8cbac01 (Ship sample hooks with .sample suffix, 2008-06-24).

    So the hooks were already disabled by default, but as can be seen
    from "mkdir .git/hooks" changes various tests needed to re-setup
    that directory. Now they no longer do.

 5. Since we don't need to move the .git/hooks directory we don't need
    the subshell here either.

In the end it turns out that all we needed was a plain "git init" invocation with a custom --template directory.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5406-remote-rejects.sh           |  1 -
 t/t5407-post-rewrite-hook.sh        |  2 --
 t/t5409-colorize-remote-messages.sh |  1 -
 t/test-lib-functions.sh             | 15 +++------------
 4 files changed, 3 insertions(+), 16 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

It might be unsafe to assume that we'd never have an 'enabled-by-default' hook in blt/ template directory forever, though.

On Mon, Apr 12 2021, Junio C Hamano wrote:

I would think that if we injected run-by-default hooks into the top-level repo creation any such breakage in the test suite would be a feature, i.e. we'd want to see and adjust for the breakage, because surely the same sort of breakage would occur in the wild.

FWIW I've got some WIP local patches for the "just ship a README in hooks/": https://lore.kernel.org/git/20180601070609.GC15578@sigill.intra.peff.net/

From memory the cumultive footprint of foot a -d run on the whole test suite is on the order of ~1GB, and ~100MB is just the hooks over and over again...

On Mon, Apr 12, 2021 at 01:09:01PM +0200, Ævar Arnfjörð Bjarmason wrote:

Or it will succeed if invoked as e.g. 'test_create_repo --bare repo'.

ENOSPC? Some rogue background process on Windows still desperately clinging to an open file descriptor to some file in the same directory, preventing 'rm -rf "$TRASH_DIRECTORY"' near the beginning of 'test-lib.sh' and interfering with 'git init'?

I agree that if we already have a 'git' binary that can run 'git version', then we can safely assume that it will be able to run 'git init' as well. It might be that 'git init' is buggy and segfaults, but that is not a "have you built things yet?" kind of error.

We needed the subshell because we changed directory in 'test_create_repo' (even you referenced 0d314ce834d above), not because we moved the hooks directory.

We don't need '--template', either; see below.

This patch removes this '|| exit', which is...

  - good: if 'test_create_repo' is invoked in a test case (and not in
    a subshell) and if it were to fail for some reason, then it won't
    abort the whole test script, but will fail only that test case.

  - bad: 'test_create_repo' is responsible for creating the repository
    in the trash directory as well; if that were to fail for any
    reason, then the test script will not be aborted early.

I think the 'exit' on error should be removed from 'test_create_repo', but the callsite in 'test-lib.sh' should become 'test_create_repo || exit 1'.

In any case, removing this '|| exit' is not mentioned in the commit message.

PATH is already set up to start with GIT_TEST_INSTALLED and/or GIT_EXEC_PATH before 'test_create_repo' is called to init the repo in the trash directory, so we could simply run 'git' and rely on PATH lookup choosing the right executable.

Likewise, GIT_TEMPLATE_DIR is already set up to the same value as in this '--template=...' option, so we could omit this option as well.

And after that all that would remain in this function is:

  git init "$1" >&3 2>&4

And those redirections are only needed when this function is called to initialize the repo in the trash directory, but not when it creates a repo in a test case. This makes me question the value of having this 'test_create_repo' helper function at all; there are already over 800 cases where we run plain 'git init' outside of 't0001-init.sh' instead of 'test_create_repo', though removing it would be definitely cause some churn with over 300 callsites.

On Thu, Apr 15, 2021 at 11:10:13PM +0200, SZEDER Gábor wrote:

Case in point: the bug I just reported in

  https://public-inbox.org/git/20210416211451.GP2947267@szeder.dev/

does break Git in a way that in one of our CI jobs 'git init' is unable to create the repository in the trash directory.

On Thu, Apr 15 2021, SZEDER Gábor wrote:

Which is fine, no? I'll make it clearer in the commit message, but I'm aiming to address plausible bugs due to these functions not erroring out in one way or another, if you call it with "--bare repo" that's clearly what you wanted, as opposed to "foo bar" and we don't create a repo under "bar".

I mean to say we won't hit it for the reasons we'd have that error there in the first place.

Of course "git init" can still error, but we'll catch that in other ways.

*nod*

Well, yes and no, a minor point but we'd need to do some juggling with saving the $PWD in a variable etc. if it wasn't for the subshell, I think that's why it got added in the first place.

Ah, interesting.

*nod*

I'll fix that code. FWIW I the "|| exit 1" is effectively not removed in almost all cases where "git init" would error, because in test-lib.sh we cd to the just-init-ed directory, and if we can't we'll fail.

But yes, it won't catch e.g. git-init dying midway through, us running out of space while populating that directory etc.

I'd missed how GIT_TEMPLATE_DIR was being set, that allows us to make this much simpler indeed!

Change the test_path_is_missing() to be consistent with related functions. Since 2caf20c52b7 (test-lib: user-friendly alternatives to test [-d|-f|-e], 2010-08-10) we've been ls -ld-ing the bad path and echo-ing $* if it exists. Let's just say that it exists instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Change a few functions that relied on a "false" being the last statement in the function to use an explicit "return 1" like the other functions in this file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

While not wrong per-se, this and a few other changes in this series are typical of your multi-patch series that unnecessarily consume reviewer bandwidth that could be better spent on other more important and correctness sensitive steps (like 07/16) in the same series.

You should find a good balance to avoid wearing out and distracting your reviewers---they are the most scarce resource around here that must be shared by other people, not just with other topics of your own.

Thanks.

This reverts and amends my my own e7884b353b7 (test-lib-functions: assert correct parameter count, 2021-02-12) in order to improve the -x output.

When I added these BUG assertions in e7884b353b7 I missed that this made the -x output much more verbose.

E.g. for each test_cmp invocation we'd now emit:

    + test_cmp expect actual
    + test 2 -ne 2
    + eval diff -u "$@"
    + diff -u expect actual

That "test 2 -ne 2" line is new in e7884b353b7. As noted in 45a2686441b (test-lib-functions: remove bug-inducing "diagnostics" helper param, 2021-02-12) we had buggy invocations of some of these functions with too many parameters.

Let's instead use "$@" instead of "$1" to achieve the same ends with no extra -x output verbosity. The "test" operator will die with an error if given more than one argument in these contexts, so using "$@" achieves the same goal.

The same goes for "cmp" and "diff -u" (which we typically use for test_cmp).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 41 ++++++++++++++++-------------------------
 1 file changed, 16 insertions(+), 25 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

What does it even mean to call

	test_path_is_file Documentation/ Makefile

with this patch applied?

If there were three files "COPYING Makefile", "COPYING", and "Makefile", what would happen when you did

	test_path_is_file COPYING Makefile

(without dq around them)?

I think this particular medicine is far worse than the symptom it tries to cure.

On Mon, Apr 12 2021, Junio C Hamano wrote:

We'll error with:

    test: foo: unexpected operator

And then say:

    File COPYING Makefile doesn't exist

I thought guarding just for the one-off development error of not using
the function correctly wasn't worth it, but I thought it made sense not
to litter all of this with:
	
	diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
	index 28b8826e565..0bd7367a07e 100644
	--- a/t/test-lib-functions.sh
	+++ b/t/test-lib-functions.sh
	@@ -690,6 +690,7 @@ test_expect_success () {
	 test_path_is_file () {
	 	if ! test -f "$@"
	 	then
	+		test $# -eq 1 || BUG "Do not call test_path_is_file() with more than one argument!"
	 		echo "File $@ doesn't exist"
	 		return 1
	 	fi

But I could do that if you think it's better.

We could also just emit $1 in the "echo". I don't feel strongly about
that, but think that's arguably worse, since having it be $@ means the
developer is more likely to notice the invalid usage right away.
	

On Apr 15 2021, Ævar Arnfjörð Bjarmason wrote:

If you want a single argument you should use "$*", not "$@".

Andreas.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Ah, so use of "$@" was intentional. That's clever (I thought it was a common typo people make when they mean "$*").

Of course, it would not work if the caller did a nonsense like so:

	test_path_is_file foo -o ok

but as long as we trust that the callers would not make stupid mistakes, this is OK. Is that the reasoning behind this removal of the BUG?

But this breaks our assumption that the caller would not be making stupid mistakes, so I am not sure if it is worth it. If we were to have a sanity check, shouldn't we do the check upfront, like the original?

Thanks.

On Thu, Apr 15 2021, Junio C Hamano wrote:

The reasoning is to get rid of verbosity in the trace output, while still effectively retaining the error checking.

Yes you could do "foo -o ok", but as my already-on-master fixes to the few misuses showed we only realistically have to worry about them being used with many normal looking file names (if that).

It's fine to do sanity checking if it's in the "else" branch where we're already emitting an error, and we only do so in the BUG case.

I.e. if it's written like e.g. this:
	
	test_path_is_file () {
		if test $# -ne 1
		then
			BUG "Do not call test_path_is_file() with more than one argument!"
		elif ! test -f "$@"
		then
			echo "File $@ doesn't exist"

Then with e.g.:

    ./t3600-rm.sh  --run=1-3 -vx

We get:
	
	+ test_path_is_file foo
	+ test 1 -ne 1
	+ test -f foo
	+ git ls-files --error-unmatch foo

But if we, as my patch does, piggy-pack on the test-built in to panic on
too many arguments we get the much more succinct:
	
	+ test_path_is_file foo
	+ test -f foo
	+ git ls-files --error-unmatch foo

I think that's the only trace output that matters, having "test 2 -ne 1" or whatever in the case where we're just about to invoke BUG anyway is fine.

On Mon, Apr 12, 2021 at 01:09:04PM +0200, Ævar Arnfjörð Bjarmason wrote:

I prefer the current check for its explicitness over the implicit and somewhat cryptic approach introduced in this patch. I hope that sooner or later I'll finish up my patch series to suppress '-x' output from test helper functions, and then this issue will become moot anyway.

SZEDER Gábor <szeder.dev@gmail.com> writes:

That is certainly a better approach than tweaking each call site of BUG.

The interface to BUG is "write a code to determine condition and then die by calling BUG", which means under '-x' you are bound to see the trace from "code to determine condition" part.

I wonder if introducing a BUG_ON helper function that

 - turns off '-x' trace upon entry;
 - takes a condition as one of its arguments and evals it;
 - issues a message and dies if needed;
 - otherwise arranges to turn '-x' trace on and return.

would solve it well?

Thanks.

On Fri, Apr 16 2021, SZEDER Gábor wrote:

Fair enough, I think it's worth it to have a bit of a non-obvious pattern there for less trace verbosity across the board.

That sounds like an interesting feature for those who want it, but it's entirely orthagonal to the direction this patch is taking.

I'd like to have trace output for when tests descend into test-lib-functions.sh and friends, I'd just like the most frequently used code there to not be needlessly verbose.

That's not the same as wanting to hide it entirely, i.e. treat test_path_is_file et al as though they were shell built-ins.

Remove a couple of parameter count assertions where we'll now silently do the wrong thing if given too many parameters, unlike the "$@" cases in the preceding commit where "test" etc. handle the check for usi.

In the case of "test_file_size", the "test-tool" we're invoking is happy to accept N parameters (it'll print out all N sizes). Let's just use "$@" in that case anyway, there's only a few callers, and eventually those should probably be moved to use the test-tool directly.

That only leaves test_line_count, I suppose I could leave that one alone, but since it's the only common function left that does this assertion let's remove it for the brevity of the -x output and consistency with other functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

What's "usi" (not a "cow", I presume)?

If test_line_count is given 2 or 4 parameters, we'd call BUG---does that "silently do the wrong thing"? Why?

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I guess I'm expected to discard the other topics that have been expecting rerolls from my tree to make room for this, queue this topic and wait it to stabilize, before taking the rerools of these other topics.

I find it a bit distracting to retract multiple topics that have been already reviewed and reorganizing them in a new topic. The reviewer resource spent for the last round might not be completely lost, but for reviewers it makes it unnecessarily harder to verify which review comments have been addressed and which parts haven't changed, so the net effect is to require them a fresh review anyway. If you can entice reviewers to review this new series and making them giving it priority over other topics in flight, that is.

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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
    @@ 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

Stop setting the GIT_TEST_FRAMEWORK_SELFTEST variable. This was originally needed back in 4231d1ba99 (t0000: do not get self-test disrupted by environment warnings, 2018-09-20).

It hasn't been needed since I deleted the relevant code in test-lib.sh in c0eedbc009 (test-lib: remove check_var_migration, 2021-02-09), I just didn't notice that it was set here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0000-basic.sh | 4 ----
 1 file changed, 4 deletions(-)

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).

These constructs are considered obsolescent by POSIX[1]. GNU has likewise warned against them[2] for a few decades.

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 <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 2 ++
 1 file changed, 2 insertions(+)

Reword the documentation for "test_commit --append" added in my 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

A follow-up commit will make the "echo" part of this configurable, and in any case saying "echo >>" rather than ">>" was redundant.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

In 76b8b8d05c (test-lib functions: document arguments to test_commit, 2021-01-12) I added missing documentation to test_commit, but in less than a month later in 3803a3a099 (t: add --no-tag option to test_commit, 2021-02-09) we got another undocumented option. Let's fix that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

Convert the setup of the describe tests to use test_commit when possible. This makes use of the new --annotated-tag option to test_commit.

Some of the setup here could simply be removed since the data being created wasn't important to any of the subsequent tests, so I've done so. E.g. assigning to the "one" variable was always useless, and just checking that we can describe HEAD after the first commit wasn't useful.

In the case of the "two" variable we could instead use the tag we just created. See 5312ab11fbf (Add describe test., 2007-01-13) for the initial version of this code. There's other cases here like redundant "test_tick" invocations, or the simplification of not echoing "X" to a file we're about to tag as "x", now we just use "x" in both cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 58 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

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 demonstration.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1403-show-ref.sh     |  6 ++----
 t/test-lib-functions.sh | 27 ++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 9 deletions(-)

Add a --printf option to test_commit to allow writing to the file with "printf" instead of "echo".

This is useful for writing "\n", "\0" etc., in particular in combination with the --append option added in 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

I'm converting a few tests to use the new option rather than a manual printf/add/commit combination to demonstrate its usefulness. While I'm at it use "test_create_repo" where appropriate, and give the first/second commit a meaningful/more conventional log message in cases where no test cared about that message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1307-config-blob.sh    |  4 +---
 t/t2030-unresolve-info.sh |  3 +--
 t/t4006-diff-mode.sh      |  6 ++----
 t/t4030-diff-textconv.sh  |  8 ++------
 t/t5520-pull.sh           | 10 ++--------
 t/test-lib-functions.sh   | 12 ++++++++++--
 6 files changed, 18 insertions(+), 25 deletions(-)

Change a use of $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME added in 704fed9ea22 (tests: start moving to a different default main branch name, 2020-10-23) to simply discover the initial branch name of a repository set up in this function with "symbolic-ref --short".

That's something done in another test in 704fed9ea22, so doing it like this seems like an omission, or rather an overly eager search/replacement instead of fixing the test logic.

There are only three uses of the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable in the test suite, this gets rid of one of those.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Reformat an argument list changed in 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) to have the "-c" on the same line as the argument it sets. This whitespace-only change makes it easier to review a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Arrange for the advice about naming the initial branch not to be shown in the --verbose output of the test suite.

Since 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) some tests have been very chatty with repeated occurrences of this multi-line advice. Having it be this verbose isn't helpful for anyone in the context of git's own test suite, and it makes debugging tests that use their own "git init" invocations needlessly distracting.

By setting the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable early in test-lib.sh itself we'll squash the warning not only for test_create_repo(), as 675704c74dd explicitly intended, but also for other "git init" invocations.

And once we'd like to have this configuration set for all "git init" invocations in the test suite we can get rid of the init.defaultBranch configuration setting in test_create_repo(), as repo_default_branch_name() in refs.c will take the GIT_TEST_* variable over it being set.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 1 -
 t/test-lib.sh           | 5 +++++
 2 files changed, 5 insertions(+), 1 deletion(-)

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

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. 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.

    Checking the test environment sanity when doing "git init" dates
    back to eea420693be (t0000: catch trivial pilot errors.,
    2005-12-10) and 2ccd2027b01 (trivial: check, if t/trash directory
    was successfully created, 2006-01-05).

    We can also see it in another form a bit later in my own
    0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd
    $old, 2010-08-30).

    But since 2006f0adaee (t/test-lib: make sure Git has already been
    built, 2012-09-17) we already check if we have a built git
    earlier.

    The one thing this was testing after that 2012 change was that
    we'd just built "git", but not "git-init", but since
    3af4c7156c4 (tests: respect GIT_TEST_INSTALLED when initializing
    repositories, 2018-11-12) we invoke "git", not "git-init".

    So all of that's been checked already, and we don't need to
    re-check it here.

 4. We don't need to move .git/hooks out of the way.

    That dates back to c09a69a83e3 (Disable hooks during tests.,
    2005-10-16), since then hooks became disabled by default in
    f98f8cbac01 (Ship sample hooks with .sample suffix, 2008-06-24).

    So the hooks were already disabled by default, but as can be seen
    from "mkdir .git/hooks" changes various tests needed to re-setup
    that directory. Now they no longer do.

 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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5406-remote-rejects.sh           |  1 -
 t/t5407-post-rewrite-hook.sh        |  2 --
 t/t5409-colorize-remote-messages.sh |  1 -
 t/test-lib-functions.sh             | 15 ++-------------
 t/test-lib.sh                       |  5 ++++-
 5 files changed, 6 insertions(+), 18 deletions(-)

On Sat, Apr 17, 2021 at 02:52:45PM +0200, Ævar Arnfjörð Bjarmason wrote:

Since we don't change directory anymore...

And then this dubious explanation will not be necessary.

Quoting myself from my review of the previous version of this patch:

  PATH is already set up to start with GIT_TEST_INSTALLED and/or
  GIT_EXEC_PATH before 'test_create_repo' is called to init the repo in
  the trash directory, so we could simply run 'git' and rely on PATH
  lookup choosing the right executable.

So this could be just:

  git init "$TRASH_DIRECTORY" >&3 2>&4 ||

On Sat, Apr 17 2021, SZEDER Gábor wrote:

Why dubious? That's why we had the subshell-ing. See 0d314ce834 (test-lib: use subshell instead of cd $new && .. && cd $old, 2010-08-30).

I don't mind rewording or not including some of this verbosity per-se, but I wonder why you're honing in on the subshell part in particular. Maybe there's something I'm missing...

The goal of the commit messsage is to point-by-point tear apart facets of the previous behavior, and assure the reader that e.g. the subshelling isn't needed for some other subtle reason.

Ah yes, I see that now. FWIW I managed to misread that in the last round as it applying only once we were calling the test-lib-functions.sh helper, but I see it's finishe setting up a few lines before the test_create_repo in test-lib.sh too, nice.

On Sat, Apr 17, 2021 at 11:45:41PM +0200, Ævar Arnfjörð Bjarmason wrote:

That commit doesn't mention hooks at all, so I'm not sure what I should see there. OTOH, you did specifically write "use a subshell instead of keeping track of the old working directory and cd-ing back when it's done"... which is in line with our best practices on how to change directories in our tests in order to reliably change back even in case of an error.

And that cd-ing back and forth was not added because of the hooks either, but because back in the day when 'test_create_repo' was created 'git init-db' could only create a new repository in the current directory.

Agreed.

The role of moving the hooks with respect to the subshell is not a subtlety, it is simply irrelevant. And while you are so focused on this irrevelancy you keep forgetting to mention the factor that actually does matter: that we won't change directories anymore.

There's no point in creating a repository or directory only to decide 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 tests earlier.

I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf under --debug at the end of the test, 2017-04-24) for a bug we don't want to re-introduce.

While I'm at it let's move the HOME assignment to just before test_create_repo, it could be lower, but it seems better to set it before calling anything in test-lib-functions.sh

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

This series makes the "-x" output less verbose for our various helper functions, mainly using the trickery of feeding "$@" to "test" to rely on it to die when given too many arguments.

It was split off from v2 of my larger test-lib.sh series as noted in its CL: https://lore.kernel.org/git/cover-00.12-00000000000-20210417T124424Z-avarab@gmail.com/

The range-diff below is to v1 of the relevant part of that series, showing what's changed just for these commits.

Ævar Arnfjörð Bjarmason (3):
  test-lib-functions: normalize test_path_is_missing() debugging
  Revert and amend "test-lib-functions: assert correct parameter count"
  test-lib-functions: remove last two parameter count assertions

 t/test-lib-functions.sh | 64 +++++++++++++++--------------------------
 1 file changed, 23 insertions(+), 41 deletions(-)

Range-diff:
1:  0813aa8e34e = 1:  c22e3f7764f test-lib-functions: normalize test_path_is_missing() debugging
2:  b6e9d971b40 < -:  ----------- test-lib-functions: use "return 1" instead of "false"
3:  0cd511206c4 ! 2:  6f9e09a2017 Revert and amend "test-lib-functions: assert correct parameter count"
    @@ Commit message
         assert correct parameter count, 2021-02-12) in order to improve the -x
         output.
     
    -    When I added these BUG assertions in e7884b353b7 I missed that this
    -    made the -x output much more verbose.
    +    The goal here is to get rid of the verbosity of having e.g. a "test 2
    +    -ne 2" line for every "test_cmp". We use "$@" as an argument to "test"
    +    to intentionally feed the "test" operator too many arguments if the
    +    functions are called with too many arguments, thus piggy-backing on it
    +    to check the number of arguments we get.
     
    -    E.g. for each test_cmp invocation we'd now emit:
    +    Before this for each test_cmp invocation we'd emit:
     
             + test_cmp expect actual
             + test 2 -ne 2
             + eval diff -u "$@"
             + diff -u expect actual
     
    -    That "test 2 -ne 2" line is new in e7884b353b7. As noted in
    +    That "test 2 -ne 2" line is new in my e7884b353b7. As noted in
         45a2686441b (test-lib-functions: remove bug-inducing "diagnostics"
         helper param, 2021-02-12) we had buggy invocations of some of these
         functions with too many parameters.
     
    -    Let's instead use "$@" instead of "$1" to achieve the same ends with
    -    no extra -x output verbosity. The "test" operator will die with an
    -    error if given more than one argument in these contexts, so using "$@"
    -    achieves the same goal.
    +    Now we'll get just:
     
    -    The same goes for "cmp" and "diff -u" (which we typically use for
    -    test_cmp).
    +        + test_cmp expect actual
    +        + eval diff -u "$@"
    +        + diff -u expect actual
    +
    +    This does not to the "right" thing in cases like:
    +
    +        test_path_is_file x -a y
    +
    +    Which will now turn into:
    +
    +        test -f x -a y
    +
    +    I consider that to be OK given the trade-off that any extra checking
    +    would produce more verbose trace output. As shown in 45a2686441b we
    +    had issues with these functions being invoked with multiple
    +    parameters (e.g. a glob) by accident, we don't need to be paranoid in
    +    guarding against hostile misuse from our own test suite.
    +
    +    While I'm at it change a few functions that relied on a "false" being
    +    the last statement in the function to use an explicit "return 1" like
    +    the other functions in this file.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/test-lib-functions.sh: test_external_without_stderr () {
     +	if ! test -f "$@"
      	then
     -		echo "File $1 doesn't exist"
    -+		echo "File $@ doesn't exist"
    - 		return 1
    +-		false
    ++		echo "File $* doesn't exist"
    ++		return 1
      	fi
      }
      
    @@ t/test-lib-functions.sh: test_external_without_stderr () {
     +	if ! test -d "$@"
      	then
     -		echo "Directory $1 doesn't exist"
    -+		echo "Directory $@ doesn't exist"
    - 		return 1
    +-		false
    ++		echo "Directory $* doesn't exist"
    ++		return 1
      	fi
      }
      
    @@ t/test-lib-functions.sh: test_external_without_stderr () {
     +	if ! test -e "$@"
      	then
     -		echo "Path $1 doesn't exist"
    -+		echo "Path $@ doesn't exist"
    - 		return 1
    +-		false
    ++		echo "Path $* doesn't exist"
    ++		return 1
      	fi
      }
      
    @@ t/test-lib-functions.sh: test_external_without_stderr () {
     +	if test -n "$(ls -a1 "$@" | egrep -v '^\.\.?$')"
      	then
     -		echo "Directory '$1' is not empty, it contains:"
    -+		echo "Directory '$@' is not empty, it contains:"
    ++		echo "Directory '$*' is not empty, it contains:"
      		ls -la "$1"
      		return 1
      	fi
    @@ t/test-lib-functions.sh: test_dir_is_empty () {
     +	if ! test -s "$@"
      	then
     -		echo "'$1' is not a non-empty file."
    -+		echo "'$@' is not a non-empty file."
    - 		return 1
    +-		false
    ++		echo "'$*' is not a non-empty file."
    ++		return 1
      	fi
      }
      
    @@ t/test-lib-functions.sh: test_dir_is_empty () {
     -	if test -e "$1"
     +	if test -e "$@"
      	then
    - 		echo "Path $1 exists!"
    +-		echo "Path $1 exists!"
    ++		echo "Path $* exists!"
      		false
    + 	fi
    + }
     @@ t/test-lib-functions.sh: test_expect_code () {
      # - not all diff versions understand "-u"
      
    @@ t/test-lib-functions.sh: verbose () {
      	then
     -		echo "'$1' is not empty, it contains:"
     -		cat "$1"
    -+		echo "'$@' is not empty, it contains:"
    ++		echo "'$*' is not empty, it contains:"
     +		cat "$@"
      		return 1
      	fi
4:  b4a018a63f3 ! 3:  b7b11a60bcd test-lib-functions: remove last two parameter count assertions
    @@ Metadata
      ## Commit message ##
         test-lib-functions: remove last two parameter count assertions
     
    -    Remove a couple of parameter count assertions where we'll now silently
    -    do the wrong thing if given too many parameters, unlike the "$@" cases
    -    in the preceding commit where "test" etc. handle the check for usi.
    +    Remove a couple of parameter count assertions where, unlike the
    +    preceding commit's migration to 'test -$x "$@"', we'll now silently do
    +    the "wrong" thing if given too many parameters. The benefit is less
    +    verbose trace output, as noted in the preceding commit.
     
         In the case of "test_file_size", the "test-tool" we're invoking is
         happy to accept N parameters (it'll print out all N sizes). Let's just
    -    use "$@" in that case anyway, there's only a few callers, and
    +    use "$@" in that case anyway. There's only a few callers, and
         eventually those should probably be moved to use the test-tool
         directly.
     

Change the test_path_is_missing() to be consistent with related functions. Since 2caf20c52b7 (test-lib: user-friendly alternatives to test [-d|-f|-e], 2010-08-10) we've been ls -ld-ing the bad path and echo-ing $* if it exists. Let's just say that it exists instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

This reverts and amends my my own e7884b353b7 (test-lib-functions: assert correct parameter count, 2021-02-12) in order to improve the -x output.

The goal here is to get rid of the verbosity of having e.g. a "test 2 -ne 2" line for every "test_cmp". We use "$@" as an argument to "test" to intentionally feed the "test" operator too many arguments if the functions are called with too many arguments, thus piggy-backing on it to check the number of arguments we get.

Before this for each test_cmp invocation we'd emit:

    + test_cmp expect actual
    + test 2 -ne 2
    + eval diff -u "$@"
    + diff -u expect actual

That "test 2 -ne 2" line is new in my e7884b353b7. As noted in 45a2686441b (test-lib-functions: remove bug-inducing "diagnostics" helper param, 2021-02-12) we had buggy invocations of some of these functions with too many parameters.

Now we'll get just:

    + test_cmp expect actual
    + eval diff -u "$@"
    + diff -u expect actual

This does not to the "right" thing in cases like:

    test_path_is_file x -a y

Which will now turn into:

    test -f x -a y

I consider that to be OK given the trade-off that any extra checking would produce more verbose trace output. As shown in 45a2686441b we had issues with these functions being invoked with multiple parameters (e.g. a glob) by accident, we don't need to be paranoid in guarding against hostile misuse from our own test suite.

While I'm at it change a few functions that relied on a "false" being the last statement in the function to use an explicit "return 1" like the other functions in this file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 51 +++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 30 deletions(-)

On Sat, Apr 17, 2021 at 8:58 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

My one concern about this change is that it enters the realm of "huh?". Although it's dead-simple to understand what this code is doing:

    test "$#" -ne 1 && BUG "1 param"
    if ! test -f "$1"

the replacement code:

    if ! test -f "$@"

can easily lead to head-scratching, wondering why the author chose to use "$@" rather than the more obvious "$1". This sort of unusual local idiom might normally deserve an in-code comment explaining why the more obvious code is not employed. However, adding a comment at each site would be overkill, so it might be the sort of thing to explain in documentation somewhere ("In order to make -x output less noisy, employ "$@" rather than explicitly checking function arguments when writing new test functions...".) Otherwise, the reader is forced to consult the commit message.

Not a major objection; just voicing the bit of unease I feel about it.

Channeling Dscho's inner Eric Sunshine[1]: s/to/do/

[1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103222235150.50@tvgsbejvaqbjf.bet/

On Sun, Apr 18 2021, Eric Sunshine wrote:

I'm including a well-placed comment in the re-roll of this. FWIW this would be much easier to follow with my rejected split-up of test-lib-functions.sh, then we could group these functions into one file: https://lore.kernel.org/git/20210209214159.22815-13-avarab@gmail.com/

Sunshine recursion error? :)

Remove a couple of parameter count assertions where, unlike the preceding commit's migration to 'test -$x "$@"', we'll now silently do the "wrong" thing if given too many parameters. The benefit is less verbose trace output, as noted in the preceding commit.

In the case of "test_file_size", the "test-tool" we're invoking is happy to accept N parameters (it'll print out all N sizes). Let's just use "$@" in that case anyway. There's only a few callers, and eventually those should probably be moved to use the test-tool directly.

That only leaves test_line_count, I suppose I could leave that one alone, but since it's the only common function left that does this assertion let's remove it for the brevity of the -x output and consistency with other functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

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

Since v2 the only changes are to 12/12, per SZEDER's comments we can count on $PATH this late in test-lib.sh, so do that, and clarify the commit message for that change per the discussion on v2.

Æ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                       | 39 +++++++++++--------
 15 files changed, 88 insertions(+), 119 deletions(-)

Range-diff against v2:
 -:  ---------- >  1:  a8b483bc77 check-non-portable-shell: check for "test <cond> -a/-o <cond>"
 -:  ---------- >  2:  39759d00ad test-lib: bring $remove_trash out of retirement
 -:  ---------- >  3:  d669ce3196 test-lib tests: remove dead GIT_TEST_FRAMEWORK_SELFTEST variable
 -:  ---------- >  4:  9313d35bf8 test-lib-functions: reword "test_commit --append" docs
 -:  ---------- >  5:  c916d648e4 test-lib-functions: document test_commit --no-tag
 -:  ---------- >  6:  9b6bf65ce1 test-lib functions: add an --annotated option to "test_commit"
 -:  ---------- >  7:  683b3ba3dd describe tests: convert setup to use test_commit
 -:  ---------- >  8:  dc0a863db7 test-lib functions: add --printf option to test_commit
 -:  ---------- >  9:  90bf55d2d4 submodule tests: use symbolic-ref --short to discover branch name
 -:  ---------- > 10:  e0a1e2fd52 test-lib: reformat argument list in test_create_repo()
 -:  ---------- > 11:  cedf5d383b test-lib: do not show advice about init.defaultBranch under --verbose
 1:  a3e20ef18f ! 12:  0dc0da9490 test-lib: split up and deprecate test_create_repo()
    @@ Commit message
     
         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
    +    from it.
     
    -    Reasons for why we can remove various code from test_create_repo():
    +    This leave us with a pass-through wrapper for "git init" in
    +    test-lib-functions.sh, in test-lib.sh we have the same, except for
    +    needing to redirect stdout/stderr, and emitting an error ourselves if
    +    it fails. We don't need to error() ourselves when test_create_repo()
    +    is invoked, as the invocation will be a part of a test's "&&"-chain.
    +
    +    Everything below this paragraph is a detailed summary of the history
    +    of test_create_repo() explaining why it's safe to remove the various
    +    things it was doing:
     
          1. "mkdir -p" isn't needed because "git init" itself will create
             leading directories if needed.
    @@ 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.
    +        See 0d314ce834 for when the subshell use got introduced for the
    +        convenience of not having to "cd" back and forth while setting up
    +        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".
    +        test-lib.sh.
    +
    +        We could still invoke test_create_repo() there, but as the
    +        invocation is now trivial and we don't have a good reason to use
    +        test_create_repo() elsewhere let's call "git init" there
    +        ourselves.
     
    -        Let's move that special behavior into test-lib.sh itself.
    +     8. We didn't need to resolve "git" as
    +        "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" in test_create_repo(),
    +        even for the use of test-lib.sh
     
    -    In the end it turns out that all we needed was a plain "git init"
    -    invocation.
    +        PATH is already set up in test-lib.sh to start with
    +        GIT_TEST_INSTALLED and/or GIT_EXEC_PATH before
    +        test_create_repo() (now "git init") is called.. So we can simply
    +        run "git" and rely on the PATH lookup choosing the right
    +        executable.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/test-lib.sh: rm -fr "$TRASH_DIRECTORY" || {
      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 ||
    ++	git init "$TRASH_DIRECTORY" >&3 2>&4 ||
     +	error "cannot run git init"
      else
      	mkdir -p "$TRASH_DIRECTORY"

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).

These constructs are considered obsolescent by POSIX[1]. GNU has likewise warned against them[2] for a few decades.

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 <avarab@gmail.com>
---
 t/check-non-portable-shell.pl | 2 ++
 1 file changed, 2 insertions(+)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I still do not understand why you have to insist on dashed operator as the first thing given to "test", like this:

IOW, what over-matching would we get if we simplified the condition like so?

    /\btest\s+[^&|]+\s+-a\s/
    /\btest\s+[^&|]+\s+-o\s/

The one in the patch would miss things like

	test "$a" = "$b" -o "$a" -lt "$b"
	test "$n" -a "$n" -lt 4

but the only thing that we care about is that a command that started with "test " has "-a" or "-o" before we see "&" or "|", no?

On Wed, Apr 21 2021, Junio C Hamano wrote:

Applying your suggestion results in these false positives:
	
	t4038-diff-combined.sh:135: error: "test A && test B" preferred to "test A -a B": git commit -m "test space change" -a &&
	t4038-diff-combined.sh:147: error: "test A && test B" preferred to "test A -a B": git commit -m "test other space changes" -a &&
	t6400-merge-df.sh:89: error: "test A || test B" preferred to "test A -o B": test 0 -eq $(git ls-files -o | wc -l)
	t6400-merge-df.sh:91: error: "test A || test B" preferred to "test A -o B": test 1 -eq $(git ls-files -o | wc -l)
	t6400-merge-df.sh:110: error: "test A || test B" preferred to "test A -o B": test 0 -eq $(git ls-files -o | wc -l)
	t6400-merge-df.sh:112: error: "test A || test B" preferred to "test A -o B": test 1 -eq $(git ls-files -o | wc -l)
	t6402-merge-rename.sh:639: error: "test A || test B" preferred to "test A -o B": test 0 -eq "$(git ls-files -o | wc -l)"
	t6402-merge-rename.sh:646: error: "test A || test B" preferred to "test A -o B": test 2 -eq "$(git ls-files -o | wc -l)"
	t6402-merge-rename.sh:686: error: "test A || test B" preferred to "test A -o B": test 0 -eq "$(git ls-files -o | wc -l)" &&
	t6402-merge-rename.sh:865: error: "test A || test B" preferred to "test A -o B": test 0 -eq $(git ls-files -o | wc -l) &&
	annotate-tests.sh:201: error: "test A && test B" preferred to "test A -a B": GIT_AUTHOR_NAME="E" GIT_AUTHOR_EMAIL="E at test dot git" git commit -a -m "norobots"

I'll just drop this from the re-roll of this series. Maybe you/someone has a better suggestion for something that's simple but still catches these cases, but in any case I'd like to not have this series blocked on this minor thing (which none of the rest of it needs).

On 2021-04-21 10:46:08+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

With: 7dbe8c8003, (check-non-portable-shell.pl: `wc -l` may have
leading WS, 2017-12-21)
Unless the situation has been changed, since.
I think those tests with quoted "$(.. | wc -l)" has been deemed
unportable and should be replaced with test_line_count anyway?
Does "test -eq" strip spaces from integer strings?

And I think we're working on moving "git" command to its own commmand instead of put it in the left of a pipe.

2 followed patch will clean them out

We will lose the exit status of "git ls-files" if it's being run in anywhere-but-not-final part of a pipe.

Let's send the output of "git ls-files" to a file first, and adjust the expected result for "git ls-files -o" since a new untracked file will be created as a side effect.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6400-merge-df.sh | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

We will lose the exit status of "git ls-files" if it's being run in anywhere-but-not-final part of a pipe.

Let's send the output of "git ls-files" to a file first, and adjust the expected result for "git ls-files -o" since a new untracked file will be created as a side effect.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t6402-merge-rename.sh | 148 +++++++++++++++++++++++++++-------------
 1 file changed, 101 insertions(+), 47 deletions(-)

On Wed, Apr 21, 2021 at 6:41 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

This idiom crops up so frequently in this test script that it almost begs for the introduction of a helper function rather than applying the manual transformation repeatedly. For instance, the helper might be called like this:

    count_ls_files 5 -s &&
    count_ls_files 3 -u &&
    count_ls_files 1 -u one~HEAD &&
    ...

The nice thing about having a helper function is that it can clean up after itself by not leaving a new file lying around, thus you wouldn't have to make adjustments to the expected number of untracked files (as mentioned in the commit message). If this is the sort of thing which comes up often enough (if there are more such cases beyond the two scripts you changed in this series), then it might make sense to promote the helper function to test-lib-functions.sh.

On Wed, Apr 21, 2021 at 12:55 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

The frequency with which this idiom crops up with commands beyond git-ls-files suggests the more general solution of supporting it directly in test-lib-functions.sh for any command. For instance:

    test_cmd_line_count = 3 git ls-files -u &&

Or, perhaps, a new mode of test_line_count():

    test_line_count = 3 -c git ls-files -u &&

Eric Sunshine <sunshine@sunshineco.com> writes:

That looks nice on paper, but may be going too far.

We may want to count the lines in the error message, or we may want to count the lines after filtering the output with pipe.

A test file that is dedicated to test ls-files with a file local helper "count_ls_files" smells like a better place to stop, at least to me.

On 2021-04-21 16:32:40-0700, Junio C Hamano <gitster@pobox.com> wrote:

Hm, I'm not sure how would we implement such mode:

* Will we run such command in a subprocess and and pipe to "wc -l"
  directly to avoid a temporary file, but we will lose the exit code
  of running command in process?
* Will we run in a separated chain? Thus, a temporary file would be
  created, skimming over test-lib-functions.sh, I couldn't find any
  convention for creating such files, except for specific use cases,
  let's say "*.config".
* Another clever hacks that I don't know *shrug*

Let's assume that we solve above puzzle.

Count the lines in the error messages is not too hard to be imagined, let's say by -c2 or something like that.

However, when it involved a pipe, things becomes complicated.

Hence, I'll stick with local help "count_ls_files" for now.

On Thu, Apr 22 2021, Đoàn Trần Công Danh wrote:

Just use a tempfile.

Yeah we happen not to have one of those in test-lib-functions.sh, but a bunch of helpers create those.

Yes, there's no portable way to both stream stdout/stderr in the correct order and to intercept it. You'd need a mkfifo. See also how the "--tee" mode works (i.e. they're squashed).

Here's an implementation that works, sans the fancy opts parsing etc. I just use -1 for "don't care". You can see "test_commit" etc. for how to do that.

It doesn't emit output, but that's just a matter of re-cat-ing the relevant files.

Yes we wouldn't have combined output when you do that, but I don't think we care, you can chain this thing, and most thing that would do so would only care about piping stdout/stderr to their own files anyway, and not that we emit one line of stdout, then another of stderr, another of stdout etc.

On Thu, Apr 22, 2021 at 11:04 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

That's what I had in mind when I suggested the function. The function would remove the tempfile if the check succeeded.

It's not clear why you manually catch the exit code of the command being run but then use &&-chain for the other parts.

An important reason I suggested creating a function was because the function could clean up after itself thus freeing the test author from worrying about adjusting counts in later tests (or later in the same test). So, just before this function returns, I'd expect to see:

    rm -f cnt.out cnt.err &&

On 2021-04-22 20:49:04+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

Hm, I figured out, just a bit insane for reviewing.
-----8<--------
Subject: [PATCH] test-lib-functions: add test_line_count_in helper

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t0000-basic.sh        |  7 +++++
 t/test-lib-functions.sh | 62 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

On Wed, Apr 21 2021, Eric Sunshine wrote:

Agreed, but I'd saythat it makes more sense to have something like:

	test_line_count = 1 -- git ls-files

Or maybe another function, but in any case not something specific to ls-files, but just a wrapper like the various "$@" wrappers in test-lib-functions.sh to count the lines emitted by an arbitrary command.

It would also leave things open for:

    test_line_count --stout=1 --stderr=2 -- git ls-files
    test_line_count --combined=3 -- git ls-files

Or whatever, to count the different output streams.

On Wed, Apr 21 2021, Đoàn Trần Công Danh wrote:

I think those patches are good in their own right, i.e. replacing things with more incremental helpers and test_cmp-like functions.

But I believe the code you're changing is not non-portable. It was using the output of "wc -l" with the "=" operator that wasn't portable.

These ones are all occurances that use "-eq".

And:

    test "0" -eq " 0"

etc., is true, which is why these pass on OSX and beyond.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Not necessarily.  The last one shown above

	test 0 -eq $(git ls-files -o | wc -l) &&

is still valid and correct with s/-eq/=/. It lets shell to remove excess whitespaces around output from some implementations of "wc", so textual '=' is perfectly fine.

Perhaps, but I am not sure how portable it is to rely on "-eq" ignoring the leading whitespaces.

A more conventional (read: time tested) way is to $(... | wc -l) *NOT* enclosed inside dq, so that the shell will strip out any excess spaces around it, and use '=', i.e.

    test 3 = $(... | wc -l)

and that is what we end up evaluating when we write

    test_line_count = 3 file

This does rely on that "wc -l" will run and produce _some_ output to be syntactically correct, though.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

This is utterly amusing.  Consider the suggestion withdrawn.

There's no point in creating a repository or directory only to decide 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 tests earlier.

I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf under --debug at the end of the test, 2017-04-24) for a bug we don't want to re-introduce.

While I'm at it let's move the HOME assignment to just before test_create_repo, it could be lower, but it seems better to set it before calling anything in test-lib-functions.sh

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Makes sense, but then remove_trash should be initialized to a non-empty string at the beginning, to avoid it leaking from end-user environment, just like the way store_arg_to and opt_required_arg are protected?

Stop setting the GIT_TEST_FRAMEWORK_SELFTEST variable. This was originally needed back in 4231d1ba99 (t0000: do not get self-test disrupted by environment warnings, 2018-09-20).

It hasn't been needed since I deleted the relevant code in test-lib.sh in c0eedbc009 (test-lib: remove check_var_migration, 2021-02-09), I just didn't notice that it was set here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0000-basic.sh | 4 ----
 1 file changed, 4 deletions(-)

In 76b8b8d05c (test-lib functions: document arguments to test_commit, 2021-01-12) I added missing documentation to test_commit, but in less than a month later in 3803a3a099 (t: add --no-tag option to test_commit, 2021-02-09) we got another undocumented option. Let's fix that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

Reword the documentation for "test_commit --append" added in my 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

A follow-up commit will make the "echo" part of this configurable, and in any case saying "echo >>" rather than ">>" was redundant.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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 demonstration.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1403-show-ref.sh     |  6 ++----
 t/test-lib-functions.sh | 27 ++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 9 deletions(-)

Convert the setup of the describe tests to use test_commit when possible. This makes use of the new --annotated-tag option to test_commit.

Some of the setup here could simply be removed since the data being created wasn't important to any of the subsequent tests, so I've done so. E.g. assigning to the "one" variable was always useless, and just checking that we can describe HEAD after the first commit wasn't useful.

In the case of the "two" variable we could instead use the tag we just created. See 5312ab11fbf (Add describe test., 2007-01-13) for the initial version of this code. There's other cases here like redundant "test_tick" invocations, or the simplification of not echoing "X" to a file we're about to tag as "x", now we just use "x" in both cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 58 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

Add a --printf option to test_commit to allow writing to the file with "printf" instead of "echo".

This is useful for writing "\n", "\0" etc., in particular in combination with the --append option added in 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

I'm converting a few tests to use the new option rather than a manual printf/add/commit combination to demonstrate its usefulness. While I'm at it use "test_create_repo" where appropriate, and give the first/second commit a meaningful/more conventional log message in cases where no test cared about that message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1307-config-blob.sh    |  4 +---
 t/t2030-unresolve-info.sh |  3 +--
 t/t4006-diff-mode.sh      |  6 ++----
 t/t4030-diff-textconv.sh  |  8 ++------
 t/t5520-pull.sh           | 10 ++--------
 t/test-lib-functions.sh   | 12 ++++++++++--
 6 files changed, 18 insertions(+), 25 deletions(-)

Change a use of $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME added in 704fed9ea22 (tests: start moving to a different default main branch name, 2020-10-23) to simply discover the initial branch name of a repository set up in this function with "symbolic-ref --short".

That's something done in another test in 704fed9ea22, so doing it like this seems like an omission, or rather an overly eager search/replacement instead of fixing the test logic.

There are only three uses of the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable in the test suite, this gets rid of one of those.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Reformat an argument list changed in 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) to have the "-c" on the same line as the argument it sets. This whitespace-only change makes it easier to review a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Arrange for the advice about naming the initial branch not to be shown in the --verbose output of the test suite.

Since 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) some tests have been very chatty with repeated occurrences of this multi-line advice. Having it be this verbose isn't helpful for anyone in the context of git's own test suite, and it makes debugging tests that use their own "git init" invocations needlessly distracting.

By setting the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable early in test-lib.sh itself we'll squash the warning not only for test_create_repo(), as 675704c74dd explicitly intended, but also for other "git init" invocations.

And once we'd like to have this configuration set for all "git init" invocations in the test suite we can get rid of the init.defaultBranch configuration setting in test_create_repo(), as repo_default_branch_name() in refs.c will take the GIT_TEST_* variable over it being set.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 1 -
 t/test-lib.sh           | 5 +++++
 2 files changed, 5 insertions(+), 1 deletion(-)

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.

This leave us with a pass-through wrapper for "git init" in test-lib-functions.sh, in test-lib.sh we have the same, except for needing to redirect stdout/stderr, and emitting an error ourselves if it fails. We don't need to error() ourselves when test_create_repo() is invoked, as the invocation will be a part of a test's "&&"-chain.

Everything below this paragraph is a detailed summary of the history of test_create_repo() explaining why it's safe to remove the various things it was doing:

 1. "mkdir -p" isn't needed because "git init" itself will create
    leading directories if needed.

 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.

    Checking the test environment sanity when doing "git init" dates
    back to eea420693be (t0000: catch trivial pilot errors.,
    2005-12-10) and 2ccd2027b01 (trivial: check, if t/trash directory
    was successfully created, 2006-01-05).

    We can also see it in another form a bit later in my own
    0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd
    $old, 2010-08-30).

    But since 2006f0adaee (t/test-lib: make sure Git has already been
    built, 2012-09-17) we already check if we have a built git
    earlier.

    The one thing this was testing after that 2012 change was that
    we'd just built "git", but not "git-init", but since
    3af4c7156c4 (tests: respect GIT_TEST_INSTALLED when initializing
    repositories, 2018-11-12) we invoke "git", not "git-init".

    So all of that's been checked already, and we don't need to
    re-check it here.

 4. We don't need to move .git/hooks out of the way.

    That dates back to c09a69a83e3 (Disable hooks during tests.,
    2005-10-16), since then hooks became disabled by default in
    f98f8cbac01 (Ship sample hooks with .sample suffix, 2008-06-24).

    So the hooks were already disabled by default, but as can be seen
    from "mkdir .git/hooks" changes various tests needed to re-setup
    that directory. Now they no longer do.

 5. Since we don't need to move the .git/hooks directory we don't need
    the subshell here either.

    See 0d314ce834 for when the subshell use got introduced for the
    convenience of not having to "cd" back and forth while setting up
    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.

    We could still invoke test_create_repo() there, but as the
    invocation is now trivial and we don't have a good reason to use
    test_create_repo() elsewhere let's call "git init" there
    ourselves.

 8. We didn't need to resolve "git" as
    "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" in test_create_repo(),
    even for the use of test-lib.sh

    PATH is already set up in test-lib.sh to start with
    GIT_TEST_INSTALLED and/or GIT_EXEC_PATH before
    test_create_repo() (now "git init") is called.. So we can simply
    run "git" and rely on the PATH lookup choosing the right
    executable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5406-remote-rejects.sh           |  1 -
 t/t5407-post-rewrite-hook.sh        |  2 --
 t/t5409-colorize-remote-messages.sh |  1 -
 t/test-lib-functions.sh             | 15 ++-------------
 t/test-lib.sh                       |  3 ++-
 5 files changed, 4 insertions(+), 18 deletions(-)

A fast re-roll of this test topic that a few things depend on, per https://lore.kernel.org/git/xmqqk0ow1plt.fsf@gitster.g/

Changes since v3:

 * Dropped the check-non-portable-shell change

   As noted in
   https://lore.kernel.org/git/87r1j42ffz.fsf@evledraar.gmail.com/ it
   wasn't needed for the rest of this series, so let's not hold it up
   for the discussion of if/how to check "-a" and "-o" invocations of
   "test".

 * Set the remove_trash= variable so we won't leak it from the
   environment.

Ævar Arnfjörð Bjarmason (11):
  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/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                       | 40 +++++++++++--------
 14 files changed, 87 insertions(+), 119 deletions(-)

Range-diff against v3:
 1:  a8b483bc77 <  -:  ---------- check-non-portable-shell: check for "test <cond> -a/-o <cond>"
 2:  39759d00ad !  1:  a76ea749bb test-lib: bring $remove_trash out of retirement
    @@ Commit message
         variable, 2017-04-23) and move the decision about whether to skip all
         tests earlier.
     
    +    Let's also fix a bug that was with us since abc5d372ec (Enable
    +    parallel tests, 2008-08-08): we would leak $remove_trash from the
    +    environment. We don't want this to error out, so let's reset it to the
    +    empty string first:
    +
    +         remove_trash=t GIT_SKIP_TESTS=t0001 ./t0001-init.sh
    +
         I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf
         under --debug at the end of the test, 2017-04-24) for a bug we don't
         want to re-introduce.
    @@ t/test-lib.sh: then
      fi
      
     +# Are we running this test at all?
    ++remove_trash=
     +this_test=${0##*/}
     +this_test=${this_test%%-*}
     +if match_pattern_list "$this_test" $GIT_SKIP_TESTS
 3:  d669ce3196 =  2:  de7be7844e test-lib tests: remove dead GIT_TEST_FRAMEWORK_SELFTEST variable
 4:  9313d35bf8 =  3:  709bc773fb test-lib-functions: reword "test_commit --append" docs
 5:  c916d648e4 =  4:  b67654334e test-lib-functions: document test_commit --no-tag
 6:  9b6bf65ce1 =  5:  3a166c9206 test-lib functions: add an --annotated option to "test_commit"
 7:  683b3ba3dd =  6:  981fc43ee6 describe tests: convert setup to use test_commit
 8:  dc0a863db7 =  7:  15057cdecf test-lib functions: add --printf option to test_commit
 9:  90bf55d2d4 =  8:  5d437f53ec submodule tests: use symbolic-ref --short to discover branch name
10:  e0a1e2fd52 =  9:  9ee13ee71b test-lib: reformat argument list in test_create_repo()
11:  cedf5d383b = 10:  6ba568df9f test-lib: do not show advice about init.defaultBranch under --verbose
12:  0dc0da9490 = 11:  311a9dba36 test-lib: split up and deprecate test_create_repo()

Stop setting the GIT_TEST_FRAMEWORK_SELFTEST variable. This was originally needed back in 4231d1ba99 (t0000: do not get self-test disrupted by environment warnings, 2018-09-20).

It hasn't been needed since I deleted the relevant code in test-lib.sh in c0eedbc009 (test-lib: remove check_var_migration, 2021-02-09), I just didn't notice that it was set here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0000-basic.sh | 4 ----
 1 file changed, 4 deletions(-)

There's no point in creating a repository or directory only to decide 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 tests earlier.

Let's also fix a bug that was with us since abc5d372ec (Enable parallel tests, 2008-08-08): we would leak $remove_trash from the environment. We don't want this to error out, so let's reset it to the empty string first:

     remove_trash=t GIT_SKIP_TESTS=t0001 ./t0001-init.sh

I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf under --debug at the end of the test, 2017-04-24) for a bug we don't want to re-introduce.

While I'm at it let's move the HOME assignment to just before test_create_repo, it could be lower, but it seems better to set it before calling anything in test-lib-functions.sh

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

Reword the documentation for "test_commit --append" added in my 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

A follow-up commit will make the "echo" part of this configurable, and in any case saying "echo >>" rather than ">>" was redundant.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

In 76b8b8d05c (test-lib functions: document arguments to test_commit, 2021-01-12) I added missing documentation to test_commit, but in less than a month later in 3803a3a099 (t: add --no-tag option to test_commit, 2021-02-09) we got another undocumented option. Let's fix that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

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 demonstration.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1403-show-ref.sh     |  6 ++----
 t/test-lib-functions.sh | 27 ++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 9 deletions(-)

Add a --printf option to test_commit to allow writing to the file with "printf" instead of "echo".

This is useful for writing "\n", "\0" etc., in particular in combination with the --append option added in 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

I'm converting a few tests to use the new option rather than a manual printf/add/commit combination to demonstrate its usefulness. While I'm at it use "test_create_repo" where appropriate, and give the first/second commit a meaningful/more conventional log message in cases where no test cared about that message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1307-config-blob.sh    |  4 +---
 t/t2030-unresolve-info.sh |  3 +--
 t/t4006-diff-mode.sh      |  6 ++----
 t/t4030-diff-textconv.sh  |  8 ++------
 t/t5520-pull.sh           | 10 ++--------
 t/test-lib-functions.sh   | 12 ++++++++++--
 6 files changed, 18 insertions(+), 25 deletions(-)

Convert the setup of the describe tests to use test_commit when possible. This makes use of the new --annotated-tag option to test_commit.

Some of the setup here could simply be removed since the data being created wasn't important to any of the subsequent tests, so I've done so. E.g. assigning to the "one" variable was always useless, and just checking that we can describe HEAD after the first commit wasn't useful.

In the case of the "two" variable we could instead use the tag we just created. See 5312ab11fbf (Add describe test., 2007-01-13) for the initial version of this code. There's other cases here like redundant "test_tick" invocations, or the simplification of not echoing "X" to a file we're about to tag as "x", now we just use "x" in both cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 58 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

Change a use of $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME added in 704fed9ea22 (tests: start moving to a different default main branch name, 2020-10-23) to simply discover the initial branch name of a repository set up in this function with "symbolic-ref --short".

That's something done in another test in 704fed9ea22, so doing it like this seems like an omission, or rather an overly eager search/replacement instead of fixing the test logic.

There are only three uses of the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable in the test suite, this gets rid of one of those.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Reformat an argument list changed in 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) to have the "-c" on the same line as the argument it sets. This whitespace-only change makes it easier to review a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Arrange for the advice about naming the initial branch not to be shown in the --verbose output of the test suite.

Since 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) some tests have been very chatty with repeated occurrences of this multi-line advice. Having it be this verbose isn't helpful for anyone in the context of git's own test suite, and it makes debugging tests that use their own "git init" invocations needlessly distracting.

By setting the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable early in test-lib.sh itself we'll squash the warning not only for test_create_repo(), as 675704c74dd explicitly intended, but also for other "git init" invocations.

And once we'd like to have this configuration set for all "git init" invocations in the test suite we can get rid of the init.defaultBranch configuration setting in test_create_repo(), as repo_default_branch_name() in refs.c will take the GIT_TEST_* variable over it being set.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 1 -
 t/test-lib.sh           | 5 +++++
 2 files changed, 5 insertions(+), 1 deletion(-)

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.

This leave us with a pass-through wrapper for "git init" in test-lib-functions.sh, in test-lib.sh we have the same, except for needing to redirect stdout/stderr, and emitting an error ourselves if it fails. We don't need to error() ourselves when test_create_repo() is invoked, as the invocation will be a part of a test's "&&"-chain.

Everything below this paragraph is a detailed summary of the history of test_create_repo() explaining why it's safe to remove the various things it was doing:

 1. "mkdir -p" isn't needed because "git init" itself will create
    leading directories if needed.

 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.

    Checking the test environment sanity when doing "git init" dates
    back to eea420693be (t0000: catch trivial pilot errors.,
    2005-12-10) and 2ccd2027b01 (trivial: check, if t/trash directory
    was successfully created, 2006-01-05).

    We can also see it in another form a bit later in my own
    0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd
    $old, 2010-08-30).

    But since 2006f0adaee (t/test-lib: make sure Git has already been
    built, 2012-09-17) we already check if we have a built git
    earlier.

    The one thing this was testing after that 2012 change was that
    we'd just built "git", but not "git-init", but since
    3af4c7156c4 (tests: respect GIT_TEST_INSTALLED when initializing
    repositories, 2018-11-12) we invoke "git", not "git-init".

    So all of that's been checked already, and we don't need to
    re-check it here.

 4. We don't need to move .git/hooks out of the way.

    That dates back to c09a69a83e3 (Disable hooks during tests.,
    2005-10-16), since then hooks became disabled by default in
    f98f8cbac01 (Ship sample hooks with .sample suffix, 2008-06-24).

    So the hooks were already disabled by default, but as can be seen
    from "mkdir .git/hooks" changes various tests needed to re-setup
    that directory. Now they no longer do.

 5. Since we don't need to move the .git/hooks directory we don't need
    the subshell here either.

    See 0d314ce834 for when the subshell use got introduced for the
    convenience of not having to "cd" back and forth while setting up
    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.

    We could still invoke test_create_repo() there, but as the
    invocation is now trivial and we don't have a good reason to use
    test_create_repo() elsewhere let's call "git init" there
    ourselves.

 8. We didn't need to resolve "git" as
    "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" in test_create_repo(),
    even for the use of test-lib.sh

    PATH is already set up in test-lib.sh to start with
    GIT_TEST_INSTALLED and/or GIT_EXEC_PATH before
    test_create_repo() (now "git init") is called.. So we can simply
    run "git" and rely on the PATH lookup choosing the right
    executable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5406-remote-rejects.sh           |  1 -
 t/t5407-post-rewrite-hook.sh        |  2 --
 t/t5409-colorize-remote-messages.sh |  1 -
 t/test-lib-functions.sh             | 15 ++-------------
 t/test-lib.sh                       |  3 ++-
 5 files changed, 4 insertions(+), 18 deletions(-)

On Wed, Apr 21, 2021 at 12:15:16PM +0200, Ævar Arnfjörð Bjarmason wrote:

Again: Since we don't need to change directories....

I think this is worse and even more misleading than even v2 was, for the reasons I mentioned earlier.

On Wed, Apr 21 2021, SZEDER Gábor wrote:

Between this & the thread at http://lore.kernel.org/git/20210420212731.GY2947267@szeder.dev I'm not sure what wording you'd be OK with, would you mind suggesting an alternative?

As far as I can see it's some subtlety of how we explain the cd-ing & subshelling we did as a combination of eedf8f97e58 (Abstract test_create_repo out for use in tests., 2006-02-17) and my 0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd $old, 2010-08-30).

Maybe you'd be happy with this:

    In the code being removed here we both "cd"'d to the repository
    before calling "init", and did so in a subshell. It's not important
    to do either, so both of those can be removed. We cd'd because this
    code grew from test-lib.sh code where we'd have done so already, see
    0d314ce834d, and later "cd"'d inside a subshell since 0d314ce834d to
    avoid having to keep track of an "old pwd" variable to cd back after
    the setup.

    Being in the repository directory made moving the hooks around
    easier (we wouldn't have to fully qualify the path), but since we're
    not moving the hooks anymore we don't need it for that reason
    either.

?

Changes since v4: Only a commit message change/re-wording per <87v98e1oj7.fsf@evledraar.gmail.com>.

Ævar Arnfjörð Bjarmason (11):
  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/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                       | 40 +++++++++++--------
 14 files changed, 87 insertions(+), 119 deletions(-)

Range-diff against v4:
 1:  a76ea749bb6 =  1:  75667f98f3a test-lib: bring $remove_trash out of retirement
 2:  de7be7844ea =  2:  55c9413f9cd test-lib tests: remove dead GIT_TEST_FRAMEWORK_SELFTEST variable
 3:  709bc773fb0 =  3:  361e34654e9 test-lib-functions: reword "test_commit --append" docs
 4:  b67654334e6 =  4:  2db68a4ac7c test-lib-functions: document test_commit --no-tag
 5:  3a166c92063 =  5:  4ceba3d404b test-lib functions: add an --annotated option to "test_commit"
 6:  981fc43ee69 =  6:  589eaf7a078 describe tests: convert setup to use test_commit
 7:  15057cdecfe =  7:  a0fe0640148 test-lib functions: add --printf option to test_commit
 8:  5d437f53ec8 =  8:  7fb8849ce66 submodule tests: use symbolic-ref --short to discover branch name
 9:  9ee13ee71bc =  9:  f67245ba40d test-lib: reformat argument list in test_create_repo()
10:  6ba568df9f4 = 10:  37338c88300 test-lib: do not show advice about init.defaultBranch under --verbose
11:  311a9dba36b ! 11:  7793311e5f1 test-lib: split up and deprecate test_create_repo()
    @@ Commit message
             from "mkdir .git/hooks" changes various tests needed to re-setup
             that directory. Now they no longer do.
     
    -     5. Since we don't need to move the .git/hooks directory we don't need
    -        the subshell here either.
    -
    -        See 0d314ce834 for when the subshell use got introduced for the
    -        convenience of not having to "cd" back and forth while setting up
    -        the hooks.
    +        This makes us implicitly depend on the default hooks being
    +        disabled, which is a good thing. If and when we'd have any
    +        on-by-default hooks (I see no reason we ever would) we'd want to
    +        see the subtle and not so subtle ways that would break the test
    +        suite.
    +
    +     5. We don't need to "cd" to the "$repo" directory at all anymore.
    +
    +        In the code being removed here we both "cd"'d to the repository
    +        before calling "init", and did so in a subshell.
    +
    +        It's not important to do either, so both of those can be
    +        removed. We cd'd because this code grew from test-lib.sh code
    +        where we'd have done so already, see eedf8f97e58 (Abstract
    +        test_create_repo out for use in tests., 2006-02-17), and later
    +        "cd"'d inside a subshell since 0d314ce834d to avoid having to keep
    +        track of an "old pwd" variable to cd back after the setup.
    +
    +        Being in the repository directory made moving the hooks around
    +        easier (we wouldn't have to fully qualify the path). Since we're
    +        not moving the hooks per #4 above we don't need to "cd" for that
    +        reason either.
     
          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

There's no point in creating a repository or directory only to decide 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 tests earlier.

Let's also fix a bug that was with us since abc5d372ec (Enable parallel tests, 2008-08-08): we would leak $remove_trash from the environment. We don't want this to error out, so let's reset it to the empty string first:

     remove_trash=t GIT_SKIP_TESTS=t0001 ./t0001-init.sh

I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf under --debug at the end of the test, 2017-04-24) for a bug we don't want to re-introduce.

While I'm at it let's move the HOME assignment to just before test_create_repo, it could be lower, but it seems better to set it before calling anything in test-lib-functions.sh

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

On 2021-04-23 09:21:05+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

s/test_done time/&,/ perhaps?

I guess it maybe only me (I'm not a native speaker of any Indo-European languages). But "because if" sounds weird to me. In my native tongue, I would say something like:

	We carry around ... variable because we'll hit the error ...
	if the directory is expectedly gone ...

s/at it/&,/ too?

Stop setting the GIT_TEST_FRAMEWORK_SELFTEST variable. This was originally needed back in 4231d1ba99 (t0000: do not get self-test disrupted by environment warnings, 2018-09-20).

It hasn't been needed since I deleted the relevant code in test-lib.sh in c0eedbc009 (test-lib: remove check_var_migration, 2021-02-09), I just didn't notice that it was set here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0000-basic.sh | 4 ----
 1 file changed, 4 deletions(-)

Reword the documentation for "test_commit --append" added in my 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

A follow-up commit will make the "echo" part of this configurable, and in any case saying "echo >>" rather than ">>" was redundant.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

In 76b8b8d05c (test-lib functions: document arguments to test_commit, 2021-01-12) I added missing documentation to test_commit, but in less than a month later in 3803a3a099 (t: add --no-tag option to test_commit, 2021-02-09) we got another undocumented option. Let's fix that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

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 demonstration.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1403-show-ref.sh     |  6 ++----
 t/test-lib-functions.sh | 27 ++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 9 deletions(-)

Convert the setup of the describe tests to use test_commit when possible. This makes use of the new --annotated-tag option to test_commit.

Some of the setup here could simply be removed since the data being created wasn't important to any of the subsequent tests, so I've done so. E.g. assigning to the "one" variable was always useless, and just checking that we can describe HEAD after the first commit wasn't useful.

In the case of the "two" variable we could instead use the tag we just created. See 5312ab11fbf (Add describe test., 2007-01-13) for the initial version of this code. There's other cases here like redundant "test_tick" invocations, or the simplification of not echoing "X" to a file we're about to tag as "x", now we just use "x" in both cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 58 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

On 2021-04-23 09:21:10+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

s/--annotated-tag/--annotate/

Otherwise, looks correct.

Add a --printf option to test_commit to allow writing to the file with "printf" instead of "echo".

This is useful for writing "\n", "\0" etc., in particular in combination with the --append option added in 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

I'm converting a few tests to use the new option rather than a manual printf/add/commit combination to demonstrate its usefulness. While I'm at it use "test_create_repo" where appropriate, and give the first/second commit a meaningful/more conventional log message in cases where no test cared about that message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1307-config-blob.sh    |  4 +---
 t/t2030-unresolve-info.sh |  3 +--
 t/t4006-diff-mode.sh      |  6 ++----
 t/t4030-diff-textconv.sh  |  8 ++------
 t/t5520-pull.sh           | 10 ++--------
 t/test-lib-functions.sh   | 12 ++++++++++--
 6 files changed, 18 insertions(+), 25 deletions(-)

On 2021-04-23 09:21:11+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

This is the second appearance of "You" in this file, and it's the first in the usage of test_commit Perhaps:

	Trailing "\n" won't be added automatically. This option
	supports nothing but the FORMAT of printf(1),
	not even its ARGUMENT(s).

Or something along the line.

Change a use of $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME added in 704fed9ea22 (tests: start moving to a different default main branch name, 2020-10-23) to simply discover the initial branch name of a repository set up in this function with "symbolic-ref --short".

That's something done in another test in 704fed9ea22, so doing it like this seems like an omission, or rather an overly eager search/replacement instead of fixing the test logic.

There are only three uses of the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable in the test suite, this gets rid of one of those.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Reformat an argument list changed in 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) to have the "-c" on the same line as the argument it sets. This whitespace-only change makes it easier to review a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Arrange for the advice about naming the initial branch not to be shown in the --verbose output of the test suite.

Since 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) some tests have been very chatty with repeated occurrences of this multi-line advice. Having it be this verbose isn't helpful for anyone in the context of git's own test suite, and it makes debugging tests that use their own "git init" invocations needlessly distracting.

By setting the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable early in test-lib.sh itself we'll squash the warning not only for test_create_repo(), as 675704c74dd explicitly intended, but also for other "git init" invocations.

And once we'd like to have this configuration set for all "git init" invocations in the test suite we can get rid of the init.defaultBranch configuration setting in test_create_repo(), as repo_default_branch_name() in refs.c will take the GIT_TEST_* variable over it being set.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 1 -
 t/test-lib.sh           | 5 +++++
 2 files changed, 5 insertions(+), 1 deletion(-)

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.

This leave us with a pass-through wrapper for "git init" in test-lib-functions.sh, in test-lib.sh we have the same, except for needing to redirect stdout/stderr, and emitting an error ourselves if it fails. We don't need to error() ourselves when test_create_repo() is invoked, as the invocation will be a part of a test's "&&"-chain.

Everything below this paragraph is a detailed summary of the history of test_create_repo() explaining why it's safe to remove the various things it was doing:

 1. "mkdir -p" isn't needed because "git init" itself will create
    leading directories if needed.

 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.

    Checking the test environment sanity when doing "git init" dates
    back to eea420693be (t0000: catch trivial pilot errors.,
    2005-12-10) and 2ccd2027b01 (trivial: check, if t/trash directory
    was successfully created, 2006-01-05).

    We can also see it in another form a bit later in my own
    0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd
    $old, 2010-08-30).

    But since 2006f0adaee (t/test-lib: make sure Git has already been
    built, 2012-09-17) we already check if we have a built git
    earlier.

    The one thing this was testing after that 2012 change was that
    we'd just built "git", but not "git-init", but since
    3af4c7156c4 (tests: respect GIT_TEST_INSTALLED when initializing
    repositories, 2018-11-12) we invoke "git", not "git-init".

    So all of that's been checked already, and we don't need to
    re-check it here.

 4. We don't need to move .git/hooks out of the way.

    That dates back to c09a69a83e3 (Disable hooks during tests.,
    2005-10-16), since then hooks became disabled by default in
    f98f8cbac01 (Ship sample hooks with .sample suffix, 2008-06-24).

    So the hooks were already disabled by default, but as can be seen
    from "mkdir .git/hooks" changes various tests needed to re-setup
    that directory. Now they no longer do.

    This makes us implicitly depend on the default hooks being
    disabled, which is a good thing. If and when we'd have any
    on-by-default hooks (I see no reason we ever would) we'd want to
    see the subtle and not so subtle ways that would break the test
    suite.

 5. We don't need to "cd" to the "$repo" directory at all anymore.

    In the code being removed here we both "cd"'d to the repository
    before calling "init", and did so in a subshell.

    It's not important to do either, so both of those can be
    removed. We cd'd because this code grew from test-lib.sh code
    where we'd have done so already, see eedf8f97e58 (Abstract
    test_create_repo out for use in tests., 2006-02-17), and later
    "cd"'d inside a subshell since 0d314ce834d to avoid having to keep
    track of an "old pwd" variable to cd back after the setup.

    Being in the repository directory made moving the hooks around
    easier (we wouldn't have to fully qualify the path). Since we're
    not moving the hooks per #4 above we don't need to "cd" for that
    reason either.

 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.

    We could still invoke test_create_repo() there, but as the
    invocation is now trivial and we don't have a good reason to use
    test_create_repo() elsewhere let's call "git init" there
    ourselves.

 8. We didn't need to resolve "git" as
    "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" in test_create_repo(),
    even for the use of test-lib.sh

    PATH is already set up in test-lib.sh to start with
    GIT_TEST_INSTALLED and/or GIT_EXEC_PATH before
    test_create_repo() (now "git init") is called.. So we can simply
    run "git" and rely on the PATH lookup choosing the right
    executable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5406-remote-rejects.sh           |  1 -
 t/t5407-post-rewrite-hook.sh        |  2 --
 t/t5409-colorize-remote-messages.sh |  1 -
 t/test-lib-functions.sh             | 15 ++-------------
 t/test-lib.sh                       |  3 ++-
 5 files changed, 4 insertions(+), 18 deletions(-)

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

I wasn't paying much attention to this series, and didn't look at the last step at all (as I saw somebody else is already deeply biting into it), but the earlier parts looked good to me.

It was painful to come back to the pile of topics after a week's interruption, though. While juggling ~50 topics, it is unfair to expect the maintainer to remember that a topic that is still going through iterations are depended on two other topics in flight (it is easier for contributors, who will be juggling far fewer number of topics at one time).

But I think I've sorted out the duplicates caused by forgetting that I had to rebase ab/pickaxe-pcre2 and ab/describe-tests-fix on top of this topic. Will be pushing out the result of today's integration sometime this evening, which is far from complete as I am still in the "catch up with what happened during my absense" mode.

Thanks.

On Fri, Apr 23 2021, Ævar Arnfjörð Bjarmason wrote:

SZEDER, Eric, Đoàn et al: does this v5 look good to you? Junio's suggested in [1] that this needs some final review feedback before advancing to 'next'. If you could take a look that would be great!

1. https://lore.kernel.org/git/xmqqsg30zdql.fsf@gitster.g/

On 2021-05-06 17:32:01+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I have some minor nit-picks here and there, I didn't look into 11/11. Otherwise, I think the series is fine, now.

Changes since v5 only trivial commit message / comment fixes suggested by Đoàn Trần Công Danh. Thanks! Link to v5: http://lore.kernel.org/git/cover-00.11-00000000000-20210423T072006Z-avarab@gmail.com

Ævar Arnfjörð Bjarmason (11):
  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/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             | 62 ++++++++++++++++++-----------
 t/test-lib.sh                       | 40 ++++++++++++-------
 14 files changed, 89 insertions(+), 119 deletions(-)

Range-diff against v5:
 1:  75667f98f3a !  1:  febf98e87a1 test-lib: bring $remove_trash out of retirement
    @@ Commit message
         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).
    +    is unexpectedly gone at test_done time we'll still want to hit the
    +    "trash directory already removed" error, but not if we never created
    +    the trash directory. See df4c0d1a792 (test-lib: abort when can't
    +    remove trash directory, 2017-04-20) for the addition of that error.
     
         So let's partially revert 06478dab4c (test-lib: retire $remove_trash
         variable, 2017-04-23) and move the decision about whether to skip all
    @@ Commit message
         under --debug at the end of the test, 2017-04-24) for a bug we don't
         want to re-introduce.
     
    -    While I'm at it let's move the HOME assignment to just before
    +    While I'm at it, let's move the HOME assignment to just before
         test_create_repo, it could be lower, but it seems better to set it
         before calling anything in test-lib-functions.sh
     
 2:  55c9413f9cd =  2:  6f360e37e84 test-lib tests: remove dead GIT_TEST_FRAMEWORK_SELFTEST variable
 3:  361e34654e9 =  3:  c2a0140fcd9 test-lib-functions: reword "test_commit --append" docs
 4:  2db68a4ac7c =  4:  1a675a6cfe5 test-lib-functions: document test_commit --no-tag
 5:  4ceba3d404b =  5:  2e0d69209e9 test-lib functions: add an --annotated option to "test_commit"
 6:  589eaf7a078 !  6:  07e8b63596b describe tests: convert setup to use test_commit
    @@ Commit message
         describe tests: convert setup to use test_commit
     
         Convert the setup of the describe tests to use test_commit when
    -    possible. This makes use of the new --annotated-tag option to
    -    test_commit.
    +    possible. This makes use of the new --annotate option to test_commit.
     
         Some of the setup here could simply be removed since the data being
         created wasn't important to any of the subsequent tests, so I've done
 7:  a0fe0640148 !  7:  5aaeadf01ff test-lib functions: add --printf option to test_commit
    @@ t/test-lib-functions.sh: debug () {
      #	Use ">>" instead of ">" when writing "<contents>" to "<file>"
     +#   --printf
     +#       Use "printf" instead of "echo" when writing "<contents>" to
    -+#       "<file>". You will need to provide your own trailing "\n". You
    -+#       can only supply the FORMAT for the printf(1), not its ARGUMENT(s).
    ++#       "<file>", use this to write escape sequences such as "\0", a
    ++#       trailing "\n" won't be added automatically. This option
    ++#       supports nothing but the FORMAT of printf(1), i.e. no custom
    ++#       ARGUMENT(s).
      #   --signoff
      #	Invoke "git commit" with --signoff
      #   --author <author>
 8:  7fb8849ce66 =  8:  0b31b1ffeff submodule tests: use symbolic-ref --short to discover branch name
 9:  f67245ba40d =  9:  c806f3888c5 test-lib: reformat argument list in test_create_repo()
10:  37338c88300 = 10:  105a95fb7a0 test-lib: do not show advice about init.defaultBranch under --verbose
11:  7793311e5f1 = 11:  217c5aed491 test-lib: split up and deprecate test_create_repo()

There's no point in creating a repository or directory only to decide 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 still want to hit the "trash directory already removed" error, but not if we never created the trash directory. See df4c0d1a792 (test-lib: abort when can't remove trash directory, 2017-04-20) for the addition of that error.

So let's partially revert 06478dab4c (test-lib: retire $remove_trash variable, 2017-04-23) and move the decision about whether to skip all tests earlier.

Let's also fix a bug that was with us since abc5d372ec (Enable parallel tests, 2008-08-08): we would leak $remove_trash from the environment. We don't want this to error out, so let's reset it to the empty string first:

     remove_trash=t GIT_SKIP_TESTS=t0001 ./t0001-init.sh

I tested this with --debug, see 4d0912a206 (test-lib.sh: do not barf under --debug at the end of the test, 2017-04-24) for a bug we don't want to re-introduce.

While I'm at it, let's move the HOME assignment to just before test_create_repo, it could be lower, but it seems better to set it before calling anything in test-lib-functions.sh

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib.sh | 32 ++++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

Stop setting the GIT_TEST_FRAMEWORK_SELFTEST variable. This was originally needed back in 4231d1ba99 (t0000: do not get self-test disrupted by environment warnings, 2018-09-20).

It hasn't been needed since I deleted the relevant code in test-lib.sh in c0eedbc009 (test-lib: remove check_var_migration, 2021-02-09), I just didn't notice that it was set here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0000-basic.sh | 4 ----
 1 file changed, 4 deletions(-)

Reword the documentation for "test_commit --append" added in my 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

A follow-up commit will make the "echo" part of this configurable, and in any case saying "echo >>" rather than ">>" was redundant.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

In 76b8b8d05c (test-lib functions: document arguments to test_commit, 2021-01-12) I added missing documentation to test_commit, but in less than a month later in 3803a3a099 (t: add --no-tag option to test_commit, 2021-02-09) we got another undocumented option. Let's fix that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

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 demonstration.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1403-show-ref.sh     |  6 ++----
 t/test-lib-functions.sh | 27 ++++++++++++++++++++++-----
 2 files changed, 24 insertions(+), 9 deletions(-)

Convert the setup of the describe tests to use test_commit when possible. This makes use of the new --annotate option to test_commit.

Some of the setup here could simply be removed since the data being created wasn't important to any of the subsequent tests, so I've done so. E.g. assigning to the "one" variable was always useless, and just checking that we can describe HEAD after the first commit wasn't useful.

In the case of the "two" variable we could instead use the tag we just created. See 5312ab11fbf (Add describe test., 2007-01-13) for the initial version of this code. There's other cases here like redundant "test_tick" invocations, or the simplification of not echoing "X" to a file we're about to tag as "x", now we just use "x" in both cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 58 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

Add a --printf option to test_commit to allow writing to the file with "printf" instead of "echo".

This is useful for writing "\n", "\0" etc., in particular in combination with the --append option added in 3373518cc8 (test-lib functions: add an --append option to test_commit, 2021-01-12).

I'm converting a few tests to use the new option rather than a manual printf/add/commit combination to demonstrate its usefulness. While I'm at it use "test_create_repo" where appropriate, and give the first/second commit a meaningful/more conventional log message in cases where no test cared about that message.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1307-config-blob.sh    |  4 +---
 t/t2030-unresolve-info.sh |  3 +--
 t/t4006-diff-mode.sh      |  6 ++----
 t/t4030-diff-textconv.sh  |  8 ++------
 t/t5520-pull.sh           | 10 ++--------
 t/test-lib-functions.sh   | 14 ++++++++++++--
 6 files changed, 20 insertions(+), 25 deletions(-)

Change a use of $GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME added in 704fed9ea22 (tests: start moving to a different default main branch name, 2020-10-23) to simply discover the initial branch name of a repository set up in this function with "symbolic-ref --short".

That's something done in another test in 704fed9ea22, so doing it like this seems like an omission, or rather an overly eager search/replacement instead of fixing the test logic.

There are only three uses of the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable in the test suite, this gets rid of one of those.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/lib-submodule-update.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Arrange for the advice about naming the initial branch not to be shown in the --verbose output of the test suite.

Since 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) some tests have been very chatty with repeated occurrences of this multi-line advice. Having it be this verbose isn't helpful for anyone in the context of git's own test suite, and it makes debugging tests that use their own "git init" invocations needlessly distracting.

By setting the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME variable early in test-lib.sh itself we'll squash the warning not only for test_create_repo(), as 675704c74dd explicitly intended, but also for other "git init" invocations.

And once we'd like to have this configuration set for all "git init" invocations in the test suite we can get rid of the init.defaultBranch configuration setting in test_create_repo(), as repo_default_branch_name() in refs.c will take the GIT_TEST_* variable over it being set.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 1 -
 t/test-lib.sh           | 5 +++++
 2 files changed, 5 insertions(+), 1 deletion(-)

Reformat an argument list changed in 675704c74dd (init: provide useful advice about init.defaultBranch, 2020-12-11) to have the "-c" on the same line as the argument it sets. This whitespace-only change makes it easier to review a subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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.

This leave us with a pass-through wrapper for "git init" in test-lib-functions.sh, in test-lib.sh we have the same, except for needing to redirect stdout/stderr, and emitting an error ourselves if it fails. We don't need to error() ourselves when test_create_repo() is invoked, as the invocation will be a part of a test's "&&"-chain.

Everything below this paragraph is a detailed summary of the history of test_create_repo() explaining why it's safe to remove the various things it was doing:

 1. "mkdir -p" isn't needed because "git init" itself will create
    leading directories if needed.

 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.

    Checking the test environment sanity when doing "git init" dates
    back to eea420693be (t0000: catch trivial pilot errors.,
    2005-12-10) and 2ccd2027b01 (trivial: check, if t/trash directory
    was successfully created, 2006-01-05).

    We can also see it in another form a bit later in my own
    0d314ce834d (test-lib: use subshell instead of cd $new && .. && cd
    $old, 2010-08-30).

    But since 2006f0adaee (t/test-lib: make sure Git has already been
    built, 2012-09-17) we already check if we have a built git
    earlier.

    The one thing this was testing after that 2012 change was that
    we'd just built "git", but not "git-init", but since
    3af4c7156c4 (tests: respect GIT_TEST_INSTALLED when initializing
    repositories, 2018-11-12) we invoke "git", not "git-init".

    So all of that's been checked already, and we don't need to
    re-check it here.

 4. We don't need to move .git/hooks out of the way.

    That dates back to c09a69a83e3 (Disable hooks during tests.,
    2005-10-16), since then hooks became disabled by default in
    f98f8cbac01 (Ship sample hooks with .sample suffix, 2008-06-24).

    So the hooks were already disabled by default, but as can be seen
    from "mkdir .git/hooks" changes various tests needed to re-setup
    that directory. Now they no longer do.

    This makes us implicitly depend on the default hooks being
    disabled, which is a good thing. If and when we'd have any
    on-by-default hooks (I see no reason we ever would) we'd want to
    see the subtle and not so subtle ways that would break the test
    suite.

 5. We don't need to "cd" to the "$repo" directory at all anymore.

    In the code being removed here we both "cd"'d to the repository
    before calling "init", and did so in a subshell.

    It's not important to do either, so both of those can be
    removed. We cd'd because this code grew from test-lib.sh code
    where we'd have done so already, see eedf8f97e58 (Abstract
    test_create_repo out for use in tests., 2006-02-17), and later
    "cd"'d inside a subshell since 0d314ce834d to avoid having to keep
    track of an "old pwd" variable to cd back after the setup.

    Being in the repository directory made moving the hooks around
    easier (we wouldn't have to fully qualify the path). Since we're
    not moving the hooks per #4 above we don't need to "cd" for that
    reason either.

 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.

    We could still invoke test_create_repo() there, but as the
    invocation is now trivial and we don't have a good reason to use
    test_create_repo() elsewhere let's call "git init" there
    ourselves.

 8. We didn't need to resolve "git" as
    "${GIT_TEST_INSTALLED:-$GIT_EXEC_PATH}/git$X" in test_create_repo(),
    even for the use of test-lib.sh

    PATH is already set up in test-lib.sh to start with
    GIT_TEST_INSTALLED and/or GIT_EXEC_PATH before
    test_create_repo() (now "git init") is called.. So we can simply
    run "git" and rely on the PATH lookup choosing the right
    executable.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t5406-remote-rejects.sh           |  1 -
 t/t5407-post-rewrite-hook.sh        |  2 --
 t/t5409-colorize-remote-messages.sh |  1 -
 t/test-lib-functions.sh             | 15 ++-------------
 t/test-lib.sh                       |  3 ++-
 5 files changed, 4 insertions(+), 18 deletions(-)

Ævar Arnfjörð Bjarmason wrote:

If this is deprecated why not add a warning?

  echo "warning: test_create_repo is deprecated in favor of git init" >&2

On Thu, May 13 2021, Felipe Contreras wrote:

Because like test_i18ncmp, test_i18ngrep or whatever this is in the state of "don't use this for new code", but annoying everyone who runs the test suite with loads of this output under -v would be too distracting.

The attention of the developer community is much better spent on one person doing a s/test_create_repo/git init/ patch than having everyone see this warning until somebody does that.

Ævar Arnfjörð Bjarmason wrote:

Then it's not really deprecated; it's merely disfavored.

To deprecate means to express disapproval of. If we are not going to express disapproval (i.e. annoy the users of the test suite), then it's not really deprecated. You can't eat your cake and have it too.

I agree we shouldn't throw a warning right now, but at some point in the future we should, *then* it will be deprecated.

Cheers.

On Thu, May 13 2021, Felipe Contreras wrote:

I don't feel strongly about either term, as long as it's consistent.

I do think that "deprecated" is consistent in the way I'm using it within the git project. If you grep various things we've "deprecated" we usually just do so by noting so in the docs. In this case the docs are the comments in test-lib.sh.

We do also have things like git rebase --preserve-merges which emit a warning, but we use "deprecated" for both. By contrast we don't have a single git for "git grep -i disfavor".

Anyway, for now I'd prefer to just have this land as-is and sort out such minor things later, given the v6 and having two other topics waiting on this...

Ævar Arnfjörð Bjarmason wrote:

Fair enough.

On 2021-05-10 16:18:59+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I skimmed over this series and I think it's in a good shape now. So, FWIW.

Acked-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Thanks.

This series makes the "-x" output less verbose for our various helper functions, mainly using the trickery of feeding "$@" to "test" to rely on it to die when given too many arguments.

Since v1 the only change is adding a comment as suggested by Eric Sunshine, it's not ideal as noted in <87pmyp2led.fsf@evledraar.gmail.com> (I wish these were just in one file, so we could have a comment at the top), but will aid future maintenance anyway.

Ævar Arnfjörð Bjarmason (3):
  test-lib-functions: normalize test_path_is_missing() debugging
  Revert and amend "test-lib-functions: assert correct parameter count"
  test-lib-functions: remove last two parameter count assertions

 t/test-lib-functions.sh | 72 +++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 42 deletions(-)

Range-diff against v1:
1:  a3e20ef18f < -:  ---------- test-lib: split up and deprecate test_create_repo()
2:  c22e3f7764 = 1:  fd33f6d71f test-lib-functions: normalize test_path_is_missing() debugging
3:  6f9e09a201 ! 2:  67ddd821df Revert and amend "test-lib-functions: assert correct parameter count"
    @@ Commit message
     
      ## t/test-lib-functions.sh ##
     @@ t/test-lib-functions.sh: test_external_without_stderr () {
    + }
    + 
      # debugging-friendly alternatives to "test [-f|-d|-e]"
    - # The commands test the existence or non-existence of $1
    +-# The commands test the existence or non-existence of $1
    ++# The commands test the existence or non-existence of
    ++# a given argument.
    ++#
    ++# The pattern of using "$@" to "test" instead of "$1" is not a bug. We
    ++# are counting on "test" to error on too many arguments if more than
    ++# one is given. Checking "$#" explicitly would lead to overly verbose
    ++# -x output.
      test_path_is_file () {
     -	test "$#" -ne 1 && BUG "1 param"
     -	if ! test -f "$1"
4:  b7b11a60bc = 3:  8fd51861b5 test-lib-functions: remove last two parameter count assertions

Change the test_path_is_missing() to be consistent with related functions. Since 2caf20c52b7 (test-lib: user-friendly alternatives to test [-d|-f|-e], 2010-08-10) we've been ls -ld-ing the bad path and echo-ing $* if it exists. Let's just say that it exists instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)