Johannes Schindelin <johannes.schindelin@xxxxxx> writes:
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 7365559..fce9c75 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -174,6 +174,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
>
> if (argc > 1)
> usage_with_options(usage_str, options);
> +
> + /* These option values will be free()d */
> + if (opts->gpg_sign)
> + opts->gpg_sign = xstrdup(opts->gpg_sign);
> + if (opts->strategy)
> + opts->strategy = xstrdup(opts->strategy);
> }
This certainly is good, but I wonder if a new variant of OPT_STRING
and OPTION_STRING that does the strdup for you, something along the
lines of ...
diff --git a/parse-options.c b/parse-options.c
index 312a85dbde..6aab6b0b05 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -138,6 +138,21 @@ static int get_value(struct parse_opt_ctx_t *p,
return get_arg(p, opt, flags, (const char **)opt->value);
return 0;
+ case OPTION_STRDUP:
+ err = 0;
+ free(opt->value);
+ if (unset)
+ *(const char **)opt->value = NULL;
+ else if (opt->flags & PARSE_OPT_OPTARG && !p->opt)
+ *(const char **)opt->value = xstrdup(opt->defval);
+ else {
+ const char *v;
+ err = get_arg(p, opt, flags, &v);
+ if (!err)
+ *(const char **)opt->value = xstrdup(v);
+ }
+ return err;
+
case OPTION_FILENAME:
err = 0;
if (unset)
... may make it even more pleasant to use? Only for two fields in
this patch that may probably be an overkill, but we may eventually
benefit from such an approach when we audit and plug leaks in
parse-options users. I dunno.
It is a sign that the caller wants to _own_ the memory to mark a
variable or field with OPTION_STRDUP, which is why I added the
free() at the beginning there.
The remainder of this patch looks sensible. The code that frees
these fields when we are done with the struct (and when we are
re-assigning) looked all good.