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

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Mar 13, 2020 at 1:30 PM Abhishek Kumar
<abhishekkumar8222@gmail.com> wrote:
> > 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.

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

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