| Message ID | d226d3bc-fb15-58a4-f516-bda51a912228@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | diff: build parseopts array on demand | expand |
René Scharfe <l.s.r@web.de> writes: > repo_diff_setup() builds the struct option array with git diff's command > line options and stores a pointer to it in the parseopts member of > struct diff_options. The array is freed by diff_setup_done(), but not > by release_revisions(). repo_init_revisions() calls repo_diff_setup(), > thus calling repo_init_revisions() then release_revisions() leaks it. > > We could free the array in release_revisions() as well to plug that > leak, but there is a better way: Only build it when needed. Move the > get_diff_parseopts() calls to the two places that use the array, free it > after use and get rid of the parseopts member. > > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > diff.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) I think this hunk is missing? diff.h | 1 - 1 file changed, 1 deletion(-) diff --git c/diff.h w/diff.h index 5229f20486..6840499844 100644 --- c/diff.h +++ w/diff.h @@ -394,7 +394,6 @@ struct diff_options { unsigned color_moved_ws_handling; struct repository *repo; - struct option *parseopts; struct strmap *additional_path_headers; int no_free;
Am 01.12.22 um 02:25 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> repo_diff_setup() builds the struct option array with git diff's command >> line options and stores a pointer to it in the parseopts member of >> struct diff_options. The array is freed by diff_setup_done(), but not >> by release_revisions(). repo_init_revisions() calls repo_diff_setup(), >> thus calling repo_init_revisions() then release_revisions() leaks it. >> >> We could free the array in release_revisions() as well to plug that >> leak, but there is a better way: Only build it when needed. Move the >> get_diff_parseopts() calls to the two places that use the array, free it >> after use and get rid of the parseopts member. >> >> Signed-off-by: René Scharfe <l.s.r@web.de> >> --- >> diff.c | 17 ++++++++--------- >> 1 file changed, 8 insertions(+), 9 deletions(-) > > I think this hunk is missing? Yes. O_o > > diff.h | 1 - > 1 file changed, 1 deletion(-) > > diff --git c/diff.h w/diff.h > index 5229f20486..6840499844 100644 > --- c/diff.h > +++ w/diff.h > @@ -394,7 +394,6 @@ struct diff_options { > unsigned color_moved_ws_handling; > > struct repository *repo; > - struct option *parseopts; > struct strmap *additional_path_headers; > > int no_free;
René Scharfe <l.s.r@web.de> writes: > Am 01.12.22 um 02:25 schrieb Junio C Hamano: >> René Scharfe <l.s.r@web.de> writes: >> >>> repo_diff_setup() builds the struct option array with git diff's command >>> line options and stores a pointer to it in the parseopts member of >>> struct diff_options. The array is freed by diff_setup_done(), but not >>> by release_revisions(). repo_init_revisions() calls repo_diff_setup(), >>> thus calling repo_init_revisions() then release_revisions() leaks it. >>> >>> We could free the array in release_revisions() as well to plug that >>> leak, but there is a better way: Only build it when needed. Move the >>> get_diff_parseopts() calls to the two places that use the array, free it >>> after use and get rid of the parseopts member. >>> >>> Signed-off-by: René Scharfe <l.s.r@web.de> >>> --- >>> diff.c | 17 ++++++++--------- >>> 1 file changed, 8 insertions(+), 9 deletions(-) >> >> I think this hunk is missing? > > Yes. O_o I did not see any other issue in the series, so if no further tweaks are needed, I could just squash it in. Thanks for working on this clean-up. The way parse-options parser was bolted on (eh, rather, the way the original parser of diff command line options way predated the parse-options machinery) has always disturbed me as being klunky. > >> >> diff.h | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git c/diff.h w/diff.h >> index 5229f20486..6840499844 100644 >> --- c/diff.h >> +++ w/diff.h >> @@ -394,7 +394,6 @@ struct diff_options { >> unsigned color_moved_ws_handling; >> >> struct repository *repo; >> - struct option *parseopts; >> struct strmap *additional_path_headers; >> >> int no_free;
Am 01.12.2022 um 22:56 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Am 01.12.22 um 02:25 schrieb Junio C Hamano: >>> >>> I think this hunk is missing? >> >> Yes. O_o > > I did not see any other issue in the series, so if no further tweaks > are needed, I could just squash it in. Thank you, but I'm about to send a version 3. René
diff --git a/diff.c b/diff.c index e469d5d2a0..6415c4dc2d 100644 --- a/diff.c +++ b/diff.c @@ -4615,8 +4615,6 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o) builtin_checkdiff(name, other, attr_path, p->one, p->two, o); } -static struct option *get_diff_parseopts(struct diff_options *options); - void repo_diff_setup(struct repository *r, struct diff_options *options) { memcpy(options, &default_diff_options, sizeof(*options)); @@ -4662,8 +4660,6 @@ void repo_diff_setup(struct repository *r, struct diff_options *options) options->color_moved = diff_color_moved_default; options->color_moved_ws_handling = diff_color_moved_ws_default; - - options->parseopts = get_diff_parseopts(options); } static const char diff_status_letters[] = { @@ -4821,8 +4817,6 @@ void diff_setup_done(struct diff_options *options) options->filter = ~filter_bit[DIFF_STATUS_FILTER_AON]; options->filter &= ~options->filter_not; } - - FREE_AND_NULL(options->parseopts); } int parse_long_opt(const char *opt, const char **argv, @@ -5695,21 +5689,27 @@ static struct option *get_diff_parseopts(struct diff_options *options) struct option *add_diff_options(const struct option *parseopts, struct diff_options *options) { - return parse_options_concat(parseopts, options->parseopts); + struct option *diff_parseopts = get_diff_parseopts(options); + struct option *result = parse_options_concat(parseopts, diff_parseopts); + free(diff_parseopts); + return result; } int diff_opt_parse(struct diff_options *options, const char **av, int ac, const char *prefix) { + struct option *diff_parseopts = get_diff_parseopts(options); + if (!prefix) prefix = ""; - ac = parse_options(ac, av, prefix, options->parseopts, NULL, + ac = parse_options(ac, av, prefix, diff_parseopts, NULL, PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN_OPT | PARSE_OPT_NO_INTERNAL_HELP | PARSE_OPT_ONE_SHOT | PARSE_OPT_STOP_AT_NON_OPTION); + free(diff_parseopts); return ac; } @@ -6518,7 +6518,6 @@ void diff_free(struct diff_options *options) diff_free_file(options); diff_free_ignore_regex(options); clear_pathspec(&options->pathspec); - FREE_AND_NULL(options->parseopts); } void diff_flush(struct diff_options *options)
repo_diff_setup() builds the struct option array with git diff's command line options and stores a pointer to it in the parseopts member of struct diff_options. The array is freed by diff_setup_done(), but not by release_revisions(). repo_init_revisions() calls repo_diff_setup(), thus calling repo_init_revisions() then release_revisions() leaks it. We could free the array in release_revisions() as well to plug that leak, but there is a better way: Only build it when needed. Move the get_diff_parseopts() calls to the two places that use the array, free it after use and get rid of the parseopts member. Signed-off-by: René Scharfe <l.s.r@web.de> --- diff.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) -- 2.38.1