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

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

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), but during a
single invocation, it primarily is used as a flag "are we showing
the editor invocation notice?" two places in the function can use
without having to do the same checks twice.

If we want this as an optimization "we've already checked the
condition in our previous call, so let's use the result without
checking again", then this has to become tristate:

 - We haven't checked, and needs checking (probably NULL);

 - We have checked, and we know we want to show the notice---here is
   a string to use when we clear the notice (what we have here);

 - We have checked, and we know we do not want to show the notice
   (there is no provision for this in the posted code---that makes
   this not an optimization yet).

Perhaps an empty string can be used for the last one, but if/when it
is done, the above needs to go in a comment near the definition of
the variable.

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

I agree that the verbose message Lars substituted makes it harder to
read.