From e5b17bda8b6c830aa65f8c52e08755cae5b815a1 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Mon, 6 Dec 2021 15:55:55 +0000 Subject: git: ensure correct git directory setup with -h Ensure correct git directory setup when -h is passed with commands. This specifically applies to repos with special help text configuration variables and to commands run with -h outside a repository. This will also protect against test failures in the upcoming change to BUG in prepare_repo_settings if no git directory exists. Note: this diff is better seen when ignoring whitespace changes. Co-authored-by: Junio C Hamano Signed-off-by: Lessley Dennington Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/git.c b/git.c index 60c2784..eb68900 100644 --- a/git.c +++ b/git.c @@ -421,27 +421,30 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) int status, help; struct stat st; const char *prefix; + int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY)); - prefix = NULL; help = argc == 2 && !strcmp(argv[1], "-h"); - if (!help) { - if (p->option & RUN_SETUP) - prefix = setup_git_directory(); - else if (p->option & RUN_SETUP_GENTLY) { - int nongit_ok; - prefix = setup_git_directory_gently(&nongit_ok); - } - precompose_argv_prefix(argc, argv, NULL); - if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) && - !(p->option & DELAY_PAGER_CONFIG)) - use_pager = check_pager_config(p->cmd); - if (use_pager == -1 && p->option & USE_PAGER) - use_pager = 1; - - if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) && - startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */ - trace_repo_setup(prefix); + if (help && (run_setup & RUN_SETUP)) + /* demote to GENTLY to allow 'git cmd -h' outside repo */ + run_setup = RUN_SETUP_GENTLY; + + if (run_setup & RUN_SETUP) { + prefix = setup_git_directory(); + } else if (run_setup & RUN_SETUP_GENTLY) { + int nongit_ok; + prefix = setup_git_directory_gently(&nongit_ok); + } else { + prefix = NULL; } + precompose_argv_prefix(argc, argv, NULL); + if (use_pager == -1 && run_setup && + !(p->option & DELAY_PAGER_CONFIG)) + use_pager = check_pager_config(p->cmd); + if (use_pager == -1 && p->option & USE_PAGER) + use_pager = 1; + if (run_setup && startup_info->have_repository) + /* get_git_dir() may set up repo, avoid that */ + trace_repo_setup(prefix); commit_pager_choice(); if (!help && get_super_prefix()) { -- cgit v0.10.2-6-g49f6 From 0803f9c7cdff63d0c6cb992ba90f53fdcae50869 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Mon, 6 Dec 2021 15:55:56 +0000 Subject: commit-graph: return if there is no git directory Return early if git directory does not exist. This will protect against test failures in the upcoming change to BUG in prepare_repo_settings if no git directory exists. Signed-off-by: Lessley Dennington Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/commit-graph.c b/commit-graph.c index 2706683..265c010 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -632,10 +632,13 @@ static int prepare_commit_graph(struct repository *r) struct object_directory *odb; /* + * Early return if there is no git dir or if the commit graph is + * disabled. + * * This must come before the "already attempted?" check below, because * we want to disable even an already-loaded graph file. */ - if (r->commit_graph_disabled) + if (!r->gitdir || r->commit_graph_disabled) return 0; if (r->objects->commit_graph_attempted) -- cgit v0.10.2-6-g49f6 From 27a443b8208b802a9856819d79175976f9a9b993 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Mon, 6 Dec 2021 15:55:57 +0000 Subject: test-read-cache: set up repo after git directory Move repo setup to occur after git directory is set up. This will protect against test failures in the upcoming change to BUG in prepare_repo_settings if no git directory exists. Signed-off-by: Lessley Dennington Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c index b52c174..0d9f089 100644 --- a/t/helper/test-read-cache.c +++ b/t/helper/test-read-cache.c @@ -39,8 +39,6 @@ int cmd__read_cache(int argc, const char **argv) int table = 0, expand = 0; initialize_the_repository(); - prepare_repo_settings(r); - r->settings.command_requires_full_index = 0; for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) { if (skip_prefix(*argv, "--print-and-refresh=", &name)) @@ -56,6 +54,9 @@ int cmd__read_cache(int argc, const char **argv) setup_git_directory(); git_config(git_default_config, NULL); + prepare_repo_settings(r); + r->settings.command_requires_full_index = 0; + for (i = 0; i < cnt; i++) { repo_read_index(r); -- cgit v0.10.2-6-g49f6 From 44c7e62e51e504c060844360fc56b613cfc1beea Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Mon, 6 Dec 2021 15:55:58 +0000 Subject: repo-settings: prepare_repo_settings only in git repos Check whether git directory exists before adding any repo settings. If it does not exist, BUG with the message that one cannot add settings for an uninitialized repository. If it does exist, proceed with adding repo settings. Signed-off-by: Lessley Dennington Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/repo-settings.c b/repo-settings.c index b93e91a..00ca557 100644 --- a/repo-settings.c +++ b/repo-settings.c @@ -17,6 +17,9 @@ void prepare_repo_settings(struct repository *r) char *strval; int manyfiles; + if (!r->gitdir) + BUG("Cannot add settings for uninitialized repository"); + if (r->settings.initialized++) return; -- cgit v0.10.2-6-g49f6 From 338e2a9acc801ed4c5153b926579e7bea55a0be9 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Mon, 6 Dec 2021 15:55:59 +0000 Subject: diff: replace --staged with --cached in t1092 tests Replace uses of the synonym --staged in t1092 tests with --cached (which is the real and preferred option). This will allow consistency in the new tests to be added with the upcoming change to enable the sparse index for diff. Signed-off-by: Lessley Dennington Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 569d63c..fea9942 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -401,7 +401,7 @@ test_expect_success 'checkout and reset --hard' ' test_all_match git reset --hard update-folder2 ' -test_expect_success 'diff --staged' ' +test_expect_success 'diff --cached' ' init_repos && write_script edit-contents <<-\EOF && @@ -410,10 +410,10 @@ test_expect_success 'diff --staged' ' run_on_all ../edit-contents && test_all_match git diff && - test_all_match git diff --staged && + test_all_match git diff --cached && test_all_match git add README.md && test_all_match git diff && - test_all_match git diff --staged + test_all_match git diff --cached ' # NEEDSWORK: sparse-checkout behaves differently from full-checkout when @@ -430,8 +430,8 @@ test_expect_success 'diff with renames and conflicts' ' test_all_match git checkout rename-base && test_all_match git checkout $branch -- . && test_all_match git status --porcelain=v2 && - test_all_match git diff --staged --no-renames && - test_all_match git diff --staged --find-renames || return 1 + test_all_match git diff --cached --no-renames && + test_all_match git diff --cached --find-renames || return 1 done ' @@ -450,8 +450,8 @@ test_expect_success 'diff with directory/file conflicts' ' test_all_match git checkout $branch && test_all_match git checkout rename-base -- . && test_all_match git status --porcelain=v2 && - test_all_match git diff --staged --no-renames && - test_all_match git diff --staged --find-renames || return 1 + test_all_match git diff --cached --no-renames && + test_all_match git diff --cached --find-renames || return 1 done ' -- cgit v0.10.2-6-g49f6 From 51ba65b5c355a835fb0d83ee88dc7276dccb66a4 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Mon, 6 Dec 2021 15:56:00 +0000 Subject: diff: enable and test the sparse index Enable the sparse index within the 'git diff' command. Its implementation already safely integrates with the sparse index because it shares code with the 'git status' and 'git checkout' commands that were already integrated. For more details see: d76723ee53 (status: use sparse-index throughout, 2021-07-14) 1ba5f45132 (checkout: stop expanding sparse indexes, 2021-06-29) The most interesting thing to do is to add tests that verify that 'git diff' behaves correctly when the sparse index is enabled. These cases are: 1. The index is not expanded for 'diff' and 'diff --staged' 2. 'diff' and 'diff --staged' behave the same in full checkout, sparse checkout, and sparse index repositories in the following partially-staged scenarios (i.e. the index, HEAD, and working directory differ at a given path): 1. Path is within sparse-checkout cone 2. Path is outside sparse-checkout cone 3. A merge conflict exists for paths outside sparse-checkout cone The `p2000` tests demonstrate a ~44% execution time reduction for 'git diff' and a ~86% execution time reduction for 'git diff --staged' using a sparse index: Test before after ------------------------------------------------------------- 2000.30: git diff (full-v3) 0.33 0.34 +3.0% 2000.31: git diff (full-v4) 0.33 0.35 +6.1% 2000.32: git diff (sparse-v3) 0.53 0.31 -41.5% 2000.33: git diff (sparse-v4) 0.54 0.29 -46.3% 2000.34: git diff --cached (full-v3) 0.07 0.07 +0.0% 2000.35: git diff --cached (full-v4) 0.07 0.08 +14.3% 2000.36: git diff --cached (sparse-v3) 0.28 0.04 -85.7% 2000.37: git diff --cached (sparse-v4) 0.23 0.03 -87.0% Co-authored-by: Derrick Stolee Signed-off-by: Lessley Dennington Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/builtin/diff.c b/builtin/diff.c index dd8ce68..fa46833 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -437,6 +437,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix) prefix = setup_git_directory_gently(&nongit); + if (!nongit) { + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + } + if (!no_index) { /* * Treat git diff with at least one path outside of the diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh index bfd3321..5cf9462 100755 --- a/t/perf/p2000-sparse-operations.sh +++ b/t/perf/p2000-sparse-operations.sh @@ -113,5 +113,7 @@ test_perf_on_all git checkout -f - test_perf_on_all git reset test_perf_on_all git reset --hard test_perf_on_all git reset -- does-not-exist +test_perf_on_all git diff +test_perf_on_all git diff --cached test_done diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index fea9942..e6885d2 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -891,6 +891,52 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' ' ) ' +test_expect_success 'sparse index is not expanded: diff' ' + init_repos && + + write_script edit-contents <<-\EOF && + echo text >>$1 + EOF + + # Add file within cone + test_sparse_match git sparse-checkout set deep && + run_on_all ../edit-contents deep/testfile && + test_all_match git add deep/testfile && + run_on_all ../edit-contents deep/testfile && + + test_all_match git diff && + test_all_match git diff --cached && + ensure_not_expanded diff && + ensure_not_expanded diff --cached && + + # Add file outside cone + test_all_match git reset --hard && + run_on_all mkdir newdirectory && + run_on_all ../edit-contents newdirectory/testfile && + test_sparse_match git sparse-checkout set newdirectory && + test_all_match git add newdirectory/testfile && + run_on_all ../edit-contents newdirectory/testfile && + test_sparse_match git sparse-checkout set && + + test_all_match git diff && + test_all_match git diff --cached && + ensure_not_expanded diff && + ensure_not_expanded diff --cached && + + # Merge conflict outside cone + # The sparse checkout will report a warning that is not in the + # full checkout, so we use `run_on_all` instead of + # `test_all_match` + run_on_all git reset --hard && + test_all_match git checkout merge-left && + test_all_match test_must_fail git merge merge-right && + + test_all_match git diff && + test_all_match git diff --cached && + ensure_not_expanded diff && + ensure_not_expanded diff --cached +' + # NEEDSWORK: a sparse-checkout behaves differently from a full checkout # in this scenario, but it shouldn't. test_expect_success 'reset mixed and checkout orphan' ' -- cgit v0.10.2-6-g49f6 From add4c864b60766174ad4f74ba7be17e66d61ef16 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Mon, 6 Dec 2021 15:56:01 +0000 Subject: blame: enable and test the sparse index Enable the sparse index for the 'git blame' command. The index was already not expanded with this command, so the most interesting thing to do is to add tests that verify that 'git blame' behaves correctly when the sparse index is enabled and that its performance improves. More specifically, these cases are: 1. The index is not expanded for 'blame' when given paths in the sparse checkout cone at multiple levels. 2. Performance measurably improves for 'blame' with sparse index when given paths in the sparse checkout cone at multiple levels. The `p2000` tests demonstrate a ~60% execution time reduction when running 'blame' for a file two levels deep and and a ~30% execution time reduction for a file three levels deep. Test before after ---------------------------------------------------------------- 2000.62: git blame f2/f4/a (full-v3) 0.31 0.32 +3.2% 2000.63: git blame f2/f4/a (full-v4) 0.29 0.31 +6.9% 2000.64: git blame f2/f4/a (sparse-v3) 0.55 0.23 -58.2% 2000.65: git blame f2/f4/a (sparse-v4) 0.57 0.23 -59.6% 2000.66: git blame f2/f4/f3/a (full-v3) 0.77 0.85 +10.4% 2000.67: git blame f2/f4/f3/a (full-v4) 0.78 0.81 +3.8% 2000.68: git blame f2/f4/f3/a (sparse-v3) 1.07 0.72 -32.7% 2000.99: git blame f2/f4/f3/a (sparse-v4) 1.05 0.73 -30.5% We do not include paths outside the sparse checkout cone because blame does not support blaming files that are not present in the working directory. This is true in both sparse and full checkouts. Signed-off-by: Lessley Dennington Reviewed-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/builtin/blame.c b/builtin/blame.c index 641523f..33e411d 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -940,6 +940,9 @@ parse_done: revs.diffopt.flags.follow_renames = 0; argc = parse_options_end(&ctx); + prepare_repo_settings(the_repository); + the_repository->settings.command_requires_full_index = 0; + if (incremental || (output_option & OUTPUT_PORCELAIN)) { if (show_progress > 0) die(_("--progress can't be used with --incremental or porcelain formats")); diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh index 5cf9462..cb777c7 100755 --- a/t/perf/p2000-sparse-operations.sh +++ b/t/perf/p2000-sparse-operations.sh @@ -115,5 +115,7 @@ test_perf_on_all git reset --hard test_perf_on_all git reset -- does-not-exist test_perf_on_all git diff test_perf_on_all git diff --cached +test_perf_on_all git blame $SPARSE_CONE/a +test_perf_on_all git blame $SPARSE_CONE/f3/a test_done diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index e6885d2..f65bd7a 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -472,21 +472,36 @@ test_expect_success 'log with pathspec outside sparse definition' ' test_expect_success 'blame with pathspec inside sparse definition' ' init_repos && - test_all_match git blame a && - test_all_match git blame deep/a && - test_all_match git blame deep/deeper1/a && - test_all_match git blame deep/deeper1/deepest/a + for file in a \ + deep/a \ + deep/deeper1/a \ + deep/deeper1/deepest/a + do + test_all_match git blame $file + done ' -# TODO: blame currently does not support blaming files outside of the -# sparse definition. It complains that the file doesn't exist locally. -test_expect_failure 'blame with pathspec outside sparse definition' ' +# Without a revision specified, blame will error if passed any file that +# is not present in the working directory (even if the file is tracked). +# Here we just verify that this is also true with sparse checkouts. +test_expect_success 'blame with pathspec outside sparse definition' ' init_repos && + test_sparse_match git sparse-checkout set && - test_all_match git blame folder1/a && - test_all_match git blame folder2/a && - test_all_match git blame deep/deeper2/a && - test_all_match git blame deep/deeper2/deepest/a + for file in a \ + deep/a \ + deep/deeper1/a \ + deep/deeper1/deepest/a + do + test_sparse_match test_must_fail git blame $file && + cat >expect <<-EOF && + fatal: Cannot lstat '"'"'$file'"'"': No such file or directory + EOF + # We compare sparse-checkout-err and sparse-index-err in + # `test_sparse_match`. Given we know they are the same, we + # only check the content of sparse-index-err here. + test_cmp expect sparse-index-err + done ' test_expect_success 'checkout and reset (mixed)' ' @@ -937,6 +952,18 @@ test_expect_success 'sparse index is not expanded: diff' ' ensure_not_expanded diff --cached ' +test_expect_success 'sparse index is not expanded: blame' ' + init_repos && + + for file in a \ + deep/a \ + deep/deeper1/a \ + deep/deeper1/deepest/a + do + ensure_not_expanded blame $file + done +' + # NEEDSWORK: a sparse-checkout behaves differently from a full checkout # in this scenario, but it shouldn't. test_expect_success 'reset mixed and checkout orphan' ' -- cgit v0.10.2-6-g49f6