Hi,
On Tue, May 19, 2015 at 1:41 AM, Johannes Schindelin
<johannes.schindelin@xxxxxx> wrote:
> 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.
> /**
> * 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 :-(.
> 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.
>> /**
>> + * 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html