From efdd55c126d9704e750f00d9fc3f8e99a7c7343a Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Fri, 3 Dec 2021 11:29:00 -0600 Subject: [PATCH 1/7] 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 --- git.c | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/git.c b/git.c index 60c2784be45e35..eb6890087c3472 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()) { From f676f03ccb024b9011eff999b342f96cfb76b892 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Fri, 3 Dec 2021 12:00:09 -0600 Subject: [PATCH 2/7] 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 --- commit-graph.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/commit-graph.c b/commit-graph.c index 2706683acfe1c1..265c010122e8ed 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) From 7b1fab86a4a15f33b37ac3364a599c3f5237bf21 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Fri, 3 Dec 2021 12:00:57 -0600 Subject: [PATCH 3/7] 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 --- t/helper/test-read-cache.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c index b52c174acc7a1f..0d9f08931a15f1 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); From fd28be71ca47a71b224a5b08cb81f903ada6a2bd Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Fri, 3 Dec 2021 11:56:44 -0600 Subject: [PATCH 4/7] 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 --- repo-settings.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/repo-settings.c b/repo-settings.c index b93e91a212eb98..00ca5571a1ab77 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; From 2a1524a7e9ac267a550d5b88c02f853fe36fe08c Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Tue, 23 Nov 2021 13:31:58 -0800 Subject: [PATCH 5/7] 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 --- t/t1092-sparse-checkout-compatibility.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index 77e302a0ef38bf..203a594fa4529c 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -371,7 +371,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 && @@ -380,10 +380,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 @@ -400,8 +400,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 ' @@ -420,8 +420,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 ' From 897611682af64ba6bd0d2dfcfeae56cfe953c45e Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Tue, 9 Nov 2021 15:39:46 -0800 Subject: [PATCH 6/7] 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 --- builtin/diff.c | 5 +++ t/perf/p2000-sparse-operations.sh | 2 ++ t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/builtin/diff.c b/builtin/diff.c index dd8ce688ba7054..fa4683377ebbe5 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 bfd332120c8df8..5cf94627383e17 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 203a594fa4529c..abfb4994bb90c5 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -846,6 +846,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' ' From 85bcbaa1771c97a0f6ca3cc03c80f02ee2f84061 Mon Sep 17 00:00:00 2001 From: Lessley Dennington Date: Fri, 1 Oct 2021 12:58:33 -0700 Subject: [PATCH 7/7] 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 --- builtin/blame.c | 3 ++ t/perf/p2000-sparse-operations.sh | 2 + t/t1092-sparse-checkout-compatibility.sh | 49 ++++++++++++++++++------ 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index 641523ff9af693..33e411d2203c18 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -940,6 +940,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix) 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 5cf94627383e17..cb777c74a24f55 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 abfb4994bb90c5..6187a997b7d63f 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -442,21 +442,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)' ' @@ -892,6 +907,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' '