@matheusbsbs / matheus-bagliano / commits / e900d494dc

diff: add an API for deferred freeing

Add a diff_free() function to free anything we may have allocated in the "diff_options" struct, and the ability to make calling it a noop by setting "no_free" in "diff_options". This is required because when e.g. "git diff" is run we'll allocate things in that struct, use the diff machinery once, and then exit. But if we run e.g. "git log -p" we're going to re-use what we allocated across multiple diff_flush() calls, and only want to free things at the end. We've thus ended up with features like the recently added "diff -I"[1] where we'll leak memory. As it turns out it could have simply used the pattern established in 6ea57703f6 (log: prepare log/log-tree to reuse the diffopt.close_file attribute, 2016-06-22). Manually adding more such flags to things log_tree_commit() every time we need to allocate something would be tedious. Let's instead move that fclose() code it to a new diff_free(), in anticipation of freeing more things in that function in follow-up commits. Some functions such as log_tree_commit() need an idiom of optionally retaining a previous "no_free", as they may either free the memory themselves, or their caller may do so. I'm keeping that idiom in log_show_early() for good measure, even though I don't think it's currently called in this manner. It also gets passed an existing "struct rev_info", so future callers may want to set the "no_free" flag. This change is a bit hard to read because while the freeing pattern we're introducing isn't unusual, the "file" member is a special snowflake. We usually don't want to fclose() it. This is because "file" is usually stdout, in which case we don't want to fclose() it. We only want to opt-in to closing it when we e.g. open a file on the filesystem. Thus the opt-in "close_file" flag. So the API in general just needs a "no_free" flag to defer freeing, but the "file" member still needs its "close_file" flag. This is made more confusing because while refactoring this code we could replace some "close_file=0" with "no_free=1", whereas others need to set both flags. This is because there were some cases where an existing "close_file=0" meant "let's defer deallocation", and others where it meant "we don't want to close this file handle at all". 1. 296d4a94e7 (diff: add -I<regex> that ignores matching changes, 2020-10-20) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Feb 11, 2021 at 11:45 UTC e900d494dcff7bb9033865e61b452128ff232481

4 files changed +48 -20

builtin/log.c

+12 -11

@@ -307,10 +307,11 @@ static struct itimerval early_output_timer;

307

308 static void log_show_early(struct rev_info *revs, struct commit_list *list)

309 {

310 - int i = revs->early_output, close_file = revs->diffopt.close_file;

310 + int i = revs->early_output;

311 int show_header = 1;

312 + int no_free = revs->diffopt.no_free;

313

313 - revs->diffopt.close_file = 0;

314 + revs->diffopt.no_free = 0;

315 sort_in_topological_order(&list, revs->sort_order);

316 while (list && i) {

317 struct commit *commit = list->item;

@@ -327,8 +328,8 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)

328 case commit_ignore:

329 break;

330 case commit_error:

330 - if (close_file)

331 - fclose(revs->diffopt.file);

331 + revs->diffopt.no_free = no_free;

332 + diff_free(&revs->diffopt);

333 return;

334 }

335 list = list->next;

@@ -336,8 +337,8 @@ static void log_show_early(struct rev_info *revs, struct commit_list *list)

337

338 /* Did we already get enough commits for the early output? */

339 if (!i) {

339 - if (close_file)

340 - fclose(revs->diffopt.file);

340 + revs->diffopt.no_free = 0;

341 + diff_free(&revs->diffopt);

342 return;

343 }

344

@@ -401,7 +402,7 @@ static int cmd_log_walk(struct rev_info *rev)

402 {

403 struct commit *commit;

404 int saved_nrl = 0;

404 - int saved_dcctc = 0, close_file = rev->diffopt.close_file;

405 + int saved_dcctc = 0;

406

407 if (rev->early_output)

408 setup_early_output();

@@ -417,7 +418,7 @@ static int cmd_log_walk(struct rev_info *rev)

418 * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to

419 * retain that state information if replacing rev->diffopt in this loop

420 */

420 - rev->diffopt.close_file = 0;

421 + rev->diffopt.no_free = 1;

422 while ((commit = get_revision(rev)) != NULL) {

423 if (!log_tree_commit(rev, commit) && rev->max_count >= 0)

424 /*

@@ -442,8 +443,8 @@ static int cmd_log_walk(struct rev_info *rev)

443 }

444 rev->diffopt.degraded_cc_to_c = saved_dcctc;

445 rev->diffopt.needed_rename_limit = saved_nrl;

445 - if (close_file)

446 - fclose(rev->diffopt.file);

446 + rev->diffopt.no_free = 0;

447 + diff_free(&rev->diffopt);

448

449 if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&

450 rev->diffopt.flags.check_failed) {

@@ -1955,7 +1956,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)

1956 * file, but but we must instruct it not to close after each

1957 * diff.

1958 */

1958 - rev.diffopt.close_file = 0;

1959 + rev.diffopt.no_free = 1;

1960 } else {

1961 int saved;

1962

diff.c

+16 -4

@@ -6336,6 +6336,20 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)

6336 }

