| Message ID | ccfea26de333ac5a08a5df4c9b790811067bd437.1669033620.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-jump: support Emacs | expand |
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
Jeff King <peff@peff.net> writes: > 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. Yes. "M-x" gives Emacs users a command line prompt to type (and tab complete) an Emacs command, and in the above explanation, the user is running the "grep" command of Emacs, which in turn prompts for a shell command that produces series of <filename>:<lineno>:<cruft> to jump around [*]. "M-x grep<RET>git jump --stdout diff<RET>" is what I would have written on the second line instead of "Run grep (like this)...". [Footnote] * People often run "grep -n -r -e <pattern>" but you can run things like "git grep -n -e <pattern> -- <pathspec>" and "find -name \*.h | xargs grep -n -e <pattern>".
On Tue, Nov 22, 2022 at 8:38 AM Junio C Hamano <gitster@pobox.com> wrote: > > Jeff King <peff@peff.net> writes: > > > On Mon, Nov 21, 2022 at 12:26:59PM +0000, Yoichi Nakayama via GitGitGadget wrote: > > > >> From: Yoichi Nakayama <yoichi.nakayama@gmail.com> > >> +-------------------------------------------------- > >> +# 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. > > Yes. "M-x" gives Emacs users a command line prompt to type (and tab > complete) an Emacs command, and in the above explanation, the user > is running the "grep" command of Emacs, which in turn prompts for a > shell command that produces series of <filename>:<lineno>:<cruft> to > jump around [*]. > > "M-x grep<RET>git jump --stdout diff<RET>" is what I would have > written on the second line instead of "Run grep (like this)...". I think Junio's suggestion of "M-x grep<RET>git jump --stdout diff<RET>" is concise and understandable to most Emacs users, so I'd like to adopt it. Below are the details of what I thought: By M-x grep<RET>, Emacs displays Run grep (like this): grep --color=auto -nH --null -e where - "Run grep (like this): " is a prompt (like "$ " in bash). - "grep --color=auto -nH --null -e " is a part of search command (missing keyword part). We can supply "keyword<RET>" to execute the search. We can also remove the whole command and replace it with the command like "git jump --stdout diff". So "M-x grep<RET>git jump --stdout diff<RET>" does not represent the complete procedure. It lacks the operation to remove the default command (controlled by the grep-command setting). For example, adding C-a C-k after "M-x grep<RET>" is more accurate, but it feels a bit redundant.
On Tue, Nov 22, 2022 at 3:38 AM Jeff King <peff@peff.net> wrote: > 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): Thanks. I've applied it. I was throwing away stderr because Emacs' M-x grep inserted both stdout and stderr into the output destination, and a perl warning was issued. However, the warning itself is meaningful, and I thought it would be very bad not to know the reason when an error occurred, so I came to the conclusion that stderr should be left as is.
On Tue, Nov 22, 2022 at 10:00:45PM +0900, Yoichi Nakayama wrote: > > "M-x grep<RET>git jump --stdout diff<RET>" is what I would have > > written on the second line instead of "Run grep (like this)...". > > I think Junio's suggestion of "M-x grep<RET>git jump --stdout diff<RET>" > is concise and understandable to most Emacs users, so I'd like to adopt it. > > Below are the details of what I thought: > > By M-x grep<RET>, Emacs displays > Run grep (like this): grep --color=auto -nH --null -e > where > - "Run grep (like this): " is a prompt (like "$ " in bash). > - "grep --color=auto -nH --null -e " is a part of search command (missing > keyword part). We can supply "keyword<RET>" to execute the search. > We can also remove the whole command and replace it with the command > like "git jump --stdout diff". Ah, OK. I am happy with anything that emacs users will understand, and I trust the two of you to come up with something there. :) -Peff
diff --git a/contrib/git-jump/README b/contrib/git-jump/README index 8bcace29d21..0340980959b 100644 --- a/contrib/git-jump/README +++ b/contrib/git-jump/README @@ -79,6 +79,14 @@ git jump grep -i foo_bar git config jump.grepCmd "ag --column" -------------------------------------------------- +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 +-------------------------------------------------- Related Programs ---------------- @@ -100,7 +108,7 @@ Limitations ----------- This script was written and tested with vim. Given that the quickfix -format is the same as what gcc produces, I expect emacs users have a +format is the same as what gcc produces, I expect other tools have a similar feature for iterating through the list, but I know nothing about how to activate it. diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump index 92dbd4cde18..091d1add0ec 100755 --- a/contrib/git-jump/git-jump +++ b/contrib/git-jump/git-jump @@ -2,7 +2,7 @@ usage() { cat <<\EOF -usage: git jump <mode> [<args>] +usage: git jump [--stdout] <mode> [<args>] Jump to interesting elements in an editor. The <mode> parameter is one of: @@ -15,6 +15,9 @@ grep: elements are grep hits. Arguments are given to git grep or, if configured, to the command in `jump.grepCmd`. ws: elements are whitespace errors. Arguments are given to diff --check. + +If the optional argument `--stdout` is given, print the quickfix +lines to standard output instead of feeding it to the editor. EOF } @@ -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 trap 'rm -f "$tmp"' 0 1 2 3 15 tmp=`mktemp -t git-jump.XXXXXX` || exit 1