Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

mv/in-to-out-v2 #8

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

mv/in-to-out-v2 #8

wants to merge 9 commits into from

Conversation

ffyuanda
Copy link
Owner

@ffyuanda ffyuanda commented Aug 4, 2022

No description provided.

ffyuanda added 9 commits August 4, 2022 20:14
@ffyuanda
t7002: add tests for moving from in-cone to out-of-cone
5de0a25 
Add corresponding tests to test that user can move an in-cone <source>
to out-of-cone <destination> when --sparse is supplied.

Such <source> can be either clean or dirty, and moving it results in
different behaviors:

A clean move should move the <source> to the <destination>, both in the
working tree and the index, then remove the resulted path from the
working tree, and turn on its CE_SKIP_WORKTREE bit.

A dirty move should move the <source> to the <destination>, both in the
working tree and the index, but should *not* remove the resulted path
from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.

Also make sure that if <destination> exists in the index (existing
check for if <destination> is in the worktree is not enough in
in-to-out moves), warn user against the overwrite. And Git should force
the overwrite when supplied with -f or --force.

Helped-by: Derrick Stolee <[email protected]>
Helped-by: Victoria Dye <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
@ffyuanda
mv: rename check_dir_in_index() to empty_dir_has_sparse_contents()
ebf8382 
Method check_dir_in_index() introduced in b91a2b6 (mv: add
check_dir_in_index() and solve general dir check issue, 2022-06-30)
does not describe its intent and behavior well.

Change its name to empty_dir_has_sparse_contents(), which more
precisely describes its purpose.

Reverse the return values, check_dir_in_index() return 0 for success
and 1 for failure; reverse the values so empty_dir_has_sparse_contents()
return 1 for success and 0 for failure. These values are more intuitive
because 1 usually means "has" and 0 means "not found".

Also modify the documentation to better align with the method's
intent and behavior.

Helped-by: Derrick Stolee <[email protected]>
Helped-by: Victoria Dye <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
@ffyuanda
mv: free the *with_slash in check_dir_in_index()
26871b1 
*with_slash may be a malloc'd pointer, and when it is, free it.

Helped-by: Derrick Stolee <[email protected]>
Helped-by: Victoria Dye <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
@ffyuanda
mv: check if <destination> is a SKIP_WORKTREE_DIR
872b3eb 
Originally, <destination> is assumed to be in the working tree. If it is
not found as a directory, then it is determined to be either a regular file
path, or error out if used under the second form (move into a directory)
of 'git-mv'. Such behavior is not ideal, mainly because Git does not
look into the index for <destination>, which could potentially be a
SKIP_WORKTREE_DIR, which we need to determine for the later "moving from
in-cone to out-of-cone" patch.

Change the logic so that Git first check if <destination> is a directory
with all its contents sparsified (a SKIP_WORKTREE_DIR).

If <destination> is such a sparse directory, then we should modify the
index the same way as we would if this were a non-sparse directory. We
must be careful to ensure that the <destination> is marked with
SKIP_WORKTREE_DIR.

Also add a `dst_w_slash` to reuse the result from `add_slash()`, which
was everywhere and can be simplified.

Helped-by: Derrick Stolee <[email protected]>
Helped-by: Victoria Dye <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
@ffyuanda
mv: remove BOTH from enum update_mode
df625cf 
Since BOTH is not used anywhere in the code and its meaning is unclear,
remove it.

Helped-by: Derrick Stolee <[email protected]>
Helped-by: Victoria Dye <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
@ffyuanda
mv: from in-cone to out-of-cone
e4a4742 
Originally, moving an in-cone <source> to an out-of-cone <destination>
was not possible, mainly because such <destination> is a directory that
is not present in the working tree.

Change the behavior so that we can move an in-cone <source> to
out-of-cone <destination> when --sparse is supplied.

Such <source> can be either clean or dirty, and moving it results in
different behaviors:

A clean move should move the <source> to the <destination>, both in the
working tree and the index, then remove the resulted path from the
working tree, and turn on its CE_SKIP_WORKTREE bit.

A dirty move should move the <source> to the <destination>, both in the
working tree and the index, but should *not* remove the resulted path
from the working tree and should *not* turn on its CE_SKIP_WORKTREE bit.

Helped-by: Derrick Stolee <[email protected]>
Helped-by: Victoria Dye <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
@ffyuanda
mv: cleanup empty WORKING_DIRECTORY
fec084a 
Originally, moving from-in-to-out may leave an empty <source>
directory on-disk (this kind of directory is marked as
WORKING_DIRECTORY).

Cleanup such directories if they are empty (don't have any entries
under them).

Modify two tests that take <source> as WORKING_DIRECTORY to test
this behavior.

Suggested-by: Derrick Stolee <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
@ffyuanda
advice.h: add advise_on_moving_dirty_path()
ac5345c 
Add an advice.

When the user use `git mv --sparse <dirty-path> <destination>`, Git
will warn the user to use `git add --sparse <paths>` then use
`git sparse-checkout reapply` to apply the sparsity rules.

Add a few lines to previous "move dirty path" tests so we can test
this new advice is working.

Suggested-by: Derrick Stolee <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
@ffyuanda
mv: check overwrite for in-to-out move
70d5a67 
Add checking logic for overwriting when moving from in-cone to
out-of-cone. It is the index version of the original overwrite logic.

Helped-by: Derrick Stolee <[email protected]>
Signed-off-by: Shaoxuan Yuan <[email protected]>
ffyuanda pushed a commit that referenced this pull request Aug 26, 2022
@peff @gitster
is_promisor_object(): fix use-after-free of tree buffer
1490d7d 
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]>
ffyuanda pushed a commit that referenced this pull request Sep 1, 2022
@gitster
preload-index: fix memleak
2357890 
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ffyuanda