grep: simplify is_empty_line
PATCH 1/1 grep: simplify is_empty_line
From: Seija Kijin <[email protected]>
---
grep: simplify is_empty_line
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(-)
grep.c +4 -3
@@ -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
Signature · 1 line
gitgitgadget
grep.c static int fill_textconv_grep(struct repository *r, · line 1489
Quoted 12 lines · Rose via GitGitGadget
grep.c +4
-3@@ -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 1/1 grep: simplify is_empty_line
In reply to Rose via GitGitGadgetFrom: Seija Kijin <[email protected]>
---
grep: simplify is_empty_line
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(-)
grep.c +5 -2
@@ -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
Signature · 1 line
gitgitgadget
Quoted 5 lines · Rose via GitGitGadget
Signed-off-by Seija Kijin <[email protected]>---
grep.c +0
-0@@ -1483,9 +1483,12 @@ static int fill_textconv_grep(struct repository *r,static int is_empty_line (const char *bol,const char *eol)
grep.c static int fill_textconv_grep(struct repository *r, · line 1491
Quoted 10 lines
grep.c +5
-2{ -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/