Subject: RE: [External] : bug#75626: 31.0.50; Dired misses or double-processes files when auto-revert-mode is enabled

From: Drew Adams <[email-masked]>
Date: Mon, 20 Jan 2025 22:32:48 +0000

> >> > That shouldn't happen, IMO.  Too general, and I doubt it's needed.
> >>
> >> This bug contains a recipe showing at least one ocassion where it is
> >> needed.
> >
> > It's needed for the _macro_ to do?  I don't see
> > that demonstrated.
> >
> > An occasion where the macro is used and you want
> > to prevent XYZ should be handled by the _code
> > that invokes the macro_, not by the macro itself,
> > i.e., not by expanding the macro.
> 
> That would be possible, too.  The difference is just one change to the
> macro vs. N changes to different functions like dired-do-compress.

Requiring a user to change the macro is not what
I'd call "the code that invokes the macro"
preventing XYZ.  You seem to be saying that a
user can always redefine the macro.  That excuse
could be offered for _any_ change to the Emacs
code: don't like it, just change the code. 

> >> > One might very well want to allow reversion
> >> > during some particular operation on marked
> >> > files.  Let's not assume otherwise.
> >>
> >> Sure, and that's still allowed, e.g., the code given as BODY of
> >> dired-map-over-marks could explicitly call revert-buffer if it can
> >> handle the result.
> >
> > When you say BODY, do you mean the _function_
> > passed to the macro as its ARG, or the BODY
> > argument?
> 
> It doesn't get a function, it gets a form like (funcall
> #'dired-compress) in the case of dired-do-compress.  You could give it
> somithing like (progn (funcall #'whatever) (revert-buffer)) when you are
> certain that reverting after the operation is safe.

No.  It gets two (possibly 3 or 4) arguments:
BODY, ARG, and possibly SHOW-PROGRESS and
DISTINGUISH-ONE-MARKED.  `C-h f'-it, or check
its definition in the code.

> > In any case, how is an invocation of the macro
> > supposed to override the denial of reversion?
> > Can it simply let-bind a variable around the
> > macro call?  I was guessing that, with your
> > change the macro code itself would override
> > that, e.g., with its own such binding, making
> > it impossible to control the behavior from
> > _around_ the macro call.
> 
> That's true.

So you admit that a code invoking the macro can't
override the behavior you're now hard-coding in it.

> >> The point is that auto-revert-mode reverts at _unpredictable_ moments
> >> where chances are high that the dired buffer contents change in a way
> >> that the processing logic goes wrong, e.g., a marked and not yet
> >> processed file is now before point and will be skipped, or the other
> >> way round, an already processed file is now after point and will be
> >> processed again.
> >
> > Yes, I made clear that I understand that.
> > And I explicitly agreed that that's a no-no.
> >
> > My point was that, until now, it was up to
> > a _user_ to just _not do that_, i.e., not
> > to shoot herself in the foot.  IIUC, Emacs is
> > now preventing her from reverting the buffer,
> > including, but not limited to, via
> > `auto-revert-mode'.
> 
> That's not true.  The user could not manually revert before because as
> Eli explained, Emacs isn't processing key events during the long-running
> operation on marked files through dired-map-over-marks.

We're (I'm) talking about user code, not user
altering the behavior interactively.  It's
not about "manually revert"ing.  It's about
letting `auto-revert-mode' revert during
`dired-map-over-marks'.

And to _disallow_ reversion (auto or other)
during `d-m-o-m' user code can, now, just
bind `revert-buffer-function' to `ignore',
around the call to `dired-map-over-marks'.

If you change `dired-map-over-marks' then
user code can't have any such effect - you're
hard-coding prevention of reversion during
`d-m-o-m'.

> > If so, I'd prefer the original, more general
> > behavior: leave it up to the _calling_ code to
> > decide whether to limit the behavior in that
> > way (or in any other way).  If it's important
> > for the particular use case to prevent doing
> > XYZ then the _calling code_ can, and should,
> > prevent doing XYZ.  The macro shouldn't try to
> > guess what should be prevented - even in the
> > case of buffer reversion, which, I agree, is
> > usually something to be prevented.
> >>
> >> I don't see what feature you think I have stolen from you.  We just
> >> prevent auto-revert-mode from reverting the dired buffer as long as
> >> an operation on marked files is in progress.
> >
> > Why?  Because usually that's a good thing to
> > prevent?  Not good/general enough.  Leave it up
> > to calling code to prevent that.  Add a note
> > about this to the doc string, if you like.  But
> > why have the _macro_ prevent it?
> 
> Because it catches a category of (potential) errors at a central
> location.  It's not unthinkable that users wrote their own processing
> functions which are also vulnerable to misbehave if auto-revert-mode is
> activated.

That might sound reasonable, IF you also provided
a way for user code to override that prohibition
of reverting during `d-m-o-m'. 

> And keep in mind that the changes that make auto-revert-mode revert
> don't need to be "our" changes.  For example, assume you have a dired
> buffer for /tmp, do some operation on marked files, and some other
> process creates files in /tmp causing auto-reverts.  That will produce
> new lines at random locations in your dired buffer which is currently
> processed marked line by marked line with code that relies on the
> position of point in that buffer.  How should processing code handle
> that?  I'd argue it can't and we should make sure the buffer stays
> stable during the operation.

That's why I agreed that usually users will want
to prohibit reversion during `d-m-o-m'.  That's
not the same thing as hard-coding that prohibition
and not giving user code an easy way to override
it.

> >> Progress is still visible
> >> (SHOW-PROGRESS arg of dired-map-over-marks), i.e., the dired buffer is
> >> periodically redisplayed showing the changes so far because that has
> >> nothing to do with auto-revert-mode.
> >
> > No one questioned visibility of progress.
> > Dunno why you mention that.
> 
> I had the impression that this could be the reason for you vehement
> disagreement with the change.

No.  And I don't have a vehement disagreement.
I'm not convinced that the solution being
implemented is required.  Seems like overkill,
to me.  At least provide a variable that the
`d-m-o-m' will respect whose value you can
change to not prohibit <whatever>, including
reversion.  That costs nothing, and you can
even tell users why they generally won't want
to change the variable value.

> Anyway, can you come up with some concrete scenario where inhibiting
> auto-revert-mode for a dired marked files operation could do harm or
> have any negative effect?  That's the thing I don't get.

The negative effect is in removing the possibility
of allowing reversion during `d-m-o-m'.  What's
the argument for hard-coding disallowing it, not
letting code allow it, conditionally or otherwise?

I haven't seen an argument for that.  I've seen
only an argument (of which I'm convinced, and have
said so several times) that most of the time it's
better to disallow it.  Disallowing it always is
too rigid - not the same as disallowing it by
default but allowing user code to do whatever it
wants.