Hi Daniel, Daniel Pettersson 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!