bug#62444: [PATCH] erc: Fix "dcc get" flag parsing

[J.P.]

Hi Daniel,

Daniel Pettersson <daniel@dpettersson.net> writes:

> Proposed patch:
> erc: Fix "dcc get" flag parsing
>
> When nick or filename starts with `?-' or filename contains the
> following string " -", "dcc get" is unable determine nick/filename and
> fails to download file.
>
> Flag parsing rules is kept as is:
> [flag] nick [flag] filename [flag]

As you've mentioned elsewhere, we should probably try to reduce the
complexity WRT these flag positions. I agree that "terminal" is best,
but since we're already nominally in bed with "initial", how 'bout we
try sticking with that for now?

> Flags have the highest priority when parsing the arguments to dcc
> get. This is not an complete fix as dcc will fail on:
>      - nicks "-s" and "-t"

AFAIK, nicknames can't normally begin with a ?-, and attempting to
procure one like that will earn you a

  :mercury.libera.chat 432 testing123 -testing123 :Erroneous nickname

or similar. Of course, if you know of any popular servers where this
isn't the case, please share.

>      - filenames starting with r"-s|t +"

In the attached changes, which iterate on yours and which I'd like your
comments on, I've tried adding a familiar "end of options" separator,
i.e., "--", to cover this case. Given the unlikelihood of such
collisions, I think it's worth the occasional inconvenience.

>      - filenames with ending  with r" -s|t"

In the interest of preserving some symmetry with DCC SEND, which quotes
its outgoing arguments, I think erc-dcc should parse its own input line
rather than rely on the treatment from `erc-extract-command-from-line'.
This approach seems to work in cursory trials, but a few complications
arise when it comes to completion (also present in ERC <5.5.), although
there are workarounds. (How's your pcomplete-fu?)

> An more robust solution and cleaner implementation would be possible
> if flag position was limited to the end of the arguments list.
>
> This would also make it easier to implement pcomplete for flags as well.

Agreed. See above.

Alas, the following are just mechanical, style-related nits. Ignore them
if you wish, but please see the attached patches for a reprise of your
initial proposal with the changes I've outlined applied atop. (The first
two patches are just thrown in for convenience but ultimately
unrelated.)

> ---
>  lisp/erc/erc-dcc.el            | 36 +++++++++++++++++++++++-----------
>  test/lisp/erc/erc-dcc-tests.el | 27 ++++++++++++++-----------
>  2 files changed, 41 insertions(+), 22 deletions(-)
>
> diff --git a/lisp/erc/erc-dcc.el b/lisp/erc/erc-dcc.el
> index 4c557e0e0f9..d7c685e9413 100644
> --- a/lisp/erc/erc-dcc.el
> +++ b/lisp/erc/erc-dcc.el
> @@ -504,18 +504,32 @@ erc-dcc-do-CLOSE-command
>             ?n (erc-extract-nick (plist-get ret :nick))))))
>      t))
>
> -(defun erc-dcc-do-GET-command (proc nick &rest file)
> -  "Do a DCC GET command.  NICK is the person who is sending the file.
> -FILE is the filename.  If FILE is split into multiple arguments,
> -re-join the arguments, separated by a space.
> +(defun erc-dcc-do-GET-command (proc &rest args)
> +  "Do a DCC GET command.
> +ARGS are expected to contain:
> +  nick     The person who is sending the file.
> +  filename The filename to be downloaded. Can be split into multiple 
> arguments
                                            ^1                                 
2^

Two spaces between sentences. Not my preference either, but them's the
rules. Also, the doc-string fill column is a parsimonious 65. I often
ignore it for preformatted, "tabular" stuff like this and thus take it
up to 70 or so, but 78 is likely pushing it.

> +           which is then joined by a space.
> +  flags    \"-t\" sets `:turbo' see `erc-dcc-list'
> +           \"-s\" sets `:secure' see `erc-dcc-list'
> +ARGS are parsed as follows:
> +  [flag] nick [flag] filename [flag]
>  PROC is the server process."
> -  (let* ((args (seq-group-by (lambda (s) (eq ?- (aref s 0))) (cons nick 
> file)))
> -         (flags (prog1 (cdr (assq t args))
> -                  (setq args (cdr (assq nil args))
> -                        nick (pop args)
> -                        file (and args (mapconcat #'identity args " ")))))
> -         (elt (erc-dcc-member :nick nick :type 'GET :file file))
> -         (filename (or file (plist-get elt :file) "unknown")))
> +  (let ((possible-flags '("-s" "-t"))
> +        flags nick elt possible-files filename)
> +    ;; Get flags between get and nick
> +    (while (seq-contains-p possible-flags (car args) 'equal)
> +      (setq flags (cons (pop args) flags)))
> +    (setq nick (or (pop args) ""))
> +    ;; Get flags between nick and filename
> +    (while (seq-contains-p possible-flags (car args) 'equal)
> +      (setq flags (cons (pop args) flags)))
> +    ;; Get flags after filename
> +    (setq args (reverse args))
> +    (while (seq-contains-p possible-flags (car args) 'equal)
> +      (setq flags (cons (pop args) flags)))
> +    (setq filename (or (mapconcat #'identity (reverse args) " ") "")
> +          elt (erc-dcc-member :nick nick :type 'GET :file filename))

Some of the above, such as (setq foo (cons x foo)) instead of `push' and
`seq-contains-p' instead of `member', might distract a few readers. I
don't really care, personally.

>      (if elt
>          (let* ((file (read-file-name
>                        (format-prompt "Local filename"
> diff --git a/test/lisp/erc/erc-dcc-tests.el b/test/lisp/erc/erc-dcc-tests.el
> index bd8a9fc7951..f21463bb5a0 100644
> --- a/test/lisp/erc/erc-dcc-tests.el
> +++ b/test/lisp/erc/erc-dcc-tests.el
> @@ -100,7 +100,7 @@ erc-dcc-handle-ctcp-send--base
>  (ert-deftest erc-dcc-handle-ctcp-send--turbo ()
>    (erc-dcc-tests--dcc-handle-ctcp-send t))
>
[...]
> +
> +(ert-deftest erc-dcc-do-GET-command ()
> +  (erc-dcc-tests--erc-dcc-do-GET-command "foo.bin")
> +  (erc-dcc-tests--erc-dcc-do-GET-command "foo - file.bin")
> +  (erc-dcc-tests--erc-dcc-do-GET-command "foo -t file.bin"))

This is nice. I should have done this originally.

>  (defun erc-dcc-tests--pcomplete-common (test-fn)
>    (with-current-buffer (get-buffer-create "*erc-dcc-do-GET-command*")

As mentioned, I've taken a slightly different tack WRT parsing based on
the presence of pre-quoted args. Please check it out, give feedback, and
by all means iterate.

Thanks!

0000-v0-v1.diff
Description: Text Data

0001-Add-subcommand-dispatch-facility-to-erc-cmd-HELP.patch
Description: Text Data

0002-Add-subcommand-erc-cmd-HELP-handler-to-erc-dcc.patch
Description: Text Data

0003-Fix-DCC-GET-flag-parsing-in-erc-dcc.patch
Description: Text Data