[PATCH v3] launch_editor(): indicate that Git waits for user input

[PATCH v3] launch_editor(): indicate that Git waits for user input

[lars.schneider]

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

When a graphical GIT_EDITOR is spawned by a Git command that opens
and waits for user input (e.g. "git rebase -i"), then the editor window
might be obscured by other windows. The user may be left staring at the
original Git terminal window without even realizing that s/he needs to
interact with another window before Git can proceed. To this user Git
appears hanging.

Show a message in the original terminal and get rid of it when the
editor returns.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---


Hi,

diff to v2:
    - shortened and localized the "waiting" message
    - detect "emacsclient" and suppress "waiting" message

I did not add a newline after the "waiting" message in case of an error.
Junio indicated [1] that it is not necessary with the shorter "waiting"
message.

Thanks,
Lars


[1] https://public-inbox.org/git/xmqqk1ygnpu7.fsf@gitster.mtv.corp.google.com/

Notes:
    Base Commit: 89ea799ffc (89ea799ffcc5c8a0547d3c9075eb979256ee95b8)
    Diff on Web: https://github.com/larsxschneider/git/commit/0d43814931
    Checkout:    git fetch https://github.com/larsxschneider/git editor-v3 && git checkout 0d43814931

 editor.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/editor.c b/editor.c
index 7519edecdc..4251ae9d7a 100644
--- a/editor.c
+++ b/editor.c
@@ -40,6 +40,35 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		const char *args[] = { editor, real_path(path), NULL };
 		struct child_process p = CHILD_PROCESS_INIT;
 		int ret, sig;
+		static const char *close_notice = NULL;
+
+		if (isatty(2) && !close_notice) {
+			char *term = getenv("TERM");
+
+			if (term && strcmp(term, "dumb"))
+				/*
+				 * go back to the beginning and erase the
+				 * entire line if the terminal is capable
+				 * to do so, to avoid wasting the vertical
+				 * space.
+				 */
+				close_notice = "\r\033[K";
+			else if (term && strstr(term, "emacsclient"))
+				/*
+				 * `emacsclient` (or `emacsclientw` on Windows) already prints
+				 * ("Waiting for Emacs...") if a file is opened for editing.
+				 * Therefore, we don't need to print the editor launch info.
+				 */
+				;
+			else
+				/* otherwise, complete and waste the line */
+				close_notice = _("done.\n");
+		}
+
+		if (close_notice) {
+			fprintf(stderr, _("Launched editor. Waiting for your input... "));
+			fflush(stderr);
+		}

 		p.argv = args;
 		p.env = env;
@@ -53,11 +82,14 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		sig = ret - 128;
 		sigchain_pop(SIGINT);
 		sigchain_pop(SIGQUIT);
+
 		if (sig == SIGINT || sig == SIGQUIT)
 			raise(sig);
 		if (ret)
 			return error("There was a problem with the editor '%s'.",
 					editor);
+		if (close_notice)
+			fputs(close_notice, stderr);
 	}

 	if (!buffer)
--
2.15.0

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Kaartic Sivaraam]

On Monday 27 November 2017 07:17 PM, lars.schneider@autodesk.com wrote:

> Show a message in the original terminal and get rid of it when the
> editor returns.
> 

"... except in the case when an error occurs." could  be included if needed.
> +		static const char *close_notice = NULL;
> +

IIRC, this variable is bound to be `static` for sake of future proofing. 
So, I guess you could use the following suggestion of Eric Sunshine in 
the below conditional.

     If you reverse this condition to say (!close_notice && isatty(2)),
     then you save an isatty() invocation each time if close_notice is
     already assigned.

