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

lars.schneider@autodesk.com writes:

Thanks for moving this forward.

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.

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.

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.