Re: [PATCH v2 02/19] parse-options-cb: implement parse_opt_pass_argv_array()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 9, 2015, 23:16 UTC
Message-ID: <xmqqr3pkwjz6.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433314143-4478-3-git-send-email-pyokagan@gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 49 quoted lines

> Certain git commands, such as git-pull, are simply wrappers around other
> git commands like git-fetch, git-merge and git-rebase. As such, these
> wrapper commands will typically need to "pass through" command-line
> options of the commands they wrap.
>
> Implement the parse_opt_pass_argv_array() parse-options callback, which
> will reconstruct all the provided command-line options into an
> argv_array, such that it can be passed to another git command. This is
> useful for passing command-line options that can be specified multiple
> times.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> Notes:
>     v2
>     
>     * This function is a requirement for the rewrite of git-am to handle
>       passing git-apply's options to git-apply. Since it would be
>       implemented anyway I thought it would be good if git-pull could take
>       advantage of it as well to handle --strategy and --strategy-option.
>
>  parse-options-cb.c | 32 ++++++++++++++++++++++++++++++++
>  parse-options.h    |  1 +
>  2 files changed, 33 insertions(+)
>
> diff --git a/parse-options-cb.c b/parse-options-cb.c
> index 5b1dbcf..7330506 100644
> --- a/parse-options-cb.c
> +++ b/parse-options-cb.c
> @@ -4,6 +4,7 @@
>  #include "commit.h"
>  #include "color.h"
>  #include "string-list.h"
> +#include "argv-array.h"
>  
>  /*----- some often used options -----*/
>  
> @@ -163,3 +164,34 @@ int parse_opt_pass_strbuf(const struct option *opt, const char *arg, int unset)
>  
>  	return 0;
>  }
> +
> +/**
> + * For an option opt, recreate the command-line option, appending it to
> + * opt->value which must be a argv_array. This is useful when we need to pass
> + * the command-line option, which can be specified multiple times, to another
> + * command.
> + */

Almost the same comment as 01/19 applies to this comment.

I think it makes good sense to have two variants, one that lets the last one win and pass only that last one (i.e. 01/19) and the other that accumulates them into an argv_array (i.e. this one). But it feels iffy, given that the "acculate" version essentially creates an array of (char *), to make "the last one wins, leaving a single string" to use strbuf. I'd find it much more understandable if 01/19 took (char **) as opt->value instead of a strbuf.

In any case, these two need to be added as a related pair to the API documentation.

Thanks.

Show 36 quoted lines

> +int parse_opt_pass_argv_array(const struct option *opt, const char *arg, int unset)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	struct argv_array *opt_value = opt->value;
> +
> +	if (opt->long_name) {
> +		strbuf_addstr(&sb, unset ? "--no-" : "--");
> +		strbuf_addstr(&sb, opt->long_name);
> +		if (arg) {
> +			strbuf_addch(&sb, '=');
> +			strbuf_addstr(&sb, arg);
> +		}
> +	} else if (opt->short_name && !unset) {
> +		strbuf_addch(&sb, '-');
> +		strbuf_addch(&sb, opt->short_name);
> +		if (arg)
> +			strbuf_addstr(&sb, arg);
> +	} else
> +		return -1;
> +
> +	argv_array_push(opt_value, sb.buf);
> +	strbuf_release(&sb);
> +	return 0;
> +}
> diff --git a/parse-options.h b/parse-options.h
> index 1d21398..b663f87 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -225,6 +225,7 @@ extern int parse_opt_tertiary(const struct option *, const char *, int);
>  extern int parse_opt_string_list(const struct option *, const char *, int);
>  extern int parse_opt_noop_cb(const struct option *, const char *, int);
>  extern int parse_opt_pass_strbuf(const struct option *, const char *, int);
> +extern int parse_opt_pass_argv_array(const struct option *, const char *, int);
>  
>  #define OPT__VERBOSE(var, h)  OPT_COUNTUP('v', "verbose", (var), (h))
>  #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))