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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2024, 00:32 UTC
Message-ID: <xmqqjzlzt61d.fsf@gitster.g>
In-Reply-To: <CAPig+cQ6Y2oOaPkKFsD41beXLHjhD++nmf59xrcswpb6_Q-sdA@mail.gmail.com>

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.