Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparse Index: diff and blame builtins #1050

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions builtin/blame.c
Original file line number Diff line number Diff line change
Expand Up @@ -940,6 +940,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
revs.diffopt.flags.follow_renames = 0;
argc = parse_options_end(&ctx);

prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;

if (incremental || (output_option & OUTPUT_PORCELAIN)) {
if (show_progress > 0)
die(_("--progress can't be used with --incremental or porcelain formats"));
Expand Down
5 changes: 5 additions & 0 deletions builtin/diff.c
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Derrick Stolee wrote (reply to this):

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.

> +	run_on_all ../edit-contents deep/testfile &&
> +	test_all_match git add deep/testfile &&

But the test fails here because "deep/testfile" doesn't match
the sparse-checkout definition in the full-checkout repo.

> +	run_on_all ../edit-contents deep/testfile &&
> +
> +	test_all_match git diff &&
> +	test_all_match git diff --staged &&
> +
> +	# Add file outside cone
> +	test_all_match git reset --hard &&
> +	run_on_all mkdir newdirectory &&
> +	run_on_all ../edit-contents newdirectory/testfile &&
> +	test_all_match git sparse-checkout set newdirectory &&

You'll want to change this line, too.

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Fri, Oct 15, 2021 at 09:20:34PM +0000, Lessley Dennington via GitGitGadget wrote:
> From: Lessley Dennington <lessleydennington@gmail.com>
>
> 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.

> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index f19c1b3e2eb..e5d15be9d45 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -386,6 +386,46 @@ test_expect_success 'diff --staged' '
>  	test_all_match git diff --staged
>  '
>
> +test_expect_success 'diff partially-staged' '
> +	init_repos &&
> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>$1
> +	EOF
> +
> +	# Add file within cone
> +	test_sparse_match git sparse-checkout set deep &&
> +	run_on_all ../edit-contents deep/testfile &&
> +	test_all_match git add deep/testfile &&
> +	run_on_all ../edit-contents deep/testfile &&
> +
> +	test_all_match git diff &&
> +	test_all_match git diff --staged &&
> +
> +	# Add file outside cone
> +	test_all_match git reset --hard &&
> +	run_on_all mkdir newdirectory &&
> +	run_on_all ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git sparse-checkout set newdirectory &&
> +	test_all_match git add newdirectory/testfile &&
> +	run_on_all ../edit-contents newdirectory/testfile &&
> +	test_sparse_match git sparse-checkout set &&
> +
> +	test_all_match git diff &&
> +	test_all_match git diff --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_all git reset --hard &&
> +	test_all_match git checkout merge-left &&
> +	test_all_match test_must_fail git merge merge-right &&
> +
> +	test_all_match git diff &&
> +	test_all_match git diff --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 expansion
>  	ensure_not_expanded reset deepest -- folder\* &&
>
> +	echo a test change >>sparse-index/README.md &&
> +	ensure_not_expanded diff &&

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Lessley Dennington wrote (reply to this):



On 10/25/21 1:47 PM, Taylor Blau wrote:
> On Fri, Oct 15, 2021 at 09:20:34PM +0000, Lessley Dennington via GitGitGadget wrote:
>> From: Lessley Dennington <lessleydennington@gmail.com>
>>
>> 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!
>> 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.
> 
>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>> index f19c1b3e2eb..e5d15be9d45 100755
>> --- a/t/t1092-sparse-checkout-compatibility.sh
>> +++ b/t/t1092-sparse-checkout-compatibility.sh
>> @@ -386,6 +386,46 @@ test_expect_success 'diff --staged' '
>>   	test_all_match git diff --staged
>>   '
>>
>> +test_expect_success 'diff partially-staged' '
>> +	init_repos &&
>> +
>> +	write_script edit-contents <<-\EOF &&
>> +	echo text >>$1
>> +	EOF
>> +
>> +	# Add file within cone
>> +	test_sparse_match git sparse-checkout set deep &&
>> +	run_on_all ../edit-contents deep/testfile &&
>> +	test_all_match git add deep/testfile &&
>> +	run_on_all ../edit-contents deep/testfile &&
>> +
>> +	test_all_match git diff &&
>> +	test_all_match git diff --staged &&
>> +
>> +	# Add file outside cone
>> +	test_all_match git reset --hard &&
>> +	run_on_all mkdir newdirectory &&
>> +	run_on_all ../edit-contents newdirectory/testfile &&
>> +	test_sparse_match git sparse-checkout set newdirectory &&
>> +	test_all_match git add newdirectory/testfile &&
>> +	run_on_all ../edit-contents newdirectory/testfile &&
>> +	test_sparse_match git sparse-checkout set &&
>> +
>> +	test_all_match git diff &&
>> +	test_all_match git diff --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_all git reset --hard &&
>> +	test_all_match git checkout merge-left &&
>> +	test_all_match test_must_fail git merge merge-right &&
>> +
>> +	test_all_match git diff &&
>> +	test_all_match git diff --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 expansion
>>   	ensure_not_expanded reset deepest -- folder\* &&
>>
>> +	echo a test change >>sparse-index/README.md &&
>> +	ensure_not_expanded diff &&
> 
> 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.

> Thanks,
> Taylor
> 
Best,
Lessley

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Taylor Blau wrote (reply to this):

On Tue, Oct 26, 2021 at 09:10:20AM -0700, Lessley Dennington wrote:
> > 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> 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%

Please do not add more use of the synonym to the test suite, other
than the one that makes sure the synonym works the same way as the
real option, which is "--cached".

> diff --git a/builtin/diff.c b/builtin/diff.c
> index dd8ce688ba7..cbf7b51c7c0 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -437,6 +437,9 @@ 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;
> +

Doesn't the code need to be protected with

	if (!nongit) {
		prepare_repo_settings(the_repository);
		the_repository->settings.command_requires_full_index = 0;
	}

at the very least?  It may be that the code is getting lucky because
the_repository may be initialized with a random value (after all,
when we are not in a repository, there is nowhere to read the
on-disk settings from) and we may even be able to set a bit in the
settings structure without crashing, but conceptually, doing the
above when we _know_ we are not in any repository is simply wrong.

I wonder if prepare_repo_settings() needs be more strict.  For
example, shouldn't it check if we have a repository to begin with
and BUG() if it was called when there is not a repository?  After
all, it tries to read from the repository configuration file, so any
necessary set-up to discover where the gitdir is must have been done
already before it can be called.

With such a safety feature to catch a programmer errors, perhaps the
above could have been caught before the patch hit the list.

Thoughts?  Am I missing some chicken-and-egg situation where
prepare_repo_settings() must be callable before we know where the
repository is, or something, which justifies why the function is so
loose in its sanity checks in the current form?


Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Lessley Dennington wrote (reply to this):

On 11/3/21 10:05 AM, Junio C Hamano wrote:
> "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> 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%
> 
> Please do not add more use of the synonym to the test suite, other
> than the one that makes sure the synonym works the same way as the
> real option, which is "--cached".
>

Thank you, changed for v4.

>> diff --git a/builtin/diff.c b/builtin/diff.c
>> index dd8ce688ba7..cbf7b51c7c0 100644
>> --- a/builtin/diff.c
>> +++ b/builtin/diff.c
>> @@ -437,6 +437,9 @@ 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;
>> +
> 
> Doesn't the code need to be protected with
> 
> 	if (!nongit) {
> 		prepare_repo_settings(the_repository);
> 		the_repository->settings.command_requires_full_index = 0;
> 	}
> 
> at the very least?  It may be that the code is getting lucky because
> the_repository may be initialized with a random value (after all,
> when we are not in a repository, there is nowhere to read the
> on-disk settings from) and we may even be able to set a bit in the
> settings structure without crashing, but conceptually, doing the
> above when we _know_ we are not in any repository is simply wrong.
> 
> I wonder if prepare_repo_settings() needs be more strict.  For
> example, shouldn't it check if we have a repository to begin with
> and BUG() if it was called when there is not a repository?  After
> all, it tries to read from the repository configuration file, so any
> necessary set-up to discover where the gitdir is must have been done
> already before it can be called.
> 
> With such a safety feature to catch a programmer errors, perhaps the
> above could have been caught before the patch hit the list.
> 
> Thoughts?  Am I missing some chicken-and-egg situation where
> prepare_repo_settings() must be callable before we know where the
> repository is, or something, which justifies why the function is so
> loose in its sanity checks in the current form?
> 
> 

This seems like a good idea. I've added both the nongit check and the 
prepare_repo_settings() updates you've suggested for v4, pending review 
by my team.

Best,
Lessley

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Mon, Nov 22, 2021 at 2:42 PM Lessley Dennington via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Lessley Dennington <lessleydennington@gmail.com>
>
> 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.)

