Re: [PATCH v2] grep: simplify is_empty_line

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 29, 2022, 17:45 UTC
Message-ID: <CAPig+cTmW_7eXhH0-y0cj_r1UqDc+JJvMWVSFKOYSYFvpkjQJw@mail.gmail.com>
In-Reply-To: <pull.1418.v2.git.git.1672334314401.gitgitgadget@gmail.com>

On Thu, Dec 29, 2022 at 12:18 PM Rose via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 15 quoted lines

> 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/