On Thu, Dec 9, 2021 at 11:28 AM Elijah Newren [off-list ref] wrote:

On Thu, Dec 9, 2021 at 12:22 AM Eric Sunshine [off-list ref] wrote:

quoted

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

quoted

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.

quoted

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.