git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Derrick Stolee <derrickstolee@github.com>
To: Josh Steadmon <steadmon@google.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, gitster@pobox.com, vdye@github.com,
	shaoxuan.yuan02@gmail.com, Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 2/4] show: integrate with the sparse index
Date: Mon, 18 Apr 2022 08:28:41 -0400	[thread overview]
Message-ID: <4c999e1f-e6bb-72e0-a27d-e072ce9487f8@github.com> (raw)
In-Reply-To: <Ylhs3t9nFS/IeO2Y@google.com>

On 4/14/2022 2:50 PM, Josh Steadmon wrote:
> On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>

>>  	# Asking "git show" for directories in the index
>> -	# does not work as implemented. The error message is
>> -	# different for a full checkout and a sparse checkout
>> -	# when the directory is outside of the cone.
>> +	# changes depending on the existence of a sparse index.
> 
> The wording here seems awkward after these changes are applied. Without
> other context, it makes it sound to me like the command(s) used to show
> a directory change depending on the existence of a sparse index, rather
> than the fact that the behavior of `git show` changes.

I can see that.

>> +	# The output of "git show" includes the way we referenced the
>> +	# objects, so strip that out.
>> +	test_line_count = 4 actual &&
>> +	tail -n 2 actual >actual-trunc &&
>> +	tail -n 2 expect >expect-trunc &&
>> +	test_cmp expect-trunc actual-trunc
>>  '
> 
> It's not specific to this commit, but in general I think the series of
> changes to this test would be easier to follow if we used hard-coded
> strings to compare against, rather than matching parts of files against
> each other. It makes it more clear to the reader exactly which behavior
> is changing, and can make it more obvious why certain output is
> undesirable. However, it would make the test more brittle to future
> changes.

The tests here are designed with this approach in mind: demonstrate
success by comparing to existing behavior. We don't want to be
coupled to the exact behavior of these commands, but we _do_ want to
demonstrate that the sparse-checkout or sparse-index features do not
change from the full-checkout behavior (unless we are demonstrating an
expected difference).

In particular, using comparisons like this are also robust against
changes in the test repository data shape, which has been necessary to
update as bugs are found.

Thanks,
-Stolee

  reply	other threads:[~2022-04-18 12:37 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-07 16:37 [PATCH 0/4] Sparse index integration with 'git show' Derrick Stolee via GitGitGadget
2022-04-07 16:37 ` [PATCH 1/4] t1092: add compatibility tests for " Derrick Stolee via GitGitGadget
2022-04-14 18:37   ` Josh Steadmon
2022-04-18 12:23     ` Derrick Stolee
2022-04-07 16:37 ` [PATCH 2/4] show: integrate with the sparse index Derrick Stolee via GitGitGadget
2022-04-14 18:50   ` Josh Steadmon
2022-04-18 12:28     ` Derrick Stolee [this message]
2022-04-07 16:37 ` [PATCH 3/4] object-name: reject trees found in the index Derrick Stolee via GitGitGadget
2022-04-14 18:57   ` Josh Steadmon
2022-04-18 12:31     ` Derrick Stolee
2022-04-07 16:37 ` [PATCH 4/4] object-name: diagnose trees in index properly Derrick Stolee via GitGitGadget
2022-04-07 20:46   ` Philip Oakley
2022-04-12  6:32   ` Junio C Hamano
2022-04-12 13:52     ` Derrick Stolee
2022-04-12 15:45       ` Junio C Hamano
2022-04-14 18:37 ` [PATCH 0/4] Sparse index integration with 'git show' Josh Steadmon
2022-04-14 21:14   ` Junio C Hamano
2022-04-18 12:42     ` Derrick Stolee
2022-04-26 20:43 ` [PATCH v2 0/5] " Derrick Stolee via GitGitGadget
2022-04-26 20:43   ` [PATCH v2 1/5] t1092: add compatibility tests for " Derrick Stolee via GitGitGadget
2022-04-26 20:43   ` [PATCH v2 2/5] show: integrate with the sparse index Derrick Stolee via GitGitGadget
2022-04-26 20:43   ` [PATCH v2 3/5] object-name: reject trees found in the index Derrick Stolee via GitGitGadget
2022-04-26 20:43   ` [PATCH v2 4/5] object-name: diagnose trees in index properly Derrick Stolee via GitGitGadget
2022-04-26 20:43   ` [PATCH v2 5/5] rev-parse: integrate with sparse index Derrick Stolee via GitGitGadget
2022-04-26 20:55   ` [PATCH v2 0/5] Sparse index integration with 'git show' Junio C Hamano
2022-04-27 13:47     ` Derrick Stolee

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=4c999e1f-e6bb-72e0-a27d-e072ce9487f8@github.com \
    --to=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=shaoxuan.yuan02@gmail.com \
    --cc=steadmon@google.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).