@matheusbsbs / matheus-bagliano / commits / a5aecb2cdc

diff: improve lifecycle management of diff queues

The lifecycle management of diff queues is somewhat confusing: - For most of the part this can be attributed to `DIFF_QUEUE_CLEAR()`, which does not release any memory but rather initializes the queue, only. This is in contrast to our common naming schema, where "clearing" means that we release underlying memory and then re-initialize the data structure such that it is ready to use. - A second offender is `diff_free_queue()`, which does not free the queue structure itself. It is rather a release-style function. Refactor the code to make things less confusing. `DIFF_QUEUE_CLEAR()` is replaced by `DIFF_QUEUE_INIT` and `diff_queue_init()`, while `diff_free_queue()` is replaced by `diff_queue_release()`. While on it, adapt callsites where we call `DIFF_QUEUE_CLEAR()` with the intent to release underlying memory to instead call `diff_queue_clear()` to fix memory leaks. This memory leak is exposed by t4211, but plugging it alone does not make the whole test suite pass. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Sep 30, 2024 at 11:13 UTC a5aecb2cdc8c5f2c1501bdbe30c02959948d8442

10 files changed +32 -47

bloom.c

+1 -7

@@ -476,8 +476,6 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,

476 *last_slash = '\0';

477

478 } while (*path);

479 -

480 - diff_free_filepair(diff_queued_diff.queue[i]);

479 }

480

481 if (hashmap_get_size(&pathmap) > settings->max_changed_paths) {

@@ -508,8 +506,6 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,

506 cleanup:

507 hashmap_clear_and_free(&pathmap, struct pathmap_hash_entry, entry);

508 } else {

511 - for (i = 0; i < diff_queued_diff.nr; i++)

512 - diff_free_filepair(diff_queued_diff.queue[i]);

509 init_truncated_large_filter(filter, settings->hash_version);

510

511 if (computed)

@@ -519,9 +515,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,

515 if (computed)

516 *computed |= BLOOM_COMPUTED;

517

522 - free(diff_queued_diff.queue);

523 - DIFF_QUEUE_CLEAR(&diff_queued_diff);

524 -

518 + diff_queue_clear(&diff_queued_diff);

519 return filter;

520 }

521

diff.c

+12 -10

@@ -5983,11 +5983,18 @@ void diff_free_filepair(struct diff_filepair *p)

5983 free(p);

5984 }

5985

5986 -void diff_free_queue(struct diff_queue_struct *q)

5986 +void diff_queue_init(struct diff_queue_struct *q)

5987 +{

5988 + struct diff_queue_struct blank = DIFF_QUEUE_INIT;

5989 + memcpy(q, &blank, sizeof(*q));

5990 +}

5991 +

5992 +void diff_queue_clear(struct diff_queue_struct *q)

5993 {

5994 for (int i = 0; i < q->nr; i++)

5995 diff_free_filepair(q->queue[i]);

5996 free(q->queue);

5997 + diff_queue_init(q);

5998 }

5999

6000 const char *diff_aligned_abbrev(const struct object_id *oid, int len)

@@ -6551,8 +6558,7 @@ int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int

6558 struct diff_queue_struct *q = &diff_queued_diff;

6559 int result = diff_get_patch_id(options, oid, diff_header_only);

6560

6554 - diff_free_queue(q);

6555 - DIFF_QUEUE_CLEAR(q);

6561 + diff_queue_clear(q);

6562

6563 return result;

6564 }

@@ -6835,8 +6841,7 @@ void diff_flush(struct diff_options *options)

6841 }

6842

6843 free_queue:

6838 - diff_free_queue(q);

6839 - DIFF_QUEUE_CLEAR(q);

6844 + diff_queue_clear(q);

6845 diff_free(options);

6846

