[RFC][PATCH v1] write-tree: integrate with sparse index

[RFC][PATCH v1] write-tree: integrate with sparse index

[Shuqi Liang]

Update 'git write-tree' to allow using the sparse-index in memory
without expanding to a full one.

The recursive algorithm for update_one() was already updated in 2de37c5
(cache-tree: integrate with sparse directory entries, 2021-03-03) to
handle sparse directory entries in the index. Hence we can just set the
requires-full-index to false for "write-tree".

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
write-tree' using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/write-tree.c                     |  4 ++++
 t/perf/p2000-sparse-operations.sh        |  1 +
 t/t1092-sparse-checkout-compatibility.sh | 28 ++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 45d61707e7..28c45b4301 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -35,6 +35,10 @@ int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
 	};
 
 	git_config(git_default_config, NULL);
+	
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+
 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
 			     write_tree_usage, 0);
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..9924adfc26 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,6 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git write-tree 
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..3b8191b390 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,32 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'write-tree on all' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+	run_on_all git update-index deep/a &&
+	test_all_match git write-tree &&
+
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+	run_on_all ../edit-contents folder1/a &&
+	run_on_all git update-index folder1/a &&
+	test_all_match git write-tree
+'
+
+test_expect_success 'sparse-index is not expanded: write-tree' '
+	init_repos &&
+
+	ensure_not_expanded write-tree &&
+
+	echo "test1" >>sparse-index/a &&
+	git -C sparse-index update-index a &&
+	ensure_not_expanded write-tree 
+'
+
 test_done
-- 
2.39.0

Re: [RFC][PATCH v1] write-tree: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> Update 'git write-tree' to allow using the sparse-index in memory
> without expanding to a full one.
>
> The recursive algorithm for update_one() was already updated in 2de37c5
> (cache-tree: integrate with sparse directory entries, 2021-03-03) to
> handle sparse directory entries in the index. Hence we can just set the
> requires-full-index to false for "write-tree".
>
> The `p2000` tests demonstrate a ~96% execution time reduction for 'git
> write-tree' using a sparse index:
>
> Test                                           before  after
> -----------------------------------------------------------------
> 2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
> 2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
> 2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
> 2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%
>
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>  builtin/write-tree.c                     |  4 ++++
>  t/perf/p2000-sparse-operations.sh        |  1 +
>  t/t1092-sparse-checkout-compatibility.sh | 28 ++++++++++++++++++++++++
>  3 files changed, 33 insertions(+)

Has the test suite been exercised with this patch?  It seems to
break at least t0012

Re: [RFC][PATCH v1] write-tree: integrate with sparse index

[Shuqi Liang]

On Mon, Apr 3, 2023 at 4:58 PM Junio C Hamano <gitster@pobox.com> wrote:

> Has the test suite been exercised with this patch?  It seems to
> break at least t0012
>

Hi Junio

I commented out the 'test_perf_on_all git grep --cached bogus --
"f2/f1/f1/*"' before
running 'p2000-sparse-operations.sh'.  I did this because I found that
with its presence,
even without adding any code, the tests wouldn't pass.  After commenting it out,
everything worked well. (In the patch I submitted above I did not
commented it out )

Thanks
Shuqi

Re: [RFC][PATCH v1] write-tree: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

>> Has the test suite been exercised with this patch?  It seems to
>> break at least t0012
>>
>
> Hi Junio
>
> I commented out the 'test_perf_on_all git grep --cached bogus --
> "f2/f1/f1/*"' before
> running 'p2000-sparse-operations.sh'.

Sorry, but I do not see why you are bringing up p2000 performance
measurement script here.

[PATCH v2] write-tree: integrate with sparse index

[Shuqi Liang]

Update 'git write-tree' to allow using the sparse-index in memory
without expanding to a full one.

The recursive algorithm for update_one() was already updated in 2de37c5
(cache-tree: integrate with sparse directory entries, 2021-03-03) to
handle sparse directory entries in the index. Hence we can just set the
requires-full-index to false for "write-tree".

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
write-tree' using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---

* change the position of "settings.command_requires_full_index = 0"

Range-diff against v1:
1:  d8a9ccd0b3 ! 1:  8873c79759 write-tree: integrate with sparse index
    @@ Commit message
     
      ## builtin/write-tree.c ##
     @@ builtin/write-tree.c: int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
    - 	};
    - 
    - 	git_config(git_default_config, NULL);
    -+	
    -+	prepare_repo_settings(the_repository);
    -+	the_repository->settings.command_requires_full_index = 0;
    -+
      	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
      			     write_tree_usage, 0);
      
    ++	prepare_repo_settings(the_repository);
    ++	the_repository->settings.command_requires_full_index = 0;
    ++	
    + 	ret = write_cache_as_tree(&oid, flags, tree_prefix);
    + 	switch (ret) {
    + 	case 0:
     
      ## t/perf/p2000-sparse-operations.sh ##
     @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all


 builtin/write-tree.c                     |  3 +++
 t/perf/p2000-sparse-operations.sh        |  1 +
 t/t1092-sparse-checkout-compatibility.sh | 28 ++++++++++++++++++++++++
 3 files changed, 32 insertions(+)

diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 45d61707e7..4492da0912 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -38,6 +38,9 @@ int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
 			     write_tree_usage, 0);
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+	
 	ret = write_cache_as_tree(&oid, flags, tree_prefix);
 	switch (ret) {
 	case 0:
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..9924adfc26 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,6 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git write-tree 
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..3b8191b390 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,32 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'write-tree on all' '
+	init_repos &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	run_on_all ../edit-contents deep/a &&
+	run_on_all git update-index deep/a &&
+	test_all_match git write-tree &&
+
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+	run_on_all ../edit-contents folder1/a &&
+	run_on_all git update-index folder1/a &&
+	test_all_match git write-tree
+'
+
+test_expect_success 'sparse-index is not expanded: write-tree' '
+	init_repos &&
+
+	ensure_not_expanded write-tree &&
+
+	echo "test1" >>sparse-index/a &&
+	git -C sparse-index update-index a &&
+	ensure_not_expanded write-tree 
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v2] write-tree: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> Update 'git write-tree' to allow using the sparse-index in memory
> without expanding to a full one.
> 
> The recursive algorithm for update_one() was already updated in 2de37c5
> (cache-tree: integrate with sparse directory entries, 2021-03-03) to
> handle sparse directory entries in the index. Hence we can just set the
> requires-full-index to false for "write-tree".
> 
> The `p2000` tests demonstrate a ~96% execution time reduction for 'git
> write-tree' using a sparse index:
> 
> Test                                           before  after
> -----------------------------------------------------------------
> 2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
> 2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
> 2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
> 2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%
> 
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
> 
> * change the position of "settings.command_requires_full_index = 0"

