{"thread":{"id":"52990","subject":"Re: [RFC PATCH] Make rev-parse -q and --is-* quiet","startedAt":"2020-03-13T17:30:34Z","lastAt":"2020-03-13T19:11:11Z","messageCount":5,"participants":["Abhishek Kumar","Jeff King","Eric Sunshine","Junio C Hamano","Erlend Aasland"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"393215","messageId":"CAHk66ftWBYF3d_L0-__BP5mKNxBioj57y44yhyqGrtK3TZTjSg@mail.gmail.com","threadId":"52990","inReplyTo":null,"subject":"Re: [RFC PATCH] Make rev-parse -q and --is-* quiet","fromName":"Abhishek Kumar","fromEmail":"abhishekkumar8222@gmail.com","sentAt":"2020-03-13T17:30:00Z","receivedAt":"2020-03-13T17:30:34Z","isPatch":true,"sender":{"key":"abhishekkumar8222@gmail.com","avatar":"https://avatars.githubusercontent.com/u/31231064?v=4"},"body":"> If rev-parse is called with both -q and an --is-* option, the result is\n> provided as the return code of the command, iso. printed to stdout.\n>\n> This simplifies using these queries in shell scripts:\n> git rev-parse --is-bare-repository && do_stuff\n> git rev-parse --is-shallow-repository && do_stuff\n>\n> Signed-off-by: Erlend E. Aasland \n> ---\n> builtin/rev-parse.c | 25 ++++++++++++++++---------\n> 1 file changed, 16 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> index 7a00da8203..5a8b404ec7 100644\n> --- a/builtin/rev-parse.c\n> +++ b/builtin/rev-parse.c\n> @@ -874,24 +874,31 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> continue;\n> }\n> if (!strcmp(arg, \"--is-inside-git-dir\")) {\n> - printf(\"%s\\n\", is_inside_git_dir() ? \"true\"\n> - : \"false\");\n> + int is_set = is_inside_git_dir();\n\nAvoid declaration after statement. Move is_set to the beginning of\ncmd_rev_parse().\n\nAlso, be more specific than \"is_set\". A variable like \"is_inside\" would\nbe more appropriate.\n\n> + if (quiet)\n> + return is_set ? 0 : 1;\n\nReturning prematurely might be a bad idea. If there are more options after\n\"--is-inside-git-dir\", they will be not executed. You can maybe rewrite this as:\n\n```\n if (!strcmp(arg, \"--is-inside-git-dir\")) {\n is_set = is_inside_git_dir();\n if (!quiet)\n printf(\"%s\\n\", is_set ? \"true\"\n : \"false\");\n continue;\n }\n```\nAnd then return the value at the end of function, replacing '0' with !is_set.\n\nSame comment for other blocks.\n\n\n> if (!strcmp(arg, \"--is-inside-work-tree\")) {\n> - printf(\"%s\\n\", is_inside_work_tree() ? \"true\"\n> - : \"false\");\n> + int is_set = is_inside_work_tree();\n> + if (quiet)\n> + return is_set ? 0 : 1;\n> + printf(\"%s\\n\", is_set ? \"true\" : \"false\");\n> continue;\n> }\n> if (!strcmp(arg, \"--is-bare-repository\")) {\n> - printf(\"%s\\n\", is_bare_repository() ? \"true\"\n> - : \"false\");\n> + int is_set = is_bare_repository();\n> + if (quiet)\n> + return is_set ? 0 : 1;\n> + printf(\"%s\\n\", is_set ? \"true\" : \"false\");\n> continue;\n> }\n> if (!strcmp(arg, \"--is-shallow-repository\")) {\n> - printf(\"%s\\n\",\n> - is_repository_shallow(the_repository) ? \"true\"\n> - : \"false\");\n> + int is_set = is_repository_shallow(the_repository);\n> + if (quiet)\n> + return is_set ? 0 : 1;\n> + printf(\"%s\\n\", is_set ? \"true\" : \"false\");\n> continue;\n> }\n> if (!strcmp(arg, \"--shared-index-path\")) {\n> --\n> 2.25.1\n\nI am wondering if we should stop the script from running if both quiet\nand --is-* options are passed.\n\nRegards\nAbhishek\n"},{"id":"393218","messageId":"20200313174755.GA549554@coredump.intra.peff.net","threadId":"52990","inReplyTo":"CAHk66ftWBYF3d_L0-__BP5mKNxBioj57y44yhyqGrtK3TZTjSg@mail.gmail.com","subject":"Re: [RFC PATCH] Make rev-parse -q and --is-* quiet","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-13T17:47:55Z","receivedAt":"2020-03-13T17:47:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 13, 2020 at 11:00:00PM +0530, Abhishek Kumar wrote:\n\n> > @@ -874,24 +874,31 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> > continue;\n> > }\n> > if (!strcmp(arg, \"--is-inside-git-dir\")) {\n> > - printf(\"%s\\n\", is_inside_git_dir() ? \"true\"\n> > - : \"false\");\n> > + int is_set = is_inside_git_dir();\n> \n> Avoid declaration after statement. Move is_set to the beginning of\n> cmd_rev_parse().\n\nThis one is OK, because we're starting a new block (and the printf there\nis going away). And it's nicer to keep the variable local to this block\nif we can.\n\n> > + if (quiet)\n> > + return is_set ? 0 : 1;\n> \n> Returning prematurely might be a bad idea. If there are more options after\n> \"--is-inside-git-dir\", they will be not executed. You can maybe rewrite this as:\n> \n> ```\n> if (!strcmp(arg, \"--is-inside-git-dir\")) {\n> is_set = is_inside_git_dir();\n> if (!quiet)\n> printf(\"%s\\n\", is_set ? \"true\"\n> : \"false\");\n> continue;\n> }\n> ```\n> And then return the value at the end of function, replacing '0' with !is_set.\n\nYeah, the inability of this new mode to handle multiple options is a\ndrawback. We could perhaps live with it as a restriction (it is up to\nthe caller to decide if they want to make multiple calls with --quiet,\nor do it all as one and accept the hassle of parsing), but it's rather\nunfortunate if:\n\n git rev-parse --quiet --is-inside-git-dir --is-bare-repository\n\nquietly ignores the third argument. That seems like a recipe for errors.\n\nI'm not sure if returning a single is_set makes sense, though. It\neffectively becomes an OR, and you wouldn't know which flag triggered\nit. It would make more sense to me for the invocation above to simply be\nan error, reminding the caller that they need to handle it more\ncarefully.\n\nWe _could_ encode each value into the exit code (e.g., set bit 1 if the\nfirst condition was true, and so on). But checking that becomes as much\nhassle as reading stdout, so there's little value.\n\n-Peff\n"},{"id":"393219","messageId":"CAPig+cQBz-ZoV5ZX17WgW9p+ychONN8o_E13HHnR8ZGmhLk5aQ@mail.gmail.com","threadId":"52990","inReplyTo":"CAHk66ftWBYF3d_L0-__BP5mKNxBioj57y44yhyqGrtK3TZTjSg@mail.gmail.com","subject":"Re: [RFC PATCH] Make rev-parse -q and --is-* quiet","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-03-13T17:50:01Z","receivedAt":"2020-03-13T17:50:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 13, 2020 at 1:30 PM Abhishek Kumar\n wrote:\n> > If rev-parse is called with both -q and an --is-* option, the result is\n> > provided as the return code of the command, iso. printed to stdout.\n> >\n> > This simplifies using these queries in shell scripts:\n> > git rev-parse --is-bare-repository && do_stuff\n> > git rev-parse --is-shallow-repository && do_stuff\n> >\n> > Signed-off-by: Erlend E. Aasland \n> > ---\n> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> > @@ -874,24 +874,31 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n> > continue;\n> > }\n> > if (!strcmp(arg, \"--is-inside-git-dir\")) {\n> > - printf(\"%s\\n\", is_inside_git_dir() ? \"true\"\n> > - : \"false\");\n> > + int is_set = is_inside_git_dir();\n>\n> Avoid declaration after statement. Move is_set to the beginning of\n> cmd_rev_parse().\n\nThanks for taking up this review. You hit on some of the points I was\ngoing to mention. Note, however, that this declaration is fine as-is;\nit immediately follows the opening brace of the 'if' statement, thus\nit is the not \"declaration after statement\". Also, it makes sense to\ndeclare it here to keep its scope small and visible only where the\nvariable is needed/used.\n\n> Also, be more specific than \"is_set\". A variable like \"is_inside\" would\n> be more appropriate.\n\nAgreed. And since the variable can be declared in this tiny scope, an\neven shorter and simpler name (such as \"x\") could likely work, though\nI don't insist upon it.\n\n> > + if (quiet)\n> > + return is_set ? 0 : 1;\n>\n> Returning prematurely might be a bad idea. If there are more options after\n> \"--is-inside-git-dir\", they will be not executed.\n\nThe big problem with returning early like this is that it means that:\n\n git rev-parse -q --is-whatever\n\nbehaves differently than:\n\n git rev-parse --is-whatever -q\n\nwhich is undesirable.\n\n> You can maybe rewrite this as:\n>\n> if (!strcmp(arg, \"--is-inside-git-dir\")) {\n> is_set = is_inside_git_dir();\n> if (!quiet)\n> printf(\"%s\\n\", is_set ? \"true\"\n> : \"false\");\n> continue;\n> }\n\n> And then return the value at the end of function, replacing '0' with !is_set.\n\nSince git-rev-parse knows how to respond to multiple --is-whatever\nflags in a single invocation, my thought was that the return code\nwould be cumulative. That is, something like this:\n\n int rc = 0;\n ...\n if (!strcmp(arg, \"--is-whatever\")) {\n int x = is_inside_git_dir();\n rc |= x;\n if (!quiet)\n printf(\"%s\\n\", x ? \"true\" : \"false\");\n continue\n }\n\n> I am wondering if we should stop the script from running if both quiet\n> and --is-* options are passed.\n\nThat's a possibility. More generally, I haven't seen enough evidence\nto convince me that this new mode of behavior is worth the effort.\nThat is, after this patch, one can script like this:\n\n if git rev-parse -q --is-whatever\n then\n ...\n fi\n\nwhich is not much different from:\n\n if test \"$(git rev-parse --is-whatever) == \"true\"\n then\n ...\n fi\n\nThis is RFC, so I let the lack of documentation and test update slide.\n"},{"id":"393223","messageId":"xmqqa74kce4t.fsf@gitster.c.googlers.com","threadId":"52990","inReplyTo":"20200313174755.GA549554@coredump.intra.peff.net","subject":"Re: [RFC PATCH] Make rev-parse -q and --is-* quiet","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-13T18:18:26Z","receivedAt":"2020-03-13T18:18:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not sure if returning a single is_set makes sense, though. It\n> effectively becomes an OR, and you wouldn't know which flag triggered\n> it. It would make more sense to me for the invocation above to simply be\n> an error, reminding the caller that they need to handle it more\n> carefully.\n>\n> We _could_ encode each value into the exit code (e.g., set bit 1 if the\n> first condition was true, and so on). But checking that becomes as much\n> hassle as reading stdout, so there's little value.\n\nNone of the above excites me. I do not think it makes much sense to\ncombine -q with --is-* for the reasons you stated already (i.e. the\ncaller cannot tell between \"failure\" and \"false\") in the first\nplace, but if we must do this:\n\n - reserve 0 (true) and 1 (false) for successful exit and use 2 (or\n above) for other failures;\n\n - when --quiet is in use, make --is-* mutually exclusive and die\n when more than one of them is given. I think any of the --is-*,\n when used with --quiet, should also be an error if there are revs\n on the command line (e.g. \"git rev-parse --is-inside-git-dir\n HEAD\" is OK, but not with \"--quiet\").\n\nis the minimum that makes me feel that we have semi-reasonable\nbehaviour that can be explained to end users.\n"},{"id":"393228","messageId":"CAB5DABE-BBBE-4E1A-83FE-E2840714CA7C@innova.no","threadId":"52990","inReplyTo":"xmqqa74kce4t.fsf@gitster.c.googlers.com","subject":"Re: [RFC PATCH] Make rev-parse -q and --is-* quiet","fromName":"Erlend Aasland","fromEmail":"erlend-a@innova.no","sentAt":"2020-03-13T19:10:23Z","receivedAt":"2020-03-13T19:11:11Z","isPatch":true,"sender":{"key":"erlend-a@innova.no","avatar":null},"body":"Thank you all for your thorough reviews and constructive comments.\n\nMany of you comment on the same flaws in my patch; I’ll address all of\nthem in the event of a second version of this patch, if you all this this\nfeature is worth it.\n\nIssue 1) Naming\nI’ll rename the is_set variables according to Abhishek’s proposal (is_inline,\nis_shallow, etc.)\n\nIssue 2) Premature return\nYes, I chose the easiest way out, since this was a RFC :) One\npossibility could be to AND or OR the —is-* options together, but I think\ngoing with Junio’s proposal is the most sane thing to do:\n\n\nOn 13 Mar 2020, at 19:18, Junio C Hamano wrote:\n> - when --quiet is in use, make --is-* mutually exclusive and die\n> when more than one of them is given. I think any of the --is-*,\n> when used with --quiet, should also be an error if there are revs\n> on the command line (e.g. \"git rev-parse --is-inside-git-dir\n> HEAD\" is OK, but not with \"--quiet\").\n\nIssue 3) Exit code\nI think going with Junio’s proposal (0 => true, 1 => false, 2 => error) is ok.\n\n\nI’ll have another go at it, if you all think it’s worth it. If so, I’ll also add docs\nand unit tests.\n\n\nErlend\n\n\n> On 13 Mar 2020, at 19:18, Junio C Hamano wrote:\n> \n> Jeff King writes:\n> \n>> I'm not sure if returning a single is_set makes sense, though. It\n>> effectively becomes an OR, and you wouldn't know which flag triggered\n>> it. It would make more sense to me for the invocation above to simply be\n>> an error, reminding the caller that they need to handle it more\n>> carefully.\n>> \n>> We _could_ encode each value into the exit code (e.g., set bit 1 if the\n>> first condition was true, and so on). But checking that becomes as much\n>> hassle as reading stdout, so there's little value.\n> \n> None of the above excites me. I do not think it makes much sense to\n> combine -q with --is-* for the reasons you stated already (i.e. the\n> caller cannot tell between \"failure\" and \"false\") in the first\n> place, but if we must do this:\n> \n> - reserve 0 (true) and 1 (false) for successful exit and use 2 (or\n> above) for other failures;\n> \n> - when --quiet is in use, make --is-* mutually exclusive and die\n> when more than one of them is given. I think any of the --is-*,\n> when used with --quiet, should also be an error if there are revs\n> on the command line (e.g. \"git rev-parse --is-inside-git-dir\n> HEAD\" is OK, but not with \"--quiet\").\n> \n> is the minimum that makes me feel that we have semi-reasonable\n> behaviour that can be explained to end users.\n\n"}]}