]> Kevux Git Server - rit/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 12dcc0b..e461d29 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 49c042a..cd1931d 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 2a2cf91..e486493 100755 (executable)
@@ -5,6 +5,7 @@
 
 test_description='Move a binary file'
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 
index 697e86c..f788428 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 3de4ef6..27d6efd 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 ca018d1..d0863a8 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 b059475..62f0180 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 0f39ed0..cb1c4ce 100755 (executable)
@@ -2,6 +2,7 @@
 
 test_description="remember regular & dir renames in sequence of merges"
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 #