Could you describe why you made this change? You don't need to re-roll, but
in the future please make sure to describe the reasoning for changes like
this in these version notes if the context can't be gathered from other
discussions in the thread. 

> 
> Range-diff against v1:
> 1:  d8a9ccd0b3 ! 1:  8873c79759 write-tree: integrate with sparse index
>     @@ Commit message
>      
>       ## builtin/write-tree.c ##
>      @@ builtin/write-tree.c: int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
>     - 	};
>     - 
>     - 	git_config(git_default_config, NULL);
>     -+	
>     -+	prepare_repo_settings(the_repository);
>     -+	the_repository->settings.command_requires_full_index = 0;
>     -+
>       	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
>       			     write_tree_usage, 0);
>       
>     ++	prepare_repo_settings(the_repository);
>     ++	the_repository->settings.command_requires_full_index = 0;
>     ++	
>     + 	ret = write_cache_as_tree(&oid, flags, tree_prefix);
>     + 	switch (ret) {
>     + 	case 0:
>      
>       ## t/perf/p2000-sparse-operations.sh ##
>      @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
> 
> 
>  builtin/write-tree.c                     |  3 +++
>  t/perf/p2000-sparse-operations.sh        |  1 +
>  t/t1092-sparse-checkout-compatibility.sh | 28 ++++++++++++++++++++++++
>  3 files changed, 32 insertions(+)
> 
> diff --git a/builtin/write-tree.c b/builtin/write-tree.c
> index 45d61707e7..4492da0912 100644
> --- a/builtin/write-tree.c
> +++ b/builtin/write-tree.c
> @@ -38,6 +38,9 @@ int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
>  	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
>  			     write_tree_usage, 0);
>  
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;
> +	
>  	ret = write_cache_as_tree(&oid, flags, tree_prefix);
>  	switch (ret) {
>  	case 0:
> diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
> index 3242cfe91a..9924adfc26 100755
> --- a/t/perf/p2000-sparse-operations.sh
> +++ b/t/perf/p2000-sparse-operations.sh
> @@ -125,5 +125,6 @@ test_perf_on_all git checkout-index -f --all
>  test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
>  test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
>  test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
> +test_perf_on_all git write-tree 
>  
>  test_done
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e..3b8191b390 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,32 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_success 'write-tree on all' '

It's not clear what "on all" means in this context. If it's "write-tree with
changes both inside and outside the cone", then please either make that
explicit in the test name or simplify the name to just 'write-tree' (like
'clean').

> +	init_repos &&

It would be nice to have a baseline 'test_all_match git write-tree' before
making any changes to the index (as you do in the 'sparse-index is not
expanded: write-tree' test). 

> +
> +	write_script edit-contents <<-\EOF &&
> +	echo text >>"$1"
> +	EOF
> +
> +	run_on_all ../edit-contents deep/a &&
> +	run_on_all git update-index deep/a &&
> +	test_all_match git write-tree &&

First you make a change inside the sparse cone and 'write-tree'...

> +
> +	run_on_all mkdir -p folder1 &&
> +	run_on_all cp a folder1/a &&
> +	run_on_all ../edit-contents folder1/a &&
> +	run_on_all git update-index folder1/a &&
> +	test_all_match git write-tree

...then make a change outside the cone and 'write-tree' again. Makes sense.

However, there isn't any test of the working tree after 'write-tree' exits.
For example, I'd be interested in seeing a comparison of the output of 'git
status --porcelain=v2', as well as ensuring that SKIP_WORKTREE files weren't
materialized on disk in 'sparse-checkout' and 'sparse-index' (e.g.,
'folder2/a' shouldn't exist).

It also wouldn't hurt to 'test_all_match' on the 'git update-index' calls,
but I don't feel too strongly either way.

> +'
> +
> +test_expect_success 'sparse-index is not expanded: write-tree' '
> +	init_repos &&
> +
> +	ensure_not_expanded write-tree &&
> +
> +	echo "test1" >>sparse-index/a &&
> +	git -C sparse-index update-index a &&
> +	ensure_not_expanded write-tree 

This also looks good. 

> +'
> +
>  test_done

Re: [PATCH v2] write-tree: integrate with sparse index

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> Shuqi Liang wrote:
>> Update 'git write-tree' to allow using the sparse-index in memory
>> without expanding to a full one.
>> 
>> The recursive algorithm for update_one() was already updated in 2de37c5
>> (cache-tree: integrate with sparse directory entries, 2021-03-03) to
>> handle sparse directory entries in the index. Hence we can just set the
>> requires-full-index to false for "write-tree".
>> 
>> The `p2000` tests demonstrate a ~96% execution time reduction for 'git
>> write-tree' using a sparse index:
>> 
>> Test                                           before  after
>> -----------------------------------------------------------------
>> 2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
>> 2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
>> 2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
>> 2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%
>> 
>> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
>> ---
>> 
>> * change the position of "settings.command_requires_full_index = 0"
>
> Could you describe why you made this change? You don't need to re-roll, but
> in the future please make sure to describe the reasoning for changes like
> this in these version notes if the context can't be gathered from other
> discussions in the thread. 

The reason, I think, is because previous iteration hit a BUG() when
the command "git write-tree -h" is run outside a repository.  That
form of the help request is handled in the parse_options() machinery
without any need to have a repository or a working tree.

But prepare_repo_settings() does need to be run inside a repository,
so calling it without first checking if we are even in a repository
is asking for trouble.

I guess an alternative fix could have been to see if we are indeed
in a repository, by doing something like

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

like implementations of some subcommands do.  And being explicit
that way, instead of relying on an implicit safety given by ordering
of calls, would be more maintainable in the longer haul.

[PATCH v3] write-tree: integrate with sparse index

[Shuqi Liang]

Update 'git write-tree' to allow using the sparse-index in memory
without expanding to a full one.

The recursive algorithm for update_one() was already updated in 2de37c5
(cache-tree: integrate with sparse directory entries, 2021-03-03) to
handle sparse directory entries in the index. Hence we can just set the
requires-full-index to false for "write-tree".

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
write-tree' using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---

* Modified the code to ensure prepare_repo_settings() is called only 
when inside a repository.

* Change 'write-tree on all' to just 'write-tree'.

* Have a baseline 'test_all_match git write-tree' before making any 
changes to the index.

* Add  'git status --porcelain=v2'.

* Ensuring that SKIP_WORKTREE files weren't materialized on disk by
using "test_path_is_missing".

* Use 'test_all_match' on the 'git update-index'.



Range-diff against v2:
1:  8873c79759 ! 1:  cfa43c6cc7 write-tree: integrate with sparse index
    @@ Commit message
     
      ## builtin/write-tree.c ##
     @@ builtin/write-tree.c: int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
    - 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
    - 			     write_tree_usage, 0);
    + 	};
      
    + 	git_config(git_default_config, NULL);
    ++	
    ++	if (the_repository->gitdir) {
     +	prepare_repo_settings(the_repository);
     +	the_repository->settings.command_requires_full_index = 0;
    -+	
    - 	ret = write_cache_as_tree(&oid, flags, tree_prefix);
    - 	switch (ret) {
    - 	case 0:
    ++	}
    ++
    + 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
    + 			     write_tree_usage, 0);
    + 
     
      ## t/perf/p2000-sparse-operations.sh ##
     @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
      	test_cmp actual expect
      '
      
    -+test_expect_success 'write-tree on all' '
    ++test_expect_success 'write-tree' '
     +	init_repos &&
     +
    ++	test_all_match git write-tree &&
    ++
     +	write_script edit-contents <<-\EOF &&
     +	echo text >>"$1"
     +	EOF
     +
    ++	# make a change inside the sparse cone
     +	run_on_all ../edit-contents deep/a &&
    -+	run_on_all git update-index deep/a &&
    ++	test_all_match git update-index deep/a &&
     +	test_all_match git write-tree &&
    ++	test_all_match git status --porcelain=v2 &&
     +
    ++	# make a change outside the sparse cone
     +	run_on_all mkdir -p folder1 &&
     +	run_on_all cp a folder1/a &&
     +	run_on_all ../edit-contents folder1/a &&
    -+	run_on_all git update-index folder1/a &&
    -+	test_all_match git write-tree
    ++	test_all_match git update-index folder1/a &&
    ++	test_all_match git write-tree &&
    ++	test_all_match git status --porcelain=v2 &&
    ++	
    ++	# check that SKIP_WORKTREE files are not materialized
    ++	test_path_is_missing sparse-checkout/folder2/a &&
    ++	test_path_is_missing sparse-index/folder2/a
     +'
     +
     +test_expect_success 'sparse-index is not expanded: write-tree' '



 builtin/write-tree.c                     |  6 ++++
 t/perf/p2000-sparse-operations.sh        |  1 +
 t/t1092-sparse-checkout-compatibility.sh | 38 ++++++++++++++++++++++++
 3 files changed, 45 insertions(+)

diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 45d61707e7..23d63844de 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -35,6 +35,12 @@ int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
 	};
 
 	git_config(git_default_config, NULL);
