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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 1, 2022, 09:29 UTC
Message-ID: <220701.86bku9b2cb.gmgdl@evledraar.gmail.com>
In-Reply-To: <CABPp-BEcojvfeuhp7rSi-O+9oEu4KpwPDwbKS-MiD1qCKde-CA@mail.gmail.com>

On Thu, Jun 30 2022, Elijah Newren wrote:

Show 20 quoted lines

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

Sorry, genuinely I didn't mean to mention it again, just saw it scroll past & wondered if it was intentional. I'm fine with keeping it...

Show 8 quoted lines

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

Thanks!

Show 34 quoted lines

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

Ah, thanks!

Show 5 quoted lines

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

I'm fine with keeping this as-is, but FWIW perhaps this pattern is more explicit about the intent:

	test_expect_success 'do merge stuff' '
		... &&
		rm -f expect actual &&
		git ls-files -o ':!out' >out &&
		test_must_be_empty out
	'

Or piping it to ".git/out", to avoid the path exclude, but like this is also fine:) Thanks.