# [PATCH] grep: simplify is_empty_line 4 messages from 2022-12-29 to 2022-12-29. Participants: Rose via GitGitGadget, Eric Sunshine. Thread: https://gitlist.dev/t/59014 ## Rose via GitGitGadget, 2022-12-29 16:58 Subject: [PATCH] grep: simplify is_empty_line Message-ID: URL: https://gitlist.dev/e/pull.1418.git.git.1672333122193.gitgitgadget%40gmail.com ``` From: Seija Kijin Signed-off-by: Seija Kijin --- grep: simplify is_empty_line Signed-off-by: Seija Kijin doremylover123@gmail.com Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1418%2FAtariDreams%2FisEmpty-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1418/AtariDreams/isEmpty-v1 Pull-Request: https://github.com/git/git/pull/1418 grep.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/grep.c b/grep.c index 06eed694936..f29f4dd9e08 100644 --- a/grep.c +++ b/grep.c @@ -1483,9 +1483,10 @@ static int fill_textconv_grep(struct repository *r, static int is_empty_line(const char *bol, const char *eol) { - while (bol < eol && isspace(*bol)) - bol++; - return bol == eol; + while (bol < eol) + if (!isspace(*bol)) + return 0; + return 1; } static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits) base-commit: 6bae53b138a1f38d8887f6b46d17661357a1468b -- gitgitgadget ``` ## Eric Sunshine, 2022-12-29 17:06 Subject: Re: [PATCH] grep: simplify is_empty_line Message-ID: URL: https://gitlist.dev/e/CAPig%2BcSFAUcU74qULYkN7OX4-OqU_3VJeTdb1ZH_QoOW9FBwZQ%40mail.gmail.com In-Reply-To: ``` On Thu, Dec 29, 2022 at 12:00 PM Rose via GitGitGadget wrote: > diff --git a/grep.c b/grep.c > @@ -1483,9 +1483,10 @@ static int fill_textconv_grep(struct repository *r, > static int is_empty_line(const char *bol, const char *eol) > { > - while (bol < eol && isspace(*bol)) > - bol++; > - return bol == eol; > + while (bol < eol) > + if (!isspace(*bol)) > + return 0; > + return 1; > } The rewritten code appears to be quite broken. It never increments `bol`. ``` ## Rose via GitGitGadget, 2022-12-29 17:18 Subject: [PATCH v2] grep: simplify is_empty_line Message-ID: URL: https://gitlist.dev/e/pull.1418.v2.git.git.1672334314401.gitgitgadget%40gmail.com In-Reply-To: ``` From: Seija Kijin Signed-off-by: Seija Kijin --- grep: simplify is_empty_line Signed-off-by: Seija Kijin doremylover123@gmail.com Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1418%2FAtariDreams%2FisEmpty-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1418/AtariDreams/isEmpty-v2 Pull-Request: https://github.com/git/git/pull/1418 Range-diff vs v1: 1: 03a97005a67 ! 1: b193c9537de grep: simplify is_empty_line @@ grep.c: static int fill_textconv_grep(struct repository *r, static int is_empty_line(const char *bol, const char *eol) { - while (bol < eol && isspace(*bol)) -- bol++; -- return bol == eol; -+ while (bol < eol) ++ while (bol < eol) { + if (!isspace(*bol)) + return 0; + bol++; +- return bol == eol; ++ } + return 1; } grep.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/grep.c b/grep.c index 06eed694936..d78ee08da6f 100644 --- a/grep.c +++ b/grep.c @@ -1483,9 +1483,12 @@ static int fill_textconv_grep(struct repository *r, static int is_empty_line(const char *bol, const char *eol) { - while (bol < eol && isspace(*bol)) + while (bol < eol) { + if (!isspace(*bol)) + return 0; bol++; - return bol == eol; + } + return 1; } static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int collect_hits) base-commit: 6bae53b138a1f38d8887f6b46d17661357a1468b -- gitgitgadget ``` ## Eric Sunshine, 2022-12-29 17:45 Subject: Re: [PATCH v2] grep: simplify is_empty_line Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTmW_7eXhH0-y0cj_r1UqDc%2BJJvMWVSFKOYSYFvpkjQJw%40mail.gmail.com In-Reply-To: ``` On Thu, Dec 29, 2022 at 12:18 PM Rose via GitGitGadget wrote: > Signed-off-by: Seija Kijin > --- > diff --git a/grep.c b/grep.c > @@ -1483,9 +1483,12 @@ static int fill_textconv_grep(struct repository *r, > static int is_empty_line(const char *bol, const char *eol) > { > - while (bol < eol && isspace(*bol)) > + while (bol < eol) { > + if (!isspace(*bol)) > + return 0; > bol++; > - return bol == eol; > + } > + return 1; > } It is subjective (personal opinion) whether or not the new code is clearer than the original. As a general policy, this project tends not to accept patches like this which merely "churn" the code without improving it. From Documentation/SubmittingPatches: "Once it _is_ in the tree, it's not really worth the patch noise to go and fix it up." Cf. http://lkml.iu.edu/hypermail/linux/kernel/1001.3/01069.html One reason for avoiding churn is that even simple and innocuous changes like this can introduce bugs or unwanted behavior, as v1 of this patch illustrated[1]. Another reason is that it eats up reviewer time. Did the Git test suite pass with v1 of this patch even though it was buggy? If so, a better use of your time and reviewer time would be to improve test coverage so that it detects the sort of breakage caused by v1. [1]: https://lore.kernel.org/git/CAPig+cSFAUcU74qULYkN7OX4-OqU_3VJeTdb1ZH_QoOW9FBwZQ@mail.gmail.com/ ```