Re: [PATCH 02/14] pull: pass verbosity, --progress flags to fetch and merge

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 18, 2015, 17:41 UTC
Message-ID: <4213f4fa8fb52fb020c2e9b5d78fbf7b@www.dscho.org>
In-Reply-To: <1431961571-20370-3-git-send-email-pyokagan@gmail.com>

Hi Paul,

On 2015-05-18 17:05, Paul Tan wrote:

Show 16 quoted lines

> diff --git a/builtin/pull.c b/builtin/pull.c
> index 0b771b9..a4d9c92 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -11,16 +11,64 @@
>  #include "argv-array.h"
>  #include "run-command.h"
>  
> +/**
> + * Given an option opt, where opt->value points to a char* and opt->defval is a
> + * string, sets opt->value to the evaluation of "--$defval=$arg". If `arg` is
> + * NULL, then opt->value is set to "--$defval". If unset is true, then
> + * opt->value is set to "--no-$defval".
> + */
> +static int parse_opt_passthru(const struct option *opt, const char
> *arg, int unset)

How about adding this to parse-options-cb.c in a separate patch? I guess the description could say something like:

/**
 * Given an option opt, recreate the command-line option, as strbuf. This is useful
 * when a command needs to parse a command-line option in order to pass it to yet
 * another command. This callback can be used in conjunction with the
 * PARSE_OPT_(OPTARG|NOARG|NONEG) options, but not with PARSE_OPT_LASTARG_DEFAULT
 * because it expects `defval` to be the name of the command-line option to
 * reconstruct.
 *
 * The result will be stored in opt->value, which is expected to be a pointer to an
 * strbuf.
 */

Implied by my suggested description, I also propose to put the re-recreated command-line option into a strbuf instead of a char *, to make memory management easier (read: avoid memory leaks).

You might also want to verify that arg is `NULL` when `unset != 0`. Something like this:

int parse_opt_passthru(const struct option *opt, const char *arg, int unset)
{
	struct strbuf *buf = opt->value;

	assert(opt->defval);
	strbuf_reset(buf);
	if (unset) {
		assert(!arg);
		strbuf_addf(buf, "--no-%s", opt->defval);
	}
	else {
		strbuf_addf(buf, "--%s", opt->defval);
		if (arg)
			strbuf_addf(buf, "=%s", arg);
	}

	return 0;
}

Show 6 quoted lines

>  static struct option pull_options[] = {
> +	/* Shared options */
> +	OPT__VERBOSITY(&opt_verbosity),
> +	{ OPTION_CALLBACK, 0, "progress", &opt_progress, NULL,
> +	  N_("force progress reporting"),
> +	  PARSE_OPT_NOARG, parse_opt_passthru},

I *think* there is a '(intptr_t) "progress"' missing...

Show 13 quoted lines

>  /**
> + * Pushes "-q" or "-v" switches into arr to match the opt_verbosity level.
> + */
> +static void argv_push_verbosity(struct argv_array *arr)
> +{
> +	int verbosity;
> +
> +	for (verbosity = opt_verbosity; verbosity > 0; verbosity--)
> +		argv_array_push(arr, "-v");
> +
> +	for (verbosity = opt_verbosity; verbosity < 0; verbosity++)
> +		argv_array_push(arr, "-q");
> +}

Hmm... I guess this is *really* nit-picky, but I'd rather use "i" instead of "verbosity" because the second loop is about quietness instead of verbosity ;-)

Ciao, Dscho