[v2] grep: simplify is_empty_line

Commit Message

Seija Kijin Dec. 29, 2022, 5:18 p.m. UTC

From: Seija Kijin <[e-mail]>

Signed-off-by: Seija Kijin <[e-mail]>
---
    grep: simplify is_empty_line
    
    Signed-off-by: Seija Kijin [e-mail]

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(-)


base-commit: 6bae53b138a1f38d8887f6b46d17661357a1468b

Comments

Eric Sunshine Dec. 29, 2022, 5:45 p.m. UTC | #1

On Thu, Dec 29, 2022 at 12:18 PM Rose via GitGitGadget
<[e-mail]> wrote:
> Signed-off-by: Seija Kijin <[e-mail]>
> ---
> 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/[e-mail]/

Patch

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)