6847 /*

@@ -6867,9 +6872,7 @@ static void diffcore_apply_filter(struct diff_options *options)

6872 {

6873 int i;

6874 struct diff_queue_struct *q = &diff_queued_diff;

6870 - struct diff_queue_struct outq;

6871 -

6872 - DIFF_QUEUE_CLEAR(&outq);

6875 + struct diff_queue_struct outq = DIFF_QUEUE_INIT;

6876

6877 if (!options->filter)

6878 return;

@@ -6962,8 +6965,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)

6965 {

6966 int i;

6967 struct diff_queue_struct *q = &diff_queued_diff;

6965 - struct diff_queue_struct outq;

6966 - DIFF_QUEUE_CLEAR(&outq);

6968 + struct diff_queue_struct outq = DIFF_QUEUE_INIT;

6969

6970 for (i = 0; i < q->nr; i++) {

6971 struct diff_filepair *p = q->queue[i];

diffcore-break.c

+2 -6

@@ -131,7 +131,7 @@ static int should_break(struct repository *r,

131 void diffcore_break(struct repository *r, int break_score)

132 {

133 struct diff_queue_struct *q = &diff_queued_diff;

134 - struct diff_queue_struct outq;

134 + struct diff_queue_struct outq = DIFF_QUEUE_INIT;

135

136 /* When the filepair has this much edit (insert and delete),

137 * it is first considered to be a rewrite and broken into a

@@ -178,8 +178,6 @@ void diffcore_break(struct repository *r, int break_score)

178 if (!merge_score)

179 merge_score = DEFAULT_MERGE_SCORE;

180

181 - DIFF_QUEUE_CLEAR(&outq);

182 -

181 for (i = 0; i < q->nr; i++) {

182 struct diff_filepair *p = q->queue[i];

183 int score;

@@ -275,11 +273,9 @@ static void merge_broken(struct diff_filepair *p,

273 void diffcore_merge_broken(void)

274 {

275 struct diff_queue_struct *q = &diff_queued_diff;

278 - struct diff_queue_struct outq;

276 + struct diff_queue_struct outq = DIFF_QUEUE_INIT;

277 int i, j;

278

281 - DIFF_QUEUE_CLEAR(&outq);

282 -

279 for (i = 0; i < q->nr; i++) {

280 struct diff_filepair *p = q->queue[i];

281 if (!p)

diffcore-pickaxe.c

+1 -3

@@ -182,9 +182,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,

182 regex_t *regexp, kwset_t kws, pickaxe_fn fn)

183 {

184 int i;

185 - struct diff_queue_struct outq;

186 -

187 - DIFF_QUEUE_CLEAR(&outq);

185 + struct diff_queue_struct outq = DIFF_QUEUE_INIT;

186

187 if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {

188 /* Showing the whole changeset if needle exists */

diffcore-rename.c

+1 -2

