Re: [PATCH 02/14] pull: pass verbosity, --progress flags to fetch and merge
- From
Paul Tan <pyokagan@gmail.com>
- Date
- May 21, 2015, 09:48 UTC
- Message-ID
- <CACRoPnRVcjcegRU8J6a=X6uN=b7fhJyD9=js4LqJ7ORX6gT=jg@mail.gmail.com>
- In-Reply-To
- <4213f4fa8fb52fb020c2e9b5d78fbf7b@www.dscho.org>
Hi,
On Tue, May 19, 2015 at 1:41 AM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
Show 19 quoted lines
> On 2015-05-18 17:05, Paul Tan wrote: >> 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:
Yeah, I was also thinking if the callback could be generalized as well.
Specifically, I wonder if we should re-construct just the last-provided option (the current solution), or all the options (e.g. something like OPT_STRING_LIST). I'm currently working on the rewrite for git-am.sh as well, and it turns out it makes heavy use of the latter, so we probably should support both.
Show 12 quoted lines
> /** > * 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. > */ >
Heh, this docstring reads much better than mine.
> 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).
Unfortunately, the usage of strbuf means that we lose the ability to know if an option was not provided at all (the value is NULL). This is important as some of these options are used to override the default configured behavior.
> You might also want to verify that arg is `NULL` when `unset != 0`. Something like this:
Hmm, would there be a situation where arg is NULL when `unset != 0`? At first glance in the source code, it won't unless PARSE_OPT_LASTARG_DEFAULT is set, I guess. Anyway, there's no harm in being careful, though I think it's more likely that an invalid pointer is passed in through "arg", which we can't detect :-(.
Show 27 quoted lines
> 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;
> }
>
>> 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...Ugh. I think this shows that relying on "defval" is too error-prone. Anyway, it is only required for the options -n (which maps to --no-stat), and --summary (which maps to --stat), so we should probably make it such that if defval is not provided, we fallback to using long_name or short_name.
Show 15 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 ;-)Well, my thinking was that a negative verbosity *is* quietness ;-).
Thanks, Paul