Re: [PATCH v2 1/3] t6423: add tests of dual directory rename plus add/add conflict

From: Elijah Newren <newren@gmail.com>
Date: Jul 1, 2022, 02:57 UTC
Message-ID: <CABPp-BEcojvfeuhp7rSi-O+9oEu4KpwPDwbKS-MiD1qCKde-CA@mail.gmail.com>
In-Reply-To: <220630.86tu82e9b8.gmgdl@evledraar.gmail.com>

On Thu, Jun 30, 2022 at 3:26 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 10 quoted lines

>
> On Thu, Jun 30 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
>
> > +test_setup_12l () {
> > +     test_create_repo 12l_$1 &&
>
> Can & should just be "git init", see f0d4d398e28 (test-lib: split up and
> deprecate test_create_repo(), 2021-05-10).

I've switched to "git init" and even encouraged others to do the same in other test scripts, but since literally every other test in this file uses test_create_repo, I think they should all be converted at once and just be consistent here. But, so we can stop having this conversation, after this series lands, I'll send one in to update the various merge testfiles that make heavy use of test_create_repo and convert them over.

Show 6 quoted lines

> > +     (
> > +             cd 12l_$1 &&
> > +
> > +             mkdir -p sub1 sub2
>
> The "-p" here isn't needed, you're not creating recursive directories.

Indeed; will fix.

> > +             git ls-files -s >out &&
> > +             test_line_count = 5 out &&
>
> Can't this just use test_stdout_line_count? Except...

Ooh, new function from late last year that I was unaware of. Yeah, I'm happy to start using it.

Show 19 quoted lines

> > +
> > +             git rev-parse >actual \
> > +                     :0:sub3/file :0:sub3/newfile :0:sub3/sub2/other \
> > +                     :2:sub1/sub2/new_add_add_file \
> > +                     :3:sub1/sub2/new_add_add_file &&
> > +             git rev-parse >expect \
> > +                     O:sub1/file  B:sub1/newfile O:sub2/other \
> > +                     A:sub2/new_add_add_file \
> > +                     B:sub1/sub2/new_add_add_file &&
> > +             test_cmp expect actual &&
> > +
> > +             git ls-files -o >actual &&
> > +             test_write_lines actual expect out >expect &&
> > +             test_cmp expect actual
>
> This test seems a bit odd, here we're just checking that we've created
> scratch files for the internal use of our test, why is it important that
> we have an "out" file, when the only reason we have it is because we
> needed it for checking the "ls-files" line count above?

Nah, you've misunderstood the purpose of the check. It isn't "make sure that these untracked files are present among whatever else might also be present", it's "make sure the merge step did not introduce *any* untracked files" (something the recursive backend periodically did, and they weren't cruft untracked files but stored actual merge results). There wasn't a nice easy check for that, the closest was to translate the requirement to "make sure the only untracked files are the ones explicitly added by this test script", which is the check you see here. I don't actually care about "actual", "expect", or "out", I just care that there aren't any _other_ untracked files.