GNU bug report logs - #43714
28.1: auto-revert code improvements [PATCH]

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 43714 in the body.
You can then email your comments to 43714 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Emacs Bug Reporting <bug-gnu-emacs <at> gnu.org>
Subject: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 02:26:33 -0400

[Message part 1 (text/plain, inline)]

In pursuing a 'best' solution for bug-report#43412, I've taken the
plunge and gotten an emacs 28 snapshot installed locally, and in
examining the code, saw things that didn't make sense to me or were
unnecessary or could just do with better readability. My testing
indicates no harm done by the changes, but they should be peer-reviewed
and undergo further testing. Nothing in the attached patch is aimed at
changing functionality, so you may want to reject it on the basis of "if
it ain't broke, don't fix it", but it does improve the code.

Just as a 'for example': I started on this because I saw that the code
was saving match data, which had me on a wild goose chase hunting for
why and where it was necessary - I found nothing, and removing the code
doesn't seem to have ill effect.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

[auto-revert-28-cosmetic.patch (text/x-diff, attachment)]

Message #8 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 15:48:05 +0200

Boruch Baum <boruch_baum <at> gmx.com> writes:

> Just as a 'for example': I started on this because I saw that the code
> was saving match data, which had me on a wild goose chase hunting for
> why and where it was necessary - I found nothing, and removing the code
> doesn't seem to have ill effect.

That's really hard to tell, though -- the reason for saving the match
data may be in hard-to-trigger circumstances, and the saving was
introduced in a commit of its own, so I'm guessing there was some
specific reason for it.

The commit message is super-helpful here:

commit 33512cbeb168764f83f5a740090ce40b4b948591
Author:     Luc Teirlinck <teirllm <at> auburn.edu>
AuthorDate: Wed Jun 1 20:51:03 2005 +0000

    (auto-revert-buffers): Use save-match-data.


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #11 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: boruch_baum <at> gmx.com, 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 17:25:41 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Date: Wed, 30 Sep 2020 15:48:05 +0200
> Cc: 43714 <at> debbugs.gnu.org
> 
> commit 33512cbeb168764f83f5a740090ce40b4b948591
> Author:     Luc Teirlinck <teirllm <at> auburn.edu>
> AuthorDate: Wed Jun 1 20:51:03 2005 +0000
> 
>     (auto-revert-buffers): Use save-match-data.

It could be due to remote files (and file-handlers)?

Message #14 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 11:09:38 -0400

On 2020-09-30 15:48, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum <at> gmx.com> writes:
>
> > Just as a 'for example': I started on this because I saw that the code
> > was saving match data, which had me on a wild goose chase hunting for
> > why and where it was necessary - I found nothing, and removing the code
> > doesn't seem to have ill effect.
>
> That's really hard to tell, though -- the reason for saving the match
> data may be in hard-to-trigger circumstances, and the saving was
> introduced in a commit of its own, so I'm guessing there was some
> specific reason for it.
>
> The commit message is super-helpful here:
>
> commit 33512cbeb168764f83f5a740090ce40b4b948591
> Author:     Luc Teirlinck <teirllm <at> auburn.edu>
> AuthorDate: Wed Jun 1 20:51:03 2005 +0000
>
>     (auto-revert-buffers): Use save-match-data.

Seriously now. Should I send an email to Luc, asking him what he was
thinking 15 years ago? Otherwise, I'd be willing to look over the
versions at that commit and it's HEAD^ if you can send them to me.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

Message #17 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 17:12:01 +0200

Boruch Baum <boruch_baum <at> gmx.com> writes:

> Seriously now. Should I send an email to Luc, asking him what he was
> thinking 15 years ago? Otherwise, I'd be willing to look over the
> versions at that commit and it's HEAD^ if you can send them to me.

Pulling down (and building) Emacs is super-duper easy:

sudo apt build-dep emacs
git clone git://git.savannah.gnu.org/emacs.git
cd emacs
make
./src/emacs &


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #20 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 11:28:27 -0400

On 2020-09-30 17:25, Eli Zaretskii wrote:
> >     (auto-revert-buffers): Use save-match-data.
>
> It could be due to remote files (and file-handlers)?

But I don't see anywhere in the code itself being evaluated in
auto-revert.el that would produce its own match-data to potentially
disturb the pre-existing state.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

Message #23 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: larsi <at> gnus.org, 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 19:03:55 +0300

> Date: Wed, 30 Sep 2020 11:28:27 -0400
> From: Boruch Baum <boruch_baum <at> gmx.com>
> Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 43714 <at> debbugs.gnu.org
> 
> On 2020-09-30 17:25, Eli Zaretskii wrote:
> > >     (auto-revert-buffers): Use save-match-data.
> >
> > It could be due to remote files (and file-handlers)?
> 
> But I don't see anywhere in the code itself being evaluated in
> auto-revert.el that would produce its own match-data to potentially
> disturb the pre-existing state.

