builtin/mv: refactor to use `struct strvec`
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>
Patrick Steinhardt committed
May 27, 2024 at 13:47 UTC
52a7dab439e53d05adb9edc870c92a446e5a9143
8 files changed
+68
-67
builtin/mv.c
+60
-65
@@ -20,6 +20,7 @@
20
#include "read-cache-ll.h"
21
#include "repository.h"
22
#include "setup.h"
23
+#include "strvec.h"
24
#include "submodule.h"
25
#include "entry.h"
26
@@ -38,42 +39,32 @@ enum update_mode {
39
#define DUP_BASENAME 1
40
#define KEEP_TRAILING_SLASH 2
41
41
-static const char **internal_prefix_pathspec(const char *prefix,
42
- const char **pathspec,
43
- int count, unsigned flags)
42
+static void internal_prefix_pathspec(struct strvec *out,
43
+ const char *prefix,
44
+ const char **pathspec,
45
+ int count, unsigned flags)
46
{
45
- int i;
46
- const char **result;
47
int prefixlen = prefix ? strlen(prefix) : 0;
48
- ALLOC_ARRAY(result, count + 1);
48
49
/* Create an intermediate copy of the pathspec based on the flags */
51
- for (i = 0; i < count; i++) {
52
- int length = strlen(pathspec[i]);
53
- int to_copy = length;
54
- char *it;
50
+ for (int i = 0; i < count; i++) {
51
+ size_t length = strlen(pathspec[i]);
52
+ size_t to_copy = length;
53
+ const char *maybe_basename;
54
+ char *trimmed, *prefixed_path;
55
+
56
while (!(flags & KEEP_TRAILING_SLASH) &&
57
to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
58
to_copy--;
59
59
- it = xmemdupz(pathspec[i], to_copy);
60
- if (flags & DUP_BASENAME) {
61
- result[i] = xstrdup(basename(it));
62
- free(it);
63
- } else {
64
- result[i] = it;
65
- }
66
- }
67
- result[count] = NULL;
60
+ trimmed = xmemdupz(pathspec[i], to_copy);
61
+ maybe_basename = (flags & DUP_BASENAME) ? basename(trimmed) : trimmed;
62
+ prefixed_path = prefix_path(prefix, prefixlen, maybe_basename);
63
+ strvec_push(out, prefixed_path);
64
69
- /* Prefix the pathspec and free the old intermediate strings */
70
- for (i = 0; i < count; i++) {
71
- const char *match = prefix_path(prefix, prefixlen, result[i]);
72
- free((char *) result[i]);
73
- result[i] = match;
65
+ free(prefixed_path);
66
+ free(trimmed);
67
}
75
-
76
- return result;
68
}
69
70
static char *add_slash(const char *path)
@@ -176,7 +167,10 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
167
OPT_BOOL(0, "sparse", &ignore_sparse, N_("allow updating entries outside of the sparse-checkout cone")),
168
OPT_END(),
169
};
179
- const char **source, **destination, **dest_path, **submodule_gitfile;
170
+ struct strvec sources = STRVEC_INIT;
171
+ struct strvec dest_paths = STRVEC_INIT;
172
+ struct strvec destinations = STRVEC_INIT;
173
+ const char **submodule_gitfile;
174
char *dst_w_slash = NULL;
175
const char **src_dir = NULL;
176
int src_dir_nr = 0, src_dir_alloc = 0;
@@ -201,7 +195,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
195
if (repo_read_index(the_repository) < 0)
196
die(_("index file corrupt"));
197
204
- source = internal_prefix_pathspec(prefix, argv, argc, 0);
198
+ internal_prefix_pathspec(&sources, prefix, argv, argc, 0);
199
CALLOC_ARRAY(modes, argc);
200
201
/*
@@ -212,41 +206,39 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
206
flags = KEEP_TRAILING_SLASH;
207
if (argc == 1 && is_directory(argv[0]) && !is_directory(argv[1]))
208
flags = 0;
215
- dest_path = internal_prefix_pathspec(prefix, argv + argc, 1, flags);
216
- dst_w_slash = add_slash(dest_path[0]);
209
+ internal_prefix_pathspec(&dest_paths, prefix, argv + argc, 1, flags);
210
+ dst_w_slash = add_slash(dest_paths.v[0]);
211
submodule_gitfile = xcalloc(argc, sizeof(char *));
212
219
- if (dest_path[0][0] == '\0')
213
+ if (dest_paths.v[0][0] == '\0')
214
/* special case: "." was normalized to "" */
221
- destination = internal_prefix_pathspec(dest_path[0], argv, argc, DUP_BASENAME);
222
- else if (!lstat(dest_path[0], &st) &&
223
- S_ISDIR(st.st_mode)) {
224
- destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
215
+ internal_prefix_pathspec(&destinations, dest_paths.v[0], argv, argc, DUP_BASENAME);
216
+ else if (!lstat(dest_paths.v[0], &st) && S_ISDIR(st.st_mode)) {
217
+ internal_prefix_pathspec(&destinations, dst_w_slash, argv, argc, DUP_BASENAME);
218
+ } else if (!path_in_sparse_checkout(dst_w_slash, the_repository->index) &&
219
+ empty_dir_has_sparse_contents(dst_w_slash)) {
220
+ internal_prefix_pathspec(&destinations, dst_w_slash, argv, argc, DUP_BASENAME);
221
+ dst_mode = SKIP_WORKTREE_DIR;
222
+ } else if (argc != 1) {
223
+ die(_("destination '%s' is not a directory"), dest_paths.v[0]);
224
} else {
226
- if (!path_in_sparse_checkout(dst_w_slash, the_repository->index) &&
227
- empty_dir_has_sparse_contents(dst_w_slash)) {
228
- destination = internal_prefix_pathspec(dst_w_slash, argv, argc, DUP_BASENAME);
229
- dst_mode = SKIP_WORKTREE_DIR;
230
- } else if (argc != 1) {
231
- die(_("destination '%s' is not a directory"), dest_path[0]);
232
- } else {
233
- destination = dest_path;
234
- /*
235
- * <destination> is a file outside of sparse-checkout
236
- * cone. Insist on cone mode here for backward
237
- * compatibility. We don't want dst_mode to be assigned
238
- * for a file when the repo is using no-cone mode (which
239
- * is deprecated at this point) sparse-checkout. As
240
- * SPARSE here is only considering cone-mode situation.
241
- */
242
- if (!path_in_cone_mode_sparse_checkout(destination[0], the_repository->index))
243
- dst_mode = SPARSE;
244
- }
225
+ strvec_pushv(&destinations, dest_paths.v);
226
+
227
+ /*
228
+ * <destination> is a file outside of sparse-checkout
229
+ * cone. Insist on cone mode here for backward
230
+ * compatibility. We don't want dst_mode to be assigned
231
+ * for a file when the repo is using no-cone mode (which
232
+ * is deprecated at this point) sparse-checkout. As
233
+ * SPARSE here is only considering cone-mode situation.
234
+ */
235
+ if (!path_in_cone_mode_sparse_checkout(destinations.v[0], the_repository->index))
236
+ dst_mode = SPARSE;
237
}
238
239
/* Checking */
240
for (i = 0; i < argc; i++) {
249
- const char *src = source[i], *dst = destination[i];
241
+ const char *src = sources.v[i], *dst = destinations.v[i];
242
int length;
243
const char *bad = NULL;
244
int skip_sparse = 0;
@@ -330,8 +322,6 @@ dir_check:
322
src_dir[src_dir_nr++] = src;
323
324
n = argc + last - first;
333
- REALLOC_ARRAY(source, n);
334
- REALLOC_ARRAY(destination, n);
325
REALLOC_ARRAY(modes, n);
326
REALLOC_ARRAY(submodule_gitfile, n);
327
@@ -341,12 +331,16 @@ dir_check:
331
for (j = 0; j < last - first; j++) {
332
const struct cache_entry *ce = the_repository->index->cache[first + j];
333
const char *path = ce->name;
344
- source[argc + j] = path;
345
- destination[argc + j] =
346
- prefix_path(dst_with_slash, dst_with_slash_len, path + length + 1);
334
+ char *prefixed_path = prefix_path(dst_with_slash, dst_with_slash_len, path + length + 1);
335
+
336
+ strvec_push(&sources, path);
337
+ strvec_push(&destinations, prefixed_path);
338
+
339
memset(modes + argc + j, 0, sizeof(enum update_mode));
340
modes[argc + j] |= ce_skip_worktree(ce) ? SPARSE : INDEX;
341
submodule_gitfile[argc + j] = NULL;
342
+
343
+ free(prefixed_path);
344
}
345
346
free(dst_with_slash);
@@ -430,8 +424,8 @@ act_on_entry:
424
remove_entry:
425
if (--argc > 0) {
426
int n = argc - i;
433
- MOVE_ARRAY(source + i, source + i + 1, n);
434
- MOVE_ARRAY(destination + i, destination + i + 1, n);
427
+ strvec_remove(&sources, i);
428
+ strvec_remove(&destinations, i);
429
MOVE_ARRAY(modes + i, modes + i + 1, n);
430
MOVE_ARRAY(submodule_gitfile + i,
431
submodule_gitfile + i + 1, n);
@@ -448,7 +442,7 @@ remove_entry:
442
}
443
444
for (i = 0; i < argc; i++) {
451
- const char *src = source[i], *dst = destination[i];
445
+ const char *src = sources.v[i], *dst = destinations.v[i];
446
enum update_mode mode = modes[i];
447
int pos;
448
int sparse_and_dirty = 0;
@@ -576,8 +570,9 @@ out:
570
string_list_clear(&src_for_dst, 0);
571
string_list_clear(&dirty_paths, 0);
572
string_list_clear(&only_match_skip_worktree, 0);
579
- UNLEAK(source);
580
- UNLEAK(dest_path);
573
+ strvec_clear(&sources);
574
+ strvec_clear(&dest_paths);
575
+ strvec_clear(&destinations);
576
free(submodule_gitfile);
577
free(modes);
578
return ret;
t/t4001-diff-rename.sh
+2
-2
@@ -3,9 +3,9 @@
3
# Copyright (c) 2005 Junio C Hamano
4
#
5
6
-test_description='Test rename detection in diff engine.
6
+test_description='Test rename detection in diff engine.'
7
8
-'
8
+TEST_PASSES_SANITIZE_LEAK=true
9
. ./test-lib.sh
10
. "$TEST_DIRECTORY"/lib-diff.sh
11
t/t4043-diff-rename-binary.sh
+1
@@ -5,6 +5,7 @@
5
6
test_description='Move a binary file'
7
8
+TEST_PASSES_SANITIZE_LEAK=true
9
. ./test-lib.sh
10
11
t/t4120-apply-popt.sh
+1
@@ -5,6 +5,7 @@
5
6
test_description='git apply -p handling.'
7
8
+TEST_PASSES_SANITIZE_LEAK=true
9
. ./test-lib.sh
10
11
test_expect_success setup '
t/t6400-merge-df.sh
+1
@@ -7,6 +7,7 @@ test_description='Test merge with directory/file conflicts'
7
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
8
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
9
10
+TEST_PASSES_SANITIZE_LEAK=true
11
. ./test-lib.sh
12
13
test_expect_success 'prepare repository' '
t/t6412-merge-large-rename.sh
+1
@@ -4,6 +4,7 @@ test_description='merging with large rename matrix'
4
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
5
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
6
7
+TEST_PASSES_SANITIZE_LEAK=true
8
. ./test-lib.sh
9
10
count() {
t/t6426-merge-skip-unneeded-updates.sh
+1
@@ -22,6 +22,7 @@ test_description="merge cases"
22
# underscore notation is to differentiate different
23
# files that might be renamed into each other's paths.)
24
25
+TEST_PASSES_SANITIZE_LEAK=true
26
. ./test-lib.sh
27
. "$TEST_DIRECTORY"/lib-merge.sh
28
t/t6429-merge-sequence-rename-caching.sh
+1
@@ -2,6 +2,7 @@
2
3
test_description="remember regular & dir renames in sequence of merges"
4
5
+TEST_PASSES_SANITIZE_LEAK=true
6
. ./test-lib.sh
7
8
#