Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Po Lu]

Yuan Fu <casouri@gmail.com> writes:

> branch: emacs-29
> commit 1ef8b90ae06d698ab2ba9b43f67fde7289db2c5d
> Author: Randy Taylor <dev@rjt.dev>
> Commit: Yuan Fu <casouri@gmail.com>
>
>     Simplify imenu setup for {cmake,dockerfile}-ts-modes

Why was this installed on emacs-29?  It is no bugfix.

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Randy Taylor <dev@rjt.dev>
> Date: Sat, 27 Jan 2024 12:59:07 +0800
> 
> Yuan Fu <casouri@gmail.com> writes:
> 
> > branch: emacs-29
> > commit 1ef8b90ae06d698ab2ba9b43f67fde7289db2c5d
> > Author: Randy Taylor <dev@rjt.dev>
> > Commit: Yuan Fu <casouri@gmail.com>
> >
> >     Simplify imenu setup for {cmake,dockerfile}-ts-modes
> 
> Why was this installed on emacs-29?  It is no bugfix.

Bug#68708 claims that this is a bugfix.

Btw, Yuan: please always mention the bug number in the commit log
messages.  If you install someone else's change via "git am", and the
author didn't mention the bug number, please use "git commit --amend"
afterwards to add the bug number (or modify the log message in any
other way), before you push.

Thanks.

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Why was this installed on emacs-29?  It is no bugfix.
>
> Bug#68708 claims that this is a bugfix.

??? That is a Coreutils bug.  But I see bug#68705 in the bug tracker
now.

> Btw, Yuan: please always mention the bug number in the commit log
> messages.  If you install someone else's change via "git am", and the
> author didn't mention the bug number, please use "git commit --amend"
> afterwards to add the bug number (or modify the log message in any
> other way), before you push.

Yes, please.  Furthermore, if such bugfix cannot be described as
anything but a simplification (with a correspondingly large diff), there
should be another approach safer for the release branch.

Quoting Yuan's response on the bug tracker:

> Thanks Randy, pushed to emacs-29 since it includes a fix.
>
> Yuan

Our basic criterion for installing patches on emacs-29 is not whether a
change contains a bugfix, but whether it runs a risk of destablizing the
branch.  It is more important that a bugfix be the only component of a
change, since that reduces the probability of unanticipated disruption.

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Stefan Kangas]

Po Lu <luangruo@yahoo.com> writes:

> Yes, please.  Furthermore, if such bugfix cannot be described as
> anything but a simplification (with a correspondingly large diff), there
> should be another approach safer for the release branch.

We must not get overly rigid about that.  It takes extra work, and it
will not always be clear that it's worth the effort.  Often it is, but
sometimes it isn't.  We tend to decide on a case-by-case basis.

Furthermore, please keep in mind that Yuan is both the principal author
and maintainer of our treesitter support.  He's closer to the code than
anyone else, and is thus in the best position to make such judgement
calls.  I see no reason to second-guess him here, myself.

If there are any specific technical arguments for why this particular
change must not be installed on emacs-29, then let's hear them.  Neither
generalizations nor administrative arguments will cut it, I think.

> Our basic criterion for installing patches on emacs-29 is not whether a
> change contains a bugfix, but whether it runs a risk of destablizing the
> branch.  It is more important that a bugfix be the only component of a
> change, since that reduces the probability of unanticipated disruption.

(Speaking here as the co-maintainer of GNU Emacs, responsible for
leading Emacs development together with Eli.)

The above does not necessarily reflect the official line of the project.

It is true that we have preferred to avoid bug fixes that might risk
destabilizing the branch, but this has usually been applied in an ad-hoc
and not in a rigid fashion.  I'm not myself prepared to commit to much
more rigid guidelines at this point.

On a personal note, if we believe that some changed guidelines are
necessary, I'd probably be more inclined to make things more relaxed
rather than more strict.  But please note that I'm _not_ proposing to
open a discussion about that at the present time.

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Po Lu]

Stefan Kangas <stefankangas@gmail.com> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Yes, please.  Furthermore, if such bugfix cannot be described as
>> anything but a simplification (with a correspondingly large diff), there
>> should be another approach safer for the release branch.
>
> We must not get overly rigid about that.  It takes extra work, and it
> will not always be clear that it's worth the effort.  Often it is, but
> sometimes it isn't.  We tend to decide on a case-by-case basis.
>
> Furthermore, please keep in mind that Yuan is both the principal author
> and maintainer of our treesitter support.  He's closer to the code than
> anyone else, and is thus in the best position to make such judgement
> calls.  I see no reason to second-guess him here, myself.