> +		if (isatty(2) && !close_notice) {
> +			char *term = getenv("TERM");
> +
> +			if (term && strcmp(term, "dumb"))
> +				/*
> +				 * go back to the beginning and erase the
> +				 * entire line if the terminal is capable
> +				 * to do so, to avoid wasting the vertical
> +				 * space.
> +				 */
> +				close_notice = "\r\033[K";
> +			else if (term && strstr(term, "emacsclient"))
> +				/*
> +				 * `emacsclient` (or `emacsclientw` on Windows) already prints
> +				 * ("Waiting for Emacs...") if a file is opened for editing.
> +				 * Therefore, we don't need to print the editor launch info.
> +				 */
> +				;
> +			else
> +				/* otherwise, complete and waste the line */
> +				close_notice = _("done.\n");
> +		}
> +
> +		if (close_notice) {
> +			fprintf(stderr, _("Launched editor. Waiting for your input... "));
> +			fflush(stderr);
> +		}
> 
>   		p.argv = args;
>   		p.env = env;
> @@ -53,11 +82,14 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>   		sig = ret - 128;
>   		sigchain_pop(SIGINT);
>   		sigchain_pop(SIGQUIT);
> +
>   		if (sig == SIGINT || sig == SIGQUIT)
>   			raise(sig);
>   		if (ret)
>   			return error("There was a problem with the editor '%s'.",
>   					editor);
> +		if (close_notice)
> +			fputs(close_notice, stderr);
>   	}
> 
>   	if (!buffer)
> --
> 2.15.0
> 

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Eric Sunshine]

On Mon, Nov 27, 2017 at 8:47 AM,  <lars.schneider@autodesk.com> wrote:
> When a graphical GIT_EDITOR is spawned by a Git command that opens
> and waits for user input (e.g. "git rebase -i"), then the editor window
> might be obscured by other windows. The user may be left staring at the
> original Git terminal window without even realizing that s/he needs to
> interact with another window before Git can proceed. To this user Git
> appears hanging.
>
> Show a message in the original terminal and get rid of it when the
> editor returns.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
> diff --git a/editor.c b/editor.c
> @@ -40,6 +40,35 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
> +               static const char *close_notice = NULL;
> +
> +               if (isatty(2) && !close_notice) {
> +                       char *term = getenv("TERM");
> +
> +                       if (term && strcmp(term, "dumb"))
> +                               /*
> +                                * go back to the beginning and erase the
> +                                * entire line if the terminal is capable
> +                                * to do so, to avoid wasting the vertical
> +                                * space.
> +                                */
> +                               close_notice = "\r\033[K";
> +                       else if (term && strstr(term, "emacsclient"))

You need to check 'editor' here, not 'term', and you should do it
before the "not dumb" terminal check, otherwise you'll never get this
far.

> +                               /*
> +                                * `emacsclient` (or `emacsclientw` on Windows) already prints
> +                                * ("Waiting for Emacs...") if a file is opened for editing.
> +                                * Therefore, we don't need to print the editor launch info.
> +                                */
> +                               ;
> +                       else
> +                               /* otherwise, complete and waste the line */
> +                               close_notice = _("done.\n");
> +               }

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 27 Nov 2017, at 19:36, Eric Sunshine <sunshine@sunshineco.com> wrote:
> 
> On Mon, Nov 27, 2017 at 8:47 AM,  <lars.schneider@autodesk.com> wrote:
>> When a graphical GIT_EDITOR is spawned by a Git command that opens
>> and waits for user input (e.g. "git rebase -i"), then the editor window
>> might be obscured by other windows. The user may be left staring at the
>> original Git terminal window without even realizing that s/he needs to
>> interact with another window before Git can proceed. To this user Git
>> appears hanging.
>> 
>> Show a message in the original terminal and get rid of it when the
>> editor returns.
>> 
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
>> ---
>> diff --git a/editor.c b/editor.c
>> @@ -40,6 +40,35 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>> +               static const char *close_notice = NULL;
>> +
>> +               if (isatty(2) && !close_notice) {
>> +                       char *term = getenv("TERM");
>> +
>> +                       if (term && strcmp(term, "dumb"))
>> +                               /*
>> +                                * go back to the beginning and erase the
>> +                                * entire line if the terminal is capable
>> +                                * to do so, to avoid wasting the vertical
>> +                                * space.
>> +                                */
>> +                               close_notice = "\r\033[K";
>> +                       else if (term && strstr(term, "emacsclient"))
> 
> You need to check 'editor' here, not 'term', and you should do it
> before the "not dumb" terminal check, otherwise you'll never get this
> far.

Ouch. That happens if I try to do two things at once. Embarrassing.

Thanks,
Lars


> 
>> +                               /*
>> +                                * `emacsclient` (or `emacsclientw` on Windows) already prints
>> +                                * ("Waiting for Emacs...") if a file is opened for editing.
>> +                                * Therefore, we don't need to print the editor launch info.
>> +                                */
>> +                               ;
>> +                       else
>> +                               /* otherwise, complete and waste the line */
>> +                               close_notice = _("done.\n");
>> +               }

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Mon, Nov 27, 2017 at 02:47:16PM +0100, lars.schneider@autodesk.com wrote:
> From: Junio C Hamano <gitster@pobox.com>
> 
> When a graphical GIT_EDITOR is spawned by a Git command that opens
> and waits for user input (e.g. "git rebase -i"), then the editor window
> might be obscured by other windows. The user may be left staring at the
> original Git terminal window without even realizing that s/he needs to
> interact with another window before Git can proceed. To this user Git
> appears hanging.
> 
> Show a message in the original terminal and get rid of it when the
> editor returns.
> diff --git a/editor.c b/editor.c
> index 7519edecdc..4251ae9d7a 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -40,6 +40,35 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>  		const char *args[] = { editor, real_path(path), NULL };
>  		struct child_process p = CHILD_PROCESS_INIT;
>  		int ret, sig;
> +		static const char *close_notice = NULL;
> +
> +		if (isatty(2) && !close_notice) {

Sorry for coming to the topic so late, but it occurred to me that we
might want to conditionalize this on an advice.* flag.  I expect there
are some people who will never want to see this, and letting them turn
it off would be good.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Jeff King]

