Interpreting EDITOR/VISUAL environment variables.

Whatever I have been able to Google, is completely silent on this matter. If anybody has an idea where to find authoritative information, holler.

In the meantime, in the Emacs manual there is the following bit of information:

(info "(emacs) Invoking Emacsclient")

       The option `-a COMMAND' or `--alternate-editor=COMMAND' specifies a
    command to run if `emacsclient' fails to contact Emacs.  This is useful
    when running `emacsclient' in a script.  For example, the following
    setting for the `EDITOR' environment variable will always give you an
    editor, even if no Emacs server is running:

         EDITOR="emacsclient --alternate-editor emacs +%d %s"

That makes it likely that the way to call an editor should be via system. However, there are certainly programs around which will not interpret the +%d and %s thingies. My current setting is

         EDITOR="emacsclient --alternate-editor vi"

and this seems to do the trick with most applications. Not so with git-commit and other git scripts. The easiest way out will be to create something like ~/bin/myemacsclient which does the respective argument splicing. I am just not sure this is the "canonically correct way" of interpreting $EDITOR.

Actually, splicing $EDITOR into a system command is a nuisance because it means having to shell-quote its arguments. So the current interpretation is likely easier to maintain.

Is it the correct one?

David Kastrup <dak@gnu.org> writes:

I've been torn on this one. From the point of view of "specified behaviour in the documentation", which is "EDITOR and VISUAL name the editor of your choice", not splicing is not violating the letter (I am not talking about our documentation here, but many other programs'). Splicing and shell quoting other parameters, while it is technically not a problem at all doing that in scripts, feels "dirty". Maybe it's just me.

Both cvs and svn seems to splice, I suspect they just do a straight system(3) invocation.

We recently normalized the script callers not to splice at all (the scripts were hand-rolling "the VISUAL or EDITOR or vi" and slightly differently). It obviously has negative (i.e. setting EDITOR to "emacsclient --alternate-editor vi" does not work) as well as positive side (i.e. "/home/dak/My Programs/editor" would work).

On Wed, Aug 01, 2007 at 10:12:13AM -0700, Junio C Hamano wrote:

And, indeed, --alternate-editor could be supplemented by another envvar to be able to work in our situation. Maybe the various emacsen vendors would be willing to integrate such a patch ?

Best regards,

Yann Dirson <ydirson@altern.org> writes:

It is already. But if git is pretty much alone in breaking a setup that is working everywhere else, is having a workaround available really a good excuse for not doing the right thing?

Actually, it is a nuisance because nobody remembers this variable. It is called (looking in the Emacs manual, using the index to find emacsclient, following a link after two pages to the invocation, going down two pages again) ALTERNATE_EDITOR. It does not even _mention_ Emacs or emacsclient in its name. The "-a" option is easier to remember.

So yes, emacsclient has an environment hook making it possible to work around git's idiosyncratic behavior here. But should it really be necessary?

Junio C Hamano <gitster@pobox.com> writes:

Well, I just checked the behavior with "less", "more", "mail" and "mailx", quite traditional commands that would not have a reason to complicate things by using "system" and quoting instead of exec.

less, mail and mailx apparently go via system, more (wtf?!?) apparently via exec.

Taken together with the behavior by cvs and svn, I think we should not just throw EDITOR/VISUAL into one exec arg.

Then there are two implementations to pick from:

a) Using system and shell-quoting the filename.  Advantage: one can
set EDITOR='"/home/dak/My Programs/editor"' and have it work.
Disadvantage: shell-quoting a file name seems shell- and
system-dependent.

It turns out all three contestants still in the race apparently do a). If nobody has a sensible idea how to shell-quote generally enough... Under Unix, one has the option of using "..." and quoting the three of "$\ with \ or using '...' and replacing every contained ' with '\''. I don't think that there is a library function generally available. The " quote type would probably be more typical.

It turns out that gitk and gitk-wish _already_ do this. So the normalization does not seem to have covered them if I read the code correctly:

proc shellquote {str} {
    if {![string match "*\['\"\\ \t]*" $str]} {
        return $str
    }
    if {![string match "*\['\"\\]*" $str]} {
        return "\"$str\""
    }
    if {![string match "*'*" $str]} {
        return "'$str'"
    }
    return "\"[string map {\" \\\" \\ \\\\} $str]\""
}

Note that the first case does not cover strings with newlines in them, though, and the second does not help with dollar signs. And I have no clue what the last return does. Presumably maps " to \" and \ to \\ inside of double quotes.

b) splitting EDITOR/VISUAL at spaces and using exec. Nobody else appears to do this, so may neither should be.

It appears that the C code already has quote.c, so it is probably more or less doable.

David Kastrup <dak@gnu.org> writes:

Our scripts and C-level both tend to prefer using '\'' for its simplicity (this also applies to rare case the user wants to unquote it by hand). Because it now is all in git-sh-setup, it should be reasonably straightforward to implement the quoting of the temporary file name and drop dq around ${EDITOR-VISUAL}.

Please make it so.

The previous code only allowed specifying a single executable rather than a complete command like "emacsclient --alternate-editor vi" in those variables. Since VISUAL/EDITOR appear to be traditionally passed to a shell for interpretation (as corroborated with "less", "mail" and "mailx", while the really ancient "more" indeed allows only an executable name), the shell function git_editor has been amended appropriately.

"eval" is employed to have quotes and similar interpreted _after_ expansion, so that specifying EDITOR='"/home/dak/My Commands/notepad.exe"' can be used for actually using commands with blanks.

Instead of passing just the first argument of git_editor on, we pass all of them (so that +lineno might be employed at a later point of time, or so that multiple files may be edited when appropriate).

Strictly speaking, there is a change in behavior: when git config core.editor returns a valid but empty string, the fallbacks are still searched. This is more consistent, and the old code was problematic with regard to multiple blanks. Putting in additional quotes might have worked, but quotes inside of command substitution inside of quotes is nasty enough to not reliably work the same across "Bourne shells".

Signed-off-by: David Kastrup <dak@gnu.org>
---
 git-sh-setup.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

David Kastrup <dak@gnu.org> writes:

Oops, won't apply cleanly. I found that I had already made a different (trivial) patch previously. Let me try again and fold that patch in manually.

The previous code only allowed specifying a single executable rather than a complete command like "emacsclient --alternate-editor vi" in those variables. Since VISUAL/EDITOR appear to be traditionally passed to a shell for interpretation (as corroborated with "less", "mail" and "mailx", while the really ancient "more" indeed allows only an executable name), the shell function git_editor has been amended appropriately.

"eval" is employed to have quotes and similar interpreted _after_ expansion, so that specifying EDITOR='"/home/dak/My Commands/notepad.exe"' can be used for actually using commands with blanks.

Instead of passing just the first argument of git_editor on, we pass all of them (so that +lineno might be employed at a later point of time, or so that multiple files may be edited when appropriate).

Strictly speaking, there is a change in behavior: when git config core.editor returns a valid but empty string, the fallbacks are still searched. This is more consistent, and the old code was problematic with regard to multiple blanks. Putting in additional quotes might have worked, but quotes inside of command substitution inside of quotes is nasty enough to not reliably work the same across "Bourne shells".

Signed-off-by: David Kastrup <dak@gnu.org>
---
 git-sh-setup.sh |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

David Kastrup <dak@gnu.org> writes:

It is not just "won't apply". What if GIT_DIR had spaces (which is fine) and single-quotes in it? Wouldn't it percolate down to $@ because it becomes the leading directory of the temporary file name? And you quote '"$@"' and eval it, now what happens?

Junio C Hamano <gitster@pobox.com> writes:

Ah, I spoke too fast. It is fine --- the shell that actually is doing the eval then interpolates "$@". Clever.

Junio C Hamano <gitster@pobox.com> writes:

Since eval folds all its arguments into a single string separated by single blanks, actual blanks (which can be multiple or interspersed with newlines) must not yet be seen. The string '"$@"' contains no blanks and thus gets through unmolested.

David Kastrup <dak@gnu.org> writes:

Yup.  I just misread the code.

Thanks, applied.

Junio C Hamano <gitster@pobox.com> writes:

The eval removes the outer single quotes and then evaluates the remaining "$@" which leaves the original argument structure completely intact: What got passed into git_editor as 3 arguments will remain as 3 arguments, and even multiple embedded spaces in one argument will get preserved perfectly.

If you don't believe me, throw whatever you want at it.

David Kastrup <dak@gnu.org> wrote:

What about this instead of quoting the argument?

    sh -c '$EDITOR "$1" "$2"' editor +5 /path/to/file

(i.e. for C execvp("/bin/sh", "-c", "$EDITOR \"$1\" \"$2\"", "editor",
    "+5", "/path/to/file"))

Matthias Lederhofer <matled@gmx.net> writes:

Actually I was talking C here, and the editor is never called from C in git but rather from the shell. So this problem is a non-problem for us.

It suffers from the fault that it does not work as far as I can see. -c does not set the positional parameters.