The burden of proof is on the author of the patch, who might be the most
qualified person to comment on tree-sitter, but certainly not imenu,
autoloads, or other areas of Emacs no less affected by this change.  The
bug fix in this change should have been separately installed on
emacs-29, with the rest on master.

> If there are any specific technical arguments for why this particular
> change must not be installed on emacs-29, then let's hear them.  Neither
> generalizations nor administrative arguments will cut it, I think.

I haven't heard any specific technical arguments as to why this change
is safe.  Rather, I'm the person who is frequently told that such
specific statements regarding the safety of a large change are "famous
last words"--which they are, all too often, so I have taken that advice
to heart.

> The above does not necessarily reflect the official line of the project.

??? I was summarizing what I myself have been told and observed over a
certain length of time.

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Stefan Kangas]

Po Lu <luangruo@yahoo.com> writes:

> The burden of proof is on the author of the patch, who might be the most
> qualified person to comment on tree-sitter, but certainly not imenu,
> autoloads, or other areas of Emacs no less affected by this change.

? They are not affected by this change though?  Are we looking at the
same patch (commit 1ef8b90a)?

Which areas of Emacs do you think are affected?  AFAICT, nothing risks
breaking outside of the changed modes.  For example, autoloading is not
affected (`declare-function' only affects byte-compilation).

>> If there are any specific technical arguments for why this particular
>> change must not be installed on emacs-29, then let's hear them.  Neither
>> generalizations nor administrative arguments will cut it, I think.
>
> I haven't heard any specific technical arguments as to why this change
> is safe.

Did you read Bug#68706?  Yuan decided to install it as a bugfix on
emacs-29.

The question is if this change is different from the many other bug
fixes that we routinely install without much justification.  This is why
I'm asking for something more specific.

> Rather, I'm the person who is frequently told that such specific
> statements regarding the safety of a large change are "famous last
> words"--which they are, all too often, so I have taken that advice to
> heart.

I don't know which changes this is in reference to, but I know that you
often work on things like X support, that carries with it a bigger risk
for breakage (as it's more complex), and it affects more users to boot.
It is natural to be more careful with changes in core functionality than
in specific modes.

Note also that we have been decided to be more lax when it comes to the
treesitter stuff, seeing as its new in Emacs 29.

>> The above does not necessarily reflect the official line of the project.
>
> ??? I was summarizing what I myself have been told and observed over a
> certain length of time.

Maybe I wasn't clear enough.  I didn't entirely agree with the summary,
because it was too rigid to adequately summarize what I understand to
have been our policy so far.  What's more, the way it was formulated, I
felt like it was open to the misinterpretation that it reflected some
official decision.

I hope it is more clear now what it is that I wanted to clarify.

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Po Lu]

Stefan Kangas <stefankangas@gmail.com> writes:

> ? They are not affected by this change though?  Are we looking at the
> same patch (commit 1ef8b90a)?
>
> Which areas of Emacs do you think are affected?  AFAICT, nothing risks
> breaking outside of the changed modes.  For example, autoloading is not
> affected (`declare-function' only affects byte-compilation).

You are correct as to autoloads, but it certainly affects imenu.

> Did you read Bug#68706?  Yuan decided to install it as a bugfix on
> emacs-29.
>
> The question is if this change is different from the many other bug
> fixes that we routinely install without much justification.  This is why
> I'm asking for something more specific.

Yes I did: it's larger, and encompasses much more than only the fix.
Yuan's precise wording was:

> Thanks Randy, pushed to emacs-29 since it includes a fix.

which does not suggest that the safety of the patch in whole was a
factor in his decision, but only the fact that a bugfix was one
component of the patch.  This prompted my reply that the inclusion of a
bugfix is not the sole criterion we expect changes to the release branch
to satisfy, and I trust we are in agreement in this respect.

> I don't know which changes this is in reference to, but I know that you
> often work on things like X support, that carries with it a bigger risk
> for breakage (as it's more complex), and it affects more users to boot.
> It is natural to be more careful with changes in core functionality than
> in specific modes.
>
> Note also that we have been decided to be more lax when it comes to the
> treesitter stuff, seeing as its new in Emacs 29.

No disagreements there, but "the treesitter stuff" doesn't extend to
imenu setup code in major modes which happen to make use of tree-sitter.

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Dmitry Gutov]

On 28/01/2024 00:40, Stefan Kangas wrote:
> It is true that we have preferred to avoid bug fixes that might risk
> destabilizing the branch, but this has usually been applied in an ad-hoc
> and not in a rigid fashion.  I'm not myself prepared to commit to much
> more rigid guidelines at this point.

We also tend to me more strict when a release is imminent, and less so 
otherwise.

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 27 Jan 2024 14:40:02 -0800
> Cc: emacs-devel@gnu.org, dev@rjt.dev
> 
> Po Lu <luangruo@yahoo.com> writes:
> 
> > Yes, please.  Furthermore, if such bugfix cannot be described as
> > anything but a simplification (with a correspondingly large diff), there
> > should be another approach safer for the release branch.
> 
> We must not get overly rigid about that.  It takes extra work, and it
> will not always be clear that it's worth the effort.  Often it is, but
> sometimes it isn't.  We tend to decide on a case-by-case basis.
> 
> Furthermore, please keep in mind that Yuan is both the principal author
> and maintainer of our treesitter support.  He's closer to the code than
> anyone else, and is thus in the best position to make such judgement
> calls.  I see no reason to second-guess him here, myself.
> 
> If there are any specific technical arguments for why this particular
> change must not be installed on emacs-29, then let's hear them.  Neither
> generalizations nor administrative arguments will cut it, I think.

My arguments for not objecting to this change on the release branch:

 . the change only affects these two modes
 . the modes are new in Emacs 29, so their fixes should preferably be
   in Emacs 29 as well

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Yuan Fu]

