[PATCH] fix goto-line

[PATCH] fix goto-line

[Jose E. Marchesi]

Hi hackers!

Just a tiny patch fixing a problem in goto-line, which was passing a
string as the second parameter to read-number, triggering an error.


2011-07-28  Jose E. Marchesi  <jemarch@gnu.org>

	* simple.el (goto-line): Use string-to-number to provide a
	numeric argument to read-number.

--- lisp/simple.el	2011-07-16 18:42:38 +0000
+++ lisp/simple.el	2011-07-28 12:47:34 +0000
@@ -900,10 +900,11 @@
 	      (save-excursion
 		(skip-chars-backward "0-9")
 		(if (looking-at "[0-9]")
-		    (buffer-substring-no-properties
+		    (string-to-number
+                     (buffer-substring-no-properties
 		     (point)
 		     (progn (skip-chars-forward "0-9")
-			    (point))))))
+			    (point)))))))
 	    ;; Decide if we're switching buffers.
 	    (buffer
 	     (if (consp current-prefix-arg)

-- 
Jose E. Marchesi    jemarch@gnu.org
GNU Project         http://www.gnu.org

Re: [PATCH] fix goto-line

[Juanma Barranquero]

On Thu, Jul 28, 2011 at 14:57, Jose E. Marchesi <jemarch@gnu.org> wrote:

> Just a tiny patch fixing a problem in goto-line, which was passing a
> string as the second parameter to read-number, triggering an error.

Committed.

Next time please file it as a bug report.

Thanks,

    Juanma

Re: [PATCH] fix goto-line

[Eduard Wiebe]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Thu, Jul 28, 2011 at 14:57, Jose E. Marchesi <jemarch@gnu.org> wrote:
>
>> Just a tiny patch fixing a problem in goto-line, which was passing a
>> string as the second parameter to read-number, triggering an error.
>
> Committed.
>
> Next time please file it as a bug report.

Please close bug#9163 ;-)

-- 
Eduard Wiebe

Re: [PATCH] fix goto-line

[Juri Linkov]

>> Just a tiny patch fixing a problem in goto-line, which was passing a
>> string as the second parameter to read-number, triggering an error.
>
> Committed.

I noticed the following areas for improvement:

1. `read-number' already inserts the default value into prompt.
   So the same code is not necessary in `goto-line'.

2. It would be useful to add the current line number to the defaults
   of `goto-line' to allow its easier modification by users.

This is implemented with:

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2011-07-28 14:05:07 +0000
+++ lisp/simple.el	2011-07-29 11:14:04 +0000
@@ -914,11 +914,8 @@ (defun goto-line (line &optional buffer)
 		 (concat " in " (buffer-name buffer))
 	       "")))
        ;; Read the argument, offering that number (if any) as default.
-       (list (read-number (format (if default "Goto line%s (%s): "
-                                    "Goto line%s: ")
-                                  buffer-prompt
-                                  default)
-                          default)
+       (list (read-number (format "Goto line%s: " buffer-prompt)
+                          (list default (line-number-at-pos)))
 	     buffer))))
   ;; Switch to the desired buffer, one way or another.
   (if buffer


3. `read-number' still doesn't support multiple default values
   like other minibuffer reading functions.  Let's do it.

=== modified file 'lisp/subr.el'
--- lisp/subr.el	2011-07-15 23:59:25 +0000
+++ lisp/subr.el	2011-07-29 11:12:45 +0000
@@ -2113,23 +2113,27 @@ (defun read-number (prompt &optional def
   "Read a numeric value in the minibuffer, prompting with PROMPT.
 DEFAULT specifies a default value to return if the user just types RET.
 The value of DEFAULT is inserted into PROMPT."
-  (let ((n nil))
-    (when default
+  (let ((n nil)
+	(default1 (if (consp default) (car default) default)))
+    (when default1
       (setq prompt
 	    (if (string-match "\\(\\):[ \t]*\\'" prompt)
-		(replace-match (format " (default %s)" default) t t prompt 1)
+		(replace-match (format " (default %s)" default1) t t prompt 1)
 	      (replace-regexp-in-string "[ \t]*\\'"
-					(format " (default %s) " default)
+					(format " (default %s) " default1)
 					prompt t t))))
     (while
 	(progn
-	  (let ((str (read-from-minibuffer prompt nil nil nil nil
-					   (and default
-						(number-to-string default)))))
+	  (let ((str (read-from-minibuffer
+		      prompt nil nil nil nil
+		      (when default
+			(if (consp default)
+			    (mapcar 'number-to-string (delq nil default))
+			  (number-to-string default))))))
 	    (condition-case nil
 		(setq n (cond
-			 ((zerop (length str)) default)
-			 ((stringp str) (read str))))
+			 ((zerop (length str)) default1)
+			 ((stringp str) (string-to-number str))))
 	      (error nil)))
 	  (unless (numberp n)
 	    (message "Please enter a number.")


4. In the patch above I replaced `read' with `string-to-number'
   for consistency.

Re: [PATCH] fix goto-line

[Juanma Barranquero]

On Fri, Jul 29, 2011 at 13:15, Juri Linkov <juri@jurta.org> wrote:

> I noticed the following areas for improvement:
>
> 1. `read-number' already inserts the default value into prompt.
>   So the same code is not necessary in `goto-line'.
>
> 2. It would be useful to add the current line number to the defaults
>   of `goto-line' to allow its easier modification by users.

There are improvements and not bug fixes, so please file this as a
wishlist item for 24.2 (and do not forget to add the "patch" tag).

    Juanma

Re: [PATCH] fix goto-line

[Juri Linkov]

> There are improvements and not bug fixes, so please file this as a
> wishlist item for 24.2 (and do not forget to add the "patch" tag).

These are improvements, but changes in the default value of `dired-do-touch'
is a regression.  I'd like to fix this regression with the following patch.
It restores the old pre-24 essential behavior, but still keeps the
requirement asked by the reporter of bug#6887 to use the current time.
More changes from http://thread.gmane.org/gmane.emacs.devel/81414/focus=82988
could be postponed to 24.2.

=== modified file 'lisp/dired-aux.el'
--- lisp/dired-aux.el	2011-07-25 08:23:29 +0000
+++ lisp/dired-aux.el	2011-07-29 15:27:45 +0000
@@ -227,20 +227,6 @@ (defun dired-files-attributes (dir)
    (directory-files dir)))
 \f
 
-(defun dired-touch-initial (files)
-  "Create initial input value for `touch' command."
-  ;; Nobody can explain what this version is supposed to do.  (Bug#6887)
-  ;; Also, the manual says it uses "the present time".
-  ;;; (let (initial)
-  ;;;   (while files
-  ;;;     (let ((current (nth 5 (file-attributes (car files)))))
-  ;;;       (if (and initial (not (equal initial current)))
-  ;;;           (setq initial (current-time) files nil)
-  ;;;         (setq initial current))
-  ;;;       (setq files (cdr files))))
-  ;;;   (format-time-string "%Y%m%d%H%M.%S" initial)))
-  (format-time-string "%Y%m%d%H%M.%S" (current-time)))
-
 (defun dired-do-chxxx (attribute-name program op-symbol arg)
   ;; Change file attributes (mode, group, owner, timestamp) of marked files and
   ;; refresh their file lines.
@@ -249,11 +235,18 @@ (defun dired-do-chxxx (attribute-name pr
   ;; OP-SYMBOL is the type of operation (for use in dired-mark-pop-up).
   ;; ARG describes which files to use, as in dired-get-marked-files.
   (let* ((files (dired-get-marked-files t arg))
+	 (initial
+	  (if (eq op-symbol 'touch)
+	      (format-time-string "%Y%m%d%H%M.%S" (current-time))))
+	 (default
+	   (if (eq op-symbol 'touch)
+	       (and (stringp (car files))
+		    (format-time-string "%Y%m%d%H%M.%S"
+					(nth 5 (file-attributes (car files)))))))
 	 (new-attribute
 	  (dired-mark-read-string
 	   (concat "Change " attribute-name " of %s to: ")
-	   (if (eq op-symbol 'touch) (dired-touch-initial files))
-	   op-symbol arg files))
+	   initial op-symbol arg files default))
 	 (operation (concat program " " new-attribute))
 	 failures)
     (setq failures

Re: [PATCH] fix goto-line

[Paul Eggert]

On 07/29/11 08:28, Juri Linkov wrote:

> I'd like to fix this regression with the following patch.

Thanks, could you please open up a bug report for this
on debbugs.gnu.org?  That'd be a better place to discuss
this anyway.

> +	  (if (eq op-symbol 'touch)
> +	      (format-time-string "%Y%m%d%H%M.%S" (current-time))))

I don't know what the overall problem is here, but I can
see two issues with this proposed patch.  First, presumably it
invokes (current-time) at one point, and then, later,
uses that saved current-time to do the 'touch'.  But that's
not quite right; it's the equivalent of the shell command

  touch --date="$(date)" file

whereas surely what is wanted is the equivalent of

  touch file

The latter command avoids some race conditions, because it
uses the time at the point of the 'touch', not at the point of
the invocation of 'date' (or of 'current-time').

The second issue is minor, and perhaps fixing the first issue
will make it irrelevant, but here it is anyway: the
"(current-time)" can be omitted in the above code, as it's the default
for that argument of format-time-string.

Re: dired-do-touch (was: [PATCH] fix goto-line)

[Juri Linkov]

> Thanks, could you please open up a bug report for this
> on debbugs.gnu.org?  That'd be a better place to discuss
> this anyway.

I've unarchived that bug report and referred to this thread from it.

>> +	  (if (eq op-symbol 'touch)
>> +	      (format-time-string "%Y%m%d%H%M.%S" (current-time))))
>
> I don't know what the overall problem is here, but I can
> see two issues with this proposed patch.  First, presumably it
> invokes (current-time) at one point, and then, later,
> uses that saved current-time to do the 'touch'.  But that's
> not quite right; it's the equivalent of the shell command
>
>   touch --date="$(date)" file

In theory you are right.  But in practice a 1-2 sec delay
is not a problem.

> whereas surely what is wanted is the equivalent of
>
>   touch file

When `touch file' is really wanted then it's easy to type `! touch'.
`dired-do-touch' was created for the convenience of changing
the file's timestamp providing useful default values for editing.

> The latter command avoids some race conditions, because it
> uses the time at the point of the 'touch', not at the point of
> the invocation of 'date' (or of 'current-time').

`T' is intended to allow editing the file attribute of the last
modification time, like `M' is used for editing the file's mode,
`O' - owner's UID, `G' - GID.  The docstring of `dired-do-touch' says:
"Change the timestamp of the marked (or next ARG) files."
And Dired is "Directory Editor" after all, so it should allow
editing file attributes including timestamps.

> The second issue is minor, and perhaps fixing the first issue
> will make it irrelevant, but here it is anyway: the
> "(current-time)" can be omitted in the above code, as it's the default
> for that argument of format-time-string.

Thanks, I removed "(current-time)" from the call to `format-time-string'.

BTW, I noticed that the docstring of `format-time-string' refers
to the argument `TIME' whereas the real argument name is `TIMEVAL'.
Adding something like "\n\(fn FORMAT-STRING &optional TIME UNIVERSAL)"
to the last line of the docstring would help.

Another documentation mismatch is that (info "(emacs) Operating on Files")
says:

  `M MODESPEC <RET>'
       Change the mode (also called "permission bits") of the specified
       files (`dired-do-chmod').  This uses the `chmod' program, so
       MODESPEC can be any argument that `chmod' can handle.

But actually `dired-do-chmod' doesn't use the `chmod' program anymore.

Re: dired-do-touch

[Juri Linkov]

> BTW, I noticed that the docstring of `format-time-string' refers
> to the argument `TIME' whereas the real argument name is `TIMEVAL'.
> Adding something like "\n\(fn FORMAT-STRING &optional TIME UNIVERSAL)"
> to the last line of the docstring would help.

The "\(fn" syntax is used only for the docstrings of Lisp functions.
So it should be
"usage: (format-time-string FORMAT-STRING &optional TIME UNIVERSAL)"

Re: dired-do-touch (was: [PATCH] fix goto-line)

[Andreas Schwab]

Juri Linkov <juri@jurta.org> writes:

>>> +	  (if (eq op-symbol 'touch)
>>> +	      (format-time-string "%Y%m%d%H%M.%S" (current-time))))
>>
>> I don't know what the overall problem is here, but I can
>> see two issues with this proposed patch.  First, presumably it
>> invokes (current-time) at one point, and then, later,
>> uses that saved current-time to do the 'touch'.  But that's
>> not quite right; it's the equivalent of the shell command
>>
>>   touch --date="$(date)" file
>
> In theory you are right.  But in practice a 1-2 sec delay
> is not a problem.

Note that changing the timestamp to a specific value requires owner
rights, whereas setting it to the current time only requires write
access.

>> whereas surely what is wanted is the equivalent of
>>
>>   touch file
>
> When `touch file' is really wanted then it's easy to type `! touch'.
> `dired-do-touch' was created for the convenience of changing
> the file's timestamp providing useful default values for editing.

An empty timestamp should mean running touch without time argument.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: dired-do-touch

[Juri Linkov]

> An empty timestamp should mean running touch without time argument.

Good idea.

But unfortunately `dired-do-chxxx' reads arguments using `read-string'
that can't return an empty string when there are values for M-n.