Re: [RFC PATCH] Make rev-parse -q and --is-* quiet

From: Erlend Aasland <erlend-a@innova.no>
Date: Mar 13, 2020, 19:10 UTC
Message-ID: <CAB5DABE-BBBE-4E1A-83FE-E2840714CA7C@innova.no>
In-Reply-To: <xmqqa74kce4t.fsf@gitster.c.googlers.com>

Thank you all for your thorough reviews and constructive comments.

Many of you comment on the same flaws in my patch; I’ll address all of them in the event of a second version of this patch, if you all this this feature is worth it.

Issue 1) Naming I’ll rename the is_set variables according to Abhishek’s proposal (is_inline, is_shallow, etc.)

Issue 2) Premature return Yes, I chose the easiest way out, since this was a RFC :) One possibility could be to AND or OR the —is-* options together, but I think going with Junio’s proposal is the most sane thing to do:

On 13 Mar 2020, at 19:18, Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

> - when --quiet is in use, make --is-* mutually exclusive and die
>  when more than one of them is given.  I think any of the --is-*,
>  when used with --quiet, should also be an error if there are revs
>  on the command line (e.g. "git rev-parse --is-inside-git-dir
>  HEAD" is OK, but not with "--quiet").

Issue 3) Exit code I think going with Junio’s proposal (0 => true, 1 => false, 2 => error) is ok.

I’ll have another go at it, if you all think it’s worth it. If so, I’ll also add docs and unit tests.

Erlend

Show 30 quoted lines

> On 13 Mar 2020, at 19:18, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Jeff King <peff@peff.net> writes:
> 
>> I'm not sure if returning a single is_set makes sense, though. It
>> effectively becomes an OR, and you wouldn't know which flag triggered
>> it. It would make more sense to me for the invocation above to simply be
>> an error, reminding the caller that they need to handle it more
>> carefully.
>> 
>> We _could_ encode each value into the exit code (e.g., set bit 1 if the
>> first condition was true, and so on). But checking that becomes as much
>> hassle as reading stdout, so there's little value.
> 
> None of the above excites me.  I do not think it makes much sense to
> combine -q with --is-* for the reasons you stated already (i.e. the
> caller cannot tell between "failure" and "false") in the first
> place, but if we must do this:
> 
> - reserve 0 (true) and 1 (false) for successful exit and use 2 (or
>   above) for other failures;
> 
> - when --quiet is in use, make --is-* mutually exclusive and die
>   when more than one of them is given.  I think any of the --is-*,
>   when used with --quiet, should also be an error if there are revs
>   on the command line (e.g. "git rev-parse --is-inside-git-dir
>   HEAD" is OK, but not with "--quiet").
> 
> is the minimum that makes me feel that we have semi-reasonable
> behaviour that can be explained to end users.