Re: [PATCH v3 1/2] git-jump: add an optional argument '--stdout'
- From
Jeff King <peff@peff.net>
- Date
- Nov 21, 2022, 18:38 UTC
- Message-ID
- <Y3vFpNbWswu/8gjb@coredump.intra.peff.net>
- In-Reply-To
- <ccfea26de333ac5a08a5df4c9b790811067bd437.1669033620.git.gitgitgadget@gmail.com>
On Mon, Nov 21, 2022 at 12:26:59PM +0000, Yoichi Nakayama via GitGitGadget wrote:
> From: Yoichi Nakayama <yoichi.nakayama@gmail.com> > > It can be used with M-x grep on Emacs.
Thanks, I like what this feature is doing overall, but I have some small nits about the implementation.
Show 8 quoted lines
> +You can use the optional argument '--stdout' to print the listing to > +standard output instead of feeding it to the editor. You can use the > +argument with M-x grep on Emacs: > + > +-------------------------------------------------- > +# In Emacs, M-x grep and invoke "git jump --stdout <mode>" > +Run grep (like this): git jump --stdout diff > +--------------------------------------------------
This example confused me because it says "run grep", but then runs a diff jump. But maybe this is because it means to run the emacs grep command? I don't use emacs, so it may make more sense to somebody who does.
Show 10 quoted lines
> @@ -69,6 +72,12 @@ if test $# -lt 1; then
> exit 1
> fi
> mode=$1; shift
> +if test "$mode" = "--stdout"; then
> + mode=$1; shift
> + type "mode_$mode" >/dev/null 2>&1 || { usage >&2; exit 1; }
> + "mode_$mode" "$@" 2>/dev/null
> + exit 0
> +fiBecause this happens after we check that "$1" isn't empty and call shift, it may not notice if the mode is missing when we do this second shift. I.e., with your patch I get:
$ ./git-jump --stdout ./git-jump: 76: shift: can't shift that many
when I should get the usage message. We can fix that by parsing out --stdout before we try to read the mode. It's a little more code, but I think it nicely sets us up if we ever want to parse more options.
It's also unfortunate that we have to repeat the ugly "type" check above, which also happens again later, after we make the temp file. I see why you did it this way; the stdout code path does not want to make the tempfile. But the code before your patch was silly to do it this way; we should always have been checking the parameters before making a tempfile.
I was also puzzled why the stdout mode redirects stderr from the mode function. Wouldn't the user want to see any errors?
So together, it might look something like this (instead of, rather than on top of your patch):
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump index b9cc602ebf..05a0ff1430 100755 --- a/contrib/git-jump/git-jump +++ b/contrib/git-jump/git-jump @@ -67,15 +67,38 @@ mode_ws() { git diff --check "$@" } +use_stdout= +while test $# -gt 0; do + case "$1" in + --stdout) + use_stdout=t + shift + ;; + --*) + usage >&2 + exit 1 + ;; + *) + break + ;; + esac +done + if test $# -lt 1; then usage >&2 exit 1 fi + mode=$1; shift +type "mode_$mode" >/dev/null 2>&1 || { usage >&2; exit 1; } + +if test "$use_stdout" = "t"; then + "mode_$mode" "$@" + exit 0 +fi trap 'rm -f "$tmp"' 0 1 2 3 15 tmp=`mktemp -t git-jump.XXXXXX` || exit 1 -type "mode_$mode" >/dev/null 2>&1 || { usage >&2; exit 1; } "mode_$mode" "$@" >"$tmp" test -s "$tmp" || exit 0 open_editor "$tmp" Though I'd perhaps break some of the shuffling into a preparatory patch. I'm happy to do that separately if you prefer. -Peff