~pkal/public-inbox

3 2

[PATCH] shell-command+: Fix false substitution of "\ "

Visuwesh <visuweshm@gmail.com>
Details
Message ID
<87ikx4mkr7.fsf@gmail.com>
DKIM signature
pass
Download raw message
4 months ago 
Good day, Philip!

The regexp used in shell-command+-expand-% seems to falsely replace all
"\ " with the filename.  To reproduce,

    1. M-! touch "/tmp/file with space" RET
    2. M-! cat /tmp/file\ with\ space RET

in the second trial, shell-command+ will falsely replace all "\ " with
the current buffer's filename:

    % touch /tmp/file/home/viz/lib/repos/shell-command-plus/shell-command+.el with/home/viz/lib/repos/shell-command-plus/shell-command+.el space
    touch: cannot touch '/tmp/file/home/viz/lib/repos/shell-command-plus/shell-command+.el': No such file or directory
    touch: cannot touch 'with/home/viz/lib/repos/shell-command-plus/shell-command+.el': No such file or directory

I debugged this a long while back but if the commit message is right,
the problem is with the first (* ?\\ ?\\) in the regexp.  I replaced it
to match all % not preceded by \.
Philip Kaludercic <philipk@posteo.net>
Details
Message ID
<87bk2wi26i.fsf@posteo.net>
In-Reply-To
<87ikx4mkr7.fsf@gmail.com> (view parent)
DKIM signature
pass
Download raw message
4 months ago 
Visuwesh <visuweshm@gmail.com> writes:

> Good day, Philip!

Hi,

> The regexp used in shell-command+-expand-% seems to falsely replace all
> "\ " with the filename.  To reproduce,
>
>     1. M-! touch "/tmp/file with space" RET
>     2. M-! cat /tmp/file\ with\ space RET
>
> in the second trial, shell-command+ will falsely replace all "\ " with
> the current buffer's filename:
>
>     % touch /tmp/file/home/viz/lib/repos/shell-command-plus/shell-command+.el with/home/viz/lib/repos/shell-command-plus/shell-command+.el space
>     touch: cannot touch '/tmp/file/home/viz/lib/repos/shell-command-plus/shell-command+.el': No such file or directory
>     touch: cannot touch 'with/home/viz/lib/repos/shell-command-plus/shell-command+.el': No such file or directory
>
> I debugged this a long while back but if the commit message is right,
> the problem is with the first (* ?\\ ?\\) in the regexp.  I replaced it
> to match all % not preceded by \.

The issue is that it breaks if the \ was escaped:

(let ((buffer-file-name "foo.el"))
  (shell-command+-expand-% (shell-command+-parse "x%") :form :context))
;;=> ((nil nil "\\\\%" "\\\\%") :form :context)

while it should be

;;=> ((nil nil "\\\\%" "\\\\foo.el") :form :context)

but now I realise that it actually is

;;=> ((nil nil "\\\\%" "foo.el") :form :context)

which I appear to fix by just replacing the subexpression 1 as you
suggested in your patch:
Visuwesh <visuweshm@gmail.com>
Details
Message ID
<87plrcb0l9.fsf@gmail.com>
In-Reply-To
<87bk2wi26i.fsf@posteo.net> (view parent)
DKIM signature
pass
Download raw message
4 months ago 
[புதன் ஜூலை 17, 2024] Philip Kaludercic wrote:

