grep: simplify is_empty_line

[AtariDreams]

Signed-off-by: Seija Kijin doremylover123@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com

[AtariDreams]

/submit

[gitgitgadget-git]

Submitted as pull.1418.git.git.1672333122193.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1418/AtariDreams/isEmpty-v1

To fetch this version to local tag pr-git-1418/AtariDreams/isEmpty-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1418/AtariDreams/isEmpty-v1

[gitgitgadget-git]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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`.

[gitgitgadget-git]

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

[AtariDreams]

/submit

[gitgitgadget-git]

Submitted as pull.1418.v2.git.git.1672334314401.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-git-1418/AtariDreams/isEmpty-v2

To fetch this version to local tag pr-git-1418/AtariDreams/isEmpty-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-git-1418/AtariDreams/isEmpty-v2

[gitgitgadget-git]

On the Git mailing list, Eric Sunshine wrote (reply to this):

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/