From 475b362c2a326ddc9987ee0ff6448e795f009d5d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:27 -0700 Subject: diff: skip implicit no-index check when given --no-index We can invoke no-index mode in two ways: by an explicit request from the user, or implicitly by noticing that we have two paths, and at least one is outside the repository. If the user already told us --no-index, there is no need for us to do the implicit test at all. However, we currently do, and downgrade our "explicit" to DIFF_NO_INDEX_IMPLICIT. This doesn't have any user-visible behavior, though it's not immediately obvious why. We only trigger the implicit check when we have exactly two non-option arguments. And the only code that cares about implicit versus explicit is an error message that we show when we _don't_ have two non-option arguments. However, it's worth fixing anyway. Besides being slightly more efficient, it makes the code easier to follow, which will help when we modify it in future patches. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/diff.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) (limited to 'builtin/diff.c') diff --git a/builtin/diff.c b/builtin/diff.c index d6b8f9834d..7b2a7448ed 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -301,20 +301,21 @@ int cmd_diff(int argc, const char **argv, const char *prefix) break; } - if (!no_index) + if (!no_index) { prefix = setup_git_directory_gently(&nongit); - /* - * Treat git diff with at least one path outside of the - * repo the same as if the command would have been executed - * outside of a git repository. In this case it behaves - * the same way as "git diff --no-index ", which acts - * as a colourful "diff" replacement. - */ - if (nongit || ((argc == i + 2) && - (!path_inside_repo(prefix, argv[i]) || - !path_inside_repo(prefix, argv[i + 1])))) - no_index = DIFF_NO_INDEX_IMPLICIT; + /* + * Treat git diff with at least one path outside of the + * repo the same as if the command would have been executed + * outside of a git repository. In this case it behaves + * the same way as "git diff --no-index ", which acts + * as a colourful "diff" replacement. + */ + if (nongit || ((argc == i + 2) && + (!path_inside_repo(prefix, argv[i]) || + !path_inside_repo(prefix, argv[i + 1])))) + no_index = DIFF_NO_INDEX_IMPLICIT; + } if (!no_index) gitmodules_config(); -- cgit v1.2.1 From 28a4e580218b79dd5a7d55c2a30431469129321d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:36 -0700 Subject: diff: always try to set up the repository If we see an explicit "--no-index", we do not bother calling setup_git_directory_gently() at all. This means that we may miss out on reading repo-specific config. It's arguable whether this is correct or not. If we were designing from scratch, making "git diff --no-index" completely ignore the repository makes some sense. But we are nowhere near scratch, so let's look at the existing behavior: 1. If you're in the top-level of a repository and run an explicit "diff --no-index", the config subsystem falls back to reading ".git/config", and we will respect repo config. 2. If you're in a subdirectory of a repository, then we still try to read ".git/config", but it generally doesn't exist. So "diff --no-index" there does not respect repo config. 3. If you have $GIT_DIR set in the environment, we read and respect $GIT_DIR/config, 4. If you run "git diff /tmp/foo /tmp/bar" to get an implicit no-index, we _do_ run the repository setup, and set $GIT_DIR (or respect an existing $GIT_DIR variable). We find the repo config no matter where we started, and respect it. So we already respect the repository config in a number of common cases, and case (2) is the only one that does not. And at least one of our tests, t4034, depends on case (1) behaving as it does now (though it is just incidental, not an explicit test for this behavior). So let's bring case (2) in line with the others by always running the repository setup, even with an explicit "--no-index". We shouldn't need to change anything else, as the implicit case already handles the prefix. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/diff.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'builtin/diff.c') diff --git a/builtin/diff.c b/builtin/diff.c index 7b2a7448ed..7be7806146 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -301,9 +301,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix) break; } - if (!no_index) { - prefix = setup_git_directory_gently(&nongit); + prefix = setup_git_directory_gently(&nongit); + if (!no_index) { /* * Treat git diff with at least one path outside of the * repo the same as if the command would have been executed -- cgit v1.2.1