+	
+	if (the_repository->gitdir) {
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+	}
+
 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
 			     write_tree_usage, 0);
 
diff --git a/t/perf/p2000-sparse-operations.sh b/t/perf/p2000-sparse-operations.sh
index 3242cfe91a..9924adfc26 100755
--- a/t/perf/p2000-sparse-operations.sh
+++ b/t/perf/p2000-sparse-operations.sh
@@ -125,5 +125,6 @@ test_perf_on_all git checkout-index -f --all
 test_perf_on_all git update-index --add --remove $SPARSE_CONE/a
 test_perf_on_all "git rm -f $SPARSE_CONE/a && git checkout HEAD -- $SPARSE_CONE/a"
 test_perf_on_all git grep --cached --sparse bogus -- "f2/f1/f1/*"
+test_perf_on_all git write-tree 
 
 test_done
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..d3eb31326b 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,42 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'write-tree' '
+	init_repos &&
+
+	test_all_match git write-tree &&
+
+	write_script edit-contents <<-\EOF &&
+	echo text >>"$1"
+	EOF
+
+	# make a change inside the sparse cone
+	run_on_all ../edit-contents deep/a &&
+	test_all_match git update-index deep/a &&
+	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
+
+	# make a change outside the sparse cone
+	run_on_all mkdir -p folder1 &&
+	run_on_all cp a folder1/a &&
+	run_on_all ../edit-contents folder1/a &&
+	test_all_match git update-index folder1/a &&
+	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
+	
+	# check that SKIP_WORKTREE files are not materialized
+	test_path_is_missing sparse-checkout/folder2/a &&
+	test_path_is_missing sparse-index/folder2/a
+'
+
+test_expect_success 'sparse-index is not expanded: write-tree' '
+	init_repos &&
+
+	ensure_not_expanded write-tree &&
+
+	echo "test1" >>sparse-index/a &&
+	git -C sparse-index update-index a &&
+	ensure_not_expanded write-tree 
+'
+
 test_done
