# [PATCH 0/3] diff: build parseopts array on demand 23 messages from 2022-11-30 to 2022-12-01. Participants: René Scharfe, Junio C Hamano, ZheNing Hu, Ævar Arnfjörð Bjarmason, Eric Sunshine. Thread: https://gitlist.dev/t/58876 ## René Scharfe, 2022-11-30 18:01 Subject: [PATCH 0/3] diff: build parseopts array on demand Message-ID: <0620b4a6-b80a-d8a0-5a35-857b9fe0c986@web.de> URL: https://gitlist.dev/e/0620b4a6-b80a-d8a0-5a35-857b9fe0c986%40web.de ``` Calling repo_init_revisions() and release_revisions() in that order leaks the memory allocated for the parseopts array in the embedded struct diff_options member. Get rid of that leak by reducing the lifetime of that array. Original patch: https://lore.kernel.org/git/4fd82dc6-e0f8-0638-5b10-16bfef39a171@web.de/ Submitted separately from that thread because it's independent enough. diff: factor out add_diff_options() diff: let prep_parse_options() return parseopt array diff: remove parseopts member of struct diff_options builtin/range-diff.c | 2 +- diff-no-index.c | 3 +-- diff.c | 26 +++++++++++++++----------- diff.h | 1 + 4 files changed, 18 insertions(+), 14 deletions(-) -- 2.38.1 ``` ## René Scharfe, 2022-11-30 18:03 Subject: [PATCH 1/3] diff: factor out add_diff_options() Message-ID: <19be5afa-63e8-5f11-c5f4-6f391677e4c4@web.de> URL: https://gitlist.dev/e/19be5afa-63e8-5f11-c5f4-6f391677e4c4%40web.de In-Reply-To: <0620b4a6-b80a-d8a0-5a35-857b9fe0c986@web.de> ``` Add a function for appending the parseopts member of struct diff_options to a struct option array. Use it in two sites instead of accessing the parseopts member directly. Decoupling callers from diff internals like that allows us to change the latter. Signed-off-by: René Scharfe --- builtin/range-diff.c | 2 +- diff-no-index.c | 3 +-- diff.c | 6 ++++++ diff.h | 1 + 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/builtin/range-diff.c b/builtin/range-diff.c index e2a74efb42..aecfae12d3 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -47,7 +47,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) repo_diff_setup(the_repository, &diffopt); - options = parse_options_concat(range_diff_options, diffopt.parseopts); + options = add_diff_options(range_diff_options, &diffopt); argc = parse_options(argc, argv, prefix, options, builtin_range_diff_usage, PARSE_OPT_KEEP_DASHDASH); diff --git a/diff-no-index.c b/diff-no-index.c index 18edbdf4b5..05fafd0019 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -255,8 +255,7 @@ int diff_no_index(struct rev_info *revs, }; struct option *options; - options = parse_options_concat(no_index_options, - revs->diffopt.parseopts); + options = add_diff_options(no_index_options, &revs->diffopt); argc = parse_options(argc, argv, revs->prefix, options, diff_no_index_usage, 0); if (argc != 2) { diff --git a/diff.c b/diff.c index 1054a4b732..e01129f0ea 100644 --- a/diff.c +++ b/diff.c @@ -5693,6 +5693,12 @@ static void prep_parse_options(struct diff_options *options) memcpy(options->parseopts, parseopts, sizeof(parseopts)); } +struct option *add_diff_options(const struct option *parseopts, + struct diff_options *options) +{ + return parse_options_concat(parseopts, options->parseopts); +} + int diff_opt_parse(struct diff_options *options, const char **av, int ac, const char *prefix) { diff --git a/diff.h b/diff.h index fd33caeb25..c20a1ad76d 100644 --- a/diff.h +++ b/diff.h @@ -539,6 +539,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb); #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts) #endif void repo_diff_setup(struct repository *, struct diff_options *); +struct option *add_diff_options(const struct option *, struct diff_options *); int diff_opt_parse(struct diff_options *, const char **, int, const char *); void diff_setup_done(struct diff_options *); int git_config_rename(const char *var, const char *value); -- 2.38.1 ``` ## René Scharfe, 2022-11-30 18:04 Subject: [PATCH 2/3] diff: let prep_parse_options() return parseopt array Message-ID: URL: https://gitlist.dev/e/b5f611f5-86e2-6b11-20a4-a847c97b145f%40web.de In-Reply-To: <0620b4a6-b80a-d8a0-5a35-857b9fe0c986@web.de> ``` prep_parse_options() sets the parseopts member of struct diff_options. Let it return the pointer instead and have its caller do the assignment instead and rename the function to get_diff_parseopts() to reflect that change. This allows using it in other places and with other variables. Signed-off-by: René Scharfe --- diff.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index e01129f0ea..e469d5d2a0 100644 --- a/diff.c +++ b/diff.c @@ -4615,7 +4615,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o) builtin_checkdiff(name, other, attr_path, p->one, p->two, o); } -static void prep_parse_options(struct diff_options *options); +static struct option *get_diff_parseopts(struct diff_options *options); void repo_diff_setup(struct repository *r, struct diff_options *options) { @@ -4663,7 +4663,7 @@ 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; - prep_parse_options(options); + options->parseopts = get_diff_parseopts(options); } static const char diff_status_letters[] = { @@ -5419,7 +5419,7 @@ static int diff_opt_rotate_to(const struct option *opt, const char *arg, int uns return 0; } -static void prep_parse_options(struct diff_options *options) +static struct option *get_diff_parseopts(struct diff_options *options) { struct option parseopts[] = { OPT_GROUP(N_("Diff output format options")), @@ -5689,8 +5689,7 @@ static void prep_parse_options(struct diff_options *options) OPT_END() }; - ALLOC_ARRAY(options->parseopts, ARRAY_SIZE(parseopts)); - memcpy(options->parseopts, parseopts, sizeof(parseopts)); + return parse_options_dup(parseopts); } struct option *add_diff_options(const struct option *parseopts, -- 2.38.1 ``` ## René Scharfe, 2022-11-30 18:04 Subject: [PATCH 3/3] diff: remove parseopts member of struct diff_options Message-ID: URL: https://gitlist.dev/e/d226d3bc-fb15-58a4-f516-bda51a912228%40web.de In-Reply-To: <0620b4a6-b80a-d8a0-5a35-857b9fe0c986@web.de> ``` 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 --- diff.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) 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) -- 2.38.1 ``` ## Junio C Hamano, 2022-12-01 01:02 Subject: Re: [PATCH 0/3] diff: build parseopts array on demand Message-ID: URL: https://gitlist.dev/e/xmqqh6ygszpm.fsf%40gitster.g In-Reply-To: <0620b4a6-b80a-d8a0-5a35-857b9fe0c986@web.de> ``` René Scharfe writes: > Calling repo_init_revisions() and release_revisions() in that order > leaks the memory allocated for the parseopts array in the embedded > struct diff_options member. Get rid of that leak by reducing the > lifetime of that array. Makes sense. ``` ## Junio C Hamano, 2022-12-01 01:25 Subject: Re: [PATCH 3/3] diff: remove parseopts member of struct diff_options Message-ID: URL: https://gitlist.dev/e/xmqqcz93ud8o.fsf%40gitster.g In-Reply-To: ``` René Scharfe 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 > --- > 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; ``` ## René Scharfe, 2022-12-01 07:52 Subject: Re: [PATCH 3/3] diff: remove parseopts member of struct diff_options Message-ID: <20b5d6d1-ec09-e09f-4f92-5925fb9c96f8@web.de> URL: https://gitlist.dev/e/20b5d6d1-ec09-e09f-4f92-5925fb9c96f8%40web.de In-Reply-To: ``` Am 01.12.22 um 02:25 schrieb Junio C Hamano: > René Scharfe 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 >> --- >> 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, 2022-12-01 13:39 Subject: [PATCH v2 0/3] diff: build parseopts array on demand Message-ID: URL: https://gitlist.dev/e/ea838ae7-b635-d4d2-d9df-e96b3d8980af%40web.de In-Reply-To: <0620b4a6-b80a-d8a0-5a35-857b9fe0c986@web.de> ``` Calling repo_init_revisions() and release_revisions() in that order leaks the memory allocated for the parseopts array in the embedded struct diff_options member. Get rid of that leak by reducing the lifetime of that array. Original patch: https://lore.kernel.org/git/4fd82dc6-e0f8-0638-5b10-16bfef39a171@web.de/ Submitted separately from that thread because it's independent enough. Change since v1: - Actually remove the parseopts member. Its removal got lost during refactoring in v1. Thank you for spotting that, Junio! diff: factor out add_diff_options() diff: let prep_parse_options() return parseopt array diff: remove parseopts member from struct diff_options builtin/range-diff.c | 2 +- diff-no-index.c | 3 +-- diff.c | 26 +++++++++++++++----------- diff.h | 2 +- 4 files changed, 18 insertions(+), 15 deletions(-) Range-Diff gegen v1: 1: 630f95320f = 1: 4dc8b2632b diff: factor out add_diff_options() 2: 4b56fa795c = 2: 10903d355e diff: let prep_parse_options() return parseopt array 3: 7e54e4370a ! 3: 24bd18ae79 diff: remove parseopts member from struct diff_options @@ diff.c: void diff_free(struct diff_options *options) } void diff_flush(struct diff_options *options) + + ## diff.h ## +@@ diff.h: struct diff_options { + unsigned color_moved_ws_handling; + + struct repository *repo; +- struct option *parseopts; + struct strmap *additional_path_headers; + + int no_free; -- 2.38.1 ``` ## René Scharfe, 2022-12-01 13:42 Subject: [PATCH v2 1/3] diff: factor out add_diff_options() Message-ID: URL: https://gitlist.dev/e/ed84a688-e1fd-945d-7dfc-10d051652c77%40web.de In-Reply-To: ``` Add a function for appending the parseopts member of struct diff_options to a struct option array. Use it in two sites instead of accessing the parseopts member directly. Decoupling callers from diff internals like that allows us to change the latter. Signed-off-by: René Scharfe --- builtin/range-diff.c | 2 +- diff-no-index.c | 3 +-- diff.c | 6 ++++++ diff.h | 1 + 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/builtin/range-diff.c b/builtin/range-diff.c index e2a74efb42..aecfae12d3 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -47,7 +47,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) repo_diff_setup(the_repository, &diffopt); - options = parse_options_concat(range_diff_options, diffopt.parseopts); + options = add_diff_options(range_diff_options, &diffopt); argc = parse_options(argc, argv, prefix, options, builtin_range_diff_usage, PARSE_OPT_KEEP_DASHDASH); diff --git a/diff-no-index.c b/diff-no-index.c index 18edbdf4b5..05fafd0019 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -255,8 +255,7 @@ int diff_no_index(struct rev_info *revs, }; struct option *options; - options = parse_options_concat(no_index_options, - revs->diffopt.parseopts); + options = add_diff_options(no_index_options, &revs->diffopt); argc = parse_options(argc, argv, revs->prefix, options, diff_no_index_usage, 0); if (argc != 2) { diff --git a/diff.c b/diff.c index 1054a4b732..e01129f0ea 100644 --- a/diff.c +++ b/diff.c @@ -5693,6 +5693,12 @@ static void prep_parse_options(struct diff_options *options) memcpy(options->parseopts, parseopts, sizeof(parseopts)); } +struct option *add_diff_options(const struct option *parseopts, + struct diff_options *options) +{ + return parse_options_concat(parseopts, options->parseopts); +} + int diff_opt_parse(struct diff_options *options, const char **av, int ac, const char *prefix) { diff --git a/diff.h b/diff.h index fd33caeb25..c20a1ad76d 100644 --- a/diff.h +++ b/diff.h @@ -539,6 +539,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb); #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts) #endif void repo_diff_setup(struct repository *, struct diff_options *); +struct option *add_diff_options(const struct option *, struct diff_options *); int diff_opt_parse(struct diff_options *, const char **, int, const char *); void diff_setup_done(struct diff_options *); int git_config_rename(const char *var, const char *value); -- 2.38.1 ``` ## René Scharfe, 2022-12-01 13:43 Subject: [PATCH v2 2/3] diff: let prep_parse_options() return parseopt array Message-ID: <090e9e6b-00ea-c23b-aa2f-6d9f3d79c22a@web.de> URL: https://gitlist.dev/e/090e9e6b-00ea-c23b-aa2f-6d9f3d79c22a%40web.de In-Reply-To: ``` prep_parse_options() sets the parseopts member of struct diff_options. Let it return the pointer instead and have its caller do the assignment instead and rename the function to get_diff_parseopts() to reflect that change. This allows using it in other places and with other variables. Signed-off-by: René Scharfe --- diff.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index e01129f0ea..e469d5d2a0 100644 --- a/diff.c +++ b/diff.c @@ -4615,7 +4615,7 @@ static void run_checkdiff(struct diff_filepair *p, struct diff_options *o) builtin_checkdiff(name, other, attr_path, p->one, p->two, o); } -static void prep_parse_options(struct diff_options *options); +static struct option *get_diff_parseopts(struct diff_options *options); void repo_diff_setup(struct repository *r, struct diff_options *options) { @@ -4663,7 +4663,7 @@ 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; - prep_parse_options(options); + options->parseopts = get_diff_parseopts(options); } static const char diff_status_letters[] = { @@ -5419,7 +5419,7 @@ static int diff_opt_rotate_to(const struct option *opt, const char *arg, int uns return 0; } -static void prep_parse_options(struct diff_options *options) +static struct option *get_diff_parseopts(struct diff_options *options) { struct option parseopts[] = { OPT_GROUP(N_("Diff output format options")), @@ -5689,8 +5689,7 @@ static void prep_parse_options(struct diff_options *options) OPT_END() }; - ALLOC_ARRAY(options->parseopts, ARRAY_SIZE(parseopts)); - memcpy(options->parseopts, parseopts, sizeof(parseopts)); + return parse_options_dup(parseopts); } struct option *add_diff_options(const struct option *parseopts, -- 2.38.1 ``` ## René Scharfe, 2022-12-01 13:44 Subject: [PATCH v2 3/3] diff: remove parseopts member from struct diff_options Message-ID: URL: https://gitlist.dev/e/ec70aee7-2cca-c184-930d-e61a6f4597c3%40web.de In-Reply-To: ``` 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(). Thus calling only repo_diff_setup() and release_revisions() leaks that array. We could free it 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 --- diff.c | 17 ++++++++--------- diff.h | 1 - 2 files changed, 8 insertions(+), 10 deletions(-) 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) diff --git a/diff.h b/diff.h index c20a1ad76d..41eb2c3d42 100644 --- a/diff.h +++ b/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; -- 2.38.1 ``` ## ZheNing Hu, 2022-12-01 14:11 Subject: Re: [PATCH 1/3] diff: factor out add_diff_options() Message-ID: URL: https://gitlist.dev/e/CAOLTT8SpF%3Di6Lwc28SWSDf2uOqoxLpMHiVS5GefwYcCHigL2NQ%40mail.gmail.com In-Reply-To: <19be5afa-63e8-5f11-c5f4-6f391677e4c4@web.de> ``` Hi, René Scharfe 于2022年12月1日周四 02:13写道： > > Add a function for appending the parseopts member of struct diff_options > to a struct option array. Use it in two sites instead of accessing the > parseopts member directly. Decoupling callers from diff internals like > that allows us to change the latter. > > Signed-off-by: René Scharfe > --- > builtin/range-diff.c | 2 +- > diff-no-index.c | 3 +-- > diff.c | 6 ++++++ > diff.h | 1 + > 4 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/builtin/range-diff.c b/builtin/range-diff.c > index e2a74efb42..aecfae12d3 100644 > --- a/builtin/range-diff.c > +++ b/builtin/range-diff.c > @@ -47,7 +47,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) > > repo_diff_setup(the_repository, &diffopt); > > - options = parse_options_concat(range_diff_options, diffopt.parseopts); > + options = add_diff_options(range_diff_options, &diffopt); > argc = parse_options(argc, argv, prefix, options, > builtin_range_diff_usage, PARSE_OPT_KEEP_DASHDASH); > > diff --git a/diff-no-index.c b/diff-no-index.c > index 18edbdf4b5..05fafd0019 100644 > --- a/diff-no-index.c > +++ b/diff-no-index.c > @@ -255,8 +255,7 @@ int diff_no_index(struct rev_info *revs, > }; > struct option *options; > > - options = parse_options_concat(no_index_options, > - revs->diffopt.parseopts); > + options = add_diff_options(no_index_options, &revs->diffopt); > argc = parse_options(argc, argv, revs->prefix, options, > diff_no_index_usage, 0); > if (argc != 2) { > diff --git a/diff.c b/diff.c > index 1054a4b732..e01129f0ea 100644 > --- a/diff.c > +++ b/diff.c > @@ -5693,6 +5693,12 @@ static void prep_parse_options(struct diff_options *options) > memcpy(options->parseopts, parseopts, sizeof(parseopts)); > } > > +struct option *add_diff_options(const struct option *parseopts, > + struct diff_options *options) > +{ > + return parse_options_concat(parseopts, options->parseopts); > +} > + This idea is very good. I am implementing `--scope` for git diff recently [1], but I don’t want this option to be directly inherited by commands such as git log, git format-patch, etc. It would be best if the option can be added dynamically... This patch looks like it can be used to do just that! :) > int diff_opt_parse(struct diff_options *options, > const char **av, int ac, const char *prefix) > { > diff --git a/diff.h b/diff.h > index fd33caeb25..c20a1ad76d 100644 > --- a/diff.h > +++ b/diff.h > @@ -539,6 +539,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb); > #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts) > #endif > void repo_diff_setup(struct repository *, struct diff_options *); > +struct option *add_diff_options(const struct option *, struct diff_options *); > int diff_opt_parse(struct diff_options *, const char **, int, const char *); > void diff_setup_done(struct diff_options *); > int git_config_rename(const char *var, const char *value); > -- > 2.38.1 Thanks! [1]: https://lore.kernel.org/git/pull.1398.v3.git.1669723221.gitgitgadget@gmail.com/ ``` ## Ævar Arnfjörð Bjarmason, 2022-12-01 16:54 Subject: Re: [PATCH v2 0/3] diff: build parseopts array on demand Message-ID: <221201.86r0xjgicc.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/221201.86r0xjgicc.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Thu, Dec 01 2022, René Scharfe wrote: > Calling repo_init_revisions() and release_revisions() in that order > leaks the memory allocated for the parseopts array in the embedded > struct diff_options member. Get rid of that leak by reducing the > lifetime of that array. > > Original patch: > https://lore.kernel.org/git/4fd82dc6-e0f8-0638-5b10-16bfef39a171@web.de/ > > Submitted separately from that thread because it's independent enough. > > Change since v1: > - Actually remove the parseopts member. Its removal got lost during > refactoring in v1. Thank you for spotting that, Junio! > > diff: factor out add_diff_options() > diff: let prep_parse_options() return parseopt array > diff: remove parseopts member from struct diff_options > > builtin/range-diff.c | 2 +- > diff-no-index.c | 3 +-- > diff.c | 26 +++++++++++++++----------- > diff.h | 2 +- > 4 files changed, 18 insertions(+), 15 deletions(-) > > Range-Diff gegen v1: > 1: 630f95320f = 1: 4dc8b2632b diff: factor out add_diff_options() > 2: 4b56fa795c = 2: 10903d355e diff: let prep_parse_options() return parseopt array > 3: 7e54e4370a ! 3: 24bd18ae79 diff: remove parseopts member from struct diff_options > @@ diff.c: void diff_free(struct diff_options *options) > } > > void diff_flush(struct diff_options *options) > + > + ## diff.h ## > +@@ diff.h: struct diff_options { > + unsigned color_moved_ws_handling; > + > + struct repository *repo; > +- struct option *parseopts; > + struct strmap *additional_path_headers; > + > + int no_free; This looks good to me. Would you mind running the tests with: GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true make SANITIZE=leak And then marking up the ones that now pass with TEST_PASSES_SANITIZE_LEAK=true. I think it's all except one of these (one isn't marked on "master", I forget which one): Test Summary Report ------------------- t1022-read-tree-partial-clone.sh (Wstat: 256 Tests: 1 Failed: 0) Non-zero exit status: 1 t2012-checkout-last.sh (Wstat: 256 Tests: 22 Failed: 0) Non-zero exit status: 1 t3210-pack-refs.sh (Wstat: 256 Tests: 30 Failed: 0) Non-zero exit status: 1 t4053-diff-no-index.sh (Wstat: 256 Tests: 19 Failed: 0) Non-zero exit status: 1 t5554-noop-fetch-negotiator.sh (Wstat: 256 Tests: 1 Failed: 0) Non-zero exit status: 1 t5613-info-alternate.sh (Wstat: 256 Tests: 13 Failed: 0) Non-zero exit status: 1 t6021-rev-list-exclude-hidden.sh (Wstat: 256 Tests: 42 Failed: 0) Non-zero exit status: 1 t6415-merge-dir-to-symlink.sh (Wstat: 256 Tests: 24 Failed: 0) Non-zero exit status: 1 t7403-submodule-sync.sh (Wstat: 256 Tests: 18 Failed: 0) Non-zero exit status: 1 t7504-commit-msg-hook.sh (Wstat: 256 Tests: 30 Failed: 0) Non-zero exit status: 1 t9115-git-svn-dcommit-funky-renames.sh (Wstat: 256 Tests: 12 Failed: 0) Non-zero exit status: 1 t9146-git-svn-empty-dirs.sh (Wstat: 256 Tests: 14 Failed: 0) Non-zero exit status: 1 t9160-git-svn-preserve-empty-dirs.sh (Wstat: 256 Tests: 12 Failed: 0) Non-zero exit status: 1 I.e. this makes a lot more tests pass leak-free, yay! To nitpick a bit: I didn't find that splitting this up into three patches helped to read it, e.g. 2/3 adds code that promptly goes away in 3/3. I also wondered why add two API functions for this, instead of just teaching the "prep options" to concat passed-in options with the user options? That also avoids a parse_options_dup(). I.e. with that squashed in the whole thing is: builtin/range-diff.c | 2 +- diff-no-index.c | 3 +-- diff.c | 19 ++++++++----------- diff.h | 3 ++- 4 files changed, 12 insertions(+), 15 deletions(-) diff --git a/builtin/range-diff.c b/builtin/range-diff.c index e2a74efb42a..8fcd6663b89 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -47,7 +47,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) repo_diff_setup(the_repository, &diffopt); - options = parse_options_concat(range_diff_options, diffopt.parseopts); + options = add_diff_parseopts(range_diff_options, &diffopt); argc = parse_options(argc, argv, prefix, options, builtin_range_diff_usage, PARSE_OPT_KEEP_DASHDASH); diff --git a/diff-no-index.c b/diff-no-index.c index 18edbdf4b59..efac1d38b38 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -255,8 +255,7 @@ int diff_no_index(struct rev_info *revs, }; struct option *options; - options = parse_options_concat(no_index_options, - revs->diffopt.parseopts); + options = add_diff_parseopts(no_index_options, &revs->diffopt); argc = parse_options(argc, argv, revs->prefix, options, diff_no_index_usage, 0); if (argc != 2) { diff --git a/diff.c b/diff.c index 1054a4b7329..e186fc91802 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 void prep_parse_options(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; - - prep_parse_options(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, @@ -5419,7 +5413,8 @@ static int diff_opt_rotate_to(const struct option *opt, const char *arg, int uns return 0; } -static void prep_parse_options(struct diff_options *options) +struct option *add_diff_parseopts(struct option *useropts, + struct diff_options *options) { struct option parseopts[] = { OPT_GROUP(N_("Diff output format options")), @@ -5689,22 +5684,25 @@ static void prep_parse_options(struct diff_options *options) OPT_END() }; - ALLOC_ARRAY(options->parseopts, ARRAY_SIZE(parseopts)); - memcpy(options->parseopts, parseopts, sizeof(parseopts)); + return parse_options_concat(useropts, parseopts); } int diff_opt_parse(struct diff_options *options, const char **av, int ac, const char *prefix) { + struct option no_options[] = { OPT_END() }; + struct option *diff_parseopts = add_diff_parseopts(no_options, 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; } @@ -6513,7 +6511,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) diff --git a/diff.h b/diff.h index fd33caeb25d..56704d3de22 100644 --- a/diff.h +++ b/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; @@ -539,6 +538,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb); #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts) #endif void repo_diff_setup(struct repository *, struct diff_options *); +struct option *add_diff_parseopts(struct option *useropts, + struct diff_options *options); int diff_opt_parse(struct diff_options *, const char **, int, const char *); void diff_setup_done(struct diff_options *); int git_config_rename(const char *var, const char *value); ``` ## René Scharfe, 2022-12-01 19:01 Subject: Re: [PATCH v2 0/3] diff: build parseopts array on demand Message-ID: <572e29a2-3f19-e946-d152-009999651084@web.de> URL: https://gitlist.dev/e/572e29a2-3f19-e946-d152-009999651084%40web.de In-Reply-To: <221201.86r0xjgicc.gmgdl@evledraar.gmail.com> ``` Am 01.12.2022 um 17:54 schrieb Ævar Arnfjörð Bjarmason: > > On Thu, Dec 01 2022, René Scharfe wrote: > >> Calling repo_init_revisions() and release_revisions() in that order >> leaks the memory allocated for the parseopts array in the embedded >> struct diff_options member. Get rid of that leak by reducing the >> lifetime of that array. >> >> Original patch: >> https://lore.kernel.org/git/4fd82dc6-e0f8-0638-5b10-16bfef39a171@web.de/ >> >> Submitted separately from that thread because it's independent enough. >> >> Change since v1: >> - Actually remove the parseopts member. Its removal got lost during >> refactoring in v1. Thank you for spotting that, Junio! >> >> diff: factor out add_diff_options() >> diff: let prep_parse_options() return parseopt array >> diff: remove parseopts member from struct diff_options >> >> builtin/range-diff.c | 2 +- >> diff-no-index.c | 3 +-- >> diff.c | 26 +++++++++++++++----------- >> diff.h | 2 +- >> 4 files changed, 18 insertions(+), 15 deletions(-) >> >> Range-Diff gegen v1: >> 1: 630f95320f = 1: 4dc8b2632b diff: factor out add_diff_options() >> 2: 4b56fa795c = 2: 10903d355e diff: let prep_parse_options() return parseopt array >> 3: 7e54e4370a ! 3: 24bd18ae79 diff: remove parseopts member from struct diff_options >> @@ diff.c: void diff_free(struct diff_options *options) >> } >> >> void diff_flush(struct diff_options *options) >> + >> + ## diff.h ## >> +@@ diff.h: struct diff_options { >> + unsigned color_moved_ws_handling; >> + >> + struct repository *repo; >> +- struct option *parseopts; >> + struct strmap *additional_path_headers; >> + >> + int no_free; > > This looks good to me. Would you mind running the tests with: > > GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true make SANITIZE=leak > > And then marking up the ones that now pass with > TEST_PASSES_SANITIZE_LEAK=true. I think it's all except one of these > (one isn't marked on "master", I forget which one): > > Test Summary Report > ------------------- > t1022-read-tree-partial-clone.sh (Wstat: 256 Tests: 1 Failed: 0) > Non-zero exit status: 1 > t2012-checkout-last.sh (Wstat: 256 Tests: 22 Failed: 0) > Non-zero exit status: 1 > t3210-pack-refs.sh (Wstat: 256 Tests: 30 Failed: 0) > Non-zero exit status: 1 > t4053-diff-no-index.sh (Wstat: 256 Tests: 19 Failed: 0) > Non-zero exit status: 1 > t5554-noop-fetch-negotiator.sh (Wstat: 256 Tests: 1 Failed: 0) > Non-zero exit status: 1 > t5613-info-alternate.sh (Wstat: 256 Tests: 13 Failed: 0) > Non-zero exit status: 1 > t6021-rev-list-exclude-hidden.sh (Wstat: 256 Tests: 42 Failed: 0) > Non-zero exit status: 1 > t6415-merge-dir-to-symlink.sh (Wstat: 256 Tests: 24 Failed: 0) > Non-zero exit status: 1 > t7403-submodule-sync.sh (Wstat: 256 Tests: 18 Failed: 0) > Non-zero exit status: 1 > t7504-commit-msg-hook.sh (Wstat: 256 Tests: 30 Failed: 0) > Non-zero exit status: 1 > t9115-git-svn-dcommit-funky-renames.sh (Wstat: 256 Tests: 12 Failed: 0) > Non-zero exit status: 1 > t9146-git-svn-empty-dirs.sh (Wstat: 256 Tests: 14 Failed: 0) > Non-zero exit status: 1 > t9160-git-svn-preserve-empty-dirs.sh (Wstat: 256 Tests: 12 Failed: 0) > Non-zero exit status: 1 > > I.e. this makes a lot more tests pass leak-free, yay! With -rc1 (i.e. without this series) I get: t1022-read-tree-partial-clone.sh (Wstat: 256 Tests: 1 Failed: 0) Non-zero exit status: 1 t2016-checkout-patch.sh (Wstat: 256 Tests: 16 Failed: 0) Non-zero exit status: 1 t2012-checkout-last.sh (Wstat: 256 Tests: 22 Failed: 0) Non-zero exit status: 1 t4023-diff-rename-typechange.sh (Wstat: 256 Tests: 4 Failed: 0) Non-zero exit status: 1 t4053-diff-no-index.sh (Wstat: 256 Tests: 19 Failed: 0) Non-zero exit status: 1 t4058-diff-duplicates.sh (Wstat: 256 Tests: 16 Failed: 0) Non-zero exit status: 1 t4205-log-pretty-formats.sh (Wstat: 256 Tests: 21 Failed: 0) Non-zero exit status: 1 Parse errors: No plan found in TAP output t5406-remote-rejects.sh (Wstat: 256 Tests: 3 Failed: 0) Non-zero exit status: 1 t5507-remote-environment.sh (Wstat: 256 Tests: 5 Failed: 0) Non-zero exit status: 1 t5554-noop-fetch-negotiator.sh (Wstat: 256 Tests: 1 Failed: 0) Non-zero exit status: 1 t6001-rev-list-graft.sh (Wstat: 256 Tests: 14 Failed: 0) Non-zero exit status: 1 t6021-rev-list-exclude-hidden.sh (Wstat: 256 Tests: 42 Failed: 0) Non-zero exit status: 1 t6401-merge-criss-cross.sh (Wstat: 256 Tests: 4 Failed: 0) Non-zero exit status: 1 t6407-merge-binary.sh (Wstat: 256 Tests: 3 Failed: 0) Non-zero exit status: 1 t6415-merge-dir-to-symlink.sh (Wstat: 256 Tests: 24 Failed: 0) Non-zero exit status: 1 t7006-pager.sh (Wstat: 256 Tests: 109 Failed: 0) Non-zero exit status: 1 t7008-filter-branch-null-sha1.sh (Wstat: 256 Tests: 6 Failed: 0) Non-zero exit status: 1 t7504-commit-msg-hook.sh (Wstat: 256 Tests: 30 Failed: 0) Non-zero exit status: 1 t7517-per-repo-email.sh (Wstat: 256 Tests: 16 Failed: 0) Non-zero exit status: 1 t7605-merge-resolve.sh (Wstat: 256 Tests: 4 Failed: 0) Non-zero exit status: 1 There is some overlap with your results, but also several differences. I wonder why so many more tests appear to be leak-free for me. I used Debian clang version 11.0.1-2. In any case it seems we need to update the marks before we can attribute which tests are made leak-free by any new patches. The TAP error in t4205-log-pretty-formats.sh is fixed by the following patch, but I can't explain it: diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index e448ef2928..0404491d6e 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -156,7 +156,7 @@ test_expect_success 'NUL termination with --reflog --pretty=oneline' ' for r in $revs do git show -s --pretty=oneline "$r" >raw && - cat raw | lf_to_nul || exit 1 + cat raw | lf_to_nul || return 1 done >expect && # the trailing NUL is already produced so we do not need to # output another one > To nitpick a bit: I didn't find that splitting this up into three > patches helped to read it, e.g. 2/3 adds code that promptly goes away in > 3/3. > > I also wondered why add two API functions for this, instead of just > teaching the "prep options" to concat passed-in options with the user > options? Only add_diff_options() is exported. get_diff_parseopts() is more flexible than a function that concatenates. Callers can choose the order, or not concatenate at all. An inter-diff would have been nice. Let's see if I can find the relevant parts. > That also avoids a parse_options_dup(). I.e. with that squashed > in the whole thing is: > > builtin/range-diff.c | 2 +- > diff-no-index.c | 3 +-- > diff.c | 19 ++++++++----------- > diff.h | 3 ++- > 4 files changed, 12 insertions(+), 15 deletions(-) > > diff --git a/builtin/range-diff.c b/builtin/range-diff.c > index e2a74efb42a..8fcd6663b89 100644 > --- a/builtin/range-diff.c > +++ b/builtin/range-diff.c > @@ -47,7 +47,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) > > repo_diff_setup(the_repository, &diffopt); > > - options = parse_options_concat(range_diff_options, diffopt.parseopts); > + options = add_diff_parseopts(range_diff_options, &diffopt); > argc = parse_options(argc, argv, prefix, options, > builtin_range_diff_usage, PARSE_OPT_KEEP_DASHDASH); > > diff --git a/diff-no-index.c b/diff-no-index.c > index 18edbdf4b59..efac1d38b38 100644 > --- a/diff-no-index.c > +++ b/diff-no-index.c > @@ -255,8 +255,7 @@ int diff_no_index(struct rev_info *revs, > }; > struct option *options; > > - options = parse_options_concat(no_index_options, > - revs->diffopt.parseopts); > + options = add_diff_parseopts(no_index_options, &revs->diffopt); > argc = parse_options(argc, argv, revs->prefix, options, > diff_no_index_usage, 0); > if (argc != 2) { > diff --git a/diff.c b/diff.c > index 1054a4b7329..e186fc91802 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 void prep_parse_options(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; > - > - prep_parse_options(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, > @@ -5419,7 +5413,8 @@ static int diff_opt_rotate_to(const struct option *opt, const char *arg, int uns > return 0; > } > > -static void prep_parse_options(struct diff_options *options) > +struct option *add_diff_parseopts(struct option *useropts, > + struct diff_options *options) You rename add_diff_options() to add_diff_parseopts(). OK. > { > struct option parseopts[] = { > OPT_GROUP(N_("Diff output format options")), > @@ -5689,22 +5684,25 @@ static void prep_parse_options(struct diff_options *options) > OPT_END() > }; > > - ALLOC_ARRAY(options->parseopts, ARRAY_SIZE(parseopts)); > - memcpy(options->parseopts, parseopts, sizeof(parseopts)); > + return parse_options_concat(useropts, parseopts); You bring back the concatenate from my demo patch. > } > > int diff_opt_parse(struct diff_options *options, > const char **av, int ac, const char *prefix) > { > + struct option no_options[] = { OPT_END() }; > + struct option *diff_parseopts = add_diff_parseopts(no_options, options); This kinda open-codes parse_options_dup(). The reduced flexibility of the concatenating variant is easily worked around here and it avoids the need for slightly awkward manual concatenation. Well, the get_diff_parseopts() variant feels cleaner, but baking the concat in is simpler overall. > + > 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; > } > @@ -6513,7 +6511,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) > diff --git a/diff.h b/diff.h > index fd33caeb25d..56704d3de22 100644 > --- a/diff.h > +++ b/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; > @@ -539,6 +538,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb); > #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts) > #endif > void repo_diff_setup(struct repository *, struct diff_options *); > +struct option *add_diff_parseopts(struct option *useropts, > + struct diff_options *options); > int diff_opt_parse(struct diff_options *, const char **, int, const char *); > void diff_setup_done(struct diff_options *); > int git_config_rename(const char *var, const char *value); ``` ## Eric Sunshine, 2022-12-01 19:19 Subject: Re: [PATCH v2 0/3] diff: build parseopts array on demand Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTdSNfWwN-c7R3-MF6ipAD6-7aVjxRK9f9XaKzg4yhhyg%40mail.gmail.com In-Reply-To: <572e29a2-3f19-e946-d152-009999651084@web.de> ``` On Thu, Dec 1, 2022 at 2:11 PM René Scharfe wrote: > t4205-log-pretty-formats.sh (Wstat: 256 Tests: 21 Failed: 0) > Non-zero exit status: 1 > Parse errors: No plan found in TAP output > > The TAP error in t4205-log-pretty-formats.sh is fixed by the following > patch, but I can't explain it: > > diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh > @@ -156,7 +156,7 @@ test_expect_success 'NUL termination with --reflog --pretty=oneline' ' > for r in $revs > do > git show -s --pretty=oneline "$r" >raw && > - cat raw | lf_to_nul || exit 1 > + cat raw | lf_to_nul || return 1 > done >expect && Makes sense. The `exit 1` undesirably causes the entire script to abort, which means test_done() is never invoked, whereas `return 1` makes only the test fail. `exit 1` would be appropriate inside a subshell but there is no subshell here. ``` ## René Scharfe, 2022-12-01 19:43 Subject: Re: [PATCH v2 0/3] diff: build parseopts array on demand Message-ID: <3d9ccbda-201b-fdaf-5fa3-224ba10b8c40@web.de> URL: https://gitlist.dev/e/3d9ccbda-201b-fdaf-5fa3-224ba10b8c40%40web.de In-Reply-To: ``` Am 01.12.2022 um 20:19 schrieb Eric Sunshine: > On Thu, Dec 1, 2022 at 2:11 PM René Scharfe wrote: >> t4205-log-pretty-formats.sh (Wstat: 256 Tests: 21 Failed: 0) >> Non-zero exit status: 1 >> Parse errors: No plan found in TAP output >> >> The TAP error in t4205-log-pretty-formats.sh is fixed by the following >> patch, but I can't explain it: >> >> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh >> @@ -156,7 +156,7 @@ test_expect_success 'NUL termination with --reflog --pretty=oneline' ' >> for r in $revs >> do >> git show -s --pretty=oneline "$r" >raw && >> - cat raw | lf_to_nul || exit 1 >> + cat raw | lf_to_nul || return 1 >> done >expect && > > Makes sense. The `exit 1` undesirably causes the entire script to > abort, which means test_done() is never invoked, whereas `return 1` > makes only the test fail. `exit 1` would be appropriate inside a > subshell but there is no subshell here. Right. Here's how I got confused: What's failing? "cat raw" works in the trash directory. lf_to_nul is not found. Of course, it's defined in test-lib-functions.sh. So source that. Then "cat raw | lf_to_nul" can be started and fails as expected. And it reports: "-bash: 4: Bad file descriptor". What?! lf_to_nul calls perl, and test-lib-functions.sh defines "perl" as a function that redirects to &4. Which is not open in my shell. But the confusion was at the beginning: Of course it's "git show" that's failing and triggering the exit from the test, not cat or lf_to_nul. Didn't even get to that point because I somehow thought the redirection magic was somehow broken. So the lesson is to start searching at the beginning, I guess.. René ``` ## Junio C Hamano, 2022-12-01 21:56 Subject: Re: [PATCH 3/3] diff: remove parseopts member of struct diff_options Message-ID: URL: https://gitlist.dev/e/xmqqiliuss8k.fsf%40gitster.g In-Reply-To: <20b5d6d1-ec09-e09f-4f92-5925fb9c96f8@web.de> ``` René Scharfe writes: > Am 01.12.22 um 02:25 schrieb Junio C Hamano: >> René Scharfe 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 >>> --- >>> 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; ``` ## René Scharfe, 2022-12-01 22:45 Subject: Re: [PATCH 3/3] diff: remove parseopts member of struct diff_options Message-ID: URL: https://gitlist.dev/e/fd51f518-0d74-2077-3e6b-4c796489503b%40web.de In-Reply-To: ``` Am 01.12.2022 um 22:56 schrieb Junio C Hamano: > René Scharfe 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é ``` ## René Scharfe, 2022-12-01 22:45 Subject: [PATCH v3 0/3] diff: build parseopts array on demand Message-ID: <19325420-5630-87bb-0c2c-362151214b36@web.de> URL: https://gitlist.dev/e/19325420-5630-87bb-0c2c-362151214b36%40web.de In-Reply-To: <0620b4a6-b80a-d8a0-5a35-857b9fe0c986@web.de> ``` Calling repo_init_revisions() and release_revisions() in that order leaks the memory allocated for the parseopts array in the embedded struct diff_options member. Get rid of that leak by reducing the lifetime of that array. Original patch: https://lore.kernel.org/git/4fd82dc6-e0f8-0638-5b10-16bfef39a171@web.de/ Submitted separately from that thread because it's independent enough. Changes since v2: - Inline get_diff_parseopts() to simplify the code. Change since v1: - Actually remove the parseopts member. Its removal got lost during refactoring in v1. Thank you for spotting that, Junio! diff: factor out add_diff_options() diff: use add_diff_options() in diff_opt_parse() diff: remove parseopts member from struct diff_options builtin/range-diff.c | 2 +- diff-no-index.c | 3 +-- diff.c | 19 ++++++++----------- diff.h | 2 +- 4 files changed, 11 insertions(+), 15 deletions(-) Interdiff against v2: diff --git a/diff.c b/diff.c index 6415c4dc2d..4dfe824c85 100644 --- a/diff.c +++ b/diff.c @@ -5413,7 +5413,8 @@ static int diff_opt_rotate_to(const struct option *opt, const char *arg, int uns return 0; } -static struct option *get_diff_parseopts(struct diff_options *options) +struct option *add_diff_options(const struct option *opts, + struct diff_options *options) { struct option parseopts[] = { OPT_GROUP(N_("Diff output format options")), @@ -5683,33 +5684,25 @@ static struct option *get_diff_parseopts(struct diff_options *options) OPT_END() }; - return parse_options_dup(parseopts); -} - -struct option *add_diff_options(const struct option *parseopts, - struct diff_options *options) -{ - struct option *diff_parseopts = get_diff_parseopts(options); - struct option *result = parse_options_concat(parseopts, diff_parseopts); - free(diff_parseopts); - return result; + return parse_options_concat(opts, parseopts); } int diff_opt_parse(struct diff_options *options, const char **av, int ac, const char *prefix) { - struct option *diff_parseopts = get_diff_parseopts(options); + struct option no_options[] = { OPT_END() }; + struct option *parseopts = add_diff_options(no_options, options); if (!prefix) prefix = ""; - ac = parse_options(ac, av, prefix, diff_parseopts, NULL, + ac = parse_options(ac, av, prefix, 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); + free(parseopts); return ac; } -- 2.30.2 ``` ## René Scharfe, 2022-12-01 22:49 Subject: [PATCH v3 1/3] diff: factor out add_diff_options() Message-ID: <31f0e4ed-4a31-2423-65f0-2f9f1f6c7df3@web.de> URL: https://gitlist.dev/e/31f0e4ed-4a31-2423-65f0-2f9f1f6c7df3%40web.de In-Reply-To: <19325420-5630-87bb-0c2c-362151214b36@web.de> ``` Add a function for appending the parseopts member of struct diff_options to a struct option array. Use it in two sites instead of accessing the parseopts member directly. Decoupling callers from diff internals like that allows us to change the latter. Signed-off-by: René Scharfe --- builtin/range-diff.c | 2 +- diff-no-index.c | 3 +-- diff.c | 6 ++++++ diff.h | 1 + 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/builtin/range-diff.c b/builtin/range-diff.c index e2a74efb42..aecfae12d3 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -47,7 +47,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix) repo_diff_setup(the_repository, &diffopt); - options = parse_options_concat(range_diff_options, diffopt.parseopts); + options = add_diff_options(range_diff_options, &diffopt); argc = parse_options(argc, argv, prefix, options, builtin_range_diff_usage, PARSE_OPT_KEEP_DASHDASH); diff --git a/diff-no-index.c b/diff-no-index.c index 18edbdf4b5..05fafd0019 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -255,8 +255,7 @@ int diff_no_index(struct rev_info *revs, }; struct option *options; - options = parse_options_concat(no_index_options, - revs->diffopt.parseopts); + options = add_diff_options(no_index_options, &revs->diffopt); argc = parse_options(argc, argv, revs->prefix, options, diff_no_index_usage, 0); if (argc != 2) { diff --git a/diff.c b/diff.c index 1054a4b732..f1cf13e8e7 100644 --- a/diff.c +++ b/diff.c @@ -5419,6 +5419,12 @@ static int diff_opt_rotate_to(const struct option *opt, const char *arg, int uns return 0; } +struct option *add_diff_options(const struct option *opts, + struct diff_options *options) +{ + return parse_options_concat(opts, options->parseopts); +} + static void prep_parse_options(struct diff_options *options) { struct option parseopts[] = { diff --git a/diff.h b/diff.h index fd33caeb25..c20a1ad76d 100644 --- a/diff.h +++ b/diff.h @@ -539,6 +539,7 @@ int git_diff_ui_config(const char *var, const char *value, void *cb); #define diff_setup(diffopts) repo_diff_setup(the_repository, diffopts) #endif void repo_diff_setup(struct repository *, struct diff_options *); +struct option *add_diff_options(const struct option *, struct diff_options *); int diff_opt_parse(struct diff_options *, const char **, int, const char *); void diff_setup_done(struct diff_options *); int git_config_rename(const char *var, const char *value); -- 2.30.2 ``` ## René Scharfe, 2022-12-01 22:51 Subject: [PATCH v3 2/3] diff: use add_diff_options() in diff_opt_parse() Message-ID: URL: https://gitlist.dev/e/fb901635-8058-856d-c7cd-6fef24329f62%40web.de In-Reply-To: <19325420-5630-87bb-0c2c-362151214b36@web.de> ``` Prepare the removal of the parseopts member of struct diff_options by using the API function add_diff_options() instead of accessing it directly to get the command line option definitions. Building the copy by concatenating with an empty option array is slightly awkward, but simpler than a non-concat version of add_diff_options() would be to use in places that need concatenation. Suggested-by: Ævar Arnfjörð Bjarmason Signed-off-by: René Scharfe --- diff.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index f1cf13e8e7..f6d85122a9 100644 --- a/diff.c +++ b/diff.c @@ -5702,15 +5702,19 @@ static void prep_parse_options(struct diff_options *options) int diff_opt_parse(struct diff_options *options, const char **av, int ac, const char *prefix) { + struct option no_options[] = { OPT_END() }; + struct option *parseopts = add_diff_options(no_options, options); + if (!prefix) prefix = ""; - ac = parse_options(ac, av, prefix, options->parseopts, NULL, + ac = parse_options(ac, av, prefix, 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(parseopts); return ac; } -- 2.30.2 ``` ## René Scharfe, 2022-12-01 22:53 Subject: [PATCH v3 3/3] diff: remove parseopts member from struct diff_options Message-ID: <3816dd61-ee61-94b4-a2b2-49ee07986c5b@web.de> URL: https://gitlist.dev/e/3816dd61-ee61-94b4-a2b2-49ee07986c5b%40web.de In-Reply-To: <19325420-5630-87bb-0c2c-362151214b36@web.de> ``` 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(). Thus calling only repo_diff_setup() and release_revisions() leaks that array. We could free it in release_revisions() as well to plug that leak, but there is a better way: Only build it when needed. Absorb prep_parse_options() into the last place that uses the parseopts member of struct diff_options, add_diff_parseopts(), and get rid of said member. Signed-off-by: René Scharfe --- diff.c | 15 +-------------- diff.h | 1 - 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/diff.c b/diff.c index f6d85122a9..4dfe824c85 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 void prep_parse_options(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; - - prep_parse_options(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, @@ -5421,11 +5415,6 @@ static int diff_opt_rotate_to(const struct option *opt, const char *arg, int uns struct option *add_diff_options(const struct option *opts, struct diff_options *options) -{ - return parse_options_concat(opts, options->parseopts); -} - -static void prep_parse_options(struct diff_options *options) { struct option parseopts[] = { OPT_GROUP(N_("Diff output format options")), @@ -5695,8 +5684,7 @@ static void prep_parse_options(struct diff_options *options) OPT_END() }; - ALLOC_ARRAY(options->parseopts, ARRAY_SIZE(parseopts)); - memcpy(options->parseopts, parseopts, sizeof(parseopts)); + return parse_options_concat(opts, parseopts); } int diff_opt_parse(struct diff_options *options, @@ -6523,7 +6511,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) diff --git a/diff.h b/diff.h index c20a1ad76d..41eb2c3d42 100644 --- a/diff.h +++ b/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; -- 2.30.2 ``` ## Ævar Arnfjörð Bjarmason, 2022-12-01 23:00 Subject: Re: [PATCH v2 0/3] diff: build parseopts array on demand Message-ID: <221202.86iliuhgk4.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/221202.86iliuhgk4.gmgdl%40evledraar.gmail.com In-Reply-To: <572e29a2-3f19-e946-d152-009999651084@web.de> ``` On Thu, Dec 01 2022, René Scharfe wrote: > Am 01.12.2022 um 17:54 schrieb Ævar Arnfjörð Bjarmason: >> >> On Thu, Dec 01 2022, René Scharfe wrote: >> >>> Calling repo_init_revisions() and release_revisions() in that order >>> leaks the memory allocated for the parseopts array in the embedded >>> struct diff_options member. Get rid of that leak by reducing the >>> lifetime of that array. >>> >>> Original patch: >>> https://lore.kernel.org/git/4fd82dc6-e0f8-0638-5b10-16bfef39a171@web.de/ >>> >>> Submitted separately from that thread because it's independent enough. >>> >>> Change since v1: >>> - Actually remove the parseopts member. Its removal got lost during >>> refactoring in v1. Thank you for spotting that, Junio! >>> >>> diff: factor out add_diff_options() >>> diff: let prep_parse_options() return parseopt array >>> diff: remove parseopts member from struct diff_options >>> >>> builtin/range-diff.c | 2 +- >>> diff-no-index.c | 3 +-- >>> diff.c | 26 +++++++++++++++----------- >>> diff.h | 2 +- >>> 4 files changed, 18 insertions(+), 15 deletions(-) >>> >>> Range-Diff gegen v1: >>> 1: 630f95320f = 1: 4dc8b2632b diff: factor out add_diff_options() >>> 2: 4b56fa795c = 2: 10903d355e diff: let prep_parse_options() return parseopt array >>> 3: 7e54e4370a ! 3: 24bd18ae79 diff: remove parseopts member from struct diff_options >>> @@ diff.c: void diff_free(struct diff_options *options) >>> } >>> >>> void diff_flush(struct diff_options *options) >>> + >>> + ## diff.h ## >>> +@@ diff.h: struct diff_options { >>> + unsigned color_moved_ws_handling; >>> + >>> + struct repository *repo; >>> +- struct option *parseopts; >>> + struct strmap *additional_path_headers; >>> + >>> + int no_free; >> >> This looks good to me. Would you mind running the tests with: >> >> GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true make SANITIZE=leak >> >> And then marking up the ones that now pass with >> TEST_PASSES_SANITIZE_LEAK=true. I think it's all except one of these >> (one isn't marked on "master", I forget which one): >> >> Test Summary Report >> ------------------- >> t1022-read-tree-partial-clone.sh (Wstat: 256 Tests: 1 Failed: 0) >> Non-zero exit status: 1 >> t2012-checkout-last.sh (Wstat: 256 Tests: 22 Failed: 0) >> Non-zero exit status: 1 >> t3210-pack-refs.sh (Wstat: 256 Tests: 30 Failed: 0) >> Non-zero exit status: 1 >> t4053-diff-no-index.sh (Wstat: 256 Tests: 19 Failed: 0) >> Non-zero exit status: 1 >> t5554-noop-fetch-negotiator.sh (Wstat: 256 Tests: 1 Failed: 0) >> Non-zero exit status: 1 >> t5613-info-alternate.sh (Wstat: 256 Tests: 13 Failed: 0) >> Non-zero exit status: 1 >> t6021-rev-list-exclude-hidden.sh (Wstat: 256 Tests: 42 Failed: 0) >> Non-zero exit status: 1 >> t6415-merge-dir-to-symlink.sh (Wstat: 256 Tests: 24 Failed: 0) >> Non-zero exit status: 1 >> t7403-submodule-sync.sh (Wstat: 256 Tests: 18 Failed: 0) >> Non-zero exit status: 1 >> t7504-commit-msg-hook.sh (Wstat: 256 Tests: 30 Failed: 0) >> Non-zero exit status: 1 >> t9115-git-svn-dcommit-funky-renames.sh (Wstat: 256 Tests: 12 Failed: 0) >> Non-zero exit status: 1 >> t9146-git-svn-empty-dirs.sh (Wstat: 256 Tests: 14 Failed: 0) >> Non-zero exit status: 1 >> t9160-git-svn-preserve-empty-dirs.sh (Wstat: 256 Tests: 12 Failed: 0) >> Non-zero exit status: 1 >> >> I.e. this makes a lot more tests pass leak-free, yay! > > With -rc1 (i.e. without this series) I get: > > t1022-read-tree-partial-clone.sh (Wstat: 256 Tests: 1 Failed: 0) > Non-zero exit status: 1 > t2016-checkout-patch.sh (Wstat: 256 Tests: 16 Failed: 0) > Non-zero exit status: 1 > t2012-checkout-last.sh (Wstat: 256 Tests: 22 Failed: 0) > Non-zero exit status: 1 > t4023-diff-rename-typechange.sh (Wstat: 256 Tests: 4 Failed: 0) > Non-zero exit status: 1 > t4053-diff-no-index.sh (Wstat: 256 Tests: 19 Failed: 0) > Non-zero exit status: 1 > t4058-diff-duplicates.sh (Wstat: 256 Tests: 16 Failed: 0) > Non-zero exit status: 1 > t4205-log-pretty-formats.sh (Wstat: 256 Tests: 21 Failed: 0) > Non-zero exit status: 1 > Parse errors: No plan found in TAP output > t5406-remote-rejects.sh (Wstat: 256 Tests: 3 Failed: 0) > Non-zero exit status: 1 > t5507-remote-environment.sh (Wstat: 256 Tests: 5 Failed: 0) > Non-zero exit status: 1 > t5554-noop-fetch-negotiator.sh (Wstat: 256 Tests: 1 Failed: 0) > Non-zero exit status: 1 > t6001-rev-list-graft.sh (Wstat: 256 Tests: 14 Failed: 0) > Non-zero exit status: 1 > t6021-rev-list-exclude-hidden.sh (Wstat: 256 Tests: 42 Failed: 0) > Non-zero exit status: 1 > t6401-merge-criss-cross.sh (Wstat: 256 Tests: 4 Failed: 0) > Non-zero exit status: 1 > t6407-merge-binary.sh (Wstat: 256 Tests: 3 Failed: 0) > Non-zero exit status: 1 > t6415-merge-dir-to-symlink.sh (Wstat: 256 Tests: 24 Failed: 0) > Non-zero exit status: 1 > t7006-pager.sh (Wstat: 256 Tests: 109 Failed: 0) > Non-zero exit status: 1 > t7008-filter-branch-null-sha1.sh (Wstat: 256 Tests: 6 Failed: 0) > Non-zero exit status: 1 > t7504-commit-msg-hook.sh (Wstat: 256 Tests: 30 Failed: 0) > Non-zero exit status: 1 > t7517-per-repo-email.sh (Wstat: 256 Tests: 16 Failed: 0) > Non-zero exit status: 1 > t7605-merge-resolve.sh (Wstat: 256 Tests: 4 Failed: 0) > Non-zero exit status: 1 > > There is some overlap with your results, but also several differences. > I wonder why so many more tests appear to be leak-free for me. I used > Debian clang version 11.0.1-2. Sorry to have sent you down this rabbit hole, I misrecalled that it was relatively clean on "master", but it's "next" that's in that state (due to ab/various-leak-fixes). > In any case it seems we need to update the marks before we can > attribute which tests are made leak-free by any new patches. Yes, let's leave it for now in this series. I think it would make sense if "master" was almost clean, but I'll just submit another change like e5e37517dd9 (tests: mark tests as passing with SANITIZE=leak, 2022-11-08) after this lands. ```