On Wed, Nov 02 2022, SZEDER Gábor wrote:
> The first patch plugs the reported big memory leak, the second one
> plugs a minor leak, and the little cleanup in the third puts the
> cherry on top.
Looks good as far as it goes.
The "further" part seems a real mess though, e.g. I came up with the
below on top as a quick test, i.e. we have other existing users that
also loop over the same struct, and free "queue[n]", but do so
differently.
I handled the combine-diff.c one (rough WIP, only compiled it), but then
diffcore-rename.c has another such case.
I wonder if you looked at that further, and if the free function we're
adding now should anticipate that case or not.
And, orthagonally I came up with this rough WIP yesterday:
@@ -6640,9 +6633,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_STRUCT_INIT;
if (!options->filter)
return;
@@ -6735,8 +6726,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_STRUCT_INIT;
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
diff --git a/diffcore.h b/diffcore.h
index badc2261c20..a0a89568cec 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -150,12 +150,7 @@ struct diff_queue_struct {
int alloc;
int nr;
};
-
-#define DIFF_QUEUE_CLEAR(q) \
- do { \
- (q)->queue = NULL; \
- (q)->nr = (q)->alloc = 0; \
- } while (0)
+ #define DIFF_QUEUE_STRUCT_INIT { 0 }
You leave the DIFF_QUEUE_CLEAR in place, but I wonder given that that's
the common pattern whether you shouldn't have a *_reset() and
*_{free,release}() (one of the two, your current naming is fine) which
resets it too, as some callers seen below & in your diff context want
that.
diff --git a/combine-diff.c b/combine-diff.c
index b0ece954808..509c58ad556 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1297,12 +1297,6 @@ void show_combined_diff(struct combine_diff_path *p,
show_patch_diff(p, num_parent, 1, rev);
}
-static void free_combined_pair(struct diff_filepair *pair)
-{
- free(pair->two);
- free(pair);
-}
-
/*
* A combine_diff_path expresses N parents on the LHS against 1 merge
* result. Synthesize a diff_filepair that has N entries on the "one"
@@ -1355,9 +1349,7 @@ static void handle_combined_callback(struct diff_options *opt,
for (i = 0, p = paths; p; p = p->next)
q.queue[i++] = combined_pair(p, num_parent);
opt->format_callback(&q, opt, opt->format_callback_data);
- for (i = 0; i < num_paths; i++)
- free_combined_pair(q.queue[i]);
- free(q.queue);
+ diff_free_queue(&q, 1);
}
static const char *path_path(void *obj)
diff --git a/diff.c b/diff.c
index 03e6ffb5e4e..b48105c070f 100644
--- a/diff.c
+++ b/diff.c
@@ -5773,10 +5773,20 @@ void diff_free_filepair(struct diff_filepair *p)
free(p);
}
-void diff_free_queue(struct diff_queue_struct *q)
+static void free_combined_pair(struct diff_filepair *pair)
{
- for (int i = 0; i < q->nr; i++)
- diff_free_filepair(q->queue[i]);
+ free(pair->two);
+ free(pair);
+}
+
+void diff_free_queue(struct diff_queue_struct *q, int combined)
+{
+ for (int i = 0; i < q->nr; i++) {
+ if (combined)
+ diff_free_filepair(q->queue[i]);
+ else
+ free_combined_pair(q->queue[i]);
+ }
free(q->queue);
}
@@ -6339,7 +6349,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_free_queue(q, 0);
DIFF_QUEUE_CLEAR(q);
return result;
@@ -6609,7 +6619,7 @@ void diff_flush(struct diff_options *options)
options->format_callback(q, options, options->format_callback_data);
free_queue:
- diff_free_queue(q);
+ diff_free_queue(q, 0);
DIFF_QUEUE_CLEAR(q);
diff_free(options);
diff --git a/diffcore-rename.c b/diffcore-rename.c
index c0422d9e709..1754411a916 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -1686,6 +1686,7 @@ void diffcore_rename_extended(struct diff_options *options,
pair_to_free = p;
if (pair_to_free)
+ /* ??? */
pool_diff_free_filepair(pool, pair_to_free);
}
diff_debug_queue("done copying original", &outq);
diff --git a/diffcore.h b/diffcore.h
index 9b588a1ee15..6cb74e6eadf 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -162,7 +162,7 @@ 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);
+void diff_free_queue(struct diff_queue_struct *q, int combined);
/* dir_rename_relevance: the reason we want rename information for a dir */
enum dir_rename_relevance {
diff --git a/line-log.c b/line-log.c
index a7f3e7f6ce4..88c22b20c0f 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1090,7 +1090,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_free_queue(&dq[i], 0);
free(dq);
}
@@ -1193,7 +1193,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_free_queue(&queue, 0);
return changed;
}
