Re: [PATCH] Teach/Fix git-pull/git-merge --quiet and --verbose

From: Tuncer Ayaz <tuncer.ayaz@gmail.com>
Date: Oct 16, 2008, 20:08 UTC
Message-ID: <4ac8254d0810161308q3d463850k69e4f5615a974367@mail.gmail.com>
In-Reply-To: <7vtzbdjcb8.fsf@gitster.siamese.dyndns.org>

On Thu, Oct 16, 2008 at 8:15 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>
>> On Thu, Oct 16, 2008 at 2:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>>>
>>>> On Wed, Oct 15, 2008 at 9:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>>> "Tuncer Ayaz" <tuncer.ayaz@gmail.com> writes:
>>>>>
>> Would you prefer to leave -v out?
>
> Not at all.
>
> Perhaps there is a deeper misunderstanding.

Perhaps there was one :-)

Show 9 quoted lines

> It makes perfect sense _at the end user interface level_ to have -v and -q
> as two separate options, perhaps with "later one wins" semantics.  Another
> possible semantics is "-q and -v are mutually incompatible", but I think
> "later one wins" makes it much more usable from the end user's point of view.
>
> The only thing I was objecting to was your repeated (verbose || !quiet)
> expression in the _implementation_, which would have been much easier to
> read and maintain, if it were expressed as a single variable "verbosity"
> that can have one of three values.

This leaves no space for speculation and is as clear as it gets :D

Show 18 quoted lines

> IOW,
>
>        static enum { QUIET, NORMAL, VERBOSE } verbosity = NORMAL;
>        ...
>
>                if (!strcmp("--quiet", arg))
>                        verbosity = QUIET;
>                else if (!strcmp("--verbose", arg))
>                        verbosity = VERBOSE;
>                else ...
>
>        ...
>                if (verbosity > QUIET)
>                        print informational message;
>                if (verbosity > NORMAL)
>                        print verbose message;
>
> See?

Yeah, no problem with that. How do you propose we should integrate this with the existing usage of parse_options() and OPT_ macros? I want to keep using it and not redo argv handling from scratch in builtin-fetch/builtin-merge.