{"thread":{"id":"59014","subject":"[PATCH] grep: simplify is_empty_line","startedAt":"2022-12-29T16:59:07Z","lastAt":"2022-12-29T17:46:00Z","messageCount":4,"participants":["Rose via GitGitGadget","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469634","messageId":"pull.1418.git.git.1672333122193.gitgitgadget@gmail.com","threadId":"59014","inReplyTo":null,"subject":"[PATCH] grep: simplify is_empty_line","fromName":"Rose via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-29T16:58:42Z","receivedAt":"2022-12-29T16:59:07Z","isPatch":true,"sender":{"key":"ckelsch@jgrcpa.com","avatar":null},"body":"From: Seija Kijin \n\nSigned-off-by: Seija Kijin \n---\n grep: simplify is_empty_line\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1418%2FAtariDreams%2FisEmpty-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1418/AtariDreams/isEmpty-v1\nPull-Request: https://github.com/git/git/pull/1418\n\n grep.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 06eed694936..f29f4dd9e08 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1483,9 +1483,10 @@ static int fill_textconv_grep(struct repository *r,\n \n static int is_empty_line(const char *bol, const char *eol)\n {\n-\twhile (bol < eol && isspace(*bol))\n-\t\tbol++;\n-\treturn bol == eol;\n+\twhile (bol < eol)\n+\t\tif (!isspace(*bol))\n+\t\t\treturn 0;\n+\treturn 1;\n }\n \n static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n\nbase-commit: 6bae53b138a1f38d8887f6b46d17661357a1468b\n-- \ngitgitgadget\n"},{"id":"469635","messageId":"CAPig+cSFAUcU74qULYkN7OX4-OqU_3VJeTdb1ZH_QoOW9FBwZQ@mail.gmail.com","threadId":"59014","inReplyTo":"pull.1418.git.git.1672333122193.gitgitgadget@gmail.com","subject":"Re: [PATCH] grep: simplify is_empty_line","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-29T17:06:04Z","receivedAt":"2022-12-29T17:06:20Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 29, 2022 at 12:00 PM Rose via GitGitGadget\n wrote:\n> diff --git a/grep.c b/grep.c\n> @@ -1483,9 +1483,10 @@ static int fill_textconv_grep(struct repository *r,\n> static int is_empty_line(const char *bol, const char *eol)\n> {\n> - while (bol < eol && isspace(*bol))\n> - bol++;\n> - return bol == eol;\n> + while (bol < eol)\n> + if (!isspace(*bol))\n> + return 0;\n> + return 1;\n> }\n\nThe rewritten code appears to be quite broken. It never increments `bol`.\n"},{"id":"469636","messageId":"pull.1418.v2.git.git.1672334314401.gitgitgadget@gmail.com","threadId":"59014","inReplyTo":"pull.1418.git.git.1672333122193.gitgitgadget@gmail.com","subject":"[PATCH v2] grep: simplify is_empty_line","fromName":"Rose via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-12-29T17:18:34Z","receivedAt":"2022-12-29T17:18:51Z","isPatch":true,"sender":{"key":"ckelsch@jgrcpa.com","avatar":null},"body":"From: Seija Kijin \n\nSigned-off-by: Seija Kijin \n---\n grep: simplify is_empty_line\n \n Signed-off-by: Seija Kijin doremylover123@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1418%2FAtariDreams%2FisEmpty-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1418/AtariDreams/isEmpty-v2\nPull-Request: https://github.com/git/git/pull/1418\n\nRange-diff vs v1:\n\n 1: 03a97005a67 ! 1: b193c9537de grep: simplify is_empty_line\n @@ grep.c: static int fill_textconv_grep(struct repository *r,\n static int is_empty_line(const char *bol, const char *eol)\n {\n -\twhile (bol < eol && isspace(*bol))\n --\t\tbol++;\n --\treturn bol == eol;\n -+\twhile (bol < eol)\n ++\twhile (bol < eol) {\n +\t\tif (!isspace(*bol))\n +\t\t\treturn 0;\n + \t\tbol++;\n +-\treturn bol == eol;\n ++\t}\n +\treturn 1;\n }\n \n\n\n grep.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 06eed694936..d78ee08da6f 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1483,9 +1483,12 @@ static int fill_textconv_grep(struct repository *r,\n \n static int is_empty_line(const char *bol, const char *eol)\n {\n-\twhile (bol < eol && isspace(*bol))\n+\twhile (bol < eol) {\n+\t\tif (!isspace(*bol))\n+\t\t\treturn 0;\n \t\tbol++;\n-\treturn bol == eol;\n+\t}\n+\treturn 1;\n }\n \n static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits)\n\nbase-commit: 6bae53b138a1f38d8887f6b46d17661357a1468b\n-- \ngitgitgadget\n"},{"id":"469637","messageId":"CAPig+cTmW_7eXhH0-y0cj_r1UqDc+JJvMWVSFKOYSYFvpkjQJw@mail.gmail.com","threadId":"59014","inReplyTo":"pull.1418.v2.git.git.1672334314401.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] grep: simplify is_empty_line","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-12-29T17:45:44Z","receivedAt":"2022-12-29T17:46:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 29, 2022 at 12:18 PM Rose via GitGitGadget\n wrote:\n> Signed-off-by: Seija Kijin \n> ---\n> diff --git a/grep.c b/grep.c\n> @@ -1483,9 +1483,12 @@ static int fill_textconv_grep(struct repository *r,\n> static int is_empty_line(const char *bol, const char *eol)\n> {\n> - while (bol < eol && isspace(*bol))\n> + while (bol < eol) {\n> + if (!isspace(*bol))\n> + return 0;\n> bol++;\n> - return bol == eol;\n> + }\n> + return 1;\n> }\n\nIt is subjective (personal opinion) whether or not the new code is\nclearer than the original. As a general policy, this project tends not\nto accept patches like this which merely \"churn\" the code without\nimproving it. From Documentation/SubmittingPatches:\n\n \"Once it _is_ in the tree, it's not really worth the patch noise to\n go and fix it up.\"\n Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html\n\nOne reason for avoiding churn is that even simple and innocuous\nchanges like this can introduce bugs or unwanted behavior, as v1 of\nthis patch illustrated[1]. Another reason is that it eats up reviewer\ntime.\n\nDid the Git test suite pass with v1 of this patch even though it was\nbuggy? If so, a better use of your time and reviewer time would be to\nimprove test coverage so that it detects the sort of breakage caused\nby v1.\n\n[1]: https://lore.kernel.org/git/CAPig+cSFAUcU74qULYkN7OX4-OqU_3VJeTdb1ZH_QoOW9FBwZQ@mail.gmail.com/\n"}]}