>
> 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 <dstolee@microsoft.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
> ---
>  builtin/diff.c                           |  5 +++
>  t/perf/p2000-sparse-operations.sh        |  2 ++
>  t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
>  3 files changed, 53 insertions(+)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index dd8ce688ba7..fa4683377eb 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -437,6 +437,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>
>         prefix = setup_git_directory_gently(&nongit);
>
> +       if (!nongit) {
> +               prepare_repo_settings(the_repository);
> +               the_repository->settings.command_requires_full_index = 0;
> +       }
> +
>         if (!no_index) {
>                 /*
>                  * Treat git diff with at least one path outside of the
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index bfd332120c8..5cf94627383 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -113,5 +113,7 @@ test_perf_on_all git checkout -f -
>  test_perf_on_all git reset
>  test_perf_on_all git reset --hard
>  test_perf_on_all git reset -- does-not-exist
> +test_perf_on_all git diff
> +test_perf_on_all git diff --cached
>
>  test_done
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 44d5e11c762..53524660759 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -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_script edit-contents <<-\EOF &&
> +       echo text >>$1
> +       EOF
> +
> +       # Add file within cone
> +       test_sparse_match git sparse-checkout set deep &&
> +       run_on_all ../edit-contents deep/testfile &&
> +       test_all_match git add deep/testfile &&
> +       run_on_all ../edit-contents deep/testfile &&
> +
> +       test_all_match git diff &&
> +       test_all_match git diff --staged &&
> +       ensure_not_expanded diff &&
> +       ensure_not_expanded diff --staged &&
> +
> +       # Add file outside cone
> +       test_all_match git reset --hard &&
> +       run_on_all mkdir newdirectory &&
> +       run_on_all ../edit-contents newdirectory/testfile &&
> +       test_sparse_match git sparse-checkout set newdirectory &&
> +       test_all_match git add newdirectory/testfile &&
> +       run_on_all ../edit-contents newdirectory/testfile &&
> +       test_sparse_match git sparse-checkout set &&
> +
> +       test_all_match git diff &&
> +       test_all_match git diff --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
> +       # full checkout, so we use `run_on_all` instead of
> +       # `test_all_match`
> +       run_on_all git reset --hard &&
> +       test_all_match git checkout merge-left &&
> +       test_all_match test_must_fail git merge merge-right &&
> +
> +       test_all_match git diff &&
> +       test_all_match git diff --staged &&
> +       ensure_not_expanded diff &&
> +       ensure_not_expanded diff --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
>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Lessley Dennington wrote (reply to this):



On 11/22/21 11:47 PM, Elijah Newren wrote:
> On Mon, Nov 22, 2021 at 2:42 PM Lessley Dennington via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Lessley Dennington <lessleydennington@gmail.com>
>>
>> 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.
>>
>> 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 <dstolee@microsoft.com>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
>> ---
>>   builtin/diff.c                           |  5 +++
>>   t/perf/p2000-sparse-operations.sh        |  2 ++
>>   t/t1092-sparse-checkout-compatibility.sh | 46 ++++++++++++++++++++++++
>>   3 files changed, 53 insertions(+)
>>
>> diff --git a/builtin/diff.c b/builtin/diff.c
>> index dd8ce688ba7..fa4683377eb 100644
>> --- a/builtin/diff.c
>> +++ b/builtin/diff.c
>> @@ -437,6 +437,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>>
>>          prefix = setup_git_directory_gently(&nongit);
>>
>> +       if (!nongit) {
>> +               prepare_repo_settings(the_repository);
>> +               the_repository->settings.command_requires_full_index = 0;
>> +       }
>> +
>>          if (!no_index) {
>>                  /*
>>                   * Treat git diff with at least one path outside of the
>> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
>> index bfd332120c8..5cf94627383 100755
>> --- a/t/perf/p2000-sparse-operations.sh
>> +++ b/t/perf/p2000-sparse-operations.sh
>> @@ -113,5 +113,7 @@ test_perf_on_all git checkout -f -
>>   test_perf_on_all git reset
>>   test_perf_on_all git reset --hard
>>   test_perf_on_all git reset -- does-not-exist
>> +test_perf_on_all git diff
>> +test_perf_on_all git diff --cached
>>
>>   test_done
>> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
>> index 44d5e11c762..53524660759 100755
>> --- a/t/t1092-sparse-checkout-compatibility.sh
>> +++ b/t/t1092-sparse-checkout-compatibility.sh
>> @@ -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_script edit-contents <<-\EOF &&
>> +       echo text >>$1
>> +       EOF
>> +
>> +       # Add file within cone
>> +       test_sparse_match git sparse-checkout set deep &&
>> +       run_on_all ../edit-contents deep/testfile &&
>> +       test_all_match git add deep/testfile &&
>> +       run_on_all ../edit-contents deep/testfile &&
>> +
>> +       test_all_match git diff &&
>> +       test_all_match git diff --staged &&
>> +       ensure_not_expanded diff &&
>> +       ensure_not_expanded diff --staged &&
>> +
>> +       # Add file outside cone
>> +       test_all_match git reset --hard &&
>> +       run_on_all mkdir newdirectory &&
>> +       run_on_all ../edit-contents newdirectory/testfile &&
>> +       test_sparse_match git sparse-checkout set newdirectory &&
>> +       test_all_match git add newdirectory/testfile &&
>> +       run_on_all ../edit-contents newdirectory/testfile &&
>> +       test_sparse_match git sparse-checkout set &&
>> +
>> +       test_all_match git diff &&
>> +       test_all_match git diff --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
>> +       # full checkout, so we use `run_on_all` instead of
>> +       # `test_all_match`
>> +       run_on_all git reset --hard &&
>> +       test_all_match git checkout merge-left &&
>> +       test_all_match test_must_fail git merge merge-right &&
>> +
>> +       test_all_match git diff &&
>> +       test_all_match git diff --staged &&
>> +       ensure_not_expanded diff &&
>> +       ensure_not_expanded diff --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.
>> +'
>> +
>>   # 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
>>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/builtin/diff.c b/builtin/diff.c
> index dd8ce688ba7..fa4683377eb 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -437,6 +437,11 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  
>  	prefix = setup_git_directory_gently(&nongit);
>  
> +	if (!nongit) {
> +		prepare_repo_settings(the_repository);
> +		the_repository->settings.command_requires_full_index = 0;
> +	}
> +

It is very pleasing to see that with all the preparations, it is
jsut a very simple matter of adding four lines to enable the big
feature.

At this point immediately after asking setup_git_directory_gently(),
we positively know if we are in a repository, so the placement of
the new code makes perfect sense.

Nice.


prefix = setup_git_directory_gently(&nongit);

if (!nongit) {
ldennington marked this conversation as resolved.
Show resolved Hide resolved
prepare_repo_settings(the_repository);
the_repository->settings.command_requires_full_index = 0;
}

if (!no_index) {
/*
* Treat git diff with at least one path outside of the
Expand Down
5 changes: 4 additions & 1 deletion commit-graph.c
Original file line number Diff line number Diff line change
Expand Up @@ -632,10 +632,13 @@ static int prepare_commit_graph(struct repository *r)
struct object_directory *odb;

/*
* Early return if there is no git dir or if the commit graph is
* disabled.
*
* This must come before the "already attempted?" check below, because
* we want to disable even an already-loaded graph file.
*/
if (r->commit_graph_disabled)
if (!r->gitdir || r->commit_graph_disabled)
return 0;

if (r->objects->commit_graph_attempted)
Expand Down
39 changes: 21 additions & 18 deletions git.c
Original file line number Diff line number Diff line change
Expand Up @@ -421,27 +421,30 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
int status, help;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Elijah Newren wrote (reply to this):

On Fri, Dec 3, 2021 at 1:16 PM Lessley Dennington via GitGitGadget
<gitgitgadget@gmail.com> wrote:

Simple typo in the subject of the commit message: "esnure" -> "ensure"

> From: Lessley Dennington <lessleydennington@gmail.com>
>
> 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 <gitster@pobox.com>
> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
> ---
>  git.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
>
> diff --git a/git.c b/git.c
> index 60c2784be45..03a2dfa5174 100644
> --- a/git.c
> +++ b/git.c
> @@ -421,27 +421,28 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
>         int status, help;
>         struct stat st;
>         const char *prefix;
> -
> +       int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));
>         prefix = NULL;
>         help = argc == 2 && !strcmp(argv[1], "-h");
> -       if (!help) {
> -               if (p->option & RUN_SETUP)
> -                       prefix = setup_git_directory();
> -               else if (p->option & RUN_SETUP_GENTLY) {
> -                       int nongit_ok;
> -                       prefix = setup_git_directory_gently(&nongit_ok);
> -               }
> -               precompose_argv_prefix(argc, argv, NULL);
> -               if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
> -                   !(p->option & DELAY_PAGER_CONFIG))
> -                       use_pager = check_pager_config(p->cmd);
> -               if (use_pager == -1 && p->option & USE_PAGER)
> -                       use_pager = 1;
> -
> -               if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
> -                   startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
> -                       trace_repo_setup(prefix);
> +       if (help && (run_setup & RUN_SETUP))
> +               /* demote to GENTLY to allow 'git cmd -h' outside repo */
> +               run_setup = RUN_SETUP_GENTLY;
> +
> +       if (run_setup & RUN_SETUP)
> +               prefix = setup_git_directory();
> +       else if (run_setup & RUN_SETUP_GENTLY) {
> +               int nongit_ok;
> +               prefix = setup_git_directory_gently(&nongit_ok);
>         }
> +       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()) {
> --
> gitgitgadget

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> @@ -421,27 +421,28 @@ 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;

Don't lose the blank line that separates the variable declarations
and the first statement.  Also, it probably makes sense to remove
the assignment of NULL to prefix from here (see below).

>  	help = argc == 2 && !strcmp(argv[1], "-h");
> -...
> +	if (help && (run_setup & RUN_SETUP))
> +		/* demote to GENTLY to allow 'git cmd -h' outside repo */
> +		run_setup = RUN_SETUP_GENTLY;

OK.

> +	if (run_setup & RUN_SETUP)
> +		prefix = setup_git_directory();
> +	else if (run_setup & RUN_SETUP_GENTLY) {
> +		int nongit_ok;
> +		prefix = setup_git_directory_gently(&nongit_ok);
>  	}

Here, we say "depending on how run_setup is specified, we compute
the prefix in different ways".  So the "we do not run setup, so set
prefix to NULL" naturally belongs here.  Perhaps something like...

	if (run_setup & RUN_SETUP) {
		prefix = setup_git_directory();
	} else if (run_setup & RUN_SETUP_GENTLY) {
		int nongit_ok;
		prefix = setup_git_directory_gently(&nongit_ok);
	} else {
		prefix = NULL;
	}

> +	precompose_argv_prefix(argc, argv, NULL);
> +	if (use_pager == -1 && run_setup &&
> +		!(p->option & DELAY_PAGER_CONFIG))
> +		use_pager = check_pager_config(p->cmd);
> +	if (use_pager == -1 && p->option & USE_PAGER)
> +		use_pager = 1;
> +	if (run_setup && startup_info->have_repository)
> +		/* get_git_dir() may set up repo, avoid that */
> +		trace_repo_setup(prefix);
>  	commit_pager_choice();
>  
>  	if (!help && get_super_prefix()) {

struct stat st;
const char *prefix;
int run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));

