On Mon, Nov 21, 2022 at 12:26:59PM +0000, Yoichi Nakayama via GitGitGadget wrote:
> From: Yoichi Nakayama <yoichi.nakayama@xxxxxxxxx>
>
> 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.
> +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.
> @@ -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
> +fi
Because 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
