threads / rfc / 59521

RFC patchwrite-tree: integrate with sparse index

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

## tl;dr

20 messages between Apr 2, 2023 and May 8, 2023. Diffs are folded; open one to read it.

replies: 19people: 3as markdown or json

Shuqi Liang· Apr 2, 2023, 00:01 UTC · lore

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(+)
Show changes to 3 files +33 −0

builtin/write-tree.c, t/perf/p2000-sparse-operations.sh, t/t1092-sparse-checkout-compatibility.sh

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
Junio C Hamano· Apr 3, 2023, 20:58 UTC · re: Shuqi Liang · lore

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

Shuqi Liang <cheskaqiqi@gmail.com> writes:
Show 24 quoted lines
> 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

Shuqi Liang· Apr 3, 2023, 22:16 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Apr 3, 2023, 22:54 UTC · re: Shuqi Liang · lore

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

Shuqi Liang <cheskaqiqi@gmail.com> writes:
Show 9 quoted lines
>> 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.

Shuqi Liang· Apr 4, 2023, 00:35 UTC · re: Shuqi Liang · lore

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

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(+)
Show changes to 3 files +32 −0

builtin/write-tree.c, t/perf/p2000-sparse-operations.sh, t/t1092-sparse-checkout-compatibility.sh

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
Victoria Dye· Apr 5, 2023, 17:31 UTC · re: Shuqi Liang · lore

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

Shuqi Liang wrote:
Show 22 quoted lines
> 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.

Show 67 quoted lines
> 
> 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).

Show 8 quoted lines
> +
> +	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'...
Show 6 quoted lines
> +
> +	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.

Show 10 quoted lines
> +'
> +
> +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
Junio C Hamano· Apr 5, 2023, 19:48 UTC · re: Victoria Dye · lore

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

Victoria Dye <vdye@github.com> writes:
Show 28 quoted lines
> 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.

Shuqi Liang· Apr 19, 2023, 07:21 UTC · re: Shuqi Liang · lore

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

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(+)
Show changes to 3 files +45 −0

builtin/write-tree.c, t/perf/p2000-sparse-operations.sh, t/t1092-sparse-checkout-compatibility.sh

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
Junio C Hamano· Apr 19, 2023, 15:47 UTC · re: Shuqi Liang · lore

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

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.
Shuqi Liang· Apr 20, 2023, 05:24 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Wed, Apr 19, 2023 at 11:47 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
>
> 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

Junio C Hamano· Apr 20, 2023, 15:55 UTC · re: Shuqi Liang · lore

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

Shuqi Liang <cheskaqiqi@gmail.com> writes:
Show 8 quoted lines
>> 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.

Shuqi Liang· Apr 21, 2023, 00:41 UTC · re: Shuqi Liang · lore

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

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(-)
Show changes to 2 files +21 −8

builtin/write-tree.c, t/t1092-sparse-checkout-compatibility.sh

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
Victoria Dye· Apr 21, 2023, 21:42 UTC · re: Shuqi Liang · lore

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

Shuqi Liang wrote:
Show 17 quoted lines
> 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.

Show 20 quoted lines
> 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'.

Show 38 quoted lines
> 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!
Show 9 quoted lines
>  '
>  
>  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
Junio C Hamano· Apr 24, 2023, 15:14 UTC · re: Victoria Dye · lore

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

Victoria Dye <vdye@github.com> writes:
Show 23 quoted lines
> 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.
Shuqi Liang· Apr 23, 2023, 07:12 UTC · re: Shuqi Liang · lore

[PATCH v5] 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 "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(-)
Show changes to 2 files +18 −6

builtin/write-tree.c, t/t1092-sparse-checkout-compatibility.sh

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
Junio C Hamano· Apr 24, 2023, 16:00 UTC · re: Shuqi Liang · lore

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

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.

Show 8 quoted lines
> '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.
Shuqi Liang· May 8, 2023, 20:05 UTC · re: Shuqi Liang · lore

[PATCH v6] write-tree: optimize sparse integration

* 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
Shuqi Liang· May 8, 2023, 20:21 UTC · re: Shuqi Liang · lore

[PATCH v6] write-tree: optimize sparse integration

* '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
Junio C Hamano· May 8, 2023, 21:09 UTC · re: Shuqi Liang · lore

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

Shuqi Liang <cheskaqiqi@gmail.com> writes:
Show 16 quoted lines
> * '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.
Shuqi Liang· May 8, 2023, 21:27 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Mon, May 8, 2023 at 5:09 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
>
> 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.

Show 5 quoted lines
> 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

← back to recent threads