bug#28323: Patchset to fix 28323

[Noam Postavsky]

Jay Kamat <jaygkamat@gmail.com> writes:

> This was bugging me for other commands I was running (emerge -uDN
> world), so I decided to try to write a patch for this.

Thanks!

> In the process, I found another blocking bug, which would have broken
> the -u flag entirely. Currently, 'sudo -u root whoami' returns '-u',
> because of a bug involved with processing leading positional arguments.

Ah, looks like [1: 170266d096] missed a rename of args into
eshell--args.

[1: 170266d096]: 2013-09-12 01:20:07 -0400
  Cleanup Eshell to rely less on dynamic scoping.
  
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=170266d096bc4d0952bee907532d14503e882bf6

> I fixed the blocking bug first in a separate patch, and then added a new
> parameter for commands like sudo, which would prefer :raw-positional
> arguments after the first non flag argument. I'm still not sure if this
> is the best name for this idea, so if anyone has a better name I'd be
> happy to change it!

:parse-leading-options-only?  Perhaps that's too long though.

> * lisp/eshell/esh-opt.el (eshell--process-args): Refactor usage of
>   args to eshell--args, as we rely on modifications from
>   eshell--process-option and vice versa. These modifications were not
                                          ^
                                          double space
>   being propogated in the (if (= ai 0)) case, since we cannot
>   destructively modify the first element of a list.

This sentence here is a bit confusing, because of course we can modify
the first element with setcar, but you meant something different.
Something more like

    Popping the first element of a list doesn't destructively modify the
    underlying list object.

>
> Examples of broken behavior:
>
> sudo -u root whoami
> ls -I '*.txt' /dev/null

I think it would be helpful to mention how these are broken.

> * lisp/eshell/esh-opt.el: Add a new :raw-positional argument which

This entry should start with

* lisp/eshell/esh-opt.el (eshell-eval-using-options):

>   ignores dash/switch arguments after the first positional
>   argument. Useful for eshell/sudo, to avoid parsing subcommand
             ^
             double space
>   switches as switches of the root command.

Though I think it would make more sense to put the second sentence in
its own "* lisp/eshell/em-tramp.el (eshell/sudo)" entry.

> (eshell--process-option): add a new posarg argument which signals that
> we have found a positional argument already.

This entry should mention eshell--process-args as well, but actually I
think it would make sense to change the patch, such that only
eshell--process-args is fixed, see below.

> -(defun eshell--process-option (name switch kind ai options opt-vals)
> +(defun eshell--process-option (name switch kind ai posarg options opt-vals)
>    "For NAME, process SWITCH (of type KIND), from args at index AI.
>  The SWITCH will be looked up in the set of OPTIONS.
>  
> @@ -219,7 +223,10 @@ eshell--process-option
>      (while opts
>        (if (and (listp (car opts))
>                 (nth kind (car opts))
> -               (equal switch (nth kind (car opts))))
> +               (equal switch (nth kind (car opts)))
> +               ;; If we want to ignore arguments after a pos one, don't find.
> +               (not (and posarg
> +                         (memq ':raw-positional options))))

By the way, you don't need to quote keyword symbols (though it does
still work fine).

>         (progn
>           (eshell--set-option name ai (car opts) options opt-vals)
>           (setq found t opts nil))
> @@ -245,27 +252,33 @@ eshell--process-args
>                                               (list sym)))))
>                                    options)))
>           (ai 0) arg
> -         (eshell--args args))
> +         (eshell--args args)
> +         (pos-argument-found nil))
>      (while (< ai (length eshell--args))
>        (setq arg (nth ai eshell--args))
>        (if (not (and (stringp arg)
>                   (string-match "^-\\(-\\)?\\(.*\\)" arg)))
> -       (setq ai (1+ ai))
> +          ;; Positional argument found, skip
> +       (setq ai (1+ ai)
> +                pos-argument-found t)
> +        ;; dash or switch argument found, parse

I think you should be able to just terminate the loop here once you have
pos-argument-found and (memq :raw-positional options) is true, rather
than passing an arg to eshell--process-option to make the rest of the
loop iterations into nops.

> -         (setcdr (nthcdr (1- ai) eshell--args)
> +            (setcdr (nthcdr (1- ai) eshell--args)

This is just a whitespace change, right?