| Message ID | 96b81f3573d3f29bb97e77d623be0d53cf8085b0.1710781235.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | New config option for git-grep to include untracked files | expand |
On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@manjaro.org> wrote: > Move some variable definitions around, and reflow one comment block, to > make the code a bit neater after spotting those slightly unpolished areas. > There are no functional changes to the source code. > > Signed-off-by: Dragan Simic <dsimic@manjaro.org> > --- > diff --git a/builtin/grep.c b/builtin/grep.c > @@ -623,13 +623,13 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, > - struct repository *repo = opt->repo; > - int hit = 0; > + int hit = 0, name_base_len = 0; > + int old_baselen = base->len; > enum interesting match = entry_not_interesting; > + struct repository *repo = opt->repo; > struct name_entry entry; > - int old_baselen = base->len; > struct strbuf name = STRBUF_INIT; > - int name_base_len = 0; > @@ -890,19 +890,15 @@ static int pattern_callback(const struct option *opt, const char *arg, > - int hit = 0; > + int hit = 0, seen_dashdash = 0, use_index = 1; > int cached = 0, untracked = 0, opt_exclude = -1; > - int seen_dashdash = 0; > int external_grep_allowed__ignored; > + int i, dummy, allow_revs; > const char *show_in_pager = NULL, *default_pager = "dummy"; > struct grep_opt opt; > struct object_array list = OBJECT_ARRAY_INIT; > struct pathspec pathspec; > struct string_list path_list = STRING_LIST_INIT_DUP; > - int i; > - int dummy; > - int use_index = 1; > - int allow_revs; It's entirely subjective, of course, so no right-or-wrong answer, but I personally do not find that this change improves code quality or readability. With my reviewer hat on, I spent an inordinate amount of time staring at this change trying to locate each variable's new location to verify that no initializers were dropped and that the declared type hadn't changed. Taking into consideration that reviewers are a limited resource on this project, I'd probably have skipped this patch altogether if I were doing this series unless these changes concretely help a subsequent patch.
On 2024-03-18 20:59, Eric Sunshine wrote: > On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> Move some variable definitions around, and reflow one comment block, >> to >> make the code a bit neater after spotting those slightly unpolished >> areas. >> There are no functional changes to the source code. >> >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >> --- >> diff --git a/builtin/grep.c b/builtin/grep.c >> @@ -623,13 +623,13 @@ static int grep_tree(struct grep_opt *opt, const >> struct pathspec *pathspec, >> - struct repository *repo = opt->repo; >> - int hit = 0; >> + int hit = 0, name_base_len = 0; >> + int old_baselen = base->len; >> enum interesting match = entry_not_interesting; >> + struct repository *repo = opt->repo; >> struct name_entry entry; >> - int old_baselen = base->len; >> struct strbuf name = STRBUF_INIT; >> - int name_base_len = 0; >> @@ -890,19 +890,15 @@ static int pattern_callback(const struct option >> *opt, const char *arg, >> - int hit = 0; >> + int hit = 0, seen_dashdash = 0, use_index = 1; >> int cached = 0, untracked = 0, opt_exclude = -1; >> - int seen_dashdash = 0; >> int external_grep_allowed__ignored; >> + int i, dummy, allow_revs; >> const char *show_in_pager = NULL, *default_pager = "dummy"; >> struct grep_opt opt; >> struct object_array list = OBJECT_ARRAY_INIT; >> struct pathspec pathspec; >> struct string_list path_list = STRING_LIST_INIT_DUP; >> - int i; >> - int dummy; >> - int use_index = 1; >> - int allow_revs; > > It's entirely subjective, of course, so no right-or-wrong answer, but > I personally do not find that this change improves code quality or > readability. > > With my reviewer hat on, I spent an inordinate amount of time staring > at this change trying to locate each variable's new location to verify > that no initializers were dropped and that the declared type hadn't > changed. Taking into consideration that reviewers are a limited > resource on this project, I'd probably have skipped this patch > altogether if I were doing this series unless these changes concretely > help a subsequent patch. Oh, I'm fully aware that the reviewers are a limited resource, and I do agree that all this is subjective. Though, I believe it makes the code look nicer, which is the only reason why I performed and submitted those changes in the first place. Though, maybe it would've been better if I submitted these changes as a separate patch, instead as part of this series.
Eric Sunshine <sunshine@sunshineco.com> writes: > It's entirely subjective, of course, so no right-or-wrong answer, but > I personally do not find that this change improves code quality or > readability. I agree that this is entirely subjective. To those who wrote these variable decls and inits, what they wrote was the most readable, wasn't it? It probably falls into the "to some readers the existing code may not be perfect, but once it is written, it is not worth a patch noise to fix it" category. > With my reviewer hat on, I spent an inordinate amount of time staring > at this change trying to locate each variable's new location to verify > that no initializers were dropped and that the declared type hadn't > changed. It is true that "cleaning up, no behaviour changes intended" patches are unpleasant to review. They are boring to read, and the risk of breakage due to mistake is unnecessary and severe. But if the result is objectively better, such a one-time cost may be worth it. We are investing into the better future. For example, we may have an unsorted mess of an enum definition, and we do appreciate in the longer run, such a definition were "more or less" sorted within the constraint of some other criteria (like, "errors get negative value"). If the enum is a huge one, it may need some careful reviewing to verify such a change that turns the unsorted mess into a sorted nice list, but the cost of doing so may be justified. Does the change in this patch qualify as "objectively better"? I dunno. Thanks.
On 2024-03-19 01:32, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> It's entirely subjective, of course, so no right-or-wrong answer, but >> I personally do not find that this change improves code quality or >> readability. > > I agree that this is entirely subjective. To those who wrote these > variable decls and inits, what they wrote was the most readable, > wasn't it? It probably falls into the "to some readers the existing > code may not be perfect, but once it is written, it is not worth a > patch noise to fix it" category. There's no doubt that it was the most readable form to the people who wrote the code, which was some time ago, but the time inevitably passes and the surrounding code changes over time, maybe even some new coding guidelines become introduced, etc. Things inevitably change, that's all I'm trying to say. >> With my reviewer hat on, I spent an inordinate amount of time staring >> at this change trying to locate each variable's new location to verify >> that no initializers were dropped and that the declared type hadn't >> changed. > > It is true that "cleaning up, no behaviour changes intended" patches > are unpleasant to review. They are boring to read, and the risk of > breakage due to mistake is unnecessary and severe. > > But if the result is objectively better, such a one-time cost may be > worth it. We are investing into the better future. For example, we > may have an unsorted mess of an enum definition, and we do > appreciate in the longer run, such a definition were "more or less" > sorted within the constraint of some other criteria (like, "errors > get negative value"). If the enum is a huge one, it may need some > careful reviewing to verify such a change that turns the unsorted > mess into a sorted nice list, but the cost of doing so may be > justified. I fully agree that reviewing code-cleanup patches it usually boring and often taxing. I mean, why change something that has served us well for years, just to make it look nicer in someone's eyes? What does even "nicer" mean to everyone? Well, I sometimes look at the code as if it were a beautiful painting. To some people, it doesn't matter if a painting has some rough areas, as long as it can be hung on a wall. To them, it's just a square thing. Though, to some other people, spotting such rough areas is what makes the painting less beautiful to them. Paintings usually cannot be fixed easily, but fixing the code is much easier. Of course, "nicer" is very hard to define, but I believe that, in the domain of computing, it could be partially defined as "more consistent and flowing better". That's what I tried to achieve with this patch, and I hope I've managed to convey my viewpoint. > Does the change in this patch qualify as "objectively better"? I > dunno. I'd say that these changes qualify as "semi-objectively nicer", but surely not as "objectively better". For any changes to be objectively better, they'd need to introduce some functional changes to the code, while a well-performed code cleanup actually introduces no functional changes.
diff --git a/builtin/grep.c b/builtin/grep.c index 982bcfc4b1df..af89c8b5cb19 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -623,13 +623,13 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec, struct tree_desc *tree, struct strbuf *base, int tn_len, int check_attr) { - struct repository *repo = opt->repo; - int hit = 0; + int hit = 0, name_base_len = 0; + int old_baselen = base->len; enum interesting match = entry_not_interesting; + struct repository *repo = opt->repo; struct name_entry entry; - int old_baselen = base->len; struct strbuf name = STRBUF_INIT; - int name_base_len = 0; + if (repo->submodule_prefix) { strbuf_addstr(&name, repo->submodule_prefix); name_base_len = name.len; @@ -890,19 +890,15 @@ static int pattern_callback(const struct option *opt, const char *arg, int cmd_grep(int argc, const char **argv, const char *prefix) { - int hit = 0; + int hit = 0, seen_dashdash = 0, use_index = 1; int cached = 0, untracked = 0, opt_exclude = -1; - int seen_dashdash = 0; int external_grep_allowed__ignored; + int i, dummy, allow_revs; const char *show_in_pager = NULL, *default_pager = "dummy"; struct grep_opt opt; struct object_array list = OBJECT_ARRAY_INIT; struct pathspec pathspec; struct string_list path_list = STRING_LIST_INIT_DUP; - int i; - int dummy; - int use_index = 1; - int allow_revs; struct option options[] = { OPT_BOOL(0, "cached", &cached, @@ -1059,9 +1055,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) recurse_submodules = 0; /* - * skip a -- separator; we know it cannot be - * separating revisions from pathnames if - * we haven't even had any patterns yet + * skip a -- separator; we know it cannot be separating revisions + * from pathnames if we haven't even had any patterns yet */ if (argc > 0 && !opt.pattern_list && !strcmp(argv[0], "--")) { argv++;
Move some variable definitions around, and reflow one comment block, to make the code a bit neater after spotting those slightly unpolished areas. There are no functional changes to the source code. Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- builtin/grep.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)