On Mon, Nov 27, 2017 at 08:09:32PM +0000, brian m. carlson wrote:

> > Show a message in the original terminal and get rid of it when the
> > editor returns.
> [...]
> 
> Sorry for coming to the topic so late, but it occurred to me that we
> might want to conditionalize this on an advice.* flag.  I expect there
> are some people who will never want to see this, and letting them turn
> it off would be good.

I am torn between saying "yes please, I would absolutely set such an
option myself" and "if we need advice.*, that is a good sign that the
feature is mis-designed".

Let me elaborate a bit on the latter.

My gut feeling is that this is absolutely the wrong place to put a
message like this. We don't know enough about what the editor is doing,
so we have to take pains to avoid a crufty message in the terminal,
including:

  - playing ANSI-term trickery to erase the message

  - hard-coding (!) emacsclient as a special case

And that's why I say that "advice.*" is a bad sign, because it means
those other techniques are failing, and somebody is seeing and being
annoyed by the cruft.

The right place for this message, IMHO, is for the editor itself (or a
wrapper script) to say "hey, I'm opening a new window" (like emacsclient
does).

But I also recognize that the world isn't perfect. Not all editors will
get this right, and not all users are savvy enough to set up a wrapper
script for editors which don't. So defaulting this to "on" tries to help
those cases.

If the anti-cruft techniques I mentioned above work well in practice,
then we get to have our cake and eat it, too. If they don't, then I'm
not sure if the tradeoff is worth it.

-Peff

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... "if we need advice.*, that is a good sign that the
> feature is mis-designed".
>
> Let me elaborate a bit on the latter.
>
> My gut feeling is that this is absolutely the wrong place to put a
> message like this....
> The right place for this message, IMHO, is for the editor itself (or a
> wrapper script) to say "hey, I'm opening a new window" (like emacsclient
> does).

Yes, I think we already had that discussion and if you recall those
involved in the thread except Lars were in favor of writing this off
as "here is a nickel--get a better editor".

One thing that may sway you slightly in favor of doing this in Git
is that a new user _might_ not be expecting Git to open an editor in
response to the command s/he just gave it [*1*].

It is one thing that the user _knowingly_ runs an editor and fails
to locate which window the editor opened.  It is a bit different if
the user didn't even know that s/he is supposed to be interacting
with an editor.

> But I also recognize that the world isn't perfect. Not all editors will
> get this right, and not all users are savvy enough to set up a wrapper
> script for editors which don't. So defaulting this to "on" tries to help
> those cases.
>
> If the anti-cruft techniques I mentioned above work well in practice,
> then we get to have our cake and eat it, too. If they don't, then I'm
> not sure if the tradeoff is worth it.