6337 }

6338

6339 +static void diff_free_file(struct diff_options *options)

6340 +{

6341 + if (options->close_file)

6342 + fclose(options->file);

6343 +}

6344 +

6345 +void diff_free(struct diff_options *options)

6346 +{

6347 + if (options->no_free)

6348 + return;

6349 +

6350 + diff_free_file(options);

6351 +}

6352 +

6353 void diff_flush(struct diff_options *options)

6354 {

6355 struct diff_queue_struct *q = &diff_queued_diff;

@@ -6399,8 +6413,7 @@ void diff_flush(struct diff_options *options)

6413 * options->file to /dev/null should be safe, because we

6414 * aren't supposed to produce any output anyway.

6415 */

6402 - if (options->close_file)

6403 - fclose(options->file);

6416 + diff_free_file(options);

6417 options->file = xfopen("/dev/null", "w");

6418 options->close_file = 1;

6419 options->color_moved = 0;

@@ -6433,8 +6446,7 @@ void diff_flush(struct diff_options *options)

6446 free_queue:

6447 free(q->queue);

6448 DIFF_QUEUE_CLEAR(q);

6436 - if (options->close_file)

6437 - fclose(options->file);

6449 + diff_free(options);

6450

6451 /*

6452 * Report the content-level differences with HAS_CHANGES;

diff.h

+14 -1

@@ -49,7 +49,17 @@

49 * - Once you finish feeding the pairs of files, call `diffcore_std()`.

50 * This will tell the diffcore library to go ahead and do its work.

51 *

52 - * - Calling `diff_flush()` will produce the output.

52 + * - Calling `diff_flush()` will produce the output, it will call

53 + * `diff_free()` to free any resources, e.g. those allocated in

54 + * `diff_opt_parse()`.

55 + *

56 + * - Set `.no_free = 1` before calling `diff_flush()` to defer the

57 + * freeing of allocated memory in diff_options. This is useful when

58 + * `diff_flush()` is being called in a loop, rather than as a

59 + * one-off. When setting `.no_free = 1` you must ensure that

60 + * `diff_free()` is called at the end, either by flipping the flag

61 + * before the last `diff_flush()` call, or by flipping it before

62 + * calling `diff_free()` yourself.

63 */

65 struct combine_diff_path;

@@ -365,6 +375,8 @@ struct diff_options {

375

376 struct repository *repo;

377 struct option *parseopts;

378 +

379 + int no_free;

380 };

381

382 unsigned diff_filter_bit(char status);

@@ -559,6 +571,7 @@ void diffcore_fix_diff_index(void);

571

572 int diff_queue_is_empty(void);

573 void diff_flush(struct diff_options*);

574 +void diff_free(struct diff_options*);

575 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);

576

577 /* diff-raw status letters */

log-tree.c

+6 -4

@@ -958,12 +958,14 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log

958 int log_tree_commit(struct rev_info *opt, struct commit *commit)

959 {

960 struct log_info log;

961 - int shown, close_file = opt->diffopt.close_file;

961 + int shown;

962 + /* maybe called by e.g. cmd_log_walk(), maybe stand-alone */

963 + int no_free = opt->diffopt.no_free;

964

965 log.commit = commit;

966 log.parent = NULL;

967 opt->loginfo = &log;

966 - opt->diffopt.close_file = 0;

968 + opt->diffopt.no_free = 1;

969

970 if (opt->line_level_traverse)

971 return line_log_print(opt, commit);

@@ -980,7 +982,7 @@ int log_tree_commit(struct rev_info *opt, struct commit *commit)

982 fprintf(opt->diffopt.file, "\n%s\n", opt->break_bar);

983 opt->loginfo = NULL;

984 maybe_flush_or_die(opt->diffopt.file, "stdout");

983 - if (close_file)

984 - fclose(opt->diffopt.file);

985 + opt->diffopt.no_free = no_free;

986 + diff_free(&opt->diffopt);

987 return shown;

988 }