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

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

[Abhishek Kumar]

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


>             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

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

[Jeff King]

On Fri, Mar 13, 2020 at 11:00:00PM +0530, Abhishek Kumar wrote:

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

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

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

[Junio C Hamano]

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.

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

[Erlend Aasland]

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


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

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

[Eric Sunshine]

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.

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

[Erlend Aasland]

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();
+				if (quiet)
+					return is_set ? 0 : 1;
+				printf("%s\n", is_set ? "true" : "false");
				continue;
			}
			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

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

[Jeff King]

On Fri, Mar 13, 2020 at 12:13:52PM +0000, Erlend Aasland 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

I left some comments in the other part of the thread, but another
potential downside here is that the exit code also tells us whether the
command was unable to run for unrelated reasons (i.e., the answer is
really a tristate: yes, no, or "we don't know the answer").

One obvious reason to exit non-zero would be that we're not in a
repository at all. I _think_ that works OK with all of these
conditionals, because if we're not in a repo, then we are obviously not
inside a git-dir, and so on.

Another reason is that the caller misspelled the option name. :) That
might be an acceptable risk, though. It's not like it isn't present in
other commands, probably people are doing:

  test "$(git rev-parse --is-whatever") = "true" && do_stuff

anyway, which has the same problem. Though perhaps the simplicity there
is an argument that we don't need the new exit-code mechanism.

-Peff