@@ -1388,7 +1388,7 @@ void diffcore_rename_extended(struct diff_options *options,

1388 int detect_rename = options->detect_rename;

1389 int minimum_score = options->rename_score;

1390 struct diff_queue_struct *q = &diff_queued_diff;

1391 - struct diff_queue_struct outq;

1391 + struct diff_queue_struct outq = DIFF_QUEUE_INIT;

1392 struct diff_score *mx;

1393 int i, j, rename_count, skip_unmodified = 0;

1394 int num_destinations, dst_cnt;

@@ -1638,7 +1638,6 @@ void diffcore_rename_extended(struct diff_options *options,

1638 * are recorded in rename_dst. The original list is still in *q.

1639 */

1640 trace2_region_enter("diff", "write back to queue", options->repo);

1641 - DIFF_QUEUE_CLEAR(&outq);

1641 for (i = 0; i < q->nr; i++) {

1642 struct diff_filepair *p = q->queue[i];

1643 struct diff_filepair *pair_to_free = NULL;

diffcore-rotate.c

+1 -2

@@ -10,7 +10,7 @@

10 void diffcore_rotate(struct diff_options *opt)

11 {

12 struct diff_queue_struct *q = &diff_queued_diff;

13 - struct diff_queue_struct outq;

13 + struct diff_queue_struct outq = DIFF_QUEUE_INIT;

14 int rotate_to, i;

16 if (!q->nr)

@@ -31,7 +31,6 @@ void diffcore_rotate(struct diff_options *opt)

31 return;

32 }

34 - DIFF_QUEUE_CLEAR(&outq);

34 rotate_to = i;

36 for (i = rotate_to; i < q->nr; i++)

diffcore.h

+4 -6

@@ -153,18 +153,16 @@ struct diff_queue_struct {

153 int nr;

154 };

155

156 -#define DIFF_QUEUE_CLEAR(q) \

157 - do { \

158 - (q)->queue = NULL; \

159 - (q)->nr = (q)->alloc = 0; \

160 - } while (0)

156 +#define DIFF_QUEUE_INIT { 0 }

157 +

158 +void diff_queue_init(struct diff_queue_struct *q);

159 +void diff_queue_clear(struct diff_queue_struct *q);

160

161 extern struct diff_queue_struct diff_queued_diff;

162 struct diff_filepair *diff_queue(struct diff_queue_struct *,

163 struct diff_filespec *,

164 struct diff_filespec *);

165 void diff_q(struct diff_queue_struct *, struct diff_filepair *);

167 -void diff_free_queue(struct diff_queue_struct *q);

166

167 /* dir_rename_relevance: the reason we want rename information for a dir */

168 enum dir_rename_relevance {

line-log.c

+7 -8

@@ -787,15 +787,14 @@ static void move_diff_queue(struct diff_queue_struct *dst,

787 struct diff_queue_struct *src)

788 {

789 assert(src != dst);

790 - memcpy(dst, src, sizeof(struct diff_queue_struct));

791 - DIFF_QUEUE_CLEAR(src);

790 + memcpy(dst, src, sizeof(*dst));

791 + diff_queue_init(src);

792 }

793

794 static void filter_diffs_for_paths(struct line_log_data *range, int keep_deletions)

795 {

796 int i;

797 - struct diff_queue_struct outq;

798 - DIFF_QUEUE_CLEAR(&outq);

797 + struct diff_queue_struct outq = DIFF_QUEUE_INIT;

798

799 for (i = 0; i < diff_queued_diff.nr; i++) {

800 struct diff_filepair *p = diff_queued_diff.queue[i];

@@ -850,12 +849,12 @@ static void queue_diffs(struct line_log_data *range,

849 clear_pathspec(&opt->pathspec);

850 parse_pathspec_from_ranges(&opt->pathspec, range);

851 }

853 - DIFF_QUEUE_CLEAR(&diff_queued_diff);

852 + diff_queue_clear(&diff_queued_diff);

853 diff_tree_oid(parent_tree_oid, tree_oid, "", opt);

854 if (opt->detect_rename && diff_might_be_rename()) {

855 /* must look at the full tree diff to detect renames */

856 clear_pathspec(&opt->pathspec);

858 - DIFF_QUEUE_CLEAR(&diff_queued_diff);

857 + diff_queue_clear(&diff_queued_diff);

858

859 diff_tree_oid(parent_tree_oid, tree_oid, "", opt);

860

@@ -1097,7 +1096,7 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)

1096 static void free_diffqueues(int n, struct diff_queue_struct *dq)

1097 {

1098 for (int i = 0; i < n; i++)

1100 - diff_free_queue(&dq[i]);

1099 + diff_queue_clear(&dq[i]);

1100 free(dq);

1101 }

1102

@@ -1200,7 +1199,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c

1199 if (parent)

1200 add_line_range(rev, parent, parent_range);

1201 free_line_log_data(parent_range);

1203 - diff_free_queue(&queue);

1202 + diff_queue_clear(&queue);

1203 return changed;

1204 }

1205

log-tree.c

+2 -2

@@ -675,7 +675,7 @@ static void show_diff_of_diff(struct rev_info *opt)

675 struct diff_queue_struct dq;

676

677 memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));

678 - DIFF_QUEUE_CLEAR(&diff_queued_diff);

678 + diff_queue_init(&diff_queued_diff);

679

680 fprintf_ln(opt->diffopt.file, "\n%s", opt->idiff_title);

681 show_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,

@@ -694,7 +694,7 @@ static void show_diff_of_diff(struct rev_info *opt)

694 };

695

696 memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));

697 - DIFF_QUEUE_CLEAR(&diff_queued_diff);

697 + diff_queue_init(&diff_queued_diff);

698

699 fprintf_ln(opt->diffopt.file, "\n%s", opt->rdiff_title);

700 /*

merge-ort.c

+1 -1

@@ -3536,7 +3536,7 @@ simple_cleanup:

3536 /* Free memory for renames->pairs[] and combined */

3537 for (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++) {

3538 free(renames->pairs[s].queue);

3539 - DIFF_QUEUE_CLEAR(&renames->pairs[s]);

3539 + diff_queue_init(&renames->pairs[s]);

3540 }

3541 for (i = 0; i < combined.nr; i++)

3542 pool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);