Package: emacs;
Reported by: Daniel Pettersson <daniel <at> dpettersson.net>
Date: Sat, 25 Mar 2023 15:27:02 UTC
Severity: normal
Tags: patch
Done: "J.P." <jp <at> neverwas.me>
Bug is archived. No further changes may be made.
View this message in rfc822 format
From: help-debbugs <at> gnu.org (GNU bug Tracking System) To: "J.P." <jp <at> neverwas.me> Cc: tracker <at> debbugs.gnu.org Subject: bug#62444: closed ([PATCH] erc: Fix "dcc get" flag parsing) Date: Sat, 08 Apr 2023 22:54:02 +0000
[Message part 1 (text/plain, inline)]
Your message dated Sat, 08 Apr 2023 15:53:23 -0700 with message-id <87mt3i56l8.fsf <at> neverwas.me> and subject line Re: bug#62444: [PATCH] erc: Fix "dcc get" flag parsing has caused the debbugs.gnu.org bug report #62444, regarding [PATCH] erc: Fix "dcc get" flag parsing to be marked as done. (If you believe you have received this mail in error, please contact help-debbugs <at> gnu.org.) -- 62444: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=62444 GNU Bug Tracking System Contact help-debbugs <at> gnu.org with problems
[Message part 2 (message/rfc822, inline)]
From: Daniel Pettersson <daniel <at> dpettersson.net> To: bug-gnu-emacs <at> gnu.org Subject: [PATCH] erc: Fix "dcc get" flag parsing Date: Sat, 25 Mar 2023 16:25:48 +0100In erc mode when receiving a file with "/dcc get" if the nick or filename starts with a dash or the filename contains the following string " -", "/dcc get" is unable to download the file. Reproduce: As this is a bit cumbersome to reproduce without mocking files. I included a patch of erc-dcc-tests where the file name contains a the string " - ". Apply the following patch for erc-dcc-tests and run lisp-erc tests. --- diff --git a/test/lisp/erc/erc-dcc-tests.el b/test/lisp/erc/erc-dcc-tests.el index bd8a9fc7951..a487f9067cd 100644 --- a/test/lisp/erc/erc-dcc-tests.el +++ b/test/lisp/erc/erc-dcc-tests.el @@ -109,7 +109,7 @@ erc-dcc-do-GET-command :parent proc :ip "127.0.0.1" :port "9899" - :file "foo.bin" + :file "foo - .bin" :size 1405135128)) (erc-dcc-list (list elt)) ;; @@ -124,7 +124,7 @@ erc-dcc-do-GET-command erc-server-current-nick "dummy") (set-process-query-on-exit-flag proc nil) (cl-letf (((symbol-function 'read-file-name) - (lambda (&rest _) "foo.bin")) + (lambda (&rest _) "foo - .bin")) ((symbol-function 'erc-dcc-get-file) (lambda (&rest r) (push r calls)))) (goto-char (point-max)) @@ -134,36 +134,36 @@ erc-dcc-do-GET-command (ert-info ("No turbo") (should-not (plist-member elt :turbo)) (goto-char erc-input-marker) - (insert "/dcc GET tester foo.bin") + (insert "/dcc GET tester foo - .bin") (erc-send-current-line) (should-not (plist-member (car erc-dcc-list) :turbo)) - (should (equal (pop calls) (list elt "foo.bin" proc)))) + (should (equal (pop calls) (list elt "foo - .bin" proc)))) (ert-info ("Arg turbo in pos 2") (should-not (plist-member elt :turbo)) (goto-char erc-input-marker) - (insert "/dcc GET -t tester foo.bin") + (insert "/dcc GET -t tester foo - .bin") (erc-send-current-line) (should (eq t (plist-get (car erc-dcc-list) :turbo))) - (should (equal (pop calls) (list elt "foo.bin" proc)))) + (should (equal (pop calls) (list elt "foo - .bin" proc)))) (ert-info ("Arg turbo in pos 4") (setq elt (plist-put elt :turbo nil) erc-dcc-list (list elt)) (goto-char erc-input-marker) - (insert "/dcc GET tester -t foo.bin") + (insert "/dcc GET tester -t foo - .bin") (erc-send-current-line) (should (eq t (plist-get (car erc-dcc-list) :turbo))) - (should (equal (pop calls) (list elt "foo.bin" proc)))) + (should (equal (pop calls) (list elt "foo - .bin" proc)))) (ert-info ("Arg turbo in pos 6") (setq elt (plist-put elt :turbo nil) erc-dcc-list (list elt)) (goto-char erc-input-marker) - (insert "/dcc GET tester foo.bin -t") + (insert "/dcc GET tester foo - .bin -t") (erc-send-current-line) (should (eq t (plist-get (car erc-dcc-list) :turbo))) - (should (equal (pop calls) (list elt "foo.bin" proc)))))))) + (should (equal (pop calls) (list elt "foo - .bin" proc)))))))) (defun erc-dcc-tests--pcomplete-common (test-fn) (with-current-buffer (get-buffer-create "*erc-dcc-do-GET-command*") --- Issue present since: df1e553688b * Accommodate nonstandard turbo file senders in erc-dcc 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] 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" - filenames starting with r"-s|t +" - filenames with ending with r" -s|t" 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. --- 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 + 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)) (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 () +(defun erc-dcc-tests--erc-dcc-do-GET-command (file) (with-temp-buffer (let* ((proc (start-process "fake" (current-buffer) "sleep" "10")) (elt (list :nick "tester!~tester <at> fake.irc" @@ -109,7 +109,7 @@ erc-dcc-do-GET-command :parent proc :ip "127.0.0.1" :port "9899" - :file "foo.bin" + :file file :size 1405135128)) (erc-dcc-list (list elt)) ;; @@ -124,7 +124,7 @@ erc-dcc-do-GET-command erc-server-current-nick "dummy") (set-process-query-on-exit-flag proc nil) (cl-letf (((symbol-function 'read-file-name) - (lambda (&rest _) "foo.bin")) + (lambda (&rest _) file)) ((symbol-function 'erc-dcc-get-file) (lambda (&rest r) (push r calls)))) (goto-char (point-max)) @@ -134,36 +134,41 @@ erc-dcc-do-GET-command (ert-info ("No turbo") (should-not (plist-member elt :turbo)) (goto-char erc-input-marker) - (insert "/dcc GET tester foo.bin") + (insert "/dcc GET tester " file) (erc-send-current-line) (should-not (plist-member (car erc-dcc-list) :turbo)) - (should (equal (pop calls) (list elt "foo.bin" proc)))) + (should (equal (pop calls) (list elt file proc)))) (ert-info ("Arg turbo in pos 2") (should-not (plist-member elt :turbo)) (goto-char erc-input-marker) - (insert "/dcc GET -t tester foo.bin") + (insert "/dcc GET -t tester " file) (erc-send-current-line) (should (eq t (plist-get (car erc-dcc-list) :turbo))) - (should (equal (pop calls) (list elt "foo.bin" proc)))) + (should (equal (pop calls) (list elt file proc)))) (ert-info ("Arg turbo in pos 4") (setq elt (plist-put elt :turbo nil) erc-dcc-list (list elt)) (goto-char erc-input-marker) - (insert "/dcc GET tester -t foo.bin") + (insert "/dcc GET tester -t " file) (erc-send-current-line) (should (eq t (plist-get (car erc-dcc-list) :turbo))) - (should (equal (pop calls) (list elt "foo.bin" proc)))) + (should (equal (pop calls) (list elt file proc)))) (ert-info ("Arg turbo in pos 6") (setq elt (plist-put elt :turbo nil) erc-dcc-list (list elt)) (goto-char erc-input-marker) - (insert "/dcc GET tester foo.bin -t") + (insert "/dcc GET tester " file " -t") (erc-send-current-line) (should (eq t (plist-get (car erc-dcc-list) :turbo))) - (should (equal (pop calls) (list elt "foo.bin" proc)))))))) + (should (equal (pop calls) (list elt file proc)))))))) + +(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")) (defun erc-dcc-tests--pcomplete-common (test-fn) (with-current-buffer (get-buffer-create "*erc-dcc-do-GET-command*") -- 2.32.0 (Apple Git-132)
[Message part 3 (message/rfc822, inline)]
From: "J.P." <jp <at> neverwas.me> To: Daniel Pettersson <daniel <at> dpettersson.net> Cc: emacs-erc <at> gnu.org, 62444-done <at> debbugs.gnu.org Subject: Re: bug#62444: [PATCH] erc: Fix "dcc get" flag parsing Date: Sat, 08 Apr 2023 15:53:23 -0700Hi Daniel, Daniel Pettersson <daniel <at> dpettersson.net> writes: > Hi J.P., > > Sorry for the delay for some reason the responses bounced my mailbox, > first and foremost I would like to thank you for your patience and > clarity in your response. Not at all, and you're welcome. >> Lastly, I have to mention the dreaded copyright thing because I couldn't >> tell from the discussion for bug#57905 whether you ended up filing. If >> not and we go with changes resembling those you've proposed, you'll >> probably want to do so. > > Attached my assignment. Thanks. (I would have taken your word for it). >> 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.) > > This is great, reminds me that I have some homework here; coding > standard, reading some more elisp in emacs packages and the bug > reporting. > > Sorry about that. No worries. I'm quite the serial offender myself. >> 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. > > I don't have any feedback, looks great to me! Cool. I've added your changes to master and will probably backport the bug-fixing portion to 29.1 (or 29.2) once we figure out what to do with ERC's version number on the release branch. Thanks, and please consider contributing to ERC again in the future!
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.