git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Yoichi Nakayama via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
Subject: Re: [PATCH v3 1/2] git-jump: add an optional argument '--stdout'
Date: Mon, 21 Nov 2022 13:38:28 -0500	[thread overview]
Message-ID: <Y3vFpNbWswu/8gjb@coredump.intra.peff.net> (raw)
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.

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

  reply	other threads:[~2022-11-21 18:38 UTC|newest]

Thread overview: 68+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-19 14:02 [PATCH 0/2] git-jump: support Emacs Yoichi NAKAYAMA via GitGitGadget
2022-11-19 14:02 ` [PATCH 1/2] git-jump: add an optional argument 'stdout' Yoichi Nakayama via GitGitGadget
2022-11-19 14:02 ` [PATCH 2/2] git-jump: invoke emacsclient Yoichi Nakayama via GitGitGadget
2022-11-19 15:53   ` Eric Sunshine
2022-11-19 23:44     ` Yoichi Nakayama
2022-11-19 23:59       ` Eric Sunshine
2022-11-21  4:05         ` Yoichi Nakayama
2022-11-20  1:27 ` [PATCH v2 0/2] git-jump: support Emacs Yoichi NAKAYAMA via GitGitGadget
2022-11-20  1:27   ` [PATCH v2 1/2] git-jump: add an optional argument 'stdout' Yoichi Nakayama via GitGitGadget
2022-11-21  5:43     ` Junio C Hamano
2022-11-21 11:25       ` Yoichi Nakayama
2022-11-21 18:18       ` Jeff King
2022-11-20  1:27   ` [PATCH v2 2/2] git-jump: invoke emacsclient Yoichi Nakayama via GitGitGadget
2022-11-21 12:26   ` [PATCH v3 0/2] git-jump: support Emacs Yoichi NAKAYAMA via GitGitGadget
2022-11-21 12:26     ` [PATCH v3 1/2] git-jump: add an optional argument '--stdout' Yoichi Nakayama via GitGitGadget
2022-11-21 18:38       ` Jeff King [this message]
2022-11-21 23:38         ` Junio C Hamano
2022-11-22 13:00           ` Yoichi Nakayama
2022-11-22 18:23             ` Jeff King
2022-11-22 13:29         ` Yoichi Nakayama
2022-11-21 12:27     ` [PATCH v3 2/2] git-jump: invoke emacs/emacsclient Yoichi Nakayama via GitGitGadget
2022-11-21 18:50       ` Jeff King
2022-11-22 12:06         ` Yoichi Nakayama
2022-11-22 14:18     ` [PATCH v4 0/2] git-jump: support Emacs Yoichi NAKAYAMA via GitGitGadget
2022-11-22 14:18       ` [PATCH v4 1/2] git-jump: add an optional argument '--stdout' Yoichi Nakayama via GitGitGadget
2022-11-22 18:30         ` Jeff King
2022-11-22 14:18       ` [PATCH v4 2/2] git-jump: invoke emacs/emacsclient Yoichi Nakayama via GitGitGadget
2022-11-22 16:40         ` Phillip Wood
2022-11-23  5:01           ` Yoichi Nakayama
2022-11-22 18:54         ` Jeff King
2022-11-23  5:33           ` Yoichi Nakayama
2022-11-24  1:09             ` Jeff King
2022-11-24 12:32               ` Yoichi Nakayama
2022-11-24 12:58                 ` Yoichi Nakayama
2022-11-24 16:31                   ` Jeff King
2022-11-24 16:29                 ` Jeff King
2022-11-25  3:46                   ` Yoichi Nakayama
2022-11-23  7:04       ` [PATCH v5 0/3] git-jump: support Emacs Yoichi NAKAYAMA via GitGitGadget
2022-11-23  7:04         ` [PATCH v5 1/3] git-jump: add an optional argument '--stdout' Yoichi Nakayama via GitGitGadget
2022-11-23  7:04         ` [PATCH v5 2/3] git-jump: move valid-mode check earlier Jeff King via GitGitGadget
2022-11-23  7:04         ` [PATCH v5 3/3] git-jump: invoke emacs/emacsclient Yoichi Nakayama via GitGitGadget
2022-11-23 14:58           ` Phillip Wood
2022-11-23 21:54             ` Yoichi Nakayama
2022-11-24  1:11         ` [PATCH v5 0/3] git-jump: support Emacs Jeff King
2022-11-24  3:47         ` [PATCH v6 " Yoichi NAKAYAMA via GitGitGadget
2022-11-24  3:47           ` [PATCH v6 1/3] git-jump: add an optional argument '--stdout' Yoichi Nakayama via GitGitGadget
2022-11-24  3:47           ` [PATCH v6 2/3] git-jump: move valid-mode check earlier Jeff King via GitGitGadget
2022-11-24  3:47           ` [PATCH v6 3/3] git-jump: invoke emacs/emacsclient Yoichi Nakayama via GitGitGadget
2022-11-25  3:36           ` [PATCH v7 0/3] git-jump: support Emacs Yoichi NAKAYAMA via GitGitGadget
2022-11-25  3:36             ` [PATCH v7 1/3] git-jump: add an optional argument '--stdout' Yoichi Nakayama via GitGitGadget
2022-11-25  9:06               ` Ævar Arnfjörð Bjarmason
2022-11-27  0:31                 ` Yoichi Nakayama
2022-11-25  3:37             ` [PATCH v7 2/3] git-jump: move valid-mode check earlier Jeff King via GitGitGadget
2022-11-25  3:37             ` [PATCH v7 3/3] git-jump: invoke emacs/emacsclient Yoichi Nakayama via GitGitGadget
2022-11-25  8:55               ` Ævar Arnfjörð Bjarmason
2022-11-25 16:01                 ` Yoichi Nakayama
2022-11-25 23:52                   ` Ævar Arnfjörð Bjarmason
2022-11-28  5:10                     ` Jeff King
2022-11-28 10:54                       ` Ævar Arnfjörð Bjarmason
2022-11-28 15:38                         ` Yoichi Nakayama
2022-11-27  0:37                   ` Yoichi Nakayama
2022-11-27  1:18             ` [PATCH v8 0/3] git-jump: support Emacs Yoichi NAKAYAMA via GitGitGadget
2022-11-27  1:18               ` [PATCH v8 1/3] git-jump: add an optional argument '--stdout' Yoichi Nakayama via GitGitGadget
2022-11-27  1:18               ` [PATCH v8 2/3] git-jump: move valid-mode check earlier Jeff King via GitGitGadget
2022-11-27  1:18               ` [PATCH v8 3/3] git-jump: invoke emacs/emacsclient Yoichi Nakayama via GitGitGadget
2022-11-28 10:13               ` [PATCH v8 0/3] git-jump: support Emacs Phillip Wood
2022-11-28 23:35                 ` Junio C Hamano
2022-11-29 21:05                 ` Yoichi Nakayama

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y3vFpNbWswu/8gjb@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=yoichi.nakayama@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).