On Wed, May 07, 2025 at 02:33:21AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <[email protected]>

This commit is missing an explanation of the problem. When does it
trigger? Which subsystem triggers it? Why didn't we observe the issue
beforehand? And given that the solution seems to be a bit more complex
it should also explain how you're fixing the issue.

Furthermore, the subject of such a commit should be prefixed with the
subsystem in which you're fixing the issue. So in your case,
"parse-options:".

I'd recommend reading through "Documentation/MyFirstContribution.adoc",
which explains all of this.

> diff --git a/parse-options.c b/parse-options.c
> index a9a39ecaef6c..4279dfe4d313 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -638,6 +638,16 @@ static int has_subcommands(const struct option *options)
>  	return 0;
>  }
>  
> +static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {

Style: the opening brace should be on its own line.

> +	for (; options->type != OPTION_END; options++)
> +		;
> +	if (options->value && options->strdup_fn) {
> +		ctx->unknown_opts = options->value;
> +		ctx->strdup_fn = options->strdup_fn;
> +		return;
> +	}

It's not really clear what this is doing. Is the intent to only change
this this value for the last option? If so, why?

> @@ -981,7 +992,11 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
>  					 *
>  					 * This is leaky, too bad.
>  					 */
> -					ctx->argv[0] = xstrdup(ctx->opt - 1);
> +					if (ctx->unknown_opts && ctx->strdup_fn) {
> +						ctx->argv[0] = ctx->strdup_fn(ctx->unknown_opts, ctx->opt - 1);
> +					} else {
> +						ctx->argv[0] = xstrdup(ctx->opt - 1);
> +					}
>  					*(char *)ctx->argv[0] = '-';
>  					goto unknown;
>  				case PARSE_OPT_NON_OPTION:

Hm. I'm at a loss what we're solving here. All of this really should be
explained in the commit message to give context to the reviewer.

> diff --git a/parse-options.h b/parse-options.h
> index 91c3e3c29b3d..af06a09abb8e 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -388,6 +391,12 @@ static char *parse_options_noop_ignored_value MAYBE_UNUSED;
>  }
>  #define OPT_SUBCOMMAND(l, v, fn)    OPT_SUBCOMMAND_F((l), (v), (fn), 0)
>  
> +#define OPT_UNKNOWN(v, fn) { \
> +	.type = OPTION_END, \
> +	.value = (v), \
> +	.strdup_fn = (fn), \
> +}
> +
>  /*
>   * parse_options() will filter out the processed options and leave the
>   * non-option arguments in argv[]. argv0 is assumed program name and
> @@ -496,6 +505,9 @@ struct parse_opt_ctx_t {
>  	const char *prefix;
>  	const char **alias_groups; /* must be in groups of 3 elements! */
>  	struct parse_opt_cmdmode_list *cmdmode_list;
> +
> +	void *unknown_opts;
> +	parse_opt_strdup_fn *strdup_fn;
>  };
>  
>  void parse_options_start(struct parse_opt_ctx_t *ctx,

Okay. So the intent seems to be that we start storing any options that
we don't understand?

> diff --git a/t/helper/test-free-unknown-options.c b/t/helper/test-free-unknown-options.c
> new file mode 100644
> index 000000000000..9d658115ba8f
> --- /dev/null
> +++ b/t/helper/test-free-unknown-options.c
> @@ -0,0 +1,35 @@
> +#include "git-compat-util.h"
> +#include "parse-options.h"
> +#include "setup.h"
> +#include "strvec.h"
> +
> +static const char *const free_unknown_options_usage[] = {
> +    "test-tool free-unknown-options",
> +    NULL
> +};

Can't we expand `test-tool parse-options` instead of introducing a new
helper?

> +int cmd__free_unknown_options(int argc, const char **argv) {
> +    struct strvec *unknown_opts = xmalloc(sizeof(struct strvec));
> +    strvec_init(unknown_opts);
> +    const char *prefix = setup_git_directory();
> +
> +    bool a, b;
> +	struct option options[] = {
> +		OPT_BOOL('a', "test-a", &a, N_("option a, only for test use")),
> +	OPT_BOOL('b', "test-b", &b, N_("option b, only for test use")),
> +	OPT_UNKNOWN(unknown_opts, (parse_opt_strdup_fn *)&strvec_push),
> +	};
> +
> +    parse_options(argc, argv, prefix, options,
> +	free_unknown_options_usage, PARSE_OPT_KEEP_UNKNOWN_OPT);
> +
> +    printf("a = %s\n", a? "true": "false");
> +    printf("b = %s\n", b? "true": "false");
> +
> +    int i;
> +    for (i = 0; i < unknown_opts->nr; i++) {
> +	printf("free unknown option: %s\n", unknown_opts->v[i]);
> +    }
> +    strvec_clear(unknown_opts);
> +    free(unknown_opts);
> +}
> \ No newline at end of file

Missing newline.

> diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
> index 6d62a5b53d95..33fa7828b9f6 100644
> --- a/t/helper/test-tool.h
> +++ b/t/helper/test-tool.h
> @@ -44,6 +44,7 @@ int cmd__parse_options(int argc, const char **argv);
>  int cmd__parse_options_flags(int argc, const char **argv);
>  int cmd__parse_pathspec_file(int argc, const char** argv);
>  int cmd__parse_subcommand(int argc, const char **argv);
> +int cmd__free_unknown_options(int argc, const char **argv);
>  int cmd__partial_clone(int argc, const char **argv);
>  int cmd__path_utils(int argc, const char **argv);
>  int cmd__path_walk(int argc, const char **argv);
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index ca55ea8228c3..773f54103fd3 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -822,4 +822,18 @@ test_expect_success 'u16 limits range' '
>  	test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
>  '
>  
> +cat >expect <<\EOF
> +a = true
> +b = true
> +free unknown option: -c
> +free unknown option: -d
> +EOF

This should be done inside `test_expect_success` itself.

> +test_expect_success 'free unknown options' '
> +	test-tool free-unknown-options -ac -bd \
> +	>output 2>output.err &&
> +	test_cmp expect output &&
> +	test_must_be_empty output.err
> +'
> +
>  test_done

Okay, so the memory leak seems to happen when handling unknown options
indeed, as I started to expect after the middle of this patch. In any
case, this commit really needs to be polished to have a proper commit
message that sets the stage for the reviewer.

Patrick

On Wed, May 07, 2025 at 02:33:22AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <[email protected]>
> 
> Signed-off-by: Lidong Yan <[email protected]>

The same is true here -- this needs to have a proper commit subject and
message.

> diff --git a/parse-options.c b/parse-options.c
> index 4279dfe4d313..12721b83000a 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -638,7 +638,9 @@ static int has_subcommands(const struct option *options)
>  	return 0;
>  }
>  
> -static void set_strdup_fn(struct parse_opt_ctx_t *ctx, const struct option *options) {
> +static void set_strdup_fn(struct parse_opt_ctx_t *ctx,
> +			  const struct option *options)
> +{
>  	for (; options->type != OPTION_END; options++)
>  		;
>  	if (options->value && options->strdup_fn) {

Please refrain from doing code style cleanups of code you have just
introduced in the preceding commit.

Patrick

On Wed, May 07, 2025 at 02:33:23AM +0000, Lidong Yan via GitGitGadget wrote:
> From: Lidong Yan <[email protected]>
> 
> Signed-off-by: Lidong Yan <[email protected]>

This looks like another fixup-style commit. I assume that all of this
really should be a single commit, as the latter two commits don't seem
to do anything new compared to the first commit.

Patrick

Thank you for your suggestion. I will squash all the commits and include a detailed description in the next submission.

In short, calling parse_options may cause a memory leak when encountering unknown short options. To fix the leak, parse_options can use a user-provided allocation function, so that the user can later free the memory and avoid the leak. I also defined a macro called OPT_UNKNOWN to make it easier for users to pass in their own allocation function.

On the other hand, I think a new file should be added under t/helper/. The reason is that this new file not only serves as a test, but also provides a simple example of using OPT_UNKNOWN, which others can refer to when modifying their own option definitions to avoid memory leaks.

> 2025年5月7日 15:54，Patrick Steinhardt <[email protected]> 写道：
> 
> On Wed, May 07, 2025 at 02:33:23AM +0000, Lidong Yan via GitGitGadget wrote:
>> From: Lidong Yan <[email protected]>
>> 
>> Signed-off-by: Lidong Yan <[email protected]>
> 
> This looks like another fixup-style commit. I assume that all of this
> really should be a single commit, as the latter two commits don't seem
> to do anything new compared to the first commit.
> 
> Patrick
>