-- 
2.39.0

Re: [PATCH v3] write-tree: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> Update 'git write-tree' to allow using the sparse-index in memory
> without expanding to a full one.

Sorry, but after this exchange

    https://lore.kernel.org/git/xmqqmt3bw9ir.fsf@gitster.g/

I am confused what we want to do with this version.

Re: [PATCH v3] write-tree: integrate with sparse index

[Shuqi Liang]

Hi Junio,

On Wed, Apr 19, 2023 at 11:47 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Shuqi Liang <cheskaqiqi@gmail.com> writes:
>
> > Update 'git write-tree' to allow using the sparse-index in memory
> > without expanding to a full one.
>
> Sorry, but after this exchange
>
>     https://lore.kernel.org/git/xmqqmt3bw9ir.fsf@gitster.g/
>
> I am confused what we want to do with this version.

Apologies for not noticing the patch was already merged to master.  I'll make
the necessary changes and submit a new patch soon.

Thanks
Shuqi

Re: [PATCH v3] write-tree: integrate with sparse index

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

>> Sorry, but after this exchange
>>
>>     https://lore.kernel.org/git/xmqqmt3bw9ir.fsf@gitster.g/
>>
>> I am confused what we want to do with this version.
>
> Apologies for not noticing the patch was already merged to master.  I'll make
> the necessary changes and submit a new patch soon.

No need to apologize.  I should have been able to guess what happend
myself.

Thanks for offering to make your updates incremental.  Will look
forward to seeing them.

[PATCH v4] write-tree: integrate with sparse index

[Shuqi Liang]

Update 'git write-tree' to allow using the sparse-index in memory
without expanding to a full one.

The recursive algorithm for update_one() was already updated in 2de37c5
(cache-tree: integrate with sparse directory entries, 2021-03-03) to
handle sparse directory entries in the index. Hence we can just set the
requires-full-index to false for "write-tree".

The `p2000` tests demonstrate a ~96% execution time reduction for 'git
write-tree' using a sparse index:

Test                                           before  after
-----------------------------------------------------------------
2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---

* Modified the code to ensure prepare_repo_settings() is called only 
when inside a repository.

* Change 'write-tree on all' to just 'write-tree'.

* Have a baseline 'test_all_match git write-tree' before making any 
changes to the index.

* Add 'git status --porcelain=v2'.

* Ensuring that SKIP_WORKTREE files weren't materialized on disk by
using "test_path_is_missing".

* Use 'test_all_match' on the 'git update-index'.


 builtin/write-tree.c                     |  9 ++++++---
 t/t1092-sparse-checkout-compatibility.sh | 20 +++++++++++++++-----
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 32e302a813..a9d5c20cde 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -38,12 +38,15 @@ int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
 	};
 
 	git_config(git_default_config, NULL);
+	
+	if (the_repository->gitdir) {
+		prepare_repo_settings(the_repository);
+		the_repository->settings.command_requires_full_index = 0;
+	}
+
 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
 			     write_tree_usage, 0);
 
-	prepare_repo_settings(the_repository);
-	the_repository->settings.command_requires_full_index = 0;
-
 	ret = write_index_as_tree(&oid, &the_index, get_index_file(), flags,
 				  tree_prefix);
 	switch (ret) {
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 9bbc0d646b..d3eb31326b 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,22 +2055,32 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
-test_expect_success 'write-tree on all' '
+test_expect_success 'write-tree' '
 	init_repos &&
 
+	test_all_match git write-tree &&
+
 	write_script edit-contents <<-\EOF &&
 	echo text >>"$1"
 	EOF
 
+	# make a change inside the sparse cone
 	run_on_all ../edit-contents deep/a &&
-	run_on_all git update-index deep/a &&
+	test_all_match git update-index deep/a &&
 	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
 
+	# make a change outside the sparse cone
 	run_on_all mkdir -p folder1 &&
 	run_on_all cp a folder1/a &&
 	run_on_all ../edit-contents folder1/a &&
-	run_on_all git update-index folder1/a &&
-	test_all_match git write-tree
+	test_all_match git update-index folder1/a &&
+	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
+	
+	# check that SKIP_WORKTREE files are not materialized
+	test_path_is_missing sparse-checkout/folder2/a &&
+	test_path_is_missing sparse-index/folder2/a
 '
 
 test_expect_success 'sparse-index is not expanded: write-tree' '
@@ -2080,7 +2090,7 @@ test_expect_success 'sparse-index is not expanded: write-tree' '
 
 	echo "test1" >>sparse-index/a &&
 	git -C sparse-index update-index a &&
-	ensure_not_expanded write-tree
+	ensure_not_expanded write-tree 
 '
 
 test_done
-- 
2.39.0

Re: [PATCH v4] write-tree: integrate with sparse index

[Victoria Dye]

Shuqi Liang wrote:
> Update 'git write-tree' to allow using the sparse-index in memory
> without expanding to a full one.
> 
> The recursive algorithm for update_one() was already updated in 2de37c5
> (cache-tree: integrate with sparse directory entries, 2021-03-03) to
> handle sparse directory entries in the index. Hence we can just set the
> requires-full-index to false for "write-tree".
> 
> The `p2000` tests demonstrate a ~96% execution time reduction for 'git
> write-tree' using a sparse index:
> 
> Test                                           before  after
> -----------------------------------------------------------------
> 2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
> 2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
> 2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
> 2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%

Please update your commit message to explain only the incremental updates on
top of 1a65b41b38a (write-tree: integrate with sparse index, 2023-04-03);
that patch's message (what you have here) does not accurately describe what
_this_ patch is doing.

> diff --git a/builtin/write-tree.c b/builtin/write-tree.c
> index 32e302a813..a9d5c20cde 100644
> --- a/builtin/write-tree.c
> +++ b/builtin/write-tree.c
> @@ -38,12 +38,15 @@ int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
>  	};
>  
>  	git_config(git_default_config, NULL);
> +	
> +	if (the_repository->gitdir) {
> +		prepare_repo_settings(the_repository);
> +		the_repository->settings.command_requires_full_index = 0;
> +	}
> +
>  	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
>  			     write_tree_usage, 0);
>  
> -	prepare_repo_settings(the_repository);
> -	the_repository->settings.command_requires_full_index = 0;
> -

