mv/in-to-out-v1.3 #4
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ffyuanda
force-pushed
the
mv/in-to-out-v1.3
branch
3 times, most recently
from
1ed1b28 to
f339b7a
Compare
Recent update to vimdiff layout code has been made more robust against different end-user vim settings. * fr/vimdiff-layout-fix: vimdiff: make layout engine more robust against user vim settings
Apply Coccinelle rule to turn raw memmove() into MOVE_ARRAY() cpp macro, which would improve maintainability and readability. * jc/builtin-mv-move-array: builtin/mv.c: use the MOVE_ARRAY() macro instead of memmove()
Plug various memory leaks. * ab/leakfix: pull: fix a "struct oid_array" memory leak cat-file: fix a common "struct object_context" memory leak gc: fix a memory leak checkout: avoid "struct unpack_trees_options" leak merge-file: fix memory leaks on error path merge-file: refactor for subsequent memory leak fix cat-file: fix a memory leak in --batch-command mode revert: free "struct replay_opts" members submodule.c: free() memory from xgetcwd() clone: fix memory leak in wanted_peer_refs() check-ref-format: fix trivial memory leak
Plug various memory leaks in test-tool commands.
* ab/test-tool-leakfix:
test-tool delta: fix a memory leak
test-tool ref-store: fix a memory leak
test-tool bloom: fix memory leaks
test-tool json-writer: fix memory leaks
test-tool regex: call regfree(), fix memory leaks
test-tool urlmatch-normalization: fix a memory leak
test-tool {dump,scrap}-cache-tree: fix memory leaks
test-tool path-utils: fix a memory leak
test-tool test-hash: fix a memory leak
Teach "make all" to build gitweb as well.
* ab/build-gitweb:
gitweb/Makefile: add a "NO_GITWEB" parameter
Makefile: build 'gitweb' in the default target
gitweb/Makefile: include in top-level Makefile
gitweb: remove "test" and "test-installed" targets
gitweb/Makefile: prepare to merge into top-level Makefile
gitweb/Makefile: clear up and de-duplicate the gitweb.{css,js} vars
gitweb/Makefile: add a $(GITWEB_ALL) variable
gitweb/Makefile: define all .PHONY prerequisites inline
Tweak tests so that they still work when the "git init" template did not create .git/info directory. * ab/test-without-templates: tests: don't assume a .git/info for .git/info/sparse-checkout tests: don't assume a .git/info for .git/info/exclude tests: don't assume a .git/info for .git/info/refs tests: don't assume a .git/info for .git/info/attributes tests: don't assume a .git/info for .git/info/grafts tests: don't depend on template-created .git/branches t0008: don't rely on default ".git/info/exclude"
Fixes a long-standing corner case bug around directory renames in the merge-ort strategy. * en/merge-dual-dir-renames-fix: merge-ort: fix issue with dual rename and add/add conflict merge-ort: shuffle the computation and cleanup of potential collisions merge-ort: make a separate function for freeing struct collisions merge-ort: small cleanups of check_for_directory_rename t6423: add tests of dual directory rename plus add/add conflict
Another step to rewrite more parts of "git submodule" in C. * gc/submodule-use-super-prefix: submodule--helper: remove display path helper submodule--helper update: use --super-prefix submodule--helper: remove unused SUPPORT_SUPER_PREFIX flags submodule--helper: use correct display path helper submodule--helper: don't recreate recursive prefix submodule--helper update: use display path helper submodule--helper tests: add missing "display path" coverage
Add Coccinelle rules to detect the pattern of initializing and then
finalizing a structure without using it in between at all, which
happens after code restructuring and the compilers fail to
recognize as an unused variable.
* ab/cocci-unused:
cocci: generalize "unused" rule to cover more than "strbuf"
cocci: add and apply a rule to find "unused" strbufs
cocci: have "coccicheck{,-pending}" depend on "coccicheck-test"
cocci: add a "coccicheck-test" target and test *.cocci rules
Makefile & .gitignore: ignore & clean "git.res", not "*.res"
Makefile: remove mandatory "spatch" arguments from SPATCH_FLAGS
The code to convert between GPG trust level strings and internal constants we use to represent them have been cleaned up. * jd/gpg-interface-trust-level-string: gpg-interface: add function for converting trust level to string
Support for libnettle as SHA256 implementation has been added. * bc/nettle-sha256: sha256: add support for Nettle
The way "git multi-pack" uses parse-options API has been improved. * sg/multi-pack-index-parse-options-fix: multi-pack-index: simplify handling of unknown --options
Signed-off-by: Junio C Hamano <[email protected]>
ffyuanda
force-pushed
the
mv/in-to-out-v1.3
branch
from
f339b7a to
75881ac
Compare
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. Instead advise user to "git add" this path and run "git sparse-checkout reapply" to re-sparsify that path. 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. Signed-off-by: Shaoxuan Yuan <[email protected]>
Using check_dir_in_index without checking if the directory is on-disk could get a false positive for partially sparsified directory. Add a note in the documentation to warn potential user. Signed-off-by: Shaoxuan Yuan <[email protected]>
*with_slash may be a malloc'd pointer, and when it is, free it. Signed-off-by: Shaoxuan Yuan <[email protected]>
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 yes, then treat <destination> as a directory exists in the working tree, and thus using the second form of 'git-mv', i.e. move into this <destination>, and mark <destination> as a SKIP_WORKTREE_DIR. If no, continue the original checking logic. Also add a `dst_w_slash` to reuse the result from `add_slash()`, which was everywhere and can be simplified. Signed-off-by: Shaoxuan Yuan <[email protected]>
Since BOTH is not practically used anywhere in the code and its meaning is unclear, remove it. Signed-off-by: Shaoxuan Yuan <[email protected]>
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. Instead advise user to "git add" this path and run "git sparse-checkout reapply" to re-sparsify that path. Helped-by: Victoria Dye <[email protected]> Signed-off-by: Shaoxuan Yuan <[email protected]>
Add checking logic for overwritng when moving from in-cone to out-of-cone. It is the index version of the original overwrite logic. Signed-off-by: Shaoxuan Yuan <[email protected]>
ffyuanda
force-pushed
the
mv/in-to-out-v1.3
branch
from
75881ac to
7e60507
Compare
ffyuanda
pushed a commit
that referenced
this pull request
Aug 26, 2022
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
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.