Subject: Re: [PATCH v2 23/33] diff-merges: fix style of functions definitions

From: Sergey Organov <[email-masked]>
Date: Sun, 20 Dec 2020 22:15:39 +0300

Sergey Organov <[email-masked]> writes:

> [...]
>
> Felipe Contreras <[email-masked]> writes:
>
>> Elijah Newren wrote:
>>> Personally, I think that a really important point to keep in mind when
>>> submitting patch series is trying to figure out the easiest way to
>>> move the code from point A to point B, not the route you took to get
>>> from point A to point B.  This is especially true for longer patch
>>> series.  It's common after you've finished a series to discover there
>>> was an easier or cleaner route to follow that would have arrived at
>>> the same end-point.  It's not uncommon for me to spend a significant
>>> chunk of time rebasing and restructuring a patch series to try to
>>> highlight such a better path.  This includes not just style fixups,
>>> but different patch orderings, alternate ways to break up functions,
>>> using different data structures, etc.
>>
>> Me as well.
>>
>> It's extra work for one person, but everyone else benefits, including
>> that one person in the future (who usually forgets why he/she did things
>> in that particular way).
>>
>> Cheers.
>
> Junio C Hamano <[email-masked]> writes:
>
>> Sergey Organov <[email-masked]> writes:
>>
>>> When Junio noticed and pointed to this deficiency, I asked him if I
>>> should fix all the series from the start, or it'd be OK to use fixup
>>> commit. As he didn't answer and nobody else commented either, I opted
>>> for the latter.
>>
>> Sorry if it slipped through the cracks---I get too many discussion
>> threads to pay attention to.
>>
>> Yes, we strongly prefer *not* to keep the honest history that
>> records all the mistakes we made along the way.  Rather, we take the
>> time a topic is still in flight and not yet cast in stone by merged
>> to 'next' as an opportunity to pretend that the topic came to
>> existence in the perfect shape, thanks to collective brain effort.
>>
>> It is our basic courtesy to future developers who has to read our
>> code (i.e. "log -p") to understand what we've been thinking, when
>> they want to fix some stupid bugs we will inevitably leave in our
>> codebase.  It is distracting to read from the beginning of a topic,
>> notice something funny going on and keep moveing to later patches,
>> while harboring puzzlement in our minds, then later discover that
>> the funny thing we noticed earlier was a simple mistake that gets
>> fixed, not some clever trick the reader needs to think deeply to
>> understand.
>
> OK, so I'll do it.
>

While we are at it, Linux kernel developers care enough about style to
provide script that checks patch series for issues and to insist it is
run before submitting.

I've used this script to cleanup these series for the next re-roll and I
think that having semi-automated test makes a lot of sense, so you may
wish to consider to adopt it for Git as well (it seems to have some
requirements that are not enforced for Git sources, but not many).

The script is called scripts/checkpatch.pl in the Linux kernel sources.

Thanks,
-- Sergey