Re: [PATCH v3] submodule: port subcommand 'set-url' from shell to C

[Junio C Hamano <gitster@pobox.com>]

Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> On 04/05 08:55, Junio C Hamano wrote: 
>> > +	argc = parse_options(argc, argv, prefix, set_url_options,
>> > +			     usage, 0);
>> > +
>> > +	if (quiet)
>> > +		quiet |= OPT_QUIET;
>> 
>> This is bogus.  "command --quiet --quiet" would count-up quiet twice
>> and would make it 2, and you or OPT_QUIET==1 in to make it 3, but
>> your intention is quite clear that you want to pass 1 to
>> sync_submodule() in such a case.
>
> This is a grave mistake from my side. Though I do not understand how
> will `quiet` be counted twice.

The way I read the definition of OPT__QUIET() 

    #define OPT__QUIET(var, h)    OPT_COUNTUP('q', "quiet",   (var), (h))

is that it is OPT_COUNTUP() in disguise, and that is designed to
yield increased quietness when "-q" is given more than once.

> The fix you suggested (quiet ? OPT_QUIET : 0), we use this because we
> want to ensure `quiet` goes into sync as either 1/0 right? Not any other
> non-zero positive integer right?

The "if (quiet) quiet |= OPT_QUIET" does not make *any* sense, if
you are expecting quiet to be set to 1 or left as 0 as initialized
by parse_options() API.  You are defeating the whole point of using
preprocessor macro OPT_QUIET, as the correctness of the construct
heavily rely on OPT_QUIET defined to be 1.  If for any reason the
preprocessor macro gets redefined to 8, writing

	quiet ? OPT_QUIET : 0

would need *no* adjustment, while "if (quiet) quiet |= OPT_QUIET"
would require fixing.