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

[Junio C Hamano <gitster@pobox.com>]

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.