I agree with all of the above; I do not know if the "go back and
erase to the end of the line" is good enough in practice, and that
is why I sent "here is a possible anti-cruft" but did not push it
strongly forward myself.


[Footnote]

*1* "git merge topic" used to just complete the merge with canned
    log message and people were not all that unhappy--they just
    thought it is an OK design that a clean merge is a
    non-interactive operation.  Perhaps a person new to Git may be
    expecting that behaviour (without knowing anything like (1) that
    was what we used to do or (2) we open an editor by default these
    days), wondering why nothing seems to be happening, staring at
    an inactive window, after typing "git merge topic".

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 28 Nov 2017, at 00:05, Jeff King <peff@peff.net> wrote:
> 
> On Mon, Nov 27, 2017 at 08:09:32PM +0000, brian m. carlson wrote:
> 
>>> Show a message in the original terminal and get rid of it when the
>>> editor returns.
>> [...]
>> 
>> Sorry for coming to the topic so late, but it occurred to me that we
>> might want to conditionalize this on an advice.* flag.  I expect there
>> are some people who will never want to see this, and letting them turn
>> it off would be good.
> 
> I am torn between saying "yes please, I would absolutely set such an
> option myself" and "if we need advice.*, that is a good sign that the
> feature is mis-designed".
> 
> Let me elaborate a bit on the latter.
> 
> My gut feeling is that this is absolutely the wrong place to put a
> message like this. We don't know enough about what the editor is doing,
> so we have to take pains to avoid a crufty message in the terminal,
> including:
> 
>  - playing ANSI-term trickery to erase the message
> 
>  - hard-coding (!) emacsclient as a special case
> 
> And that's why I say that "advice.*" is a bad sign, because it means
> those other techniques are failing, and somebody is seeing and being
> annoyed by the cruft.

I agree with your cruft assessments, especially regarding the hard-coded 
emacsclient thingy. 

However, I really like Brian's "advice" suggestion. Would you be more
in favor of this change if we don't do emacsclient hardcoding and rely on 
"advice.openEditor"? Maybe we could also remove the term trickery but
this seems to be convenient in practice. 

According to the docs advice.* defaults to "true". For my use case it would
be ok if "advice.openEditor" would default to "false" as I distribute
a common Git config via "include.path" to my users. However, that is likely
confusing to the "advice" machinery and users.


> The right place for this message, IMHO, is for the editor itself (or a
> wrapper script) to say "hey, I'm opening a new window" (like emacsclient
> does).

I 100% agree. However, as you mentioned, the world isn't perfect.

Git's core concepts are pretty simple and most people understand them
if you explain them visually. However, applying/using these concepts
is often the problem. If you want to use Git efficiently then you need
to know lots of other things. Being command-line savvy is one of them.
From experience I know that this is hard for many people. Especially
Windows users as they are not used to BASH and Unix-like command-line
tools. That's why I think "advice.openEditor" could help a few people.

- Lars

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2863 bytes --]

On Mon, Nov 27, 2017 at 06:05:20PM -0500, Jeff King wrote:
> On Mon, Nov 27, 2017 at 08:09:32PM +0000, brian m. carlson wrote:
> 
> > > Show a message in the original terminal and get rid of it when the
> > > editor returns.
> > [...]
> > 
> > Sorry for coming to the topic so late, but it occurred to me that we
> > might want to conditionalize this on an advice.* flag.  I expect there
> > are some people who will never want to see this, and letting them turn
> > it off would be good.
> 
> I am torn between saying "yes please, I would absolutely set such an
> option myself" and "if we need advice.*, that is a good sign that the
> feature is mis-designed".

I myself would also set such an option.  More importantly, I think there
are other developers who would complain about such a message, and I'd
like to give them an easy way to turn it off.

Note that I am not altogether opposed to advice.*, since I personally
find it helpful in certain cases as an aide-mémoire of what state my
branch is in.

> Let me elaborate a bit on the latter.
> 
> My gut feeling is that this is absolutely the wrong place to put a
> message like this. We don't know enough about what the editor is doing,
> so we have to take pains to avoid a crufty message in the terminal,
> including:
> 
>   - playing ANSI-term trickery to erase the message
> 
>   - hard-coding (!) emacsclient as a special case

