[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#28323: Patchset to fix 28323
bug#28323: Patchset to fix 28323
Thu, 10 May 2018 21:01:25 -0400
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
Jay Kamat <address@hidden> 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.
> 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
[1: 170266d096]: 2013-09-12 01:20:07 -0400
Cleanup Eshell to rely less on dynamic scoping.
> 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
> 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
> 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).
> (eshell--set-option name ai (car opts) options opt-vals)
> (setq found t opts nil))
> @@ -245,27 +252,33 @@ eshell--process-args
> (list sym)))))
> (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?