[v1.1,2/19] t1010: fix unnoticed failure on Windows

Commit Message

Eric Sunshine Dec. 11, 2021, 9:58 a.m. UTC

On Microsoft Windows, a directory name should never end with a period.
Quoting from Microsoft documentation[1]:

    Do not end a file or directory name with a space or a period.
    Although the underlying file system may support such names, the
    Windows shell and user interface does not.

Naming a directory with a trailing period is indeed perilous:

    % git init foo
    % cd foo
    % mkdir a.
    % git status
    warning: could not open directory 'a./': No such file or directory

The t1010 "setup" test:

    for d in a a. a0
    do
        mkdir "$d" && echo "$d/one" >"$d/one" &&
        git add "$d"
    done &&

runs afoul of this Windows limitation, as can be observed when running
the test verbosely:

    error: open("a./one"): No such file or directory
    error: unable to index file 'a./one'
    fatal: adding files failed

The reason this problem has gone unnoticed for so long is twofold.
First, the failed `git add` is swallowed silently because the loop is
not terminated explicitly by `|| return 1` to signal the failure.
Second, none of the tests in this script care about the literal
directory names ("a", "a.", "a0") or the specific number of tree
entries. They care instead about the order of entries in the tree, and
that the tree synthesized in the index and created by `git write-tree`
matches the tree created by the output of `git ls-tree` fed into `git
mktree`, thus the absence of "a./one" has no impact on the tests.

Skipping these tests on Windows by, for instance, checking the
FUNNYNAMES predicate would avoid the problem, however, the funny-looking
name is not what is being tested here. Rather, the tests are about
checking that `git mktree` produces stable results for various input
conditions, such as when the input order is not consistent or when an
object is missing.

Therefore, resolve the problem simply by using a directory name which is
legal on Windows and sorts the same as "a.". While at it, add the
missing `|| return 1` to the loop body in order to catch this sort of
problem in the future.

[1]: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file

Signed-off-by: Eric Sunshine <[e-mail]>
Reviewed-by: Elijah Newren <[e-mail]>
---

This is a re-roll of patch 2/19 of es/test-chain-lint[1] to improve the
commit message in response to Elijah's review comments[2,3] since the v1
commit message was perhaps a bit too subtle[4].

Rather than spamming the list with a full v2 re-roll, I decided to
re-roll just this one patch with its minor commit message rewrite since
it was the only change requested, thus should be less of a burden on
reviewers, though perhaps a bit more burden on Junio (but it's such a
minor change that the original commit message is probably good enough
should Junio not pick up this single-patch re-roll).

[1]: https://lore.kernel.org/git/[e-mail]/
[2]: https://lore.kernel.org/git/CABPp-BGBZ6_CqbUg3=sK2b4yELC5NHHyH68_df22n=t=[e-mail]/
[3]: https://lore.kernel.org/git/CABPp-BFM5ZbFAzVfvDE3=[e-mail]/
[4]: https://lore.kernel.org/git/[e-mail]/

Range-diff against v1:
1:  4e7d8e8c7e ! 1:  95d419ed90 t1010: fix unnoticed failure on Windows
    @@ Commit message
         The reason this problem has gone unnoticed for so long is twofold.
         First, the failed `git add` is swallowed silently because the loop is
         not terminated explicitly by `|| return 1` to signal the failure.
    -    Second, none of the tests in this script care about the actual directory
    -    names or even the number of tree entries. They care only that the tree
    -    synthesized in the index and created by `git write-tree` matches the
    -    tree created by the output of `git ls-tree` fed into `git mktree`, and
    -    the failure of `git add "a./one"` doesn't change that outcome.
    +    Second, none of the tests in this script care about the literal
    +    directory names ("a", "a.", "a0") or the specific number of tree
    +    entries. They care instead about the order of entries in the tree, and
    +    that the tree synthesized in the index and created by `git write-tree`
    +    matches the tree created by the output of `git ls-tree` fed into `git
    +    mktree`, thus the absence of "a./one" has no impact on the tests.
     
         Skipping these tests on Windows by, for instance, checking the
         FUNNYNAMES predicate would avoid the problem, however, the funny-looking
    @@ Commit message
         object is missing.
     
         Therefore, resolve the problem simply by using a directory name which is
    -    legal on Windows (i.e. "a-" rather than "a."). While at it, add the
    +    legal on Windows and sorts the same as "a.". While at it, add the
         missing `|| return 1` to the loop body in order to catch this sort of
         problem in the future.
     
         [1]: https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file
     
         Signed-off-by: Eric Sunshine <[e-mail]>
    +    Reviewed-by: Elijah Newren <[e-mail]>
     
      ## t/t1010-mktree.sh ##
     @@ t/t1010-mktree.sh: TEST_PASSES_SANITIZE_LEAK=true

 t/t1010-mktree.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch

diff --git a/t/t1010-mktree.sh b/t/t1010-mktree.sh
index 48bfad07ab..3c08194526 100755
--- a/t/t1010-mktree.sh
+++ b/t/t1010-mktree.sh
@@ -6,10 +6,10 @@  TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
-	for d in a a. a0
+	for d in a a- a0
 	do
 		mkdir "$d" && echo "$d/one" >"$d/one" &&
-		git add "$d"
+		git add "$d" || return 1
 	done &&
 	echo zero >one &&
 	git update-index --add --info-only one &&