I agree that the editor is the right place to put this, but I also
understand that the people most likely to be helped by this are the
least likely to write such scripting.  I think this is especially so
because in my experience newer, less advanced users are more likely than
not to use graphical editors.

Git is an extraordinarily powerful piece of software, but it's also hard
for people to use (judging by my Twitter feed), so if we can make it
less painful for new users, I'm okay with an advice.* setting.

I'm slightly negative on hard-coding emacsclient, since I'm sure someone
will come up with another editor that does the same thing, and then we'd
have to update it.

> If the anti-cruft techniques I mentioned above work well in practice,
> then we get to have our cake and eat it, too. If they don't, then I'm
> not sure if the tradeoff is worth it.

I have a feeling that this may not work properly in editors that don't
support the concept of xterm alternate screens (such as the Linux
console) where the editor window is left on the screen, but it may be
that it works fine and I've just misunderstood how it's supposed to
work.  It may also be that we don't care about such cases, as any cruft
would have already scrolled off the screen.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Junio C Hamano]

lars.schneider@autodesk.com writes:

> diff to v2:
>     - shortened and localized the "waiting" message
>     - detect "emacsclient" and suppress "waiting" message

Thanks for moving this forward.


> +		static const char *close_notice = NULL;

Because this thing is "static", the variable is NULL when the first
call to this function is made, and the value left in the variable
when a call returns will be seen by the next call.

> +		if (isatty(2) && !close_notice) {

Declaring a "static" variable initialized to NULL and checking its
NULL-ness upfront is a common pattern to make sure that the code
avoids repeated computation of the same thing.  The body of the if
statement is run only when standard error stream is a tty (hinting
an interactive session) *and* close_notice is (still) NULL.

> +			char *term = getenv("TERM");
> +
> +			if (term && strcmp(term, "dumb"))
> +				/*
> +				 * go back to the beginning and erase the
> +				 * entire line if the terminal is capable
> +				 * to do so, to avoid wasting the vertical
> +				 * space.
> +				 */
> +				close_notice = "\r\033[K";
> +			else if (term && strstr(term, "emacsclient"))
> +				/*
> +				 * `emacsclient` (or `emacsclientw` on Windows) already prints
> +				 * ("Waiting for Emacs...") if a file is opened for editing.
> +				 * Therefore, we don't need to print the editor launch info.
> +				 */
> +				;
> +			else
> +				/* otherwise, complete and waste the line */
> +				close_notice = _("done.\n");
> +		}

It assigns a non-NULL value to close_notice unless the editor is
emacsclient (modulo the bug that "emacsclient" is to be compared
with EDITOR, GIT_EDITOR, core.editor etc. -- git_editor() can be
used to pick the right one).  For a user of that particular editor,
it is left as NULL.  Because it is unlikely that EDITOR etc. would
change across calls to this function, for them, and only for them,
the above is computed to yield the same result every time this
function is called.

That feels a bit uneven, doesn't it?

There are two possible solutions:

1. drop "static" from the declaration to stress the fact that the
   variable and !close_notice in the upfront if() statement is not
   avoiding repeated computation of the same thing, or

2. arrange that "emacsclient" case also participates in "avoid
   repeated computation" dance.  While at it, swap the order of
   checking isatty(2) and !close_notice (aka "have we done this
   already?)--because we do not expect us swapping file descriptor
   #2 inside this single process, we'd be repeatedly asking
   isatty(2) for the same answer.

The former may be simpler and easier, as an editor invocation would
not be a performance critical codepath.

If we want to do the latter, a cleaner way may be to have another
static "static int use_notice_checked = 0;" declared, and then

	if (!use_notice_checked && isatty(2)) {
		... what you currently have, modulo the
		... fix for the editor thing, and set
		... close_notice to a string (or NULL).
                use_notice_checked = 1;
	}

The users of close_notice after this part that use !close_notice
as "do not give the notice at all" flag and also as "this is the
string to show after editor comes back" can stay the same if you go
this route.  That would be solution 2a.

Of course, you can instead use close_notice = "" (empty string) as a
signal "we checked and we know that we are not using the notice
thing".  If you go that route, then the users after this if statement
that sets up close_notice upon the first call would say !*close_notice
instead of !close_notice when they try to see if the notice is in use.
That would be solution 2b.

