]> git.ipfire.org Git - thirdparty/git.git/commitdiff
builtin/mv: refactor to use `struct strvec`
authorPatrick Steinhardt <ps@pks.im>
Mon, 27 May 2024 11:47:18 +0000 (13:47 +0200)
committerJunio C Hamano <gitster@pobox.com>
Mon, 27 May 2024 18:20:02 +0000 (11:20 -0700)
Memory allocation patterns in git-mv(1) are extremely hard to follow:
We copy around string pointers into manually-managed arrays, some of
which alias each other, but only sometimes, while we also drop some of
those strings at other times without ever daring to free them.

While this may be my own subjective feeling, it seems like others have
given up as the code has multiple calls to `UNLEAK()`. These are not
sufficient though, and git-mv(1) is still leaking all over the place
even with them.

Refactor the code to instead track strings in `struct strvec`. While
this has the effect of effectively duplicating some of the strings
without an actual need, it is way easier to reason about and fixes all
of the aliasing of memory that has been going on. It allows us to get
rid of the `UNLEAK()` calls and also fixes leaks that those calls did
not paper over.

Mark tests which are now leak-free accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/mv.c
t/t4001-diff-rename.sh
t/t4043-diff-rename-binary.sh
t/t4120-apply-popt.sh
t/t6400-merge-df.sh
t/t6412-merge-large-rename.sh
t/t6426-merge-skip-unneeded-updates.sh
t/t6429-merge-sequence-rename-caching.sh

index 12dcc0b13ce6743e3138517750e886756f3d4201..e461d29ca114e9873c48795007cc958efaa8303d 100644 (file)
@@ -20,6 +20,7 @@
 #include "read-cache-ll.h"
 #include "repository.h"
 #include "setup.h"
+#include "strvec.h"
 #include "submodule.h"
 #include "entry.h"
 