What is the functional benefit of this change? AFAICT, we don't need
'command_requires_full_index' to be set before 'parse_options' in this case,
so this won't have any effect on the behavior of 'write-tree'.

> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 9bbc0d646b..d3eb31326b 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,22 +2055,32 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> -test_expect_success 'write-tree on all' '
> +test_expect_success 'write-tree' '
>  	init_repos &&
>  
> +	test_all_match git write-tree &&
> +
>  	write_script edit-contents <<-\EOF &&
>  	echo text >>"$1"
>  	EOF
>  
> +	# make a change inside the sparse cone
>  	run_on_all ../edit-contents deep/a &&
> -	run_on_all git update-index deep/a &&
> +	test_all_match git update-index deep/a &&
>  	test_all_match git write-tree &&
> +	test_all_match git status --porcelain=v2 &&
>  
> +	# make a change outside the sparse cone
>  	run_on_all mkdir -p folder1 &&
>  	run_on_all cp a folder1/a &&
>  	run_on_all ../edit-contents folder1/a &&
> -	run_on_all git update-index folder1/a &&
> -	test_all_match git write-tree
> +	test_all_match git update-index folder1/a &&
> +	test_all_match git write-tree &&
> +	test_all_match git status --porcelain=v2 &&
> +	
> +	# check that SKIP_WORKTREE files are not materialized
> +	test_path_is_missing sparse-checkout/folder2/a &&
> +	test_path_is_missing sparse-index/folder2/a

Test updates look good!

>  '
>  
>  test_expect_success 'sparse-index is not expanded: write-tree' '
> @@ -2080,7 +2090,7 @@ test_expect_success 'sparse-index is not expanded: write-tree' '
>  
>  	echo "test1" >>sparse-index/a &&
>  	git -C sparse-index update-index a &&
> -	ensure_not_expanded write-tree
> +	ensure_not_expanded write-tree 

nit: trailing whitespace should be removed

>  '
>  
>  test_done

[PATCH v5] write-tree: optimize sparse integration

[Shuqi Liang]

'prepare_repo_settings()' needs to be run inside a repository. Ensure
that the code checks for the presence of a repository before calling
this function. 'write-tree on all' had an unclear meaning of 'on all'.
Change the test name to simply 'write-tree'. Add a baseline
'test_all_match git write-tree' before making any changes to the index,
providing a reference point for the 'write-tree' prior to any
modifications. Add a comparison of the output of
'git status --porcelain=v2' to test the working tree after 'write-tree'
exits. Ensure SKIP_WORKTREE files weren't materialized on disk by using
"test_path_is_missing".

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---

* Update commit message.

* 'command_requires_full_index' to be set after 'parse_options'.

* Remove trailing whitespace.


Range-diff against v4:
1:  07f9bbd3c4 ! 1:  df470c2d61 write-tree: integrate with sparse index
    @@ Metadata
     Author: Shuqi Liang <cheskaqiqi@gmail.com>
     
      ## Commit message ##
    -    write-tree: integrate with sparse index
    +    write-tree: optimize sparse integration
     
    -    Update 'git write-tree' to allow using the sparse-index in memory
    -    without expanding to a full one.
    -
    -    The recursive algorithm for update_one() was already updated in 2de37c5
    -    (cache-tree: integrate with sparse directory entries, 2021-03-03) to
    -    handle sparse directory entries in the index. Hence we can just set the
    -    requires-full-index to false for "write-tree".
    -
    -    The `p2000` tests demonstrate a ~96% execution time reduction for 'git
    -    write-tree' using a sparse index:
    -
    -    Test                                           before  after
    -    -----------------------------------------------------------------
    -    2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
    -    2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
    -    2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
    -    2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%
    +    'prepare_repo_settings()' needs to be run inside a repository. Ensure
    +    that the code checks for the presence of a repository before calling
    +    this function. 'write-tree on all' had an unclear meaning of 'on all'.
    +    Change the test name to simply 'write-tree'. Add a baseline
    +    'test_all_match git write-tree' before making any changes to the index,
    +    providing a reference point for the 'write-tree' prior to any
    +    modifications. Add a comparison of the output of
    +    'git status --porcelain=v2' to test the working tree after 'write-tree'
    +    exits. Ensure SKIP_WORKTREE files weren't materialized on disk by using
    +    "test_path_is_missing".
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
      ## builtin/write-tree.c ##
     @@ builtin/write-tree.c: int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
    - 	};
    + 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
    + 			     write_tree_usage, 0);
      
    - 	git_config(git_default_config, NULL);
    -+	
    +-	prepare_repo_settings(the_repository);
    +-	the_repository->settings.command_requires_full_index = 0;
     +	if (the_repository->gitdir) {
     +		prepare_repo_settings(the_repository);
     +		the_repository->settings.command_requires_full_index = 0;
     +	}
    -+
    - 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
    - 			     write_tree_usage, 0);
      
    --	prepare_repo_settings(the_repository);
    --	the_repository->settings.command_requires_full_index = 0;
    --
      	ret = write_index_as_tree(&oid, &the_index, get_index_file(), flags,
      				  tree_prefix);
    - 	switch (ret) {
     
      ## t/t1092-sparse-checkout-compatibility.sh ##
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse directory within submodules' '
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
      '
      
      test_expect_success 'sparse-index is not expanded: write-tree' '
    -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'sparse-index is not expanded: write-tree' '
    - 
    - 	echo "test1" >>sparse-index/a &&
    - 	git -C sparse-index update-index a &&
    --	ensure_not_expanded write-tree
    -+	ensure_not_expanded write-tree 
    - '
    - 
    - test_done

 builtin/write-tree.c                     |  6 ++++--
 t/t1092-sparse-checkout-compatibility.sh | 18 ++++++++++++++----
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index 32e302a813..52caa096a8 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -41,8 +41,10 @@ int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
 			     write_tree_usage, 0);
 