prefix = NULL;
help = argc == 2 && !strcmp(argv[1], "-h");
if (!help) {
if (p->option & RUN_SETUP)
prefix = setup_git_directory();
else if (p->option & RUN_SETUP_GENTLY) {
int nongit_ok;
prefix = setup_git_directory_gently(&nongit_ok);
}
precompose_argv_prefix(argc, argv, NULL);
if (use_pager == -1 && p->option & (RUN_SETUP | RUN_SETUP_GENTLY) &&
!(p->option & DELAY_PAGER_CONFIG))
use_pager = check_pager_config(p->cmd);
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;

if ((p->option & (RUN_SETUP | RUN_SETUP_GENTLY)) &&
startup_info->have_repository) /* get_git_dir() may set up repo, avoid that */
trace_repo_setup(prefix);
if (help && (run_setup & RUN_SETUP))
/* demote to GENTLY to allow 'git cmd -h' outside repo */
run_setup = RUN_SETUP_GENTLY;

if (run_setup & RUN_SETUP) {
prefix = setup_git_directory();
} else if (run_setup & RUN_SETUP_GENTLY) {
int nongit_ok;
prefix = setup_git_directory_gently(&nongit_ok);
} else {
prefix = NULL;
}
precompose_argv_prefix(argc, argv, NULL);
if (use_pager == -1 && run_setup &&
!(p->option & DELAY_PAGER_CONFIG))
use_pager = check_pager_config(p->cmd);
if (use_pager == -1 && p->option & USE_PAGER)
use_pager = 1;
if (run_setup && startup_info->have_repository)
/* get_git_dir() may set up repo, avoid that */
trace_repo_setup(prefix);
commit_pager_choice();

if (!help && get_super_prefix()) {
Expand Down
3 changes: 3 additions & 0 deletions repo-settings.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ void prepare_repo_settings(struct repository *r)
char *strval;
int manyfiles;

if (!r->gitdir)
BUG("Cannot add settings for uninitialized repository");

if (r->settings.initialized++)
return;

Expand Down
5 changes: 3 additions & 2 deletions t/helper/test-read-cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ int cmd__read_cache(int argc, const char **argv)
int table = 0, expand = 0;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Lessley Dennington <lessleydennington@gmail.com>
>
> 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.

This looks obviously the right thing to do.  Would anything break
because of the "wrong" ordering of events in the original code?

IOW, can this "bugfix" be protected with a new test against
regression?

> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
> ---
>  t/helper/test-read-cache.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
> index b52c174acc7..0d9f08931a1 100644
> --- a/t/helper/test-read-cache.c
> +++ b/t/helper/test-read-cache.c
> @@ -39,8 +39,6 @@ int cmd__read_cache(int argc, const char **argv)
>  	int table = 0, expand = 0;
>  
>  	initialize_the_repository();
> -	prepare_repo_settings(r);
> -	r->settings.command_requires_full_index = 0;
>  
>  	for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) {
>  		if (skip_prefix(*argv, "--print-and-refresh=", &name))
> @@ -56,6 +54,9 @@ int cmd__read_cache(int argc, const char **argv)
>  	setup_git_directory();
>  	git_config(git_default_config, NULL);
>  
> +	prepare_repo_settings(r);
> +	r->settings.command_requires_full_index = 0;
> +
>  	for (i = 0; i < cnt; i++) {
>  		repo_read_index(r);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Lessley Dennington wrote (reply to this):



On 11/23/21 3:42 PM, Junio C Hamano wrote:
> "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
>> From: Lessley Dennington <lessleydennington@gmail.com>
>>
>> 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.
> 
> This looks obviously the right thing to do.  Would anything break
> because of the "wrong" ordering of events in the original code?
> 
> IOW, can this "bugfix" be protected with a new test against
> regression?
> 
Yep! Tests 2, 3, 28, and 34 in t1092-sparse-checkout-compatibility.sh
will fail without this change.
>> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
>> ---
>>   t/helper/test-read-cache.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/t/helper/test-read-cache.c b/t/helper/test-read-cache.c
>> index b52c174acc7..0d9f08931a1 100644
>> --- a/t/helper/test-read-cache.c
>> +++ b/t/helper/test-read-cache.c
>> @@ -39,8 +39,6 @@ int cmd__read_cache(int argc, const char **argv)
>>   	int table = 0, expand = 0;
>>   
>>   	initialize_the_repository();
>> -	prepare_repo_settings(r);
>> -	r->settings.command_requires_full_index = 0;
>>   
>>   	for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) {
>>   		if (skip_prefix(*argv, "--print-and-refresh=", &name))
>> @@ -56,6 +54,9 @@ int cmd__read_cache(int argc, const char **argv)
>>   	setup_git_directory();
>>   	git_config(git_default_config, NULL);
>>   
>> +	prepare_repo_settings(r);
>> +	r->settings.command_requires_full_index = 0;
>> +
>>   	for (i = 0; i < cnt; i++) {
>>   		repo_read_index(r);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

Lessley Dennington <lessleydennington@gmail.com> writes:

> On 11/23/21 3:42 PM, Junio C Hamano wrote:
>> "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>> 
>>> From: Lessley Dennington <lessleydennington@gmail.com>
>>>
>>> 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.

>> This looks obviously the right thing to do.  Would anything break
>> because of the "wrong" ordering of events in the original code?
>> IOW, can this "bugfix" be protected with a new test against
>> regression?
>> 
> Yep! Tests 2, 3, 28, and 34 in t1092-sparse-checkout-compatibility.sh
> will fail without this change.

I do not understand.  When 1/4 and 2/4 are applied, no tests in
t1092 fail for me.

I think the presentation order of this series is not reviewer
friendly; "the new BUG" is introduced in a separate step and
obscures the reason why this step is needed.  It is better than
adding "the new BUG" first and let some tests fail and then fix the
breakage the series caused in later steps, though.




Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Lessley Dennington wrote (reply to this):



On 11/24/21 10:36 AM, Junio C Hamano wrote:
> Lessley Dennington <lessleydennington@gmail.com> writes:
> 
>> On 11/23/21 3:42 PM, Junio C Hamano wrote:
>>> "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>
>>> writes:
>>>
>>>> From: Lessley Dennington <lessleydennington@gmail.com>
>>>>
>>>> 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.
> 
>>> This looks obviously the right thing to do.  Would anything break
>>> because of the "wrong" ordering of events in the original code?
>>> IOW, can this "bugfix" be protected with a new test against
>>> regression?
>>>
>> Yep! Tests 2, 3, 28, and 34 in t1092-sparse-checkout-compatibility.sh
>> will fail without this change.
> 
> I do not understand.  When 1/4 and 2/4 are applied, no tests in
> t1092 fail for me.
> 
> I think the presentation order of this series is not reviewer
> friendly; "the new BUG" is introduced in a separate step and
> obscures the reason why this step is needed.  It is better than
> adding "the new BUG" first and let some tests fail and then fix the
> breakage the series caused in later steps, though.
> 
> 
> 
> 
What I meant was that those tests fail with the first patch in the
series. Once this patch is applied they no longer fail.

That being said, I agree with this feedback - I have reorganized the
changes for v5 in a way in which I hope will make more sense and will
improve the reviewer experience.

Lessley


initialize_the_repository();
prepare_repo_settings(r);
r->settings.command_requires_full_index = 0;

for (++argv, --argc; *argv && starts_with(*argv, "--"); ++argv, --argc) {
if (skip_prefix(*argv, "--print-and-refresh=", &name))
Expand All @@ -56,6 +54,9 @@ int cmd__read_cache(int argc, const char **argv)
setup_git_directory();
git_config(git_default_config, NULL);

prepare_repo_settings(r);
r->settings.command_requires_full_index = 0;

for (i = 0; i < cnt; i++) {
repo_read_index(r);

Expand Down
4 changes: 4 additions & 0 deletions t/perf/p2000-sparse-operations.sh
Original file line number Diff line number Diff line change
Expand Up @@ -113,5 +113,9 @@ test_perf_on_all git checkout -f -
test_perf_on_all git reset
test_perf_on_all git reset --hard
test_perf_on_all git reset -- does-not-exist
test_perf_on_all git diff
test_perf_on_all git diff --cached
test_perf_on_all git blame $SPARSE_CONE/a
test_perf_on_all git blame $SPARSE_CONE/f3/a

test_done
109 changes: 91 additions & 18 deletions t/t1092-sparse-checkout-compatibility.sh
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ test_expect_success 'checkout and reset --hard' '
test_all_match git reset --hard update-folder2
'

test_expect_success 'diff --staged' '
test_expect_success 'diff --cached' '
init_repos &&

write_script edit-contents <<-\EOF &&
Expand All @@ -380,10 +380,10 @@ test_expect_success 'diff --staged' '
run_on_all ../edit-contents &&

test_all_match git diff &&
test_all_match git diff --staged &&
test_all_match git diff --cached &&
test_all_match git add README.md &&
test_all_match git diff &&
test_all_match git diff --staged
test_all_match git diff --cached
'

# NEEDSWORK: sparse-checkout behaves differently from full-checkout when
Expand All @@ -400,8 +400,8 @@ test_expect_success 'diff with renames and conflicts' '
test_all_match git checkout rename-base &&
test_all_match git checkout $branch -- . &&
test_all_match git status --porcelain=v2 &&
test_all_match git diff --staged --no-renames &&
test_all_match git diff --staged --find-renames || return 1
test_all_match git diff --cached --no-renames &&
test_all_match git diff --cached --find-renames || return 1
done
'

Expand All @@ -420,8 +420,8 @@ test_expect_success 'diff with directory/file conflicts' '
test_all_match git checkout $branch &&
test_all_match git checkout rename-base -- . &&
test_all_match git status --porcelain=v2 &&
test_all_match git diff --staged --no-renames &&
test_all_match git diff --staged --find-renames || return 1
test_all_match git diff --cached --no-renames &&
test_all_match git diff --cached --find-renames || return 1
done
'

Expand All @@ -442,21 +442,36 @@ test_expect_success 'log with pathspec outside sparse definition' '
test_expect_success 'blame with pathspec inside sparse definition' '
init_repos &&

test_all_match git blame a &&
test_all_match git blame deep/a &&
test_all_match git blame deep/deeper1/a &&
test_all_match git blame deep/deeper1/deepest/a
for file in a \
deep/a \
deep/deeper1/a \
deep/deeper1/deepest/a
do
test_all_match git blame $file
done
'

# TODO: blame currently does not support blaming files outside of the
# sparse definition. It complains that the file doesn't exist locally.
test_expect_failure 'blame with pathspec outside sparse definition' '
# Without a revision specified, blame will error if passed any file that
# is not present in the working directory (even if the file is tracked).
# Here we just verify that this is also true with sparse checkouts.
test_expect_success 'blame with pathspec outside sparse definition' '
ldennington marked this conversation as resolved.
Show resolved Hide resolved
init_repos &&
test_sparse_match git sparse-checkout set &&

test_all_match git blame folder1/a &&
test_all_match git blame folder2/a &&
test_all_match git blame deep/deeper2/a &&
test_all_match git blame deep/deeper2/deepest/a
for file in a \
deep/a \
deep/deeper1/a \
deep/deeper1/deepest/a
do
test_sparse_match test_must_fail git blame $file &&
cat >expect <<-EOF &&
fatal: Cannot lstat '"'"'$file'"'"': No such file or directory
EOF
# We compare sparse-checkout-err and sparse-index-err in
# `test_sparse_match`. Given we know they are the same, we
# only check the content of sparse-index-err here.
test_cmp expect sparse-index-err
done
'

test_expect_success 'checkout and reset (mixed)' '
Expand Down Expand Up @@ -846,6 +861,64 @@ test_expect_success 'sparse-index is not expanded: merge conflict in cone' '
)
'

test_expect_success 'sparse index is not expanded: diff' '
init_repos &&

write_script edit-contents <<-\EOF &&
echo text >>$1
EOF

# Add file within cone
test_sparse_match git sparse-checkout set deep &&
run_on_all ../edit-contents deep/testfile &&
test_all_match git add deep/testfile &&
run_on_all ../edit-contents deep/testfile &&

test_all_match git diff &&
test_all_match git diff --cached &&
ensure_not_expanded diff &&
ensure_not_expanded diff --cached &&

# Add file outside cone
test_all_match git reset --hard &&
run_on_all mkdir newdirectory &&
run_on_all ../edit-contents newdirectory/testfile &&
test_sparse_match git sparse-checkout set newdirectory &&
test_all_match git add newdirectory/testfile &&
run_on_all ../edit-contents newdirectory/testfile &&
test_sparse_match git sparse-checkout set &&

test_all_match git diff &&
test_all_match git diff --cached &&
ensure_not_expanded diff &&
ensure_not_expanded diff --cached &&

# Merge conflict outside cone
# The sparse checkout will report a warning that is not in the
# full checkout, so we use `run_on_all` instead of
# `test_all_match`
run_on_all git reset --hard &&
test_all_match git checkout merge-left &&
test_all_match test_must_fail git merge merge-right &&

test_all_match git diff &&
test_all_match git diff --cached &&
ensure_not_expanded diff &&
ensure_not_expanded diff --cached
'

test_expect_success 'sparse index is not expanded: blame' '
init_repos &&

for file in a \
deep/a \
deep/deeper1/a \
deep/deeper1/deepest/a
do
ensure_not_expanded blame $file
done
'

# NEEDSWORK: a sparse-checkout behaves differently from a full checkout
# in this scenario, but it shouldn't.
test_expect_success 'reset mixed and checkout orphan' '
Expand Down