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

[J.P.]

Hi Daniel,

Thanks for submitting this report. I haven't gotten around to reviewing
your proposed changes properly but definitely will in the coming days.
For now, all I can offer are a few boring administrative notes.

Daniel Pettersson <daniel@dpettersson.net> writes:

> In 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.

Indeed, sorry for any inconvenience this might have caused.

> 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
[...]
>
> ---

This example makes sense, thanks. BTW, in the future, can you save out
your patches with git-format-patch and attach them somehow? This and
other conventions worth noting, such as formatting change-log style
commit messages, are detailed in CONTRIBUTE. As an example, I've
re-attached your patch with a reformatted commit message more along the
lines of what's expected (if you wouldn't mind taking a look).

Also, if you can remember, please add the header:

  X-Debbugs-CC: emacs-erc@gnu.org

to any future bug reports (see also `erc-bug').

> 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"

Guessing r"..." just means regexp? If not, please clarify.

> 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.

I would tend to agree. Perhaps we ought to go ahead and only make these
-s and -t flags valid in the terminal position. Normally, that'd be a
hassle, likely involving the introduction of a "compat" user option. But
we've deliberately refrained from announcing these DCC features (other
than in the "usage" portion of the Commentary section atop the library
file). So I think a trivially breaking change in a patch version, like
5.5.1, might be doable.

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.

Thanks,
J.P.

0001-POC-Demo-broken-flags-parsing-in-erc-dcc-do-GET-comm.patch
Description: Text Data

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