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

From: Junio C Hamano <gitster@pobox.com>
Date: May 4, 2020, 19:14 UTC
Message-ID: <xmqq368fh5s6.fsf@gitster.c.googlers.com>
In-Reply-To: <20200504173936.GA8919@konoha>

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

Show 14 quoted lines

> 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.