On 11/3/21 10:05 AM, Junio C Hamano wrote:
"Lessley Dennington via GitGitGadget" <gitgitgadget@xxxxxxxxx>
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".
Thank you, changed for v4.
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?
This seems like a good idea. I've added both the nongit check and the
prepare_repo_settings() updates you've suggested for v4, pending review
by my team.
Best,
Lessley
