git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: Elijah Newren <newren@gmail.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Victoria Dye <vdye@github.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 2/2] ls-files: add --sparse option
Date: Fri, 10 Dec 2021 16:13:36 +0100	[thread overview]
Message-ID: <211210.86zgp8bi48.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <745e79e3-521d-58bd-2ddf-1da4e0f6fe26@gmail.com>


On Fri, Dec 10 2021, Derrick Stolee wrote:

> 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

  reply	other threads:[~2021-12-10 15:22 UTC|newest]

Thread overview: 51+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-16 15:38 [PATCH 0/2] Sparse index: fetch, pull, ls-files Derrick Stolee via GitGitGadget
2021-11-16 15:38 ` [PATCH 1/2] fetch/pull: use the sparse index Derrick Stolee via GitGitGadget
2021-11-16 15:38 ` [PATCH 2/2] ls-files: add --sparse option Derrick Stolee via GitGitGadget
2021-11-22 18:36   ` Elijah Newren
2021-11-22 19:44     ` Derrick Stolee
2021-11-23  2:07   ` Ævar Arnfjörð Bjarmason
2021-12-08 15:14     ` Derrick Stolee
2021-12-08 15:20       ` Derrick Stolee
2021-12-08 17:04       ` Elijah Newren
2021-12-08 18:23         ` Derrick Stolee
2021-12-08 18:36           ` Elijah Newren
2021-12-08 19:06             ` Derrick Stolee
2021-12-09 12:50               ` Ævar Arnfjörð Bjarmason
2021-12-10 13:57                 ` Derrick Stolee
2021-12-10 15:13                   ` Ævar Arnfjörð Bjarmason [this message]
2021-12-13 19:16                   ` Junio C Hamano
2021-12-16 14:11                     ` Derrick Stolee
2021-11-17  9:29 ` [PATCH 0/2] Sparse index: fetch, pull, ls-files Junio C Hamano
2021-11-17 15:28   ` Derrick Stolee
2021-11-18 22:13     ` Junio C Hamano
2021-11-23  1:57 ` Ævar Arnfjörð Bjarmason
2021-12-08 19:39 ` [PATCH v2 0/5] " Derrick Stolee via GitGitGadget
2021-12-08 19:39   ` [PATCH v2 1/5] fetch/pull: use the sparse index Derrick Stolee via GitGitGadget
2021-12-08 19:39   ` [PATCH v2 2/5] ls-files: add --sparse option Derrick Stolee via GitGitGadget
2021-12-09  5:08     ` Elijah Newren
2021-12-10 13:51       ` Derrick Stolee
2021-12-08 19:39   ` [PATCH v2 3/5] t1092: replace 'read-cache --table' with 'ls-files --sparse' Derrick Stolee via GitGitGadget
2021-12-09  5:19     ` Elijah Newren
2021-12-08 19:39   ` [PATCH v2 4/5] t1091/t3705: remove 'test-tool read-cache --table' Derrick Stolee via GitGitGadget
2021-12-09  5:20     ` Elijah Newren
2021-12-08 19:39   ` [PATCH v2 5/5] test-read-cache: remove --table, --expand options Derrick Stolee via GitGitGadget
2021-12-09  5:23   ` [PATCH v2 0/5] Sparse index: fetch, pull, ls-files Elijah Newren
2021-12-10 15:13   ` [PATCH v3 " Derrick Stolee via GitGitGadget
2021-12-10 15:13     ` [PATCH v3 1/5] fetch/pull: use the sparse index Derrick Stolee via GitGitGadget
2021-12-10 15:13     ` [PATCH v3 2/5] ls-files: add --sparse option Derrick Stolee via GitGitGadget
2021-12-10 15:13     ` [PATCH v3 3/5] t1092: replace 'read-cache --table' with 'ls-files --sparse' Derrick Stolee via GitGitGadget
2021-12-10 15:13     ` [PATCH v3 4/5] t1091/t3705: remove 'test-tool read-cache --table' Derrick Stolee via GitGitGadget
2021-12-10 15:13     ` [PATCH v3 5/5] test-read-cache: remove --table, --expand options Derrick Stolee via GitGitGadget
2021-12-10 16:16     ` [PATCH v3 0/5] Sparse index: fetch, pull, ls-files Ævar Arnfjörð Bjarmason
2021-12-10 18:45       ` Elijah Newren
2021-12-11  2:24         ` Ævar Arnfjörð Bjarmason
2021-12-11  4:45           ` Elijah Newren
2021-12-10 18:53     ` Elijah Newren
2021-12-22 14:20     ` [PATCH v4 " Derrick Stolee via GitGitGadget
2021-12-22 14:20       ` [PATCH v4 1/5] fetch/pull: use the sparse index Derrick Stolee via GitGitGadget
2021-12-22 14:20       ` [PATCH v4 2/5] ls-files: add --sparse option Derrick Stolee via GitGitGadget
2021-12-22 14:20       ` [PATCH v4 3/5] t1092: replace 'read-cache --table' with 'ls-files --sparse' Derrick Stolee via GitGitGadget
2021-12-22 14:20       ` [PATCH v4 4/5] t1091/t3705: remove 'test-tool read-cache --table' Derrick Stolee via GitGitGadget
2021-12-22 14:20       ` [PATCH v4 5/5] test-read-cache: remove --table, --expand options Derrick Stolee via GitGitGadget
2021-12-22 19:17       ` [PATCH v4 0/5] Sparse index: fetch, pull, ls-files Elijah Newren
2021-12-22 23:56         ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=211210.86zgp8bi48.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=stolee@gmail.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).