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

Shourya Shukla [off-list ref] writes:

On 04/05 08:55, Junio C Hamano wrote: 

quoted

quoted

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