Re: [PATCH 02/19] t1010: fix unnoticed failure on Windows

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Dec 9, 2021 at 11:28 AM Elijah Newren <newren@gmail.com> wrote:
> On Thu, Dec 9, 2021 at 12:22 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > 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.
>
> Is this true?  The names look like they were selected on the basis that
>     '.' < '/' < '0'
>
> We have multiple paths in the code that write tree structures, and
> it's important that the order be
>   100644 blob $OID1 a.
>   040000 tree $OID2 a
>   100644 blob $OID3 a0
>
> i.e. that 'a' as a tree object sorts as though it were actually named
> 'a/'.  I suspect the code might have been making sure that the
> different paths creating tree objects did so consistently, and the
> special handling of subdirectories is the edge case that needs careful
> checks.

I meant it in the sense that the tests don't care about the literal
names "a", "a.", "a0". They do care about ordering...

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

... which I hoped to convey to readers by saying "stable results ...
when input order is not consistent", but I guess I wasn't clear enough
or obvious enough, or this mention is too far removed from the "don't
care about the actual names" statement.

> > 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
> > missing `|| return 1` to the loop body in order to catch this sort of
> > problem in the future.
>
> The choice to replace '.' with '-' is excellent since '-' < '/' as well.

I can rewrite the earlier paragraph to mention name ordering so it's
more obvious that it has significance.

I am a bit hesitant about spamming the list with a reroll of this
lengthy series just for this change, though I can certainly do it if
you consider it important.