From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-10-14 17:26:01
From: Lessley Dennington <redacted>
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.
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 ~30% execution time reduction for 'git
diff' and a ~75% execution time reduction for 'git diff --staged' using a
sparse index:
Test before after
-------------------------------------------------------------
2000.30: git diff (full-v3) 0.37 0.36 -2.7%
2000.31: git diff (full-v4) 0.36 0.35 -2.8%
2000.32: git diff (sparse-v3) 0.46 0.30 -34.8%
2000.33: git diff (sparse-v4) 0.43 0.31 -27.9%
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%
Co-authored-by: Derrick Stolee [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Lessley Dennington <redacted>
---
builtin/diff.c | 3 ++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 42 ++++++++++++++++++++++++
3 files changed, 47 insertions(+)
@@ -386,6 +386,43 @@ test_expect_success 'diff --staged' 'test_all_matchgitdiff--staged'+test_expect_success'diff partially-staged''+init_repos&&++write_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_all_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_all_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_all_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&++# Merge conflict outside cone+test_all_matchgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged+'+# NEEDSWORK: sparse-checkout behaves differently from full-checkout when# running this test with 'df-conflict-2' after 'df-conflict-1'. test_expect_success'diff with renames and conflicts''
@@ -800,6 +837,11 @@ test_expect_success 'sparse-index is not expanded' '# Wildcard identifies only full sparse directories, no index expansionensure_not_expandedresetdeepest--folder\*&&+echoatestchange>>sparse-index/README.md&&+ensure_not_expandeddiff&&+git-Csparse-indexaddREADME.md&&+ensure_not_expandeddiff--staged&&+ensure_not_expandedcheckout-fupdate-deep&&test_config-Csparse-indexpull.twoheadort&&(
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-10-14 17:26:03
From: Lessley Dennington <redacted>
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
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
fatal: no such path '<path outside sparse definition>' in HEAD
Signed-off-by: Lessley Dennington <redacted>
---
builtin/blame.c | 3 +++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 24 +++++++++++++++++-------
3 files changed, 22 insertions(+), 7 deletions(-)
@@ -485,15 +485,16 @@ test_expect_success 'blame with pathspec inside sparse definition' 'test_all_matchgitblamedeep/deeper1/deepest/a'-# 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''+# Blame does not support blaming files outside of the sparse+# definition, so we verify this scenario.+test_expect_success'blame with pathspec outside sparse definition''init_repos&&-test_all_matchgitblamefolder1/a&&-test_all_matchgitblamefolder2/a&&-test_all_matchgitblamedeep/deeper2/a&&-test_all_matchgitblamedeep/deeper2/deepest/a+test_sparse_matchgitsparse-checkoutset&&+test_sparse_matchtest_must_failgitblamefolder1/a&&+test_sparse_matchtest_must_failgitblamefolder2/a&&+test_sparse_matchtest_must_failgitblamedeep/deeper2/a&&+test_sparse_matchtest_must_failgitblamedeep/deeper2/deepest/a' test_expect_success'checkout and reset (mixed)''
@@ -871,6 +872,15 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' ')'+test_expect_success'sparse index is not expanded: blame''+init_repos&&++ensure_not_expandedblamea&&+ensure_not_expandedblamedeep/a&&+ensure_not_expandedblamedeep/deeper1/a&&+ensure_not_expandedblamedeep/deeper1/deepest/a+'+# 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''
On 10/14/2021 1:25 PM, Lessley Dennington via GitGitGadget wrote:
There is a failure in 'seen', and it's due to a subtle reason
that we didn't catch in gitgitgadget PR builds. It's because
ds/add-rm-with-sparse-index wasn't in your history until it was
merged into 'seen'.
+test_expect_success 'diff partially-staged' '
+ init_repos &&
+
+ write_script edit-contents <<-\EOF &&
+ echo text >>$1
+ EOF
+
+ # Add file within cone
+ test_all_match git sparse-checkout set deep &&
The root cause is that you should use "test_sparse_match" when
adjusting the sparse-checkout definition. The full-checkout repo
is getting the sparse-checkout set to a single pattern "deep",
but without cone mode.
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-10-15 21:20:42
From: Lessley Dennington <redacted>
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.
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 ~30% execution time reduction for 'git
diff' and a ~75% execution time reduction for 'git diff --staged' using a
sparse index:
Test before after
-------------------------------------------------------------
2000.30: git diff (full-v3) 0.37 0.36 -2.7%
2000.31: git diff (full-v4) 0.36 0.35 -2.8%
2000.32: git diff (sparse-v3) 0.46 0.30 -34.8%
2000.33: git diff (sparse-v4) 0.43 0.31 -27.9%
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%
Co-authored-by: Derrick Stolee [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Lessley Dennington <redacted>
---
builtin/diff.c | 3 ++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 45 ++++++++++++++++++++++++
3 files changed, 50 insertions(+)
@@ -386,6 +386,46 @@ test_expect_success 'diff --staged' 'test_all_matchgitdiff--staged'+test_expect_success'diff partially-staged''+init_repos&&++write_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&++# 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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged+'+# NEEDSWORK: sparse-checkout behaves differently from full-checkout when# running this test with 'df-conflict-2' after 'df-conflict-1'. test_expect_success'diff with renames and conflicts''
@@ -800,6 +840,11 @@ test_expect_success 'sparse-index is not expanded' '# Wildcard identifies only full sparse directories, no index expansionensure_not_expandedresetdeepest--folder\*&&+echoatestchange>>sparse-index/README.md&&+ensure_not_expandeddiff&&+git-Csparse-indexaddREADME.md&&+ensure_not_expandeddiff--staged&&+ensure_not_expandedcheckout-fupdate-deep&&test_config-Csparse-indexpull.twoheadort&&(
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-10-15 21:20:44
From: Lessley Dennington <redacted>
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
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
fatal: no such path '<path outside sparse definition>' in HEAD
Signed-off-by: Lessley Dennington <redacted>
---
builtin/blame.c | 3 +++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 24 +++++++++++++++++-------
3 files changed, 22 insertions(+), 7 deletions(-)
@@ -488,15 +488,16 @@ test_expect_success 'blame with pathspec inside sparse definition' 'test_all_matchgitblamedeep/deeper1/deepest/a'-# 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''+# Blame does not support blaming files outside of the sparse+# definition, so we verify this scenario.+test_expect_success'blame with pathspec outside sparse definition''init_repos&&-test_all_matchgitblamefolder1/a&&-test_all_matchgitblamefolder2/a&&-test_all_matchgitblamedeep/deeper2/a&&-test_all_matchgitblamedeep/deeper2/deepest/a+test_sparse_matchgitsparse-checkoutset&&+test_sparse_matchtest_must_failgitblamefolder1/a&&+test_sparse_matchtest_must_failgitblamefolder2/a&&+test_sparse_matchtest_must_failgitblamedeep/deeper2/a&&+test_sparse_matchtest_must_failgitblamedeep/deeper2/deepest/a' test_expect_success'checkout and reset (mixed)''
@@ -874,6 +875,15 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' ')'+test_expect_success'sparse index is not expanded: blame''+init_repos&&++ensure_not_expandedblamea&&+ensure_not_expandedblamedeep/a&&+ensure_not_expandedblamedeep/deeper1/a&&+ensure_not_expandedblamedeep/deeper1/deepest/a+'+# 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: Taylor Blau <hidden> Date: 2021-10-25 20:47:42
On Fri, Oct 15, 2021 at 09:20:34PM +0000, Lessley Dennington via GitGitGadget wrote:
From: Lessley Dennington <redacted>
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.
Good, it looks like most of the heavy-lifting to make `git diff` work
with the sparse index was already done elsewhere.
It may be helpful here to include either one of two things to help
readers and reviewers understand what's going on:
- A summary of what `git status` and/or `git checkout` does to work
with the sparse index.
- Or the patches which make those commands work with the sparse index
so that readers can refer back to them.
Having either of those would help readers who are unfamiliar with
builtin/diff.c convince themselves more easily that setting
'command_requires_full_index = 0' is all that's needed here.
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
Nice, these are all of the test cases that I would expect to demonstrate
interesting behavior.
@@ -386,6 +386,46 @@ test_expect_success 'diff --staged' 'test_all_matchgitdiff--staged'+test_expect_success'diff partially-staged''+init_repos&&++write_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&++# 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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged+'+# NEEDSWORK: sparse-checkout behaves differently from full-checkout when# running this test with 'df-conflict-2' after 'df-conflict-1'. test_expect_success'diff with renames and conflicts''
@@ -800,6 +840,11 @@ test_expect_success 'sparse-index is not expanded' '# Wildcard identifies only full sparse directories, no index expansionensure_not_expandedresetdeepest--folder\*&&+echoatestchange>>sparse-index/README.md&&+ensure_not_expandeddiff&&
Thinking aloud here as somebody who is unfamiliar with the sparse-index
tests. ensure_not_expanded relies on the existence of the "sparse-index"
repository, and its top-level README.md is outside of the
sparse-checkout cone.
That makes sense, and when I create a repository with a file outside of
the sparse-checkout cone and then run `git diff`, I see no changes as
expected.
But isn't the top-level directory always part of the cone? If so, I
think that what this (and the below test) is demonstrating is that we
can show changes inside of the cone without expanding the sparse-index.
Having that test makes absolute sense to me. But I think it might also
make sense to have a test that creates some directory structure outside
of the cone, modifies it, and then ensures that both (a) those changes
aren't visible to `git diff` when the sparse-checkout is active and (b)
that running `git diff` doesn't cause the sparse-index to be expanded.
Thanks,
Taylor
From: Taylor Blau <hidden> Date: 2021-10-25 20:53:23
On Fri, Oct 15, 2021 at 09:20:35PM +0000, Lessley Dennington via GitGitGadget wrote:
From: Lessley Dennington <redacted>
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.
Eek. What's eating up the other 30% when we have to open up another
layer of trees?
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
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
fatal: no such path '<path outside sparse definition>' in HEAD.
Small nit; this error message should be indented with a couple of space
characters to indicate that it's the output of running Git instead of
part of your patch message. Not worth a reroll on its own, but something
to keep in mind for your many future patches :).
@@ -488,15 +488,16 @@ test_expect_success 'blame with pathspec inside sparse definition' 'test_all_matchgitblamedeep/deeper1/deepest/a'-# 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''+# Blame does not support blaming files outside of the sparse+# definition, so we verify this scenario.+test_expect_success'blame with pathspec outside sparse definition''init_repos&&-test_all_matchgitblamefolder1/a&&-test_all_matchgitblamefolder2/a&&-test_all_matchgitblamedeep/deeper2/a&&-test_all_matchgitblamedeep/deeper2/deepest/a+test_sparse_matchgitsparse-checkoutset&&+test_sparse_matchtest_must_failgitblamefolder1/a&&+test_sparse_matchtest_must_failgitblamefolder2/a&&+test_sparse_matchtest_must_failgitblamedeep/deeper2/a&&+test_sparse_matchtest_must_failgitblamedeep/deeper2/deepest/a'
test_must_fail used to allow for segfaults, but doesn't these days. So
this is a good test of "it should fail in sparse checkouts but not
crash", although I think it would be good to ensure that it's failing in
the way you expect (i.e., by checking that stderr contains "no such path
<xyz> in HEAD").
quoted hunk
test_expect_success 'checkout and reset (mixed)' '
@@ -874,6 +875,15 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' ' ) '+test_expect_success 'sparse index is not expanded: blame' '+ init_repos &&++ ensure_not_expanded blame a &&+ ensure_not_expanded blame deep/a &&+ ensure_not_expanded blame deep/deeper1/a &&+ ensure_not_expanded blame deep/deeper1/deepest/a+'
Makes sense. Probably just one of these is necessary, but I haven't
looked into init_repos (or the "setup" test) enough to know for sure.
Either way, not worth changing.
Thanks,
Taylor
On Fri, Oct 15, 2021 at 09:20:34PM +0000, Lessley Dennington via GitGitGadget wrote:
quoted
From: Lessley Dennington <redacted>
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.
Good, it looks like most of the heavy-lifting to make `git diff` work
with the sparse index was already done elsewhere.
It may be helpful here to include either one of two things to help
readers and reviewers understand what's going on:
- A summary of what `git status` and/or `git checkout` does to work
with the sparse index.
- Or the patches which make those commands work with the sparse index
so that readers can refer back to them.
Having either of those would help readers who are unfamiliar with
builtin/diff.c convince themselves more easily that setting
'command_requires_full_index = 0' is all that's needed here.
Great suggestion, thank you!
quoted
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
Nice, these are all of the test cases that I would expect to demonstrate
interesting behavior.
@@ -386,6 +386,46 @@ test_expect_success 'diff --staged' 'test_all_matchgitdiff--staged'+test_expect_success'diff partially-staged''+init_repos&&++write_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&++# 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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged+'+# NEEDSWORK: sparse-checkout behaves differently from full-checkout when# running this test with 'df-conflict-2' after 'df-conflict-1'.test_expect_success'diff with renames and conflicts''
@@ -800,6 +840,11 @@ test_expect_success 'sparse-index is not expanded' '# Wildcard identifies only full sparse directories, no index expansionensure_not_expandedresetdeepest--folder\*&&+echoatestchange>>sparse-index/README.md&&+ensure_not_expandeddiff&&
Thinking aloud here as somebody who is unfamiliar with the sparse-index
tests. ensure_not_expanded relies on the existence of the "sparse-index"
repository, and its top-level README.md is outside of the
sparse-checkout cone.
That makes sense, and when I create a repository with a file outside of
the sparse-checkout cone and then run `git diff`, I see no changes as
expected.
But isn't the top-level directory always part of the cone? If so, I
think that what this (and the below test) is demonstrating is that we
can show changes inside of the cone without expanding the sparse-index.
Having that test makes absolute sense to me. But I think it might also
make sense to have a test that creates some directory structure outside
of the cone, modifies it, and then ensures that both (a) those changes
aren't visible to `git diff` when the sparse-checkout is active and (b)
that running `git diff` doesn't cause the sparse-index to be expanded.
README.md is actually within the sparse checkout cone - all files at
root are included by default. So your understanding is correct - we are
ensuring that making a change to a file in the cone and running both
diff and diff --staged once the file is in the index doesn't expand the
sparse index.
I like your idea of verifying that running diff against files outside
the sparse checkout cone won't expand the index. I've updated the diff
tests in v3 (which I will send out shortly) to do so.
From: Taylor Blau <hidden> Date: 2021-10-26 16:16:06
On Tue, Oct 26, 2021 at 09:10:20AM -0700, Lessley Dennington wrote:
quoted
But isn't the top-level directory always part of the cone? If so, I
think that what this (and the below test) is demonstrating is that we
can show changes inside of the cone without expanding the sparse-index.
Having that test makes absolute sense to me. But I think it might also
make sense to have a test that creates some directory structure outside
of the cone, modifies it, and then ensures that both (a) those changes
aren't visible to `git diff` when the sparse-checkout is active and (b)
that running `git diff` doesn't cause the sparse-index to be expanded.
README.md is actually within the sparse checkout cone - all files at root
are included by default. So your understanding is correct - we are ensuring
that making a change to a file in the cone and running both diff and diff
--staged once the file is in the index doesn't expand the sparse index.
I like your idea of verifying that running diff against files outside the
sparse checkout cone won't expand the index. I've updated the diff tests in
v3 (which I will send out shortly) to do so.
Great, thank you! There is no hurry to send out an updated revision from
me, either. It may be good to wait a day or two and see if any other
review trickles in before sending another revision to the list that way
you can batch together updates from multiple reviewers.
Thanks,
Taylor
On Fri, Oct 15, 2021 at 09:20:35PM +0000, Lessley Dennington via GitGitGadget wrote:
quoted
From: Lessley Dennington <redacted>
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.
Eek. What's eating up the other 30% when we have to open up another
layer of trees?
I'm not sure to be totally honest. However, given these are both pretty
good time reductions I don't think we should be terribly concerned.
quoted
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
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
fatal: no such path '<path outside sparse definition>' in HEAD.
Small nit; this error message should be indented with a couple of space
characters to indicate that it's the output of running Git instead of
part of your patch message. Not worth a reroll on its own, but something
to keep in mind for your many future patches :).
Eh, I'm making some changes based on your suggestions anyway, so I'm
including this in v3. Thanks for letting me know!
@@ -488,15 +488,16 @@ test_expect_success 'blame with pathspec inside sparse definition' 'test_all_matchgitblamedeep/deeper1/deepest/a'-# 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''+# Blame does not support blaming files outside of the sparse+# definition, so we verify this scenario.+test_expect_success'blame with pathspec outside sparse definition''init_repos&&-test_all_matchgitblamefolder1/a&&-test_all_matchgitblamefolder2/a&&-test_all_matchgitblamedeep/deeper2/a&&-test_all_matchgitblamedeep/deeper2/deepest/a+test_sparse_matchgitsparse-checkoutset&&+test_sparse_matchtest_must_failgitblamefolder1/a&&+test_sparse_matchtest_must_failgitblamefolder2/a&&+test_sparse_matchtest_must_failgitblamedeep/deeper2/a&&+test_sparse_matchtest_must_failgitblamedeep/deeper2/deepest/a'
test_must_fail used to allow for segfaults, but doesn't these days. So
this is a good test of "it should fail in sparse checkouts but not
crash", although I think it would be good to ensure that it's failing in
the way you expect (i.e., by checking that stderr contains "no such path
<xyz> in HEAD").
Good suggestion, coming in v3!
quoted
test_expect_success 'checkout and reset (mixed)' '
@@ -874,6 +875,15 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' ' ) '+test_expect_success 'sparse index is not expanded: blame' '+ init_repos &&++ ensure_not_expanded blame a &&+ ensure_not_expanded blame deep/a &&+ ensure_not_expanded blame deep/deeper1/a &&+ ensure_not_expanded blame deep/deeper1/deepest/a+'
Makes sense. Probably just one of these is necessary, but I haven't
looked into init_repos (or the "setup" test) enough to know for sure.
Either way, not worth changing.
Thanks,
Taylor
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-11-01 21:27:55
This series is based on vd/sparse-reset. It integrates the sparse index with
git diff and git blame and includes:
1. tests added to t1092 and p2000 to establish the baseline functionality
of the commands
2. repository settings to enable the sparse index
The p2000 tests demonstrate a ~30% execution time reduction for 'git diff'
and a ~75% execution time reduction for 'git diff --staged' using a sparse
index. For 'git blame', the reduction time was ~60% for a file two levels
deep and ~30% for a file three levels deep.
Test before after
----------------------------------------------------------------
2000.30: git diff (full-v3) 0.37 0.36 -2.7%
2000.31: git diff (full-v4) 0.36 0.35 -2.8%
2000.32: git diff (sparse-v3) 0.46 0.30 -34.8%
2000.33: git diff (sparse-v4) 0.43 0.31 -27.9%
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%
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%
Changes since V1
================
* Fix failing diff partially-staged test in
t1092-sparse-checkout-compatibility.sh, which was breaking in seen.
Changes since V2
================
* Update diff commit description to include patches that make the checkout
and status commands work with the sparse index for readers to reference.
* Add new test case to verify diff behaves as expected when run against
files outside the sparse checkout cone.
* Indent error message in blame commit
* Check error message in blame with pathspec outside sparse definition test
matches expectations.
* Loop blame tests (instead of running the same command multiple time
against different files).
Thanks, Lessley
Lessley Dennington (2):
diff: enable and test the sparse index
blame: enable and test the sparse index
builtin/blame.c | 3 +
builtin/diff.c | 3 +
t/perf/p2000-sparse-operations.sh | 4 +
t/t1092-sparse-checkout-compatibility.sh | 94 +++++++++++++++++++++---
4 files changed, 93 insertions(+), 11 deletions(-)
base-commit: 7159bf518eed5c997cf4ff0f17d9cb69192a091c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1050%2Fldennington%2Fdiff-blame-sparse-index-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1050/ldennington/diff-blame-sparse-index-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1050
Range-diff vs v2:
1: ac33159d020 ! 1: 991aaad37b4 diff: enable and test the sparse index
@@ Commit message
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:
@@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout -f -
test_done
## t/t1092-sparse-checkout-compatibility.sh ##
-@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --staged' '
- test_all_match git diff --staged
+@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded: merge conflict in cone' '
+ )
'
-+test_expect_success 'diff partially-staged' '
++test_expect_success 'sparse index is not expanded: diff' '
+ init_repos &&
+
+ write_script edit-contents <<-\EOF &&
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --staged' '
+
+ test_all_match git diff &&
+ test_all_match git diff --staged &&
++ ensure_not_expanded diff &&
++ ensure_not_expanded diff --staged &&
+
+ # Add file outside cone
+ test_all_match git reset --hard &&
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --staged' '
+
+ test_all_match git diff &&
+ test_all_match git diff --staged &&
++ ensure_not_expanded diff &&
++ ensure_not_expanded diff --staged &&
+
+ # Merge conflict outside cone
+ # The sparse checkout will report a warning that is not in the
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff --staged' '
+ test_all_match test_must_fail git merge merge-right &&
+
+ test_all_match git diff &&
-+ test_all_match git diff --staged
-+'
-+
- # NEEDSWORK: sparse-checkout behaves differently from full-checkout when
- # running this test with 'df-conflict-2' after 'df-conflict-1'.
- test_expect_success 'diff with renames and conflicts' '
-@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded' '
- # Wildcard identifies only full sparse directories, no index expansion
- ensure_not_expanded reset deepest -- folder\* &&
-
-+ echo a test change >>sparse-index/README.md &&
++ test_all_match git diff --staged &&
+ ensure_not_expanded diff &&
-+ git -C sparse-index add README.md &&
-+ ensure_not_expanded diff --staged &&
++ ensure_not_expanded diff --staged
++'
+
- ensure_not_expanded checkout -f update-deep &&
- test_config -C sparse-index pull.twohead ort &&
- (
+ # 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' '
2: a0b6a152c75 ! 2: cfdd33129ec blame: enable and test the sparse index
@@ Commit message
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
- fatal: no such path '<path outside sparse definition>' in HEAD
+ fatal: no such path '<path outside sparse definition>' in HEAD
Signed-off-by: Lessley Dennington [off-list ref]
@@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git reset --hard
test_done
## t/t1092-sparse-checkout-compatibility.sh ##
-@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'blame with pathspec inside sparse definition' '
- test_all_match git blame deep/deeper1/deepest/a
+@@ t/t1092-sparse-checkout-compatibility.sh: 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
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'blame with pathsp
+# definition, so we verify this scenario.
+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
-+ test_sparse_match git sparse-checkout set &&
-+ test_sparse_match test_must_fail git blame folder1/a &&
-+ test_sparse_match test_must_fail git blame folder2/a &&
-+ test_sparse_match test_must_fail git blame deep/deeper2/a &&
-+ test_sparse_match test_must_fail 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)' '
-@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded: merge conflict in cone' '
- )
+@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is not expanded: diff' '
+ ensure_not_expanded diff --staged
'
+test_expect_success 'sparse index is not expanded: blame' '
+ init_repos &&
+
-+ ensure_not_expanded blame a &&
-+ ensure_not_expanded blame deep/a &&
-+ ensure_not_expanded blame deep/deeper1/a &&
-+ ensure_not_expanded blame deep/deeper1/deepest/a
++ 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
--
gitgitgadget
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-11-01 21:27:56
From: Lessley Dennington <redacted>
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 ~30% execution time reduction for 'git
diff' and a ~75% execution time reduction for 'git diff --staged' using a
sparse index:
Test before after
-------------------------------------------------------------
2000.30: git diff (full-v3) 0.37 0.36 -2.7%
2000.31: git diff (full-v4) 0.36 0.35 -2.8%
2000.32: git diff (sparse-v3) 0.46 0.30 -34.8%
2000.33: git diff (sparse-v4) 0.43 0.31 -27.9%
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%
Co-authored-by: Derrick Stolee [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Lessley Dennington <redacted>
---
builtin/diff.c | 3 ++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
3 files changed, 51 insertions(+)
@@ -832,6 +832,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_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged&&++# 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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged+'+# 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: Lessley Dennington via GitGitGadget <hidden> Date: 2021-11-01 21:28:02
From: Lessley Dennington <redacted>
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
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
fatal: no such path '<path outside sparse definition>' in HEAD
Signed-off-by: Lessley Dennington <redacted>
---
builtin/blame.c | 3 ++
t/perf/p2000-sparse-operations.sh | 2 +
t/t1092-sparse-checkout-compatibility.sh | 48 ++++++++++++++++++------
3 files changed, 42 insertions(+), 11 deletions(-)
@@ -442,21 +442,35 @@ test_expect_success 'log with pathspec outside sparse definition' ' test_expect_success'blame with pathspec inside sparse definition''init_repos&&-test_all_matchgitblamea&&-test_all_matchgitblamedeep/a&&-test_all_matchgitblamedeep/deeper1/a&&-test_all_matchgitblamedeep/deeper1/deepest/a+forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+test_all_matchgitblame$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''+# Blame does not support blaming files outside of the sparse+# definition, so we verify this scenario.+test_expect_success'blame with pathspec outside sparse definition''init_repos&&+test_sparse_matchgitsparse-checkoutset&&-test_all_matchgitblamefolder1/a&&-test_all_matchgitblamefolder2/a&&-test_all_matchgitblamedeep/deeper2/a&&-test_all_matchgitblamedeep/deeper2/deepest/a+forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+test_sparse_matchtest_must_failgitblame$file&&+cat>expect<<-EOF&&+fatal:Cannotlstat'"'"'$file'"'"':Nosuchfileordirectory+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_cmpexpectsparse-index-err+done' test_expect_success'checkout and reset (mixed)''
@@ -878,6 +892,18 @@ test_expect_success 'sparse index is not expanded: diff' 'ensure_not_expandeddiff--staged'+test_expect_success'sparse index is not expanded: blame''+init_repos&&++forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+ensure_not_expandedblame$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''
On Tue, Oct 26, 2021 at 9:17 AM Lessley Dennington
[off-list ref] wrote:
On 10/25/21 1:53 PM, Taylor Blau wrote:
quoted
On Fri, Oct 15, 2021 at 09:20:35PM +0000, Lessley Dennington via GitGitGadget wrote:
quoted
From: Lessley Dennington <redacted>
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.
Eek. What's eating up the other 30% when we have to open up another
layer of trees?
I'm not sure to be totally honest. However, given these are both pretty
good time reductions I don't think we should be terribly concerned.
It's not something eating up more time in the sparse-index code; let's
look a bit closer...
Time was ~0.55s for the full at two levels deep, and dropped by just
over 0.3s in sparse-index.
Time was ~1.05s for the full at three levels deep, and dropped by just
over 0.3s in sparse-index.
So, the sparse-index enabling saves us the same amount of time, it's
just that the overall execution time for the non-sparse-index
comparison point goes up. Saving the same amount of time for the two
cases seems intuitive to me; both cases get to avoid looking at the
same number of index entries outside the sparsity paths.
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-11-22 22:42:44
This series is based on vd/sparse-reset. It integrates the sparse index with
git diff and git blame and includes:
1. tests added to t1092 and p2000 to establish the baseline functionality
of the commands
2. repository settings to enable the sparse index
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. For 'git blame', the reduction time was ~60% for a file two levels
deep and ~30% for a file three levels deep.
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%
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%
Changes since V1
================
* Fix failing diff partially-staged test in
t1092-sparse-checkout-compatibility.sh, which was breaking in seen.
Changes since V2
================
* Update diff commit description to include patches that make the checkout
and status commands work with the sparse index for readers to reference.
* Add new test case to verify diff behaves as expected when run against
files outside the sparse checkout cone.
* Indent error message in blame commit
* Check error message in blame with pathspec outside sparse definition test
matches expectations.
* Loop blame tests (instead of running the same command multiple time
against different files).
Changes since V3
================
* Update diff p2000 tests to use --cached instead of --staged. Execute new
run and update results in commit description and cover letter.
* Update comment on blame with pathspec outside sparse definition test in
t1092-sparse-checkout-compatibility.sh to clarify that it tests the
current state and could be improved in the future.
* Ensure sparse index is only activated when diff is running against files
in a Git repo.
* BUG if prepare_repo_settings() is called outside a repository.
* Ensure sparse index is not activated for calls to blame, checkout, or
pack-object with -h.
* Ensure commit-graph is only loaded if a git directory exists.
Thanks, Lessley
Lessley Dennington (4):
sparse index: enable only for git repos
test-read-cache: set up repo after git directory
diff: enable and test the sparse index
blame: enable and test the sparse index
builtin/blame.c | 5 ++
builtin/checkout.c | 6 +-
builtin/diff.c | 5 ++
builtin/pack-objects.c | 9 ++-
commit-graph.c | 5 +-
repo-settings.c | 3 +
t/helper/test-read-cache.c | 5 +-
t/perf/p2000-sparse-operations.sh | 4 +
t/t1092-sparse-checkout-compatibility.sh | 95 +++++++++++++++++++++---
9 files changed, 118 insertions(+), 19 deletions(-)
base-commit: 7159bf518eed5c997cf4ff0f17d9cb69192a091c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1050%2Fldennington%2Fdiff-blame-sparse-index-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1050/ldennington/diff-blame-sparse-index-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1050
Range-diff vs v3:
-: ----------- > 1: 81e208cf454 sparse index: enable only for git repos
-: ----------- > 2: 5bc5e8465ab test-read-cache: set up repo after git directory
1: 991aaad37b4 ! 3: 273ee16b74e diff: enable and test the sparse index
@@ Commit message
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:
+ 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)
+ d76723e (status: use sparse-index throughout, 2021-07-14)
+ 1ba5f45 (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:
+ 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
@@ Commit message
2. Path is outside sparse-checkout cone
3. A merge conflict exists for paths outside sparse-checkout cone
- The `p2000` tests demonstrate a ~30% execution time reduction for 'git
- diff' and a ~75% execution time reduction for 'git diff --staged' using a
+ 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.37 0.36 -2.7%
- 2000.31: git diff (full-v4) 0.36 0.35 -2.8%
- 2000.32: git diff (sparse-v3) 0.46 0.30 -34.8%
- 2000.33: git diff (sparse-v4) 0.43 0.31 -27.9%
- 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%
+ 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 [off-list ref]
Signed-off-by: Derrick Stolee [off-list ref]
@@ builtin/diff.c: 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;
++ if (!nongit) {
++ prepare_repo_settings(the_repository);
++ the_repository->settings.command_requires_full_index = 0;
++ }
+
if (!no_index) {
/*
@@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout -f -
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 --staged
++test_perf_on_all git diff --cached
test_done
2: cfdd33129ec ! 4: 7acf5118bf5 blame: enable and test the sparse index
@@ builtin/blame.c: int cmd_blame(int argc, const char **argv, const char *prefix)
long anchor;
const int hexsz = the_hash_algo->hexsz;
-+ prepare_repo_settings(the_repository);
-+ the_repository->settings.command_requires_full_index = 0;
++ if (startup_info->have_repository) {
++ prepare_repo_settings(the_repository);
++ the_repository->settings.command_requires_full_index = 0;
++ }
+
setup_default_color_by_age();
git_config(git_blame_config, &output_option);
repo_init_revisions(the_repository, &revs, NULL);
## t/perf/p2000-sparse-operations.sh ##
-@@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git reset --hard
+@@ t/perf/p2000-sparse-operations.sh: 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 --staged
+-test_perf_on_all git diff --cached
++test_perf_on_all git diff --staged
+test_perf_on_all git blame $SPARSE_CONE/a
+test_perf_on_all git blame $SPARSE_CONE/f3/a
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'log with pathspec
-# 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' '
-+# Blame does not support blaming files outside of the sparse
-+# definition, so we verify this scenario.
++# NEEDSWORK: This test documents the current behavior, but this could
++# change in the future if we decide to support blaming files outside
++# the sparse definition.
+test_expect_success 'blame with pathspec outside sparse definition' '
init_repos &&
+ test_sparse_match git sparse-checkout set &&
--
gitgitgadget
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-11-22 22:42:48
From: Lessley Dennington <redacted>
Check whether git dir 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.
Additionally, ensure the above BUG is not triggered when users pass the -h
flag by adding a check for the repository to the checkout and pack-objects
builtins.
Finally, ensure the above BUG is not triggered for commit-graph by
returning early if the git directory does not exist.
Signed-off-by: Lessley Dennington <redacted>
---
builtin/checkout.c | 6 ++++--
builtin/pack-objects.c | 9 ++++++---
commit-graph.c | 5 ++++-
repo-settings.c | 3 +++
4 files changed, 17 insertions(+), 6 deletions(-)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-11-22 22:42:50
From: Lessley Dennington <redacted>
Move repo setup to occur after git directory is set up. This will ensure
enabling the sparse index for `diff` (and guarding against the nongit
scenario) will not cause tests to start failing, since that change will include
adding a check to prepare_repo_settings() with the new BUG.
Signed-off-by: Lessley Dennington <redacted>
---
t/helper/test-read-cache.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-11-22 22:42:51
From: Lessley Dennington <redacted>
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:
d76723e (status: use sparse-index throughout, 2021-07-14)
1ba5f45 (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 [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Lessley Dennington <redacted>
---
builtin/diff.c | 5 +++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
3 files changed, 53 insertions(+)
@@ -832,6 +832,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_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged&&++# 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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged+'+# 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: Lessley Dennington via GitGitGadget <hidden> Date: 2021-11-22 22:42:53
From: Lessley Dennington <redacted>
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
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
fatal: no such path '<path outside sparse definition>' in HEAD
Signed-off-by: Lessley Dennington <redacted>
---
builtin/blame.c | 5 +++
t/perf/p2000-sparse-operations.sh | 4 +-
t/t1092-sparse-checkout-compatibility.sh | 49 ++++++++++++++++++------
3 files changed, 46 insertions(+), 12 deletions(-)
@@ -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_matchgitblamea&&-test_all_matchgitblamedeep/a&&-test_all_matchgitblamedeep/deeper1/a&&-test_all_matchgitblamedeep/deeper1/deepest/a+forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+test_all_matchgitblame$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''+# NEEDSWORK: This test documents the current behavior, but this could+# change in the future if we decide to support blaming files outside+# the sparse definition.+test_expect_success'blame with pathspec outside sparse definition''init_repos&&+test_sparse_matchgitsparse-checkoutset&&-test_all_matchgitblamefolder1/a&&-test_all_matchgitblamefolder2/a&&-test_all_matchgitblamedeep/deeper2/a&&-test_all_matchgitblamedeep/deeper2/deepest/a+forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+test_sparse_matchtest_must_failgitblame$file&&+cat>expect<<-EOF&&+fatal:Cannotlstat'"'"'$file'"'"':Nosuchfileordirectory+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_cmpexpectsparse-index-err+done' test_expect_success'checkout and reset (mixed)''
@@ -878,6 +893,18 @@ test_expect_success 'sparse index is not expanded: diff' 'ensure_not_expandeddiff--staged'+test_expect_success'sparse index is not expanded: blame''+init_repos&&++forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+ensure_not_expandedblame$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''
On Mon, Nov 22, 2021 at 2:42 PM Lessley Dennington via GitGitGadget
[off-list ref] wrote:
From: Lessley Dennington <redacted>
Check whether git dir 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.
Additionally, ensure the above BUG is not triggered when users pass the -h
flag by adding a check for the repository to the checkout and pack-objects
builtins.
Why only checkout and pack-objects? Why don't the -h flags to all of
the following need it as well?:
$ git grep -l prepare_repo_settings | grep builtin/
builtin/add.c
builtin/blame.c
builtin/checkout.c
builtin/commit.c
builtin/diff.c
builtin/fetch.c
builtin/gc.c
builtin/merge.c
builtin/pack-objects.c
builtin/rebase.c
builtin/reset.c
builtin/revert.c
builtin/sparse-checkout.c
builtin/update-index.c
If none of these need it, was it because they put
prepare_repo_settings() calls after some other basic checks had been
done so more do not have to be added? If so, is there a similar place
in checkout and pack-objects where their calls to
prepare_repo_settings() can be moved? (Looking ahead, it appears you
moved some code in patch 2 to do something like this. Are the similar
moves that could be done here?)
Finally, ensure the above BUG is not triggered for commit-graph by
returning early if the git directory does not exist.
If commit-graph needs a special case to avoid triggering the BUG,
wouldn't several of these need it too?:
$ git grep -l prepare_repo_settings | grep -v builtin/
commit-graph.c
fetch-negotiator.c
merge-recursive.c
midx.c
read-cache.c
repo-settings.c
repository.c
repository.h
sparse-index.c
t/helper/test-read-cache.c
t/helper/test-read-graph.c
unpack-trees.c
or are their calls to prepare_repo_settings() only done after gitdir
setup? If the latter, perhaps the commit-graph function calls could
be moved after gitdir setup too to avoid the need to do extra checks
in it?
I'm not what the BUG() is trying to help us catch, but I'm worried
that there are many additional places that now need workarounds to
avoid triggering bugs.
On Mon, Nov 22, 2021 at 2:42 PM Lessley Dennington via GitGitGadget
[off-list ref] wrote:
From: Lessley Dennington <redacted>
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:
d76723e (status: use sparse-index throughout, 2021-07-14)
1ba5f45 (checkout: stop expanding sparse indexes, 2021-06-29)
I preferred the references in your v3:
d76723ee53 (status: use sparse-index throughout, 2021-07-14)
1ba5f45132 (checkout: stop expanding sparse indexes, 2021-06-29)
because 7-character abbreviations aren't very future proof;
10-character seems better to me.
(Very micro nit.)
quoted hunk
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 [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Lessley Dennington <redacted>
---
builtin/diff.c | 5 +++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
3 files changed, 53 insertions(+)
@@ -832,6 +832,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_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged&&++# 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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged
You've changed some of the --staged to --cached, but based on Junio's
comments on the previous round, you probably want to convert the
others too.
+'
+
# 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' '
--
gitgitgadget
On Thu, Oct 14, 2021 at 10:25 AM Lessley Dennington via GitGitGadget
[off-list ref] wrote:
From: Lessley Dennington <redacted>
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%
Looks good.
We do not include paths outside the sparse checkout cone because blame
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
fatal: no such path '<path outside sparse definition>' in HEAD
While technically accurate, this wording is misleading; it implies
that there is something unique to sparse checkouts, and perhaps even
to cone mode, affecting how blame handles files not in the working
directory. That's not true, though; git blame without a revision has
always reported an error when given a file that does not exist in the
working tree. Try this in git.git:
$ rm t/README
$ git blame t/README
fatal: Cannot lstat 't/README': No such file or directory
The reason is that with no revisions, calling git blame with a
filename means asking the question "Which commit did each line in that
file come from?" If there's no file, the question just doesn't make
sense. You could make sense of it by thinking in terms of some
revision of the file, but then you're passing a revision along --
which works just fine in a sparse checkout too.
@@ -485,15 +485,16 @@ test_expect_success 'blame with pathspec inside sparse definition' 'test_all_matchgitblamedeep/deeper1/deepest/a'-# 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''+# Blame does not support blaming files outside of the sparse+# definition, so we verify this scenario.
As above, this is misleading. It'd be better to word it something like:
# 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.
@@ -871,6 +872,15 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' ' ) '+test_expect_success 'sparse index is not expanded: blame' '+ init_repos &&++ ensure_not_expanded blame a &&+ ensure_not_expanded blame deep/a &&+ ensure_not_expanded blame deep/deeper1/a &&+ ensure_not_expanded blame deep/deeper1/deepest/a+'+ # 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' '--
On Mon, Nov 22, 2021 at 2:42 PM Lessley Dennington via GitGitGadget
[off-list ref] wrote:
quoted
From: Lessley Dennington <redacted>
Check whether git dir 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.
Additionally, ensure the above BUG is not triggered when users pass the -h
flag by adding a check for the repository to the checkout and pack-objects
builtins.
Why only checkout and pack-objects? Why don't the -h flags to all of
the following need it as well?:
$ git grep -l prepare_repo_settings | grep builtin/
builtin/add.c
builtin/blame.c
builtin/checkout.c
builtin/commit.c
builtin/diff.c
builtin/fetch.c
builtin/gc.c
builtin/merge.c
builtin/pack-objects.c
builtin/rebase.c
builtin/reset.c
builtin/revert.c
builtin/sparse-checkout.c
builtin/update-index.c
If none of these need it, was it because they put
prepare_repo_settings() calls after some other basic checks had been
done so more do not have to be added? If so, is there a similar place
in checkout and pack-objects where their calls to
prepare_repo_settings() can be moved? (Looking ahead, it appears you
moved some code in patch 2 to do something like this. Are the similar
moves that could be done here?)
Thank you for the quick feedback. Yes, I believe there are similar moves
that can be done here. I was attempting to be explicit about the case I'm
guarding against, but you're right - it shouldn't be done one way for some
builtins and another way for others.
quoted
Finally, ensure the above BUG is not triggered for commit-graph by
returning early if the git directory does not exist.
If commit-graph needs a special case to avoid triggering the BUG,
wouldn't several of these need it too?:
$ git grep -l prepare_repo_settings | grep -v builtin/
commit-graph.c
fetch-negotiator.c
merge-recursive.c
midx.c
read-cache.c
repo-settings.c
repository.c
repository.h
sparse-index.c
t/helper/test-read-cache.c
t/helper/test-read-graph.c
unpack-trees.c
or are their calls to prepare_repo_settings() only done after gitdir
setup? If the latter, perhaps the commit-graph function calls could
be moved after gitdir setup too to avoid the need to do extra checks
in it?
I'm not what the BUG() is trying to help us catch, but I'm worried
that there are many additional places that now need workarounds to
avoid triggering bugs.
I see your point. We're trying to make sure we catch issues like the
nongit scenario I overlooked in diff in earlier versions of this series.
But if we feel this change is too disruptive and will likely cause issues
beyond those I've fixed to ensure our tests pass, I can remove.
On Mon, Nov 22, 2021 at 2:42 PM Lessley Dennington via GitGitGadget
[off-list ref] wrote:
quoted
From: Lessley Dennington <redacted>
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:
d76723e (status: use sparse-index throughout, 2021-07-14)
1ba5f45 (checkout: stop expanding sparse indexes, 2021-06-29)
I preferred the references in your v3:
d76723ee53 (status: use sparse-index throughout, 2021-07-14)
1ba5f45132 (checkout: stop expanding sparse indexes, 2021-06-29)
because 7-character abbreviations aren't very future proof;
10-character seems better to me.
(Very micro nit.)
Appreciate the pointer - I'll return to the old formatting for v5.
quoted
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 [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Lessley Dennington <redacted>
---
builtin/diff.c | 5 +++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
3 files changed, 53 insertions(+)
@@ -832,6 +832,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_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged&&++# 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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--staged&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--staged
You've changed some of the --staged to --cached, but based on Junio's
comments on the previous round, you probably want to convert the
others too.
Will do for v5.
quoted
+'
+
# 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' '
--
gitgitgadget
On Thu, Oct 14, 2021 at 10:25 AM Lessley Dennington via GitGitGadget
[off-list ref] wrote:
quoted
From: Lessley Dennington <redacted>
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%
Looks good.
quoted
We do not include paths outside the sparse checkout cone because blame
currently does not support blaming files outside of the sparse definition.
Attempting to do so fails with the following error:
fatal: no such path '<path outside sparse definition>' in HEAD
While technically accurate, this wording is misleading; it implies
that there is something unique to sparse checkouts, and perhaps even
to cone mode, affecting how blame handles files not in the working
directory. That's not true, though; git blame without a revision has
always reported an error when given a file that does not exist in the
working tree. Try this in git.git:
$ rm t/README
$ git blame t/README
fatal: Cannot lstat 't/README': No such file or directory
The reason is that with no revisions, calling git blame with a
filename means asking the question "Which commit did each line in that
file come from?" If there's no file, the question just doesn't make
sense. You could make sense of it by thinking in terms of some
revision of the file, but then you're passing a revision along --
which works just fine in a sparse checkout too.
Thank you for clarifying that this is actually the expected behavior and
isn't something we need to "fix" for sparse-checkout. I will update
accordingly for v5.
@@ -485,15 +485,16 @@ test_expect_success 'blame with pathspec inside sparse definition' 'test_all_matchgitblamedeep/deeper1/deepest/a'-# 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''+# Blame does not support blaming files outside of the sparse+# definition, so we verify this scenario.
As above, this is misleading. It'd be better to word it something like:
# 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.
@@ -871,6 +872,15 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' ' ) '+test_expect_success 'sparse index is not expanded: blame' '+ init_repos &&++ ensure_not_expanded blame a &&+ ensure_not_expanded blame deep/a &&+ ensure_not_expanded blame deep/deeper1/a &&+ ensure_not_expanded blame deep/deeper1/deepest/a+'+ # 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: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-03 21:16:11
This series is based on vd/sparse-reset. It integrates the sparse index with
git diff and git blame and includes:
1. tests added to t1092 and p2000 to establish the baseline functionality
of the commands
2. repository settings to enable the sparse index
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. For 'git blame', the reduction time was ~60% for a file two levels
deep and ~30% for a file three levels deep.
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%
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%
Changes since V1
================
* Fix failing diff partially-staged test in
t1092-sparse-checkout-compatibility.sh, which was breaking in seen.
Changes since V2
================
* Update diff commit description to include patches that make the checkout
and status commands work with the sparse index for readers to reference.
* Add new test case to verify diff behaves as expected when run against
files outside the sparse checkout cone.
* Indent error message in blame commit
* Check error message in blame with pathspec outside sparse definition test
matches expectations.
* Loop blame tests (instead of running the same command multiple time
against different files).
Changes since V3
================
* Update diff p2000 tests to use --cached instead of --staged. Execute new
run and update results in commit description and cover letter.
* Update comment on blame with pathspec outside sparse definition test in
t1092-sparse-checkout-compatibility.sh to clarify that it tests the
current state and could be improved in the future.
* Ensure sparse index is only activated when diff is running against files
in a Git repo.
* BUG if prepare_repo_settings() is called outside a repository.
* Ensure sparse index is not activated for calls to blame, checkout, or
pack-object with -h.
* Ensure commit-graph is only loaded if a git directory exists.
Changes since V4
================
* Remove startup_info->have_repository check from checkout, pack-objects,
and blame. Update git.c to no longer bypass setup when -h is passed
instead.
* Move commit-graph, test-read-cache, and repo-settings changes into their
own patches with details in commit description of why the changes are
being made.
* Update t1092-sparse-checkout-compatibility.sh tests to use --cached
instead of --staged.
* Use 10-character hash abbreviations for commits referenced in diff commit
message.
* Clarify that being unable to blame files outside the working directory is
not supported in either sparse or non-sparse checkouts both in comment on
blame with pathspec outside sparse definition test in
t1092-sparse-checkout-compatibility.sh and blame commit message.
Thanks, Lessley
Lessley Dennington (7):
git: esnure correct git directory setup with -h
commit-graph: return if there is no git directory
test-read-cache: set up repo after git directory
repo-settings: prepare_repo_settings only in git repos
diff: replace --staged with --cached in t1092 tests
diff: enable and test the sparse index
blame: enable and test the sparse index
builtin/blame.c | 3 +
builtin/diff.c | 5 ++
commit-graph.c | 5 +-
git.c | 37 ++++----
repo-settings.c | 3 +
t/helper/test-read-cache.c | 5 +-
t/perf/p2000-sparse-operations.sh | 4 +
t/t1092-sparse-checkout-compatibility.sh | 109 +++++++++++++++++++----
8 files changed, 132 insertions(+), 39 deletions(-)
base-commit: f2a454e0a5e26c0f7b840970f69d195c37b16565
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1050%2Fldennington%2Fdiff-blame-sparse-index-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1050/ldennington/diff-blame-sparse-index-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/1050
Range-diff vs v4:
-: ----------- > 1: 09c2ff9f898 git: esnure correct git directory setup with -h
1: 81e208cf454 ! 2: 9e53a6435e4 sparse index: enable only for git repos
@@ Metadata
Author: Lessley Dennington [off-list ref]
## Commit message ##
- sparse index: enable only for git repos
+ commit-graph: return if there is no git directory
- Check whether git dir 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.
-
- Additionally, ensure the above BUG is not triggered when users pass the -h
- flag by adding a check for the repository to the checkout and pack-objects
- builtins.
-
- Finally, ensure the above BUG is not triggered for commit-graph by
- returning early if the git directory does not exist.
+ 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 [off-list ref]
- ## builtin/checkout.c ##
-@@ builtin/checkout.c: static int checkout_main(int argc, const char **argv, const char *prefix,
-
- git_config(git_checkout_config, opts);
-
-- prepare_repo_settings(the_repository);
-- the_repository->settings.command_requires_full_index = 0;
-+ if (startup_info->have_repository) {
-+ prepare_repo_settings(the_repository);
-+ the_repository->settings.command_requires_full_index = 0;
-+ }
-
- opts->track = BRANCH_TRACK_UNSPECIFIED;
-
-
- ## builtin/pack-objects.c ##
-@@ builtin/pack-objects.c: int cmd_pack_objects(int argc, const char **argv, const char *prefix)
- read_replace_refs = 0;
-
- sparse = git_env_bool("GIT_TEST_PACK_SPARSE", -1);
-- prepare_repo_settings(the_repository);
-- if (sparse < 0)
-- sparse = the_repository->settings.pack_use_sparse;
-+
-+ if (startup_info->have_repository) {
-+ prepare_repo_settings(the_repository);
-+ if (sparse < 0)
-+ sparse = the_repository->settings.pack_use_sparse;
-+ }
-
- reset_pack_idx_option(&pack_idx_opts);
- git_config(git_pack_config, NULL);
-
## commit-graph.c ##
@@ commit-graph.c: static int prepare_commit_graph(struct repository *r)
struct object_directory *odb;
@@ commit-graph.c: static int prepare_commit_graph(struct repository *r)
return 0;
if (r->objects->commit_graph_attempted)
-
- ## repo-settings.c ##
-@@ repo-settings.c: 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;
-
2: 5bc5e8465ab ! 3: 219a4158b6a test-read-cache: set up repo after git directory
@@ Metadata
## Commit message ##
test-read-cache: set up repo after git directory
- Move repo setup to occur after git directory is set up. This will ensure
- enabling the sparse index for `diff` (and guarding against the nongit
- scenario) will not cause tests to start failing, since that change will include
- adding a check to prepare_repo_settings() with the new BUG.
+ 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 [off-list ref]
-: ----------- > 4: 4d8d58c473b repo-settings: prepare_repo_settings only in git repos
-: ----------- > 5: 85e3e5c78e7 diff: replace --staged with --cached in t1092 tests
3: 273ee16b74e ! 6: 4f16366e5ad diff: enable and test the sparse index
@@ Commit message
with the 'git status' and 'git checkout' commands that were already
integrated. For more details see:
- d76723e (status: use sparse-index throughout, 2021-07-14)
- 1ba5f45 (checkout: stop expanding sparse indexes, 2021-06-29)
+ 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:
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
+ run_on_all ../edit-contents deep/testfile &&
+
+ test_all_match git diff &&
-+ test_all_match git diff --staged &&
++ test_all_match git diff --cached &&
+ ensure_not_expanded diff &&
-+ ensure_not_expanded diff --staged &&
++ ensure_not_expanded diff --cached &&
+
+ # Add file outside cone
+ test_all_match git reset --hard &&
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
+ test_sparse_match git sparse-checkout set &&
+
+ test_all_match git diff &&
-+ test_all_match git diff --staged &&
++ test_all_match git diff --cached &&
+ ensure_not_expanded diff &&
-+ ensure_not_expanded diff --staged &&
++ ensure_not_expanded diff --cached &&
+
+ # Merge conflict outside cone
+ # The sparse checkout will report a warning that is not in the
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is n
+ test_all_match test_must_fail git merge merge-right &&
+
+ test_all_match git diff &&
-+ test_all_match git diff --staged &&
++ test_all_match git diff --cached &&
+ ensure_not_expanded diff &&
-+ ensure_not_expanded diff --staged
++ ensure_not_expanded diff --cached
+'
+
# NEEDSWORK: a sparse-checkout behaves differently from a full checkout
4: 7acf5118bf5 ! 7: 04532378734 blame: enable and test the sparse index
@@ Commit message
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
- currently does not support blaming files outside of the sparse definition.
- Attempting to do so fails with the following error:
-
- fatal: no such path '<path outside sparse definition>' in HEAD
+ 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 [off-list ref]
## builtin/blame.c ##
-@@ builtin/blame.c: int cmd_blame(int argc, const char **argv, const char *prefix)
- long anchor;
- const int hexsz = the_hash_algo->hexsz;
+@@ builtin/blame.c: parse_done:
+ revs.diffopt.flags.follow_renames = 0;
+ argc = parse_options_end(&ctx);
-+ if (startup_info->have_repository) {
-+ prepare_repo_settings(the_repository);
-+ the_repository->settings.command_requires_full_index = 0;
-+ }
++ prepare_repo_settings(the_repository);
++ the_repository->settings.command_requires_full_index = 0;
+
- setup_default_color_by_age();
- git_config(git_blame_config, &output_option);
- repo_init_revisions(the_repository, &revs, NULL);
+ if (incremental || (output_option & OUTPUT_PORCELAIN)) {
+ if (show_progress > 0)
+ die(_("--progress can't be used with --incremental or porcelain formats"));
## t/perf/p2000-sparse-operations.sh ##
-@@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git reset
- test_perf_on_all git reset --hard
+@@ t/perf/p2000-sparse-operations.sh: 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 diff --staged
+ 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
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'log with pathspec
-# 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' '
-+# NEEDSWORK: This test documents the current behavior, but this could
-+# change in the future if we decide to support blaming files outside
-+# the 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 &&
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'log with pathspec
test_expect_success 'checkout and reset (mixed)' '
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse index is not expanded: diff' '
- ensure_not_expanded diff --staged
+ ensure_not_expanded diff --cached
'
+test_expect_success 'sparse index is not expanded: blame' '
--
gitgitgadget
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-03 21:16:15
From: Lessley Dennington <redacted>
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 [off-list ref]
Signed-off-by: Lessley Dennington <redacted>
---
git.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
@@ -421,27 +421,28 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)intstatus,help;structstatst;constchar*prefix;-+intrun_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();-elseif(p->option&RUN_SETUP_GENTLY){-intnongit_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();+elseif(run_setup&RUN_SETUP_GENTLY){+intnongit_ok;+prefix=setup_git_directory_gently(&nongit_ok);}+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: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-03 21:16:16
From: Lessley Dennington <redacted>
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 <redacted>
---
commit-graph.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-03 21:16:18
From: Lessley Dennington <redacted>
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 <redacted>
---
repo-settings.c | 3 +++
1 file changed, 3 insertions(+)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-03 21:16:19
From: Lessley Dennington <redacted>
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 <redacted>
---
t/helper/test-read-cache.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-03 21:16:20
From: Lessley Dennington <redacted>
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 <redacted>
---
t/t1092-sparse-checkout-compatibility.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-03 21:16:21
From: Lessley Dennington <redacted>
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 [off-list ref]
Signed-off-by: Derrick Stolee <redacted>
Signed-off-by: Lessley Dennington <redacted>
---
builtin/diff.c | 5 +++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
3 files changed, 53 insertions(+)
@@ -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_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--cached&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--cached&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--cached&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--cached&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--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: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-03 21:16:22
From: Lessley Dennington <redacted>
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 <redacted>
---
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(-)
@@ -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"));
@@ -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_matchgitblamea&&-test_all_matchgitblamedeep/a&&-test_all_matchgitblamedeep/deeper1/a&&-test_all_matchgitblamedeep/deeper1/deepest/a+forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+test_all_matchgitblame$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_matchgitsparse-checkoutset&&-test_all_matchgitblamefolder1/a&&-test_all_matchgitblamefolder2/a&&-test_all_matchgitblamedeep/deeper2/a&&-test_all_matchgitblamedeep/deeper2/deepest/a+forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+test_sparse_matchtest_must_failgitblame$file&&+cat>expect<<-EOF&&+fatal:Cannotlstat'"'"'$file'"'"':Nosuchfileordirectory+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_cmpexpectsparse-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_expandeddiff--cached'+test_expect_success'sparse index is not expanded: blame''+init_repos&&++forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+ensure_not_expandedblame$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''
On Fri, Dec 3, 2021 at 1:16 PM Lessley Dennington via GitGitGadget
[off-list ref] wrote:
Simple typo in the subject of the commit message: "esnure" -> "ensure"
quoted hunk
From: Lessley Dennington <redacted>
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 [off-list ref]
Signed-off-by: Lessley Dennington <redacted>
---
git.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
@@ -421,27 +421,28 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)intstatus,help;structstatst;constchar*prefix;-+intrun_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();-elseif(p->option&RUN_SETUP_GENTLY){-intnongit_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();+elseif(run_setup&RUN_SETUP_GENTLY){+intnongit_ok;+prefix=setup_git_directory_gently(&nongit_ok);}+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()){--
On Fri, Dec 3, 2021 at 1:16 PM Lessley Dennington via GitGitGadget
[off-list ref] wrote:
This series is based on vd/sparse-reset. It integrates the sparse index with
git diff and git blame and includes:
1. tests added to t1092 and p2000 to establish the baseline functionality
of the commands
2. repository settings to enable the sparse index
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. For 'git blame', the reduction time was ~60% for a file two levels
deep and ~30% for a file three levels deep.
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%
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%
Changes since V1
================
* Fix failing diff partially-staged test in
t1092-sparse-checkout-compatibility.sh, which was breaking in seen.
Changes since V2
================
* Update diff commit description to include patches that make the checkout
and status commands work with the sparse index for readers to reference.
* Add new test case to verify diff behaves as expected when run against
files outside the sparse checkout cone.
* Indent error message in blame commit
* Check error message in blame with pathspec outside sparse definition test
matches expectations.
* Loop blame tests (instead of running the same command multiple time
against different files).
Changes since V3
================
* Update diff p2000 tests to use --cached instead of --staged. Execute new
run and update results in commit description and cover letter.
* Update comment on blame with pathspec outside sparse definition test in
t1092-sparse-checkout-compatibility.sh to clarify that it tests the
current state and could be improved in the future.
* Ensure sparse index is only activated when diff is running against files
in a Git repo.
* BUG if prepare_repo_settings() is called outside a repository.
* Ensure sparse index is not activated for calls to blame, checkout, or
pack-object with -h.
* Ensure commit-graph is only loaded if a git directory exists.
Changes since V4
================
* Remove startup_info->have_repository check from checkout, pack-objects,
and blame. Update git.c to no longer bypass setup when -h is passed
instead.
* Move commit-graph, test-read-cache, and repo-settings changes into their
own patches with details in commit description of why the changes are
being made.
* Update t1092-sparse-checkout-compatibility.sh tests to use --cached
instead of --staged.
* Use 10-character hash abbreviations for commits referenced in diff commit
message.
* Clarify that being unable to blame files outside the working directory is
not supported in either sparse or non-sparse checkouts both in comment on
blame with pathspec outside sparse definition test in
t1092-sparse-checkout-compatibility.sh and blame commit message.
This round addresses all my concerns from previous rounds. There's a
trivial typo in the subject of the new patch 1, but feel free to add
my
Reviewed-by: Elijah Newren <redacted>
when you resubmit with that fix. Nice work!
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-06 16:10:39
This series is based on vd/sparse-reset. It integrates the sparse index with
git diff and git blame and includes:
1. tests added to t1092 and p2000 to establish the baseline functionality
of the commands
2. repository settings to enable the sparse index
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. For 'git blame', the reduction time was ~60% for a file two levels
deep and ~30% for a file three levels deep.
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%
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%
Changes since V1
================
* Fix failing diff partially-staged test in
t1092-sparse-checkout-compatibility.sh, which was breaking in seen.
Changes since V2
================
* Update diff commit description to include patches that make the checkout
and status commands work with the sparse index for readers to reference.
* Add new test case to verify diff behaves as expected when run against
files outside the sparse checkout cone.
* Indent error message in blame commit
* Check error message in blame with pathspec outside sparse definition test
matches expectations.
* Loop blame tests (instead of running the same command multiple time
against different files).
Changes since V3
================
* Update diff p2000 tests to use --cached instead of --staged. Execute new
run and update results in commit description and cover letter.
* Update comment on blame with pathspec outside sparse definition test in
t1092-sparse-checkout-compatibility.sh to clarify that it tests the
current state and could be improved in the future.
* Ensure sparse index is only activated when diff is running against files
in a Git repo.
* BUG if prepare_repo_settings() is called outside a repository.
* Ensure sparse index is not activated for calls to blame, checkout, or
pack-object with -h.
* Ensure commit-graph is only loaded if a git directory exists.
Changes since V4
================
* Remove startup_info->have_repository check from checkout, pack-objects,
and blame. Update git.c to no longer bypass setup when -h is passed
instead.
* Move commit-graph, test-read-cache, and repo-settings changes into their
own patches with details in commit description of why the changes are
being made.
* Update t1092-sparse-checkout-compatibility.sh tests to use --cached
instead of --staged.
* Use 10-character hash abbreviations for commits referenced in diff commit
message.
* Clarify that being unable to blame files outside the working directory is
not supported in either sparse or non-sparse checkouts both in comment on
blame with pathspec outside sparse definition test in
t1092-sparse-checkout-compatibility.sh and blame commit message.
Changes since V5
================
* Fix commit message typo.
* Re-add blank line to separate variable declarations from statements in
run_builtin.
* Refactor prefix NULL assignment in run_builtin.
Thanks, Lessley
Lessley Dennington (7):
git: ensure correct git directory setup with -h
commit-graph: return if there is no git directory
test-read-cache: set up repo after git directory
repo-settings: prepare_repo_settings only in git repos
diff: replace --staged with --cached in t1092 tests
diff: enable and test the sparse index
blame: enable and test the sparse index
builtin/blame.c | 3 +
builtin/diff.c | 5 ++
commit-graph.c | 5 +-
git.c | 39 ++++----
repo-settings.c | 3 +
t/helper/test-read-cache.c | 5 +-
t/perf/p2000-sparse-operations.sh | 4 +
t/t1092-sparse-checkout-compatibility.sh | 109 +++++++++++++++++++----
8 files changed, 134 insertions(+), 39 deletions(-)
base-commit: f2a454e0a5e26c0f7b840970f69d195c37b16565
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1050%2Fldennington%2Fdiff-blame-sparse-index-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1050/ldennington/diff-blame-sparse-index-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1050
Range-diff vs v5:
1: 09c2ff9f898 ! 1: efdd55c126d git: esnure correct git directory setup with -h
@@ Metadata
Author: Lessley Dennington [off-list ref]
## Commit message ##
- git: esnure correct git directory setup with -h
+ 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
@@ Commit message
Co-authored-by: Junio C Hamano [off-list ref]
Signed-off-by: Lessley Dennington [off-list ref]
+ Reviewed-by: Elijah Newren [off-list ref]
## git.c ##
@@ git.c: 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;
+
+- prefix = NULL;
help = argc == 2 && !strcmp(argv[1], "-h");
- if (!help) {
- if (p->option & RUN_SETUP)
@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
+ /* demote to GENTLY to allow 'git cmd -h' outside repo */
+ run_setup = RUN_SETUP_GENTLY;
+
-+ if (run_setup & RUN_SETUP)
++ if (run_setup & RUN_SETUP) {
+ prefix = setup_git_directory();
-+ else if (run_setup & RUN_SETUP_GENTLY) {
++ } 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 &&
2: 9e53a6435e4 ! 2: f676f03ccb0 commit-graph: return if there is no git directory
@@ Commit message
git directory exists.
Signed-off-by: Lessley Dennington [off-list ref]
+ Reviewed-by: Elijah Newren [off-list ref]
## commit-graph.c ##
@@ commit-graph.c: static int prepare_commit_graph(struct repository *r)
3: 219a4158b6a ! 3: 7b1fab86a4a test-read-cache: set up repo after git directory
@@ Commit message
prepare_repo_settings if no git directory exists.
Signed-off-by: Lessley Dennington [off-list ref]
+ Reviewed-by: Elijah Newren [off-list ref]
## t/helper/test-read-cache.c ##
@@ t/helper/test-read-cache.c: int cmd__read_cache(int argc, const char **argv)
4: 4d8d58c473b ! 4: fd28be71ca4 repo-settings: prepare_repo_settings only in git repos
@@ Commit message
settings.
Signed-off-by: Lessley Dennington [off-list ref]
+ Reviewed-by: Elijah Newren [off-list ref]
## repo-settings.c ##
@@ repo-settings.c: void prepare_repo_settings(struct repository *r)
5: 85e3e5c78e7 ! 5: 2a1524a7e9a diff: replace --staged with --cached in t1092 tests
@@ Commit message
diff.
Signed-off-by: Lessley Dennington [off-list ref]
+ Reviewed-by: Elijah Newren [off-list ref]
## t/t1092-sparse-checkout-compatibility.sh ##
@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'checkout and reset --hard' '
6: 4f16366e5ad ! 6: 897611682af diff: enable and test the sparse index
@@ Commit message
2000.37: git diff --cached (sparse-v4) 0.23 0.03 -87.0%
Co-authored-by: Derrick Stolee [off-list ref]
- Signed-off-by: Derrick Stolee [off-list ref]
Signed-off-by: Lessley Dennington [off-list ref]
+ Reviewed-by: Elijah Newren [off-list ref]
## builtin/diff.c ##
@@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)
7: 04532378734 ! 7: 85bcbaa1771 blame: enable and test the sparse index
@@ Commit message
directory. This is true in both sparse and full checkouts.
Signed-off-by: Lessley Dennington [off-list ref]
+ Reviewed-by: Elijah Newren [off-list ref]
## builtin/blame.c ##
@@ builtin/blame.c: parse_done:
--
gitgitgadget
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-06 16:10:41
From: Lessley Dennington <redacted>
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 [off-list ref]
Signed-off-by: Lessley Dennington <redacted>
Reviewed-by: Elijah Newren <redacted>
---
git.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
@@ -421,27 +421,30 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)intstatus,help;structstatst;constchar*prefix;+intrun_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();-elseif(p->option&RUN_SETUP_GENTLY){-intnongit_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();+}elseif(run_setup&RUN_SETUP_GENTLY){+intnongit_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: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-06 16:10:42
From: Lessley Dennington <redacted>
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 <redacted>
Reviewed-by: Elijah Newren <redacted>
---
commit-graph.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-06 16:10:46
From: Lessley Dennington <redacted>
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 <redacted>
Reviewed-by: Elijah Newren <redacted>
---
t/helper/test-read-cache.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-06 16:10:47
From: Lessley Dennington <redacted>
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 <redacted>
Reviewed-by: Elijah Newren <redacted>
---
repo-settings.c | 3 +++
1 file changed, 3 insertions(+)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-06 16:11:01
From: Lessley Dennington <redacted>
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 <redacted>
Reviewed-by: Elijah Newren <redacted>
---
t/t1092-sparse-checkout-compatibility.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
From: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-06 16:11:04
From: Lessley Dennington <redacted>
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 [off-list ref]
Signed-off-by: Lessley Dennington <redacted>
Reviewed-by: Elijah Newren <redacted>
---
builtin/diff.c | 5 +++
t/perf/p2000-sparse-operations.sh | 2 ++
t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
3 files changed, 53 insertions(+)
@@ -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_scriptedit-contents<<-\EOF&&+echotext>>$1+EOF++# Add file within cone+test_sparse_matchgitsparse-checkoutsetdeep&&+run_on_all../edit-contentsdeep/testfile&&+test_all_matchgitadddeep/testfile&&+run_on_all../edit-contentsdeep/testfile&&++test_all_matchgitdiff&&+test_all_matchgitdiff--cached&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--cached&&++# Add file outside cone+test_all_matchgitreset--hard&&+run_on_allmkdirnewdirectory&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutsetnewdirectory&&+test_all_matchgitaddnewdirectory/testfile&&+run_on_all../edit-contentsnewdirectory/testfile&&+test_sparse_matchgitsparse-checkoutset&&++test_all_matchgitdiff&&+test_all_matchgitdiff--cached&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--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_allgitreset--hard&&+test_all_matchgitcheckoutmerge-left&&+test_all_matchtest_must_failgitmergemerge-right&&++test_all_matchgitdiff&&+test_all_matchgitdiff--cached&&+ensure_not_expandeddiff&&+ensure_not_expandeddiff--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: Lessley Dennington via GitGitGadget <hidden> Date: 2021-12-06 16:11:05
From: Lessley Dennington <redacted>
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 <redacted>
Reviewed-by: Elijah Newren <redacted>
---
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(-)
@@ -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"));
@@ -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_matchgitblamea&&-test_all_matchgitblamedeep/a&&-test_all_matchgitblamedeep/deeper1/a&&-test_all_matchgitblamedeep/deeper1/deepest/a+forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+test_all_matchgitblame$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_matchgitsparse-checkoutset&&-test_all_matchgitblamefolder1/a&&-test_all_matchgitblamefolder2/a&&-test_all_matchgitblamedeep/deeper2/a&&-test_all_matchgitblamedeep/deeper2/deepest/a+forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+test_sparse_matchtest_must_failgitblame$file&&+cat>expect<<-EOF&&+fatal:Cannotlstat'"'"'$file'"'"':Nosuchfileordirectory+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_cmpexpectsparse-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_expandeddiff--cached'+test_expect_success'sparse index is not expanded: blame''+init_repos&&++forfileina\+deep/a\+deep/deeper1/a\+deep/deeper1/deepest/a+do+ensure_not_expandedblame$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''
On Fri, Dec 03 2021, Lessley Dennington via GitGitGadget wrote:
quoted hunk
From: Lessley Dennington <redacted>
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 <redacted>
---
repo-settings.c | 3 +++
1 file changed, 3 insertions(+)
nit: start BUG(), error() etc. messages with lower-case.
+
if (r->settings.initialized++)
return;
Our config doesn't require us to have a repo, and most of what
prepare_repo_settings() is doing is reading global config.
I think that *currently* this won't break things, but e.g. if we ever
want to have "feature.experimental" or whatever change the behavior of a
a command that doesn't require a repository we'd need to untangle this
(currently everything it changes requires a repo AFAICT).
Perhaps this is fine, and if we ever need such a "global config" point
we should stick it closer to common-main.c...
On 12/6/21 8:43 PM, Ævar Arnfjörð Bjarmason wrote:
On Fri, Dec 03 2021, Lessley Dennington via GitGitGadget wrote:
quoted
From: Lessley Dennington <redacted>
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 <redacted>
---
repo-settings.c | 3 +++
1 file changed, 3 insertions(+)
nit: start BUG(), error() etc. messages with lower-case.
Thanks for catching this. Instead of re-sending the whole series for a
one-letter change, I've included a patch with the fix at the end of this
message.
quoted
+
if (r->settings.initialized++)
return;
Our config doesn't require us to have a repo, and most of what
prepare_repo_settings() is doing is reading global config.
I think that *currently* this won't break things, but e.g. if we ever
want to have "feature.experimental" or whatever change the behavior of a
a command that doesn't require a repository we'd need to untangle this
(currently everything it changes requires a repo AFAICT).
Perhaps this is fine, and if we ever need such a "global config" point
we should stick it closer to common-main.c...
Agreed, let's keep the change for now and address updates in the future if
the need arises.
-----------
From 2764ca684320f488006093505deb074217ac7b31 Mon Sep 17 00:00:00 2001
From: Lessley Dennington <redacted>
Date: Wed, 8 Dec 2021 09:29:56 -0600
Subject: [PATCH] fixup! repo-settings: prepare_repo_settings only in git repos
Update the BUG() message in prepare_repo_settings to begin with a
lower-case letter.
Reported-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Lessley Dennington <redacted>
---
repo-settings.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)