Re: [PATCH 2/2] ls-files: add --sparse option

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 10, 2021, 15:13 UTC
Message-ID: <211210.86zgp8bi48.gmgdl@evledraar.gmail.com>
In-Reply-To: <745e79e3-521d-58bd-2ddf-1da4e0f6fe26@gmail.com>

On Fri, Dec 10 2021, Derrick Stolee wrote:

Show 50 quoted lines

> On 12/9/2021 7:50 AM, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Wed, Dec 08 2021, Derrick Stolee wrote:
>> 
>>> On 12/8/2021 1:36 PM, Elijah Newren wrote:
>>>> On Wed, Dec 8, 2021 at 10:23 AM Derrick Stolee <stolee@gmail.com> wrote:
>>>>>
>>>>> On 12/8/2021 12:04 PM, Elijah Newren wrote:
>>>>>>
>>>>>> This actually looks quite nice, though the magic '16' is kind of
>>>>>> annoying.  Could we get rid of that -- perhaps using something to rip
>>>>>> out the diff header, or using comm instead?
>>>>>
>>>>> What I really want is "remove the first two lines of this file"
>>>>
>>>> Is `tail -n +3` portable?  Looks like we have five uses of tail -n +N
>>>> in the testsuite, so it should be okay to use.
>>>
>>> Ah, that's the magic incantation. Sounds good.
>>>
>>>>> but perhaps "tail -n $(wc -l expect)" would suffice to avoid a
>>>>> magic number?
>>>>
>>>> That works too.
>>>
>>> If the "-n +X" syntax works, then I'll opt for that.
>> 
>> I think it should be per
>> https://pubs.opengroup.org/onlinepubs/009695299/utilities/tail.html
>> 
>> But isn't that "diff -u" non-portable, per GIT_TEST_CMP. I.e. sometimes
>> we'll fall back on "cmp" etc.
>
> You're talking about this hunk, right?
>
> if test -z "$GIT_TEST_CMP"
> then
> 	if test -n "$GIT_TEST_CMP_USE_COPIED_CONTEXT"
> 	then
> 		GIT_TEST_CMP="$DIFF -c"
> 	else
> 		GIT_TEST_CMP="$DIFF -u"
> 	fi
> fi
>
> This only switches from "diff -u" to "diff -c" if the
> GIT_TEST_CMP_USE_COPIED_CONTEXT variable is set, but it is not set
> by default. Thus, we are using "diff -u" by default throughout.
>
> Please let me know if I'm misreading this.

Yes and no. Yes it's not on by default, but the reason (well, maybe not *the* reason, but definitely one reason) we have GIT_TEST_CMP is because -u isn't portable.

It's not in POSIX, and not just in the theoretical sense, e.g. when I run tests on HP/UX it'll yell at me about not knowing about -u until I adjust GIT_TEST_CMP.

Thus you can't use "diff -u" as you did upthread without breaking the test on those platforms, and you can't execute "$GIT_TEST_CMP" either because it might use -c, and then your "skip the first three lines" might not hold.

So even if we avoid using "git" for the actual test helpers themselves usually using it in this case is probably fine, and a neat way out of this particular hassle without worrying about portability.

So I don't know if we want a diff here, but if so I think it would be neat to wrap that in say a "test_cmp_diff" which we'd just use as:

	test_cmp_diff a b <<-\EOF
        <unified diff here>
	EOF

Where under the hood we'd just run:

	git diff --no-index a b >actual
	cat >expect &&
	[ munge diff header here? ] &&
	test_cmp expect actual