Patch for reftex.el: master or release branch?

Patch for reftex.el: master or release branch?

[Arash Esbati]

Dear maintainers,

I have the following patch for reftex.el fixing AUCTeX bug#59638.  This
issue was introduced with commit 1e8bb313ea:

--8<---------------cut here---------------start------------->8---
diff --git a/lisp/textmodes/reftex.el b/lisp/textmodes/reftex.el
index f815419ea4..126b3777f5 100644
--- a/lisp/textmodes/reftex.el
+++ b/lisp/textmodes/reftex.el
@@ -1004,10 +1004,13 @@ reftex-compile-variables
                   reftex-section-levels))

     ;; Calculate the regular expressions
-    (let* (
-;          (wbol "\\(\\`\\|[\n\r]\\)[ \t]*")
-           (wbol "\\(^\\)%?[ \t]*") ; Need to keep the empty group because
-                                    ; match numbers are hard coded
+    (let* (;; (wbol "\\(\\`\\|[\n\r]\\)[ \t]*")
+           ;; Need to keep the empty group because match numbers are
+           ;; hard coded
+           (wbol (concat "\\(^\\)"
+                         (when (string-suffix-p ".dtx" (buffer-file-name) t)
+                           "%")
+                         "[ \t]*"))
            (label-re (concat "\\(?:"
                             (mapconcat #'identity reftex-label-regexps "\\|")
                             "\\)"))
--8<---------------cut here---------------end--------------->8---

Can you please tell me if this should go to master or emacs-29 branch?

TIA.  Best, Arash

Re: Patch for reftex.el: master or release branch?

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Date: Wed, 14 Dec 2022 14:21:23 +0100
> 
> Dear maintainers,
> 
> I have the following patch for reftex.el fixing AUCTeX bug#59638.  This
> issue was introduced with commit 1e8bb313ea:
> 
> --8<---------------cut here---------------start------------->8---
> diff --git a/lisp/textmodes/reftex.el b/lisp/textmodes/reftex.el
> index f815419ea4..126b3777f5 100644
> --- a/lisp/textmodes/reftex.el
> +++ b/lisp/textmodes/reftex.el
> @@ -1004,10 +1004,13 @@ reftex-compile-variables
>                    reftex-section-levels))
> 
>      ;; Calculate the regular expressions
> -    (let* (
> -;          (wbol "\\(\\`\\|[\n\r]\\)[ \t]*")
> -           (wbol "\\(^\\)%?[ \t]*") ; Need to keep the empty group because
> -                                    ; match numbers are hard coded
> +    (let* (;; (wbol "\\(\\`\\|[\n\r]\\)[ \t]*")
> +           ;; Need to keep the empty group because match numbers are
> +           ;; hard coded
> +           (wbol (concat "\\(^\\)"
> +                         (when (string-suffix-p ".dtx" (buffer-file-name) t)
> +                           "%")
> +                         "[ \t]*"))
>             (label-re (concat "\\(?:"
>                              (mapconcat #'identity reftex-label-regexps "\\|")
>                              "\\)"))
> --8<---------------cut here---------------end--------------->8---
> 
> Can you please tell me if this should go to master or emacs-29 branch?

How safe is the change?  If it is safe enough, emacs-29 is fine.

Tassilo, WDYT about the safety of the patch and/or about the urgency
to have the fix?

Re: Patch for reftex.el: master or release branch?

[Tassilo Horn]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli & Arash,

>> --8<---------------cut here---------------start------------->8---
>> diff --git a/lisp/textmodes/reftex.el b/lisp/textmodes/reftex.el
>> index f815419ea4..126b3777f5 100644
>> --- a/lisp/textmodes/reftex.el
>> +++ b/lisp/textmodes/reftex.el
>> @@ -1004,10 +1004,13 @@ reftex-compile-variables
>>                    reftex-section-levels))
>> 
>>      ;; Calculate the regular expressions
>> -    (let* (
>> -;          (wbol "\\(\\`\\|[\n\r]\\)[ \t]*")
>> -           (wbol "\\(^\\)%?[ \t]*") ; Need to keep the empty group because
>> -                                    ; match numbers are hard coded
>> +    (let* (;; (wbol "\\(\\`\\|[\n\r]\\)[ \t]*")
>> +           ;; Need to keep the empty group because match numbers are
>> +           ;; hard coded
>> +           (wbol (concat "\\(^\\)"
>> +                         (when (string-suffix-p ".dtx" (buffer-file-name) t)
>> +                           "%")
>> +                         "[ \t]*"))
>>             (label-re (concat "\\(?:"
>>                              (mapconcat #'identity reftex-label-regexps "\\|")
>>                              "\\)"))
>> --8<---------------cut here---------------end--------------->8---
>> 
>> Can you please tell me if this should go to master or emacs-29
>> branch?
>
> How safe is the change?  If it is safe enough, emacs-29 is fine.
>
> Tassilo, WDYT about the safety of the patch and/or 

I would like to let Arash decide.  I'm not familiar with that code but I
guess he has tested his change and I trust him.

The only thing the patch does is having a % in the regexp in the DTX
case or omitting it in the "normal TeX" case instead of optionally
allowing % in the latter case, too.  That lead to the bug that commented
sections also increased the section counter.

> about the urgency to have the fix?

Well, I guess it's broken since 2016 and only now a user complained.  So
the urgency is not very high.

Bye,
Tassilo

Re: Patch for reftex.el: master or release branch?

[Eli Zaretskii]

> From: Tassilo Horn <tsdh@gnu.org>
> Cc: Arash Esbati <arash@gnu.org>, emacs-devel@gnu.org
> Date: Wed, 14 Dec 2022 15:25:37 +0100
> 
> I would like to let Arash decide.  I'm not familiar with that code but I
> guess he has tested his change and I trust him.
> 
> The only thing the patch does is having a % in the regexp in the DTX
> case or omitting it in the "normal TeX" case instead of optionally
> allowing % in the latter case, too.  That lead to the bug that commented
> sections also increased the section counter.
> 
> > about the urgency to have the fix?
> 
> Well, I guess it's broken since 2016 and only now a user complained.  So
> the urgency is not very high.

Thanks.

So if Arash thinks this is safe, it is fine with me to install this on
emacs-29.

Re: Patch for reftex.el: master or release branch?

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> So if Arash thinks this is safe, it is fine with me to install this on
> emacs-29.

Thanks.  Yes, I think it's safe.  It basically uses the same thing done
in reftex-parse.el[1] as well.  Reg. impact of the current
implementation: I consider the impact higher than in the bug report
since one effectively can't force RefTeX to ignore a file which is
included by \input, \include, \LTXtable etc. by commenting out that
macro.  RefTeX offers a label in such a file for completion while
running LaTeX will throw a warning about unresolved references.

I installed the change (commit 622838b957) on emacs-29 branch.

Best, Arash

Footnotes:
[1]  http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/textmodes/reftex-parse.el#n276