On Thu, Dec 9, 2021 at 11:28 AM Elijah Newren <newren@xxxxxxxxx> wrote: > On Thu, Dec 9, 2021 at 12:22 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
