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

From: Dragan Simic <dsimic@manjaro.org>
Date: Mar 19, 2024, 05:33 UTC
Message-ID: <24093dca675c49cfde39f6d6efca2342@manjaro.org>
In-Reply-To: <xmqqjzlzt61d.fsf@gitster.g>

On 2024-03-19 01:32, Junio C Hamano wrote:

Show 11 quoted lines

> 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.

Show 18 quoted lines

>> 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.