{"thread":{"id":"42562","subject":"[PATCH 2/2] bisect--helper: `check_expected_revs` shell function in C","startedAt":"2016-06-08T15:24:14Z","lastAt":"2016-06-16T02:19:52Z","messageCount":9,"participants":["Pranit Bauva","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"288726","messageId":"20160608152415.7770-1-pranit.bauva@gmail.com","threadId":"42562","inReplyTo":null,"subject":"[PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-06-08T15:24:14Z","receivedAt":"2016-06-08T15:24:14Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement `is_expected_rev` shell function in C. This will further be\ncalled from `check_expected_revs` function. This is a quite small\nfunction thus subcommand facility is redundant.\n\nMentored-by: Lars Schneider <larsxschneider@gmail.com>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n---\nThis applies on the previous patches.\n\n builtin/bisect--helper.c | 14 ++++++++++++++\n 1 file changed, 14 insertions(+)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 4153e8a..06bc9b8 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -160,6 +160,20 @@ int bisect_reset(const char *commit)\n \treturn bisect_clean_state();\n }\n \n+static int is_expected_rev(const char *expected_hex)\n+{\n+\tstruct strbuf actual_hex = STRBUF_INIT;\n+\n+\tif (!file_exists(git_path_bisect_expected_rev()))\n+\t\treturn 0;\n+\n+\tif (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))\n+\t\treturn 0;\n+\n+\tstrbuf_trim(&actual_hex);\n+\treturn !strcmp(actual_hex.buf, expected_hex);\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n-- \n2.8.3\n"},{"id":"288724","messageId":"20160608152415.7770-2-pranit.bauva@gmail.com","threadId":"42562","inReplyTo":"20160608152415.7770-1-pranit.bauva@gmail.com","subject":"[PATCH 2/2] bisect--helper: `check_expected_revs` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-06-08T15:24:15Z","receivedAt":"2016-06-08T15:24:15Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Reimplement the `check_expected_revs` shell function in C and add a\n`--check-expected-revs` subcommand to `git bisect--helper` to call it\nfrom git-bisect.sh .\n\nUsing `--check-expected-revs` subcommand is a temporary measure to port\nshell function in C so as to use the existing test suite. As more\nfunctions are ported, this subcommand will be retired and will be called\nby some other method namely `bisect_state()`.\n\nNote: Previously is_expected_rev() was converted but not added as a\nsubcommand. Now since we have converted check_expected_rev() and we are\ncalling is_expected_rev()'s C implementation, we can safely delete the\nshell implementation.\n\nMentored-by: Lars Schneider <larsxschneider@gmail.com>\nMentored-by: Christian Couder <chriscool@tuxfamily.org>\nSigned-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n---\n builtin/bisect--helper.c | 21 ++++++++++++++++++++-\n git-bisect.sh            | 20 ++------------------\n 2 files changed, 22 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 06bc9b8..500efd5 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -174,13 +174,28 @@ static int is_expected_rev(const char *expected_hex)\n \treturn !strcmp(actual_hex.buf, expected_hex);\n }\n \n+static int check_expected_revs(const char **revs, int no)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < no; i++) {\n+\t\tif (!is_expected_rev(revs[i])) {\n+\t\t\tremove_path(git_path_bisect_ancestors_ok());\n+\t\t\tremove_path(git_path_bisect_expected_rev());\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n {\n \tenum {\n \t\tNEXT_ALL = 1,\n \t\tWRITE_TERMS,\n \t\tBISECT_CLEAN_STATE,\n-\t\tBISECT_RESET\n+\t\tBISECT_RESET,\n+\t\tCHECK_EXPECTED_REVS\n \t} cmdmode = 0;\n \tint no_checkout = 0;\n \tstruct option options[] = {\n@@ -192,6 +207,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"cleanup the bisection state\"), BISECT_CLEAN_STATE),\n \t\tOPT_CMDMODE(0, \"bisect-reset\", &cmdmode,\n \t\t\t N_(\"reset the bisection state\"), BISECT_RESET),\n+\t\tOPT_CMDMODE(0, \"check-expected-revs\", &cmdmode,\n+\t\t\t N_(\"check for expected revs\"), CHECK_EXPECTED_REVS),\n \t\tOPT_BOOL(0, \"no-checkout\", &no_checkout,\n \t\t\t N_(\"update BISECT_HEAD instead of checking out the current commit\")),\n \t\tOPT_END()\n@@ -218,6 +235,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tif (argc > 1)\n \t\t\tdie(_(\"--bisect-reset requires either zero or one arguments\"));\n \t\treturn bisect_reset(argc ? argv[0] : NULL);\n+\tcase CHECK_EXPECTED_REVS:\n+\t\treturn check_expected_revs(argv, argc);\n \tdefault:\n \t\tdie(\"BUG: unknown subcommand '%d'\", cmdmode);\n \t}\ndiff --git a/git-bisect.sh b/git-bisect.sh\nindex 18580b7..4f6545e 100755\n--- a/git-bisect.sh\n+++ b/git-bisect.sh\n@@ -238,22 +238,6 @@ bisect_write() {\n \ttest -n \"$nolog\" || echo \"git bisect $state $rev\" >>\"$GIT_DIR/BISECT_LOG\"\n }\n \n-is_expected_rev() {\n-\ttest -f \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n-\ttest \"$1\" = $(cat \"$GIT_DIR/BISECT_EXPECTED_REV\")\n-}\n-\n-check_expected_revs() {\n-\tfor _rev in \"$@\"; do\n-\t\tif ! is_expected_rev \"$_rev\"\n-\t\tthen\n-\t\t\trm -f \"$GIT_DIR/BISECT_ANCESTORS_OK\"\n-\t\t\trm -f \"$GIT_DIR/BISECT_EXPECTED_REV\"\n-\t\t\treturn\n-\t\tfi\n-\tdone\n-}\n-\n bisect_skip() {\n \tall=''\n \tfor arg in \"$@\"\n@@ -280,7 +264,7 @@ bisect_state() {\n \t\trev=$(git rev-parse --verify $(bisect_head)) ||\n \t\t\tdie \"$(gettext \"Bad rev input: $(bisect_head)\")\"\n \t\tbisect_write \"$state\" \"$rev\"\n-\t\tcheck_expected_revs \"$rev\" ;;\n+\t\tgit bisect--helper --check-expected-revs \"$rev\" ;;\n \t2,\"$TERM_BAD\"|*,\"$TERM_GOOD\"|*,skip)\n \t\tshift\n \t\thash_list=''\n@@ -294,7 +278,7 @@ bisect_state() {\n \t\tdo\n \t\t\tbisect_write \"$state\" \"$rev\"\n \t\tdone\n-\t\tcheck_expected_revs $hash_list ;;\n+\t\tgit bisect--helper --check-expected-revs $hash_list ;;\n \t*,\"$TERM_BAD\")\n \t\tdie \"$(eval_gettext \"'git bisect \\$TERM_BAD' can take only one argument.\")\" ;;\n \t*)\n-- \n2.8.3\n"},{"id":"288851","messageId":"CAPig+cRC_y9MBSsrLEs0OxL4=FMfU-=ACwhESUiECrWrDNvaCw@mail.gmail.com","threadId":"42562","inReplyTo":"20160608152415.7770-1-pranit.bauva@gmail.com","subject":"Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-09T21:33:05Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> Reimplement `is_expected_rev` shell function in C. This will further be\n> called from `check_expected_revs` function. This is a quite small\n> function thus subcommand facility is redundant.\n\nThis patch should be squashed into patch 2/2, as it is otherwise\npointless without that patch, and merely adds dead code.\n\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> ---\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> @@ -160,6 +160,20 @@ int bisect_reset(const char *commit)\n> +static int is_expected_rev(const char *expected_hex)\n> +{\n> +       struct strbuf actual_hex = STRBUF_INIT;\n> +\n> +       if (!file_exists(git_path_bisect_expected_rev()))\n> +               return 0;\n\nInvoking file_exists() seems unnecessarily redundant when you can\ndiscern effectively the same by checking the return value of\nstrbuf_read_file() below. I'd drop the file_exists() check altogether.\n\n> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))\n> +               return 0;\n\nWhat exactly is this trying to do? Considering that strbuf_read_file()\nreturns -1 upon error, otherwise the number of bytes read, if I'm\nreading this correctly, is_expected_rev() returns false if\nstrbuf_read_file() encounters an error (which is fine) but also when\nit successfully reads the file and its content length is non-zero\n(which is very odd).\n\n> +       strbuf_trim(&actual_hex);\n> +       return !strcmp(actual_hex.buf, expected_hex);\n\nThus, it only ever gets to this point if the file exists but is empty,\nwhich is very unlikely to match 'expected_hex'. I could understand it\nif you checked the result of strbuf_read_file() with <0 or even <=0,\nbut the current code doesn't make sense to me.\n\nAm I misunderstanding?\n\n> +}\n"},{"id":"288853","messageId":"CAPig+cTAQAnTwQ7=Akw7N6FJS6jKc7hmZFVW=03b9NVD9+22ew@mail.gmail.com","threadId":"42562","inReplyTo":"CAPig+cRC_y9MBSsrLEs0OxL4=FMfU-=ACwhESUiECrWrDNvaCw@mail.gmail.com","subject":"Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-09T21:39:15Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 9, 2016 at 5:33 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> +       strbuf_trim(&actual_hex);\n>> +       return !strcmp(actual_hex.buf, expected_hex);\n>\n> Thus, it only ever gets to this point if the file exists but is empty,\n> which is very unlikely to match 'expected_hex'. I could understand it\n> if you checked the result of strbuf_read_file() with <0 or even <=0,\n> but the current code doesn't make sense to me.\n\nBy the way, this is also leaking strbuf 'actual_hex'.\n"},{"id":"288855","messageId":"CAPig+cQVWng0idk0ETRQA44OsS5+GewVX=pt4uOg-=SFbL+deQ@mail.gmail.com","threadId":"42562","inReplyTo":"20160608152415.7770-2-pranit.bauva@gmail.com","subject":"Re: [PATCH 2/2] bisect--helper: `check_expected_revs` shell function in C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-09T21:54:45Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> Reimplement the `check_expected_revs` shell function in C and add a\n> `--check-expected-revs` subcommand to `git bisect--helper` to call it\n> from git-bisect.sh .\n> [...]\n> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n> ---\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 06bc9b8..500efd5 100644\n> @@ -174,13 +174,28 @@ static int is_expected_rev(const char *expected_hex)\n> +static int check_expected_revs(const char **revs, int no)\n\nIn this codebase, it's more common to name this 'nr' rather than 'no'.\n'revs_nr' would also be a good name.\n\n> +{\n> +       int i;\n> +\n> +       for (i = 0; i < no; i++) {\n> +               if (!is_expected_rev(revs[i])) {\n> +                       remove_path(git_path_bisect_ancestors_ok());\n> +                       remove_path(git_path_bisect_expected_rev());\n> +                       return 0;\n> +               }\n> +       }\n> +       return 0;\n> +}\n"},{"id":"288869","messageId":"CAFZEwPNYxWpFcspHL8Oo3NydUPuKQeZWCFphM_an+_iKk9wBVw@mail.gmail.com","threadId":"42562","inReplyTo":"CAPig+cQVWng0idk0ETRQA44OsS5+GewVX=pt4uOg-=SFbL+deQ@mail.gmail.com","subject":"Re: [PATCH 2/2] bisect--helper: `check_expected_revs` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-06-10T07:52:35Z","receivedAt":"2016-06-16T02:19:50Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Eric,\n\nOn Fri, Jun 10, 2016 at 3:24 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> Reimplement the `check_expected_revs` shell function in C and add a\n>> `--check-expected-revs` subcommand to `git bisect--helper` to call it\n>> from git-bisect.sh .\n>> [...]\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n>> ---\n>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n>> index 06bc9b8..500efd5 100644\n>> @@ -174,13 +174,28 @@ static int is_expected_rev(const char *expected_hex)\n>> +static int check_expected_revs(const char **revs, int no)\n>\n> In this codebase, it's more common to name this 'nr' rather than 'no'.\n> 'revs_nr' would also be a good name.\n\nThanks. I will use `revs_nr`.\n\nRegards,\nPranit Bauva\n"},{"id":"288925","messageId":"CAFZEwPOhssmTt6TLPhxjzru+B3sWgXWh71yRt9BOvLqc0imq-g@mail.gmail.com","threadId":"42562","inReplyTo":"CAPig+cRC_y9MBSsrLEs0OxL4=FMfU-=ACwhESUiECrWrDNvaCw@mail.gmail.com","subject":"Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-06-10T13:39:45Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Eric,\n\nOn Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> Reimplement `is_expected_rev` shell function in C. This will further be\n>> called from `check_expected_revs` function. This is a quite small\n>> function thus subcommand facility is redundant.\n>\n> This patch should be squashed into patch 2/2, as it is otherwise\n> pointless without that patch, and merely adds dead code.\n\nSure I will squash and will explain it in the commit message.\n\n>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>\n>> ---\n>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n>> @@ -160,6 +160,20 @@ int bisect_reset(const char *commit)\n>> +static int is_expected_rev(const char *expected_hex)\n>> +{\n>> +       struct strbuf actual_hex = STRBUF_INIT;\n>> +\n>> +       if (!file_exists(git_path_bisect_expected_rev()))\n>> +               return 0;\n>\n> Invoking file_exists() seems unnecessarily redundant when you can\n> discern effectively the same by checking the return value of\n> strbuf_read_file() below. I'd drop the file_exists() check altogether.\n\nI wanted to imitate the code. But I guess it would actually be better\nif I drop this file_exists().\n\n>> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))\n>> +               return 0;\n>\n> What exactly is this trying to do? Considering that strbuf_read_file()\n> returns -1 upon error, otherwise the number of bytes read, if I'm\n> reading this correctly, is_expected_rev() returns false if\n> strbuf_read_file() encounters an error (which is fine) but also when\n> it successfully reads the file and its content length is non-zero\n> (which is very odd).\n>\n>> +       strbuf_trim(&actual_hex);\n>> +       return !strcmp(actual_hex.buf, expected_hex);\n>\n> Thus, it only ever gets to this point if the file exists but is empty,\n> which is very unlikely to match 'expected_hex'. I could understand it\n> if you checked the result of strbuf_read_file() with <0 or even <=0,\n> but the current code doesn't make sense to me.\n>\n> Am I misunderstanding?\n\n\nDefinitely not. Thanks for pointing it out. :) It went off my head\nthat strbuf_read_file returns the bytes it reads. Also the code\ncomment regarding strbuf_read_file does not mention it which probably\nmisguided me. I should also send a fixing patch so that someone else\ndoes not fall into this like I did.\n\nI will also release the 'actual_hex'.\n\nThanks for your comments!\n\nRegards,\nPranit Bauva\n"},{"id":"288940","messageId":"CAPig+cTECf6JT6+1SWo-eEwKPOAN3eYL20tvFS90Q28gu5vrZw@mail.gmail.com","threadId":"42562","inReplyTo":"CAFZEwPOhssmTt6TLPhxjzru+B3sWgXWh71yRt9BOvLqc0imq-g@mail.gmail.com","subject":"Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-10T19:14:13Z","receivedAt":"2016-06-16T02:19:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jun 10, 2016 at 9:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n> On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>>> Reimplement `is_expected_rev` shell function in C. This will further be\n>>> called from `check_expected_revs` function. This is a quite small\n>>> function thus subcommand facility is redundant.\n>>\n>> This patch should be squashed into patch 2/2, as it is otherwise\n>> pointless without that patch, and merely adds dead code.\n>\n> Sure I will squash and will explain it in the commit message.\n\nExplain what in the commit message? If anything, I'd expect the commit\nmessage to shrink since you won't need to explain anymore that this\nfunction is split out.\n\n>>> +       if (!file_exists(git_path_bisect_expected_rev()))\n>>> +               return 0;\n>>\n>> Invoking file_exists() seems unnecessarily redundant when you can\n>> discern effectively the same by checking the return value of\n>> strbuf_read_file() below. I'd drop the file_exists() check altogether.\n>\n> I wanted to imitate the code. But I guess it would actually be better\n> if I drop this file_exists().\n\nThere is a bit of a lesson to be learned by this example. While it's\ntrue that the C conversion should retain the behavior of the original\nshell code, that does not mean blindly mirroring the implementation\nline for line is a good idea. A couple things to take into\nconsideration:\n\nThere are idiomatic ways of doing things in each language. What is\nidiomatic in shell is not necessarily so in C. The C conversion should\nemploy C idioms and flow in a way which is natural for C code.\n\nConsider what the original shell code is doing at a higher level than\nmerely by reading it line-by-line. In the case in question, the code\nis:\n\n    test -f \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n    test \"$1\" = $(cat \"$GIT_DIR/BISECT_EXPECTED_REV\")\n\nWhile it's true that it's asking \"does the file exist and is its value\nthe same as $1\", the 'test -f' avoids a \"file not found\" error from\nthe $(cat ...) invocation. Since the return value of\nstrbuf_read_file() effectively encapsulates the \"does the file exist\"\ncheck, a separate check isn't really needed.\n\n>>> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))\n>>> +               return 0;\n>>\n>> What exactly is this trying to do? Considering that strbuf_read_file()\n>> returns -1 upon error, otherwise the number of bytes read, if I'm\n>> reading this correctly, is_expected_rev() returns false if\n>> strbuf_read_file() encounters an error (which is fine) but also when\n>> it successfully reads the file and its content length is non-zero\n>> (which is very odd).\n>>\n>>> +       strbuf_trim(&actual_hex);\n>>> +       return !strcmp(actual_hex.buf, expected_hex);\n>>\n>> Thus, it only ever gets to this point if the file exists but is empty,\n>> which is very unlikely to match 'expected_hex'. I could understand it\n>> if you checked the result of strbuf_read_file() with <0 or even <=0,\n>> but the current code doesn't make sense to me.\n>>\n>> Am I misunderstanding?\n>\n> Definitely not. Thanks for pointing it out. :) It went off my head\n> that strbuf_read_file returns the bytes it reads. Also the code\n> comment regarding strbuf_read_file does not mention it which probably\n> misguided me. I should also send a fixing patch so that someone else\n> does not fall into this like I did.\n\nOut of curiosity, did the test suite pass with this patch applied?\nThis is such an egregious bug that it's hard to imagine the tests\npassing, but if they did, then that may be a good indication that\ncoverage is too sparse and ought to be improved.\n"},{"id":"289017","messageId":"CAFZEwPMqwtw061O0QVRGf+dqpxKjp1jRvFE5CUEayEiUhxWveQ@mail.gmail.com","threadId":"42562","inReplyTo":"CAPig+cTECf6JT6+1SWo-eEwKPOAN3eYL20tvFS90Q28gu5vrZw@mail.gmail.com","subject":"Re: [PATCH 1/2] bisect--helper: `is_expected_rev` shell function in C","fromName":"Pranit Bauva","fromEmail":"pranit.bauva@gmail.com","sentAt":"2016-06-11T12:18:06Z","receivedAt":"2016-06-16T02:19:52Z","isPatch":true,"sender":{"key":"pranit.bauva@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2959938?v=4"},"body":"Hey Eric,\n\nOn Sat, Jun 11, 2016 at 12:44 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Fri, Jun 10, 2016 at 9:39 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>> On Fri, Jun 10, 2016 at 3:03 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n>>> On Wed, Jun 8, 2016 at 11:24 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:\n>>>> Reimplement `is_expected_rev` shell function in C. This will further be\n>>>> called from `check_expected_revs` function. This is a quite small\n>>>> function thus subcommand facility is redundant.\n>>>\n>>> This patch should be squashed into patch 2/2, as it is otherwise\n>>> pointless without that patch, and merely adds dead code.\n>>\n>> Sure I will squash and will explain it in the commit message.\n>\n> Explain what in the commit message? If anything, I'd expect the commit\n> message to shrink since you won't need to explain anymore that this\n> function is split out.\n\nYes I would remove the part where it is explained that this function\nis split out. I will just explain that 2 functions are converted in 1\ncommit.\n\n>>>> +       if (!file_exists(git_path_bisect_expected_rev()))\n>>>> +               return 0;\n>>>\n>>> Invoking file_exists() seems unnecessarily redundant when you can\n>>> discern effectively the same by checking the return value of\n>>> strbuf_read_file() below. I'd drop the file_exists() check altogether.\n>>\n>> I wanted to imitate the code. But I guess it would actually be better\n>> if I drop this file_exists().\n>\n> There is a bit of a lesson to be learned by this example. While it's\n> true that the C conversion should retain the behavior of the original\n> shell code, that does not mean blindly mirroring the implementation\n> line for line is a good idea. A couple things to take into\n> consideration:\n>\n> There are idiomatic ways of doing things in each language. What is\n> idiomatic in shell is not necessarily so in C. The C conversion should\n> employ C idioms and flow in a way which is natural for C code.\n>\n> Consider what the original shell code is doing at a higher level than\n> merely by reading it line-by-line. In the case in question, the code\n> is:\n>\n>     test -f \"$GIT_DIR/BISECT_EXPECTED_REV\" &&\n>     test \"$1\" = $(cat \"$GIT_DIR/BISECT_EXPECTED_REV\")\n>\n> While it's true that it's asking \"does the file exist and is its value\n> the same as $1\", the 'test -f' avoids a \"file not found\" error from\n> the $(cat ...) invocation. Since the return value of\n> strbuf_read_file() effectively encapsulates the \"does the file exist\"\n> check, a separate check isn't really needed.\n\nTrue. I will keep this in mind.\n\n>>>> +       if (!strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0))\n>>>> +               return 0;\n>>>\n>>> What exactly is this trying to do? Considering that strbuf_read_file()\n>>> returns -1 upon error, otherwise the number of bytes read, if I'm\n>>> reading this correctly, is_expected_rev() returns false if\n>>> strbuf_read_file() encounters an error (which is fine) but also when\n>>> it successfully reads the file and its content length is non-zero\n>>> (which is very odd).\n>>>\n>>>> +       strbuf_trim(&actual_hex);\n>>>> +       return !strcmp(actual_hex.buf, expected_hex);\n>>>\n>>> Thus, it only ever gets to this point if the file exists but is empty,\n>>> which is very unlikely to match 'expected_hex'. I could understand it\n>>> if you checked the result of strbuf_read_file() with <0 or even <=0,\n>>> but the current code doesn't make sense to me.\n>>>\n>>> Am I misunderstanding?\n>>\n>> Definitely not. Thanks for pointing it out. :) It went off my head\n>> that strbuf_read_file returns the bytes it reads. Also the code\n>> comment regarding strbuf_read_file does not mention it which probably\n>> misguided me. I should also send a fixing patch so that someone else\n>> does not fall into this like I did.\n>\n> Out of curiosity, did the test suite pass with this patch applied?\n> This is such an egregious bug that it's hard to imagine the tests\n> passing, but if they did, then that may be a good indication that\n> coverage is too sparse and ought to be improved.\n\nYes the test suite passed perfectly. I have inculcated the habit of\nrunning the whole test suite before sending patches. Yes some parts of\na test suite seem to be missing. How about I do it in the end? By this\nI won't have to setup yet another coverage tool for shell script. I\ncan use the coverage tool by GNU to test the coverage after bisect is\na C code. Till that time the patches can reside in the pu branch.\n\nRegards,\nPranit Bauva\n"}]}