On Thu, Jun 30 2022, Elijah Newren wrote:
> On Thu, Jun 30, 2022 at 3:26 AM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> wrote:
>>
>> On Thu, Jun 30 2022, Elijah Newren via GitGitGadget wrote:
>>
>> > From: Elijah Newren <newren@xxxxxxxxx>
>>
>> > +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...
>> > + (
>> > + cd 12l_$1 &&
>> > +
>> > + mkdir -p sub1 sub2
>>
>> The "-p" here isn't needed, you're not creating recursive directories.
>
> Indeed; will fix.
Thanks!
>> > + 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!
> 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.