-	prepare_repo_settings(the_repository);
-	the_repository->settings.command_requires_full_index = 0;
+	if (the_repository->gitdir) {
+		prepare_repo_settings(the_repository);
+		the_repository->settings.command_requires_full_index = 0;
+	}
 
 	ret = write_index_as_tree(&oid, &the_index, get_index_file(), flags,
 				  tree_prefix);
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c784813f1..2a467e4b31 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2080,22 +2080,32 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
-test_expect_success 'write-tree on all' '
+test_expect_success 'write-tree' '
 	init_repos &&
 
+	test_all_match git write-tree &&
+
 	write_script edit-contents <<-\EOF &&
 	echo text >>"$1"
 	EOF
 
+	# make a change inside the sparse cone
 	run_on_all ../edit-contents deep/a &&
-	run_on_all git update-index deep/a &&
+	test_all_match git update-index deep/a &&
 	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
 
+	# make a change outside the sparse cone
 	run_on_all mkdir -p folder1 &&
 	run_on_all cp a folder1/a &&
 	run_on_all ../edit-contents folder1/a &&
-	run_on_all git update-index folder1/a &&
-	test_all_match git write-tree
+	test_all_match git update-index folder1/a &&
+	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
+	
+	# check that SKIP_WORKTREE files are not materialized
+	test_path_is_missing sparse-checkout/folder2/a &&
+	test_path_is_missing sparse-index/folder2/a
 '
 
 test_expect_success 'sparse-index is not expanded: write-tree' '
-- 
2.39.0

Re: [PATCH v4] write-tree: integrate with sparse index

[Junio C Hamano]

Victoria Dye <vdye@github.com> writes:

> Shuqi Liang wrote:
>> Update 'git write-tree' to allow using the sparse-index in memory
>> without expanding to a full one.
>> 
>> The recursive algorithm for update_one() was already updated in 2de37c5
>> (cache-tree: integrate with sparse directory entries, 2021-03-03) to
>> handle sparse directory entries in the index. Hence we can just set the
>> requires-full-index to false for "write-tree".
>> 
>> The `p2000` tests demonstrate a ~96% execution time reduction for 'git
>> write-tree' using a sparse index:
>> 
>> Test                                           before  after
>> -----------------------------------------------------------------
>> 2000.78: git write-tree (full-v3)              0.34    0.33 -2.9%
>> 2000.79: git write-tree (full-v4)              0.32    0.30 -6.3%
>> 2000.80: git write-tree (sparse-v3)            0.47    0.02 -95.8%
>> 2000.81: git write-tree (sparse-v4)            0.45    0.02 -95.6%
>
> Please update your commit message to explain only the incremental updates on
> top of 1a65b41b38a (write-tree: integrate with sparse index, 2023-04-03);
> that patch's message (what you have here) does not accurately describe what
> _this_ patch is doing.

Good point.

In addition, as this is the first iteration of a follow-up topic,
"v4" on the subject line is a bit misleading.  Let's treat it as a
new and separate topic that build on top of the previous achievement.

Thanks for working on this topic and mentoring a new contributor.

Re: [PATCH v5] write-tree: optimize sparse integration

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> 'prepare_repo_settings()' needs to be run inside a repository. Ensure
> that the code checks for the presence of a repository before calling
> this function.

Can you explain why this change is needed?

That is, if the caller made sure if this codepath is entered only
when inside a repository, such a "we need to refrain from doing
this" becomes unnecessary.  Describe under what condition the
control passes this section with !the_repository->gitdir, e.g. "When
the command is run in such and such way outside a repository, the
control reaches this position but prepare_repo_settings() cannot be
blindly called".

I suspect that it is a bug if the control reaches this point without
having a repository, as the call to write_index_as_tree() would be
already failing if we were not in a repository, but there is no such
a bug, and you did not introduce one with your previous changes to
this codepath that you need to fix here.  You can observe a few
things:

 - "write-tree" in the git.c::commands[] table has RUN_SETUP.

 - git.c::run_builtin() is repsonsible for calling cmd_write_tree();
   what happens before it calls the function?  For a command with
   RUN_SETUP set, unless the command line argument is "-h" (that is,
   "git write-tree -h" is run), setup_git_directory() is called.

 - setup_git_directory() dies unless run in a repository.

 - git.c::run_builtin() calls setup_git_directory_gently() when the
   command line argument is "-h" and it does not die even run
   outside a repository.  However, before the code you touched,
   there is a call to parse_options().

 - parse_options() called for the command line argument "-h" shows a
   short help and then exits.

So...?

Also when starting to talk about totally different things (like, you
were discussing the change to write_tree.c to avoid segfaulting when
run outside a repository, but now you are going to talk about the
title of one test piece), please make sure it is clear for readers.
A blank line here may be appropriate.

> 'write-tree on all' had an unclear meaning of 'on all'.
> Change the test name to simply 'write-tree'. Add a baseline
> 'test_all_match git write-tree' before making any changes to the index,
> providing a reference point for the 'write-tree' prior to any
> modifications. Add a comparison of the output of
> 'git status --porcelain=v2' to test the working tree after 'write-tree'
> exits. Ensure SKIP_WORKTREE files weren't materialized on disk by using
> "test_path_is_missing".

All of the above may be easier to read in a bulletted list form,
e.g.

 * 'on all' in the title of the test 'write-tree on all' was
   unclear; remove it.

 * test the baseline test_all_match git write-tree" before doing
   anything else.

 ...



> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>
> * Update commit message.

