* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only @ 2016-02-26 13:54 Kaushal Modi 2017-08-05 1:56 ` npostavs 0 siblings, 1 reply; 22+ messages in thread From: Kaushal Modi @ 2016-02-26 13:54 UTC (permalink / raw) To: 22819 [-- 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 --] ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2016-02-26 13:54 bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only Kaushal Modi @ 2017-08-05 1:56 ` npostavs 2017-08-05 6:52 ` Eli Zaretskii 0 siblings, 1 reply; 22+ messages in thread From: npostavs @ 2017-08-05 1:56 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819 [-- 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-05 1:56 ` npostavs @ 2017-08-05 6:52 ` Eli Zaretskii 2017-08-05 11:50 ` Kaushal Modi 0 siblings, 1 reply; 22+ messages in thread From: Eli Zaretskii @ 2017-08-05 6:52 UTC (permalink / raw) To: npostavs; +Cc: 22819, kaushal.modi > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-05 6:52 ` Eli Zaretskii @ 2017-08-05 11:50 ` Kaushal Modi 2017-08-05 12:10 ` Eli Zaretskii 2017-08-07 17:45 ` Kaushal Modi 0 siblings, 2 replies; 22+ messages in thread From: Kaushal Modi @ 2017-08-05 11:50 UTC (permalink / raw) To: Eli Zaretskii, Noam Postavsky; +Cc: 22819 [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-05 11:50 ` Kaushal Modi @ 2017-08-05 12:10 ` Eli Zaretskii 2017-08-05 12:29 ` Kaushal Modi 2017-08-05 12:47 ` npostavs 2017-08-07 17:45 ` Kaushal Modi 1 sibling, 2 replies; 22+ messages in thread From: Eli Zaretskii @ 2017-08-05 12:10 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819, npostavs > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-05 12:10 ` Eli Zaretskii @ 2017-08-05 12:29 ` Kaushal Modi 2017-08-05 12:37 ` Eli Zaretskii 2017-08-05 12:47 ` npostavs 1 sibling, 1 reply; 22+ messages in thread From: Kaushal Modi @ 2017-08-05 12:29 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 22819, Noam Postavsky [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-05 12:29 ` Kaushal Modi @ 2017-08-05 12:37 ` Eli Zaretskii 0 siblings, 0 replies; 22+ messages in thread From: Eli Zaretskii @ 2017-08-05 12:37 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819, npostavs > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-05 12:10 ` Eli Zaretskii 2017-08-05 12:29 ` Kaushal Modi @ 2017-08-05 12:47 ` npostavs 2017-08-05 13:13 ` Eli Zaretskii 1 sibling, 1 reply; 22+ messages in thread From: npostavs @ 2017-08-05 12:47 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 22819, Kaushal Modi 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 "\* ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-05 12:47 ` npostavs @ 2017-08-05 13:13 ` Eli Zaretskii 0 siblings, 0 replies; 22+ messages in thread From: Eli Zaretskii @ 2017-08-05 13:13 UTC (permalink / raw) To: npostavs; +Cc: 22819, kaushal.modi > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-05 11:50 ` Kaushal Modi 2017-08-05 12:10 ` Eli Zaretskii @ 2017-08-07 17:45 ` Kaushal Modi 2017-08-07 17:53 ` Noam Postavsky 1 sibling, 1 reply; 22+ messages in thread From: Kaushal Modi @ 2017-08-07 17:45 UTC (permalink / raw) To: Noam Postavsky; +Cc: 22819 [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-07 17:45 ` Kaushal Modi @ 2017-08-07 17:53 ` Noam Postavsky 2017-08-07 18:02 ` Kaushal Modi 0 siblings, 1 reply; 22+ messages in thread From: Noam Postavsky @ 2017-08-07 17:53 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-07 17:53 ` Noam Postavsky @ 2017-08-07 18:02 ` Kaushal Modi 2017-08-07 18:11 ` Noam Postavsky 0 siblings, 1 reply; 22+ messages in thread From: Kaushal Modi @ 2017-08-07 18:02 UTC (permalink / raw) To: Noam Postavsky; +Cc: 22819 [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-07 18:02 ` Kaushal Modi @ 2017-08-07 18:11 ` Noam Postavsky 2017-08-08 13:06 ` Kaushal Modi 0 siblings, 1 reply; 22+ messages in thread From: Noam Postavsky @ 2017-08-07 18:11 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819 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) ...)) ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-07 18:11 ` Noam Postavsky @ 2017-08-08 13:06 ` Kaushal Modi 2017-08-08 13:15 ` npostavs 2017-08-08 19:05 ` Eli Zaretskii 0 siblings, 2 replies; 22+ messages in thread From: Kaushal Modi @ 2017-08-08 13:06 UTC (permalink / raw) To: Noam Postavsky, Eli Zaretskii; +Cc: 22819 [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-08 13:06 ` Kaushal Modi @ 2017-08-08 13:15 ` npostavs 2017-08-08 19:05 ` Eli Zaretskii 1 sibling, 0 replies; 22+ messages in thread From: npostavs @ 2017-08-08 13:15 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-08 13:06 ` Kaushal Modi 2017-08-08 13:15 ` npostavs @ 2017-08-08 19:05 ` Eli Zaretskii 2017-08-08 19:19 ` Kaushal Modi 2019-06-25 14:33 ` Lars Ingebrigtsen 1 sibling, 2 replies; 22+ messages in thread From: Eli Zaretskii @ 2017-08-08 19:05 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819, npostavs > 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-08 19:05 ` Eli Zaretskii @ 2017-08-08 19:19 ` Kaushal Modi 2017-08-08 21:31 ` John Wiegley 2019-06-25 14:33 ` Lars Ingebrigtsen 1 sibling, 1 reply; 22+ messages in thread From: Kaushal Modi @ 2017-08-08 19:19 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 22819, npostavs [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-08 19:19 ` Kaushal Modi @ 2017-08-08 21:31 ` John Wiegley 2017-08-09 11:03 ` Kaushal Modi 0 siblings, 1 reply; 22+ messages in thread From: John Wiegley @ 2017-08-08 21:31 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819, npostavs >>>>> "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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-08 21:31 ` John Wiegley @ 2017-08-09 11:03 ` Kaushal Modi 2017-08-09 21:14 ` John Wiegley 0 siblings, 1 reply; 22+ messages in thread From: Kaushal Modi @ 2017-08-09 11:03 UTC (permalink / raw) To: John Wiegley; +Cc: 22819, npostavs [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-09 11:03 ` Kaushal Modi @ 2017-08-09 21:14 ` John Wiegley 0 siblings, 0 replies; 22+ messages in thread From: John Wiegley @ 2017-08-09 21:14 UTC (permalink / raw) To: Kaushal Modi; +Cc: 22819, npostavs [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2017-08-08 19:05 ` Eli Zaretskii 2017-08-08 19:19 ` Kaushal Modi @ 2019-06-25 14:33 ` Lars Ingebrigtsen 2019-06-25 14:35 ` Kaushal Modi 1 sibling, 1 reply; 22+ messages in thread From: Lars Ingebrigtsen @ 2019-06-25 14:33 UTC (permalink / raw) To: Eli Zaretskii; +Cc: 22819, Kaushal Modi, npostavs 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only 2019-06-25 14:33 ` Lars Ingebrigtsen @ 2019-06-25 14:35 ` Kaushal Modi 0 siblings, 0 replies; 22+ messages in thread From: Kaushal Modi @ 2019-06-25 14:35 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: 22819, Noam Postavsky [-- 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 --] ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2019-06-25 14:35 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-26 13:54 bug#22819: 25.0.91; Don't try to indent region if the buffer is read-only Kaushal Modi 2017-08-05 1:56 ` npostavs 2017-08-05 6:52 ` Eli Zaretskii 2017-08-05 11:50 ` Kaushal Modi 2017-08-05 12:10 ` Eli Zaretskii 2017-08-05 12:29 ` Kaushal Modi 2017-08-05 12:37 ` Eli Zaretskii 2017-08-05 12:47 ` npostavs 2017-08-05 13:13 ` Eli Zaretskii 2017-08-07 17:45 ` Kaushal Modi 2017-08-07 17:53 ` Noam Postavsky 2017-08-07 18:02 ` Kaushal Modi 2017-08-07 18:11 ` Noam Postavsky 2017-08-08 13:06 ` Kaushal Modi 2017-08-08 13:15 ` npostavs 2017-08-08 19:05 ` Eli Zaretskii 2017-08-08 19:19 ` Kaushal Modi 2017-08-08 21:31 ` John Wiegley 2017-08-09 11:03 ` Kaushal Modi 2017-08-09 21:14 ` John Wiegley 2019-06-25 14:33 ` Lars Ingebrigtsen 2019-06-25 14:35 ` Kaushal Modi
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.