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

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