Re: contributing to Emacs

[Konstantin Kharlamov]

On Sat, 2023-06-24 at 17:44 +0300, Eli Zaretskii wrote:
> > From: Konstantin Kharlamov <hi-angel@yandex.ru>
> > Cc: luangruo@yahoo.com, arne_bab@web.de, ams@gnu.org, emacs-devel@gnu.org
> > Date: Sat, 24 Jun 2023 15:36:20 +0300
> > 
> > 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.
> 
> You might disagree, but such considerations and decisions are the
> prerogative of the project maintainers, not of the contributors.  In
> Emacs, we don't like code churn unless it has a purpose, and
> refactoring by itself doesn't justify the downsides of making the code
> less familiar to those who read and audit it very frequently, and need
> to be able to find the relevant parts as fast as possible.  Keep in
> mind that this is an old project with code written by excellent
> programmers (I exclude myself from that group, obviously), and the
> code is generally in very good shape.  Thus, refactoring is not really
> an urgent need, like it might be in an average project out there.
> 
> Again, you might disagree, but this is not your call.  My point was
> precisely that since it is not the call of the contributors, they
> cannot be expected to make those decisions on our behalf, and are
> therefore better off not dividing the patches into several individual
> ones.
> 
> > > 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.
> 
> I think you misunderstood what I tried to explain, because you are
> actually saying that wasting effort on reviewing of and commenting on
> code that will be changed or reverted by a further commit is a Good
> Thing.

I did not say that.

> But even if you did understand my point, are you really going to tell
> me how to review patches?  Don't you think it's my decision, and not
> yours?  In the project of which you are the maintainer, you can define
> the procedures and the preferences as you see fit (and I will follow
> if I need to contribute, as I do, for example, with GDB, where they do
> want patch series), but you cannot force me use the procedures that
> you find convenient and natural in a project where I need to review so
> many patches each day.

Neither I said how *you* should review patches. You seem to be out of context:
you previous email was a reply to Sean's one-sentence email that they is talking
about the wider ecosystem. And your next reply was not in Emacs-only context,
but instead you were explaining problems with the wider approach. And so my
explanation about code review practices is also in general.

I did not expect you would start applying the practices used in "the wider
ecosystem", because you clearly seem to be fine with how it works for you. My
reply was just an explanation of why other projects do what they are doing.

(also, due to this misunderstanding I opted not to elaborate further on the
other text above, because you seem to be taking this a bit personally)