git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] contrib/git-jump: cat output when not a terminal
@ 2020-05-09 19:15 George Brown
  2020-05-09 21:41 ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: George Brown @ 2020-05-09 19:15 UTC (permalink / raw)
  To: git; +Cc: peff

contrib/git-jump: cat output when not a terminal

The current usage to populate Vim's quickfix list cannot be used from
within the editor as it invokes another instance.

Check if stdout is to a terminal or not. If not simply cat the output.

Signed-off-by: George Brown <321.george@gmail.com>
---
 contrib/git-jump/git-jump | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 931b0fe3a9..253341c64e 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -19,8 +19,12 @@ EOF
 }

 open_editor() {
-    editor=`git var GIT_EDITOR`
-    eval "$editor -q \$1"
+    if test -t 1; then
+        editor=`git var GIT_EDITOR`
+        eval "$editor -q \$1"
+    else
+        cat "$1"
+    fi
 }

 mode_diff() {

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-09 19:15 George Brown
@ 2020-05-09 21:41 ` Junio C Hamano
  2020-05-09 22:04   ` George Brown
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2020-05-09 21:41 UTC (permalink / raw)
  To: George Brown; +Cc: git, peff

George Brown <321.george@gmail.com> writes:

> contrib/git-jump: cat output when not a terminal
>
> The current usage to populate Vim's quickfix list cannot be used from
> within the editor as it invokes another instance.
>
> Check if stdout is to a terminal or not. If not simply cat the output.

The are editors that open their own window, or tell an already open
editor instance to edit the buffer (think: emacsclient) and in order
to be functional, they do not have to be launched from inside a
working terminal window (whose input comes from the keyboard---the
test "-t 1" is an approximate check for that).

Wouldn't this change hurt the users of such an editor?

Would it work to set GIT_EDITOR to "cat" while performing the
"populating quickfix list" (whatever that is) operation?

> Signed-off-by: George Brown <321.george@gmail.com>
> ---
>  contrib/git-jump/git-jump | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> index 931b0fe3a9..253341c64e 100755
> --- a/contrib/git-jump/git-jump
> +++ b/contrib/git-jump/git-jump
> @@ -19,8 +19,12 @@ EOF
>  }
>
>  open_editor() {
> -    editor=`git var GIT_EDITOR`
> -    eval "$editor -q \$1"
> +    if test -t 1; then
> +        editor=`git var GIT_EDITOR`
> +        eval "$editor -q \$1"
> +    else
> +        cat "$1"
> +    fi
>  }
>
>  mode_diff() {

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-09 21:41 ` Junio C Hamano
@ 2020-05-09 22:04   ` George Brown
  2020-05-09 23:42     ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: George Brown @ 2020-05-09 22:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff

> The are editors that open their own window, or tell an already open
> editor instance to edit the buffer (think: emacsclient) and in order
> to be functional, they do not have to be launched from inside a
> working terminal window (whose input comes from the keyboard---the
> test "-t 1" is an approximate check for that).
>
> Wouldn't this change hurt the users of such an editor?

I'm unsure of emacsclient but I would have thought that editors in
general would be more amenable to receiving lines than relying on
additional spawned instances to behave as clients.

> Would it work to set GIT_EDITOR to "cat" while performing the
> "populating quickfix list" (whatever that is) operation?

It would, though I considered this to be less elegant.

Just for background the quickfix list in Vim is simply a list of
locations that you can browse and jump to.

On Sat, 9 May 2020 at 22:41, Junio C Hamano <gitster@pobox.com> wrote:
>
> George Brown <321.george@gmail.com> writes:
>
> > contrib/git-jump: cat output when not a terminal
> >
> > The current usage to populate Vim's quickfix list cannot be used from
> > within the editor as it invokes another instance.
> >
> > Check if stdout is to a terminal or not. If not simply cat the output.
>
> The are editors that open their own window, or tell an already open
> editor instance to edit the buffer (think: emacsclient) and in order
> to be functional, they do not have to be launched from inside a
> working terminal window (whose input comes from the keyboard---the
> test "-t 1" is an approximate check for that).
>
> Wouldn't this change hurt the users of such an editor?
>
> Would it work to set GIT_EDITOR to "cat" while performing the
> "populating quickfix list" (whatever that is) operation?
>
> > Signed-off-by: George Brown <321.george@gmail.com>
> > ---
> >  contrib/git-jump/git-jump | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
> > index 931b0fe3a9..253341c64e 100755
> > --- a/contrib/git-jump/git-jump
> > +++ b/contrib/git-jump/git-jump
> > @@ -19,8 +19,12 @@ EOF
> >  }
> >
> >  open_editor() {
> > -    editor=`git var GIT_EDITOR`
> > -    eval "$editor -q \$1"
> > +    if test -t 1; then
> > +        editor=`git var GIT_EDITOR`
> > +        eval "$editor -q \$1"
> > +    else
> > +        cat "$1"
> > +    fi
> >  }
> >
> >  mode_diff() {

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-09 22:04   ` George Brown
@ 2020-05-09 23:42     ` Junio C Hamano
  2020-05-10  9:03       ` George Brown
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2020-05-09 23:42 UTC (permalink / raw)
  To: George Brown; +Cc: git, peff

George Brown <321.george@gmail.com> writes:

>> The are editors that open their own window, or tell an already open
>> editor instance to edit the buffer (think: emacsclient) and in order
>> to be functional, they do not have to be launched from inside a
>> working terminal window (whose input comes from the keyboard---the
>> test "-t 1" is an approximate check for that).
>>
>> Wouldn't this change hurt the users of such an editor?
>
> I'm unsure of emacsclient but I would have thought that editors in
> general would be more amenable to receiving lines than relying on
> additional spawned instances to behave as clients.

I am not sure what you mean by "receiving lines".  Your patch makes
the "git jump" command completely ignore configured GIT_EDITOR and
refuse to invoke *any* editor.





^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-09 23:42     ` Junio C Hamano
@ 2020-05-10  9:03       ` George Brown
  2020-05-10 16:47         ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: George Brown @ 2020-05-10  9:03 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff

> I am not sure what you mean by "receiving lines".  Your patch makes
> the "git jump" command completely ignore configured GIT_EDITOR and
> refuse to invoke *any* editor.

Running "git jump" from a shell prompt works as before with this patch.

But now within Vim it can also be used to populate the quickfix list
with something like ":cexpr system('git jump')". Because instead of
invoking another instance of (which does not work well for Vim) it
just receives the lines as formatted by "git jump".

> > Would it work to set GIT_EDITOR to "cat" while performing the
> > "populating quickfix list" (whatever that is) operation?
>
> It would, though I considered this to be less elegant.

Just to revise this, it wouldn't actually as the editor is always passed
"-q" (intended for Vim's quickfix list). So overriding this to "cat"
causes as error as "-q" is not a valid option for it.

I'm aware the examples I'm giving are specific to Vim but it really
seems that "git jump" was intended with Vim in mind. That said I'd argue
most other editors would work better with "git jump" if when they
invoked it another instance wasn't created. I've tried find an
equivalent to Vim's quickfix in Emacs this morning but can't quite wrap
my head around it.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10  9:03       ` George Brown
@ 2020-05-10 16:47         ` Junio C Hamano
  2020-05-10 17:33           ` George Brown
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2020-05-10 16:47 UTC (permalink / raw)
  To: George Brown; +Cc: git, peff

George Brown <321.george@gmail.com> writes:

> Running "git jump" from a shell prompt works as before with this patch.

I am not worried at all about that use case.  It is clear that you
had the "shell prompt" in mind from "test -t 1".

Given what "git jump" does, wouldn't it be natural to expect that
GUI programs that have no "shell prompt" interaction with the end
user want to use "git jump"?  It may even be driving a graphical
version of vim---or is it impossible for such a GUI program to
exist?

As long as such a regression for existing users use is impossible, I
think the patch is probably OK, but doing a hardcoded "cat" smells
like a very bad hack, compared to a solution on the program's side
that *wants* to read the prepared file to arrange that to happen
(e.g. via setting the GIT_EDITOR environment to "cat" within that
program).

In any case, I am not a "git jump" user, so...


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 16:47         ` Junio C Hamano
@ 2020-05-10 17:33           ` George Brown
  2020-05-10 18:12             ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: George Brown @ 2020-05-10 17:33 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff

> As long as such a regression for existing users use is impossible, I
> think the patch is probably OK, but doing a hardcoded "cat" smells
> like a very bad hack, compared to a solution on the program's side
> that *wants* to read the prepared file to arrange that to happen
> (e.g. via setting the GIT_EDITOR environment to "cat" within that
> program).

I think this change only enhances "git jump".

> In any case, I am not a "git jump" user, so...

I looked into this as other Vim users were talking about it and wanted
this behavior.

https://www.reddit.com/r/vim/comments/gf2he2/what_is_the_simplest_way_to_get_all_git_changes/fprlxtk/?context=3

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 17:33           ` George Brown
@ 2020-05-10 18:12             ` Junio C Hamano
  2020-05-10 18:34               ` George Brown
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2020-05-10 18:12 UTC (permalink / raw)
  To: George Brown; +Cc: git, peff

George Brown <321.george@gmail.com> writes:

>> As long as such a regression for existing users use is impossible, I
>> think the patch is probably OK, but doing a hardcoded "cat" smells
>> like a very bad hack, compared to a solution on the program's side
>> that *wants* to read the prepared file to arrange that to happen
>> (e.g. via setting the GIT_EDITOR environment to "cat" within that
>> program).
>
> I think this change only enhances "git jump".
>
>> In any case, I am not a "git jump" user, so...
>
> I looked into this as other Vim users were talking about it and wanted
> this behavior.

That's already irrelevant in the course of this discussion, no?

We have already established that you wrote in good faith to improve
the use experience by vim users, and nobody in this exchange doubts
that this change would help vim users.  

I am worried about the change hurting non-vim users, but no amount
of "me too" from vim users who care only about their vim experience
would allay that.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 18:12             ` Junio C Hamano
@ 2020-05-10 18:34               ` George Brown
  2020-05-10 19:10                 ` Junio C Hamano
  2020-05-11 14:31                 ` Jeff King
  0 siblings, 2 replies; 19+ messages in thread
From: George Brown @ 2020-05-10 18:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff

I think with this change all editors can benefit. The format "git jump"
is producing is something easily consumed. I think consumption of output
from tools is far more common in editors than communication between
multiple instances.

As an aside the fact that as is "git jump" invokes "$GIT_EDITOR" with
the "-q" option makes an implicit assumption the editor will be Vim or
something very much like it. To be very clear I don't mean to say this
means only Vim should be considered. However it's also making the
implicit assumption that passing the "-q" option is valid for any
"$GIT_EDITOR" and does not cause an error like that seen when trying to
override "$GIT_EDITOR" with cat. This change means other editors can
invoke "git jump" without fear of such a situation, increasing
usability.

Arguably the most interoperable way for "git jump" to work would be to
output the formatted lines and do nothing else, leaving it to users to
choose how to operate upon the output/invoke editors. Of course such
a change would break the workflow of anyone who uses "git jump" today
and isn't a valid option.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 18:34               ` George Brown
@ 2020-05-10 19:10                 ` Junio C Hamano
  2020-05-10 19:25                   ` George Brown
  2020-05-10 19:38                   ` Junio C Hamano
  2020-05-11 14:31                 ` Jeff King
  1 sibling, 2 replies; 19+ messages in thread
From: Junio C Hamano @ 2020-05-10 19:10 UTC (permalink / raw)
  To: George Brown; +Cc: git, peff

George Brown <321.george@gmail.com> writes:

> I think with this change all editors can benefit.

I am worried not only about the editors that are launched by
git-jump that runs GIT_EDITOR, or the use case where you run
git-jump from within your editor.  "git jump" that is launched from
a process, whose standard output steam is not connected to a
terminal, used to spawn the specified editor just fine.  Imagine a
user wanted to spawn a graphical editor via git-jump from within a
GUI script (probably launched from a window manager menu and has no
terminal).  With git-jump with this patch, such a use will be
broken, no?  The GIT_EDITOR the user set is totally ignored.

At the very least, we should have an escape hatch to help those this
patch is hurting, perhaps like

        open_editor() {
                if ! test -t 1 && test -z "$GIT_JUMP_IGNORE_STDOUT_CHECK"
                then
                        cat "$1"
                else
                        editor=`git var GIT_EDITOR`
                        eval "$editor -q \$1"
                fi
        }

so that at least they can spawn their chosen editor, not "cat", from
the tool they have been using.  

Because this is a new feature, instead of breaking existing users
and forcing them to do something different they did not have to
(namely, set and export the GIT_JUMP_IGNORE_STDOUT_CHECK environment
variable), we should instead make this a non-default behaviour and
those who want it should explicitly opt-in to trigger it, perhaps
like:

        open_editor() {
                if ! test -t 1 && test -n "$GIT_JUMP_AUTO_CAT"
                then
                        cat "$1"
                else
                        editor=`git var GIT_EDITOR`
                        eval "$editor -q \$1"
                fi
        }

so that existing users won't get affected by this change at all,
while allowing you and other vim users to set and export the
environment variable just once.  

Unilaterally breaking, and ignoring when you are told you are
breaking, possible existing users, without giving them any escape
hatch, is simply irresponsible, and not something done in this
project.  But I am sensing that you are not listening to and
thinking about what you hear before you respond, so I will stop
spending time on this topic for now, and wait until others chime in.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 19:10                 ` Junio C Hamano
@ 2020-05-10 19:25                   ` George Brown
  2020-05-10 19:38                   ` Junio C Hamano
  1 sibling, 0 replies; 19+ messages in thread
From: George Brown @ 2020-05-10 19:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff

> Imagine a user wanted to spawn a graphical editor via git-jump from within
> a GUI script (probably launched from a window manager menu and has no
> gqipterminal).  With git-jump with this patch, such a use will be broken, no?

Aha OK that's an example that I didn't grok from your earlier mails,
I had only been thinking in the context of editors. The reasoning behind
the "escape hatch" you describe makes sense.

> Unilaterally breaking, and ignoring when you are told you are
> breaking, possible existing users, without giving them any escape
> hatch, is simply irresponsible, and not something done in this
> project.  But I am sensing that you are not listening to and
> thinking about what you hear before you respond, so I will stop
> spending time on this topic for now, and wait until others chime in.

I apologise for not understanding, it was not my intention to be
willfully ignorant.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 19:10                 ` Junio C Hamano
  2020-05-10 19:25                   ` George Brown
@ 2020-05-10 19:38                   ` Junio C Hamano
  2020-05-10 20:20                     ` George Brown
  1 sibling, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2020-05-10 19:38 UTC (permalink / raw)
  To: George Brown; +Cc: git, peff

Junio C Hamano <gitster@pobox.com> writes:

> Because this is a new feature, instead of breaking existing users
> and forcing them to do something different they did not have to
> (namely, set and export the GIT_JUMP_IGNORE_STDOUT_CHECK environment
> variable), we should instead make this a non-default behaviour and
> those who want it should explicitly opt-in to trigger it, perhaps
> like:
>
>         open_editor() {
>                 if ! test -t 1 && test -n "$GIT_JUMP_AUTO_CAT"
>                 then
>                         cat "$1"
>                 else
>                         editor=`git var GIT_EDITOR`
>                         eval "$editor -q \$1"
>                 fi
>         }
>
> so that existing users won't get affected by this change at all,
> while allowing you and other vim users to set and export the
> environment variable just once.  
>
> Unilaterally breaking, and ignoring when you are told you are
> breaking, possible existing users, without giving them any escape
> hatch, is simply irresponsible, and not something done in this
> project.  But I am sensing that you are not listening to and
> thinking about what you hear before you respond, so I will stop
> spending time on this topic for now, and wait until others chime in.

Well, I lied ;-) 

I somehow doubt that users of vim types "!git jump diff" (or
whichever submode they want) from within vim's command prompt;
wouldn't they typically wrap the invocation in a vim macro?

If my suspicion is correct, with an opt-in feature like the above
(which is designed not to hurt existing users), the vim users can
change their macro definition to not just invoke "git jump
<whatever>", but invoke "GIT_JUMP_AUTO_CAT=yes git jump <whatever>",
i.e. tell "git jump" that you are opting into the "cat the file,
instead of launching GIT_EDITOR".  So with just a one-time setting,
vim (and other similar editor) users would benefit without hurting
others.

For that matter, instead of introducing GIT_JUMP_AUTO_CAT, the same
mechanism can be used to run "GIT_EDITOR=cat git jump <whatever>",
i.e. tell "git jump" that it is expected to run "cat" as its
editor, from such a vim macro ;-)

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 19:38                   ` Junio C Hamano
@ 2020-05-10 20:20                     ` George Brown
  2020-05-11 14:31                       ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: George Brown @ 2020-05-10 20:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, peff

> I somehow doubt that users of vim types "!git jump diff" (or
> whichever submode they want) from within vim's command prompt;
> wouldn't they typically wrap the invocation in a vim macro?

Correct, in Vim parlance we'd create a command for this.

> If my suspicion is correct, with an opt-in feature like the above
> (which is designed not to hurt existing users), the vim users can
> change their macro definition to not just invoke "git jump
> <whatever>", but invoke "GIT_JUMP_AUTO_CAT=yes git jump <whatever>",
> i.e. tell "git jump" that you are opting into the "cat the file,
> instead of launching GIT_EDITOR".  So with just a one-time setting,
> vim (and other similar editor) users would benefit without hurting
> others.
>
> For that matter, instead of introducing GIT_JUMP_AUTO_CAT, the same
> mechanism can be used to run "GIT_EDITOR=cat git jump <whatever>",
> i.e. tell "git jump" that it is expected to run "cat" as its
> editor, from such a vim macro ;-)

Yes. Another version that someone else implemented used similar method
by unsetting "$GIT_EDITOR" when invoked from Vim and modified "git jump"
to use cat when "$GIT_EDITOR" was empty.

https://gist.github.com/romainl/a3ddb1d08764b93183260f8cdf0f524f/e1f548f6d96cd6ee97c3daadb4a1546fab7814ad

I can request the author submit that as a patch if it is of interest.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
@ 2020-05-10 20:26 Benjamin
  2020-05-11 14:33 ` Junio C Hamano
  0 siblings, 1 reply; 19+ messages in thread
From: Benjamin @ 2020-05-10 20:26 UTC (permalink / raw)
  To: gitster; +Cc: 321.george, git, peff

Junio writes:

> I somehow doubt that users of vim types "!git jump diff" (or
> whichever submode they want) from within vim's command prompt;
> wouldn't they typically wrap the invocation in a vim macro?

Vim-user here. I run "git jump (options)" from a shell quite a bit, but when I'm
in vim I tend to use ":Ggrep" and similar commands from the fugitive plugin [1].
I don't *think* I'm alone in this.

I'm not really arguing either side, just pointing out that other workflows exist
(and indeed, I'm unlikely to run ":!git jump diff").

[1]: https://github.com/tpope/vim-fugitive

D. Ben Knoble

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 20:20                     ` George Brown
@ 2020-05-11 14:31                       ` Junio C Hamano
  0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2020-05-11 14:31 UTC (permalink / raw)
  To: George Brown; +Cc: git, peff

George Brown <321.george@gmail.com> writes:

> Yes. Another version that someone else implemented used similar method
> by unsetting "$GIT_EDITOR" when invoked from Vim and modified "git jump"
> to use cat when "$GIT_EDITOR" was empty.

That's curious, because with the same approach, that someone else
could have gone one step further to set GIT_EDITOR to 'cat' when vim
invokes via a macro and then there is no need to modify git-jump at
all.  "Curious" because I would intuitively think that unsetting
GIT_EDITOR and setting it to a specific value, 'cat', would require
about the same amount of effort.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 18:34               ` George Brown
  2020-05-10 19:10                 ` Junio C Hamano
@ 2020-05-11 14:31                 ` Jeff King
  2020-05-11 15:36                   ` Junio C Hamano
  1 sibling, 1 reply; 19+ messages in thread
From: Jeff King @ 2020-05-11 14:31 UTC (permalink / raw)
  To: George Brown; +Cc: Junio C Hamano, git

On Sun, May 10, 2020 at 07:34:09PM +0100, George Brown wrote:

> Arguably the most interoperable way for "git jump" to work would be to
> output the formatted lines and do nothing else, leaving it to users to
> choose how to operate upon the output/invoke editors. Of course such
> a change would break the workflow of anyone who uses "git jump" today
> and isn't a valid option.

Looks like I missed an exciting thread, but let me add a few thoughts as
the author of git-jump:

 - I had no inkling that people would run it from within vim; I assumed
   they'd use more integrated tools like fugitive.vim. ;) I don't think
   it's wrong to do so, though.

 - The paragraph quoted above is the heart of the matter. The tool does
   two things: generate the list and open the editor. My thinking in
   combining them was that "generate the list" was pretty simple and
   just used existing tools anyway, but we have grown a _bit_ of a
   complexity over the years that might make it worth using.

   IMHO the right solution here is a command-line option to say "don't
   start an editor, just send output". Setting GIT_EDITOR=cat
   accomplishes the same thing, but it's much less obvious/discoverable.

 - I'm pretty sure git-jump does _not_ work with emacs or emacsclient.
   Somebody corresponded with me off-list about patches many years ago,
   but I can't remember what the hold-up was. If anybody is interested
   in pushing that forward, I'd be happy to dig it out my archive.

   However, it should work with gvim, and any isatty() check would
   potentially cause issues there. So I'd much prefer the caller say
   explicitly that they're not expecting the editor to start.

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.

-Peff

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-10 20:26 [PATCH] contrib/git-jump: cat output when not a terminal Benjamin
@ 2020-05-11 14:33 ` Junio C Hamano
  0 siblings, 0 replies; 19+ messages in thread
From: Junio C Hamano @ 2020-05-11 14:33 UTC (permalink / raw)
  To: Benjamin; +Cc: 321.george, git, peff

Benjamin <ben.knoble@gmail.com> writes:

> Junio writes:
>
>> I somehow doubt that users of vim types "!git jump diff" (or
>> whichever submode they want) from within vim's command prompt;
>> wouldn't they typically wrap the invocation in a vim macro?
>
> Vim-user here. I run "git jump (options)" from a shell quite a bit, but when I'm
> in vim I tend to use ":Ggrep" and similar commands from the fugitive plugin [1].
> I don't *think* I'm alone in this.

I can believe there are tons of users of the plugin.  My point still
stands---such a plugin can export GIT_EDITOR=cat when running "git
jump" and there is no need to change "git jump".

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-11 14:31                 ` Jeff King
@ 2020-05-11 15:36                   ` Junio C Hamano
  2020-05-11 15:42                     ` Jeff King
  0 siblings, 1 reply; 19+ messages in thread
From: Junio C Hamano @ 2020-05-11 15:36 UTC (permalink / raw)
  To: Jeff King; +Cc: George Brown, git

Jeff King <peff@peff.net> writes:

>  - I'm pretty sure git-jump does _not_ work with emacs or emacsclient.

;-)  Its output can be used in M-x find-grep, though, actually.

>    However, it should work with gvim, and any isatty() check would
>    potentially cause issues there. So I'd much prefer the caller say
>    explicitly that they're not expecting the editor to start.

Yes, that is exactly what I was worried about.

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


Another thing I was thinking about was a change like the attached.
Plugging it thru "git var" to allow "git var GIT_JUMP_EDITOR" may
allow vim users to set it to 'cat' while setting GIT_EDITOR to vim.

It still needs a fix to get rid of the uncondtional "-q" option from
open_editor() function, though.


 cache.h  |  2 +-
 editor.c | 33 ++++++++++++++++++++++-----------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/cache.h b/cache.h
index 0f0485ecfe..b3f7a6d6c4 100644
--- a/cache.h
+++ b/cache.h
@@ -1649,7 +1649,7 @@ const char *fmt_ident(const char *name, const char *email,
 const char *fmt_name(enum want_ident);
 const char *ident_default_name(void);
 const char *ident_default_email(void);
-const char *git_editor(void);
+const char *git_editor(const char *);
 const char *git_sequence_editor(void);
 const char *git_pager(int stdout_is_tty);
 int is_terminal_dumb(void);
diff --git a/editor.c b/editor.c
index 91989ee8a1..3d04824e9e 100644
--- a/editor.c
+++ b/editor.c
@@ -14,11 +14,29 @@ int is_terminal_dumb(void)
 	return !terminal || !strcmp(terminal, "dumb");
 }
 
-const char *git_editor(void)
+const char *git_editor(const char *program)
 {
-	const char *editor = getenv("GIT_EDITOR");
+	const char *editor = NULL;
 	int terminal_is_dumb = is_terminal_dumb();
 
+	if (program) {
+		char *varname;
+
+		varname = xstrfmt("GIT_%s_EDITOR", program);
+		editor = getenv(varname);
+		free(varname);
+
+		if (!editor) {
+			struct strbuf progname = STRBUF_INIT;
+			strbuf_addstr(&progname, program);
+			strbuf_tolower(&progname);
+			varname = xstrfmt("%s.editor", progname.buf);
+			git_config_get_string_const(varname, &editor);
+			free(varname);
+			strbuf_release(&progname);
+		}
+	}
+
 	if (!editor && editor_program)
 		editor = editor_program;
 	if (!editor && !terminal_is_dumb)
@@ -37,14 +55,7 @@ const char *git_editor(void)
 
 const char *git_sequence_editor(void)
 {
-	const char *editor = getenv("GIT_SEQUENCE_EDITOR");
-
-	if (!editor)
-		git_config_get_string_const("sequence.editor", &editor);
-	if (!editor)
-		editor = git_editor();
-
-	return editor;
+	return git_editor("SEQUENCE");
 }
 
 static int launch_specified_editor(const char *editor, const char *path,
@@ -118,7 +129,7 @@ static int launch_specified_editor(const char *editor, const char *path,
 
 int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
 {
-	return launch_specified_editor(git_editor(), path, buffer, env);
+	return launch_specified_editor(git_editor(NULL), path, buffer, env);
 }
 
 int launch_sequence_editor(const char *path, struct strbuf *buffer,

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH] contrib/git-jump: cat output when not a terminal
  2020-05-11 15:36                   ` Junio C Hamano
@ 2020-05-11 15:42                     ` Jeff King
  0 siblings, 0 replies; 19+ messages in thread
From: Jeff King @ 2020-05-11 15:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: George Brown, git

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. :)

> Another thing I was thinking about was a change like the attached.
> Plugging it thru "git var" to allow "git var GIT_JUMP_EDITOR" may
> allow vim users to set it to 'cat' while setting GIT_EDITOR to vim.

I wouldn't use it myself, but I don't have any objection to adding
support.

>  cache.h  |  2 +-
>  editor.c | 33 ++++++++++++++++++++++-----------

You'd presumably need to make git-var understand that GIT_JUMP_EDITOR
should fall back to GIT_EDITOR.

-Peff

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-05-11 15:42 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-10 20:26 [PATCH] contrib/git-jump: cat output when not a terminal Benjamin
2020-05-11 14:33 ` Junio C Hamano
  -- strict thread matches above, loose matches on Subject: below --
2020-05-09 19:15 George Brown
2020-05-09 21:41 ` Junio C Hamano
2020-05-09 22:04   ` George Brown
2020-05-09 23:42     ` Junio C Hamano
2020-05-10  9:03       ` George Brown
2020-05-10 16:47         ` Junio C Hamano
2020-05-10 17:33           ` George Brown
2020-05-10 18:12             ` Junio C Hamano
2020-05-10 18:34               ` George Brown
2020-05-10 19:10                 ` Junio C Hamano
2020-05-10 19:25                   ` George Brown
2020-05-10 19:38                   ` Junio C Hamano
2020-05-10 20:20                     ` George Brown
2020-05-11 14:31                       ` Junio C Hamano
2020-05-11 14:31                 ` Jeff King
2020-05-11 15:36                   ` Junio C Hamano
2020-05-11 15:42                     ` Jeff King

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).