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

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[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" <[hidden email]>
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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Victor Orlikowski <[hidden email]>
> 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).

Reply | Threaded
Open this post in threaded view
|

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 ([hidden email])
    (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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Victor Orlikowski <[hidden email]>
> CC: "[hidden email]" <[hidden email]>
> 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?

Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Victor Orlikowski <[hidden email]>
> CC: "[hidden email]" <[hidden email]>
> 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.

Reply | Threaded
Open this post in threaded view
|

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

Reply | Threaded
Open this post in threaded view
|

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

Victor Orlikowski
In reply to this post by 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" <[hidden email]>
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

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Victor Orlikowski <[hidden email]>
> 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" <[hidden email]>
> 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.

Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> wrote:

>> From: Victor Orlikowski <[hidden email]>
>> 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" <[hidden email]>
>> 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.

Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Victor Orlikowski <[hidden email]>
> CC: "[hidden email]" <[hidden email]>
> 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.