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

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

On Saturday 18 November 2017 07:10 AM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
>>> @@ -40,6 +40,32 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>>> +               static const char *close_notice = NULL;
>>> +
>>> +               if (isatty(2) && !close_notice) {
>>
>> 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.
>>
>> However, it's not clear how much benefit you gain from stashing this
>> away in a static variable. Premature optimization?
> 
> The variable being "static" could be (but it was done primarily
> because it allowed me not to worry about freeing),

AFAIK, observing the way the variable 'close_notice' is used, I guess 
you don't need to worry about freeing it up even if it wasn't "static".
That's my interpretation of the following section of the C standard,

========================
6.5.2.5 Compound literals

...

9 EXAMPLE 2
In contrast, in
void f(void)
{
int *p;
/*...*/
p = (int [2]){*p};
/*...*/
}
p is assigned the address of the first element of an array of two ints, 
the first having the value previously pointed to by p and the second, 
zero. The expressions in this compound literal need not be constant. The
unnamed object has automatic storage duration.
=========================

So the unnamed objects created as a consequence of the string literals 
assigned to 'close_notice' should have "automatic" storage duration 
which means you don't have to worry about allocating memory for them 
which would make you worry about freeing it up. If I'm stating something 
wrong, let me know.

BTW, I find making the variable 'close_notice' to be 'static' unwanted 
as I couldn't find any piece of code that invokes 'launch_editor' more 
than once within a single run. What could be the possible cases in which 
'launch_editor' could be called twice ?


>> Should printing of close_notice be done before the error()? Otherwise,
>> you get this:
>>
>> --- 8< ---
>> Launched your editor (...) ...There was a problem...
>> --- 8< ---
> 
> In my version with a far shorter message, I deliberately chose not
> to clear the notice.  We ran the editor, and we saw a problem.  That
> is what happened and that is what will be left on the terminal.
> 

It might be a good thing to keep the notice but I think it would be 
better to have that error message in a "new line". I'm not sure if it's 
possible or not.


---
Kaartic