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

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Nov 19, 2017, 17:49 UTC
Message-ID: <d53a655b-f51e-3f44-23db-581071010fd2@gmail.com>
In-Reply-To: <xmqqh8tsqs83.fsf@gitster.mtv.corp.google.com>

On Saturday 18 November 2017 07:10 AM, Junio C Hamano wrote:

Show 16 quoted lines

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

Show 11 quoted lines

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