git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: George Brown <321.george@gmail.com>
Cc: git@vger.kernel.org, peff@peff.net
Subject: Re: [PATCH] contrib/git-jump: cat output when not a terminal
Date: Sun, 10 May 2020 12:10:48 -0700	[thread overview]
Message-ID: <xmqqk11jtxl3.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CAFKec1VGzpxVJV4zak46r_p2gGcw4UanFr7U4U4MSsG7t2A23w@mail.gmail.com> (George Brown's message of "Sun, 10 May 2020 19:34:09 +0100")

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.


  reply	other threads:[~2020-05-10 19:11 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-09 19:15 [PATCH] contrib/git-jump: cat output when not a terminal 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 [this message]
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
2020-05-11 16:46                       ` Re* " Junio C Hamano
2020-05-12 19:23                         ` Jeff King
2020-05-12 21:30                           ` Junio C Hamano
2020-05-13  4:52                             ` Jeff King
  -- strict thread matches above, loose matches on Subject: below --
2020-05-10 20:26 Benjamin
2020-05-11 14:33 ` Junio C Hamano

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=xmqqk11jtxl3.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=321.george@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /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).