OK.

> * 'command_requires_full_index' to be set after 'parse_options'.

This does not match what we see in this patch.

> * Remove trailing whitespace.

OK.  But there is a new line with a trailing whitespace before the
line that says # check that SKIP_WORKTREE files are not materialized"
in the test.

Thanks.

[PATCH v6] write-tree: optimize sparse integration

[Shuqi Liang]

* Remove 'on all' from the test title 'write-tree on all', making it
'write-tree'.

* Add a baseline 'test_all_match git write-tree' before making any
changes to the index, providing a reference point for the 'write-tree'
prior to any modifications.

* Add a comparison of the output of 'git status --porcelain=v2' to test
the working tree after 'write-tree' exits.

* Ensure SKIP_WORKTREE files weren't materialized on disk by using
"test_path_is_missing".

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---

change sine V5:

* We not need to check for the presence of a repository before calling
'prepare_repo_settings()', as the control flow should not reach this
point without a repository. This is because 'setup_git_directory()' is
called for commands with RUN_SETUP set, except when the command line
argument is "-h", in which case 'parse_options()' takes over and exits
the program.

* Change the commit message to make it easier to read.

* Remove whitespace before the line that says # check that SKIP_WORKTREE
files are not materialized".



Range-diff against v5:
1:  df470c2d61 ! 1:  0510b08c96 write-tree: optimize sparse integration
    @@ Metadata
      ## Commit message ##
         write-tree: optimize sparse integration
     
    -    'prepare_repo_settings()' needs to be run inside a repository. Ensure
    -    that the code checks for the presence of a repository before calling
    -    this function. 'write-tree on all' had an unclear meaning of 'on all'.
    -    Change the test name to simply 'write-tree'. Add a baseline
    -    'test_all_match git write-tree' before making any changes to the index,
    -    providing a reference point for the 'write-tree' prior to any
    -    modifications. Add a comparison of the output of
    -    'git status --porcelain=v2' to test the working tree after 'write-tree'
    -    exits. Ensure SKIP_WORKTREE files weren't materialized on disk by using
    +    * Remove 'on all' from the test title 'write-tree on all', making it
    +    'write-tree'.
    +
    +    * Add a baseline 'test_all_match git write-tree' before making any
    +    changes to the index, providing a reference point for the 'write-tree'
    +    prior to any modifications.
    +
    +    * Add a comparison of the output of 'git status --porcelain=v2' to test
    +    the working tree after 'write-tree' exits.
    +
    +    * Ensure SKIP_WORKTREE files weren't materialized on disk by using
         "test_path_is_missing".
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
    - ## builtin/write-tree.c ##
    -@@ builtin/write-tree.c: int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
    - 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
    - 			     write_tree_usage, 0);
    - 
    --	prepare_repo_settings(the_repository);
    --	the_repository->settings.command_requires_full_index = 0;
    -+	if (the_repository->gitdir) {
    -+		prepare_repo_settings(the_repository);
    -+		the_repository->settings.command_requires_full_index = 0;
    -+	}
    - 
    - 	ret = write_index_as_tree(&oid, &the_index, get_index_file(), flags,
    - 				  tree_prefix);
    -
      ## t/t1092-sparse-checkout-compatibility.sh ##
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse directory within submodules' '
      	test_cmp actual expect
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
     +	test_all_match git update-index folder1/a &&
     +	test_all_match git write-tree &&
     +	test_all_match git status --porcelain=v2 &&
    -+	
    ++
     +	# check that SKIP_WORKTREE files are not materialized
     +	test_path_is_missing sparse-checkout/folder2/a &&
     +	test_path_is_missing sparse-index/folder2/a
-- 

 t/t1092-sparse-checkout-compatibility.sh | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c784813f1..3aa6356a85 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2080,22 +2080,32 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
-test_expect_success 'write-tree on all' '
+test_expect_success 'write-tree' '
 	init_repos &&
 
+	test_all_match git write-tree &&
+
 	write_script edit-contents <<-\EOF &&
 	echo text >>"$1"
 	EOF
 
+	# make a change inside the sparse cone
 	run_on_all ../edit-contents deep/a &&
-	run_on_all git update-index deep/a &&
+	test_all_match git update-index deep/a &&
 	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
 
+	# make a change outside the sparse cone
 	run_on_all mkdir -p folder1 &&
 	run_on_all cp a folder1/a &&
 	run_on_all ../edit-contents folder1/a &&
-	run_on_all git update-index folder1/a &&
-	test_all_match git write-tree
+	test_all_match git update-index folder1/a &&
+	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
+
+	# check that SKIP_WORKTREE files are not materialized
+	test_path_is_missing sparse-checkout/folder2/a &&
+	test_path_is_missing sparse-index/folder2/a
 '
 
 test_expect_success 'sparse-index is not expanded: write-tree' '
-- 
2.39.0

[PATCH v6] write-tree: optimize sparse integration

[Shuqi Liang]

* 'on all' in the title of the test 'write-tree on all' was unclear;
remove it.

* Add a baseline 'test_all_match git write-tree' before making any
changes to the index, providing a reference point for the 'write-tree'
prior to any modifications.

* Add a comparison of the output of 'git status --porcelain=v2' to test
the working tree after 'write-tree' exits.

* Ensure SKIP_WORKTREE files weren't materialized on disk by using
"test_path_is_missing".

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---

My apologies, please ignore the previous v6 iteration.

change sine V5:

* We not need to check for the presence of a repository before calling
'prepare_repo_settings()', as the control flow should not reach this
point without a repository. This is because 'setup_git_directory()' is
called for commands with RUN_SETUP set, except when the command line
argument is "-h", in which case 'parse_options()' takes over and exits
the program.

* Change the commit message to make it easier to read.

* Remove whitespace before the line that says # check that SKIP_WORKTREE
files are not materialized".

