bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 1841 bytes --]

--text follows this line--

The current behavior of indent-region function is that it will first indent
the buffer and then throw an error at the end that it couldn't apply the
indentation. Instead the below patch checks if the buffer if read-only
first before trying to indent.


diff --git a/lisp/indent.el b/lisp/indent.el
index 0bbb520..d525511 100644
--- a/lisp/indent.el
+++ b/lisp/indent.el
@@ -509,6 +509,7 @@ indent-region
 If the third argument COLUMN is an integer, it specifies the
 column to indent to; if it is nil, use one of the three methods above."
   (interactive "r\nP")
+  (barf-if-buffer-read-only)
   (cond
    ;; If a numeric prefix is given, indent to that column.
    (column



In GNU Emacs 25.0.91.1 (x86_64-unknown-linux-gnu, GTK+ Version 2.24.23)
 of 2016-02-25 built on ...
Repository revision: d2dd614716e34edb5891e58c029741cd6b32217d
Windowing system distributor 'The X.Org Foundation', version 11.0.60900000
System Description: Red Hat Enterprise Linux Workstation release 6.6
(Santiago)

Configured using:
 'configure --prefix=/home/kmodi/usr_local/apps/6/emacs/emacs-25
 'CPPFLAGS=-fgnu89-inline -I/home/kmodi/usr_local/6/include
 -I/usr/include/freetype2 -I/usr/include' 'CFLAGS=-ggdb3 -O0'
 'CXXFLAGS=-ggdb3 -O0' 'LDFLAGS=-L/home/kmodi/usr_local/6/lib
 -L/home/kmodi/usr_local/6/lib64 -ggdb3'
 PKG_CONFIG_PATH=/home/kmodi/usr_local/6/lib/pkgconfig:/home/kmodi/usr_local/6/lib64/pkgconfig:/cad/adi/apps/gnu/linux/x86_64/6/lib/pkgconfig:/cad/adi/apps/gnu/linux/x86_64/6/lib64/pkgconfig:/usr/lib/pkgconfig:/usr/lib64/pkgconfig:/usr/share/pkgconfig:/lib/pkgconfig:/lib64/pkgconfig'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK2 X11

[-- Attachment #2: Type: text/html, Size: 2382 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 528 bytes --]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> The current behavior of indent-region function is that it will first indent
> the buffer and then throw an error at the end that it couldn't apply the
> indentation. Instead the below patch checks if the buffer if read-only
> first before trying to indent.

I wonder if someone will complain that they were relying on this
behaviour to check indentation in read-only buffers (currently if the
indentation is already correct there is no error).

The patch could be even simpler:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 852 bytes --]

From 54d1b5cd62572dc35eaed6f07ab9d254313c8a58 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Thu, 6 Jul 2017 20:04:43 -0400
Subject: [PATCH] * lisp/indent.el (indent-region): Fail fast if read-only
 (Bug#22819).

---
 lisp/indent.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/indent.el b/lisp/indent.el
index e7a30b885d..e9ed385faa 100644
--- a/lisp/indent.el
+++ b/lisp/indent.el
@@ -508,7 +508,7 @@ (defun indent-region (start end &optional column)
 Called from a program, START and END specify the region to indent.
 If the third argument COLUMN is an integer, it specifies the
 column to indent to; if it is nil, use one of the three methods above."
-  (interactive "r\nP")
+  (interactive "*r\nP")
   (cond
    ;; If a numeric prefix is given, indent to that column.
    (column
-- 
2.11.1

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Date: Fri, 04 Aug 2017 21:56:11 -0400
> Cc: 22819@debbugs.gnu.org
> 
> Kaushal Modi <kaushal.modi@gmail.com> writes:
> 
> > The current behavior of indent-region function is that it will first indent
> > the buffer and then throw an error at the end that it couldn't apply the
> > indentation. Instead the below patch checks if the buffer if read-only
> > first before trying to indent.
> 
> I wonder if someone will complain that they were relying on this
> behaviour to check indentation in read-only buffers (currently if the
> indentation is already correct there is no error).

The original submission provided no rationale for the change, so it's
hard to reason about its advantages.  The clear disadvantage is that
this goes against veteran Emacs behavior regarding read-only text,
behavior that is present in several other commands, and that AFAIR
resulted from some past discussions.

If the rationale is user surprise, then I'd suggest to leave the
current behavior unchanged.

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

On Sat, Aug 5, 2017, 2:53 AM Eli Zaretskii <eliz@gnu.org> wrote:

> > From: npostavs@users.sourceforge.net
> > Date: Fri, 04 Aug 2017 21:56:11 -0400
> > Cc: 22819@debbugs.gnu.org
> >
> > I wonder if someone will complain that they were relying on this
> > behaviour to check indentation in read-only buffers (currently if the
> > indentation is already correct there is no error).
>

Thanks Noam for reviewing this. I am away from PC for a few days. I'll
review your patch next week on Tuesday.

The act of indenting is an editing action. So the buffer should be checked
if it's editable before attempting an indent. If the buffer is read-only
and no indentation change is required, then good. But what if indentation
change is required? Here's what's will happen:

1. User: Try indentation
2. User: Could take several seconds or few minutes (depending on major mode
and file size)
3. Emacs: "Bummer, couldn't save all that indentation because the buffer is
read-only".
4. User: Make buffer editable. It's not a simple act of chmod. In my case,
the buffer was read-only because the file is part of a centralized version
control system (Cliosoft SOS). In "checked in" state, the file is just a
symlink to the cached version in server, and thus read-only. To make it
editable, I need to "check out" the file. That act replaces the symlink
link with a physical file copy.
5. User: Re-do that several seconds/minutes long indentation.

My commit saves the user from wasting that time in Step 2 above.

The original submission provided no rationale for the change, so it's
> hard to reason about its advantages.


Please consider the above use case.

The

clear disadvantage is that
> this goes
>

I am failing to see the disadvantage.

Before: Do indent > Attempt to write that indent to buffer

After (my patch): Check if buffer is writable > Do indent > Attempt to
write that indent.

Isn't it just logical that if you need to do an expensive indentation, the
buffer should be checked if it's editable to prevent spending that time
twice?

>against veteran Emacs behavior regarding read->only text,
>behavior that is present in several other >commands, and that AFAIR
>resulted from some past discussions.

This is the only one that provided me this surprise in about a decade of
Emacs use. Which other commands do the text manipulation, and then check
the buffer read-only status?

If

 the rationale is user surprise, then I'd suggest to leave the
> current behavior unchanged.
>

The flip question is: How common is a workflow, where a buffer is
read-only, user does indentation, and is fine with seeing that error after
the fact?

> --

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 4583 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Sat, 05 Aug 2017 11:50:59 +0000
> Cc: 22819@debbugs.gnu.org
> 
> 1. User: Try indentation
> 2. User: Could take several seconds or few minutes (depending on major mode and file size)
> 3. Emacs: "Bummer, couldn't save all that indentation because the buffer is read-only".
> 4. User: Make buffer editable. It's not a simple act of chmod. In my case, the buffer was read-only because the
> file is part of a centralized version control system (Cliosoft SOS). In "checked in" state, the file is just a
> symlink to the cached version in server, and thus read-only. To make it editable, I need to "check out" the file.
> That act replaces the symlink link with a physical file copy. 
> 5. User: Re-do that several seconds/minutes long indentation.
> 
> My commit saves the user from wasting that time in Step 2 above. 
> 
>  The original submission provided no rationale for the change, so it's
>  hard to reason about its advantages.
> 
> Please consider the above use case. 

I see no problem in it, sorry.  And why was the user not aware of the
read-only status of the buffer to begin with?  How plausible is such a
scenario?  Are we trying to change Emacs behavior to cater to a clear
cockpit error?

> >against veteran Emacs behavior regarding read->only text,
> >behavior that is present in several other >commands, and that AFAIR
> >resulted from some past discussions.
> 
> This is the only one that provided me this surprise in about a decade of Emacs use. Which other commands
> do the text manipulation, and then check the buffer read-only status? 

C-w, to name just one.

IOW, a command could have useful side effects that are produced even
if the buffer is read-only and its text cannot be changed, thus
preventing the main effect of the command from happening.

> The flip question is: How common is a workflow, where a buffer is read-only, user does indentation, and is fine
> with seeing that error after the fact?

This goes both ways: if it's uncommon enough to be unimportant, then
changing the behavior is not important as well.

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 1790 bytes --]

On Sat, Aug 5, 2017, 8:10 AM Eli Zaretskii <eliz@gnu.org> wrote:

>
> I see no problem in it, sorry.  And why was the user not aware of the
> read-only status of the buffer to begin with?


Cockpit error, as you say later. It's easy to forget if you are working on
a DVCS (like git) controlled file or CVCS (like SOS).

 How

plausible is such a
> scenario?


I resorted to writing this patch because it frustrated me quite a few
times.

 Are

we trying to change Emacs behavior to cater to a clear
> cockpit error?
>

Isn't it better to alert user of an operation that will not succeed anyways
beforehand, especially if the operation can be expensive like this one?

> >against veteran Emacs behavior regarding read->only text,
> > >behavior that is present in several other >commands, and that AFAIR
> > >resulted from some past discussions.
> >
> > This is the only one that provided me this surprise in about a decade of
> Emacs use. Which other commands
> > do the text manipulation, and then check the buffer read-only status?
>
> C-w, to name just one.
>

OK, it would be unnoticeable in that case as I have yet to see a kill
operation that can take couple of minutes.

IOW, a command could have useful side effects that are produced even
> if the buffer is read-only and its text cannot be changed, thus
> preventing the main effect of the command from happening.
>
> > The flip question is: How common is a workflow, where a buffer is
> read-only, user does indentation, and is fine
> > with seeing that error after the fact?
>
> This goes both ways: if it's uncommon enough to be unimportant, then
> changing the behavior is not important as well.
>

Would it be OK to open this up to emacs-devel to understand what workflows
can break because of this change?

> --

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 3494 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Sat, 05 Aug 2017 12:29:15 +0000
> Cc: 22819@debbugs.gnu.org, Noam Postavsky <npostavs@users.sourceforge.net>
> 
> Would it be OK to open this up to emacs-devel to understand what workflows can break because of this
> change?

You don't need my permission to start a discussion.  I'll probably
shut up about that anyway, as it's clear my opinions on this do not
have any weight at all.

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[npostavs]

Eli Zaretskii <eliz@gnu.org> writes:

>> >against veteran Emacs behavior regarding read-only text, behavior
>> >that is present in several other commands, and that AFAIR resulted
>> >from some past discussions.
>> 
>> This is the only one that provided me this surprise in about a decade of Emacs use. Which other commands
>> do the text manipulation, and then check the buffer read-only status? 
>
> C-w, to name just one.

That seems like a bit of a special case, as there is `kill-read-only-ok'
which specifically controls this behaviour.

> IOW, a command could have useful side effects that are produced even
> if the buffer is read-only and its text cannot be changed, thus
> preventing the main effect of the command from happening.

indent-region doesn't have any side-effects though, right?  There are
lots of other commands which check read-only status at the beginning:

    M-x rgrep barf-if-buffer-read-only\|(interactive "\*

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Eli Zaretskii]

> From: npostavs@users.sourceforge.net
> Cc: Kaushal Modi <kaushal.modi@gmail.com>,  22819@debbugs.gnu.org
> Date: Sat, 05 Aug 2017 08:47:37 -0400
> 
> indent-region doesn't have any side-effects though, right?

That depends on whether indent-region-function is nil, and what it
does if non-nil.

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]

On Sat, Aug 5, 2017 at 7:50 AM Kaushal Modi <kaushal.modi@gmail.com> wrote:

>
> Thanks Noam for reviewing this. I am away from PC for a few days. I'll
> review your patch next week on Tuesday.
>

Hi Noam,

I was able to try the patch sooner.

I use verilog-mode and tried out your patch on a huge SystemVerilog file,
which usually takes a tangible amount of time (10's of seconds) to indent
the whole file -- C-x h C-M-\

When using (barf-if-buffer-read-only):
- The failure due to a buffer being read-only is instantaneous.
- As soon as I do C-x h C-M-\, I get:

== *Messages* ==
Buffer is read-only: #<buffer file.sv>
=====

When using (interactive "*r\nP"):
- The failure happens soon, but not instantaneously.. I start seeing the
progress percentage of the indentation happening, and then the read-only
error shows.
- After I do C-x h C-M-\, I get the below after a second or two:

== *Messages* ==
indent-region
Indenting region...45%
verilog-do-indent: Buffer is read-only: #<buffer file.sv>
=====

So looks like the read-only check based on interactive form is kicked in
some time after the user calls indent-region.. I tried looking at
verilog-do-indent code in verilog-mode.el[1], but I couldn't understand why
interactive * based error is delayed.

[1]:
http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/verilog-mode.el
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 3377 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Noam Postavsky]

On Mon, Aug 7, 2017 at 1:45 PM, Kaushal Modi <kaushal.modi@gmail.com> wrote:

> - After I do C-x h C-M-\, I get the below after a second or two:
>
> == *Messages* ==
> indent-region
> Indenting region...45%
> verilog-do-indent: Buffer is read-only: #<buffer file.sv>
> =====

hmm, I can't reproduce here, but is it possible you have bound C-M-\
to some other command which calls indent-region non-interactively?

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

On Mon, Aug 7, 2017 at 1:53 PM Noam Postavsky <
npostavs@users.sourceforge.net> wrote:

>
> hmm, I can't reproduce here, but is it possible you have bound C-M-\
> to some other command which calls indent-region non-interactively?
>

Thanks for verifying. Of course it was my config..

This deviates from this bug report.. but I am open to suggestions on how I
could retain the * property of the interactive form while setting the
region boundaries as I do in the below advice.

I confirm that your suggestion also works fine if I remove the below advice
from my config.

=====
(defvar modi/region-or-whole-fns '(indent-region
                                   eval-region)
  "List of functions to act on the whole buffer if no region is selected.")

(defun modi/advice-region-or-whole (orig-fun &rest args)
  "Advice function that applies ORIG-FUN to the whole buffer if no region is
selected.
http://thread.gmane.org/gmane.emacs.help/109025/focus=109102 "
  ;; Required to override the "r" argument of `interactive' in functions
like
  ;; `indent-region' so that they can be called without an active region.
  (interactive (if (use-region-p)
                   (list (region-beginning) (region-end))
                 (list (point-min) (point-max))))
  ;; (message "Args: %S R: %S I: %S"
  ;;          args (use-region-p) (called-interactively-p 'interactive))
  (prog1 ; Return value of the advising fn needs to be the same as ORIG-FUN
      (apply orig-fun args)
    (when (and (called-interactively-p 'interactive)
               (not (use-region-p)))
      (message "Executed %s on the whole buffer."
               (propertize (symbol-name this-command)
                           'face 'font-lock-function-name-face)))))

(dolist (fn modi/region-or-whole-fns)
  (advice-add fn :around #'modi/advice-region-or-whole)
  ;; (advice-remove fn #'modi/advice-region-or-whole)
  )
=====
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 2915 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Noam Postavsky]

On Mon, Aug 7, 2017 at 2:02 PM, Kaushal Modi <kaushal.modi@gmail.com> wrote:
> On Mon, Aug 7, 2017 at 1:53 PM Noam Postavsky
> <npostavs@users.sourceforge.net> wrote:
>>
>> hmm, I can't reproduce here, but is it possible you have bound C-M-\
>> to some other command which calls indent-region non-interactively?
>
> This deviates from this bug report.. but I am open to suggestions on how I
> could retain the * property of the interactive form while setting the region
> boundaries as I do in the below advice.

Ah, well since you're replacing the interactive form, I suppose the
replacement should then make sure to check the read-only status as
well.

   (interactive (progn (barf-if-buffer-read-only) ...))

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Mon, Aug 7, 2017 at 2:11 PM Noam Postavsky <
npostavs@users.sourceforge.net> wrote:

> Ah, well since you're replacing the interactive form, I suppose the
> replacement should then make sure to check the read-only status as
> well.
>
>    (interactive (progn (barf-if-buffer-read-only) ...))
>

The advice gets tricky because I want to add barf-if-buffer-read-only only
if the original fn's interactive form had "*".

I am using the same advice fn for eval-region and indent-region.. so I
don't need the barf fn call for eval-region.

@Eli: Based on the discussion[1] on emacs-devel, there isn't any opposition
to doing what's proposed in this bug thread. So if it's alright by you, and
if there is no strong reason to use the more concise alternative i.e. if
both barf-if-buffer-read-only and interactive "*.." are equally correct,
can the former approach be committed?

Thanks.

[1]: http://lists.gnu.org/archive/html/emacs-devel/2017-08/msg00168.html
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 1596 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[npostavs]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> The advice gets tricky because I want to add barf-if-buffer-read-only
> only if the original fn's interactive form had "*".
>
> I am using the same advice fn for eval-region and indent-region.. so I
> don't need the barf fn call for eval-region.

Possibly you could do something with `interactive-form' and
`advice-eval-interactive-spec', but yes it's a bit tricky.

> @Eli: Based on the discussion[1] on emacs-devel, there isn't any
> opposition to doing what's proposed in this bug thread. So if it's
> alright by you, and if there is no strong reason to use the more
> concise alternative i.e. if both barf-if-buffer-read-only and
> interactive "*.." are equally correct, can the former approach be
> committed?

The choice is not between "*.." and `barf-if-buffer-read-only' as such,
"*.." is merely the string version of `barf-if-buffer-read-only'.  The
choice is between calling `barf-if-buffer-read-only' inside the
`interactive' form or inside the function itself.

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Eli Zaretskii]

> From: Kaushal Modi <kaushal.modi@gmail.com>
> Date: Tue, 08 Aug 2017 13:06:52 +0000
> Cc: 22819@debbugs.gnu.org
> 
> @Eli: Based on the discussion[1] on emacs-devel, there isn't any opposition to doing what's proposed in this
> bug thread. So if it's alright by you, and if there is no strong reason to use the more concise alternative i.e. if
> both barf-if-buffer-read-only and interactive "*.." are equally correct, can the former approach be committed? 

I'm not sure what is it that you are asking me.  You already know my
opinion on this proposal, and it hasn't changed.

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 1108 bytes --]

On Tue, Aug 8, 2017 at 3:06 PM Eli Zaretskii <eliz@gnu.org> wrote:

>
> I'm not sure what is it that you are asking me.  You already know my
> opinion on this proposal, and it hasn't changed.
>

I don't know what the outcome should be in this case:
- No one raised any issue moving forward with this in that emacs-devel.
- The concern you raised about indent-region having side-effects doesn't
seem practical. Indenting is a buffer-editing act for which the buffer
should not be read-only. If there are some side-effects other than that,
then that's a different problem. Also no one has presented a real scenario
where this proposal would cause an issue.
- Talking about pilot-error, this proposal simply alerts the user of the
pilot-error (doing indentation in read-only buffer) sooner rather than
later, thus saving the user's time.

So I don't know how differences are resolved in cases like this. The
proposal in this thread is to solve a real-life time-consuming annoyance,
where-as the against-point is about side-effects getting masked, and which
do not have any real-life examples.
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 1562 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[John Wiegley]

>>>>> "KM" == Kaushal Modi <kaushal.modi@gmail.com> writes:

KM> I don't know what the outcome should be in this case:
KM> - No one raised any issue moving forward with this in that emacs-devel.

Hello, Kaushal.

It should be pointed out here that maintenance of Emacs is at the maintainers'
discretion. Even though we do take the opinions of others into account, just
because emacs-devel "hasn't raised an issue", does not mean that a change will
happen. If Eli and I don't like it, the issue must wait for the next round of
maintainers.

There are a few factors why this change is being rejected now:

  a. It is a long-standing behavior, however less than ideal it is. We don't
     know what effect changing it will have, as obvious as it may seem. Our
     strongly-held policy is to avoid changes in long-standing behavior unless
     the reason to do so is compelling.

  b. The main force of your argument is that we waste CPU time when we don't
     need to, because we could just check before doing the indentation. I have
     no argument with that, and you're quite right. However, in all my years
     of using Emacs I've never run into this case, so I don't buy the argument
     that it is a change that needs to happen right now, for everyone.

  c. Emacs is designed to be extensible. Advise the indentation functions so
     they perform this check for you. It doesn't need to happen in core Emacs
     for you to get the behavior you want.

If your wish is to defend the interests of the "silent majority", who all,
without knowing it, would benefit from this change, then I appreciate your
concern. However, as maintainers, and given the lack of other voices *asking*
for this change, we prefer to retain the status quo, however far from perfect
it may be.

Plenty of projects on the Net strive to make every breaking change necessary
to approximate the best version of what they're trying to accomplish. That's
not how it is here. We want a stable, well-functioning Emacs with predictable
behavior, and sometimes that means keeping things as they have been for
decades -- even if, in hindsight, it shouldn't have been done that way.

What I'm interested to learn is how many other cases like this exist, and
whether a more general approach would make it less likely for it to occur.
What if we could know, for example, whether a function will try to change the
buffer, and simply stop the evaluation before it starts...

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 5139 bytes --]

On Tue, Aug 8, 2017, 5:31 PM John Wiegley <jwiegley@gmail.com> wrote:

>
> Hello, Kaushal.
>

Hi John, thanks for your input.

It should be pointed out here that maintenance of Emacs is at the
> maintainers'
> discretion. Even though we do take the opinions of others into account,
> just
> because emacs-devel "hasn't raised an issue", does not mean that a change
> will
> happen. If Eli and I don't like it, the issue must wait for the next round
> of
> maintainers.
>

Understood. My only hope that I can convince the maintainers with my reason
that this proposal fixes a real-life problem with the help democratic
voting system (if the opinions asked on emacs-devel matter and can be
called that).

There are a few factors why this change is being rejected now:
>
>   a. It is a long-standing behavior, however less than ideal it is. We
> don't
>      know what effect changing it will have, as obvious as it may seem. Our
>      strongly-held policy is to avoid changes in long-standing behavior
> unless
>      the reason to do so is compelling.
>

Wouldn't the master branch be a good playground for this?  If it affects
people negatively, it's just a one-line commit and thus easy to revert.

  b. The main force of your argument is that we waste CPU time when


I should have used the term "wall time". This issue has wasted quite a few
minutes for me.

 we don't
>      need to, because we could just check before doing the indentation. I
> have
>      no argument with that, and you're quite right.


 However

, in all my years
>      of using Emacs I've never run into this case, so I don't buy the
> argument
>      that it is a change that needs to happen right now, for everyone.
>

This change can be truly tested and appreciated only by people dealing with
read-only files everyday. This would be mostly people working with a
central version control system that makes all the files yet to be
checked-out as read-only symbolic links. I deal with dozens of read-only
files everyday, with a mix of editable files that are managed by git. So I
am likely to do the mistake of indenting a read-only file (i.e. indenting a
CVCS file before checking it out). Again, the benefit of this change is not
seen unless the indentation operation takes at least a few seconds (depends
on file size and major mode).

 c. Emacs is designed to be extensible. Advise the indentation functions so
>      they perform this check for you. It doesn't need to happen in core
> Emacs
>      for you to get the behavior you want.
>

Of course. I will do that. I was just hoping the "right fix" would get in.
(Otherwise why would anyone bother to submit bug reports and patches?)

If your wish is to defend the interests of the "silent majority", who all,
> without knowing it, would benefit from this change, then I appreciate your
> concern.


However, as maintainers, and given the lack of other voices *asking*
> for this change, we prefer to retain the status quo, however far from
> perfect
> it may be.
>

I doubt that will ever change because my situation as I explained above, of
working with primarily read-only files is not in majority.

Plenty of projects on the Net strive to make every breaking change necessary
> to approximate the best version of what they're trying to accomplish.


I haven't yet got an answer to a real-life scenario that would break by
this change. What kind of (i) side-effects would one be relying on (ii)
while running indent-region (iii) on read-only files?

It's a bit sad, I am presenting a solution to a real problem and the
counter argument is just one, and hypothetical.

 That's

not how it is here. We want a stable, well-functioning Emacs with
> predictable
> behavior,


*predictable*: What should one expect to happen if one tried to
indent-region in a read-only file? Would one be surprised if a read-only
error is thrown? Emacs already does that.. just that this proposal does
just that *before* the time-consuming indentation attempt is started.

So this patch should bring no noticeable change to a majority of people.
But people in minority like me -- (i) like to obsessively keep all files
well-indented (ii) working with read-only files (iii) time-consuming
indentation (major mode and file size dependent) -- would heavily
appreciate this.

and sometimes that means keeping things as they have been for
> decades -- even if, in hindsight, it shouldn't have been done that way.


What I'm interested to learn is how many other cases like this exist,


I doubt if many still exist because the ones that affected most people are
already fixed by using the same approach (as in this proposal) of using *
interactive form or barf-if-buffer-read-only.

and
> whether a more general approach would make it less likely for it to occur.
> What if we could know, for example, whether a function will try to change
> the
> buffer, and simply stop the evaluation before it starts...
>

This prolonged discussion makes me think "What's the point?". I can barely
convince to commit this 1-line change to the master branch that can be
easily reverted even if a single person complained.

> --

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 8875 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[John Wiegley]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

>>>>> Kaushal Modi <kaushal.modi@gmail.com> writes:

> Understood. My only hope that I can convince the maintainers with my reason
> that this proposal fixes a real-life problem with the help democratic voting
> system (if the opinions asked on emacs-devel matter and can be called that).

That's fair, though we still need to be personally convinced. :) Ancient
behavior has a mystique that requires a fair bit of motivation for us to
change. It really just comes down to the fact that Eli and I are conservative
fellows in these things.

> Wouldn't the master branch be a good playground for this? If it affects
> people negatively, it's just a one-line commit and thus easy to revert.

I'm not sure enough people use the master branch for it to determine how it
will affect the majority.

In all likelihood it won't affect anyone badly at all (I don't really see how
it could), it's just not something we need to do today. I'm fine with keeping
the issue open, though. My preference would be that another solution will
solve this, and all similar issues, by way of a better design. Otherwise,
we're picking at gnats in a sensitive area (i.e., long-held behavior).

> I should have used the term "wall time". This issue has wasted quite a few
> minutes for me.

Understood.

> Of course. I will do that. I was just hoping the "right fix" would get in.
> (Otherwise why would anyone bother to submit bug reports and patches?)

You're making us aware of bad implementation decisions made long ago, which
are good to know about. There have been other, fairly long, debates on the
meaning of the "read-only" bit, and when a buffer should get marked as
modified. I'm sure these issues relate to yours as well. For example: should a
buffer-modifying operation be aborted early even if the actual operation of
the function won't change anything? People argue that M-q shouldn't mark a
buffer as modified if the reformatting changes nothing: does that mean M-q
should be allowed on a read-only buffer if it doesn't modify, or should it
abort early because its makes no sense?

And how many people have built up macros or customizations or custom functions
in their configuration that rely on ultimately-non-modifying operations being
allowed in read-only buffers, rather than turning them into errors? Arguably
their code is "wrong", but how much of their time should we waste by fixing
this and causing their keyboard macros to break?

I realize my argument tends toward "change nothing", but that's not what I
mean. I'm only saying that there's a lot of users to be considered, so unless
we *need* to risk breakage, we prefer to avoid it. The current behavior has
been around for a long, long time, and while it's painful for your use case, I
know you have the expertise to advise Emacs as needed.

> I haven't yet got an answer to a real-life scenario that would break by this
> change. What kind of (i) side-effects would one be relying on (ii) while
> running indent-region (iii) on read-only files?
> 
> It's a bit sad, I am presenting a solution to a real problem and the counter
> argument is just one, and hypothetical.

We don't normally have any counter-evidence that an old, bad behavior *should*
be kept. It's more an argument that we don't change them until we're convinced
it's time.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 658 bytes --]

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kaushal Modi <kaushal.modi@gmail.com>
>> Date: Tue, 08 Aug 2017 13:06:52 +0000
>> Cc: 22819@debbugs.gnu.org
>> 
>> @Eli: Based on the discussion[1] on emacs-devel, there isn't any
>> opposition to doing what's proposed in this
>> bug thread. So if it's alright by you, and if there is no strong
>> reason to use the more concise alternative i.e. if
>> both barf-if-buffer-read-only and interactive "*.." are equally
>> correct, can the former approach be committed?
>
> I'm not sure what is it that you are asking me.  You already know my
> opinion on this proposal, and it hasn't changed.

I think the rough consensus was that adding this special-casing to this
command wasn't idea, so I'm closing this bug report.

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

bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only

[Kaushal Modi]

[-- Attachment #1: Type: text/plain, Size: 226 bytes --]

On Tue, Jun 25, 2019 at 10:33 AM Lars Ingebrigtsen <larsi@gnus.org> wrote:

> I think the rough consensus was that adding this special-casing to this
> command wasn't idea, so I'm closing this bug report.
>

OK.. that's fine.

[-- Attachment #2: Type: text/html, Size: 541 bytes --]