Re: contributing to Emacs

[Konstantin Kharlamov <hi-angel@yandex.ru>]

On Sat, 2023-06-24 at 10:43 +0300, Eli Zaretskii wrote:
> 
> What most people do instead is they provide a series where each patch
> is a step towards the solution.  First, a patch with some refactoring,
> then another patch with the first aspect of the solution, another
> patch with the second aspect, etc.  Such series make no sense as a
> series, because the patches are not really independent; instead, they
> are _incremental_.  For example, it usually makes no sense to do the
> refactoring if we aren't installing the changes which need it.

I might be misunderstanding something, but as someone who regularly posts such
"incremental changes" at my work, I tend to disagree that such refactoring will
not be needed if the final change not applied.

The point of such refactoring is improving existing code. Like, separating
larger functions to smaller one, encapsulating logic and reducing variables
scope, constifying stuff… I agree that the driving force for these changes is
making some final change that adds some new feature or something… However, even
if the final feature does not make it through, the changes per se make code
easier to maintain and review. I even had multiple occasions at work where I was
making such incremental changes, but the final point I was doing them for was
never reached for one reason or another. But we still applied the refactoring,
because it's an improvement nonetheless.

> Moreover, this technique frequently leads to multiple patches touching
> the same places several times, so when you review the first patch, you
> are looking at code that will be modified later, and risk providing
> comments that are irrelevant, because a later patch in the series
> rewrites that code anyway, perhaps exactly in a way that you want to
> tell the contributor to use.

Actually, this is exactly how review works. It *does not* matter if later code
rewrote the same place again. If you found a problem in *current* commit/patch,
that means that exactly *this* commit/patch needs to be fixed.

That α) simplifies review, and β) makes sure that if later commit gets reverted
at some point, the code that is left would still be valid.