> Visuwesh <visuweshm@gmail.com> writes:
>
>> Good day, Philip!
>
> Hi,
>
>> The regexp used in shell-command+-expand-% seems to falsely replace all
>> "\ " with the filename.  To reproduce,
>>
>>     1. M-! touch "/tmp/file with space" RET
>>     2. M-! cat /tmp/file\ with\ space RET
>>
>> in the second trial, shell-command+ will falsely replace all "\ " with
>> the current buffer's filename:
>>
>>     % touch /tmp/file/home/viz/lib/repos/shell-command-plus/shell-command+.el with/home/viz/lib/repos/shell-command-plus/shell-command+.el space
>>     touch: cannot touch '/tmp/file/home/viz/lib/repos/shell-command-plus/shell-command+.el': No such file or directory
>>     touch: cannot touch 'with/home/viz/lib/repos/shell-command-plus/shell-command+.el': No such file or directory
>>
>> I debugged this a long while back but if the commit message is right,
>> the problem is with the first (* ?\\ ?\\) in the regexp.  I replaced it
>> to match all % not preceded by \.
>
> The issue is that it breaks if the \ was escaped:
>
> (let ((buffer-file-name "foo.el"))
>   (shell-command+-expand-% (shell-command+-parse "x%") :form :context))
> ;;=> ((nil nil "\\\\%" "\\\\%") :form :context)
>
> while it should be
>
> ;;=> ((nil nil "\\\\%" "\\\\foo.el") :form :context)
>
> but now I realise that it actually is
>
> ;;=> ((nil nil "\\\\%" "foo.el") :form :context)
>
> which I appear to fix by just replacing the subexpression 1 as you
> suggested in your patch:
>
> diff --git a/shell-command+.el b/shell-command+.el
> index 4b94acb..9a4d173 100644
> --- a/shell-command+.el
> +++ b/shell-command+.el
> @@ -174,7 +174,7 @@ For PARSE, FORM and CONTEXT see `shell-command+-features'."
>             (rx (* ?\\ ?\\) (or ?\\ (group "%")))
>             (or (file-remote-p buffer-file-name 'localname)
>                 buffer-file-name)
> -           (nth 3 parse))))
> +           (nth 3 parse) nil nil 1)))
>    (list parse form context))
>  
>  (put #'shell-command+-expand-%
>
>
> WDYT?

Unless there's a mistake in the patch you sent, with

    M-! touch /tmp/file\ with\ space

I get the backtrace

    Debugger entered--Lisp error: (error "replace-match subexpression does not exist" 1)
      replace-match("/home/viz/lib/repos/shell-command-plus/shell-command+.el" nil nil "\\" 1)
      replace-regexp-in-string("\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" "/home/viz/lib/repos/shell-command-plus/shell-command+.el" "touch /tmp/file\\ with\\ space" nil nil 1)
      (setcar c (replace-regexp-in-string "\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" (or (file-remote-p buffer-file-name 'localname) buffer-file-name) (nth 3 parse) nil nil 1))
      (let* ((c (nthcdr 3 parse))) (setcar c (replace-regexp-in-string "\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" (or (file-remote-p buffer-file-name 'localname) buffer-file-name) (nth 3 parse) nil nil 1)))
      (progn (let* ((c (nthcdr 3 parse))) (setcar c (replace-regexp-in-string "\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" (or (file-remote-p buffer-file-name 'localname) buffer-file-name) (nth 3 parse) nil nil 1))))
      (if buffer-file-name (progn (let* ((c (nthcdr 3 parse))) (setcar c (replace-regexp-in-string "\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" (or (file-remote-p buffer-file-name 'localname) buffer-file-name) (nth 3 parse) nil nil 1)))))
      shell-command+-expand-%((nil nil "touch" "touch /tmp/file\\ with\\ space") #f(compiled-function (input beg end) #<bytecode 0x1ca0aa155386d296>) funcall)
      shell-command+("touch /tmp/file\\ with\\ space" 6445 6445)
      vz/shell-command+("touch /tmp/file\\ with\\ space" 6445 6445)
      funcall-interactively(vz/shell-command+ "touch /tmp/file\\ with\\ space" 6445 6445)
      call-interactively(vz/shell-command+ nil nil)
      command-execute(vz/shell-command+)

Am I doing something wrong here?
Philip Kaludercic <philipk@posteo.net>
Details
Message ID
<87plrbhi4w.fsf@posteo.net>
In-Reply-To
<87plrcb0l9.fsf@gmail.com> (view parent)
DKIM signature
pass
Download raw message
4 months ago 
Visuwesh <visuweshm@gmail.com> writes:

> [புதன் ஜூலை 17, 2024] Philip Kaludercic wrote:
>
>> Visuwesh <visuweshm@gmail.com> writes:
>>
>>> Good day, Philip!
>>
>> Hi,
>>
>>> The regexp used in shell-command+-expand-% seems to falsely replace all
>>> "\ " with the filename.  To reproduce,
>>>
>>>     1. M-! touch "/tmp/file with space" RET
>>>     2. M-! cat /tmp/file\ with\ space RET
>>>
>>> in the second trial, shell-command+ will falsely replace all "\ " with
>>> the current buffer's filename:
>>>
>>>     % touch /tmp/file/home/viz/lib/repos/shell-command-plus/shell-command+.el with/home/viz/lib/repos/shell-command-plus/shell-command+.el space
>>>     touch: cannot touch '/tmp/file/home/viz/lib/repos/shell-command-plus/shell-command+.el': No such file or directory
>>>     touch: cannot touch 'with/home/viz/lib/repos/shell-command-plus/shell-command+.el': No such file or directory
>>>
>>> I debugged this a long while back but if the commit message is right,
>>> the problem is with the first (* ?\\ ?\\) in the regexp.  I replaced it
>>> to match all % not preceded by \.
>>
>> The issue is that it breaks if the \ was escaped:
>>
>> (let ((buffer-file-name "foo.el"))
>>   (shell-command+-expand-% (shell-command+-parse "x%") :form :context))
>> ;;=> ((nil nil "\\\\%" "\\\\%") :form :context)
>>
>> while it should be
>>
>> ;;=> ((nil nil "\\\\%" "\\\\foo.el") :form :context)
>>
>> but now I realise that it actually is
>>
>> ;;=> ((nil nil "\\\\%" "foo.el") :form :context)
>>
>> which I appear to fix by just replacing the subexpression 1 as you
>> suggested in your patch:
>>
>> diff --git a/shell-command+.el b/shell-command+.el
>> index 4b94acb..9a4d173 100644
>> --- a/shell-command+.el
>> +++ b/shell-command+.el
>> @@ -174,7 +174,7 @@ For PARSE, FORM and CONTEXT see `shell-command+-features'."
>>             (rx (* ?\\ ?\\) (or ?\\ (group "%")))
>>             (or (file-remote-p buffer-file-name 'localname)
>>                 buffer-file-name)
>> -           (nth 3 parse))))
>> +           (nth 3 parse) nil nil 1)))
>>    (list parse form context))
>>  
>>  (put #'shell-command+-expand-%
>>
>>
>> WDYT?
>
> Unless there's a mistake in the patch you sent, with
>
>     M-! touch /tmp/file\ with\ space
>
> I get the backtrace
>
>     Debugger entered--Lisp error: (error "replace-match subexpression does not exist" 1)
>       replace-match("/home/viz/lib/repos/shell-command-plus/shell-command+.el" nil nil "\\" 1)
>       replace-regexp-in-string("\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" "/home/viz/lib/repos/shell-command-plus/shell-command+.el" "touch /tmp/file\\ with\\ space" nil nil 1)
>       (setcar c (replace-regexp-in-string "\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" (or (file-remote-p buffer-file-name 'localname) buffer-file-name) (nth 3 parse) nil nil 1))
>       (let* ((c (nthcdr 3 parse))) (setcar c (replace-regexp-in-string "\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" (or (file-remote-p buffer-file-name 'localname) buffer-file-name) (nth 3 parse) nil nil 1)))
>       (progn (let* ((c (nthcdr 3 parse))) (setcar c (replace-regexp-in-string "\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" (or (file-remote-p buffer-file-name 'localname) buffer-file-name) (nth 3 parse) nil nil 1))))
>       (if buffer-file-name (progn (let* ((c (nthcdr 3 parse))) (setcar c (replace-regexp-in-string "\\(?:\\\\\\\\\\)*\\(?:\\\\\\|\\(%\\)\\)" (or (file-remote-p buffer-file-name 'localname) buffer-file-name) (nth 3 parse) nil nil 1)))))
>       shell-command+-expand-%((nil nil "touch" "touch /tmp/file\\ with\\ space") #f(compiled-function (input beg end) #<bytecode 0x1ca0aa155386d296>) funcall)
>       shell-command+("touch /tmp/file\\ with\\ space" 6445 6445)
>       vz/shell-command+("touch /tmp/file\\ with\\ space" 6445 6445)
>       funcall-interactively(vz/shell-command+ "touch /tmp/file\\ with\\ space" 6445 6445)
>       call-interactively(vz/shell-command+ nil nil)
>       command-execute(vz/shell-command+)
>
> Am I doing something wrong here?

No, that was my mistake, I didn't test it properly.  I'll have to write
a few test cases to determine what we want and what we don't.

-- 
	Philip Kaludercic on peregrine
Reply to thread Export thread (mbox)