Hey sorry, I didn’t see this thread. I did forget to include the bug number, will make sure I do next time.

> On Jan 27, 2024, at 9:57 PM, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Stefan Kangas <stefankangas@gmail.com>
>> Date: Sat, 27 Jan 2024 14:40:02 -0800
>> Cc: emacs-devel@gnu.org, dev@rjt.dev
>> 
>> Po Lu <luangruo@yahoo.com> writes:
>> 
>>> Yes, please.  Furthermore, if such bugfix cannot be described as
>>> anything but a simplification (with a correspondingly large diff), there
>>> should be another approach safer for the release branch.
>> 
>> We must not get overly rigid about that.  It takes extra work, and it
>> will not always be clear that it's worth the effort.  Often it is, but
>> sometimes it isn't.  We tend to decide on a case-by-case basis.
>> 
>> Furthermore, please keep in mind that Yuan is both the principal author
>> and maintainer of our treesitter support.  He's closer to the code than
>> anyone else, and is thus in the best position to make such judgement
>> calls.  I see no reason to second-guess him here, myself.
>> 
>> If there are any specific technical arguments for why this particular
>> change must not be installed on emacs-29, then let's hear them.  Neither
>> generalizations nor administrative arguments will cut it, I think.
> 
> My arguments for not objecting to this change on the release branch:
> 
> . the change only affects these two modes
> . the modes are new in Emacs 29, so their fixes should preferably be
>   in Emacs 29 as well

The fix comes together with the simplification, so you can’t simply pick it out and only apply the fix. It’ll be a rather wasteful use of time and more error-prone. All the code that the patch removed are copy-paste boilerplate, and the code that replaces them delegate most of the work to existing functions. So I don’t think it’ll destabilize emacs-29.

Yuan

Re: emacs-29 1ef8b90ae06: Simplify imenu setup for {cmake, dockerfile}-ts-modes

[Po Lu]

Yuan Fu <casouri@gmail.com> writes:

>     * lisp/progmodes/cmake-ts-mode.el (treesit-induce-sparse-tree,
>     treesit-node-child, treesit-node-start, cmake-ts-mode--imenu,
>     cmake-ts-mode--imenu-1): Remove.
>     (treesit-search-subtree): Declare.
>     (cmake-ts-mode--function-name): New function.
>     (cmake-ts-mode): Use it.
>     
>     * lisp/progmodes/dockerfile-ts-mode.el (treesit-induce-sparse-tree,
>     treesit-node-start, dockerfile-ts-mode--imenu,
>     dockerfile-ts-mode--imenu-1): Remove.
>     (dockerfile-ts-mode--stage-name): New function.
>     (dockerfile-ts-mode): Use it.

On another score, this ChangeLog entry is formatted incorrectly.
Newlines should never be inserted inside a pair of parentheses!

I see that the `M-q' command in vc-log-mode can fill ChangeLog entries
in this manner, which will be fixed, but for now please take care to
repair any improperly formatted entries it might produce.