Re: [PATCH 1/5] grep: perform some minor code and comment cleanups

[Eric Sunshine <sunshine@sunshineco.com>]

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.