rm/sparse-index-integration-v1.1 #7

ffyuanda · 2022-08-02T11:55:27Z

1:  b5a0ac68dd ! 1:  a50c772d00 rm: integrate with sparse-index
    @@ Metadata
     Author: Shaoxuan Yuan <[email protected]>
     
      ## Commit message ##
    -    rm: integrate with sparse-index
    +    t1092: add tests for `git-rm`
     
    -    Turn off command_requires_full_index for `git-rm`.
    +    Add tests for `git-rm`, make sure it behaves as expected when
    +    <pathspec> is both inside or outside of sparse-checkout definition.
     
    -    Test `git-rm` when the target pathspec is in-cone and out-of-cone.
    -
    -    Ensure the sparse index is not expanded when operating inside of
    -    cone area.
    +    Also add ensure_not_expanded test to make sure `git-rm` does not
    +    accidentally expand the index when <pathspec> is within the
    +    sparse-checkout definition.
     
         Signed-off-by: Shaoxuan Yuan <[email protected]>
     
    - ## builtin/rm.c ##
    -@@ builtin/rm.c: int cmd_rm(int argc, const char **argv, const char *prefix)
    - 	char *seen;
    - 
    - 	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, prefix, builtin_rm_options,
    - 			     builtin_rm_usage, 0);
    -@@ builtin/rm.c: int cmd_rm(int argc, const char **argv, const char *prefix)
    - 
    - 	seen = xcalloc(pathspec.nr, 1);
    - 
    --	/* TODO: audit for interaction with sparse-index. */
    --	ensure_full_index(&the_index);
    - 	for (i = 0; i < active_nr; i++) {
    - 		const struct cache_entry *ce = active_cache[i];
    - 
    -
      ## t/t1092-sparse-checkout-compatibility.sh ##
     @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'mv directory from out-of-cone to in-cone' '
      	grep -e "H deep/0/1" actual
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'mv directory from
     +test_expect_success 'rm pathspec inside sparse definition' '
     +	init_repos &&
     +
    -+	for file in deep/a deep/deeper1/0/0/0 deep/deeper1/deepest/a
    -+	do
    -+		test_all_match git rm $file &&
    -+		test_all_match git status --porcelain=v2
    -+	done &&
    ++	test_all_match git rm deep/a &&
    ++	test_all_match git status --porcelain=v2 &&
     +
     +	# test wildcard
     +	run_on_all git reset --hard &&
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'mv directory from
     +test_expect_failure 'rm pathspec outside sparse definition' '
     +	init_repos &&
     +
    -+	for file in folder1/a folder1/0/1 folder1/0/0/0
    ++	for file in folder1/a folder1/0/1
     +	do
     +		test_sparse_match test_must_fail git rm $file &&
     +		test_sparse_match test_must_fail git rm --cached $file &&
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'mv directory from
     +		test_sparse_match git status --porcelain=v2
     +	done &&
     +
    ++	cat >folder1-full <<-EOF &&
    ++	rm ${SQ}folder1/0/0/0${SQ}
    ++	rm ${SQ}folder1/0/1${SQ}
    ++	rm ${SQ}folder1/a${SQ}
    ++	EOF
    ++
    ++	cat >folder1-sparse <<-EOF &&
    ++	rm ${SQ}folder1/${SQ}
    ++	EOF
    ++
     +	# test wildcard
     +	run_on_sparse git reset --hard &&
    ++	run_on_sparse git sparse-checkout reapply &&
     +	test_sparse_match test_must_fail git rm folder1/* &&
    -+	test_sparse_match git rm --sparse folder1/* &&
    ++	run_on_sparse git rm --sparse folder1/* &&
    ++	test_cmp folder1-full sparse-checkout-out &&
    ++	test_cmp folder1-sparse sparse-index-out &&
     +	test_sparse_match git status --porcelain=v2 &&
     +
     +	# test recursive rm
     +	run_on_sparse git reset --hard &&
    -+	test_sparse_match test_must_fail git rm folder1 &&
    -+	test_sparse_match git rm -r --sparse folder1 &&
    ++	run_on_sparse git sparse-checkout reapply &&
    ++	test_sparse_match test_must_fail git rm --sparse folder1 &&
    ++	run_on_sparse git rm --sparse -r folder1 &&
    ++	test_cmp folder1-full sparse-checkout-out &&
    ++	test_cmp folder1-sparse sparse-index-out &&
     +	test_sparse_match git status --porcelain=v2
     +'
     +
    -+test_expect_success 'sparse index is not expanded: rm' '
    ++test_expect_failure 'sparse index is not expanded: rm' '
     +	init_repos &&
     +
    -+	for file in deep/a deep/deeper1/a deep/deeper1/deepest/a
    -+	do
    -+		ensure_not_expanded rm $file
    -+	done &&
    ++	ensure_not_expanded rm deep/a &&
     +
    -+	# test in-cone wildcard not expand
    ++	# test in-cone wildcard
     +	git -C sparse-index reset --hard &&
     +	ensure_not_expanded rm deep/* &&
     +
    -+	# test recursive rm not expand
    ++	# test recursive rm
     +	git -C sparse-index reset --hard &&
     +	ensure_not_expanded rm -r deep
     +'
2:  a4be140bf0 ! 2:  f5534ddf92 pathspec.h: move pathspec_needs_expanded_index() from reset.c to here
    @@ Commit message
         the index needs to be expanded when the command is utilizing a pathspec
         rather than a literal path. Move it for reusability.
     
    +    Add a few items to the function so it can better serve its purpose as
    +    a standalone public function:
    +
    +    * Add a check in front so if the index is not sparse, return early since
    +      no expansion is needed.
    +
    +    * Add documentation to the function.
    +
         Signed-off-by: Shaoxuan Yuan <[email protected]>
     
      ## builtin/reset.c ##
    @@ pathspec.c: int match_pathspec_attrs(struct index_state *istate,
     +	char *skip_worktree_seen = NULL;
     +
     +	/*
    ++	 * If index is not sparse, no index expansion is needed.
    ++	 */
    ++	if (!istate->sparse_index)
    ++		return 0;
    ++
    ++	/*
     +	 * When using a magic pathspec, assume for the sake of simplicity that
     +	 * the index needs to be expanded to match all matchable files.
     +	 */
    @@ pathspec.h: int match_pathspec_attrs(struct index_state *istate,
      			 const char *name, int namelen,
      			 const struct pathspec_item *item);
      
    ++/*
    ++ * Determine whether a pathspec will match only entire index entries (non-sparse
    ++ * files and/or entire sparse directories). If the pathspec has the potential to
    ++ * match partial contents of a sparse directory, return 1 to indicate the index
    ++ * should be expanded to match the  appropriate index entries.
    ++ *
    ++ * For the sake of simplicity, always return 1 if using a more complex "magic"
    ++ * pathspec.
    ++ */
     +int pathspec_needs_expanded_index(struct index_state *istate,
     +				  const struct pathspec *pathspec);
     +
3:  dfc585f131 ! 3:  5921ac221e rm: expand the index only when necessary
    @@ Commit message
         environment, Git dies with "pathspec '<x>' did not match any files",
         mainly because it does not expand the index so nothing is matched.
     
    -    Expand the index when the pathspec needs an expanded index, i.e. the
    -    pathspec contains wildcard that may need a full-index or the pathspec
    -    is simply out-of-cone.
    +    Remove the `ensure_full_index()` method so `git-rm` does not always
    +    expand the index when the expansion is unnecessary, i.e. when
    +    <pathspec> does not have any possibilities to match anything outside
    +    of sparse-checkout definition.
    +
    +    Expand the index when the <pathspec> needs an expanded index, i.e. the
    +    <pathspec> contains wildcard that may need a full-index or the
    +    <pathspec> is simply outside of sparse-checkout definition.
     
         Signed-off-by: Shaoxuan Yuan <[email protected]>
     
    @@ builtin/rm.c: int cmd_rm(int argc, const char **argv, const char *prefix)
      
      	seen = xcalloc(pathspec.nr, 1);
      
    -+	if (the_index.sparse_index &&
    -+	    pathspec_needs_expanded_index(&the_index, &pathspec))
    +-	/* TODO: audit for interaction with sparse-index. */
    +-	ensure_full_index(&the_index);
    ++	if (pathspec_needs_expanded_index(&the_index, &pathspec))
     +		ensure_full_index(&the_index);
     +
      	for (i = 0; i < active_nr; i++) {
      		const struct cache_entry *ce = active_cache[i];
      
    -
    - ## t/t1092-sparse-checkout-compatibility.sh ##
    -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'rm pathspec inside sparse definition' '
    - 	test_all_match git status --porcelain=v2
    - '
    - 
    --test_expect_failure 'rm pathspec outside sparse definition' '
    -+test_expect_success 'rm pathspec outside sparse definition' '
    - 	init_repos &&
    - 
    - 	for file in folder1/a folder1/0/1 folder1/0/0/0
5:  7d6fa3062a = 4:  8d6fc0674c t1092: normalize a behavioral difference of `git-rm` under sparse-index
4:  b01a786a72 ! 5:  e6ec4ca73e t/perf/p2000: add perf test for git-rm
    @@ Metadata
     Author: Shaoxuan Yuan <[email protected]>
     
      ## Commit message ##
    -    t/perf/p2000: add perf test for git-rm
    +    rm: integrate with sparse-index
     
         The `p2000` tests demonstrate a ~96% execution time reduction for
         'git rm' using a sparse index.
    @@ Commit message
     
         Signed-off-by: Shaoxuan Yuan <[email protected]>
     
    + ## builtin/rm.c ##
    +@@ builtin/rm.c: int cmd_rm(int argc, const char **argv, const char *prefix)
    + 	char *seen;
    + 
    + 	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, prefix, builtin_rm_options,
    + 			     builtin_rm_usage, 0);
    +
      ## t/perf/p2000-sparse-operations.sh ##
     @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git blame $SPARSE_CONE/f3/a
      test_perf_on_all git read-tree -mu HEAD
    @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git blame $SPARSE_CONE/f3/a
     +test_perf_on_all git rm -f $SPARSE_CONE/a
      
      test_done
    +
    + ## t/t1092-sparse-checkout-compatibility.sh ##
    +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'rm pathspec inside sparse definition' '
    + 	test_all_match git status --porcelain=v2
    + '
    + 
    +-test_expect_failure 'rm pathspec outside sparse definition' '
    ++test_expect_success 'rm pathspec outside sparse definition' '
    + 	init_repos &&
    + 
    + 	for file in folder1/a folder1/0/1
    +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'rm pathspec outside sparse definition' '
    + 	test_sparse_match git status --porcelain=v2
    + '
    + 
    +-test_expect_failure 'sparse index is not expanded: rm' '
    ++test_expect_success 'sparse index is not expanded: rm' '
    + 	init_repos &&
    + 
    + 	ensure_not_expanded rm deep/a &&

Add tests for `git-rm`, make sure it behaves as expected when <pathspec> is both inside or outside of sparse-checkout definition. Also add ensure_not_expanded test to make sure `git-rm` does not accidentally expand the index when <pathspec> is within the sparse-checkout definition. Signed-off-by: Shaoxuan Yuan <[email protected]>

pathspec_needs_expanded_index() is reusable when we need to verify if the index needs to be expanded when the command is utilizing a pathspec rather than a literal path. Move it for reusability. Add a few items to the function so it can better serve its purpose as a standalone public function: * Add a check in front so if the index is not sparse, return early since no expansion is needed. * Add documentation to the function. Signed-off-by: Shaoxuan Yuan <[email protected]>

Originally, rm a pathspec that is out-of-cone in a sparse-index environment, Git dies with "pathspec '<x>' did not match any files", mainly because it does not expand the index so nothing is matched. Remove the `ensure_full_index()` method so `git-rm` does not always expand the index when the expansion is unnecessary, i.e. when <pathspec> does not have any possibilities to match anything outside of sparse-checkout definition. Expand the index when the <pathspec> needs an expanded index, i.e. the <pathspec> contains wildcard that may need a full-index or the <pathspec> is simply outside of sparse-checkout definition. Signed-off-by: Shaoxuan Yuan <[email protected]>

`git-rm` a sparse-directory entry within a sparse-index enabled repo behaves differently from a sparse directory within a sparse-checkout enabled repo. For example, in a sparse-index repo, where 'folder1' is a sparse-directory entry, `git rm -r --sparse folder1` provides this: rm 'folder1/' Whereas in a sparse-checkout repo *without* sparse-index, doing so provides this: rm 'folder1/0/0/0' rm 'folder1/0/1' rm 'folder1/a' Because `git rm` a sparse-directory entry does not need to expand the index, therefore we should accept the current behavior, which is faster than "expand the sparse-directory entry to match the sparse-checkout situation". Modify a previous test so such difference is not considered as an error. Signed-off-by: Shaoxuan Yuan <[email protected]>

The `p2000` tests demonstrate a ~96% execution time reduction for 'git rm' using a sparse index. Test before after ------------------------------------------------------------- 2000.74: git rm -f f2/f4/a (full-v3) 0.66 0.88 +33.0% 2000.75: git rm -f f2/f4/a (full-v4) 0.67 0.75 +12.0% 2000.76: git rm -f f2/f4/a (sparse-v3) 1.99 0.08 -96.0% 2000.77: git rm -f f2/f4/a (sparse-v4) 2.06 0.07 -96.6% Signed-off-by: Shaoxuan Yuan <[email protected]>

ffyuanda · 2022-08-02T11:57:07Z

Hi mentors, I'm not exactly sure how to do something like range-diff on GitHub...
Do you have any suggestions?

vdye

I think this is ready for the mailing list (a few nits, but nothing drastic). Sorry about the issue with git reset --hard - I'll try to find the fix, but using git sparse-checkout reapply is a good workaround for you to move forward.

builtin/reset.c

t/t1092-sparse-checkout-compatibility.sh

vdye · 2022-08-03T00:59:09Z

builtin/rm.c

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


You can avoid the need for if (the_repository->gitdir) - which is when the index is needed before parse_options() is called (link) - by putting the prepare_repo_settings() immediately before the index is accessed. In this case, I think that's right before the hold_locked_index() call (line 294).

I'm slightly suspicious here.

You can avoid the need for if (the_repository->gitdir) - which is when the index is needed before parse_options() is called (link)

In this commit they actually use if (the_repository->gitdir) as the way to go:

However, it is unfortunately not that simple. In cmd_pack_objects(),
for example, the repo settings need to be fully populated so that the
command-line options --sparse/--no-sparse can override them, not the
other way round.

Therefore, we choose to imitate the strategy taken in cmd_diff(),
where we simply do not bother to prepare and initialize the repo
settings unless we have a gitdir.

Though I'm following your advice right now because it does make sense to me ;-)

Since commit fcc07e9 (is_promisor_object(): free tree buffer after parsing, 2021-04-13), we'll always free the buffers attached to a "struct tree" after searching them for promisor links. But there's an important case where we don't want to do so: if somebody else is already using the tree! This can happen during a "rev-list --missing=allow-promisor" traversal in a partial clone that is missing one or more trees or blobs. The backtrace for the free looks like this: #1 free_tree_buffer tree.c:147 #2 add_promisor_object packfile.c:2250 #3 for_each_object_in_pack packfile.c:2190 #4 for_each_packed_object packfile.c:2215 #5 is_promisor_object packfile.c:2272 #6 finish_object__ma builtin/rev-list.c:245 #7 finish_object builtin/rev-list.c:261 #8 show_object builtin/rev-list.c:274 #9 process_blob list-objects.c:63 git#10 process_tree_contents list-objects.c:145 git#11 process_tree list-objects.c:201 git#12 traverse_trees_and_blobs list-objects.c:344 [...] We're in the middle of walking through the entries of a tree object via process_tree_contents(). We see a blob (or it could even be another tree entry) that we don't have, so we call is_promisor_object() to check it. That function loops over all of the objects in the promisor packfile, including the tree we're currently walking. When we're done with it there, we free the tree buffer. But as we return to the walk in process_tree_contents(), it's still holding on to a pointer to that buffer, via its tree_desc iterator, and it accesses the freed memory. Even a trivial use of "--missing=allow-promisor" triggers this problem, as the included test demonstrates (it's just a vanilla --blob:none clone). We can detect this case by only freeing the tree buffer if it was allocated on our behalf. This is a little tricky since that happens inside parse_object(), and it doesn't tell us whether the object was already parsed, or whether it allocated the buffer itself. But by checking for an already-parsed tree beforehand, we can distinguish the two cases. That feels a little hacky, and does incur an extra lookup in the object-hash table. But that cost is fairly minimal compared to actually loading objects (and since we're iterating the whole pack here, we're likely to be loading most objects, rather than reusing cached results). It may also be a good direction for this function in general, as there are other possible optimizations that rely on doing some analysis before parsing: - we could detect blobs and avoid reading their contents; they can't link to other objects, but parse_object() doesn't know that we don't care about checking their hashes. - we could avoid allocating object structs entirely for most objects (since we really only need them in the oidset), which would save some memory. - promisor commits could use the commit-graph rather than loading the object from disk This commit doesn't do any of those optimizations, but I think it argues that this direction is reasonable, rather than relying on parse_object() and trying to teach it to give us more information about whether it parsed. The included test fails reliably under SANITIZE=address just when running "rev-list --missing=allow-promisor". Checking the output isn't strictly necessary to detect the bug, but it seems like a reasonable addition given the general lack of coverage for "allow-promisor" in the test suite. Reported-by: Andrew Olsen <[email protected]> Signed-off-by: Jeff King <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>

Fix a memory leak occuring in case of pathspec copy in preload_index. Direct leak of 8 byte(s) in 8 object(s) allocated from: #0 0x7f0a353ead47 in __interceptor_malloc (/usr/lib/gcc/x86_64-pc-linux-gnu/11.3.0/libasan.so.6+0xb5d47) #1 0x55750995e840 in do_xmalloc /home/anthony/src/c/git/wrapper.c:51 #2 0x55750995e840 in xmalloc /home/anthony/src/c/git/wrapper.c:72 #3 0x55750970f824 in copy_pathspec /home/anthony/src/c/git/pathspec.c:684 #4 0x557509717278 in preload_index /home/anthony/src/c/git/preload-index.c:135 #5 0x55750975f21e in refresh_index /home/anthony/src/c/git/read-cache.c:1633 #6 0x55750915b926 in cmd_status builtin/commit.c:1547 #7 0x5575090e1680 in run_builtin /home/anthony/src/c/git/git.c:466 #8 0x5575090e1680 in handle_builtin /home/anthony/src/c/git/git.c:720 #9 0x5575090e284a in run_argv /home/anthony/src/c/git/git.c:787 git#10 0x5575090e284a in cmd_main /home/anthony/src/c/git/git.c:920 git#11 0x5575090dbf82 in main /home/anthony/src/c/git/common-main.c:56 git#12 0x7f0a348230ab (/lib64/libc.so.6+0x290ab) Signed-off-by: Anthony Delannoy <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>

ffyuanda added 5 commits August 2, 2022 19:24

vdye reviewed Aug 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rm/sparse-index-integration-v1.1 #7

rm/sparse-index-integration-v1.1 #7

ffyuanda commented Aug 2, 2022

ffyuanda commented Aug 2, 2022

vdye left a comment

vdye Aug 3, 2022

ffyuanda Aug 3, 2022

rm/sparse-index-integration-v1.1 #7

Are you sure you want to change the base?

rm/sparse-index-integration-v1.1 #7

Conversation

ffyuanda commented Aug 2, 2022

ffyuanda commented Aug 2, 2022

vdye left a comment

Choose a reason for hiding this comment

vdye Aug 3, 2022

Choose a reason for hiding this comment

ffyuanda Aug 3, 2022

Choose a reason for hiding this comment