bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg

bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg

[Kyle Hubert]

Forwarding arguments through to children function invocations should
be handled by current-prefix-arg. This was first uncovered when ediff
used full window width scrolling in ediff-scroll-horizontally, when
the call to scroll-left or scroll-right should have been half-window
size. Through further grepping, there was another usage in
simple.el. Note, execute-extended-command was visually inspected that
it was in interactive mode.

---
 lisp/simple.el        | 2 +-
 lisp/vc/ediff-util.el | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 2ec3da680f..53afa7b138 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -1888,7 +1888,7 @@ invoking, give a prefix argument to `execute-extended-command'."
     ;; `function' and not `execute-extended-command'.  The difference is
     ;; visible in cases such as M-x <cmd> RET and then C-x z (bug#11506).
     (setq real-this-command function)
-    (let ((prefix-arg prefixarg))
+    (let ((current-prefix-arg prefixarg))
       (command-execute function 'record))
     ;; If enabled, show which key runs this command.
     ;; But first wait, and skip the message if there is input.
diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index a8af9ba37a..5f8a4a86b1 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -1540,10 +1540,10 @@ the width of the A/B/C windows."
    ;; hscrolling.
    (if (= last-command-event ?<)
        (lambda (arg)
-	 (let ((prefix-arg arg))
+	 (let ((current-prefix-arg arg))
 	   (call-interactively #'scroll-left)))
      (lambda (arg)
-       (let ((prefix-arg arg))
+       (let ((current-prefix-arg arg))
 	 (call-interactively #'scroll-right))))
    ;; calculate argument to scroll-left/right
    ;; if there is an explicit argument
-- 
2.25.0

bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg

[Kyle Hubert]

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

I have a hard time testing the change to simple.el, as I don't understand
execute-extended-command. Can anyone help here? I'm worried since it isn't
using (interactive "P") that this is incorrect. I admit I'm deeper in the
guts of emacs than typical.

Thanks,
-Kyle


On Wed, Jan 29, 2020 at 10:35 PM Kyle Hubert <khubert@gmail.com> wrote:

> Forwarding arguments through to children function invocations should
> be handled by current-prefix-arg. This was first uncovered when ediff
> used full window width scrolling in ediff-scroll-horizontally, when
> the call to scroll-left or scroll-right should have been half-window
> size. Through further grepping, there was another usage in
> simple.el. Note, execute-extended-command was visually inspected that
> it was in interactive mode.
>
> ---
>  lisp/simple.el        | 2 +-
>  lisp/vc/ediff-util.el | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/simple.el b/lisp/simple.el
> index 2ec3da680f..53afa7b138 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -1888,7 +1888,7 @@ invoking, give a prefix argument to
> `execute-extended-command'."
>      ;; `function' and not `execute-extended-command'.  The difference is
>      ;; visible in cases such as M-x <cmd> RET and then C-x z (bug#11506).
>      (setq real-this-command function)
> -    (let ((prefix-arg prefixarg))
> +    (let ((current-prefix-arg prefixarg))
>        (command-execute function 'record))
>      ;; If enabled, show which key runs this command.
>      ;; But first wait, and skip the message if there is input.
> diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
> index a8af9ba37a..5f8a4a86b1 100644
> --- a/lisp/vc/ediff-util.el
> +++ b/lisp/vc/ediff-util.el
> @@ -1540,10 +1540,10 @@ the width of the A/B/C windows."
>     ;; hscrolling.
>     (if (= last-command-event ?<)
>         (lambda (arg)
> -        (let ((prefix-arg arg))
> +        (let ((current-prefix-arg arg))
>            (call-interactively #'scroll-left)))
>       (lambda (arg)
> -       (let ((prefix-arg arg))
> +       (let ((current-prefix-arg arg))
>          (call-interactively #'scroll-right))))
>     ;; calculate argument to scroll-left/right
>     ;; if there is an explicit argument
> --
> 2.25.0
>
>

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

bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg

[Noam Postavsky]

Kyle Hubert <khubert@gmail.com> writes:

> I have a hard time testing the change to simple.el, as I don't understand
> execute-extended-command. Can anyone help here? I'm worried since it isn't
> using (interactive "P") that this is incorrect. I admit I'm deeper in the
> guts of emacs than typical.

The simple.el let-binding is around command-execute, not
execute-extended-command.  command-execute does specifically read
prefix-arg, so I think that part of your patch should be dropped (I
haven't looked in detail at the ediff part, but it sounds right).

>> --- a/lisp/simple.el
>> +++ b/lisp/simple.el
>> @@ -1888,7 +1888,7 @@ invoking, give a prefix argument to
>> `execute-extended-command'."
>>      ;; `function' and not `execute-extended-command'.  The difference is
>>      ;; visible in cases such as M-x <cmd> RET and then C-x z (bug#11506).
>>      (setq real-this-command function)
>> -    (let ((prefix-arg prefixarg))
>> +    (let ((current-prefix-arg prefixarg))
>>        (command-execute function 'record))

bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg

[Kyle Hubert]

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

Thank you for the review. I had a feeling that
execute-extended-command's calling of prefix-arg was the correct
usage for command-execute. If I drop that from the PATCH, do I
repost to this bug ID email? I'm unfamiliar with the project.

-Kyle

On Thu, Jan 30, 2020 at 8:12 AM Noam Postavsky <npostavs@gmail.com> wrote:

> Kyle Hubert <khubert@gmail.com> writes:
>
> > I have a hard time testing the change to simple.el, as I don't understand
> > execute-extended-command. Can anyone help here? I'm worried since it
> isn't
> > using (interactive "P") that this is incorrect. I admit I'm deeper in the
> > guts of emacs than typical.
>
> The simple.el let-binding is around command-execute, not
> execute-extended-command.  command-execute does specifically read
> prefix-arg, so I think that part of your patch should be dropped (I
> haven't looked in detail at the ediff part, but it sounds right).
>
> >> --- a/lisp/simple.el
> >> +++ b/lisp/simple.el
> >> @@ -1888,7 +1888,7 @@ invoking, give a prefix argument to
> >> `execute-extended-command'."
> >>      ;; `function' and not `execute-extended-command'.  The difference
> is
> >>      ;; visible in cases such as M-x <cmd> RET and then C-x z
> (bug#11506).
> >>      (setq real-this-command function)
> >> -    (let ((prefix-arg prefixarg))
> >> +    (let ((current-prefix-arg prefixarg))
> >>        (command-execute function 'record))
>

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

bug#39353: [PATCH] Use current-prefix-arg instead of prefix-arg

[Eli Zaretskii]

> From: Kyle Hubert <khubert@gmail.com>
> Date: Thu, 30 Jan 2020 11:35:01 -0500
> Cc: 39353@debbugs.gnu.org
> 
> If I drop that from the PATCH, do I repost to this bug ID email?

Yes, please.

bug#39353: [PATCH] Ediff fix for ediff-scroll-horizontally

[Kyle Hubert]

When ediff-scroll-horizontally interactively calls scroll-left and
scroll-right, current-prefix-arg should be used. This allows for the
case that when no args or '-' are passed to ediff-scroll-horizontally,
it will determine the default amount (half the window width or
half-again, respectively).
---
 lisp/vc/ediff-util.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/vc/ediff-util.el b/lisp/vc/ediff-util.el
index a8af9ba37a..5f8a4a86b1 100644
--- a/lisp/vc/ediff-util.el
+++ b/lisp/vc/ediff-util.el
@@ -1540,10 +1540,10 @@ the width of the A/B/C windows."
    ;; hscrolling.
    (if (= last-command-event ?<)
        (lambda (arg)
-	 (let ((prefix-arg arg))
+	 (let ((current-prefix-arg arg))
 	   (call-interactively #'scroll-left)))
      (lambda (arg)
-       (let ((prefix-arg arg))
+       (let ((current-prefix-arg arg))
 	 (call-interactively #'scroll-right))))
    ;; calculate argument to scroll-left/right
    ;; if there is an explicit argument
-- 
2.25.0

bug#39353: [PATCH] Ediff fix for ediff-scroll-horizontally

[Eli Zaretskii]

> From: Kyle Hubert <khubert@gmail.com>
> Date: Fri, 31 Jan 2020 09:29:43 -0500
> Cc: Kyle Hubert <khubert@gmail.com>
> 
> When ediff-scroll-horizontally interactively calls scroll-left and
> scroll-right, current-prefix-arg should be used. This allows for the
> case that when no args or '-' are passed to ediff-scroll-horizontally,
> it will determine the default amount (half the window width or
> half-again, respectively).
> ---
>  lisp/vc/ediff-util.el | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Thanks, pushed to the master branch.