That's what I meant: it could be hidden from plain sight.

Why is that a problem to leave save-match-data alone?  Does it do any
harm?

Message #26 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 43714 <at> debbugs.gnu.org
Subject: [boruch_baum <at> gmx.com: Re: bug#43714: 28.1: auto-revert code
 improvements [PATCH]]
Date: Wed, 30 Sep 2020 12:51:27 -0400

On 2020-09-30 17:12, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum <at> gmx.com> writes:
>
> > Seriously now. Should I send an email to Luc, asking him what he was
> > thinking 15 years ago? Otherwise, I'd be willing to look over the
> > versions at that commit and it's HEAD^ if you can send them to me.
>
> Pulling down (and building) Emacs is super-duper easy:

No, it wasn't. I spent a lot of time last week failing to resolve the
ensuing dependency hell generated.

>
> sudo apt build-dep emacs
> git clone git://git.savannah.gnu.org/emacs.git
> cd emacs
> make
> ./src/emacs &

Maybe I wasn't clear. I meant just the file autorevert.el from the
commit and its HEAD^, not the entire emacs snapshot at those points.

Apart from that, all I need is probably less than 5kb of data. You're
asking me to pull in how many mega-bytes / giga-bytes of git for this?
Had I the extra space and computing power, I would have done it ages
ago (seven year old pentium...).

Another possibility would be if there is there a way to query the
savannah cgit web interface for that individual file at the snapshot and
its antecedent?

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

Message #29 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: 43714 <at> debbugs.gnu.org
Subject: Re: [boruch_baum <at> gmx.com: Re: bug#43714: 28.1: auto-revert code
 improvements [PATCH]]
Date: Wed, 30 Sep 2020 18:54:44 +0200

Boruch Baum <boruch_baum <at> gmx.com> writes:

>> Pulling down (and building) Emacs is super-duper easy:
>
> No, it wasn't. I spent a lot of time last week failing to resolve the
> ensuing dependency hell generated.
>
>> sudo apt build-dep emacs
>> git clone git://git.savannah.gnu.org/emacs.git
>> cd emacs
>> make
>> ./src/emacs &
>
> Maybe I wasn't clear. I meant just the file autorevert.el from the
> commit and its HEAD^, not the entire emacs snapshot at those points.

Even if you don't want to build Emacs, "git clone" gives you the tree
and you can investigate whetever you want.

> Apart from that, all I need is probably less than 5kb of data. You're
> asking me to pull in how many mega-bytes / giga-bytes of git for this?
> Had I the extra space and computing power, I would have done it ages
> ago (seven year old pentium...).

I didn't know they were still making Pentium CPUs seven years ago.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #32 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 12:59:34 -0400

On 2020-09-30 19:03, Eli Zaretskii wrote:
> Why is that a problem to leave save-match-data alone?  Does it do any
> harm?

I explicitly said in the introduction to the patch that it wasn't
'fixing' anything broken, just cleaning things up (potentially /
hopefully). When I was researching how to best implement that "auto-revert
only if visible" feature, I found myself puzzling over some parts of the
code and came to clean it up to be better able to wok on it.

