Skip to content

Commit

Permalink
diff: improve lifecycle management of diff queues
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@pks-t @gitster
pks-t authored and gitster committed Sep 30, 2024
1 parent fdf972a commit a5aecb2
Show file tree
Hide file tree
Showing 10 changed files with 32 additions and 47 deletions.
8 changes: 1 addition & 7 deletions bloom.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -476,8 +476,6 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
*last_slash = '\0';

} while (*path);

diff_free_filepair(diff_queued_diff.queue[i]);
}

if (hashmap_get_size(&pathmap) > settings->max_changed_paths) {
Expand Down Expand Up @@ -508,8 +506,6 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
cleanup:
hashmap_clear_and_free(&pathmap, struct pathmap_hash_entry, entry);
} else {
for (i = 0; i < diff_queued_diff.nr; i++)
diff_free_filepair(diff_queued_diff.queue[i]);
init_truncated_large_filter(filter, settings->hash_version);

if (computed)
Expand All @@ -519,9 +515,7 @@ struct bloom_filter *get_or_compute_bloom_filter(struct repository *r,
if (computed)
*computed |= BLOOM_COMPUTED;

free(diff_queued_diff.queue);
DIFF_QUEUE_CLEAR(&diff_queued_diff);

diff_queue_clear(&diff_queued_diff);
return filter;
}

Expand Down
22 changes: 12 additions & 10 deletions diff.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5983,11 +5983,18 @@ void diff_free_filepair(struct diff_filepair *p)
free(p);
}

void diff_free_queue(struct diff_queue_struct *q)
void diff_queue_init(struct diff_queue_struct *q)
{
struct diff_queue_struct blank = DIFF_QUEUE_INIT;
memcpy(q, &blank, sizeof(*q));
}

void diff_queue_clear(struct diff_queue_struct *q)
{
for (int i = 0; i < q->nr; i++)
diff_free_filepair(q->queue[i]);
free(q->queue);
diff_queue_init(q);
}

const char *diff_aligned_abbrev(const struct object_id *oid, int len)
Expand Down Expand Up @@ -6551,8 +6558,7 @@ int diff_flush_patch_id(struct diff_options *options, struct object_id *oid, int
struct diff_queue_struct *q = &diff_queued_diff;
int result = diff_get_patch_id(options, oid, diff_header_only);

diff_free_queue(q);
DIFF_QUEUE_CLEAR(q);
diff_queue_clear(q);

return result;
}
Expand Down Expand Up @@ -6835,8 +6841,7 @@ void diff_flush(struct diff_options *options)
}

free_queue:
diff_free_queue(q);
DIFF_QUEUE_CLEAR(q);
diff_queue_clear(q);
diff_free(options);

