git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, stolee@gmail.com, newren@gmail.com,
	Taylor Blau <me@ttaylorr.com>,
	Lessley Dennington <lessleydennington@gmail.com>
Subject: Re: [PATCH v3 1/2] diff: enable and test the sparse index
Date: Wed, 03 Nov 2021 10:05:12 -0700	[thread overview]
Message-ID: <xmqqcznh8913.fsf@gitster.g> (raw)
In-Reply-To: <991aaad37b41f71faa19fdef4373ccc115edcc40.1635802069.git.gitgitgadget@gmail.com> (Lessley Dennington via GitGitGadget's message of "Mon, 01 Nov 2021 21:27:48 +0000")

"Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 2000.34: git diff --staged (full-v3)      0.08    0.08 +0.0%
> 2000.35: git diff --staged (full-v4)      0.08    0.08 +0.0%
> 2000.36: git diff --staged (sparse-v3)    0.17    0.04 -76.5%
> 2000.37: git diff --staged (sparse-v4)    0.16    0.04 -75.0%

Please do not add more use of the synonym to the test suite, other
than the one that makes sure the synonym works the same way as the
real option, which is "--cached".

> diff --git a/builtin/diff.c b/builtin/diff.c
> index dd8ce688ba7..cbf7b51c7c0 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -437,6 +437,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  
>  	prefix = setup_git_directory_gently(&nongit);
>  
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;
> +

Doesn't the code need to be protected with

	if (!nongit) {
		prepare_repo_settings(the_repository);
		the_repository->settings.command_requires_full_index = 0;
	}

at the very least?  It may be that the code is getting lucky because
the_repository may be initialized with a random value (after all,
when we are not in a repository, there is nowhere to read the
on-disk settings from) and we may even be able to set a bit in the
settings structure without crashing, but conceptually, doing the
above when we _know_ we are not in any repository is simply wrong.

I wonder if prepare_repo_settings() needs be more strict.  For
example, shouldn't it check if we have a repository to begin with
and BUG() if it was called when there is not a repository?  After
all, it tries to read from the repository configuration file, so any
necessary set-up to discover where the gitdir is must have been done
already before it can be called.

With such a safety feature to catch a programmer errors, perhaps the
above could have been caught before the patch hit the list.

Thoughts?  Am I missing some chicken-and-egg situation where
prepare_repo_settings() must be callable before we know where the
repository is, or something, which justifies why the function is so
loose in its sanity checks in the current form?



  reply	other threads:[~2021-11-03 17:05 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-14 17:25 [PATCH 0/2] Sparse Index: diff and blame builtins Lessley Dennington via GitGitGadget
2021-10-14 17:25 ` [PATCH 1/2] diff: enable and test the sparse index Lessley Dennington via GitGitGadget
2021-10-15 16:46   ` Derrick Stolee
2021-10-14 17:25 ` [PATCH 2/2] blame: " Lessley Dennington via GitGitGadget
2021-11-23  7:57   ` Elijah Newren
2021-11-23 14:57     ` Lessley Dennington
2021-10-15 21:20 ` [PATCH v2 0/2] Sparse Index: diff and blame builtins Lessley Dennington via GitGitGadget
2021-10-15 21:20   ` [PATCH v2 1/2] diff: enable and test the sparse index Lessley Dennington via GitGitGadget
2021-10-25 20:47     ` Taylor Blau
2021-10-26 16:10       ` Lessley Dennington
2021-10-26 16:15         ` Taylor Blau
2021-10-15 21:20   ` [PATCH v2 2/2] blame: " Lessley Dennington via GitGitGadget
2021-10-25 20:53     ` Taylor Blau
2021-10-26 16:17       ` Lessley Dennington
2021-11-21  1:32         ` Elijah Newren
2021-11-01 21:27   ` [PATCH v3 0/2] Sparse Index: diff and blame builtins Lessley Dennington via GitGitGadget
2021-11-01 21:27     ` [PATCH v3 1/2] diff: enable and test the sparse index Lessley Dennington via GitGitGadget
2021-11-03 17:05       ` Junio C Hamano [this message]
2021-11-04 23:55         ` Lessley Dennington
2021-11-01 21:27     ` [PATCH v3 2/2] blame: " Lessley Dennington via GitGitGadget
2021-11-03 16:47       ` Junio C Hamano
2021-11-05  0:04         ` Lessley Dennington
2021-11-21  1:46         ` Elijah Newren
2021-11-22 22:42     ` [PATCH v4 0/4] Sparse Index: diff and blame builtins Lessley Dennington via GitGitGadget
2021-11-22 22:42       ` [PATCH v4 1/4] sparse index: enable only for git repos Lessley Dennington via GitGitGadget
2021-11-23  7:41         ` Elijah Newren
2021-11-23 14:52           ` Lessley Dennington
2021-11-23 23:39         ` Junio C Hamano
2021-11-24 14:41           ` Lessley Dennington
2021-11-24 18:23             ` Junio C Hamano
2021-11-29 23:38               ` Lessley Dennington
2021-11-30  6:32                 ` Junio C Hamano
2021-11-30 23:25                   ` Lessley Dennington
2021-11-22 22:42       ` [PATCH v4 2/4] test-read-cache: set up repo after git directory Lessley Dennington via GitGitGadget
2021-11-23 23:42         ` Junio C Hamano
2021-11-24 15:10           ` Lessley Dennington
2021-11-24 18:36             ` Junio C Hamano
2021-11-29 23:01               ` Lessley Dennington
2021-11-22 22:42       ` [PATCH v4 3/4] diff: enable and test the sparse index Lessley Dennington via GitGitGadget
2021-11-23  7:47         ` Elijah Newren
2021-11-23 14:53           ` Lessley Dennington
2021-11-23 23:48         ` Junio C Hamano
2021-11-22 22:42       ` [PATCH v4 4/4] blame: " Lessley Dennington via GitGitGadget
2021-11-23 23:53         ` Junio C Hamano
2021-11-24 14:52           ` Lessley Dennington
2021-12-03 21:15       ` [PATCH v5 0/7] Sparse Index: diff and blame builtins Lessley Dennington via GitGitGadget
2021-12-03 21:15         ` [PATCH v5 1/7] git: esnure correct git directory setup with -h Lessley Dennington via GitGitGadget
2021-12-04 18:41           ` Elijah Newren
2021-12-04 19:58           ` Junio C Hamano
2021-12-03 21:16         ` [PATCH v5 2/7] commit-graph: return if there is no git directory Lessley Dennington via GitGitGadget
2021-12-03 21:16         ` [PATCH v5 3/7] test-read-cache: set up repo after " Lessley Dennington via GitGitGadget
2021-12-03 21:16         ` [PATCH v5 4/7] repo-settings: prepare_repo_settings only in git repos Lessley Dennington via GitGitGadget
2021-12-07  4:43           ` Ævar Arnfjörð Bjarmason
2021-12-08 15:46             ` Lessley Dennington
2021-12-03 21:16         ` [PATCH v5 5/7] diff: replace --staged with --cached in t1092 tests Lessley Dennington via GitGitGadget
2021-12-03 21:16         ` [PATCH v5 6/7] diff: enable and test the sparse index Lessley Dennington via GitGitGadget
2021-12-03 21:16         ` [PATCH v5 7/7] blame: " Lessley Dennington via GitGitGadget
2021-12-04 19:43         ` [PATCH v5 0/7] Sparse Index: diff and blame builtins Elijah Newren
2021-12-06 15:55         ` [PATCH v6 " Lessley Dennington via GitGitGadget
2021-12-06 15:55           ` [PATCH v6 1/7] git: ensure correct git directory setup with -h Lessley Dennington via GitGitGadget
2021-12-06 15:55           ` [PATCH v6 2/7] commit-graph: return if there is no git directory Lessley Dennington via GitGitGadget
2021-12-06 15:55           ` [PATCH v6 3/7] test-read-cache: set up repo after " Lessley Dennington via GitGitGadget
2021-12-06 15:55           ` [PATCH v6 4/7] repo-settings: prepare_repo_settings only in git repos Lessley Dennington via GitGitGadget
2021-12-06 15:55           ` [PATCH v6 5/7] diff: replace --staged with --cached in t1092 tests Lessley Dennington via GitGitGadget
2021-12-06 15:56           ` [PATCH v6 6/7] diff: enable and test the sparse index Lessley Dennington via GitGitGadget
2021-12-06 15:56           ` [PATCH v6 7/7] blame: " Lessley Dennington via GitGitGadget

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=xmqqcznh8913.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=lessleydennington@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=newren@gmail.com \
    --cc=stolee@gmail.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).