I personally think any one of 1., 2a., or 2b. is fine.

Re: [PATCH v3] launch_editor(): indicate that Git waits for user input

[Lars Schneider]

> On 28 Nov 2017, at 00:18, Junio C Hamano <gitster@pobox.com> wrote:
> 
> lars.schneider@autodesk.com writes:
> 
>> diff to v2:
>>    - shortened and localized the "waiting" message
>>    - detect "emacsclient" and suppress "waiting" message
> 
> Thanks for moving this forward.
> 
> 
>> +		static const char *close_notice = NULL;
> 
> Because this thing is "static", the variable is NULL when the first
> call to this function is made, and the value left in the variable
> when a call returns will be seen by the next call.
> 
>> +		if (isatty(2) && !close_notice) {
> 
> Declaring a "static" variable initialized to NULL and checking its
> NULL-ness upfront is a common pattern to make sure that the code
> avoids repeated computation of the same thing.  The body of the if
> statement is run only when standard error stream is a tty (hinting
> an interactive session) *and* close_notice is (still) NULL.
> 
>> +			char *term = getenv("TERM");
>> +
>> +			if (term && strcmp(term, "dumb"))
>> +				/*
>> +				 * go back to the beginning and erase the
>> +				 * entire line if the terminal is capable
>> +				 * to do so, to avoid wasting the vertical
>> +				 * space.
>> +				 */
>> +				close_notice = "\r\033[K";
>> +			else if (term && strstr(term, "emacsclient"))
>> +				/*
>> +				 * `emacsclient` (or `emacsclientw` on Windows) already prints
>> +				 * ("Waiting for Emacs...") if a file is opened for editing.
>> +				 * Therefore, we don't need to print the editor launch info.
>> +				 */
>> +				;
>> +			else
>> +				/* otherwise, complete and waste the line */
>> +				close_notice = _("done.\n");
>> +		}
> 
> It assigns a non-NULL value to close_notice unless the editor is
> emacsclient (modulo the bug that "emacsclient" is to be compared
> with EDITOR, GIT_EDITOR, core.editor etc. -- git_editor() can be
> used to pick the right one).  For a user of that particular editor,
> it is left as NULL.  Because it is unlikely that EDITOR etc. would
> change across calls to this function, for them, and only for them,
> the above is computed to yield the same result every time this
> function is called.
> 
> That feels a bit uneven, doesn't it?
> 
> There are two possible solutions:
> 
> 1. drop "static" from the declaration to stress the fact that the
>   variable and !close_notice in the upfront if() statement is not
>   avoiding repeated computation of the same thing, or
> 
> 2. arrange that "emacsclient" case also participates in "avoid
>   repeated computation" dance.  While at it, swap the order of
>   checking isatty(2) and !close_notice (aka "have we done this
>   already?)--because we do not expect us swapping file descriptor
>   #2 inside this single process, we'd be repeatedly asking
>   isatty(2) for the same answer.
> 
> The former may be simpler and easier, as an editor invocation would
> not be a performance critical codepath.
> 
> If we want to do the latter, a cleaner way may be to have another
> static "static int use_notice_checked = 0;" declared, and then
> 
> 	if (!use_notice_checked && isatty(2)) {
> 		... what you currently have, modulo the
> 		... fix for the editor thing, and set
> 		... close_notice to a string (or NULL).
>                use_notice_checked = 1;
> 	}
> 
> The users of close_notice after this part that use !close_notice
> as "do not give the notice at all" flag and also as "this is the
> string to show after editor comes back" can stay the same if you go
> this route.  That would be solution 2a.
> 
> Of course, you can instead use close_notice = "" (empty string) as a
> signal "we checked and we know that we are not using the notice
> thing".  If you go that route, then the users after this if statement
> that sets up close_notice upon the first call would say !*close_notice
> instead of !close_notice when they try to see if the notice is in use.
> That would be solution 2b.
> 
> I personally think any one of 1., 2a., or 2b. is fine.

Thanks for your thoughts! I will go with 1. I also think that this is 
no performance critical codepath and therefore we should go with the
simplest/most maintainable solution.


Thanks,
Lars