@@ -38,42 +39,32 @@ enum update_mode {
 #define DUP_BASENAME 1
 #define KEEP_TRAILING_SLASH 2
 
-static const char **internal_prefix_pathspec(const char *prefix,
-                                            const char **pathspec,
-                                            int count, unsigned flags)
+static void internal_prefix_pathspec(struct strvec *out,
+                                    const char *prefix,
+                                    const char **pathspec,
+                                    int count, unsigned flags)
 {
-       int i;
-       const char **result;
        int prefixlen = prefix ? strlen(prefix) : 0;
-       ALLOC_ARRAY(result, count + 1);
 
        /* Create an intermediate copy of the pathspec based on the flags */
-       for (i = 0; i < count; i++) {
-               int length = strlen(pathspec[i]);
-               int to_copy = length;
-               char *it;
+       for (int i = 0; i < count; i++) {
+               size_t length = strlen(pathspec[i]);
+               size_t to_copy = length;
+               const char *maybe_basename;
+               char *trimmed, *prefixed_path;
+
                while (!(flags & KEEP_TRAILING_SLASH) &&
                       to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
                        to_copy--;
 
-               it = xmemdupz(pathspec[i], to_copy);
-               if (flags & DUP_BASENAME) {
-                       result[i] = xstrdup(basename(it));
-                       free(it);
-               } else {
-                       result[i] = it;
-               }
-       }
-       result[count] = NULL;
+               trimmed = xmemdupz(pathspec[i], to_copy);
+               maybe_basename = (flags & DUP_BASENAME) ? basename(trimmed) : trimmed;
+               prefixed_path = prefix_path(prefix, prefixlen, maybe_basename);
+               strvec_push(out, prefixed_path);
 
-       /* Prefix the pathspec and free the old intermediate strings */
-       for (i = 0; i < count; i++) {
-               const char *match = prefix_path(prefix, prefixlen, result[i]);
-               free((char *) result[i]);
-               result[i] = match;
+               free(prefixed_path);
+               free(trimmed);
        }
-
-       return result;
 }
 
 static char *add_slash(const char *path)
@@ -176,7 +167,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
                OPT_BOOL(0, "sparse", &ignore_sparse, N_("allow updating entries outside of the sparse-checkout cone")),
                OPT_END(),
        };
-       const char **source, **destination, **dest_path, **submodule_gitfile;
+       struct strvec sources = STRVEC_INIT;
+       struct strvec dest_paths = STRVEC_INIT;
+       struct strvec destinations = STRVEC_INIT;
+       const char **submodule_gitfile;
        char *dst_w_slash = NULL;
        const char **src_dir = NULL;
        int src_dir_nr = 0, src_dir_alloc = 0;
@@ -201,7 +195,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
        if (repo_read_index(the_repository) < 0)
                die(_("index file corrupt"));
 
-       source = internal_prefix_pathspec(prefix, argv, argc, 0);
+       internal_prefix_pathspec(&sources, prefix, argv, argc, 0);
        CALLOC_ARRAY(modes, argc);
 
        /*
@@ -212,41 +206,39 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
        flags = KEEP_TRAILING_SLASH;
        if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
                flags = 0;
-       dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
-       dst_w_slash = add_slash(dest_path[0]);
+       internal_prefix_pathspec(&dest_paths, prefix, argv + argc, 1, flags);
+       dst_w_slash = add_slash(dest_paths.v[0]);
        submodule_gitfile = xcalloc(argc, sizeof(char *));
 
-       if (dest_path[0][0] == '\0')
+       if (dest_paths.v[0][0] == '\0')
                /* special case: "." was normalized to "" */
-               destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
-       else if (!lstat(dest_path[0], &st) &&
-                       S_ISDIR(st.st_mode)) {
-               destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
+               internal_prefix_pathspec(&destinations, dest_paths.v[0], argv, argc, DUP_BASENAME);
+       else if (!lstat(dest_paths.v[0], &st) && S_ISDIR(st.st_mode)) {
+               internal_prefix_pathspec(&destinations, dst_w_slash, argv, argc, DUP_BASENAME);
+       } else if (!path_in_sparse_checkout(dst_w_slash, the_repository->index) &&
+                  empty_dir_has_sparse_contents(dst_w_slash)) {
+               internal_prefix_pathspec(&destinations, dst_w_slash, argv, argc, DUP_BASENAME);
+               dst_mode = SKIP_WORKTREE_DIR;
+       } else if (argc != 1) {
+               die(_("destination '%s' is not a directory"), dest_paths.v[0]);
        } else {
-               if (!path_in_sparse_checkout(dst_w_slash, the_repository->index) &&
-                   empty_dir_has_sparse_contents(dst_w_slash)) {
-                       destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
-                       dst_mode = SKIP_WORKTREE_DIR;
-               } else if (argc != 1) {
-                       die(_("destination '%s' is not a directory"), dest_path[0]);
-               } else {
-                       destination = dest_path;
-                       /*
-                        * <destination> is a file outside of sparse-checkout
-                        * cone. Insist on cone mode here for backward
-                        * compatibility. We don't want dst_mode to be assigned
-                        * for a file when the repo is using no-cone mode (which
-                        * is deprecated at this point) sparse-checkout. As
-                        * SPARSE here is only considering cone-mode situation.
-                        */
-                       if (!path_in_cone_mode_sparse_checkout(destination[0], the_repository->index))
-                               dst_mode = SPARSE;
-               }
+               strvec_pushv(&destinations, dest_paths.v);
+
+               /*
+                * <destination> is a file outside of sparse-checkout
+                * cone. Insist on cone mode here for backward
+                * compatibility. We don't want dst_mode to be assigned
+                * for a file when the repo is using no-cone mode (which
+                * is deprecated at this point) sparse-checkout. As
+                * SPARSE here is only considering cone-mode situation.
+                */
+               if (!path_in_cone_mode_sparse_checkout(destinations.v[0], the_repository->index))
+                       dst_mode = SPARSE;
        }
 
        /* Checking */
        for (i = 0; i < argc; i++) {
-               const char *src = source[i], *dst = destination[i];
+               const char *src = sources.v[i], *dst = destinations.v[i];
                int length;
                const char *bad = NULL;
                int skip_sparse = 0;
@@ -330,8 +322,6 @@ dir_check:
                        src_dir[src_dir_nr++] = src;
 
                        n = argc + last - first;
-                       REALLOC_ARRAY(source, n);
-                       REALLOC_ARRAY(destination, n);
                        REALLOC_ARRAY(modes, n);
                        REALLOC_ARRAY(submodule_gitfile, n);
 
@@ -341,12 +331,16 @@ dir_check:
                        for (j = 0; j < last - first; j++) {
                                const struct cache_entry *ce = the_repository->index->cache[first + j];
                                const char *path = ce->name;
-                               source[argc + j] = path;
-                               destination[argc + j] =
-                                       prefix_path(dst_with_slash, dst_with_slash_len, path + length + 1);
+                               char *prefixed_path = prefix_path(dst_with_slash, dst_with_slash_len, path + length + 1);
+
+                               strvec_push(&sources, path);
+                               strvec_push(&destinations, prefixed_path);
+
                                memset(modes + argc + j, 0, sizeof(enum update_mode));
                                modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;
                                submodule_gitfile[argc + j] = NULL;
+
+                               free(prefixed_path);
                        }
 
                        free(dst_with_slash);
@@ -430,8 +424,8 @@ act_on_entry:
 remove_entry:
                if (--argc > 0) {
                        int n = argc - i;
-                       MOVE_ARRAY(source + i, source + i + 1, n);
-                       MOVE_ARRAY(destination + i, destination + i + 1, n);
+                       strvec_remove(&sources, i);
+                       strvec_remove(&destinations, i);
                        MOVE_ARRAY(modes + i, modes + i + 1, n);
                        MOVE_ARRAY(submodule_gitfile + i,
                                   submodule_gitfile + i + 1, n);
@@ -448,7 +442,7 @@ remove_entry:
        }
 
        for (i = 0; i < argc; i++) {
-               const char *src = source[i], *dst = destination[i];
+               const char *src = sources.v[i], *dst = destinations.v[i];
                enum update_mode mode = modes[i];
                int pos;
                int sparse_and_dirty = 0;
@@ -576,8 +570,9 @@ out:
        string_list_clear(&src_for_dst, 0);
        string_list_clear(&dirty_paths, 0);
        string_list_clear(&only_match_skip_worktree, 0);
-       UNLEAK(source);
-       UNLEAK(dest_path);
+       strvec_clear(&sources);
+       strvec_clear(&dest_paths);
+       strvec_clear(&destinations);
        free(submodule_gitfile);
        free(modes);
        return ret;
index 49c042a38ae987fa95f201bd3a00f55e4adeffab..cd1931dd555702f68078fc62a91887a374bd3572 100755 (executable)
@@ -3,9 +3,9 @@
 # Copyright (c) 2005 Junio C Hamano
 #
 
-test_description='Test rename detection in diff engine.
+test_description='Test rename detection in diff engine.'
 
-'
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-diff.sh
 
index 2a2cf91352037b6f2c238237474aa1d78928f5ad..e4864939081a8c7da1f4e9b6d8e0a3a72ba0a657 100755 (executable)
@@ -5,6 +5,7 @@
 
 test_description='Move a binary file'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
index 697e86c0ff456028948a769f2b6ccfa66bee2dc2..f7884285400f505082c2a92fe80854c9f2768730 100755 (executable)
@@ -5,6 +5,7 @@
 
 test_description='git apply -p handling.'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '
index 3de4ef6bd9e640d2f82deb111dd307e9a000db91..27d6efdc9a6d377a3eab526b6c4aac46319c33d5 100755 (executable)
@@ -7,6 +7,7 @@ test_description='Test merge with directory/file conflicts'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'prepare repository' '
index ca018d11f547978bf66a41804c16bf24a0b614f8..d0863a8fb5101615df4f8ff27db46a882c0a470e 100755 (executable)
@@ -4,6 +4,7 @@ test_description='merging with large rename matrix'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 count() {
index b059475ed033440bad42a059341b63176314e642..62f0180325352b627ea25fc71ec0ffdb4e8e5bb7 100755 (executable)
@@ -22,6 +22,7 @@ test_description="merge cases"
 #                     underscore notation is to differentiate different
 #                     files that might be renamed into each other's paths.)
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-merge.sh
 
index 0f39ed0d08a34230c7c42f307992c20cc8b06d02..cb1c4ceef763416011e68053adc26b0a9c0da1c3 100755 (executable)
@@ -2,6 +2,7 @@
 
 test_description="remember regular & dir renames in sequence of merges"
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 #