From 0e94ee9415e6cf6952b755347b57319e93356210 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:17 -0700 Subject: hash-object: always try to set up the git repository When "hash-object" is run without "-w", we don't need to be in a git repository at all; we can just hash the object and write its sha1 to stdout. However, if we _are_ in a git repository, we would want to know that so we can follow the normal rules for respecting config, .gitattributes, etc. This happens to work at the top-level of a git repository because we blindly read ".git/config", but as the included test shows, it does not work when you are in a subdirectory. The solution is to just do a "gentle" setup in this case. We already take care to use prefix_filename() on any filename arguments we get (to handle the "-w" case), so we don't need to do anything extra to handle the side effects of repo setup. An alternative would be to specify RUN_SETUP_GENTLY for this command in git.c, and then die if "-w" is set but we are not in a repository. However, the error messages generated at the time of setup_git_directory() are more detailed, so it's better to find out which mode we are in, and then call the appropriate function. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/hash-object.c | 13 ++++++++----- t/t1007-hash-object.sh | 11 +++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/builtin/hash-object.c b/builtin/hash-object.c index f7d3567dd..9028e1fdc 100644 --- a/builtin/hash-object.c +++ b/builtin/hash-object.c @@ -87,6 +87,7 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix) int stdin_paths = 0; int no_filters = 0; int literally = 0; + int nongit = 0; unsigned flags = HASH_FORMAT_CHECK; const char *vpath = NULL; const struct option hash_object_options[] = { @@ -107,12 +108,14 @@ int cmd_hash_object(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, NULL, hash_object_options, hash_object_usage, 0); - if (flags & HASH_WRITE_OBJECT) { + if (flags & HASH_WRITE_OBJECT) prefix = setup_git_directory(); - prefix_length = prefix ? strlen(prefix) : 0; - if (vpath && prefix) - vpath = prefix_filename(prefix, prefix_length, vpath); - } + else + prefix = setup_git_directory_gently(&nongit); + + prefix_length = prefix ? strlen(prefix) : 0; + if (vpath && prefix) + vpath = prefix_filename(prefix, prefix_length, vpath); git_config(git_default_config, NULL); diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh index 7d2baa15b..285871c24 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -121,6 +121,17 @@ test_expect_success 'check that appropriate filter is invoke when --path is used git config --unset core.autocrlf ' +test_expect_success 'gitattributes also work in a subdirectory' ' + mkdir subdir && + ( + cd subdir && + subdir_sha0=$(git hash-object ../file0) && + subdir_sha1=$(git hash-object ../file1) && + test "$file0_sha" = "$subdir_sha0" && + test "$file1_sha" = "$subdir_sha1" + ) +' + test_expect_success 'check that --no-filters option works' ' echo fooQ | tr Q "\\015" >file0 && cp file0 file1 && -- cgit v1.2.1 From 4a73aaaf18099ec1897330dd6c4a09f10ea2f573 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:22 -0700 Subject: patch-id: use RUN_SETUP_GENTLY Patch-id does not require a repository because it is just processing the incoming diff on stdin, but it may look at git config for keys like patchid.stable. Even though we do not setup_git_directory(), this works from the top-level of a repository because we blindly look at ".git/config" in this case. But as the included test demonstrates, it does not work from a subdirectory. We can fix it by using RUN_SETUP_GENTLY. We do not take any filenames from the user on the command line, so there's no need to adjust them via prefix_filename(). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git.c | 2 +- t/t4204-patch-id.sh | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/git.c b/git.c index 968a8a464..03151f51e 100644 --- a/git.c +++ b/git.c @@ -444,7 +444,7 @@ static struct cmd_struct commands[] = { { "pack-objects", cmd_pack_objects, RUN_SETUP }, { "pack-redundant", cmd_pack_redundant, RUN_SETUP }, { "pack-refs", cmd_pack_refs, RUN_SETUP }, - { "patch-id", cmd_patch_id }, + { "patch-id", cmd_patch_id, RUN_SETUP_GENTLY }, { "pickaxe", cmd_blame, RUN_SETUP }, { "prune", cmd_prune, RUN_SETUP }, { "prune-packed", cmd_prune_packed, RUN_SETUP }, diff --git a/t/t4204-patch-id.sh b/t/t4204-patch-id.sh index baa9d3c82..10b6ad02e 100755 --- a/t/t4204-patch-id.sh +++ b/t/t4204-patch-id.sh @@ -143,6 +143,20 @@ test_expect_success 'patch-id supports git-format-patch MIME output' ' test_cmp patch-id_master patch-id_same ' +test_expect_success 'patch-id respects config from subdir' ' + test_config patchid.stable true && + mkdir subdir && + + # copy these because test_patch_id() looks for them in + # the current directory + cp bar-then-foo foo-then-bar subdir && + + ( + cd subdir && + test_patch_id irrelevant patchid.stable=true + ) +' + cat >nonl <<\EOF diff --git i/a w/a index e69de29..2e65efe 100644 -- cgit v1.2.1 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(-) diff --git a/builtin/diff.c b/builtin/diff.c index d6b8f9834..7b2a7448e 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 7d8930d903fd8b775a1db63bcf373df5ac5af0be Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:32 -0700 Subject: diff: handle --no-index prefixes consistently If we see an explicit "git diff --no-index ../foo ../bar", then we do not set up the git repository at all (we already know we are in --no-index mode, so do not have to check "are we in a repository?"), and hence have no "prefix" within the repository. A patch generated by this command will have the filenames "a/../foo" and "b/../bar", no matter which directory we are in with respect to any repository. However, in the implicit case, where we notice that the files are outside the repository, we will have chdir()'d to the top-level of the repository. We then feed the prefix back to the diff machinery. As a result, running the same diff from a subdirectory will result in paths that look like "a/subdir/../../foo". Besides being unnecessarily long, this may also be confusing to the user: they don't care about the subdir or the repository at all; it's just where they happened to be when running the command. We should treat this the same as the explicit --no-index case. One way to address this would be to chdir() back to the original path before running our diff. However, that's a bit hacky, as we would also need to adjust $GIT_DIR, which could be a relative path from our top-level. Instead, we can reuse the diff machinery's RELATIVE_NAME option, which automatically strips off the prefix. Note that this _also_ restricts the diff to this relative prefix, but that's OK for our purposes: we queue our own diff pairs manually, and do not rely on that part of the diff code. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff-no-index.c | 3 +++ t/t4053-diff-no-index.sh | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/diff-no-index.c b/diff-no-index.c index 03daadb25..59237a626 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -282,6 +282,9 @@ void diff_no_index(struct rev_info *revs, DIFF_OPT_SET(&revs->diffopt, NO_INDEX); + DIFF_OPT_SET(&revs->diffopt, RELATIVE_NAME); + revs->diffopt.prefix = prefix; + revs->max_count = -2; diff_setup_done(&revs->diffopt); diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh index 6eb83211b..e60c95118 100755 --- a/t/t4053-diff-no-index.sh +++ b/t/t4053-diff-no-index.sh @@ -89,4 +89,22 @@ test_expect_success 'turning a file into a directory' ' ) ' +test_expect_success 'diff from repo subdir shows real paths (explicit)' ' + echo "diff --git a/../../non/git/a b/../../non/git/b" >expect && + test_expect_code 1 \ + git -C repo/sub \ + diff --no-index ../../non/git/a ../../non/git/b >actual && + head -n 1 actual.head && + test_cmp expect actual.head +' + +test_expect_success 'diff from repo subdir shows real paths (implicit)' ' + echo "diff --git a/../../non/git/a b/../../non/git/b" >expect && + test_expect_code 1 \ + git -C repo/sub \ + diff ../../non/git/a ../../non/git/b >actual && + head -n 1 actual.head && + test_cmp expect actual.head +' + test_done -- 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 ++-- t/t4053-diff-no-index.sh | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/builtin/diff.c b/builtin/diff.c index 7b2a7448e..7be780614 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 diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh index e60c95118..453e6c35e 100755 --- a/t/t4053-diff-no-index.sh +++ b/t/t4053-diff-no-index.sh @@ -107,4 +107,24 @@ test_expect_success 'diff from repo subdir shows real paths (implicit)' ' test_cmp expect actual.head ' +test_expect_success 'diff --no-index from repo subdir respects config (explicit)' ' + echo "diff --git ../../non/git/a ../../non/git/b" >expect && + test_config -C repo diff.noprefix true && + test_expect_code 1 \ + git -C repo/sub \ + diff --no-index ../../non/git/a ../../non/git/b >actual && + head -n 1 actual.head && + test_cmp expect actual.head +' + +test_expect_success 'diff --no-index from repo subdir respects config (implicit)' ' + echo "diff --git ../../non/git/a ../../non/git/b" >expect && + test_config -C repo diff.noprefix true && + test_expect_code 1 \ + git -C repo/sub \ + diff ../../non/git/a ../../non/git/b >actual && + head -n 1 actual.head && + test_cmp expect actual.head +' + test_done -- cgit v1.2.1 From 01e7e90359aa6fa54d00201843e1bb5f02b632f5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:40 -0700 Subject: pager: remove obsolete comment The comment at the top of pager.c claims that we've split the code out so that Windows can do something different. This dates back to f67b45f (Introduce trivial new pager.c helper infrastructure, 2006-02-28), because the original implementation used fork(). Later, we ended up sticking the Windows #ifdefs into this file anyway. And then even later, in ea27a18 (spawn pager via run_command interface, 2008-07-22) we unified the implementations. So these days this comment is really saying nothing at all. Let's drop it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pager.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pager.c b/pager.c index 4bc048148..70159b885 100644 --- a/pager.c +++ b/pager.c @@ -6,11 +6,6 @@ #define DEFAULT_PAGER "less" #endif -/* - * This is split up from the rest of git so that we can do - * something different on Windows. - */ - static struct child_process pager_process = CHILD_PROCESS_INIT; static void wait_for_pager(int in_signal) -- cgit v1.2.1 From 4babb839aaa18b14a5ec9e8994678d75bc120920 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:44 -0700 Subject: pager: stop loading git_default_config() In git_pager(), we really only care about getting the value of core.pager. But to do so, we use the git_default_config() callback, which loads many other values. Ordinarily it isn't a big deal to load this config an extra time, as it simply overwrites the values from the previous run. But it's a bad idea here, for two reasons: 1. The pager setup may be called very early in the program, before we have found the git repository. As a result, we may fail to read the correct repo-level config file. This is a problem for core.pager, too, but we should at least try to minimize the pollution to other configured values. 2. Because we call setup_pager() from git.c, basically every builtin command _may_ end up reading this config and getting an implicit git_default_config() setup. Which doesn't sound like a terrible thing, except that we don't do it consistently; it triggers only when stdout is a tty. So if a command forgets to load the default config itself (but depends on it anyway), it may appear to work, and then mysteriously fail when the pager is not in use. We can improve this by loading _just_ the core.pager config from git_pager(). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- config.c | 3 --- pager.c | 9 ++++++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/config.c b/config.c index 6dbc8a409..1d2331aa0 100644 --- a/config.c +++ b/config.c @@ -836,9 +836,6 @@ static int git_default_core_config(const char *var, const char *value) return 0; } - if (!strcmp(var, "core.pager")) - return git_config_string(&pager_program, var, value); - if (!strcmp(var, "core.editor")) return git_config_string(&editor_program, var, value); diff --git a/pager.c b/pager.c index 70159b885..fc210f951 100644 --- a/pager.c +++ b/pager.c @@ -35,6 +35,13 @@ static void wait_for_pager_signal(int signo) raise(signo); } +static int core_pager_config(const char *var, const char *value, void *data) +{ + if (!strcmp(var, "core.pager")) + return git_config_string(&pager_program, var, value); + return 0; +} + const char *git_pager(int stdout_is_tty) { const char *pager; @@ -45,7 +52,7 @@ const char *git_pager(int stdout_is_tty) pager = getenv("GIT_PAGER"); if (!pager) { if (!pager_program) - git_config(git_default_config, NULL); + git_config(core_pager_config, NULL); pager = pager_program; } if (!pager) -- cgit v1.2.1 From c0c08897c47eb46179c8d2408dc1a91713307842 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:48 -0700 Subject: pager: make pager_program a file-local static This variable is only ever used by the routines in pager.c, and other parts of the code should always use those routines (like git_pager()) to make decisions about which pager to use. Let's reduce its scope to prevent accidents. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 1 - environment.c | 1 - pager.c | 1 + 3 files changed, 1 insertion(+), 2 deletions(-) diff --git a/cache.h b/cache.h index 4ff196c25..5e50887d6 100644 --- a/cache.h +++ b/cache.h @@ -1733,7 +1733,6 @@ extern int write_file_gently(const char *path, const char *fmt, ...); /* pager.c */ extern void setup_pager(void); -extern const char *pager_program; extern int pager_in_use(void); extern int pager_use_color; extern int term_columns(void); diff --git a/environment.c b/environment.c index 96160a75a..bccdee9d0 100644 --- a/environment.c +++ b/environment.c @@ -39,7 +39,6 @@ size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE; size_t packed_git_limit = DEFAULT_PACKED_GIT_LIMIT; size_t delta_base_cache_limit = 96 * 1024 * 1024; unsigned long big_file_threshold = 512 * 1024 * 1024; -const char *pager_program; int pager_use_color = 1; const char *editor_program; const char *askpass_program; diff --git a/pager.c b/pager.c index fc210f951..00c8baace 100644 --- a/pager.c +++ b/pager.c @@ -7,6 +7,7 @@ #endif static struct child_process pager_process = CHILD_PROCESS_INIT; +static const char *pager_program; static void wait_for_pager(int in_signal) { -- cgit v1.2.1 From 6a1e1bc0a155faa5a095ce00bd06678a1dfa0eea Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:52 -0700 Subject: pager: use callbacks instead of configset While the cached configset interface is more pleasant to use, it is not appropriate for "early" config like pager setup, which must sometimes do tricky things like reading from ".git/config" even when we have not set up the repository. As a preparatory step to handling these cases better, let's switch back to using the callback interface, which gives us more control. Note that this is essentially a revert of 586f414 (pager.c: replace `git_config()` with `git_config_get_value()`, 2014-08-07), but with some minor style fixups and modernizations. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pager.c | 47 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/pager.c b/pager.c index 00c8baace..4811f3f0a 100644 --- a/pager.c +++ b/pager.c @@ -159,23 +159,42 @@ int decimal_width(uintmax_t number) return width; } -/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */ -int check_pager_config(const char *cmd) +struct pager_command_config_data { + const char *cmd; + int want; + char *value; +}; + +static int pager_command_config(const char *var, const char *value, void *vdata) { - int want = -1; - struct strbuf key = STRBUF_INIT; - const char *value = NULL; - strbuf_addf(&key, "pager.%s", cmd); - if (git_config_key_is_valid(key.buf) && - !git_config_get_value(key.buf, &value)) { - int b = git_config_maybe_bool(key.buf, value); + struct pager_command_config_data *data = vdata; + const char *cmd; + + if (skip_prefix(var, "pager.", &cmd) && !strcmp(cmd, data->cmd)) { + int b = git_config_maybe_bool(var, value); if (b >= 0) - want = b; + data->want = b; else { - want = 1; - pager_program = xstrdup(value); + data->want = 1; + data->value = xstrdup(value); } } - strbuf_release(&key); - return want; + + return 0; +} + +/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */ +int check_pager_config(const char *cmd) +{ + struct pager_command_config_data data; + + data.cmd = cmd; + data.want = -1; + data.value = NULL; + + git_config(pager_command_config, &data); + + if (data.value) + pager_program = data.value; + return data.want; } -- cgit v1.2.1 From eed270720227c761213a61fb6d75a44ac5a6f290 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:56 -0700 Subject: pager: handle early config The pager code is often run early in the git.c startup, before we have actually found the repository. When we ask git_config() to look for values like core.pager, it doesn't know where to find the repo-level config, and will blindly examine ".git/config" if it exists. That's why t7006 shows that many pager-related features happen to work from the top-level of a repository, but not from a subdirectory. This patch pulls that ".git/config" hack explicitly into the pager code. There are two reasons for this: 1. We'd like to clean up the git_config() behavior, as looking at ".git/config" when we do not have a configured repository is often the wrong thing to do. But we'd prefer not to break the pager config any worse than it already is. 2. It's one very tiny step on the road to ultimately making the pager config work consistently. If we eventually get an equivalent of setup_git_directory() that _just_ finds the directory and doesn't chdir() or set up any global state, we could plug it in here (instead of blindly looking at ".git/config"). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- pager.c | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/pager.c b/pager.c index 4811f3f0a..f97a8bdc9 100644 --- a/pager.c +++ b/pager.c @@ -43,6 +43,37 @@ static int core_pager_config(const char *var, const char *value, void *data) return 0; } +static void read_early_config(config_fn_t cb, void *data) +{ + git_config_with_options(cb, data, NULL, 1); + + /* + * Note that this is a really dirty hack that does the wrong thing in + * many cases. The crux of the problem is that we cannot run + * setup_git_directory() early on in git's setup, so we have no idea if + * we are in a repository or not, and therefore are not sure whether + * and how to read repository-local config. + * + * So if we _aren't_ in a repository (or we are but we would reject its + * core.repositoryformatversion), we'll read whatever is in .git/config + * blindly. Similarly, if we _are_ in a repository, but not at the + * root, we'll fail to find .git/config (because it's really + * ../.git/config, etc). See t7006 for a complete set of failures. + * + * However, we have historically provided this hack because it does + * work some of the time (namely when you are at the top-level of a + * valid repository), and would rarely make things worse (i.e., you do + * not generally have a .git/config file sitting around). + */ + if (!startup_info->have_repository) { + struct git_config_source repo_config; + + memset(&repo_config, 0, sizeof(repo_config)); + repo_config.file = ".git/config"; + git_config_with_options(cb, data, &repo_config, 1); + } +} + const char *git_pager(int stdout_is_tty) { const char *pager; @@ -53,7 +84,7 @@ const char *git_pager(int stdout_is_tty) pager = getenv("GIT_PAGER"); if (!pager) { if (!pager_program) - git_config(core_pager_config, NULL); + read_early_config(core_pager_config, NULL); pager = pager_program; } if (!pager) @@ -192,7 +223,7 @@ int check_pager_config(const char *cmd) data.want = -1; data.value = NULL; - git_config(pager_command_config, &data); + read_early_config(pager_command_config, &data); if (data.value) pager_program = data.value; -- cgit v1.2.1 From 11ca4bec96e00f7ed18dbd2a475a8d568f4b85f3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:24:00 -0700 Subject: t1302: use "git -C" This is shorter, and saves a subshell. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1302-repo-version.sh | 30 ++++++------------------------ 1 file changed, 6 insertions(+), 24 deletions(-) diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh index 9bcd34969..f859809b3 100755 --- a/t/t1302-repo-version.sh +++ b/t/t1302-repo-version.sh @@ -25,10 +25,7 @@ test_expect_success 'setup' ' test_expect_success 'gitdir selection on normal repos' ' echo 0 >expect && git config core.repositoryformatversion >actual && - ( - cd test && - git config core.repositoryformatversion >../actual2 - ) && + git -C test config core.repositoryformatversion >actual2 && test_cmp expect actual && test_cmp expect actual2 ' @@ -36,35 +33,20 @@ test_expect_success 'gitdir selection on normal repos' ' test_expect_success 'gitdir selection on unsupported repo' ' # Make sure it would stop at test2, not trash echo 99 >expect && - ( - cd test2 && - git config core.repositoryformatversion >../actual - ) && + git -C test2 config core.repositoryformatversion >actual && test_cmp expect actual ' test_expect_success 'gitdir not required mode' ' git apply --stat test.patch && - ( - cd test && - git apply --stat ../test.patch - ) && - ( - cd test2 && - git apply --stat ../test.patch - ) + git -C test apply --stat ../test.patch && + git -C test2 apply --stat ../test.patch ' test_expect_success 'gitdir required mode' ' git apply --check --index test.patch && - ( - cd test && - git apply --check --index ../test.patch - ) && - ( - cd test2 && - test_must_fail git apply --check --index ../test.patch - ) + git -C test apply --check --index ../test.patch && + test_must_fail git -C test2 apply --check --index ../test.patch ' check_allow () { -- cgit v1.2.1 From 25a6c2804675a08dca648201e5bd590a787eab74 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:24:10 -0700 Subject: test-config: setup git directory The t1308 test script uses our test-config helper to read repository-level config, but never actually sets up the repository. This works because git_config() blindly reads ".git/config" even if we have not configured a repository. This means that test-config won't work from a subdirectory, though since it's just a helper for the test scripts, that's not a big deal. More important is that the behavior of git_config() is going to change, and we want to make sure that t1308 continues to work. We can just use setup_git_directory(), and not the gentle form; there's no point in being flexible, as it's just a helper for the tests. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- test-config.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test-config.c b/test-config.c index 6a7755221..7ee3210da 100644 --- a/test-config.c +++ b/test-config.c @@ -39,6 +39,9 @@ int main(int argc, char **argv) const char *v; const struct string_list *strptr; struct config_set cs; + + setup_git_directory(); + git_configset_init(&cs); if (argc < 2) { -- cgit v1.2.1 From b9605bc4f2e44042824571f70b9a3a74eeebabff Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:24:15 -0700 Subject: config: only read .git/config from configured repos When git_config() runs, it looks in the system, user-wide, and repo-level config files. It gets the latter by calling git_pathdup(), which in turn calls get_git_dir(). If we haven't set up the git repository yet, this may simply return ".git", and we will look at ".git/config". This seems like it would be helpful (presumably we haven't set up the repository yet, so it tries to find it), but it turns out to be a bad idea for a few reasons: - it's not sufficient, and therefore hides bugs in a confusing way. Config will be respected if commands are run from the top-level of the working tree, but not from a subdirectory. - it's not always true that we haven't set up the repository _yet_; we may not want to do it at all. For instance, if you run "git init /some/path" from inside another repository, it should not load config from the existing repository. - there might be a path ".git/config", but it is not the actual repository we would find via setup_git_directory(). This may happen, e.g., if you are storing a git repository inside another git repository, but have munged one of the files in such a way that the inner repository is not valid (e.g., by removing HEAD). We have at least two bugs of the second type in git-init, introduced by ae5f677 (lazily load core.sharedrepository, 2016-03-11). It causes init to use git_configset(), which loads all of the config, including values from the current repo (if any). This shows up in two ways: 1. If we happen to be in an existing repository directory, we'll read and respect core.sharedrepository from it, even though it should have no bearing on the new repository. A new test in t1301 covers this. 2. Similarly, if we're in an existing repo that sets core.logallrefupdates, that will cause init to fail to set it in a newly created repository (because it thinks that the user's templates already did so). A new test in t0001 covers this. We also need to adjust an existing test in t1302, which gives another example of why this patch is an improvement. That test creates an embedded repository with a bogus core.repositoryformatversion of "99". It wants to make sure that we actually stop at the bogus repo rather than continuing upward to find the outer repo. So it checks that "git config core.repositoryformatversion" returns 99. But that only works because we blindly read ".git/config", even though we _know_ we're in a repository whose vintage we do not understand. After this patch, we avoid reading config from the unknown vintage repository at all, which is a safer choice. But we need to tweak the test, since core.repositoryformatversion will not return 99; it will claim that it could not find the variable at all. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 6 ++++++ config.c | 2 +- environment.c | 7 +++++++ t/t0001-init.sh | 9 +++++++++ t/t1301-shared-repo.sh | 9 +++++++++ t/t1302-repo-version.sh | 4 +--- 6 files changed, 33 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index 5e50887d6..67ec356c7 100644 --- a/cache.h +++ b/cache.h @@ -453,6 +453,12 @@ static inline enum object_type object_type(unsigned int mode) */ extern const char * const local_repo_env[]; +/* + * Returns true iff we have a configured git repository (either via + * setup_git_directory, or in the environment via $GIT_DIR). + */ +int have_git_dir(void); + extern int is_bare_repository_cfg; extern int is_bare_repository(void); extern int is_inside_git_dir(void); diff --git a/config.c b/config.c index 1d2331aa0..5e50fb1a3 100644 --- a/config.c +++ b/config.c @@ -1194,7 +1194,7 @@ static int do_git_config_sequence(config_fn_t fn, void *data) int ret = 0, found = 0; char *xdg_config = xdg_config_home("config"); char *user_config = expand_user_path("~/.gitconfig"); - char *repo_config = git_pathdup("config"); + char *repo_config = have_git_dir() ? git_pathdup("config") : NULL; if (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) { ret += git_config_from_file(fn, git_etc_gitconfig(), diff --git a/environment.c b/environment.c index bccdee9d0..20a870c3d 100644 --- a/environment.c +++ b/environment.c @@ -194,6 +194,13 @@ int is_bare_repository(void) return is_bare_repository_cfg && !get_git_work_tree(); } +int have_git_dir(void) +{ + return startup_info->have_repository + || git_dir + || getenv(GIT_DIR_ENVIRONMENT); +} + const char *get_git_dir(void) { if (!git_dir) diff --git a/t/t0001-init.sh b/t/t0001-init.sh index a6fdd5ef3..8ffbbea4d 100755 --- a/t/t0001-init.sh +++ b/t/t0001-init.sh @@ -384,4 +384,13 @@ test_expect_success MINGW 'bare git dir not hidden' ' ! is_hidden newdir ' +test_expect_success 'remote init from does not use config from cwd' ' + rm -rf newdir && + test_config core.logallrefupdates true && + git init newdir && + echo true >expect && + git -C newdir config --bool core.logallrefupdates >actual && + test_cmp expect actual +' + test_done diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index ac1087540..7c28642f2 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -172,4 +172,13 @@ test_expect_success POSIXPERM 'forced modes' ' }" actual)" ' +test_expect_success POSIXPERM 'remote init does not use config from cwd' ' + git config core.sharedrepository 0666 && + umask 0022 && + git init --bare child.git && + echo "-rw-r--r--" >expect && + modebits child.git/config >actual && + test_cmp expect actual +' + test_done diff --git a/t/t1302-repo-version.sh b/t/t1302-repo-version.sh index f859809b3..ce4cff13b 100755 --- a/t/t1302-repo-version.sh +++ b/t/t1302-repo-version.sh @@ -32,9 +32,7 @@ test_expect_success 'gitdir selection on normal repos' ' test_expect_success 'gitdir selection on unsupported repo' ' # Make sure it would stop at test2, not trash - echo 99 >expect && - git -C test2 config core.repositoryformatversion >actual && - test_cmp expect actual + test_expect_code 1 git -C test2 config core.repositoryformatversion >actual ' test_expect_success 'gitdir not required mode' ' -- cgit v1.2.1 From 7c0a842b46391230661d17ad758efa6b4eccdb93 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:24:19 -0700 Subject: init: expand comments explaining config trickery git-init may copy "config" from the templates directory and then re-read it. There are some comments explaining what's going on here, but they are not grouped very well with the matching code. Let's rearrange and expand them. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/init-db.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index b2d8d40a6..0b0557138 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -191,16 +191,19 @@ static int create_default_files(const char *template_path) /* Just look for `init.templatedir` */ git_config(git_init_db_config, NULL); - /* First copy the templates -- we might have the default + /* + * First copy the templates -- we might have the default * config file there, in which case we would want to read * from it after installing. */ copy_templates(template_path); - git_config(git_default_config, NULL); - is_bare_repository_cfg = init_is_bare_repository; - /* reading existing config may have overwrote it */ + /* + * We must make sure command-line options continue to override any + * values we might have just re-read from the config. + */ + is_bare_repository_cfg = init_is_bare_repository; if (init_shared_repository != -1) set_shared_repository(init_shared_repository); -- cgit v1.2.1 From 4543926ba8f8ec596dd4e45f206f4fbc29350567 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:24:23 -0700 Subject: init: reset cached config when entering new repo After we copy the templates into place, we re-read the config in case we copied in a default config file. But since git_config() is backed by a cache these days, it's possible that the call will not actually touch the filesystem at all; we need to tell it that something has changed behind the scenes. Note that we also need to reset the shared_repository config. At first glance, it seems like this should probably just be folded into git_config_clear(). But unfortunately that is not quite right. The shared repository value may come from config, _or_ it may have been set manually. So only the caller who knows whether or not they set it is the one who can clear it (and indeed, if you _do_ put it into git_config_clear(), then many tests fail, as we have to clear the config cache any time we set a new config variable). There are three tests here. The first two actually pass already, though it's largely luck: they just don't happen to actually read any config before we enter the new repo. But the third one does fail without this patch; we look at core.sharedrepository while creating the directory, but need to make sure the value from the template config overrides it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/init-db.c | 6 ++++++ cache.h | 7 +++++++ environment.c | 5 +++++ t/t1301-shared-repo.sh | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 50 insertions(+) diff --git a/builtin/init-db.c b/builtin/init-db.c index 0b0557138..cc09fca81 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -195,8 +195,14 @@ static int create_default_files(const char *template_path) * First copy the templates -- we might have the default * config file there, in which case we would want to read * from it after installing. + * + * Before reading that config, we also need to clear out any cached + * values (since we've just potentially changed what's available on + * disk). */ copy_templates(template_path); + git_config_clear(); + reset_shared_repository(); git_config(git_default_config, NULL); /* diff --git a/cache.h b/cache.h index 67ec356c7..b2d77f3eb 100644 --- a/cache.h +++ b/cache.h @@ -669,8 +669,15 @@ extern size_t delta_base_cache_limit; extern unsigned long big_file_threshold; extern unsigned long pack_size_limit_cfg; +/* + * Accessors for the core.sharedrepository config which lazy-load the value + * from the config (if not already set). The "reset" function can be + * used to unset "set" or cached value, meaning that the value will be loaded + * fresh from the config file on the next call to get_shared_repository(). + */ void set_shared_repository(int value); int get_shared_repository(void); +void reset_shared_repository(void); /* * Do replace refs need to be checked this run? This variable is diff --git a/environment.c b/environment.c index 20a870c3d..abf3cb66e 100644 --- a/environment.c +++ b/environment.c @@ -350,3 +350,8 @@ int get_shared_repository(void) } return the_shared_repository; } + +void reset_shared_repository(void) +{ + need_shared_repository_from_config = 1; +} diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh index 7c28642f2..1312004f8 100755 --- a/t/t1301-shared-repo.sh +++ b/t/t1301-shared-repo.sh @@ -181,4 +181,36 @@ test_expect_success POSIXPERM 'remote init does not use config from cwd' ' test_cmp expect actual ' +test_expect_success POSIXPERM 're-init respects core.sharedrepository (local)' ' + git config core.sharedrepository 0666 && + umask 0022 && + echo whatever >templates/foo && + git init --template=templates && + echo "-rw-rw-rw-" >expect && + modebits .git/foo >actual && + test_cmp expect actual +' + +test_expect_success POSIXPERM 're-init respects core.sharedrepository (remote)' ' + rm -rf child.git && + umask 0022 && + git init --bare --shared=0666 child.git && + test_path_is_missing child.git/foo && + git init --bare --template=../templates child.git && + echo "-rw-rw-rw-" >expect && + modebits child.git/foo >actual && + test_cmp expect actual +' + +test_expect_success POSIXPERM 'template can set core.sharedrepository' ' + rm -rf child.git && + umask 0022 && + git config core.sharedrepository 0666 && + cp .git/config templates/config && + git init --bare --template=../templates child.git && + echo "-rw-rw-rw-" >expect && + modebits child.git/HEAD >actual && + test_cmp expect actual +' + test_done -- cgit v1.2.1 From 4d0efa101bbbc63d8bd1ec0477f027f23b9f573b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 12 Sep 2016 20:23:12 -0700 Subject: t1007: factor out repeated setup We have a series of 3 CRLF tests that do exactly the same (long) setup sequence. Let's pull it out into a common setup test, which is shorter, more efficient, and will make it easier to add new tests. Note that we don't have to worry about cleaning up any of the setup which was previously per-test; we call pop_repo after the CRLF tests, which cleans up everything. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1007-hash-object.sh | 32 ++++++++------------------------ 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh index 285871c24..acca9ac56 100755 --- a/t/t1007-hash-object.sh +++ b/t/t1007-hash-object.sh @@ -101,7 +101,7 @@ test_expect_success 'git hash-object --stdin file1 file0 && cp file0 file1 && echo "file0 -crlf" >.gitattributes && @@ -109,7 +109,10 @@ test_expect_success 'check that appropriate filter is invoke when --path is used git config core.autocrlf true && file0_sha=$(git hash-object file0) && file1_sha=$(git hash-object file1) && - test "$file0_sha" != "$file1_sha" && + test "$file0_sha" != "$file1_sha" +' + +test_expect_success 'check that appropriate filter is invoke when --path is used' ' path1_sha=$(git hash-object --path=file1 file0) && path0_sha=$(git hash-object --path=file0 file1) && test "$file0_sha" = "$path0_sha" && @@ -117,8 +120,7 @@ test_expect_success 'check that appropriate filter is invoke when --path is used path1_sha=$(cat file0 | git hash-object --path=file1 --stdin) && path0_sha=$(cat file1 | git hash-object --path=file0 --stdin) && test "$file0_sha" = "$path0_sha" && - test "$file1_sha" = "$path1_sha" && - git config --unset core.autocrlf + test "$file1_sha" = "$path1_sha" ' test_expect_success 'gitattributes also work in a subdirectory' ' @@ -133,33 +135,15 @@ test_expect_success 'gitattributes also work in a subdirectory' ' ' test_expect_success 'check that --no-filters option works' ' - echo fooQ | tr Q "\\015" >file0 && - cp file0 file1 && - echo "file0 -crlf" >.gitattributes && - echo "file1 crlf" >>.gitattributes && - git config core.autocrlf true && - file0_sha=$(git hash-object file0) && - file1_sha=$(git hash-object file1) && - test "$file0_sha" != "$file1_sha" && nofilters_file1=$(git hash-object --no-filters file1) && test "$file0_sha" = "$nofilters_file1" && nofilters_file1=$(cat file1 | git hash-object --stdin) && - test "$file0_sha" = "$nofilters_file1" && - git config --unset core.autocrlf + test "$file0_sha" = "$nofilters_file1" ' test_expect_success 'check that --no-filters option works with --stdin-paths' ' - echo fooQ | tr Q "\\015" >file0 && - cp file0 file1 && - echo "file0 -crlf" >.gitattributes && - echo "file1 crlf" >>.gitattributes && - git config core.autocrlf true && - file0_sha=$(git hash-object file0) && - file1_sha=$(git hash-object file1) && - test "$file0_sha" != "$file1_sha" && nofilters_file1=$(echo "file1" | git hash-object --stdin-paths --no-filters) && - test "$file0_sha" = "$nofilters_file1" && - git config --unset core.autocrlf + test "$file0_sha" = "$nofilters_file1" ' pop_repo -- cgit v1.2.1