Interpreting EDITOR/VISUAL environment variables.

Interpreting EDITOR/VISUAL environment variables.

[David Kastrup]

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

Re: Interpreting EDITOR/VISUAL environment variables.

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

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

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

Re: Interpreting EDITOR/VISUAL environment variables.

[Yann Dirson]

On Wed, Aug 01, 2007 at 10:12:13AM -0700, Junio C Hamano wrote:
> 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).

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

Re: Interpreting EDITOR/VISUAL environment variables.

[David Kastrup]

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

> David Kastrup <dak@gnu.org> writes:
>
>> 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?
>
> 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).

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, Kriemhildstr. 15, 44793 Bochum

Re: Interpreting EDITOR/VISUAL environment variables.

[David Kastrup]

Yann Dirson <ydirson@altern.org> writes:

> On Wed, Aug 01, 2007 at 10:12:13AM -0700, Junio C Hamano wrote:
>> 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).
>
> And, indeed, --alternate-editor could be supplemented by another
> envvar to be able to work in our situation.

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?

> Maybe the various emacsen vendors would be willing to integrate such
> a patch ?

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?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Interpreting EDITOR/VISUAL environment variables.

[Junio C Hamano]

David Kastrup <dak@gnu.org> 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.
> ...
> 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 '\''.

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.

[PATCH] git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR accept commands

[David Kastrup]

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(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 3c0367d..3c50bc1 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -29,7 +29,8 @@ set_reflog_action() {
 }
 
 git_editor() {
-	GIT_EDITOR=${GIT_EDITOR:-$(git config core.editor || echo ${VISUAL:-${EDITOR}})}
+	: "${GIT_EDITOR:=$(git config core.editor)}"
+	: "${GIT_EDITOR:=${VISUAL:-${EDITOR}}}"
 	case "$GIT_EDITOR,$TERM" in
 	,dumb)
 		echo >&2 "No editor specified in GIT_EDITOR, core.editor, VISUAL,"
@@ -40,7 +41,7 @@ git_editor() {
 		exit 1
 		;;
 	esac
-	${GIT_EDITOR:-vi} "$1"
+	eval "${GIT_EDITOR:=vi}" '"$@"'
 }
 
 is_bare_repository () {
-- 
1.5.3.rc2.86.gdc7ba

[PATCH] git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR accept commands

[David Kastrup]

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(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c51985e..3c50bc1 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -29,7 +29,8 @@ set_reflog_action() {
 }
 
 git_editor() {
-	GIT_EDITOR=${GIT_EDITOR:-$(git config core.editor || echo ${VISUAL:-${EDITOR}})}
+	: "${GIT_EDITOR:=$(git config core.editor)}"
+	: "${GIT_EDITOR:=${VISUAL:-${EDITOR}}}"
 	case "$GIT_EDITOR,$TERM" in
 	,dumb)
 		echo >&2 "No editor specified in GIT_EDITOR, core.editor, VISUAL,"
@@ -40,7 +41,7 @@ git_editor() {
 		exit 1
 		;;
 	esac
-	"${GIT_EDITOR:-vi}" "$1"
+	eval "${GIT_EDITOR:=vi}" '"$@"'
 }
 
 is_bare_repository () {
-- 
1.5.3.rc2.86.gdc7ba

Re: [PATCH] git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR accept commands

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

> The previous code only allowed specifying a single executable rather
> than a complete command like "emacsclient --alternate-editor vi" in

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.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR accept commands

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> David Kastrup <dak@gnu.org> writes:
>
>> The previous code only allowed specifying a single executable rather
>> than a complete command like "emacsclient --alternate-editor vi" in
>
> 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.

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?

Re: [PATCH] git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR accept commands

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> 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?

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

Re: [PATCH] git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR accept commands

[David Kastrup]

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

> David Kastrup <dak@gnu.org> writes:
>
>> David Kastrup <dak@gnu.org> writes:
>>
>>> The previous code only allowed specifying a single executable rather
>>> than a complete command like "emacsclient --alternate-editor vi" in
>>
>> 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.
>
> 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?

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, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR accept commands

[David Kastrup]

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

> Junio C Hamano <gitster@pobox.com> 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?
>
> Ah, I spoke too fast.  It is fine --- the shell that actually is
> doing the eval then interpolates "$@".  Clever.

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, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] git-sh-setup.sh: make GIT_EDITOR/core.editor/VISUAL/EDITOR accept commands

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> 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?
>>
>> Ah, I spoke too fast.  It is fine --- the shell that actually is
>> doing the eval then interpolates "$@".  Clever.
>
> 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.

Yup.  I just misread the code.

Thanks, applied.

Re: Interpreting EDITOR/VISUAL environment variables.

[Matthias Lederhofer]

David Kastrup <dak@gnu.org> wrote:
> 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.

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"))

Re: Interpreting EDITOR/VISUAL environment variables.

[David Kastrup]

Matthias Lederhofer <matled@gmx.net> writes:

> David Kastrup <dak@gnu.org> wrote:
>> 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.

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.

> 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"))

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

-- 
David Kastrup