diff mbox series

Re* [PATCH] contrib/git-jump: cat output when not a terminal

Message ID xmqqy2pyqv11.fsf_-_@gitster.c.googlers.com (mailing list archive)
State New, archived
Headers show
Series Re* [PATCH] contrib/git-jump: cat output when not a terminal | expand

Commit Message

Junio C Hamano May 11, 2020, 4:46 p.m. UTC 
Jeff King <peff@peff.net> writes:

> On Mon, May 11, 2020 at 08:36:05AM -0700, Junio C Hamano wrote:
>
>> > So I'm OK to leave the status quo and let people use the GIT_EDITOR
>> > solution in this instance. But I'd also be happy to take a patch for
>> > "--no-editor" or similar if somebody wants to work it up.
>> 
>> I actually would support --no-editor.  One thing nobody noticed so
>> far is that "git-jump" is only compatible with editors that support
>> the "-q" option from the command line, and "cat" is not among them.
>
> Oh, good point. GIT_EDITOR='cat -- 2>/dev/null' works, but is rather
> obscure. :)

Lest we all forget...

-- >8 --
Subject: git-jump: just show the list with the "--no-editor" option

The "git jump" script (in contrib/) creates a list of interesting
places to be visited in an editor, and then open the editor to visit
the place.  Some editors, however, can read the list directly and
use it to visit these places (e.g. vim's quickfix list, or emacs's
find-grep/compilation buffer) and do not want "git jump" to invoke
a separate editor.

Users can _almost_ do this already by setting GIT_EDITOR to "cat",
except that "git jump" assumes that the editor it spawns support a
"-q" option from the command line, and unfortunately "cat" is not
among such editors.

Teach it the "--no-editor" option, which tells the command to show
the list it generated to its standard output.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 contrib/git-jump/git-jump | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

Comments

Jeff King May 12, 2020, 7:23 p.m. UTC | #1 
On Mon, May 11, 2020 at 09:46:34AM -0700, Junio C Hamano wrote:

> Lest we all forget...
> 
> -- >8 --
> Subject: git-jump: just show the list with the "--no-editor" option

Thanks for tying this up. It seems to work as advertised. A few nits:

> +edit=yes
> +
> +while	case "$#,$1" in

Tab between "while" and "case"?

> +	0,*) break ;;
> +	*,--no-editor) edit=no ;;
> +	*,--*) usage >&2; exit 1 ;;
> +	*) break ;;
> +	esac
> +do
> +	shift
> +done

I found the use of "case" in the loop conditional a little unusual. I'd
have probably written:

  while test $# -gt 0
  do
	case "$1" in
	--no-editor) edit=no ;;
	--*) usage >&2; exit 1 ;;
	*) break ;;
	esac
	shift
  done

> @@ -75,4 +87,9 @@ 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"
> +
> +case "$edit" in
> +yes)	open_editor "$tmp" ;;
> +no)	cat "$tmp" ;;
> +esac
> +

"diff --check" complains about the empty line.

It probably doesn't matter much, but we could skip the tempfile entirely
in no-editor mode. I.e.:

  if test "$edit" = "no"
  then
    "mode_$mode" "$@"
  fi

  # otherwise set up trap, mktemp, etc

-Peff
Junio C Hamano May 12, 2020, 9:30 p.m. UTC | #2 
Jeff King <peff@peff.net> writes:

> On Mon, May 11, 2020 at 09:46:34AM -0700, Junio C Hamano wrote:
>
>> Lest we all forget...
>> 
>> -- >8 --
>> Subject: git-jump: just show the list with the "--no-editor" option
>
> Thanks for tying this up. It seems to work as advertised. A few nits:
>
>> +edit=yes
>> +
>> +while	case "$#,$1" in
>
> Tab between "while" and "case"?

Yup.  Just to align case and its arms.

>> +	0,*) break ;;
>> +	*,--no-editor) edit=no ;;
>> +	*,--*) usage >&2; exit 1 ;;
>> +	*) break ;;
>> +	esac
>> +do
>> +	shift
>> +done
>
> I found the use of "case" in the loop conditional a little unusual.

It's pretty-much personal preference, I think.  I could replace
s/break/false/ if you find it easier to understand.

> It probably doesn't matter much, but we could skip the tempfile entirely
> in no-editor mode. I.e.:
>
>   if test "$edit" = "no"
>   then
>     "mode_$mode" "$@"
>   fi
>
>   # otherwise set up trap, mktemp, etc

Makes a lot of sense to me.
Jeff King May 13, 2020, 4:52 a.m. UTC | #3 
On Tue, May 12, 2020 at 02:30:34PM -0700, Junio C Hamano wrote:

> >> +edit=yes
> >> +
> >> +while	case "$#,$1" in
> >
> > Tab between "while" and "case"?
> 
> Yup.  Just to align case and its arms.

I guess that makes sense, though I'd probably have used spaces to do so.
Having a tab in the middle of the line is unusual.

> >> +	0,*) break ;;
> >> +	*,--no-editor) edit=no ;;
> >> +	*,--*) usage >&2; exit 1 ;;
> >> +	*) break ;;
> >> +	esac
> >> +do
> >> +	shift
> >> +done
> >
> > I found the use of "case" in the loop conditional a little unusual.
> 
> It's pretty-much personal preference, I think.  I could replace
> s/break/false/ if you find it easier to understand.

I think the part that most threw me off is looking at the arg-count in
each case arm. It's "*" in most, which really means "do not bother to
look at it" (which I think is why I found a loop condition on "$# -gt 0"
to be more natural).

I can live with it either way.

-Peff
diff mbox series

Patch

diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 931b0fe3a9..26a7159053 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 [--no-editor] <mode> [<args>]
 
 Jump to interesting elements in an editor.
 The <mode> parameter is one of:
@@ -64,6 +64,18 @@  mode_ws() {
 	git diff --check "$@"
 }
 
+edit=yes
+
+while	case "$#,$1" in
+	0,*) break ;;
+	*,--no-editor) edit=no ;;
+	*,--*) usage >&2; exit 1 ;;
+	*) break ;;
+	esac
+do
+	shift
+done
+
 if test $# -lt 1; then
 	usage >&2
 	exit 1
@@ -75,4 +87,9 @@  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"
+
+case "$edit" in
+yes)	open_editor "$tmp" ;;
+no)	cat "$tmp" ;;
+esac
+