That said, You could leave the (save-match-data alone, and accept the other
changes; the other proposed changes are clearly independent.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

Message #35 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 19:01:54 +0200

Boruch Baum <boruch_baum <at> gmx.com> writes:

> That said, You could leave the (save-match-data alone, and accept the other
> changes; the other proposed changes are clearly independent.

Could you submit a patch with the save-match-data left intact?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #38 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 43714 <at> debbugs.gnu.org
Subject: Re: [boruch_baum <at> gmx.com: Re: bug#43714: 28.1: auto-revert code
 improvements [PATCH]]
Date: Wed, 30 Sep 2020 13:10:33 -0400

On 2020-09-30 18:54, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum <at> gmx.com> writes:
>
> Even if you don't want to build Emacs, "git clone" gives you the tree
> and you can investigate whetever you want.

Then you read my next paragraph...

> > Apart from that, all I need is probably less than 5kb of data. You're
> > asking me to pull in how many mega-bytes / giga-bytes of git for this?
> > Had I the extra space and computing power, I would have done it ages
> > ago (seven year old pentium...).
>
> I didn't know they were still making Pentium CPUs seven years ago.

dmidecode tells me its a pentium; I routinely forget all the detailed
hardware internals of my machine, but I remember purchasing it new seven
years ago.

So, returning to focus on the work. option 1: just flat out reject that
line of the patch; option 2: send me the old versions of the files;
option 3: tell me how to use the savannah cgit interface to pull the two
files.

This all being volunteer work, all options are of course themselves
optional.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

Message #41 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 13:13:47 -0400

On 2020-09-30 19:01, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum <at> gmx.com> writes:
>
> > That said, You could leave the (save-match-data alone, and accept the other
> > changes; the other proposed changes are clearly independent.
>
> Could you submit a patch with the save-match-data left intact?

Does that mean that the remainder is acceptable / accepted without
further discussion? If I'm going to make more submissions, I'd prefer to
minimize / aggregate the work.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

Message #44 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Michael Albinus <michael.albinus <at> gmx.de>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, boruch_baum <at> gmx.com,
 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 19:21:00 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> commit 33512cbeb168764f83f5a740090ce40b4b948591
>> Author:     Luc Teirlinck <teirllm <at> auburn.edu>
>> AuthorDate: Wed Jun 1 20:51:03 2005 +0000
>>
>>     (auto-revert-buffers): Use save-match-data.
>
> It could be due to remote files (and file-handlers)?

Could be the case. There is no promise that the file name handlers do
not change match data. And I'm kind of sure this save-match-data form
has been introduced after such an accident, that auto-revert has
corrupted somebody's match data.

So please keep it. It doesn't hurt.

Best regards, Michael.

Message #47 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Glenn Morris <rgm <at> gnu.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 13:41:08 -0400

The mailing list archives around the time of a commit are often informative.

https://lists.gnu.org/r/emacs-devel/2005-05/msg01414.html

Message #50 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Glenn Morris <rgm <at> gnu.org>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>,
 Michael Albinus <michael.albinus <at> gmx.de>, 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Wed, 30 Sep 2020 14:33:44 -0400

[Message part 1 (text/plain, inline)]

On 2020-09-30 13:41, Glenn Morris wrote:
> The mailing list archives around the time of a commit are often informative.
> https://lists.gnu.org/r/emacs-devel/2005-05/msg01414.html

Absolutely great, Glenn!

An obvious follow-up observation: The patch that was applied was for
Luc's option #3, but it seems to me that the honest solution to the
problem with timers is his option #2, to put the fix in timers. Was that
ever done?

I'm guessing a decision at some point was made not to pursue Luc's
option #2 because the emacs documentation for timers[1] says, "If a
timer function calls functions that can change the match data, it should
save and restore the match data. See Saving Match Data". Reconsidering
that decision could be of benefit emacs-wide.

Attached is a new version of the patch, with the 'save-match-data'
restored. Thanks again, Glenn.

[1] https://www.gnu.org/software/emacs/manual/html_node/elisp/Timers.html

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

[auto-revert-28-cosmetic.patch (text/x-diff, attachment)]

Message #53 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Boruch Baum <boruch_baum <at> gmx.com>
Cc: Glenn Morris <rgm <at> gnu.org>, Michael Albinus <michael.albinus <at> gmx.de>,
 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Thu, 01 Oct 2020 01:55:13 +0200

Boruch Baum <boruch_baum <at> gmx.com> writes:

> Attached is a new version of the patch, with the 'save-match-data'
> restored. Thanks again, Glenn.

Now applied to Emacs 28 (with some changes).  As far as I can see, it's
just a refactoring of auto-revert-buffers?  Normally, we don't do pure
refactorings like this (because it makes code archaeology more
difficult), but this one seemed pretty nice.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #60 received at 43714 <at> debbugs.gnu.org (full text, mbox):

From: Boruch Baum <boruch_baum <at> gmx.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Glenn Morris <rgm <at> gnu.org>, Michael Albinus <michael.albinus <at> gmx.de>,
 43714 <at> debbugs.gnu.org
Subject: Re: bug#43714: 28.1: auto-revert code improvements [PATCH]
Date: Thu, 1 Oct 2020 01:42:04 -0400

On 2020-10-01 01:55, Lars Ingebrigtsen wrote:
> Boruch Baum <boruch_baum <at> gmx.com> writes:
>
> Now applied to Emacs 28 (with some changes).  As far as I can see, it's
> just a refactoring of auto-revert-buffers?

Hope is, yes.

>  Normally, we don't do pure refactorings like this (because it makes
> code archaeology more difficult), but this one seemed pretty nice.

Thanks. I suspect that it may turn out to come in handy for bug #43412
'autorevert-only-if-visible', which will be a case of performing an
instant auto-revert on a single buffer (or small set of buffers) without
using a timer.

--
hkp://keys.gnupg.net
CA45 09B5 5351 7C11 A9D1  7286 0036 9E45 1595 8BC0

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.