Shourya Shukla <shouryashukla.oo@xxxxxxxxx> 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.