/*
Expand Down Expand Up @@ -6867,9 +6872,7 @@ static void diffcore_apply_filter(struct diff_options *options)
{
int i;
struct diff_queue_struct *q = &diff_queued_diff;
struct diff_queue_struct outq;

DIFF_QUEUE_CLEAR(&outq);
struct diff_queue_struct outq = DIFF_QUEUE_INIT;

if (!options->filter)
return;
Expand Down Expand Up @@ -6962,8 +6965,7 @@ static void diffcore_skip_stat_unmatch(struct diff_options *diffopt)
{
int i;
struct diff_queue_struct *q = &diff_queued_diff;
struct diff_queue_struct outq;
DIFF_QUEUE_CLEAR(&outq);
struct diff_queue_struct outq = DIFF_QUEUE_INIT;

for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
Expand Down
8 changes: 2 additions & 6 deletions diffcore-break.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ static int should_break(struct repository *r,
void diffcore_break(struct repository *r, int break_score)
{
struct diff_queue_struct *q = &diff_queued_diff;
struct diff_queue_struct outq;
struct diff_queue_struct outq = DIFF_QUEUE_INIT;

/* When the filepair has this much edit (insert and delete),
* it is first considered to be a rewrite and broken into a
Expand Down Expand Up @@ -178,8 +178,6 @@ void diffcore_break(struct repository *r, int break_score)
if (!merge_score)
merge_score = DEFAULT_MERGE_SCORE;

DIFF_QUEUE_CLEAR(&outq);

for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
int score;
Expand Down Expand Up @@ -275,11 +273,9 @@ static void merge_broken(struct diff_filepair *p,
void diffcore_merge_broken(void)
{
struct diff_queue_struct *q = &diff_queued_diff;
struct diff_queue_struct outq;
struct diff_queue_struct outq = DIFF_QUEUE_INIT;
int i, j;

DIFF_QUEUE_CLEAR(&outq);

for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
if (!p)
Expand Down
4 changes: 1 addition & 3 deletions diffcore-pickaxe.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,7 @@ static void pickaxe(struct diff_queue_struct *q, struct diff_options *o,
regex_t *regexp, kwset_t kws, pickaxe_fn fn)
{
int i;
struct diff_queue_struct outq;

DIFF_QUEUE_CLEAR(&outq);
struct diff_queue_struct outq = DIFF_QUEUE_INIT;

if (o->pickaxe_opts & DIFF_PICKAXE_ALL) {
/* Showing the whole changeset if needle exists */
Expand Down
3 changes: 1 addition & 2 deletions diffcore-rename.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1388,7 +1388,7 @@ void diffcore_rename_extended(struct diff_options *options,
int detect_rename = options->detect_rename;
int minimum_score = options->rename_score;
struct diff_queue_struct *q = &diff_queued_diff;
struct diff_queue_struct outq;
struct diff_queue_struct outq = DIFF_QUEUE_INIT;
struct diff_score *mx;
int i, j, rename_count, skip_unmodified = 0;
int num_destinations, dst_cnt;
Expand Down Expand Up @@ -1638,7 +1638,6 @@ void diffcore_rename_extended(struct diff_options *options,
* are recorded in rename_dst. The original list is still in *q.
*/
trace2_region_enter("diff", "write back to queue", options->repo);
DIFF_QUEUE_CLEAR(&outq);
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
struct diff_filepair *pair_to_free = NULL;
Expand Down
3 changes: 1 addition & 2 deletions diffcore-rotate.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
void diffcore_rotate(struct diff_options *opt)
{
struct diff_queue_struct *q = &diff_queued_diff;
struct diff_queue_struct outq;
struct diff_queue_struct outq = DIFF_QUEUE_INIT;
int rotate_to, i;

if (!q->nr)
Expand All @@ -31,7 +31,6 @@ void diffcore_rotate(struct diff_options *opt)
return;
}

DIFF_QUEUE_CLEAR(&outq);
rotate_to = i;

for (i = rotate_to; i < q->nr; i++)
Expand Down
10 changes: 4 additions & 6 deletions diffcore.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,18 +153,16 @@ struct diff_queue_struct {
int nr;
};

#define DIFF_QUEUE_CLEAR(q) \
do { \
(q)->queue = NULL; \
(q)->nr = (q)->alloc = 0; \
} while (0)
#define DIFF_QUEUE_INIT { 0 }

void diff_queue_init(struct diff_queue_struct *q);
void diff_queue_clear(struct diff_queue_struct *q);

extern struct diff_queue_struct diff_queued_diff;
struct diff_filepair *diff_queue(struct diff_queue_struct *,
struct diff_filespec *,
struct diff_filespec *);
void diff_q(struct diff_queue_struct *, struct diff_filepair *);
void diff_free_queue(struct diff_queue_struct *q);

/* dir_rename_relevance: the reason we want rename information for a dir */
enum dir_rename_relevance {
Expand Down
15 changes: 7 additions & 8 deletions line-log.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -787,15 +787,14 @@ static void move_diff_queue(struct diff_queue_struct *dst,
struct diff_queue_struct *src)
{
assert(src != dst);
memcpy(dst, src, sizeof(struct diff_queue_struct));
DIFF_QUEUE_CLEAR(src);
memcpy(dst, src, sizeof(*dst));
diff_queue_init(src);
}

static void filter_diffs_for_paths(struct line_log_data *range, int keep_deletions)
{
int i;
struct diff_queue_struct outq;
DIFF_QUEUE_CLEAR(&outq);
struct diff_queue_struct outq = DIFF_QUEUE_INIT;

for (i = 0; i < diff_queued_diff.nr; i++) {
struct diff_filepair *p = diff_queued_diff.queue[i];
Expand Down Expand Up @@ -850,12 +849,12 @@ static void queue_diffs(struct line_log_data *range,
clear_pathspec(&opt->pathspec);
parse_pathspec_from_ranges(&opt->pathspec, range);
}
DIFF_QUEUE_CLEAR(&diff_queued_diff);
diff_queue_clear(&diff_queued_diff);
diff_tree_oid(parent_tree_oid, tree_oid, "", opt);
if (opt->detect_rename && diff_might_be_rename()) {
/* must look at the full tree diff to detect renames */
clear_pathspec(&opt->pathspec);
DIFF_QUEUE_CLEAR(&diff_queued_diff);
diff_queue_clear(&diff_queued_diff);

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

Expand Down Expand Up @@ -1097,7 +1096,7 @@ static struct diff_filepair *diff_filepair_dup(struct diff_filepair *pair)
static void free_diffqueues(int n, struct diff_queue_struct *dq)
{
for (int i = 0; i < n; i++)
diff_free_queue(&dq[i]);
diff_queue_clear(&dq[i]);
free(dq);
}

Expand Down Expand Up @@ -1200,7 +1199,7 @@ static int process_ranges_ordinary_commit(struct rev_info *rev, struct commit *c
if (parent)
add_line_range(rev, parent, parent_range);
free_line_log_data(parent_range);
diff_free_queue(&queue);
diff_queue_clear(&queue);
return changed;
}

Expand Down
4 changes: 2 additions & 2 deletions log-tree.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -675,7 +675,7 @@ static void show_diff_of_diff(struct rev_info *opt)
struct diff_queue_struct dq;

memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
DIFF_QUEUE_CLEAR(&diff_queued_diff);
diff_queue_init(&diff_queued_diff);

fprintf_ln(opt->diffopt.file, "\n%s", opt->idiff_title);
show_interdiff(opt->idiff_oid1, opt->idiff_oid2, 2,
Expand All @@ -694,7 +694,7 @@ static void show_diff_of_diff(struct rev_info *opt)
};

memcpy(&dq, &diff_queued_diff, sizeof(diff_queued_diff));
DIFF_QUEUE_CLEAR(&diff_queued_diff);
diff_queue_init(&diff_queued_diff);

fprintf_ln(opt->diffopt.file, "\n%s", opt->rdiff_title);
/*
Expand Down
2 changes: 1 addition & 1 deletion merge-ort.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3536,7 +3536,7 @@ static int detect_and_process_renames(struct merge_options *opt)
/* Free memory for renames->pairs[] and combined */
for (s = MERGE_SIDE1; s <= MERGE_SIDE2; s++) {
free(renames->pairs[s].queue);
DIFF_QUEUE_CLEAR(&renames->pairs[s]);
diff_queue_init(&renames->pairs[s]);
}
for (i = 0; i < combined.nr; i++)
pool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);
Expand Down

0 comments on commit a5aecb2

Please sign in to comment.