Re: [PATCH v2] write-tree: integrate with sparse index
- From
Victoria Dye <vdye@github.com>
- Date
- Apr 5, 2023, 17:31 UTC
- Message-ID
- <9d0309bd-943c-dd51-97cf-59721eda78f7@github.com>
- In-Reply-To
- <20230404003539.1578245-1-cheskaqiqi@gmail.com>
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