# Re: [RFC PATCH] Make rev-parse -q and --is-* quiet 5 messages from 2020-03-13 to 2020-03-13. Participants: Abhishek Kumar, Jeff King, Eric Sunshine, Junio C Hamano, Erlend Aasland. Thread: https://gitlist.dev/t/52990 ## Abhishek Kumar, 2020-03-13 17:30 Subject: Re: [RFC PATCH] Make rev-parse -q and --is-* quiet Message-ID: URL: https://gitlist.dev/e/CAHk66ftWBYF3d_L0-__BP5mKNxBioj57y44yhyqGrtK3TZTjSg%40mail.gmail.com ``` > 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 > --- > 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 ``` ## Jeff King, 2020-03-13 17:47 Subject: Re: [RFC PATCH] Make rev-parse -q and --is-* quiet Message-ID: <20200313174755.GA549554@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200313174755.GA549554%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Eric Sunshine, 2020-03-13 17:50 Subject: Re: [RFC PATCH] Make rev-parse -q and --is-* quiet Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQBz-ZoV5ZX17WgW9p%2BychONN8o_E13HHnR8ZGmhLk5aQ%40mail.gmail.com In-Reply-To: ``` On Fri, Mar 13, 2020 at 1:30 PM Abhishek Kumar 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 > > --- > > 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. ``` ## Junio C Hamano, 2020-03-13 18:18 Subject: Re: [RFC PATCH] Make rev-parse -q and --is-* quiet Message-ID: URL: https://gitlist.dev/e/xmqqa74kce4t.fsf%40gitster.c.googlers.com In-Reply-To: <20200313174755.GA549554@coredump.intra.peff.net> ``` Jeff King 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. ``` ## Erlend Aasland, 2020-03-13 19:10 Subject: Re: [RFC PATCH] Make rev-parse -q and --is-* quiet Message-ID: URL: https://gitlist.dev/e/CAB5DABE-BBBE-4E1A-83FE-E2840714CA7C%40innova.no In-Reply-To: ``` 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 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 wrote: > > Jeff King 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. ```