Range-diff against v5:
1:  df470c2d61 ! 1:  e6c21ec6b8 write-tree: optimize sparse integration
    @@ Metadata
      ## Commit message ##
         write-tree: optimize sparse integration
     
    -    'prepare_repo_settings()' needs to be run inside a repository. Ensure
    -    that the code checks for the presence of a repository before calling
    -    this function. 'write-tree on all' had an unclear meaning of 'on all'.
    -    Change the test name to simply 'write-tree'. Add a baseline
    -    'test_all_match git write-tree' before making any changes to the index,
    -    providing a reference point for the 'write-tree' prior to any
    -    modifications. Add a comparison of the output of
    -    'git status --porcelain=v2' to test the working tree after 'write-tree'
    -    exits. Ensure SKIP_WORKTREE files weren't materialized on disk by using
    +    * 'on all' in the title of the test 'write-tree on all' was unclear;
    +    remove it.
    +
    +    * Add a baseline 'test_all_match git write-tree' before making any
    +    changes to the index, providing a reference point for the 'write-tree'
    +    prior to any modifications.
    +
    +    * Add a comparison of the output of 'git status --porcelain=v2' to test
    +    the working tree after 'write-tree' exits.
    +
    +    * Ensure SKIP_WORKTREE files weren't materialized on disk by using
         "test_path_is_missing".
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
    - ## builtin/write-tree.c ##
    -@@ builtin/write-tree.c: int cmd_write_tree(int argc, const char **argv, const char *cmd_prefix)
    - 	argc = parse_options(argc, argv, cmd_prefix, write_tree_options,
    - 			     write_tree_usage, 0);
    - 
    --	prepare_repo_settings(the_repository);
    --	the_repository->settings.command_requires_full_index = 0;
    -+	if (the_repository->gitdir) {
    -+		prepare_repo_settings(the_repository);
    -+		the_repository->settings.command_requires_full_index = 0;
    -+	}
    - 
    - 	ret = write_index_as_tree(&oid, &the_index, get_index_file(), flags,
    - 				  tree_prefix);
    -
      ## t/t1092-sparse-checkout-compatibility.sh ##
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse directory within submodules' '
      	test_cmp actual expect
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
     +	test_all_match git update-index folder1/a &&
     +	test_all_match git write-tree &&
     +	test_all_match git status --porcelain=v2 &&
    -+	
    ++
     +	# check that SKIP_WORKTREE files are not materialized
     +	test_path_is_missing sparse-checkout/folder2/a &&
     +	test_path_is_missing sparse-index/folder2/a
-- 


 t/t1092-sparse-checkout-compatibility.sh | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 0c784813f1..3aa6356a85 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2080,22 +2080,32 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
-test_expect_success 'write-tree on all' '
+test_expect_success 'write-tree' '
 	init_repos &&
 
+	test_all_match git write-tree &&
+
 	write_script edit-contents <<-\EOF &&
 	echo text >>"$1"
 	EOF
 
+	# make a change inside the sparse cone
 	run_on_all ../edit-contents deep/a &&
-	run_on_all git update-index deep/a &&
+	test_all_match git update-index deep/a &&
 	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
 
+	# make a change outside the sparse cone
 	run_on_all mkdir -p folder1 &&
 	run_on_all cp a folder1/a &&
 	run_on_all ../edit-contents folder1/a &&
-	run_on_all git update-index folder1/a &&
-	test_all_match git write-tree
+	test_all_match git update-index folder1/a &&
+	test_all_match git write-tree &&
+	test_all_match git status --porcelain=v2 &&
+
+	# check that SKIP_WORKTREE files are not materialized
+	test_path_is_missing sparse-checkout/folder2/a &&
+	test_path_is_missing sparse-index/folder2/a
 '
 
 test_expect_success 'sparse-index is not expanded: write-tree' '
-- 
2.39.0

Re: [PATCH v6] write-tree: optimize sparse integration

[Junio C Hamano]

Shuqi Liang <cheskaqiqi@gmail.com> writes:

> * 'on all' in the title of the test 'write-tree on all' was unclear;
> remove it.
>
> * Add a baseline 'test_all_match git write-tree' before making any
> changes to the index, providing a reference point for the 'write-tree'
> prior to any modifications.
>
> * Add a comparison of the output of 'git status --porcelain=v2' to test
> the working tree after 'write-tree' exits.
>
> * Ensure SKIP_WORKTREE files weren't materialized on disk by using
> "test_path_is_missing".
>
> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> ---
>

As we have lost the change to the code, the title has become stale.
How about I retitle it like so after queuing the patch?

    Subject: t1092: update write-tree test

The changes to the test seem to match what Victoria already gave a
thums-up in her review of v4; I see nothing surprising or unexpected
there.

Thanks.  Will queue.

Re: [PATCH v6] write-tree: optimize sparse integration

[Shuqi Liang]

Hi Junio,

On Mon, May 8, 2023 at 5:09 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Shuqi Liang <cheskaqiqi@gmail.com> writes:
>
> > * 'on all' in the title of the test 'write-tree on all' was unclear;
> > remove it.
> >
> > * Add a baseline 'test_all_match git write-tree' before making any
> > changes to the index, providing a reference point for the 'write-tree'
> > prior to any modifications.
> >
> > * Add a comparison of the output of 'git status --porcelain=v2' to test
> > the working tree after 'write-tree' exits.
> >
> > * Ensure SKIP_WORKTREE files weren't materialized on disk by using
> > "test_path_is_missing".
> >
> > Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
> > ---
> >
>
> As we have lost the change to the code, the title has become stale.
> How about I retitle it like so after queuing the patch?
>
>     Subject: t1092: update write-tree test

I think it's a good idea to retitle the patch， as it better reflects the
current changes in the test.

> The changes to the test seem to match what Victoria already gave a
> thums-up in her review of v4; I see nothing surprising or unexpected
> there.
>
> Thanks.  Will queue.

I really appreciate your and Victoria's continuous support and
guidance throughout
the review process :)

Thanks!
Shuqi