git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Josh Steadmon <steadmon@google.com>,
	git@vger.kernel.org, vdye@github.com, shaoxuan.yuan02@gmail.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 0/4] Sparse index integration with 'git show'
Date: Thu, 14 Apr 2022 14:14:24 -0700	[thread overview]
Message-ID: <xmqqk0brz7tb.fsf@gitster.g> (raw)
In-Reply-To: <Ylhp+q96KOt2+OGZ@google.com> (Josh Steadmon's message of "Thu, 14 Apr 2022 11:37:46 -0700")

Josh Steadmon <steadmon@google.com> writes:

>> 'git show' is relatively simple to get working in a way that doesn't fail
>> when it would previously succeed, but there are some sutbleties when the
>> user passes a directory path. If that path happens to be a sparse directory
>> entry, we suddenly start succeeding and printing the tree information!
>>
>> Since this behavior can change depending on the sparse checkout definition
>> and the state of index entries within that directory, this new behavior
>> would be more likely to confuse users than help them.
>
> The two paragraphs above did not really convey to me on first
> read-through what the problem was. IIUC the main points are:
>
> * git-show does not currently work with the sparse index.
> * A simple change fixes the above, but causes us to sometimes print
>   unexpected information about trees.
> * We can use the simple change in our implementation, but must do
>   additional work to make sure we only print the expected information.
>
> I think this could be clarified by:
> * Including examples of the unhelpful output from the simple
>   implementation.
> * Describing in more detail the situations that trigger this.
> * Describing why those situations don't currently happen without support
>   for sparse indexes.

I think the issues patches 2-4 addresses are not limited to any
specific command, but how ":<path-in-the-index>" syntax is resolved
to an object name (including the way how error messages are issued
when the given path is not found in the index).

But the title of the series and presentation place undue stress on
"git show", which I suspect may be confusing to some readers.

For example, with these patches, wouldn't "git rev-parse" start
working better, even though the proposed log message for no commit
in the series talks about "rev-parse" and no tests are done with the
"rev-parse" command?

I am not suggesting that commands other than "show" should be also
discussed in detail or tested.  It would help readers if we said
that we are using 'show' merely as an example to demonstrate how
":<path-in-the-index>" syntax interacts with the sparse index
entries (1) before the series, and (2) how well the improved
object-name parsing logic works after the series.



  reply	other threads:[~2022-04-14 21:14 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
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 [this message]
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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqqk0brz7tb.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=derrickstolee@github.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).