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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2024-03-18 20:59, Eric Sunshine wrote:
On Mon, Mar 18, 2024 at 1:04 PM Dragan Simic <dsimic@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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.




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux