threads / rfc / 52990

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

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

## tl;dr

5 messages between Mar 13, 2020 and Mar 13, 2020. Diffs are folded; open one to read it.

replies: 4people: 5as markdown or json

Abhishek Kumar· Mar 13, 2020, 17:30 UTC · lore
Show 23 quoted lines
> If rev-parse is called with both -q and an --is-* option, the result is
> provided as the return code of the command, iso. printed to stdout.
>
> This simplifies using these queries in shell scripts:
> git rev-parse --is-bare-repository && do_stuff
> git rev-parse --is-shallow-repository && do_stuff
>
> Signed-off-by: Erlend E. Aasland <erlend.aasland@innova.no>
> ---
> builtin/rev-parse.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 7a00da8203..5a8b404ec7 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -874,24 +874,31 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>                 continue;
>             }
>             if (!strcmp(arg, "--is-inside-git-dir")) {
> -                printf("%s\n", is_inside_git_dir() ? "true"
> -                        : "false");
> +                int is_set = is_inside_git_dir();

Avoid declaration after statement. Move is_set to the beginning of cmd_rev_parse().

Also, be more specific than "is_set". A variable like "is_inside" would be more appropriate.

> +                if (quiet)
> +                    return is_set ? 0 : 1;

Returning prematurely might be a bad idea. If there are more options after "--is-inside-git-dir", they will be not executed. You can maybe rewrite this as:

```
             if (!strcmp(arg, "--is-inside-git-dir")) {
                is_set = is_inside_git_dir();
                if (!quiet)
                        printf("%s\n", is_set ? "true"
                            : "false");
                 continue;
             }
```
And then return the value at the end of function, replacing '0' with !is_set.
Same comment for other blocks.
Show 31 quoted lines
>             if (!strcmp(arg, "--is-inside-work-tree")) {
> -                printf("%s\n", is_inside_work_tree() ? "true"
> -                        : "false");
> +                int is_set = is_inside_work_tree();
> +                if (quiet)
> +                    return is_set ? 0 : 1;
> +                printf("%s\n", is_set ? "true" : "false");
>                 continue;
>             }
>             if (!strcmp(arg, "--is-bare-repository")) {
> -                printf("%s\n", is_bare_repository() ? "true"
> -                        : "false");
> +                int is_set = is_bare_repository();
> +                if (quiet)
> +                    return is_set ? 0 : 1;
> +                printf("%s\n", is_set ? "true" : "false");
>                 continue;
>             }
>             if (!strcmp(arg, "--is-shallow-repository")) {
> -                printf("%s\n",
> -                        is_repository_shallow(the_repository) ? "true"
> -                        : "false");
> +                int is_set = is_repository_shallow(the_repository);
> +                if (quiet)
> +                    return is_set ? 0 : 1;
> +                printf("%s\n", is_set ? "true" : "false");
>                 continue;
>            }
>            if (!strcmp(arg, "--shared-index-path")) {
> --
> 2.25.1

I am wondering if we should stop the script from running if both quiet and --is-* options are passed.

Regards Abhishek

Jeff King· Mar 13, 2020, 17:47 UTC · re: Abhishek Kumar · lore
On Fri, Mar 13, 2020 at 11:00:00PM +0530, Abhishek Kumar wrote:
Show 10 quoted lines
> > @@ -874,24 +874,31 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >                 continue;
> >             }
> >             if (!strcmp(arg, "--is-inside-git-dir")) {
> > -                printf("%s\n", is_inside_git_dir() ? "true"
> > -                        : "false");
> > +                int is_set = is_inside_git_dir();
> 
> Avoid declaration after statement. Move is_set to the beginning of
> cmd_rev_parse().

This one is OK, because we're starting a new block (and the printf there is going away). And it's nicer to keep the variable local to this block if we can.

Show 16 quoted lines
> > +                if (quiet)
> > +                    return is_set ? 0 : 1;
> 
> Returning prematurely might be a bad idea. If there are more options after
> "--is-inside-git-dir", they will be not executed. You can maybe rewrite this as:
> 
> ```
>              if (!strcmp(arg, "--is-inside-git-dir")) {
>                 is_set = is_inside_git_dir();
>                 if (!quiet)
>                         printf("%s\n", is_set ? "true"
>                             : "false");
>                  continue;
>              }
> ```
> And then return the value at the end of function, replacing '0' with !is_set.

Yeah, the inability of this new mode to handle multiple options is a drawback. We could perhaps live with it as a restriction (it is up to the caller to decide if they want to make multiple calls with --quiet, or do it all as one and accept the hassle of parsing), but it's rather unfortunate if:

  git rev-parse --quiet --is-inside-git-dir --is-bare-repository
quietly ignores the third argument. That seems like a recipe for errors.

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.

-Peff
Junio C Hamano· Mar 13, 2020, 18:18 UTC · re: Jeff King · lore
Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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.

Erlend Aasland· Mar 13, 2020, 19:10 UTC · re: Junio C Hamano · lore
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.
Eric Sunshine· Mar 13, 2020, 17:50 UTC · re: Abhishek Kumar · lore

On Fri, Mar 13, 2020 at 1:30 PM Abhishek Kumar <abhishekkumar8222@gmail.com> wrote:

Show 20 quoted lines
> > If rev-parse is called with both -q and an --is-* option, the result is
> > provided as the return code of the command, iso. printed to stdout.
> >
> > This simplifies using these queries in shell scripts:
> > git rev-parse --is-bare-repository && do_stuff
> > git rev-parse --is-shallow-repository && do_stuff
> >
> > Signed-off-by: Erlend E. Aasland <erlend.aasland@innova.no>
> > ---
> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> > @@ -874,24 +874,31 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >                 continue;
> >             }
> >             if (!strcmp(arg, "--is-inside-git-dir")) {
> > -                printf("%s\n", is_inside_git_dir() ? "true"
> > -                        : "false");
> > +                int is_set = is_inside_git_dir();
>
> Avoid declaration after statement. Move is_set to the beginning of
> cmd_rev_parse().

Thanks for taking up this review. You hit on some of the points I was going to mention. Note, however, that this declaration is fine as-is; it immediately follows the opening brace of the 'if' statement, thus it is the not "declaration after statement". Also, it makes sense to declare it here to keep its scope small and visible only where the variable is needed/used.

> Also, be more specific than "is_set". A variable like "is_inside" would
> be more appropriate.

Agreed. And since the variable can be declared in this tiny scope, an even shorter and simpler name (such as "x") could likely work, though I don't insist upon it.

Show 5 quoted lines
> > +                if (quiet)
> > +                    return is_set ? 0 : 1;
>
> Returning prematurely might be a bad idea. If there are more options after
> "--is-inside-git-dir", they will be not executed.
The big problem with returning early like this is that it means that:
    git rev-parse -q --is-whatever
behaves differently than:
    git rev-parse --is-whatever -q
which is undesirable.
Show 9 quoted lines
> You can maybe rewrite this as:
>
>              if (!strcmp(arg, "--is-inside-git-dir")) {
>                 is_set = is_inside_git_dir();
>                 if (!quiet)
>                         printf("%s\n", is_set ? "true"
>                             : "false");
>                  continue;
>              }
> And then return the value at the end of function, replacing '0' with !is_set.

Since git-rev-parse knows how to respond to multiple --is-whatever flags in a single invocation, my thought was that the return code would be cumulative. That is, something like this:

    int rc = 0;
    ...
    if (!strcmp(arg, "--is-whatever")) {
        int x = is_inside_git_dir();
        rc |= x;
        if (!quiet)
            printf("%s\n", x ? "true" : "false");
        continue
    }
> I am wondering if we should stop the script from running if both quiet
> and --is-* options are passed.

That's a possibility. More generally, I haven't seen enough evidence to convince me that this new mode of behavior is worth the effort. That is, after this patch, one can script like this:

    if git rev-parse -q --is-whatever
    then
        ...
    fi
which is not much different from:
    if test "$(git rev-parse --is-whatever) == "true"
    then
        ...
    fi
This is RFC, so I let the lack of documentation and test update slide.

← back to recent threads