Re: [PATCH 3/8] Teach git var about GIT_EDITOR

[Jonathan Nieder <jrnieder@gmail.com>]

Johannes Sixt wrote:
> Jonathan Nieder schrieb:

>> From: Johannes Sixt <j.sixt@viscovery.net>
[...]
> Thanks for cleaning up behind me. I don't mind if you take
> authorship, but if you do keep my name, please make it:
> 
> From: Johannes Sixt <j6t@kdbg.org>

Thanks for the catch.

>>+		error(
>> 		  "No editor specified in GIT_EDITOR, core.editor, VISUAL,\n"
>> 		  "or EDITOR. Tried to fall back to vi but terminal is dumb.\n"
>> 		  "Please set one of these variables to an appropriate\n"
>> 		  "editor or run again with options that will not cause an\n"
>> 		  "editor to be invoked (e.g., -m or -F for git commit).");
>>+		return NULL;
>>+	}
> 
> I somehow dislike that this huge error message is in git_editor().

Makes sense.

I am a bit uncomfortable with this error in general.  It makes some
sense to refuse to use $VISUAL and fall back to $EDITOR if TERM=dumb,
since without this the distinction between VISUAL and EDITOR is not
very useful.  But wouldn’t that check be equally useful if GIT_EDITOR
or core.editor is set to vi?  Ideally, vi itself would check TERM and
error out, and git would only need to report and handle the exit.

Unfortunately, at least vim is happy to assume a terminal supports
ANSI sequences even if TERM=dumb (e.g., when running from a text
editor like Acme).  Unless VISUAL, GIT_EDITOR, and core.editor are
unset, nobody gets the benefit of this check.

Should git error out rather than run $VISUAL when TERM=dumb?  How
about $GIT_EDITOR?

The advice about options to avoid invoking an editor is not very
helpful except with 'git commit', so probably only 'git commit' should
print that message.

> The return value, NULL, should be indication enough for the callers
> to handle the situation suitable.

Good idea.

> In particular, launch_editor()
> wants to write this big warning, but 'git var -l' can avoid the
> error message and write only a short notice:
> 
> GIT_EDITOR=terminal is dumb, but VISUAL and EDITOR unset

Maybe 'git var -l' should omit GIT_EDITOR in this situation.

Thanks for the comments,
Jonathan