git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 0/3] diff: build parseopts array on demand

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Dec 1, 2022, 16:54 UTC
Message-ID
<221201.86r0xjgicc.gmgdl@evledraar.gmail.com>
In-Reply-To
<ea838ae7-b635-d4d2-d9df-e96b3d8980af@web.de>
On Thu, Dec 01 2022, René Scharfe wrote:
Show 42 quoted lines
> 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);
Previous: René ScharfeNext: René Scharfe
Message 15 of 23 in “diff: build parseopts array on demand”
  1. 0/3 diff: build parseopts array on demandRené Scharfe, Nov 30, 2022
  2. 1/3 diff: factor out add_diff_options()René Scharfe, Nov 30, 2022
  3. ZheNing HuDec 1, 2022
  4. 2/3 diff: let prep_parse_options() return parseopt arrayRené Scharfe, Nov 30, 2022
  5. 3/3 diff: remove parseopts member of struct diff_optionsRené Scharfe, Nov 30, 2022
  6. Junio C HamanoDec 1, 2022
  7. René ScharfeDec 1, 2022
  8. Junio C HamanoDec 1, 2022
  9. René ScharfeDec 1, 2022
  10. Junio C HamanoDec 1, 2022
  11. 0/3 diff: build parseopts array on demandRené Scharfe, Dec 1, 2022
  12. 1/3 diff: factor out add_diff_options()René Scharfe, Dec 1, 2022
  13. 2/3 diff: let prep_parse_options() return parseopt arrayRené Scharfe, Dec 1, 2022
  14. 3/3 diff: remove parseopts member from struct diff_optionsRené Scharfe, Dec 1, 2022
  15. Ævar Arnfjörð BjarmasonDec 1, 2022
  16. René ScharfeDec 1, 2022
  17. Eric SunshineDec 1, 2022
  18. René ScharfeDec 1, 2022
  19. Ævar Arnfjörð BjarmasonDec 1, 2022
  20. 0/3 diff: build parseopts array on demandRené Scharfe, Dec 1, 2022
  21. 1/3 diff: factor out add_diff_options()René Scharfe, Dec 1, 2022
  22. 2/3 diff: use add_diff_options() in diff_opt_parse()René Scharfe, Dec 1, 2022
  23. 3/3 diff: remove parseopts member from struct diff_optionsRené Scharfe, Dec 1, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.