> On 28 Nov 2017, at 00:18, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> lars.schneider@xxxxxxxxxxxx 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.
Thanks for your thoughts! I will go with 1. I also think that this is
no performance critical codepath and therefore we should go with the
simplest/most maintainable solution.
Thanks,
Lars
