[PATCH] grep: simplify is_empty_line

[PATCH] grep: simplify is_empty_line

[Rose via GitGitGadget]

From: Seija Kijin <doremylover123@gmail.com>

Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
    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

Re: [PATCH] grep: simplify is_empty_line

[Eric Sunshine]

On Thu, Dec 29, 2022 at 12:00 PM Rose via GitGitGadget
<gitgitgadget@gmail.com> 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`.

[PATCH v2] grep: simplify is_empty_line

[Rose via GitGitGadget]

From: Seija Kijin <doremylover123@gmail.com>

Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
    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

Re: [PATCH v2] grep: simplify is_empty_line

[Eric Sunshine]

On Thu, Dec 29, 2022 at 12:18 PM Rose via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
> 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/