[PATCH] Fixes to allow erc-dcc-get-filter to work properly

[PATCH] Fixes to allow erc-dcc-get-filter to work properly

[Victor Orlikowski]

When using erc-dcc-get-filter with erc-dcc-verbose set to t, message
errors prevent the DCC get from completing correctly.

The attached patch adds some additional error checking to
erc-dcc-get-filter, and uses erc-dcc-file-name in place of
buffer-file-name (which ends up being nil in this context, and
causing the message errors).

From 22bbaa226ad0550f566b4d2978e0aea4c72f82a1 Mon Sep 17 00:00:00 2001
From: "Victor J. Orlikowski" <vjo@duke.edu>
Date: Sun, 3 Feb 2019 12:36:26 -0500
Subject: [PATCH] Perform additional validation in erc-dcc-get-filter, and use
 erc-dcc-file-name rather than buffer-file-name (which is actually nil in this
 context).

---
 lisp/erc/erc-dcc.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/erc/erc-dcc.el b/lisp/erc/erc-dcc.el
index 8a40b1454b..842a3b2b19 100644
--- a/lisp/erc/erc-dcc.el
+++ b/lisp/erc/erc-dcc.el
@@ -979,17 +979,20 @@ rather than every 1024 byte block, but nobody seems to care."
     (let ((inhibit-read-only t)
           received-bytes)
       (goto-char (point-max))
-      (insert (string-make-unibyte str))
+      (if str
+          (insert (string-make-unibyte str)))
 
       (when (> (point-max) erc-dcc-receive-cache)
         (erc-dcc-append-contents (current-buffer) erc-dcc-file-name))
-      (setq received-bytes (+ (buffer-size) erc-dcc-byte-count))
+      (setq received-bytes (buffer-size))
+      (if erc-dcc-byte-count
+          (setq received-bytes (+ received-bytes erc-dcc-byte-count)))
 
       (and erc-dcc-verbose
            (erc-display-message
             nil 'notice erc-server-process
             'dcc-get-bytes-received
-            ?f (file-name-nondirectory buffer-file-name)
+            ?f (file-name-nondirectory erc-dcc-file-name)
             ?b (number-to-string received-bytes)))
       (cond
        ((and (> (plist-get erc-dcc-entry-data :size) 0)
@@ -997,7 +1000,7 @@ rather than every 1024 byte block, but nobody seems to care."
         (erc-display-message
          nil '(notice error) 'active
          'dcc-get-file-too-long
-         ?f (file-name-nondirectory buffer-file-name))
+         ?f (file-name-nondirectory erc-dcc-file-name))
         (delete-process proc))
        (t
         (process-send-string
-- 
2.20.0

Best,
Victor
-- 
Victor J. Orlikowski <> vjo@(ee.|cs.)?duke.edu

Re: [PATCH] Fixes to allow erc-dcc-get-filter to work properly

[Eli Zaretskii]

> From: Victor Orlikowski <vjo@duke.edu>
> Date: Sun, 3 Feb 2019 17:47:16 +0000
> 
> When using erc-dcc-get-filter with erc-dcc-verbose set to t, message
> errors prevent the DCC get from completing correctly.
> 
> The attached patch adds some additional error checking to
> erc-dcc-get-filter, and uses erc-dcc-file-name in place of
> buffer-file-name (which ends up being nil in this context, and
> causing the message errors).

Thanks.  Can you please elaborate on the problem, perhaps showing an
example of the behavior before and after your patch?  I don't think I
understand the issue with buffer-file-name (but then I don't use ERC).

Re: [PATCH] Fixes to allow erc-dcc-get-filter to work properly

[Victor Orlikowski]

On Fri, Feb 08, 2019, at 09:59 AM, Eli Zaretskii wrote:
> Thanks.  Can you please elaborate on the problem, perhaps showing an
> example of the behavior before and after your patch?  I don't think I
> understand the issue with buffer-file-name (but then I don't use ERC).

Certainly.

The error checking I added came as a result of debugging the
problem, and *may* not be strictly necessary (the nil check for str,
the nil check for erc-dcc-byte-count).

I run ERC with erc-dcc-verbose set to t.
I was getting the following error message, repeating several times:

error in process filter: erc-dcc-get-filter: Wrong type argument: stringp, nil

when trying to receive a file from a colleague using the DCC mechanism. I
run ERC to communicate with colleagues via XMPP, using bitlbee as a
gateway.

After putting in the error checks mentioned above, I managed to get
a different error (again repeating several times) from
erc-dcc-get-filter:

error in process filter: erc-display-message: Wrong type argument: stringp, nil

When I set erc-dcc-verbose to nil, the above errors went away.

I set erc-dcc-verbose to t once more, and added code to the function
to print each of:

buffer-file-name
received-bytes

I discovered that buffer-file-name was nil - which was what
had been causing these errors to occur.

I noted, earlier in the function:

       (when (> (point-max) erc-dcc-receive-cache)
         (erc-dcc-append-contents (current-buffer) erc-dcc-file-name))

and chose to replace "buffer-file-name" with "erc-dcc-file-name" -
which got behavior I expected to see.

To clarify further - the behavior I expected to see was, with
erc-dcc-verbose set to t, that output similar to the following
should be generated in the server buffer:

*** DCC: file IDidntBringEnoughToShare.png offered by RobCarter (xxx@xxxx.xxx)
    (size 491883)                                                       [17:03]
*** DCC: IDidntBringEnoughToShare.png: 3624 bytes received
*** DCC: IDidntBringEnoughToShare.png: 4096 bytes received
*** DCC: IDidntBringEnoughToShare.png: 8192 bytes received
*** DCC: IDidntBringEnoughToShare.png: 12288 bytes received
*** DCC: IDidntBringEnoughToShare.png: 16384 bytes received
*** DCC: IDidntBringEnoughToShare.png: 20480 bytes received
*** DCC: IDidntBringEnoughToShare.png: 24576 bytes received
*** DCC: IDidntBringEnoughToShare.png: 28672 bytes received
*** DCC: IDidntBringEnoughToShare.png: 32768 bytes received
*** DCC: IDidntBringEnoughToShare.png: 36864 bytes received
*** DCC: IDidntBringEnoughToShare.png: 40960 bytes received
*** DCC: IDidntBringEnoughToShare.png: 45056 bytes received

... additional lines elided ...

*** DCC: IDidntBringEnoughToShare.png: 479232 bytes received
*** DCC: IDidntBringEnoughToShare.png: 483328 bytes received
*** DCC: IDidntBringEnoughToShare.png: 487424 bytes received
*** DCC: IDidntBringEnoughToShare.png: 491520 bytes received
*** DCC: IDidntBringEnoughToShare.png: 491883 bytes received
*** DCC: file ~/IDidntBringEnoughToShare.png transfer complete (491883 bytes
    in 8 seconds)

The errors I had been previously receiving (prior to patching the
function) occurred every erc-dcc-receive-cache bytes.

I am presently overriding this function in my ercrc.el via:

(eval-after-load "erc-dcc"
  '(defun erc-dcc-get-filter (proc str)

(rest of function elided)))

Does that clarify the issue sufficiently?

Thanks,
Victor
-- 
Victor J. Orlikowski <> vjo@(ee.|cs.)?duke.edu

Re: [PATCH] Fixes to allow erc-dcc-get-filter to work properly

[Eli Zaretskii]

> From: Victor Orlikowski <vjo@duke.edu>
> CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Fri, 8 Feb 2019 22:00:31 +0000
> 
> The error checking I added came as a result of debugging the
> problem, and *may* not be strictly necessary (the nil check for str,
> the nil check for erc-dcc-byte-count).
> 
> I run ERC with erc-dcc-verbose set to t.
> I was getting the following error message, repeating several times:
> 
> error in process filter: erc-dcc-get-filter: Wrong type argument: stringp, nil
> 
> when trying to receive a file from a colleague using the DCC mechanism. I
> run ERC to communicate with colleagues via XMPP, using bitlbee as a
> gateway.
> 
> After putting in the error checks mentioned above, I managed to get
> a different error (again repeating several times) from
> erc-dcc-get-filter:
> 
> error in process filter: erc-display-message: Wrong type argument: stringp, nil
> 
> When I set erc-dcc-verbose to nil, the above errors went away.
> 
> I set erc-dcc-verbose to t once more, and added code to the function
> to print each of:
> 
> buffer-file-name
> received-bytes
> 
> I discovered that buffer-file-name was nil - which was what
> had been causing these errors to occur.

Could it be that the problem is elsewhere?  erc-dcc-get-filter is a
filter function of a process, which is set up like this:

  (let* ((buffer (generate-new-buffer (file-name-nondirectory file)))
         proc)
    (with-current-buffer buffer
    [...]
      (setq erc-dcc-file-name file)

      [...]
      (setq proc
            (funcall erc-dcc-connect-function
                     "dcc-get" buffer
                     (plist-get entry :ip)
                     (string-to-number (plist-get entry :port))
                     entry))
      (set-process-buffer proc buffer)
      (set-process-coding-system proc 'binary 'binary)
      (set-buffer-file-coding-system 'binary t)

      (set-process-filter proc 'erc-dcc-get-filter)

As you see, the process whose filter is erc-dcc-get-filter is set up
so that the name of its process-buffer is derived from
erc-dcc-file-name, but I don't see the buffer-file-name of that buffer
being set anywhere.  If I'm right, then your change should use
buffer-name (the function) instead of buffer-file-name.  Can you
verify that buffer-name returns the name of erc-dcc-file-name, and
that using that return value resolves the problems you saw?

Re: [PATCH] Fixes to allow erc-dcc-get-filter to work properly

[Victor Orlikowski]

On Sat, Feb 09, 2019, at 04:49 AM, Eli Zaretskii wrote:
> As you see, the process whose filter is erc-dcc-get-filter is set up
> so that the name of its process-buffer is derived from
> erc-dcc-file-name, but I don't see the buffer-file-name of that buffer
> being set anywhere.  If I'm right, then your change should use
> buffer-name (the function) instead of buffer-file-name.  Can you
> verify that buffer-name returns the name of erc-dcc-file-name, and
> that using that return value resolves the problems you saw?

I can confirm that using '(buffer-name)' instead of
'erc-dcc-file-name' works *perfectly*.

Shall I submit a new patch, using that instead?

Thanks,
Victor
-- 
Victor J. Orlikowski <> vjo@(ee.|cs.)?duke.edu

Re: [PATCH] Fixes to allow erc-dcc-get-filter to work properly

[Eli Zaretskii]

> From: Victor Orlikowski <vjo@duke.edu>
> CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Sun, 10 Feb 2019 16:07:51 +0000
> 
> I can confirm that using '(buffer-name)' instead of
> 'erc-dcc-file-name' works *perfectly*.
> 
> Shall I submit a new patch, using that instead?

Yes, please.

Re: [PATCH] Fixes to allow erc-dcc-get-filter to work properly

[Victor Orlikowski]

On Sun, Feb 10, 2019, at 06:41 PM, Eli Zaretskii wrote:
> Yes, please.

NP; re-submitting in a separate email.

Victor
-- 
Victor J. Orlikowski <> vjo@(ee.|cs.)?duke.edu

[PATCH] v2: Fixes to allow erc-dcc-get-filter to work properly

[Victor Orlikowski]

When using erc-dcc-get-filter with erc-dcc-verbose set to t, message
errors prevent the DCC get from completing correctly.

The attached patch adds some additional error checking to
erc-dcc-get-filter, and uses ethe function buffer-name in place of
the variable buffer-file-name (which appears to be nil in this
context, which thereby causes the message errors).

From cd390337b8e819248568b900e6115dde8a19fde9 Mon Sep 17 00:00:00 2001
From: "Victor J. Orlikowski" <vjo@duke.edu>
Date: Sun, 10 Feb 2019 11:13:57 -0500
Subject: [PATCH] Perform additional validation in erc-dcc-get-filter, and use
 the function buffer-name rather than buffer-file-name (which is actually nil
 in this context).

---
 lisp/erc/erc-dcc.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/erc/erc-dcc.el b/lisp/erc/erc-dcc.el
index 8a40b1454b..2849e25bf7 100644
--- a/lisp/erc/erc-dcc.el
+++ b/lisp/erc/erc-dcc.el
@@ -979,17 +979,20 @@ rather than every 1024 byte block, but nobody seems to care."
     (let ((inhibit-read-only t)
           received-bytes)
       (goto-char (point-max))
-      (insert (string-make-unibyte str))
+      (if str
+          (insert (string-make-unibyte str)))
 
       (when (> (point-max) erc-dcc-receive-cache)
         (erc-dcc-append-contents (current-buffer) erc-dcc-file-name))
-      (setq received-bytes (+ (buffer-size) erc-dcc-byte-count))
+      (setq received-bytes (buffer-size))
+      (if erc-dcc-byte-count
+          (setq received-bytes (+ received-bytes erc-dcc-byte-count)))
 
       (and erc-dcc-verbose
            (erc-display-message
             nil 'notice erc-server-process
             'dcc-get-bytes-received
-            ?f (file-name-nondirectory buffer-file-name)
+            ?f (file-name-nondirectory (buffer-name))
             ?b (number-to-string received-bytes)))
       (cond
        ((and (> (plist-get erc-dcc-entry-data :size) 0)
@@ -997,7 +1000,7 @@ rather than every 1024 byte block, but nobody seems to care."
         (erc-display-message
          nil '(notice error) 'active
          'dcc-get-file-too-long
-         ?f (file-name-nondirectory buffer-file-name))
+         ?f (file-name-nondirectory (buffer-name)))
         (delete-process proc))
        (t
         (process-send-string
-- 
2.20.0

Thanks,
Victor
-- 
Victor J. Orlikowski <> vjo@(ee.|cs.)?duke.edu

Re: [PATCH] v2: Fixes to allow erc-dcc-get-filter to work properly

[Eli Zaretskii]

> From: Victor Orlikowski <vjo@duke.edu>
> Date: Sun, 10 Feb 2019 19:14:26 +0000
> 
> When using erc-dcc-get-filter with erc-dcc-verbose set to t, message
> errors prevent the DCC get from completing correctly.
> 
> The attached patch adds some additional error checking to
> erc-dcc-get-filter, and uses ethe function buffer-name in place of
> the variable buffer-file-name (which appears to be nil in this
> context, which thereby causes the message errors).
> 
> >From cd390337b8e819248568b900e6115dde8a19fde9 Mon Sep 17 00:00:00 2001
> From: "Victor J. Orlikowski" <vjo@duke.edu>
> Date: Sun, 10 Feb 2019 11:13:57 -0500
> Subject: [PATCH] Perform additional validation in erc-dcc-get-filter, and use
>  the function buffer-name rather than buffer-file-name (which is actually nil
>  in this context).

Thanks, I pushed this to the emacs-26 branch.

For the future, please note the following 2 gotchas:

  . The title of your patch (in the Subject) was too long, and our
    Git commit hooks rejected it.  It should be a single line, not
    ending in a period, and it should not exceed 79 characters
  . You didn't provide ChangeLog-style entries for the changes you
    made, which mention the modified function(s) and what was changed
    in each one.

This is described in more detail in CONTRIBUTE, which I suggest to
read.

Thanks again for working on this.

Re: [PATCH] v2: Fixes to allow erc-dcc-get-filter to work properly

[Victor Orlikowski]

Eli,

My apologies for the belated reply, and for failing to properly follow the contribution guidelines. I'll read them and make every effort to comply with them, for any future patches I submit.

My thanks for your suggested changes, willingness to work with me, and acceptance of this patch.

Regarding my other patch (for properly backgrounding ERC's reconnection attempts) - should I re-submit, with attention to complying with contribution guidelines?

Also - what do I need to do to have it further considered for acceptance? IIRC, you had asked for a willing volunteer to review/test it, since you yourself do not use ERC.

Thanks again,
Victor
--
Victor J. Orlikowski <> vjo@(ee.|cs.)?duke.edu

On Feb 15, 2019, at 3:19 AM, Eli Zaretskii <eliz@gnu.org> wrote:

>> From: Victor Orlikowski <vjo@duke.edu>
>> Date: Sun, 10 Feb 2019 19:14:26 +0000
>> 
>> When using erc-dcc-get-filter with erc-dcc-verbose set to t, message
>> errors prevent the DCC get from completing correctly.
>> 
>> The attached patch adds some additional error checking to
>> erc-dcc-get-filter, and uses ethe function buffer-name in place of
>> the variable buffer-file-name (which appears to be nil in this
>> context, which thereby causes the message errors).
>> 
>>> From cd390337b8e819248568b900e6115dde8a19fde9 Mon Sep 17 00:00:00 2001
>> From: "Victor J. Orlikowski" <vjo@duke.edu>
>> Date: Sun, 10 Feb 2019 11:13:57 -0500
>> Subject: [PATCH] Perform additional validation in erc-dcc-get-filter, and use
>> the function buffer-name rather than buffer-file-name (which is actually nil
>> in this context).
> 
> Thanks, I pushed this to the emacs-26 branch.
> 
> For the future, please note the following 2 gotchas:
> 
>  . The title of your patch (in the Subject) was too long, and our
>    Git commit hooks rejected it.  It should be a single line, not
>    ending in a period, and it should not exceed 79 characters
>  . You didn't provide ChangeLog-style entries for the changes you
>    made, which mention the modified function(s) and what was changed
>    in each one.
> 
> This is described in more detail in CONTRIBUTE, which I suggest to
> read.
> 
> Thanks again for working on this.

Re: [PATCH] v2: Fixes to allow erc-dcc-get-filter to work properly

[Eli Zaretskii]

> From: Victor Orlikowski <vjo@duke.edu>
> CC: "emacs-devel@gnu.org" <emacs-devel@gnu.org>
> Date: Sun, 17 Feb 2019 18:57:16 +0000
> 
> Regarding my other patch (for properly backgrounding ERC's reconnection attempts) - should I re-submit, with attention to complying with contribution guidelines?

I'd suggest first to wait for someone to review it.

> Also - what do I need to do to have it further considered for acceptance? IIRC, you had asked for a willing volunteer to review/test it, since you yourself do not use ERC.

Yes, I don't consider myself an expert enough on ERC to review the
patch.  Perhaps try finding someone who knows more than I do, not sure
where to look, to tell the truth.  Sorry.