packaging vs default pager

packaging vs default pager

[Ben Walton]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]


Hi All,

I'd like to see what people think about providing a configure/Makefile
knob for overriding the default pager at build time.  Currently,
things use 'less' as the fallback and rely on the path to find
it.

On (old) solaris systems, /usr/bin/less (typically the first less
found) doesn't understand the default arguments (FXRS), which forces
users to alter their environment (PATH, GIT_PAGER, LESS, etc) or have
a local or global gitconfig before paging works as expected.

Would it be completely out of line to provide a knob so that the
fallback $pager could be set to something more specific/appropriate
during the build?  [I'll do the work but not if it's an undesirable
addition.]

Alternately, are packagers recommended to simply ship a global
gitconfig that sets core.pager?

Thanks
-Ben
-- 
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302

GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: packaging vs default pager

[Junio C Hamano]

Ben Walton <bwalton@artsci.utoronto.ca> writes:

> On (old) solaris systems, /usr/bin/less (typically the first less
> found) doesn't understand the default arguments (FXRS), which forces
> users to alter their environment (PATH, GIT_PAGER, LESS, etc) or have
> a local or global gitconfig before paging works as expected.
>
> Would it be completely out of line to provide a knob so that the
> fallback $pager could be set to something more specific/appropriate
> during the build?

I think that is a sensible thing to do.  Something like this?

 Makefile |    6 ++++++
 pager.c  |    6 +++++-
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 268aede..342d49a 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,9 @@ all::
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
+#
+# Define DEFAULT_PAGER to the path of a sensible pager (defaults to "less") if
+# you want to use something different.
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1294,6 +1297,9 @@ ifdef NO_REGEX
 	COMPAT_CFLAGS += -Icompat/regex
 	COMPAT_OBJS += compat/regex/regex.o
 endif
+ifdef DEFAULT_PAGER
+	BASIC_CFLAGS += -DDEFAULT_PAGER='"$(DEFAULT_PAGER)"'
+endif
 
 ifdef USE_NED_ALLOCATOR
        COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -DOVERRIDE_STRDUP -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR -Icompat/nedmalloc
diff --git a/pager.c b/pager.c
index 86facec..f4c992d 100644
--- a/pager.c
+++ b/pager.c
@@ -2,6 +2,10 @@
 #include "run-command.h"
 #include "sigchain.h"
 
+#ifndef DEFAULT_PAGER
+#define DEFAULT_PAGER "less"
+#endif
+
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -58,7 +62,7 @@ void setup_pager(void)
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!pager)
-		pager = "less";
+		pager = DEFAULT_PAGER;
 	else if (!*pager || !strcmp(pager, "cat"))
 		return;
 

[PATCH 0/2] Re: packaging vs default pager

[Jonathan Nieder]

Junio C Hamano wrote:
> Ben Walton <bwalton@artsci.utoronto.ca> writes:

>> Would it be completely out of line to provide a knob so that the
>> fallback $pager could be set to something more specific/appropriate
>> during the build?
> 
> I think that is a sensible thing to do.  Something like this?
> 
>  Makefile |    6 ++++++
>  pager.c  |    6 +++++-
>  2 files changed, 11 insertions(+), 1 deletions(-)

That looks good to me, but it’s missing a corresponding change to
git-svn.  I think something like this could be useful for Debian to
avoid patching to comply with the distro policy of falling back to
generic "pager" and "editor" symlinks.  How about the following two
patches?

Jonathan Nieder (1):
  Provide a build time default-editor setting

Junio C Hamano (1):
  Provide a build time default-pager setting

 Makefile                  |   18 ++++++++++++++++++
 editor.c                  |    2 +-
 git-add--interactive.perl |    3 ++-
 git-sh-setup.sh           |    6 ++++--
 git-svn.perl              |    8 +++++---
 pager.c                   |    2 +-
 t/Makefile                |    2 ++
 t/t7005-editor.sh         |   29 ++++++++++++++++++++++-------
 8 files changed, 55 insertions(+), 15 deletions(-)

[PATCH 1/2] Provide a build time default-pager setting

[Jonathan Nieder]

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

On (old) solaris systems, /usr/bin/less (typically the first less
found) doesn't understand the default arguments (FXRS), which
forces users to alter their environment (PATH, GIT_PAGER, LESS,
etc) or have a local or global gitconfig before paging works as
expected.

On Debian systems, by policy packages must fall back to the
'pager' command, so that changing the target of the
/usr/bin/pager symlink changes the default pager for all packages
at once.

Provide a DEFAULT_PAGER knob so packagers can set the fallback
pager to something appropriate during the build.

This puts the "less" default in the Makefile instead of pager.c,
since it is needed for git-svn, too.  This means that the
DEFAULT_PAGER preprocessor token _has_ to be defined on the
command line for git to build.  I was worried about this for a
moment, but GIT_VERSION already works this way without trouble.

Probably the DEFAULT_PAGER setting should be added to something
like TRACK_CFLAGS as well.  Actually, some other settings that
can change without forcing files to be rebuilt (e.g. SHELL_PATH),
too.  This should be probably be addressed separately.

Reported-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile     |    8 ++++++++
 git-svn.perl |    5 +++--
 pager.c      |    2 +-
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 268aede..fc1a461 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,9 @@ all::
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
+#
+# Define DEFAULT_PAGER to the path of a sensible pager (defaults to "less") if
+# you want to use something different.
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1294,6 +1297,10 @@ ifdef NO_REGEX
 	COMPAT_CFLAGS += -Icompat/regex
 	COMPAT_OBJS += compat/regex/regex.o
 endif
+ifndef DEFAULT_PAGER
+	DEFAULT_PAGER = less
+endif
+BASIC_CFLAGS += -DDEFAULT_PAGER='"$(DEFAULT_PAGER)"'
 
 ifdef USE_NED_ALLOCATOR
        COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -DOVERRIDE_STRDUP -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR -Icompat/nedmalloc
@@ -1451,6 +1458,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	    -e '}' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+	    -e 's/@@DEFAULT_PAGER@@/$(DEFAULT_PAGER)/g' \
 	    $@.perl >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
diff --git a/git-svn.perl b/git-svn.perl
index 6a3b501..c270b23 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3,11 +3,12 @@
 # License: GPL v2 or later
 use warnings;
 use strict;
-use vars qw/	$AUTHOR $VERSION
+use vars qw/	$AUTHOR $VERSION $DEFAULT_PAGER
 		$sha1 $sha1_short $_revision $_repository
 		$_q $_authors $_authors_prog %users/;
 $AUTHOR = 'Eric Wong <normalperson@yhbt.net>';
 $VERSION = '@@GIT_VERSION@@';
+$DEFAULT_PAGER = '@@DEFAULT_PAGER@@';
 
 # From which subdir have we been invoked?
 my $cmd_dir_prefix = eval {
@@ -5174,7 +5175,7 @@ sub git_svn_log_cmd {
 sub config_pager {
 	$pager ||= $ENV{GIT_PAGER} || $ENV{PAGER};
 	if (!defined $pager) {
-		$pager = 'less';
+		$pager = $DEFAULT_PAGER;
 	} elsif (length $pager == 0 || $pager eq 'cat') {
 		$pager = undef;
 	}
diff --git a/pager.c b/pager.c
index 86facec..416a796 100644
--- a/pager.c
+++ b/pager.c
@@ -58,7 +58,7 @@ void setup_pager(void)
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!pager)
-		pager = "less";
+		pager = DEFAULT_PAGER;
 	else if (!*pager || !strcmp(pager, "cat"))
 		return;
 
-- 
1.6.5.2

[PATCH/RFC 2/2] Provide a build time default-editor setting

[Jonathan Nieder]

Provide a DEFAULT_EDITOR knob to allow the fallback editor (to
use instead of vi if VISUAL, EDITOR, and GIT_EDITOR are unset) to
be set at build time according to a system’s policy.  For
example, on Debian systems, the default editor should be the
'editor' command.

The contrib/fast-import/git-p4 script still uses vi, since it is
not modified by the Makefile currently, and making it require
build-time modification would create too much trouble for people
deploying that script.

This change makes t7005-editor into a mess.  Any ideas for fixing
this?

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile                  |   10 ++++++++++
 editor.c                  |    2 +-
 git-add--interactive.perl |    3 ++-
 git-sh-setup.sh           |    6 ++++--
 git-svn.perl              |    5 +++--
 t/Makefile                |    2 ++
 t/t7005-editor.sh         |   29 ++++++++++++++++++++++-------
 7 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index fc1a461..fae8647 100644
--- a/Makefile
+++ b/Makefile
@@ -203,6 +203,9 @@ all::
 #
 # Define DEFAULT_PAGER to the path of a sensible pager (defaults to "less") if
 # you want to use something different.
+#
+# Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
+# want to use something different.
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1301,6 +1304,11 @@ ifndef DEFAULT_PAGER
 	DEFAULT_PAGER = less
 endif
 BASIC_CFLAGS += -DDEFAULT_PAGER='"$(DEFAULT_PAGER)"'
+ifndef DEFAULT_EDITOR
+	DEFAULT_EDITOR = vi
+endif
+export DEFAULT_EDITOR
+BASIC_CFLAGS += -DDEFAULT_EDITOR='"$(DEFAULT_EDITOR)"'
 
 ifdef USE_NED_ALLOCATOR
        COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -DOVERRIDE_STRDUP -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR -Icompat/nedmalloc
@@ -1435,6 +1443,7 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 	    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
+	    -e 's|DEFAULT_EDITOR:=vi|DEFAULT_EDITOR:=$(DEFAULT_EDITOR)|' \
 	    -e $(BROKEN_PATH_FIX) \
 	    $@.sh >$@+ && \
 	chmod +x $@+ && \
@@ -1459,6 +1468,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    -e 's/@@DEFAULT_PAGER@@/$(DEFAULT_PAGER)/g' \
+	    -e 's/@@DEFAULT_EDITOR@@/$(DEFAULT_EDITOR)/g' \
 	    $@.perl >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
diff --git a/editor.c b/editor.c
index 4d469d0..93b8cbb 100644
--- a/editor.c
+++ b/editor.c
@@ -19,7 +19,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
 
 	if (!editor)
-		editor = "vi";
+		editor = DEFAULT_EDITOR;
 
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 69aeaf0..c3d932c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1,6 +1,7 @@
 #!/usr/bin/perl -w
 
 use strict;
+use constant DEFAULT_EDITOR => '@@DEFAULT_EDITOR@@';
 use Git;
 
 binmode(STDOUT, ":raw");
@@ -988,7 +989,7 @@ EOF
 	close $fh;
 
 	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
-		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+		|| $ENV{VISUAL} || $ENV{EDITOR} || DEFAULT_EDITOR;
 	system('sh', '-c', $editor.' "$@"', $editor, $hunkfile);
 
 	if ($? != 0) {
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c41c2f7..d053d56 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -99,19 +99,21 @@ set_reflog_action() {
 }
 
 git_editor() {
+	: "${DEFAULT_EDITOR:=vi}"
 	: "${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,"
-		echo >&2 "or EDITOR. Tried to fall back to vi but terminal is dumb."
+		echo >&2 "or EDITOR. Tried to fall back to $DEFAULT_EDITOR" \
+			"but terminal is dumb."
 		echo >&2 "Please set one of these variables to an appropriate"
 		echo >&2 "editor or run $0 with options that will not cause an"
 		echo >&2 "editor to be invoked (e.g., -m or -F for git-commit)."
 		exit 1
 		;;
 	esac
-	eval "${GIT_EDITOR:=vi}" '"$@"'
+	eval "${GIT_EDITOR:=$DEFAULT_EDITOR}" '"$@"'
 }
 
 is_bare_repository () {
diff --git a/git-svn.perl b/git-svn.perl
index c270b23..b98d378 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3,12 +3,13 @@
 # License: GPL v2 or later
 use warnings;
 use strict;
-use vars qw/	$AUTHOR $VERSION $DEFAULT_PAGER
+use vars qw/	$AUTHOR $VERSION $DEFAULT_PAGER $DEFAULT_EDITOR
 		$sha1 $sha1_short $_revision $_repository
 		$_q $_authors $_authors_prog %users/;
 $AUTHOR = 'Eric Wong <normalperson@yhbt.net>';
 $VERSION = '@@GIT_VERSION@@';
 $DEFAULT_PAGER = '@@DEFAULT_PAGER@@';
+$DEFAULT_EDITOR = '@@DEFAULT_EDITOR@@';
 
 # From which subdir have we been invoked?
 my $cmd_dir_prefix = eval {
@@ -1322,7 +1323,7 @@ sub get_commit_entry {
 	close $log_fh or croak $!;
 
 	if ($_edit || ($type eq 'tree')) {
-		my $editor = $ENV{VISUAL} || $ENV{EDITOR} || 'vi';
+		my $editor = $ENV{VISUAL} || $ENV{EDITOR} || $DEFAULT_EDITOR;
 		# TODO: strip out spaces, comments, like git-commit.sh
 		system($editor, $commit_editmsg);
 	}
diff --git a/t/Makefile b/t/Makefile
index bd09390..9174bbb 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -9,6 +9,8 @@
 SHELL_PATH ?= $(SHELL)
 TAR ?= $(TAR)
 RM ?= rm -f
+DEFAULT_EDITOR ?= vi
+export DEFAULT_EDITOR
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b647957..2b76f72 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -4,7 +4,18 @@ test_description='GIT_EDITOR, core.editor, and stuff'
 
 . ./test-lib.sh
 
-for i in GIT_EDITOR core_editor EDITOR VISUAL vi
+: ${DEFAULT_EDITOR=vi}
+
+unset EDITOR VISUAL GIT_EDITOR
+
+case "$DEFAULT_EDITOR" in
+*/* | [A-Z]*)
+	DEFAULT_EDITOR=
+	;;
+esac
+
+for i in GIT_EDITOR core_editor EDITOR VISUAL \
+	${DEFAULT_EDITOR:+"$DEFAULT_EDITOR"}
 do
 	cat >e-$i.sh <<-EOF
 	#!$SHELL_PATH
@@ -12,15 +23,17 @@ do
 	EOF
 	chmod +x e-$i.sh
 done
-unset vi
-mv e-vi.sh vi
-unset EDITOR VISUAL GIT_EDITOR
+
+if test -n "$DEFAULT_EDITOR"
+then
+	mv "e-$DEFAULT_EDITOR.sh" "$DEFAULT_EDITOR"
+fi
 
 test_expect_success setup '
 
 	msg="Hand edited" &&
 	echo "$msg" >expect &&
-	git add vi &&
+	git add "e-VISUAL.sh" &&
 	test_tick &&
 	git commit -m "$msg" &&
 	git show -s --pretty=oneline |
@@ -44,7 +57,8 @@ test_expect_success 'dumb should error out when falling back on vi' '
 
 TERM=vt100
 export TERM
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in ${DEFAULT_EDITOR:+"$DEFAULT_EDITOR"} \
+	EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	unset EDITOR VISUAL GIT_EDITOR
@@ -68,7 +82,8 @@ done
 
 unset EDITOR VISUAL GIT_EDITOR
 git config --unset-all core.editor
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in ${DEFAULT_EDITOR:+"$DEFAULT_EDITOR"} \
+	EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	case "$i" in
-- 
1.6.5.2

Re: [PATCH/RFC 2/2] Provide a build time default-editor setting

[David Roundy]

Any chance this will be exported as plumbing? I know it's pretty
high-level, but it'd be handy to have be able to write `git editor
$FILENAME` and just have it do the right thing.  This would also mean
that the perl scripts below could be simplified.

Same goes for pager, of course...

David

On Thu, Oct 29, 2009 at 3:50 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Provide a DEFAULT_EDITOR knob to allow the fallback editor (to
> use instead of vi if VISUAL, EDITOR, and GIT_EDITOR are unset) to
> be set at build time according to a system’s policy.  For
> example, on Debian systems, the default editor should be the
> 'editor' command.
>
> The contrib/fast-import/git-p4 script still uses vi, since it is
> not modified by the Makefile currently, and making it require
> build-time modification would create too much trouble for people
> deploying that script.
>
> This change makes t7005-editor into a mess.  Any ideas for fixing
> this?
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  Makefile                  |   10 ++++++++++
>  editor.c                  |    2 +-
>  git-add--interactive.perl |    3 ++-
>  git-sh-setup.sh           |    6 ++++--
>  git-svn.perl              |    5 +++--
>  t/Makefile                |    2 ++
>  t/t7005-editor.sh         |   29 ++++++++++++++++++++++-------
>  7 files changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index fc1a461..fae8647 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -203,6 +203,9 @@ all::
>  #
>  # Define DEFAULT_PAGER to the path of a sensible pager (defaults to "less") if
>  # you want to use something different.
> +#
> +# Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
> +# want to use something different.
>
>  GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
>        @$(SHELL_PATH) ./GIT-VERSION-GEN
> @@ -1301,6 +1304,11 @@ ifndef DEFAULT_PAGER
>        DEFAULT_PAGER = less
>  endif
>  BASIC_CFLAGS += -DDEFAULT_PAGER='"$(DEFAULT_PAGER)"'
> +ifndef DEFAULT_EDITOR
> +       DEFAULT_EDITOR = vi
> +endif
> +export DEFAULT_EDITOR
> +BASIC_CFLAGS += -DDEFAULT_EDITOR='"$(DEFAULT_EDITOR)"'
>
>  ifdef USE_NED_ALLOCATOR
>        COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -DOVERRIDE_STRDUP -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR -Icompat/nedmalloc
> @@ -1435,6 +1443,7 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
>            -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
>            -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
>            -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
> +           -e 's|DEFAULT_EDITOR:=vi|DEFAULT_EDITOR:=$(DEFAULT_EDITOR)|' \
>            -e $(BROKEN_PATH_FIX) \
>            $@.sh >$@+ && \
>        chmod +x $@+ && \
> @@ -1459,6 +1468,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
>            -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
>            -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
>            -e 's/@@DEFAULT_PAGER@@/$(DEFAULT_PAGER)/g' \
> +           -e 's/@@DEFAULT_EDITOR@@/$(DEFAULT_EDITOR)/g' \
>            $@.perl >$@+ && \
>        chmod +x $@+ && \
>        mv $@+ $@
> diff --git a/editor.c b/editor.c
> index 4d469d0..93b8cbb 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -19,7 +19,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
>                return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
>
>        if (!editor)
> -               editor = "vi";
> +               editor = DEFAULT_EDITOR;
>
>        if (strcmp(editor, ":")) {
>                size_t len = strlen(editor);
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 69aeaf0..c3d932c 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1,6 +1,7 @@
>  #!/usr/bin/perl -w
>
>  use strict;
> +use constant DEFAULT_EDITOR => '@@DEFAULT_EDITOR@@';
>  use Git;
>
>  binmode(STDOUT, ":raw");
> @@ -988,7 +989,7 @@ EOF
>        close $fh;
>
>        my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
> -               || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
> +               || $ENV{VISUAL} || $ENV{EDITOR} || DEFAULT_EDITOR;
>        system('sh', '-c', $editor.' "$@"', $editor, $hunkfile);
>
>        if ($? != 0) {
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index c41c2f7..d053d56 100755
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -99,19 +99,21 @@ set_reflog_action() {
>  }
>
>  git_editor() {
> +       : "${DEFAULT_EDITOR:=vi}"
>        : "${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,"
> -               echo >&2 "or EDITOR. Tried to fall back to vi but terminal is dumb."
> +               echo >&2 "or EDITOR. Tried to fall back to $DEFAULT_EDITOR" \
> +                       "but terminal is dumb."
>                echo >&2 "Please set one of these variables to an appropriate"
>                echo >&2 "editor or run $0 with options that will not cause an"
>                echo >&2 "editor to be invoked (e.g., -m or -F for git-commit)."
>                exit 1
>                ;;
>        esac
> -       eval "${GIT_EDITOR:=vi}" '"$@"'
> +       eval "${GIT_EDITOR:=$DEFAULT_EDITOR}" '"$@"'
>  }
>
>  is_bare_repository () {
> diff --git a/git-svn.perl b/git-svn.perl
> index c270b23..b98d378 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -3,12 +3,13 @@
>  # License: GPL v2 or later
>  use warnings;
>  use strict;
> -use vars qw/   $AUTHOR $VERSION $DEFAULT_PAGER
> +use vars qw/   $AUTHOR $VERSION $DEFAULT_PAGER $DEFAULT_EDITOR
>                $sha1 $sha1_short $_revision $_repository
>                $_q $_authors $_authors_prog %users/;
>  $AUTHOR = 'Eric Wong <normalperson@yhbt.net>';
>  $VERSION = '@@GIT_VERSION@@';
>  $DEFAULT_PAGER = '@@DEFAULT_PAGER@@';
> +$DEFAULT_EDITOR = '@@DEFAULT_EDITOR@@';
>
>  # From which subdir have we been invoked?
>  my $cmd_dir_prefix = eval {
> @@ -1322,7 +1323,7 @@ sub get_commit_entry {
>        close $log_fh or croak $!;
>
>        if ($_edit || ($type eq 'tree')) {
> -               my $editor = $ENV{VISUAL} || $ENV{EDITOR} || 'vi';
> +               my $editor = $ENV{VISUAL} || $ENV{EDITOR} || $DEFAULT_EDITOR;
>                # TODO: strip out spaces, comments, like git-commit.sh
>                system($editor, $commit_editmsg);
>        }
> diff --git a/t/Makefile b/t/Makefile
> index bd09390..9174bbb 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -9,6 +9,8 @@
>  SHELL_PATH ?= $(SHELL)
>  TAR ?= $(TAR)
>  RM ?= rm -f
> +DEFAULT_EDITOR ?= vi
> +export DEFAULT_EDITOR
>
>  # Shell quote;
>  SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index b647957..2b76f72 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -4,7 +4,18 @@ test_description='GIT_EDITOR, core.editor, and stuff'
>
>  . ./test-lib.sh
>
> -for i in GIT_EDITOR core_editor EDITOR VISUAL vi
> +: ${DEFAULT_EDITOR=vi}
> +
> +unset EDITOR VISUAL GIT_EDITOR
> +
> +case "$DEFAULT_EDITOR" in
> +*/* | [A-Z]*)
> +       DEFAULT_EDITOR=
> +       ;;
> +esac
> +
> +for i in GIT_EDITOR core_editor EDITOR VISUAL \
> +       ${DEFAULT_EDITOR:+"$DEFAULT_EDITOR"}
>  do
>        cat >e-$i.sh <<-EOF
>        #!$SHELL_PATH
> @@ -12,15 +23,17 @@ do
>        EOF
>        chmod +x e-$i.sh
>  done
> -unset vi
> -mv e-vi.sh vi
> -unset EDITOR VISUAL GIT_EDITOR
> +
> +if test -n "$DEFAULT_EDITOR"
> +then
> +       mv "e-$DEFAULT_EDITOR.sh" "$DEFAULT_EDITOR"
> +fi
>
>  test_expect_success setup '
>
>        msg="Hand edited" &&
>        echo "$msg" >expect &&
> -       git add vi &&
> +       git add "e-VISUAL.sh" &&
>        test_tick &&
>        git commit -m "$msg" &&
>        git show -s --pretty=oneline |
> @@ -44,7 +57,8 @@ test_expect_success 'dumb should error out when falling back on vi' '
>
>  TERM=vt100
>  export TERM
> -for i in vi EDITOR VISUAL core_editor GIT_EDITOR
> +for i in ${DEFAULT_EDITOR:+"$DEFAULT_EDITOR"} \
> +       EDITOR VISUAL core_editor GIT_EDITOR
>  do
>        echo "Edited by $i" >expect
>        unset EDITOR VISUAL GIT_EDITOR
> @@ -68,7 +82,8 @@ done
>
>  unset EDITOR VISUAL GIT_EDITOR
>  git config --unset-all core.editor
> -for i in vi EDITOR VISUAL core_editor GIT_EDITOR
> +for i in ${DEFAULT_EDITOR:+"$DEFAULT_EDITOR"} \
> +       EDITOR VISUAL core_editor GIT_EDITOR
>  do
>        echo "Edited by $i" >expect
>        case "$i" in
> --
> 1.6.5.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
David Roundy

Re: [PATCH/RFC 2/2] Provide a build time default-editor setting

[Johannes Sixt]

David Roundy schrieb:
> Any chance this will be exported as plumbing? I know it's pretty
> high-level, but it'd be handy to have be able to write `git editor
> $FILENAME` and just have it do the right thing.  This would also mean
> that the perl scripts below could be simplified.

Something like below? Possible usage in shell scripts:

	editor=$(git var GIT_EDITOR)
	"$editor" "$filename"

-- Hannes

PS: warning: linewrapped.

Subject: [PATCH] Teach git var about GIT_EDITOR

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 cache.h  |    1 +
 editor.c |   13 +++++++++++--
 var.c    |    6 ++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index a5eeead..3103dda 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,7 @@ extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const
char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
+extern const char *git_editor();

 struct checkout {
 	const char *base_dir;
diff --git a/editor.c b/editor.c
index 4d469d0..bd8c828 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"

-int launch_editor(const char *path, struct strbuf *buffer, const char
*const *env)
+const char *git_editor()
 {
 	const char *editor, *terminal;

@@ -16,11 +16,20 @@ int launch_editor(const char *path, struct strbuf
 	terminal = getenv("TERM");
 	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
-		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
+		return "/dev/null";

 	if (!editor)
 		editor = "vi";

+	return editor;
+}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char
*const *env)
+{
+	const char *editor = git_editor();
+
+	if (!strcmp(editor, "/dev/null"))
+		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
diff --git a/var.c b/var.c
index 125c0d1..48d8b9a 100644
--- a/var.c
+++ b/var.c
@@ -8,6 +8,11 @@

 static const char var_usage[] = "git var [-l | <variable>]";

+static const char *editor(int unused)
+{
+	return git_editor();
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -15,6 +20,7 @@ struct git_var {
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
+	{ "GIT_EDITOR", editor },
 	{ "", NULL },
 };

-- 
1.6.5.rc2.47.g49402

[PATCH 0/2] Default Pager and Editor at build-time

[Ben Walton]

The two patches look ok to me (not withstanding the comments already
made about the test and possible future changes required).

I did make a few small tweaks to use | instead of / as the sed
substitution separator (since we're presumably working with full
paths).  The substitution was also extended into the .sh scripts
(git-am, explicitly).

Jonathan Nieder (1):
  Provide a build time default-editor setting

Junio C Hamano (1):
  Provide a build time default-pager setting

 Makefile                  |   19 +++++++++++++++++++
 editor.c                  |    2 +-
 git-add--interactive.perl |    3 ++-
 git-am.sh                 |    2 +-
 git-sh-setup.sh           |    6 ++++--
 git-svn.perl              |    8 +++++---
 pager.c                   |    2 +-
 t/Makefile                |    2 ++
 t/t7005-editor.sh         |   29 ++++++++++++++++++++++-------
 9 files changed, 57 insertions(+), 16 deletions(-)

[PATCH 1/2] Provide a build time default-pager setting

[Ben Walton]

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

On (old) solaris systems, /usr/bin/less (typically the first less
found) doesn't understand the default arguments (FXRS), which
forces users to alter their environment (PATH, GIT_PAGER, LESS,
etc) or have a local or global gitconfig before paging works as
expected.

On Debian systems, by policy packages must fall back to the
'pager' command, so that changing the target of the
/usr/bin/pager symlink changes the default pager for all packages
at once.

Provide a DEFAULT_PAGER knob so packagers can set the fallback
pager to something appropriate during the build.

This puts the "less" default in the Makefile instead of pager.c, since
it is needed for git-svn and git-am, too.  This means that the
DEFAULT_PAGER preprocessor token _has_ to be defined on the command
line for git to build.  I was worried about this for a moment, but
GIT_VERSION already works this way without trouble.

Probably the DEFAULT_PAGER setting should be added to something
like TRACK_CFLAGS as well.  Actually, some other settings that
can change without forcing files to be rebuilt (e.g. SHELL_PATH),
too.  This should be probably be addressed separately.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
 Makefile     |    9 +++++++++
 git-am.sh    |    2 +-
 git-svn.perl |    5 +++--
 pager.c      |    2 +-
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/Makefile b/Makefile
index 42b7d60..1d26800 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,9 @@ all::
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
+#
+# Define DEFAULT_PAGER to the path of a sensible pager (defaults to "less") if
+# you want to use something different.
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1294,6 +1297,10 @@ ifdef NO_REGEX
 	COMPAT_CFLAGS += -Icompat/regex
 	COMPAT_OBJS += compat/regex/regex.o
 endif
+ifndef DEFAULT_PAGER
+	DEFAULT_PAGER = less
+endif
+BASIC_CFLAGS += -DDEFAULT_PAGER='"$(DEFAULT_PAGER)"'
 
 ifdef USE_NED_ALLOCATOR
        COMPAT_CFLAGS += -DUSE_NED_ALLOCATOR -DOVERRIDE_STRDUP -DNDEBUG -DREPLACE_SYSTEM_ALLOCATOR -Icompat/nedmalloc
@@ -1428,6 +1435,7 @@ $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh
 	    -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
 	    -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
+	    -e 's|@@DEFAULT_PAGER@@|$(DEFAULT_PAGER)|g' \
 	    -e $(BROKEN_PATH_FIX) \
 	    $@.sh >$@+ && \
 	chmod +x $@+ && \
@@ -1451,6 +1459,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	    -e '}' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
+	    -e 's|@@DEFAULT_PAGER@@|$(DEFAULT_PAGER)|g' \
 	    $@.perl >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
diff --git a/git-am.sh b/git-am.sh
index c132f50..a194a4e 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -649,7 +649,7 @@ do
 		[eE]*) git_editor "$dotest/final-commit"
 		       action=again ;;
 		[vV]*) action=again
-		       LESS=-S ${PAGER:-less} "$dotest/patch" ;;
+		       LESS=-S ${PAGER:-@@DEFAULT_PAGER@@} "$dotest/patch" ;;
 		*)     action=again ;;
 		esac
 	    done
diff --git a/git-svn.perl b/git-svn.perl
index eb4b75a..a61ec55 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3,11 +3,12 @@
 # License: GPL v2 or later
 use warnings;
 use strict;
-use vars qw/	$AUTHOR $VERSION
+use vars qw/	$AUTHOR $VERSION $DEFAULT_PAGER
 		$sha1 $sha1_short $_revision $_repository
 		$_q $_authors $_authors_prog %users/;
 $AUTHOR = 'Eric Wong <normalperson@yhbt.net>';
 $VERSION = '@@GIT_VERSION@@';
+$DEFAULT_PAGER = '@@DEFAULT_PAGER@@';
 
 # From which subdir have we been invoked?
 my $cmd_dir_prefix = eval {
@@ -5031,7 +5032,7 @@ sub git_svn_log_cmd {
 sub config_pager {
 	$pager ||= $ENV{GIT_PAGER} || $ENV{PAGER};
 	if (!defined $pager) {
-		$pager = 'less';
+		$pager = $DEFAULT_PAGER;
 	} elsif (length $pager == 0 || $pager eq 'cat') {
 		$pager = undef;
 	}
diff --git a/pager.c b/pager.c
index 86facec..416a796 100644
--- a/pager.c
+++ b/pager.c
@@ -58,7 +58,7 @@ void setup_pager(void)
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!pager)
-		pager = "less";
+		pager = DEFAULT_PAGER;
 	else if (!*pager || !strcmp(pager, "cat"))
 		return;
 
-- 
1.6.5

Re: [PATCH/RFC 2/2] Provide a build time default-editor setting

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> David Roundy schrieb:
>> Any chance this will be exported as plumbing? I know it's pretty
>> high-level, but it'd be handy to have be able to write `git editor
>> $FILENAME` and just have it do the right thing.  This would also mean
>> that the perl scripts below could be simplified.
>
> Something like below? Possible usage in shell scripts:
>
> 	editor=$(git var GIT_EDITOR)
> 	"$editor" "$filename"

I think we support GIT_EDITOR that is command path plus its initial
command line arguments, so you do not want dq around $editor, right?

Re: [PATCH/RFC 2/2] Provide a build time default-editor setting

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Provide a DEFAULT_EDITOR knob to allow the fallback editor (to
> use instead of vi if VISUAL, EDITOR, and GIT_EDITOR are unset) to
> be set at build time according to a system’s policy.  For
> example, on Debian systems, the default editor should be the
> 'editor' command.

I think we allow things like

    GIT_EDITOR='"/c/my program/vi" --i-like-color --config=$HOME/.myvicfg'

and the eval construct in git-sh-setup.sh is about supporting that kind of
insanity^Wflexibility.

My "how about" patch on DEFAULT_PAGER might be minimally safe with

    make DEFAULT_PAGER="/c/my program/less"

but if you are going to do this for real, you would need to use proper
quoting in the Makefile (look for _SQ for hints).

Also I do not think it allows this at all:

    make DEFAULT_PAGER='"/c/my program/less" --i-like-color'

It probably is Ok to force the "default" one to be just the path to the
command (i.e. not part of command line), but I thought this would be worth
pointing out.

> This change makes t7005-editor into a mess.  Any ideas for fixing
> this?

I think the introduction of DEFAULT_EDITOR makes it unfixable; your
DEFAULT_EDITOR may be set to '/usr/bin/vi' not 'vi'.

Just detect DEFAULT_EDITOR being not the default 'vi' and abort/skip the
entire test, perhaps?

Re: [PATCH/RFC 2/2] Provide a build time default-editor setting

[Johannes Sixt]

On Donnerstag, 29. Oktober 2009, Junio C Hamano wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
> > Something like below? Possible usage in shell scripts:
> >
> > 	editor=$(git var GIT_EDITOR)
> > 	"$editor" "$filename"
>
> I think we support GIT_EDITOR that is command path plus its initial
> command line arguments, so you do not want dq around $editor, right?

Yeah, whatever, I didn't take the time to think it through. But this may be an 
opportunity to give some life back to the zombie that git-var currently is, 
that is, to make it the plumbing that does value discovery for variables like 
GIT_AUTHOR_INDENT, GIT_COMMITTER_IDENT, GIT_EDITOR, and perhaps also 
GIT_PAGER.

-- Hannes

Re: [PATCH/RFC 2/2] Provide a build time default-editor setting

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Yeah, whatever, I didn't take the time to think it through. But this may be an 
> opportunity to give some life back to the zombie that git-var currently is, 
> that is, to make it the plumbing that does value discovery for variables like 
> GIT_AUTHOR_INDENT, GIT_COMMITTER_IDENT, GIT_EDITOR, and perhaps also 
> GIT_PAGER.

Hmm, wouldn't it make even more sense to "run" them for the calling
Porcelain script?

A shell script Porcelain can already ". git-sh-setup" and say

	git_editor this-file

when it needs to spawn the editor of choice.  Your new plumbing support
could make the definition of git_editor in git-sh-setup.sh into something
like:

    git_editor() {
    	git var --run GIT_EDITOR "$@"
    }
    git_pager() {
    	git var --run GIT_PAGER "$@"
    }

Re: [PATCH/RFC 2/2] Provide a build time default-editor setting

[David Roundy]

On Thu, Oct 29, 2009 at 6:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> Yeah, whatever, I didn't take the time to think it through. But this may be an
>> opportunity to give some life back to the zombie that git-var currently is,
>> that is, to make it the plumbing that does value discovery for variables like
>> GIT_AUTHOR_INDENT, GIT_COMMITTER_IDENT, GIT_EDITOR, and perhaps also
>> GIT_PAGER.
>
> Hmm, wouldn't it make even more sense to "run" them for the calling
> Porcelain script?

That was what I had been thinking.  That way the caller doesn't need
to know whether it may be a space-containing absolute path or an
executable with flags, as long as git knows what to do.

David

[PATCH v2 0/8] Default pager and editor

[Jonathan Nieder]

Junio C Hamano wrote:

> My "how about" patch on DEFAULT_PAGER might be minimally safe with
> 
>     make DEFAULT_PAGER="/c/my program/less"

It isn’t, actually, since in pager.c the pager already gets run through
sh if it contains certain shell metacharacters.

> but if you are going to do this for real, you would need to use proper
> quoting in the Makefile (look for _SQ for hints).

Good catch --- thanks.
 
> > This change makes t7005-editor into a mess.  Any ideas for fixing
> > this?
> 
> I think the introduction of DEFAULT_EDITOR makes it unfixable; your
> DEFAULT_EDITOR may be set to '/usr/bin/vi' not 'vi'.
> 
> Just detect DEFAULT_EDITOR being not the default 'vi' and abort/skip the
> entire test, perhaps?

Yep, unfortunately that looks like the best thing to do.  I tried to
salvage some of the test for distros (like Debian) that might override
the default without using an absolute path.

Here’s an updated series.  It doesn’t provide git var --run yet since
the Windows exit status magic means that would require either futzing
with the run_pager() implementation or reimplementing cat in var.c.

Thoughts?

Johannes Sixt (1):
  Teach git var about GIT_EDITOR

Jonathan Nieder (6):
  launch_editor: Longer error message when TERM=dumb
  Handle more shell metacharacters in editor name
  Teach git var about GIT_PAGER
  add -i, send-email, svn, p4, etc: Use "git var GIT_EDITOR"
  am -i, git-svn: use $(git var GIT_PAGER) instead of 'less'
  Provide a build time default-editor setting

Junio C Hamano (1):
  Provide a build time default-pager setting

 Documentation/config.txt         |    4 +---
 Documentation/git-commit.txt     |    2 +-
 Documentation/git-send-email.txt |    4 ++--
 Documentation/git-var.txt        |   14 ++++++++++++++
 Makefile                         |   28 ++++++++++++++++++++++++++++
 cache.h                          |    2 ++
 contrib/fast-import/git-p4       |    5 +----
 editor.c                         |   33 ++++++++++++++++++++++++++++-----
 git-add--interactive.perl        |    3 +--
 git-am.sh                        |    5 ++++-
 git-send-email.perl              |    3 ++-
 git-sh-setup.sh                  |   19 ++++++-------------
 git-svn.perl                     |   11 ++++-------
 pager.c                          |   24 ++++++++++++++++++++----
 t/t7005-editor.sh                |   31 ++++++++++++++++++++++++-------
 var.c                            |   20 ++++++++++++++++++++
 16 files changed, 158 insertions(+), 50 deletions(-)

[PATCH 1/8] launch_editor: Longer error message when TERM=dumb

[Jonathan Nieder]

Before falling back to vi, git checks if the terminal can support
such an editor by checking if $TERM is dumb.  git-sh-setup and
editor.c have similar but distinct error messages for this case.
To avoid changes in behavior when switching from one
implementation to the other, standardize on one error message.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Probably this check should apply to $VISUAL, too, but that is a
separate topic.

I am not sure which is the better error message.  It looks like some
effort went into the longer message so I thought I should give it a
try, but I kind of prefer the shorter one.

 editor.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/editor.c b/editor.c
index 4d469d0..316d139 100644
--- a/editor.c
+++ b/editor.c
@@ -16,7 +16,13 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 
 	terminal = getenv("TERM");
 	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
-		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
+		/* Terminal is dumb but no VISUAL nor EDITOR defined. */
+		return 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).");
 
 	if (!editor)
 		editor = "vi";
-- 
1.6.5.2

[PATCH 2/8] Handle more shell metacharacters in editor names

[Jonathan Nieder]

Pass the editor name to the shell if it contains any susv3 shell
special character (globs, redirections, variable substitutions,
escapes, etc).  This way, the meaning of some characters will not
meaninglessly change when others are added, and git commands
implemented in C and in shell scripts will interpret editor names
in the same way.

This does not make the GIT_EDITOR setting any more expressive,
since one could always use single quotes to force the editor to
be passed to the shell.

Signed-off-by: Jonathan Nieder<jrnieder@gmail.com>
---
 editor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/editor.c b/editor.c
index 316d139..facd7f2 100644
--- a/editor.c
+++ b/editor.c
@@ -34,7 +34,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		const char *args[6];
 		struct strbuf arg0 = STRBUF_INIT;
 
-		if (strcspn(editor, "$ \t'") != len) {
+		if (strcspn(editor, "|&;<>()$`\\\"' \t\n*?[#~=%") != len) {
 			/* there are specials */
 			strbuf_addf(&arg0, "%s \"$@\"", editor);
 			args[i++] = "sh";
-- 
1.6.5.2

[PATCH 3/8] Teach git var about GIT_EDITOR

[Jonathan Nieder]

From: Johannes Sixt <j.sixt@viscovery.net>

Expose the command used by launch_editor() for scripts to use.
This should allow one to avoid searching for a proper editor
separately in each command.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-var.txt |    8 ++++++++
 cache.h                   |    1 +
 editor.c                  |   18 +++++++++++++++---
 var.c                     |   10 ++++++++++
 4 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index e2f4c09..89e4b4f 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -36,6 +36,14 @@ GIT_AUTHOR_IDENT::
 GIT_COMMITTER_IDENT::
     The person who put a piece of code into git.
 
+GIT_EDITOR::
+    Text editor for use by git commands.  The value is meant to be
+    interpreted by the shell when it is used.  Examples: `~/bin/vi`,
+    `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
+    --nofork`.  The order of preference is the `$GIT_EDITOR`
+    environment variable, then `core.editor` configuration, then
+    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 96840c7..311cfe1 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,7 @@ extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
+extern const char *git_editor(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/editor.c b/editor.c
index facd7f2..9dcf95c 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_editor(void)
 {
 	const char *editor, *terminal;
 
@@ -15,18 +15,30 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		editor = getenv("EDITOR");
 
 	terminal = getenv("TERM");
-	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
+	if (!editor && (!terminal || !strcmp(terminal, "dumb"))) {
 		/* Terminal is dumb but no VISUAL nor EDITOR defined. */
-		return error(
+		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;
+	}
 
 	if (!editor)
 		editor = "vi";
 
+	return editor;
+}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor = git_editor();
+
+	if (!editor)
+		return -1;
+
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
diff --git a/var.c b/var.c
index 125c0d1..342dc2c 100644
--- a/var.c
+++ b/var.c
@@ -8,6 +8,15 @@
 
 static const char var_usage[] = "git var [-l | <variable>]";
 
+static const char *editor(int flag)
+{
+	const char *pgm = git_editor();
+
+	if (!pgm && (flag & IDENT_ERROR_ON_NO_NAME))
+		die("cannot find a suitable editor");
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -15,6 +24,7 @@ struct git_var {
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
+	{ "GIT_EDITOR", editor },
 	{ "", NULL },
 };
 
-- 
1.6.5.2

[PATCH 4/8] Teach git var about GIT_PAGER

[Jonathan Nieder]

Expose the command found by setup_pager() for scripts to use.
Scripts can use this to avoid repeating the logic to look for a
proper pager in each command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/git-var.txt |    6 ++++++
 cache.h                   |    1 +
 pager.c                   |   18 +++++++++++++++---
 var.c                     |   10 ++++++++++
 4 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 89e4b4f..ef6aa81 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -44,6 +44,12 @@ GIT_EDITOR::
     environment variable, then `core.editor` configuration, then
     `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
 
+GIT_PAGER::
+    Text viewer for use by git commands (e.g., 'less').  The value
+    is meant to be interpreted by the shell.  The order of preference
+    is the `$GIT_PAGER` environment variable, then `core.pager`
+    configuration, then `$PAGER`, and then finally 'less'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 311cfe1..5aaa4ba 100644
--- a/cache.h
+++ b/cache.h
@@ -751,6 +751,7 @@ extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
 extern const char *git_editor(void);
+extern const char *git_pager(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/pager.c b/pager.c
index 86facec..0b63d99 100644
--- a/pager.c
+++ b/pager.c
@@ -44,12 +44,14 @@ static void wait_for_pager_signal(int signo)
 	raise(signo);
 }
 
-void setup_pager(void)
+const char *git_pager(void)
 {
-	const char *pager = getenv("GIT_PAGER");
+	const char *pager;
 
 	if (!isatty(1))
-		return;
+		return NULL;
+
+	pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
 			git_config(git_default_config, NULL);
@@ -60,6 +62,16 @@ void setup_pager(void)
 	if (!pager)
 		pager = "less";
 	else if (!*pager || !strcmp(pager, "cat"))
+		pager = NULL;
+
+	return pager;
+}
+
+void setup_pager(void)
+{
+	const char *pager = git_pager();
+
+	if (!pager)
 		return;
 
 	spawned_pager = 1; /* means we are emitting to terminal */
diff --git a/var.c b/var.c
index 342dc2c..18dad57 100644
--- a/var.c
+++ b/var.c
@@ -17,6 +17,15 @@ static const char *editor(int flag)
 	return pgm;
 }
 
+static const char *pager(int flag)
+{
+	const char *pgm = git_pager();
+
+	if (!pgm)
+		pgm = "cat";
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -25,6 +34,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
 	{ "GIT_EDITOR", editor },
+	{ "GIT_PAGER", pager },
 	{ "", NULL },
 };
 
-- 
1.6.5.2

[PATCH 5/8] add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR"

[Jonathan Nieder]

Use the new "git var GIT_EDITOR" feature to decide what editor to
use, instead of duplicating its logic elsewhere.  This should make
the behavior of commands in edge cases (e.g., editor names with
spaces) a little more consistent.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/config.txt         |    4 +---
 Documentation/git-commit.txt     |    2 +-
 Documentation/git-send-email.txt |    4 ++--
 contrib/fast-import/git-p4       |    5 +----
 git-add--interactive.perl        |    3 +--
 git-send-email.perl              |    3 ++-
 git-sh-setup.sh                  |   19 ++++++-------------
 git-svn.perl                     |    5 ++---
 8 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d1e2120..5181b77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -387,9 +387,7 @@ core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
 	variable when it is set, and the environment variable
-	`GIT_EDITOR` is not set.  The order of preference is
-	`GIT_EDITOR` environment, `core.editor`, `VISUAL` and
-	`EDITOR` environment variables and then finally `vi`.
+	`GIT_EDITOR` is not set.  See linkgit:git-var[1].
 
 core.pager::
 	The command that git will use to paginate output.  Can
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0578a40..3ea80c8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -323,7 +323,7 @@ ENVIRONMENT AND CONFIGURATION VARIABLES
 The editor used to edit the commit log message will be chosen from the
 GIT_EDITOR environment variable, the core.editor configuration variable, the
 VISUAL environment variable, or the EDITOR environment variable (in that
-order).
+order).  See linkgit:git-var[1] for details.
 
 HOOKS
 -----
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 767cf4d..c85d7f4 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -60,8 +60,8 @@ The --bcc option must be repeated for each user you want on the bcc list.
 The --cc option must be repeated for each user you want on the cc list.
 
 --compose::
-	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
-	introductory message for the patch series.
+	Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
+	to edit an introductory message for the patch series.
 +
 When '--compose' is used, git send-email will use the From, Subject, and
 In-Reply-To headers specified in the message. If the body of the message
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index e710219..48059d0 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -729,13 +729,10 @@ class P4Submit(Command):
             tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
             tmpFile.close()
             mtime = os.stat(fileName).st_mtime
-            defaultEditor = "vi"
-            if platform.system() == "Windows":
-                defaultEditor = "notepad"
             if os.environ.has_key("P4EDITOR"):
                 editor = os.environ.get("P4EDITOR")
             else:
-                editor = os.environ.get("EDITOR", defaultEditor);
+                editor = read_pipe("git var GIT_EDITOR")
             system(editor + " " + fileName)
 
             response = "y"
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 69aeaf0..0c74e5c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -987,8 +987,7 @@ sub edit_hunk_manually {
 EOF
 	close $fh;
 
-	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
-		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	chomp(my $editor = run_cmd_pipe(qw(git var GIT_EDITOR)));
 	system('sh', '-c', $editor.' "$@"', $editor, $hunkfile);
 
 	if ($? != 0) {
diff --git a/git-send-email.perl b/git-send-email.perl
index a0279de..4f5da4e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -162,7 +162,8 @@ my $compose_filename;
 
 # Handle interactive edition of files.
 my $multiedit;
-my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+my $editor = Git::command_oneline('var', 'GIT_EDITOR');
+
 sub do_edit {
 	if (defined($multiedit) && !$multiedit) {
 		map {
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c41c2f7..99cceeb 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -99,19 +99,12 @@ set_reflog_action() {
 }
 
 git_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,"
-		echo >&2 "or EDITOR. Tried to fall back to vi but terminal is dumb."
-		echo >&2 "Please set one of these variables to an appropriate"
-		echo >&2 "editor or run $0 with options that will not cause an"
-		echo >&2 "editor to be invoked (e.g., -m or -F for git-commit)."
-		exit 1
-		;;
-	esac
-	eval "${GIT_EDITOR:=vi}" '"$@"'
+	if test -z "${GIT_EDITOR:+set}"
+	then
+		GIT_EDITOR="$(git var GIT_EDITOR)" || return $?
+	fi
+
+	eval "$GIT_EDITOR" '"$@"'
 }
 
 is_bare_repository () {
diff --git a/git-svn.perl b/git-svn.perl
index 6a3b501..42c9a72 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1321,9 +1321,8 @@ sub get_commit_entry {
 	close $log_fh or croak $!;
 
 	if ($_edit || ($type eq 'tree')) {
-		my $editor = $ENV{VISUAL} || $ENV{EDITOR} || 'vi';
-		# TODO: strip out spaces, comments, like git-commit.sh
-		system($editor, $commit_editmsg);
+		chomp(my $editor = command_oneline(qw(var GIT_EDITOR)));
+		system('sh', '-c', $editor.' "$@"', $editor, $commit_editmsg);
 	}
 	rename $commit_editmsg, $commit_msg or croak $!;
 	{
-- 
1.6.5.2

[PATCH 6/8] am -i, git-svn: use "git var GIT_PAGER"

[Jonathan Nieder]

Use the new "git var GIT_PAGER" command to ask what pager to use.

Without this change, the core.pager configuration is ignored by
these commands.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-am.sh    |    5 ++++-
 git-svn.perl |    6 ++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c132f50..2649487 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -649,7 +649,10 @@ do
 		[eE]*) git_editor "$dotest/final-commit"
 		       action=again ;;
 		[vV]*) action=again
-		       LESS=-S ${PAGER:-less} "$dotest/patch" ;;
+		       : ${GIT_PAGER=$(git var GIT_PAGER)}
+		       : ${LESS=-FRSX}
+		       export LESS
+		       $GIT_PAGER "$dotest/patch" ;;
 		*)     action=again ;;
 		esac
 	    done
diff --git a/git-svn.perl b/git-svn.perl
index 42c9a72..c4ca548 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5171,10 +5171,8 @@ sub git_svn_log_cmd {
 
 # adapted from pager.c
 sub config_pager {
-	$pager ||= $ENV{GIT_PAGER} || $ENV{PAGER};
-	if (!defined $pager) {
-		$pager = 'less';
-	} elsif (length $pager == 0 || $pager eq 'cat') {
+	chomp(my $pager = command_oneline(qw(var GIT_PAGER)));
+	if ($pager eq 'cat') {
 		$pager = undef;
 	}
 	$ENV{GIT_PAGER_IN_USE} = defined($pager);
-- 
1.6.5.2

[PATCH 7/8] Provide a build time default-editor setting

[Jonathan Nieder]

Provide a DEFAULT_EDITOR knob to allow setting the fallback
editor to use instead of vi (when VISUAL, EDITOR, and GIT_EDITOR
are unset).  The value can be set at build time according to a
system’s policy.  For example, on Debian systems, the default
editor should be the 'editor' command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile          |   17 +++++++++++++++++
 editor.c          |   11 ++++++++---
 t/t7005-editor.sh |   31 ++++++++++++++++++++++++-------
 3 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/Makefile b/Makefile
index 268aede..625866c 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,14 @@ all::
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
+#
+# Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
+# want to use something different.  The value will be interpreted by the shell
+# if necessary when it is used.  Examples:
+#
+#   DEFAULT_EDITOR='~/bin/vi',
+#   DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
+#   DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1363,6 +1371,15 @@ BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
 	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
+# Quote for C
+
+ifdef DEFAULT_EDITOR
+DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
+DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/editor.c b/editor.c
index 9dcf95c..fcf35a8 100644
--- a/editor.c
+++ b/editor.c
@@ -2,6 +2,10 @@
 #include "strbuf.h"
 #include "run-command.h"
 
+#ifndef DEFAULT_EDITOR
+#define DEFAULT_EDITOR "vi"
+#endif
+
 const char *git_editor(void)
 {
 	const char *editor, *terminal;
@@ -19,15 +23,16 @@ const char *git_editor(void)
 		/* Terminal is dumb but no VISUAL nor EDITOR defined. */
 		error(
 		  "No editor specified in GIT_EDITOR, core.editor, VISUAL,\n"
-		  "or EDITOR. Tried to fall back to vi but terminal is dumb.\n"
+		  "or EDITOR. Tried to fall back to %s 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).");
+		  "editor to be invoked (e.g., -m or -F for git commit).",
+		  DEFAULT_EDITOR);
 		return NULL;
 	}
 
 	if (!editor)
-		editor = "vi";
+		editor = DEFAULT_EDITOR;
 
 	return editor;
 }
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b647957..73ba44c 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -4,7 +4,26 @@ test_description='GIT_EDITOR, core.editor, and stuff'
 
 . ./test-lib.sh
 
-for i in GIT_EDITOR core_editor EDITOR VISUAL vi
+unset EDITOR VISUAL GIT_EDITOR
+
+test_expect_success 'does editor have a simple name (no slashes, etc)?' '
+
+	editor=$(TERM=vt100 git var GIT_EDITOR) &&
+	test -n "$editor" &&
+	simple=t &&
+	case "$editor" in
+	*/* | core_editor | [A-Z]*)
+		unset simple;;
+	esac
+
+'
+if test -z "${simple+set}"
+then
+	say 'skipping editor tests, default editor is not sought on PATH'
+	test_done
+fi
+
+for i in GIT_EDITOR core_editor EDITOR VISUAL "$editor"
 do
 	cat >e-$i.sh <<-EOF
 	#!$SHELL_PATH
@@ -12,15 +31,13 @@ do
 	EOF
 	chmod +x e-$i.sh
 done
-unset vi
-mv e-vi.sh vi
-unset EDITOR VISUAL GIT_EDITOR
+mv "e-$editor.sh" "$editor"
 
 test_expect_success setup '
 
 	msg="Hand edited" &&
 	echo "$msg" >expect &&
-	git add vi &&
+	git add "$editor" &&
 	test_tick &&
 	git commit -m "$msg" &&
 	git show -s --pretty=oneline |
@@ -44,7 +61,7 @@ test_expect_success 'dumb should error out when falling back on vi' '
 
 TERM=vt100
 export TERM
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in "$editor" EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	unset EDITOR VISUAL GIT_EDITOR
@@ -68,7 +85,7 @@ done
 
 unset EDITOR VISUAL GIT_EDITOR
 git config --unset-all core.editor
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in "$editor" EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	case "$i" in
-- 
1.6.5.2

[PATCH 8/8] Provide a build time default-pager setting

[Jonathan Nieder]

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

Provide a DEFAULT_PAGER knob so packagers can set the fallback
pager to something appropriate during the build.

Examples:

On (old) solaris systems, /usr/bin/less (typically the first less
found) doesn't understand the default arguments (FXRS), which
forces users to alter their environment (PATH, GIT_PAGER, LESS,
etc) or have a local or global gitconfig before paging works as
expected.

On Debian systems, by policy packages must fall back to the
'pager' command, so that changing the target of the
/usr/bin/pager symlink changes the default pager for all packages
at once.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Previously I suggested that the default pager isn’t being tracked
properly with TRACK_CFLAGS.  Actually, since it is included in
BASIC_CFLAGS, it always was.  Sorry for the confusion.

 Makefile |   11 +++++++++++
 pager.c  |    6 +++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 625866c..18fc50a 100644
--- a/Makefile
+++ b/Makefile
@@ -201,6 +201,10 @@ all::
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
 #
+# Define DEFAULT_PAGER to a sensible pager command (defaults to "less") if
+# you want to use something different.  The value will be interpreted by the
+# shell at runtime when it is used.
+#
 # Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
 # want to use something different.  The value will be interpreted by the shell
 # if necessary when it is used.  Examples:
@@ -1380,6 +1384,13 @@ DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
 BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
 endif
 
+ifdef DEFAULT_PAGER
+DEFAULT_PAGER_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_PAGER)))"
+DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/pager.c b/pager.c
index 0b63d99..92c03f6 100644
--- a/pager.c
+++ b/pager.c
@@ -2,6 +2,10 @@
 #include "run-command.h"
 #include "sigchain.h"
 
+#ifndef DEFAULT_PAGER
+#define DEFAULT_PAGER "less"
+#endif
+
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -60,7 +64,7 @@ const char *git_pager(void)
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!pager)
-		pager = "less";
+		pager = DEFAULT_PAGER;
 	else if (!*pager || !strcmp(pager, "cat"))
 		pager = NULL;
 
-- 
1.6.5.2

[PATCH/RFC 9/8] Teach git var to run the editor

[Jonathan Nieder]

Expose the functionality of launch_editor() for scripts to use.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
As I mentioned in the cover letter, the analogous change for the pager
is a little more tricky.  I was wrong to blame Windows for this.  The
excellent commit ea27a18 (spawn pager via run_command interface,
2008-07-22) explains all.

The difficulties: the pager receives input from the current process
and the run_pager() function does not take an argument to take input
from somewhere else.  Also the pager is not exec()'d directly, so the
current process sticks around uselessly until it quits and it is a
little tricky to find the 'less' exit status for "git var --run" to
use as well.

 Documentation/git-var.txt |   10 ++++++++-
 var.c                     |   48 +++++++++++++++++++++++++++++++++++++-------
 2 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index ef6aa81..1bfdb6c 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -8,7 +8,10 @@ git-var - Show a git logical variable
 
 SYNOPSIS
 --------
-'git var' [ -l | <variable> ]
+[verse]
+'git var' <variable>
+'git var' -l
+'git var' --run <variable> [ args ]
 
 DESCRIPTION
 -----------
@@ -22,6 +25,11 @@ OPTIONS
 	as well. (However, the configuration variables listing functionality
 	is deprecated in favor of 'git config -l'.)
 
+--run variable [args]::
+	If the specified logical variable represents a command, run that
+	command.  For example, `git var --run GIT_EDITOR foo.txt` edits
+	foo.txt with the text editor git is configured to use.
+
 EXAMPLE
 --------
 	$ git var GIT_AUTHOR_IDENT
diff --git a/var.c b/var.c
index 18dad57..c97b2e6 100644
--- a/var.c
+++ b/var.c
@@ -6,7 +6,8 @@
 #include "cache.h"
 #include "exec_cmd.h"
 
-static const char var_usage[] = "git var [-l | <variable>]";
+static const char var_usage[] =
+	"git var { -l | <variable> | --run <variable> [args] }";
 
 static const char *editor(int flag)
 {
@@ -26,16 +27,25 @@ static const char *pager(int flag)
 	return pgm;
 }
 
+static int run_editor(int argc, const char *const *argv)
+{
+	if (argc > 1)
+		return error("cannot launch editor with more than one file");
+
+	return launch_editor(argv[0], NULL, NULL);
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
+	int (*run)(int argc, const char *const *argv);
 };
 static struct git_var git_vars[] = {
-	{ "GIT_COMMITTER_IDENT", git_committer_info },
-	{ "GIT_AUTHOR_IDENT",   git_author_info },
-	{ "GIT_EDITOR", editor },
-	{ "GIT_PAGER", pager },
-	{ "", NULL },
+	{ "GIT_COMMITTER_IDENT", git_committer_info, NULL },
+	{ "GIT_AUTHOR_IDENT", git_author_info, NULL },
+	{ "GIT_EDITOR", editor, run_editor },
+	{ "GIT_PAGER", pager, NULL },
+	{ "", NULL, NULL },
 };
 
 static void list_vars(void)
@@ -59,6 +69,17 @@ static const char *read_var(const char *var)
 	return val;
 }
 
+static int run_var_cmd(const char *var, int argc, char **argv)
+{
+	struct git_var *ptr;
+
+	for (ptr = git_vars; ptr->read; ptr++)
+		if (ptr->run && strcmp(var, ptr->name) == 0)
+			return ptr->run(argc, (const char *const *)argv);
+
+	return error("%s is not a variable command", var);
+}
+
 static int show_config(const char *var, const char *value, void *cb)
 {
 	if (value)
@@ -72,12 +93,23 @@ int main(int argc, char **argv)
 {
 	const char *val;
 	int nongit;
+
+	git_extract_argv0_path(argv[0]);
+
+	if (argv[1] && strcmp(argv[1], "--run") == 0) {
+		if (argc <= 2)
+			usage(var_usage);
+
+		setup_git_directory_gently(&nongit);
+		git_config(git_default_config, NULL);
+
+		return run_var_cmd(argv[2], argc - 3, argv + 3);
+	}
+
 	if (argc != 2) {
 		usage(var_usage);
 	}
 
-	git_extract_argv0_path(argv[0]);
-
 	setup_git_directory_gently(&nongit);
 	val = NULL;
 
-- 
1.6.5.2

Re: [PATCH 7/8] Provide a build time default-editor setting

[Jonathan Nieder]

Jonathan Nieder wrote:

> Signed-off-by: Ben Walton <bwalton@bwalton@artsci.utoronto.ca>

That should be "Ben Walton <bwalton@artsci.utoronto.ca>", without the
extra bwalton@.  Sorry for the trouble.

Regards,
Jonathan

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

[Johannes Sixt]

Jonathan Nieder schrieb:
> From: Johannes Sixt <j.sixt@viscovery.net>
> 
> Expose the command used by launch_editor() for scripts to use.
> This should allow one to avoid searching for a proper editor
> separately in each command.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

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>

> -int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
> +const char *git_editor(void)
>  {
>  	const char *editor, *terminal;
>  
> ... 
>  	terminal = getenv("TERM");
> -	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
> +	if (!editor && (!terminal || !strcmp(terminal, "dumb"))) {
>  		/* Terminal is dumb but no VISUAL nor EDITOR defined. */
> -		return error(
> +		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(). The 
return value, NULL, should be indication enough for the callers to handle 
the situation suitable. 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

> +static const char *editor(int flag)
> +{
> +	const char *pgm = git_editor();
> +
> +	if (!pgm && (flag & IDENT_ERROR_ON_NO_NAME))
> +		die("cannot find a suitable editor");
> +	return pgm;

This should be

	return pgm ? pgm : "terminal is dumb, but VISUAL and EDITOR unset";

otherwise, 'git var -l' later trips over printf("%s", NULL).

-- Hannes

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

More importantly, that is what people traditionally expected from VISUAL
and EDITOR and we do that only to follow suit.

There is no such tradition for GIT_EDITOR nor core.editor and switching
behaviour based on the name of editor ("vi"? "vim"?...) does not feel
quite right.

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

[Jonathan Nieder]

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

Re: [PATCH 8/8] Provide a build time default-pager setting

[Junio C Hamano]

I'll queue these for now probably on 'pu', but with the comments we saw on
the list expect them to be followed up with replacement patches.

Thanks.

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

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> 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.
>
> More importantly, that is what people traditionally expected from VISUAL
> and EDITOR and we do that only to follow suit.

Unfortunately, we don’t do that: we currently still use $VISUAL if
TERM=dumb and just refuse to fall back to vi in that case.  I’ll add a
patch to fix this.

> There is no such tradition for GIT_EDITOR nor core.editor

Makes sense.

Jonathan

[PATCH v3 0/8] Default pager and editor

[Jonathan Nieder]

Junio C Hamano wrote:

> I'll queue these for now probably on 'pu', but with the comments we saw on
> the list expect them to be followed up with replacement patches.

Here’s a replacement series.  It omits the longer error message when
TERM=dumb and the git var --run experiment because I was not happy
with where either of those were going.

Thanks for all the comments, everyone.

Johannes Sixt (1):
  Teach git var about GIT_EDITOR

Jonathan Nieder (6):
  Handle more shell metacharacters in editor names
  Do not use VISUAL editor on dumb terminals
  Teach git var about GIT_PAGER
  add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR"
  am -i, git-svn: use "git var GIT_PAGER"
  Provide a build time default-editor setting

Junio C Hamano (1):
  Provide a build time default-pager setting

 Documentation/config.txt         |    4 +---
 Documentation/git-commit.txt     |    2 +-
 Documentation/git-send-email.txt |    4 ++--
 Documentation/git-var.txt        |   14 ++++++++++++++
 Makefile                         |   28 ++++++++++++++++++++++++++++
 cache.h                          |    2 ++
 contrib/fast-import/git-p4       |    5 +----
 editor.c                         |   32 +++++++++++++++++++++++---------
 git-add--interactive.perl        |    3 +--
 git-am.sh                        |    5 ++++-
 git-send-email.perl              |    3 ++-
 git-sh-setup.sh                  |   19 ++++++-------------
 git-svn.perl                     |   11 ++++-------
 pager.c                          |   24 ++++++++++++++++++++----
 t/t7005-editor.sh                |   31 ++++++++++++++++++++++++-------
 var.c                            |   31 ++++++++++++++++++++++++++++++-
 16 files changed, 163 insertions(+), 55 deletions(-)

[PATCH 1/8] Handle more shell metacharacters in editor names

[Jonathan Nieder]

Pass the editor name to the shell if it contains any susv3 shell
special character (globs, redirections, variable substitutions,
escapes, etc).  This way, the meaning of some characters will not
meaninglessly change when others are added, and git commands
implemented in C and in shell scripts will interpret editor names
in the same way.

This does not make the GIT_EDITOR setting any more expressive,
since one could always use single quotes to force the editor to
be passed to the shell.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Unchanged from v2.

 editor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/editor.c b/editor.c
index 4d469d0..941c0b2 100644
--- a/editor.c
+++ b/editor.c
@@ -28,7 +28,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		const char *args[6];
 		struct strbuf arg0 = STRBUF_INIT;
 
-		if (strcspn(editor, "$ \t'") != len) {
+		if (strcspn(editor, "|&;<>()$`\\\"' \t\n*?[#~=%") != len) {
 			/* there are specials */
 			strbuf_addf(&arg0, "%s \"$@\"", editor);
 			args[i++] = "sh";
-- 
1.6.5.2

[PATCH 2/8] Do not use VISUAL editor on dumb terminals

[Jonathan Nieder]

Refuse to use $VISUAL and fall back to $EDITOR if TERM is unset
or set to "dumb".  Traditionally, VISUAL is set to a screen
editor and EDITOR to a line-based editor, which should be more
useful in that situation.

vim, for example, is happy to assume a terminal supports ANSI
sequences even if TERM is dumb (e.g., when running from a text
editor like Acme).  git already refuses to fall back to vi on a
dumb terminal if GIT_EDITOR, core.editor, VISUAL, and EDITOR are
unset, but without this patch, that check is suppressed by
VISUAL=vi.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This patch eases my discomfort about the error message a little.  It
is not actually needed to support any ways of working I engage in.

If stdout is redirected, this is probably still making the wrong
choice; isatty(STDOUT_FILENO) might be a more useful datum to use.
But it does not seem worth complicating the logic further.

 editor.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/editor.c b/editor.c
index 941c0b2..3f13751 100644
--- a/editor.c
+++ b/editor.c
@@ -4,19 +4,19 @@
 
 int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
 {
-	const char *editor, *terminal;
+	const char *editor = getenv("GIT_EDITOR");
+	const char *terminal = getenv("TERM");
+	int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
 
-	editor = getenv("GIT_EDITOR");
 	if (!editor && editor_program)
 		editor = editor_program;
-	if (!editor)
+	if (!editor && !terminal_is_dumb)
 		editor = getenv("VISUAL");
 	if (!editor)
 		editor = getenv("EDITOR");
 
-	terminal = getenv("TERM");
-	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
-		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
+	if (!editor && terminal_is_dumb)
+		return error("terminal is dumb, but EDITOR unset");
 
 	if (!editor)
 		editor = "vi";
-- 
1.6.5.2

[PATCH v2 3/8] Teach git var about GIT_EDITOR

[Jonathan Nieder]

From: Johannes Sixt <j6t@kdbg.org>

Expose the command used by launch_editor() for scripts to use.
This should allow one to avoid searching for a proper editor
separately in each command.

If no satisfactory GIT_EDITOR could be chosen, let "git var -l"
output a warning.  This warning goes to stderr so as not to
confuse scripts.  Example:

	core.logallrefupdates=true

	*** Please tell me who you are.

	Run

	  git config --global user.email "you@example.com"
	  git config --global user.name "Your Name"

	to set your account's default identity.
	Omit --global to set the identity only in this repository.

	GIT_COMMITTER_IDENT=user <user@domain> 1256952739 -0500
	GIT_AUTHOR_IDENT=user <user@domain> 1256952739 -0500
	warning: GIT_EDITOR: terminal is dumb, but EDITOR unset

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---

 Documentation/git-var.txt |    8 ++++++++
 cache.h                   |    1 +
 editor.c                  |   14 ++++++++++++--
 var.c                     |   21 ++++++++++++++++++++-
 4 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index e2f4c09..89e4b4f 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -36,6 +36,14 @@ GIT_AUTHOR_IDENT::
 GIT_COMMITTER_IDENT::
     The person who put a piece of code into git.
 
+GIT_EDITOR::
+    Text editor for use by git commands.  The value is meant to be
+    interpreted by the shell when it is used.  Examples: `~/bin/vi`,
+    `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
+    --nofork`.  The order of preference is the `$GIT_EDITOR`
+    environment variable, then `core.editor` configuration, then
+    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 96840c7..311cfe1 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,7 @@ extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
+extern const char *git_editor(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/editor.c b/editor.c
index 3f13751..4f98b72 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
 	const char *terminal = getenv("TERM");
@@ -16,11 +16,21 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		editor = getenv("EDITOR");
 
 	if (!editor && terminal_is_dumb)
-		return error("terminal is dumb, but EDITOR unset");
+		return NULL;
 
 	if (!editor)
 		editor = "vi";
 
+	return editor;
+}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor = git_editor();
+
+	if (!editor)
+		return error("terminal is dumb, but EDITOR unset");
+
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
diff --git a/var.c b/var.c
index 125c0d1..399f409 100644
--- a/var.c
+++ b/var.c
@@ -8,6 +8,21 @@
 
 static const char var_usage[] = "git var [-l | <variable>]";
 
+static const char *editor(int flag)
+{
+	const char *pgm = git_editor();
+
+	if (!pgm) {
+		if (flag & IDENT_ERROR_ON_NO_NAME)
+			die("terminal is dumb, but EDITOR unset");
+		if (flag & IDENT_WARN_ON_NO_NAME)
+			warning("GIT_EDITOR: terminal is dumb, "
+				"but EDITOR unset");
+	}
+
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -15,14 +30,18 @@ struct git_var {
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
+	{ "GIT_EDITOR", editor },
 	{ "", NULL },
 };
 
 static void list_vars(void)
 {
 	struct git_var *ptr;
+	const char *val;
+
 	for (ptr = git_vars; ptr->read; ptr++)
-		printf("%s=%s\n", ptr->name, ptr->read(IDENT_WARN_ON_NO_NAME));
+		if ((val = ptr->read(IDENT_WARN_ON_NO_NAME)))
+			printf("%s=%s\n", ptr->name, val);
 }
 
 static const char *read_var(const char *var)
-- 
1.6.5.2

[PATCH 4/8] Teach git var about GIT_PAGER

[Jonathan Nieder]

Expose the command found by setup_pager() for scripts to use.
Scripts can use this to avoid repeating the logic to look for a
proper pager in each command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
No changes from the last version sent.

 Documentation/git-var.txt |    6 ++++++
 cache.h                   |    1 +
 pager.c                   |   18 +++++++++++++++---
 var.c                     |   10 ++++++++++
 4 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 89e4b4f..ef6aa81 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -44,6 +44,12 @@ GIT_EDITOR::
     environment variable, then `core.editor` configuration, then
     `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
 
+GIT_PAGER::
+    Text viewer for use by git commands (e.g., 'less').  The value
+    is meant to be interpreted by the shell.  The order of preference
+    is the `$GIT_PAGER` environment variable, then `core.pager`
+    configuration, then `$PAGER`, and then finally 'less'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 311cfe1..5aaa4ba 100644
--- a/cache.h
+++ b/cache.h
@@ -751,6 +751,7 @@ extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
 extern const char *git_editor(void);
+extern const char *git_pager(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/pager.c b/pager.c
index 86facec..0b63d99 100644
--- a/pager.c
+++ b/pager.c
@@ -44,12 +44,14 @@ static void wait_for_pager_signal(int signo)
 	raise(signo);
 }
 
-void setup_pager(void)
+const char *git_pager(void)
 {
-	const char *pager = getenv("GIT_PAGER");
+	const char *pager;
 
 	if (!isatty(1))
-		return;
+		return NULL;
+
+	pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
 			git_config(git_default_config, NULL);
@@ -60,6 +62,16 @@ void setup_pager(void)
 	if (!pager)
 		pager = "less";
 	else if (!*pager || !strcmp(pager, "cat"))
+		pager = NULL;
+
+	return pager;
+}
+
+void setup_pager(void)
+{
+	const char *pager = git_pager();
+
+	if (!pager)
 		return;
 
 	spawned_pager = 1; /* means we are emitting to terminal */
diff --git a/var.c b/var.c
index 399f409..facec11 100644
--- a/var.c
+++ b/var.c
@@ -23,6 +23,15 @@ static const char *editor(int flag)
 	return pgm;
 }
 
+static const char *pager(int flag)
+{
+	const char *pgm = git_pager();
+
+	if (!pgm)
+		pgm = "cat";
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -31,6 +40,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
 	{ "GIT_EDITOR", editor },
+	{ "GIT_PAGER", pager },
 	{ "", NULL },
 };
 
-- 
1.6.5.2

[PATCH 5/8] add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR"

[Jonathan Nieder]

Use the new "git var GIT_EDITOR" feature to decide what editor to
use, instead of duplicating its logic elsewhere.  This should make
the behavior of commands in edge cases (e.g., editor names with
spaces) a little more consistent.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/config.txt         |    4 +---
 Documentation/git-commit.txt     |    2 +-
 Documentation/git-send-email.txt |    4 ++--
 contrib/fast-import/git-p4       |    5 +----
 git-add--interactive.perl        |    3 +--
 git-send-email.perl              |    3 ++-
 git-sh-setup.sh                  |   19 ++++++-------------
 git-svn.perl                     |    5 ++---
 8 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d1e2120..5181b77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -387,9 +387,7 @@ core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
 	variable when it is set, and the environment variable
-	`GIT_EDITOR` is not set.  The order of preference is
-	`GIT_EDITOR` environment, `core.editor`, `VISUAL` and
-	`EDITOR` environment variables and then finally `vi`.
+	`GIT_EDITOR` is not set.  See linkgit:git-var[1].
 
 core.pager::
 	The command that git will use to paginate output.  Can
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0578a40..3ea80c8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -323,7 +323,7 @@ ENVIRONMENT AND CONFIGURATION VARIABLES
 The editor used to edit the commit log message will be chosen from the
 GIT_EDITOR environment variable, the core.editor configuration variable, the
 VISUAL environment variable, or the EDITOR environment variable (in that
-order).
+order).  See linkgit:git-var[1] for details.
 
 HOOKS
 -----
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 767cf4d..c85d7f4 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -60,8 +60,8 @@ The --bcc option must be repeated for each user you want on the bcc list.
 The --cc option must be repeated for each user you want on the cc list.
 
 --compose::
-	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
-	introductory message for the patch series.
+	Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
+	to edit an introductory message for the patch series.
 +
 When '--compose' is used, git send-email will use the From, Subject, and
 In-Reply-To headers specified in the message. If the body of the message
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index e710219..48059d0 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -729,13 +729,10 @@ class P4Submit(Command):
             tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
             tmpFile.close()
             mtime = os.stat(fileName).st_mtime
-            defaultEditor = "vi"
-            if platform.system() == "Windows":
-                defaultEditor = "notepad"
             if os.environ.has_key("P4EDITOR"):
                 editor = os.environ.get("P4EDITOR")
             else:
-                editor = os.environ.get("EDITOR", defaultEditor);
+                editor = read_pipe("git var GIT_EDITOR")
             system(editor + " " + fileName)
 
             response = "y"
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 69aeaf0..0c74e5c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -987,8 +987,7 @@ sub edit_hunk_manually {
 EOF
 	close $fh;
 
-	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
-		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	chomp(my $editor = run_cmd_pipe(qw(git var GIT_EDITOR)));
 	system('sh', '-c', $editor.' "$@"', $editor, $hunkfile);
 
 	if ($? != 0) {
diff --git a/git-send-email.perl b/git-send-email.perl
index a0279de..4f5da4e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -162,7 +162,8 @@ my $compose_filename;
 
 # Handle interactive edition of files.
 my $multiedit;
-my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+my $editor = Git::command_oneline('var', 'GIT_EDITOR');
+
 sub do_edit {
 	if (defined($multiedit) && !$multiedit) {
 		map {
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c41c2f7..99cceeb 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -99,19 +99,12 @@ set_reflog_action() {
 }
 
 git_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,"
-		echo >&2 "or EDITOR. Tried to fall back to vi but terminal is dumb."
-		echo >&2 "Please set one of these variables to an appropriate"
-		echo >&2 "editor or run $0 with options that will not cause an"
-		echo >&2 "editor to be invoked (e.g., -m or -F for git-commit)."
-		exit 1
-		;;
-	esac
-	eval "${GIT_EDITOR:=vi}" '"$@"'
+	if test -z "${GIT_EDITOR:+set}"
+	then
+		GIT_EDITOR="$(git var GIT_EDITOR)" || return $?
+	fi
+
+	eval "$GIT_EDITOR" '"$@"'
 }
 
 is_bare_repository () {
diff --git a/git-svn.perl b/git-svn.perl
index 6a3b501..42c9a72 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1321,9 +1321,8 @@ sub get_commit_entry {
 	close $log_fh or croak $!;
 
 	if ($_edit || ($type eq 'tree')) {
-		my $editor = $ENV{VISUAL} || $ENV{EDITOR} || 'vi';
-		# TODO: strip out spaces, comments, like git-commit.sh
-		system($editor, $commit_editmsg);
+		chomp(my $editor = command_oneline(qw(var GIT_EDITOR)));
+		system('sh', '-c', $editor.' "$@"', $editor, $commit_editmsg);
 	}
 	rename $commit_editmsg, $commit_msg or croak $!;
 	{
-- 
1.6.5.2

[PATCH 6/8] am -i, git-svn: use "git var GIT_PAGER"

[Jonathan Nieder]

Use the new "git var GIT_PAGER" command to ask what pager to use.

Without this change, the core.pager configuration is ignored by
these commands.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 git-am.sh    |    5 ++++-
 git-svn.perl |    6 ++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c132f50..2649487 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -649,7 +649,10 @@ do
 		[eE]*) git_editor "$dotest/final-commit"
 		       action=again ;;
 		[vV]*) action=again
-		       LESS=-S ${PAGER:-less} "$dotest/patch" ;;
+		       : ${GIT_PAGER=$(git var GIT_PAGER)}
+		       : ${LESS=-FRSX}
+		       export LESS
+		       $GIT_PAGER "$dotest/patch" ;;
 		*)     action=again ;;
 		esac
 	    done
diff --git a/git-svn.perl b/git-svn.perl
index 42c9a72..c4ca548 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5171,10 +5171,8 @@ sub git_svn_log_cmd {
 
 # adapted from pager.c
 sub config_pager {
-	$pager ||= $ENV{GIT_PAGER} || $ENV{PAGER};
-	if (!defined $pager) {
-		$pager = 'less';
-	} elsif (length $pager == 0 || $pager eq 'cat') {
+	chomp(my $pager = command_oneline(qw(var GIT_PAGER)));
+	if ($pager eq 'cat') {
 		$pager = undef;
 	}
 	$ENV{GIT_PAGER_IN_USE} = defined($pager);
-- 
1.6.5.2

[PATCH 7/8] Provide a build time default-editor setting

[Jonathan Nieder]

Provide a DEFAULT_EDITOR knob to allow setting the fallback
editor to use instead of vi (when VISUAL, EDITOR, and GIT_EDITOR
are unset).  The value can be set at build time according to a
system’s policy.  For example, on Debian systems, the default
editor should be the 'editor' command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile          |   17 +++++++++++++++++
 editor.c          |    6 +++++-
 t/t7005-editor.sh |   31 ++++++++++++++++++++++++-------
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 268aede..625866c 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,14 @@ all::
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
+#
+# Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
+# want to use something different.  The value will be interpreted by the shell
+# if necessary when it is used.  Examples:
+#
+#   DEFAULT_EDITOR='~/bin/vi',
+#   DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
+#   DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1363,6 +1371,15 @@ BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
 	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
+# Quote for C
+
+ifdef DEFAULT_EDITOR
+DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
+DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/editor.c b/editor.c
index 4f98b72..2aac807 100644
--- a/editor.c
+++ b/editor.c
@@ -2,6 +2,10 @@
 #include "strbuf.h"
 #include "run-command.h"
 
+#ifndef DEFAULT_EDITOR
+#define DEFAULT_EDITOR "vi"
+#endif
+
 const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
@@ -19,7 +23,7 @@ const char *git_editor(void)
 		return NULL;
 
 	if (!editor)
-		editor = "vi";
+		editor = DEFAULT_EDITOR;
 
 	return editor;
 }
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b647957..73ba44c 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -4,7 +4,26 @@ test_description='GIT_EDITOR, core.editor, and stuff'
 
 . ./test-lib.sh
 
-for i in GIT_EDITOR core_editor EDITOR VISUAL vi
+unset EDITOR VISUAL GIT_EDITOR
+
+test_expect_success 'does editor have a simple name (no slashes, etc)?' '
+
+	editor=$(TERM=vt100 git var GIT_EDITOR) &&
+	test -n "$editor" &&
+	simple=t &&
+	case "$editor" in
+	*/* | core_editor | [A-Z]*)
+		unset simple;;
+	esac
+
+'
+if test -z "${simple+set}"
+then
+	say 'skipping editor tests, default editor is not sought on PATH'
+	test_done
+fi
+
+for i in GIT_EDITOR core_editor EDITOR VISUAL "$editor"
 do
 	cat >e-$i.sh <<-EOF
 	#!$SHELL_PATH
@@ -12,15 +31,13 @@ do
 	EOF
 	chmod +x e-$i.sh
 done
-unset vi
-mv e-vi.sh vi
-unset EDITOR VISUAL GIT_EDITOR
+mv "e-$editor.sh" "$editor"
 
 test_expect_success setup '
 
 	msg="Hand edited" &&
 	echo "$msg" >expect &&
-	git add vi &&
+	git add "$editor" &&
 	test_tick &&
 	git commit -m "$msg" &&
 	git show -s --pretty=oneline |
@@ -44,7 +61,7 @@ test_expect_success 'dumb should error out when falling back on vi' '
 
 TERM=vt100
 export TERM
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in "$editor" EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	unset EDITOR VISUAL GIT_EDITOR
@@ -68,7 +85,7 @@ done
 
 unset EDITOR VISUAL GIT_EDITOR
 git config --unset-all core.editor
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in "$editor" EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	case "$i" in
-- 
1.6.5.2

[PATCH 8/8] Provide a build time default-pager setting

[Jonathan Nieder]

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

Provide a DEFAULT_PAGER knob so packagers can set the fallback
pager to something appropriate during the build.

Examples:

On (old) solaris systems, /usr/bin/less (typically the first less
found) doesn't understand the default arguments (FXRS), which
forces users to alter their environment (PATH, GIT_PAGER, LESS,
etc) or have a local or global gitconfig before paging works as
expected.

On Debian systems, by policy packages must fall back to the
'pager' command, so that changing the target of the
/usr/bin/pager symlink changes the default pager for all packages
at once.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile |   11 +++++++++++
 pager.c  |    6 +++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 625866c..18fc50a 100644
--- a/Makefile
+++ b/Makefile
@@ -201,6 +201,10 @@ all::
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
 #
+# Define DEFAULT_PAGER to a sensible pager command (defaults to "less") if
+# you want to use something different.  The value will be interpreted by the
+# shell at runtime when it is used.
+#
 # Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
 # want to use something different.  The value will be interpreted by the shell
 # if necessary when it is used.  Examples:
@@ -1380,6 +1384,13 @@ DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
 BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
 endif
 
+ifdef DEFAULT_PAGER
+DEFAULT_PAGER_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_PAGER)))"
+DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/pager.c b/pager.c
index 0b63d99..92c03f6 100644
--- a/pager.c
+++ b/pager.c
@@ -2,6 +2,10 @@
 #include "run-command.h"
 #include "sigchain.h"
 
+#ifndef DEFAULT_PAGER
+#define DEFAULT_PAGER "less"
+#endif
+
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -60,7 +64,7 @@ const char *git_pager(void)
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!pager)
-		pager = "less";
+		pager = DEFAULT_PAGER;
 	else if (!*pager || !strcmp(pager, "cat"))
 		pager = NULL;
 
-- 
1.6.5.2

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> From: Johannes Sixt <j6t@kdbg.org>
>
> Expose the command used by launch_editor() for scripts to use.
> This should allow one to avoid searching for a proper editor
> separately in each command.
>
> If no satisfactory GIT_EDITOR could be chosen, let "git var -l"
> output a warning.  This warning goes to stderr so as not to
> confuse scripts.  Example:
>
> 	core.logallrefupdates=true
>
> 	*** Please tell me who you are.
>
> 	Run
>
> 	  git config --global user.email "you@example.com"
> 	  git config --global user.name "Your Name"
>
> 	to set your account's default identity.
> 	Omit --global to set the identity only in this repository.
>
> 	GIT_COMMITTER_IDENT=user <user@domain> 1256952739 -0500
> 	GIT_AUTHOR_IDENT=user <user@domain> 1256952739 -0500
> 	warning: GIT_EDITOR: terminal is dumb, but EDITOR unset

Sorry, I cannot grok this example.  Is it supposed to be a transcript
of a user session?  What did the user type?

Re: [PATCH 7/8] Provide a build time default-editor setting

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> +test_expect_success 'does editor have a simple name (no slashes, etc)?' '
> +
> +	editor=$(TERM=vt100 git var GIT_EDITOR) &&
> +	test -n "$editor" &&
> +	simple=t &&
> +	case "$editor" in
> +	*/* | core_editor | [A-Z]*)

Hmm, what are the latter two cases designed to catch?

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

[Jonathan Nieder]

Junio C Hamano wrote:

>> 	core.logallrefupdates=true
>>
>> 	*** Please tell me who you are.
>>
>> 	Run
>>
>> 	  git config --global user.email "you@example.com"
>> 	  git config --global user.name "Your Name"
>>
>> 	to set your account's default identity.
>> 	Omit --global to set the identity only in this repository.
>>
>> 	GIT_COMMITTER_IDENT=user <user@domain> 1256952739 -0500
>> 	GIT_AUTHOR_IDENT=user <user@domain> 1256952739 -0500
>> 	warning: GIT_EDITOR: terminal is dumb, but EDITOR unset
> 
> Sorry, I cannot grok this example.  Is it supposed to be a transcript
> of a user session?  What did the user type?

Oh, sorry about that.  The user typed 'git var -l', and that is all
output from that.  More realistic examples:

$ # what scripts see
$ git var -l 2>/dev/null
gc.auto=0
rerere.enabled
merge.log
merge.conflictstyle=diff3
core.repositoryformatversion=0
core.filemode=true
core.bare=false
core.logallrefupdates=true
remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
remote.origin.url=git://repo.or.cz/git
branch.master.remote=origin
branch.master.merge=refs/heads/master
GIT_COMMITTER_IDENT=user <user@domain> 1256952739 -0500
GIT_AUTHOR_IDENT=user <user@domain> 1256952739 -0500
$
$ # what scripts pass on to the user
$ git var -l >/dev/null

*** Please tell me who you are.

Run

  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

warning: GIT_EDITOR: terminal is dumb, but EDITOR unset
$ 

At least, that is what I was imagining (that’s one way to use git var,
anyway).

Would a more friendly message be helpful here?  I am not sure how 'git
var -l' gets used.  I never liked using it directly myself, mostly
because the long list of configuration items can be overwhelming.

Jonathan

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>>> 	core.logallrefupdates=true
>>>
>>> 	*** Please tell me who you are.
>>>
>>> 	Run
>>>
>>> 	  git config --global user.email "you@example.com"
>>> 	  git config --global user.name "Your Name"
>>>
>>> 	to set your account's default identity.
>>> 	Omit --global to set the identity only in this repository.
>>>
>>> 	GIT_COMMITTER_IDENT=user <user@domain> 1256952739 -0500
>>> 	GIT_AUTHOR_IDENT=user <user@domain> 1256952739 -0500
>>> 	warning: GIT_EDITOR: terminal is dumb, but EDITOR unset
>> 
>> Sorry, I cannot grok this example.  Is it supposed to be a transcript
>> of a user session?  What did the user type?
>
> Oh, sorry about that.  The user typed 'git var -l', and that is all
> output from that.  More realistic examples:
>
> $ # what scripts see
> $ git var -l 2>/dev/null
> gc.auto=0
> rerere.enabled
> merge.log
> merge.conflictstyle=diff3
> core.repositoryformatversion=0
> core.filemode=true
> core.bare=false
> core.logallrefupdates=true
> remote.origin.fetch=+refs/heads/*:refs/remotes/origin/*
> remote.origin.url=git://repo.or.cz/git
> branch.master.remote=origin
> branch.master.merge=refs/heads/master
> GIT_COMMITTER_IDENT=user <user@domain> 1256952739 -0500
> GIT_AUTHOR_IDENT=user <user@domain> 1256952739 -0500
> $
> $ # what scripts pass on to the user
> $ git var -l >/dev/null
>
> *** Please tell me who you are.
>
> Run
>
>   git config --global user.email "you@example.com"
>   git config --global user.name "Your Name"
>
> to set your account's default identity.
> Omit --global to set the identity only in this repository.
>
> warning: GIT_EDITOR: terminal is dumb, but EDITOR unset
> $ 

This is more readable.

But the user did not even ask for GIT_EDITOR.  Should it even mention
"unusable"?  or should it just say something like

	GIT_EDITOR=

without complaining?

For that matter, I also wonder if we can squelch the user.email one when
we are only listing the variables (I know it is not part of this topic,
but I can still wonder).

> Would a more friendly message be helpful here?  I am not sure how 'git
> var -l' gets used.  I never liked using it directly myself, mostly
> because the long list of configuration items can be overwhelming.

I think people run "git var -l", store the results in variables (think
Perl or Python script) and read from there, instead of making separate
invocations of "git var" for individual variables.

Re: [PATCH 7/8] Provide a build time default-editor setting

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
 
>> +test_expect_success 'does editor have a simple name (no slashes, etc)?' '
>> +
>> +	editor=$(TERM=vt100 git var GIT_EDITOR) &&
>> +	test -n "$editor" &&
>> +	simple=t &&
>> +	case "$editor" in
>> +	*/* | core_editor | [A-Z]*)
> 
> Hmm, what are the latter two cases designed to catch?

Both are meant to allow the test to work without too many changes.
The core_editor case is a little pedantic, since it is unlikely to
actually come up in practice.  With a default editor of core_editor,
the initial loop will overwrite e-core_editor.sh (to be used through
the core.editor configuration) with e-core_editor.sh (to be used as a
fallback editor) before renaming it to core_editor.

I missed some other cases: If editor is .git, e-GIT_EDITOR.sh, etc,
the mv will still misbehave.

The [A-Z]* test is to avoid changing the loop around line 86:

| unset EDITOR VISUAL GIT_EDITOR
| git config --unset-all core.editor
| for i in "$editor" EDITOR VISUAL core_editor GIT_EDITOR
| do
|	echo "Edited by $i" >expect
|	case "$i" in
|	core_editor)
|		git config core.editor ./e-core_editor.sh
|		;;
|	[A-Z]*)
|		eval "$i=./e-$i.sh"
|		export $i
|		;;
|	esac
|	test_expect_success "Using $i (override)" '
|		git --exec-path=. commit --amend &&
|		git show -s --pretty=oneline |
|		sed -e "s/^[0-9a-f]* //" >actual &&
|		diff actual expect
|	'
| done

which I do not think is worth making more complicated.

Maybe it would be better to just check for an editor consisting only
of alphabetical characters.  Perhaps something like the following:

-- %< --
From: Jonathan Nieder <jrnieder@gmail.com>
Subject: [PATCH] Provide a build time default-editor setting

Provide a DEFAULT_EDITOR knob to allow setting the fallback
editor to use instead of vi (when VISUAL, EDITOR, and GIT_EDITOR
are unset).  The value can be set at build time according to a
system’s policy.  For example, on Debian systems, the default
editor should be the 'editor' command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Makefile          |   17 +++++++++++++++++
 editor.c          |    6 +++++-
 t/t7005-editor.sh |   27 ++++++++++++++++++++-------
 3 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/Makefile b/Makefile
index 268aede..625866c 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,14 @@ all::
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
+#
+# Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
+# want to use something different.  The value will be interpreted by the shell
+# if necessary when it is used.  Examples:
+#
+#   DEFAULT_EDITOR='~/bin/vi',
+#   DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
+#   DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1363,6 +1371,15 @@ BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
 	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
+# Quote for C
+
+ifdef DEFAULT_EDITOR
+DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
+DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/editor.c b/editor.c
index 4f98b72..2aac807 100644
--- a/editor.c
+++ b/editor.c
@@ -2,6 +2,10 @@
 #include "strbuf.h"
 #include "run-command.h"
 
+#ifndef DEFAULT_EDITOR
+#define DEFAULT_EDITOR "vi"
+#endif
+
 const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
@@ -19,7 +23,7 @@ const char *git_editor(void)
 		return NULL;
 
 	if (!editor)
-		editor = "vi";
+		editor = DEFAULT_EDITOR;
 
 	return editor;
 }
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b647957..13c37de 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -4,7 +4,22 @@ test_description='GIT_EDITOR, core.editor, and stuff'
 
 . ./test-lib.sh
 
-for i in GIT_EDITOR core_editor EDITOR VISUAL vi
+unset EDITOR VISUAL GIT_EDITOR
+
+test_expect_success 'determine default editor' '
+
+	editor=$(TERM=vt100 git var GIT_EDITOR) &&
+	test -n "$editor"
+
+'
+
+if ! test -z "$(printf '%s\n' "$editor" | sed '/^[a-z]*$/d')"
+then
+	say 'skipping editor tests, default editor name too complicated'
+	test_done
+fi
+
+for i in GIT_EDITOR core_editor EDITOR VISUAL "$editor"
 do
 	cat >e-$i.sh <<-EOF
 	#!$SHELL_PATH
@@ -12,15 +27,13 @@ do
 	EOF
 	chmod +x e-$i.sh
 done
-unset vi
-mv e-vi.sh vi
-unset EDITOR VISUAL GIT_EDITOR
+mv "e-$editor.sh" "$editor"
 
 test_expect_success setup '
 
 	msg="Hand edited" &&
 	echo "$msg" >expect &&
-	git add vi &&
+	git add "$editor" &&
 	test_tick &&
 	git commit -m "$msg" &&
 	git show -s --pretty=oneline |
@@ -44,7 +57,7 @@ test_expect_success 'dumb should error out when falling back on vi' '
 
 TERM=vt100
 export TERM
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in "$editor" EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	unset EDITOR VISUAL GIT_EDITOR
@@ -68,7 +81,7 @@ done
 
 unset EDITOR VISUAL GIT_EDITOR
 git config --unset-all core.editor
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in "$editor" EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	case "$i" in
-- 
1.6.5.2

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

[Jonathan Nieder]

Junio C Hamano wrote:

> But the user did not even ask for GIT_EDITOR.  Should it even mention
> "unusable"?  or should it just say something like
> 
> 	GIT_EDITOR=
> 
> without complaining?
> 
> For that matter, I also wonder if we can squelch the user.email one when
> we are only listing the variables (I know it is not part of this topic,
> but I can still wonder).
[...]
> I think people run "git var -l", store the results in variables (think
> Perl or Python script) and read from there, instead of making separate
> invocations of "git var" for individual variables.

In that case, most variable-specific warnings should be suppressed as
irrelevant.  So squelching the warnings makes sense.

How about this patch?  With the "git var GIT_EDITOR" patch applied on
top, "git var -l" silently omits the GIT_EDITOR variable when a suitable
editor cannot be found.

-- %< --
Subject: Suppress warnings from "git var -l"

For scripts using "git var -l" to read all logical variables at
once, not all per-variable warnings will be relevant.  Suppress
them.

The git source tree does not include any scripts using "git var
-l", so this change should not affect other git commands.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 ident.c |    2 +-
 var.c   |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ident.c b/ident.c
index 99f1c85..26409b2 100644
--- a/ident.c
+++ b/ident.c
@@ -205,7 +205,7 @@ const char *fmt_ident(const char *name, const char *email,
 		if ((warn_on_no_name || error_on_no_name) &&
 		    name == git_default_name && env_hint) {
 			fprintf(stderr, env_hint, au_env, co_env);
-			env_hint = NULL; /* warn only once, for "git var -l" */
+			env_hint = NULL; /* warn only once */
 		}
 		if (error_on_no_name)
 			die("empty ident %s <%s> not allowed", name, email);
diff --git a/var.c b/var.c
index 125c0d1..dacbaab 100644
--- a/var.c
+++ b/var.c
@@ -22,7 +22,7 @@ static void list_vars(void)
 {
 	struct git_var *ptr;
 	for (ptr = git_vars; ptr->read; ptr++)
-		printf("%s=%s\n", ptr->name, ptr->read(IDENT_WARN_ON_NO_NAME));
+		printf("%s=%s\n", ptr->name, ptr->read(0));
 }
 
 static const char *read_var(const char *var)
-- 
1.6.5.2

[PATCH v3] Teach git var about GIT_EDITOR

[Jonathan Nieder]

From: Johannes Sixt <j6t@kdbg.org>

Expose the command used by launch_editor() for scripts to use.
This should allow one to avoid searching for a proper editor
separately in each command.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:

> How about this patch?  With the "git var GIT_EDITOR" patch applied on
> top, "git var -l" silently omits the GIT_EDITOR variable when a suitable
> editor cannot be found.
> 
> -- %< --
> Subject: Suppress warnings from "git var -l"
> 
> For scripts using "git var -l" to read all logical variables at
> once, not all per-variable warnings will be relevant.  Suppress
> them.

Here’s the "git var GIT_EDITOR" patch again, rebased on top of the
aforementioned patch.  The rest of the series should apply without
changes.

 Documentation/git-var.txt |    8 ++++++++
 cache.h                   |    1 +
 editor.c                  |   14 ++++++++++++--
 var.c                     |   16 +++++++++++++++-
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index e2f4c09..89e4b4f 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -36,6 +36,14 @@ GIT_AUTHOR_IDENT::
 GIT_COMMITTER_IDENT::
     The person who put a piece of code into git.
 
+GIT_EDITOR::
+    Text editor for use by git commands.  The value is meant to be
+    interpreted by the shell when it is used.  Examples: `~/bin/vi`,
+    `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
+    --nofork`.  The order of preference is the `$GIT_EDITOR`
+    environment variable, then `core.editor` configuration, then
+    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 96840c7..311cfe1 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,7 @@ extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
+extern const char *git_editor(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/editor.c b/editor.c
index 3f13751..4f98b72 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
 	const char *terminal = getenv("TERM");
@@ -16,11 +16,21 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		editor = getenv("EDITOR");
 
 	if (!editor && terminal_is_dumb)
-		return error("terminal is dumb, but EDITOR unset");
+		return NULL;
 
 	if (!editor)
 		editor = "vi";
 
+	return editor;
+}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor = git_editor();
+
+	if (!editor)
+		return error("terminal is dumb, but EDITOR unset");
+
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
diff --git a/var.c b/var.c
index dacbaab..12a8512 100644
--- a/var.c
+++ b/var.c
@@ -8,6 +8,16 @@
 
 static const char var_usage[] = "git var [-l | <variable>]";
 
+static const char *editor(int flag)
+{
+	const char *pgm = git_editor();
+
+	if (!pgm && flag & IDENT_ERROR_ON_NO_NAME)
+		die("terminal is dumb, but VISUAL and EDITOR unset");
+
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -15,14 +25,18 @@ struct git_var {
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
+	{ "GIT_EDITOR", editor },
 	{ "", NULL },
 };
 
 static void list_vars(void)
 {
 	struct git_var *ptr;
+	const char *val;
+
 	for (ptr = git_vars; ptr->read; ptr++)
-		printf("%s=%s\n", ptr->name, ptr->read(0));
+		if ((val = ptr->read(0))
+			printf("%s=%s\n", ptr->name, val);
 }
 
 static const char *read_var(const char *var)
-- 
1.6.5.2

Re: [PATCH v3] Teach git var about GIT_EDITOR

[Jonathan Nieder]

A typo fix --- sorry for the noise.

Jonathan Nieder wrote:

> --- a/var.c
> +++ b/var.c
> @@ -8,6 +8,16 @@
>  
>  static const char var_usage[] = "git var [-l | <variable>]";
>  
> +static const char *editor(int flag)
> +{
> +	const char *pgm = git_editor();
> +
> +	if (!pgm && flag & IDENT_ERROR_ON_NO_NAME)
> +		die("terminal is dumb, but VISUAL and EDITOR unset");

Agh...  s/VISUAL and //.

All right, time to sleep.  Apologies for all the mistakes, and thanks
for the help catching them.

Kind regards,
Jonathan

[PATCH v2 2/8] Do not use VISUAL editor on dumb terminals

[Jonathan Nieder]

Jonathan Nieder wrote:

> Refuse to use $VISUAL and fall back to $EDITOR if TERM is unset
> or set to "dumb".  Traditionally, VISUAL is set to a screen
> editor and EDITOR to a line-based editor, which should be more
> useful in that situation.

I was too lazy to wait for tests to finish on this one, and lo and
behold, they did not pass.

These additional changes seem to help, and they also add a test to
explain the change in editor behavior.  The patch with these changes
squashed is also included in this message, below the scissors mark.

In the controlled environment used for tests, TERM is set to dumb
and ever since commit 02b3566 (test-lib.sh: Add a test_set_editor
function to safely set $VISUAL, 2008-05-04), most tests set VISUAL
when they want to set an editor for git to use.  With this patch, they
should be using EDITOR instead.

--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -42,6 +42,16 @@ test_expect_success 'dumb should error out when falling back on vi' '
 	fi
 '
 
+test_expect_success 'dumb should prefer EDITOR to VISUAL' '
+
+	EDITOR=./e-EDITOR.sh &&
+	VISUAL=./e-VISUAL.sh &&
+	export EDITOR VISUAL &&
+	git commit --amend &&
+	test "$(git show -s --format=%s)" = "Edited by EDITOR"
+
+'
+
 TERM=vt100
 export TERM
 for i in vi EDITOR VISUAL core_editor GIT_EDITOR
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -86,7 +86,7 @@ chmod 755 editor
 
 test_expect_success \
 	"amend commit" \
-	"VISUAL=./editor git commit --amend"
+	"EDITOR=./editor git commit --amend"
 
 test_expect_success \
 	"passing -m and -F" \
@@ -107,7 +107,7 @@ chmod 755 editor
 test_expect_success \
 	"editing message from other commit" \
 	"echo 'hula hula' >file && \
-	 VISUAL=./editor git commit -c HEAD^ -a"
+	 EDITOR=./editor git commit -c HEAD^ -a"
 
 test_expect_success \
 	"message from stdin" \
@@ -141,10 +141,10 @@ EOF
 test_expect_success \
 	'editor not invoked if -F is given' '
 	 echo "moo" >file &&
-	 VISUAL=./editor git commit -a -F msg &&
+	 EDITOR=./editor git commit -a -F msg &&
 	 git show -s --pretty=format:"%s" | grep -q good &&
 	 echo "quack" >file &&
-	 echo "Another good message." | VISUAL=./editor git commit -a -F - &&
+	 echo "Another good message." | EDITOR=./editor git commit -a -F - &&
 	 git show -s --pretty=format:"%s" | grep -q good
 	 '
 # We could just check the head sha1, but checking each commit makes it
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -30,7 +30,7 @@ TZ=UTC
 TERM=dumb
 export LANG LC_ALL PAGER TERM TZ
 EDITOR=:
-VISUAL=:
+unset VISUAL
 unset GIT_EDITOR
 unset AUTHOR_DATE
 unset AUTHOR_EMAIL
@@ -58,7 +58,7 @@ GIT_MERGE_VERBOSITY=5
 export GIT_MERGE_VERBOSITY
 export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
 export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
-export EDITOR VISUAL
+export EDITOR
 GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
 
 # Protect ourselves from common misconfiguration to export
@@ -207,8 +207,8 @@ trap 'die' EXIT
 test_set_editor () {
 	FAKE_EDITOR="$1"
 	export FAKE_EDITOR
-	VISUAL='"$FAKE_EDITOR"'
-	export VISUAL
+	EDITOR='"$FAKE_EDITOR"'
+	export EDITOR
 }
 
 test_tick () {

-- %< --
Subject: [PATCH] Do not use VISUAL editor on dumb terminals

Refuse to use $VISUAL and fall back to $EDITOR if TERM is unset
or set to "dumb".  Traditionally, VISUAL is set to a screen
editor and EDITOR to a line-based editor, which should be more
useful in that situation.

vim, for example, is happy to assume a terminal supports ANSI
sequences even if TERM is dumb (e.g., when running from a text
editor like Acme).  git already refuses to fall back to vi on a
dumb terminal if GIT_EDITOR, core.editor, VISUAL, and EDITOR are
unset, but without this patch, that check is suppressed by
VISUAL=vi.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jonathan Nieder <jrn@progeny.tock>
---

 editor.c          |   12 ++++++------
 t/t7005-editor.sh |   10 ++++++++++
 t/t7501-commit.sh |    8 ++++----
 t/test-lib.sh     |    8 ++++----
 4 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/editor.c b/editor.c
index 941c0b2..3f13751 100644
--- a/editor.c
+++ b/editor.c
@@ -4,19 +4,19 @@
 
 int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
 {
-	const char *editor, *terminal;
+	const char *editor = getenv("GIT_EDITOR");
+	const char *terminal = getenv("TERM");
+	int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
 
-	editor = getenv("GIT_EDITOR");
 	if (!editor && editor_program)
 		editor = editor_program;
-	if (!editor)
+	if (!editor && !terminal_is_dumb)
 		editor = getenv("VISUAL");
 	if (!editor)
 		editor = getenv("EDITOR");
 
-	terminal = getenv("TERM");
-	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
-		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
+	if (!editor && terminal_is_dumb)
+		return error("terminal is dumb, but EDITOR unset");
 
 	if (!editor)
 		editor = "vi";
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b647957..a95fe19 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -42,6 +42,16 @@ test_expect_success 'dumb should error out when falling back on vi' '
 	fi
 '
 
+test_expect_success 'dumb should prefer EDITOR to VISUAL' '
+
+	EDITOR=./e-EDITOR.sh &&
+	VISUAL=./e-VISUAL.sh &&
+	export EDITOR VISUAL &&
+	git commit --amend &&
+	test "$(git show -s --format=%s)" = "Edited by EDITOR"
+
+'
+
 TERM=vt100
 export TERM
 for i in vi EDITOR VISUAL core_editor GIT_EDITOR
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d2de576..a603f6d 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -86,7 +86,7 @@ chmod 755 editor
 
 test_expect_success \
 	"amend commit" \
-	"VISUAL=./editor git commit --amend"
+	"EDITOR=./editor git commit --amend"
 
 test_expect_success \
 	"passing -m and -F" \
@@ -107,7 +107,7 @@ chmod 755 editor
 test_expect_success \
 	"editing message from other commit" \
 	"echo 'hula hula' >file && \
-	 VISUAL=./editor git commit -c HEAD^ -a"
+	 EDITOR=./editor git commit -c HEAD^ -a"
 
 test_expect_success \
 	"message from stdin" \
@@ -141,10 +141,10 @@ EOF
 test_expect_success \
 	'editor not invoked if -F is given' '
 	 echo "moo" >file &&
-	 VISUAL=./editor git commit -a -F msg &&
+	 EDITOR=./editor git commit -a -F msg &&
 	 git show -s --pretty=format:"%s" | grep -q good &&
 	 echo "quack" >file &&
-	 echo "Another good message." | VISUAL=./editor git commit -a -F - &&
+	 echo "Another good message." | EDITOR=./editor git commit -a -F - &&
 	 git show -s --pretty=format:"%s" | grep -q good
 	 '
 # We could just check the head sha1, but checking each commit makes it
diff --git a/t/test-lib.sh b/t/test-lib.sh
index f2ca536..ec3336a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -30,7 +30,7 @@ TZ=UTC
 TERM=dumb
 export LANG LC_ALL PAGER TERM TZ
 EDITOR=:
-VISUAL=:
+unset VISUAL
 unset GIT_EDITOR
 unset AUTHOR_DATE
 unset AUTHOR_EMAIL
@@ -58,7 +58,7 @@ GIT_MERGE_VERBOSITY=5
 export GIT_MERGE_VERBOSITY
 export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
 export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
-export EDITOR VISUAL
+export EDITOR
 GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
 
 # Protect ourselves from common misconfiguration to export
@@ -207,8 +207,8 @@ trap 'die' EXIT
 test_set_editor () {
 	FAKE_EDITOR="$1"
 	export FAKE_EDITOR
-	VISUAL='"$FAKE_EDITOR"'
-	export VISUAL
+	EDITOR='"$FAKE_EDITOR"'
+	export EDITOR
 }
 
 test_tick () {
-- 
1.6.5.2

> 
> vim, for example, is happy to assume a terminal supports ANSI
> sequences even if TERM is dumb (e.g., when running from a text
> editor like Acme).  git already refuses to fall back to vi on a
> dumb terminal if GIT_EDITOR, core.editor, VISUAL, and EDITOR are
> unset, but without this patch, that check is suppressed by
> VISUAL=vi.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> This patch eases my discomfort about the error message a little.  It
> is not actually needed to support any ways of working I engage in.
> 
> If stdout is redirected, this is probably still making the wrong
> choice; isatty(STDOUT_FILENO) might be a more useful datum to use.
> But it does not seem worth complicating the logic further.
> 
>  editor.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/editor.c b/editor.c
> index 941c0b2..3f13751 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -4,19 +4,19 @@
>  
>  int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
>  {
> -	const char *editor, *terminal;
> +	const char *editor = getenv("GIT_EDITOR");
> +	const char *terminal = getenv("TERM");
> +	int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
>  
> -	editor = getenv("GIT_EDITOR");
>  	if (!editor && editor_program)
>  		editor = editor_program;
> -	if (!editor)
> +	if (!editor && !terminal_is_dumb)
>  		editor = getenv("VISUAL");
>  	if (!editor)
>  		editor = getenv("EDITOR");
>  
> -	terminal = getenv("TERM");
> -	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
> -		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
> +	if (!editor && terminal_is_dumb)
> +		return error("terminal is dumb, but EDITOR unset");
>  
>  	if (!editor)
>  		editor = "vi";
> -- 
> 1.6.5.2
> 

[PATCH v4] Teach git var about GIT_EDITOR

[Jonathan Nieder]

Expose the command used by launch_editor() for scripts to use.
This should allow one to avoid searching for a proper editor
separately in each command.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
There was another typo in the patch I sent.  The paper-bag fix:

	diff -u b/var.c b/var.c
	--- b/var.c
	+++ b/var.c
	@@ -35,7 +35,7 @@
		const char *val;
	
		for (ptr = git_vars; ptr->read; ptr++)
	-		if ((val = ptr->read(0))
	+		if ((val = ptr->read(0)))
				printf("%s=%s\n", ptr->name, val);
	 }
	

Here’s an updated patch.  This one shouldn’t have any bugs (yeah, right).

Good night again,
Jonathan

 Documentation/git-var.txt |    8 ++++++++
 cache.h                   |    1 +
 editor.c                  |   14 ++++++++++++--
 var.c                     |   16 +++++++++++++++-
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index e2f4c09..89e4b4f 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -36,6 +36,14 @@ GIT_AUTHOR_IDENT::
 GIT_COMMITTER_IDENT::
     The person who put a piece of code into git.
 
+GIT_EDITOR::
+    Text editor for use by git commands.  The value is meant to be
+    interpreted by the shell when it is used.  Examples: `~/bin/vi`,
+    `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
+    --nofork`.  The order of preference is the `$GIT_EDITOR`
+    environment variable, then `core.editor` configuration, then
+    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 96840c7..311cfe1 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,7 @@ extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
+extern const char *git_editor(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/editor.c b/editor.c
index 3f13751..4f98b72 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
 	const char *terminal = getenv("TERM");
@@ -16,11 +16,21 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		editor = getenv("EDITOR");
 
 	if (!editor && terminal_is_dumb)
-		return error("terminal is dumb, but EDITOR unset");
+		return NULL;
 
 	if (!editor)
 		editor = "vi";
 
+	return editor;
+}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor = git_editor();
+
+	if (!editor)
+		return error("terminal is dumb, but EDITOR unset");
+
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
diff --git a/var.c b/var.c
index dacbaab..a303757 100644
--- a/var.c
+++ b/var.c
@@ -8,6 +8,16 @@
 
 static const char var_usage[] = "git var [-l | <variable>]";
 
+static const char *editor(int flag)
+{
+	const char *pgm = git_editor();
+
+	if (!pgm && flag & IDENT_ERROR_ON_NO_NAME)
+		die("terminal is dumb, but EDITOR unset");
+
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -15,14 +25,18 @@ struct git_var {
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
+	{ "GIT_EDITOR", editor },
 	{ "", NULL },
 };
 
 static void list_vars(void)
 {
 	struct git_var *ptr;
+	const char *val;
+
 	for (ptr = git_vars; ptr->read; ptr++)
-		printf("%s=%s\n", ptr->name, ptr->read(0));
+		if ((val = ptr->read(0)))
+			printf("%s=%s\n", ptr->name, val);
 }
 
 static const char *read_var(const char *var)
-- 
1.6.5.2

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

[Johannes Sixt]

Jonathan Nieder schrieb:
> From: Johannes Sixt <j6t@kdbg.org>
> 
> Expose the command used by launch_editor() for scripts to use...
 >
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

This patch has grown far beyond my original submission. I can't validly 
sign it off anymore. Please take authorship ;)

-- Hannes

Re: [PATCH 7/8] Provide a build time default-editor setting

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>  
>>> +test_expect_success 'does editor have a simple name (no slashes, etc)?' '
>>> +
>>> +	editor=$(TERM=vt100 git var GIT_EDITOR) &&
>>> +	test -n "$editor" &&
>>> +	simple=t &&
>>> +	case "$editor" in
>>> +	*/* | core_editor | [A-Z]*)
>> 
>> Hmm, what are the latter two cases designed to catch?
>
> Both are meant to allow the test to work without too many changes.

Honestly speaking, my preference is to see if the built-in editor is
exactly spelled as 'v' 'i', and skip this test altogether if it isn't.
Then the patch only needs to insert these lines (and reword "default
editor name too complicated" to "using customized default editor") without
touching the rest.  It simply does not look worth the complication.

You _might_ be able to skip only the "vi" part of the test when you see
that the built-in default is customized, though.  I didn't look closely
enough.

> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> ...
> +unset EDITOR VISUAL GIT_EDITOR
> +
> +test_expect_success 'determine default editor' '
> +
> +	editor=$(TERM=vt100 git var GIT_EDITOR) &&
> +	test -n "$editor"
> +
> +'
> +
> +if ! test -z "$(printf '%s\n' "$editor" | sed '/^[a-z]*$/d')"
> +then
> +	say 'skipping editor tests, default editor name too complicated'
> +	test_done
> +fi
> +

Re: [PATCH 7/8] Provide a build time default-editor setting

[Jonathan Nieder]

Junio C Hamano wrote:

> Honestly speaking, my preference is to see if the built-in editor is
> exactly spelled as 'v' 'i', and skip this test altogether if it isn't.

That does make sense.  But let me try one last time, before doing
that.  (I should have sat down and thought this through carefully
before sending the first version --- sorry.)

Though the first two iterations of the patch were pretty ugly, the
third was just 's/vi/"$editor"/g' after setting editor and bailing out
if it does not consist of lowercase letters.  As you mentioned, it
makes more sense to skip only the "vi" part of the test.

Tested with DEFAULT_EDITOR=vi, vim, /usr/bin/nonexistent.

 t/t7005-editor.sh |   37 +++++++++++++++++++++++++------------
 1 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index a95fe19..5257f4d 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -4,7 +4,21 @@ test_description='GIT_EDITOR, core.editor, and stuff'
 
 . ./test-lib.sh
 
-for i in GIT_EDITOR core_editor EDITOR VISUAL vi
+unset EDITOR VISUAL GIT_EDITOR
+
+test_expect_success 'determine default editor' '
+
+	vi=$(TERM=vt100 git var GIT_EDITOR) &&
+	test -n "$vi"
+
+'
+
+if ! expr "$vi" : '^[a-z]*$' >/dev/null
+then
+	vi=
+fi
+
+for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
 do
 	cat >e-$i.sh <<-EOF
 	#!$SHELL_PATH
@@ -12,19 +26,18 @@ do
 	EOF
 	chmod +x e-$i.sh
 done
-unset vi
-mv e-vi.sh vi
-unset EDITOR VISUAL GIT_EDITOR
+
+if ! test -z "$vi"
+then
+	mv e-$vi.sh $vi
+fi
 
 test_expect_success setup '
 
-	msg="Hand edited" &&
+	msg="Hand-edited" &&
+	test_commit "$msg" &&
 	echo "$msg" >expect &&
-	git add vi &&
-	test_tick &&
-	git commit -m "$msg" &&
-	git show -s --pretty=oneline |
-	sed -e "s/^[0-9a-f]* //" >actual &&
+	git show -s --format=%s > actual &&
 	diff actual expect
 
 '
@@ -54,7 +67,7 @@ test_expect_success 'dumb should prefer EDITOR to VISUAL' '
 
 TERM=vt100
 export TERM
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	unset EDITOR VISUAL GIT_EDITOR
@@ -78,7 +91,7 @@ done
 
 unset EDITOR VISUAL GIT_EDITOR
 git config --unset-all core.editor
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	case "$i" in

Re: [PATCH v4] Teach git var about GIT_EDITOR

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Expose the command used by launch_editor() for scripts to use.
> This should allow one to avoid searching for a proper editor
> separately in each command.

Ok, so the idea is...

 * git_editor(void) uses the logic to decide which editor to use that used
   to live in launch_editor().  The function returns NULL if there is no
   suitable editor; the caller is expected to issue an error message when
   appropriate.

 * launch_editor() uses git_editor() and gives the error message the same
   way as before when EDITOR is not set.

 * "git var GIT_EDITOR" gives the editor name, or an error message when
   there is no appropriate one.

 * "git var -l" gives GIT_EDITOR=name only if there is an appropriate
   editor.

The above all look sensible, but IIRC, the true "vi" fell back on "ex"
mode on dumb terminals and was usable as a line editor, so we should be
able to run it even on dumb terminals.  I do not know about vi-clones
though.

Re: [PATCH 7/8] Provide a build time default-editor setting

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> +unset EDITOR VISUAL GIT_EDITOR
> +
> +test_expect_success 'determine default editor' '
> +
> +	vi=$(TERM=vt100 git var GIT_EDITOR) &&
> +	test -n "$vi"
> +
> +'
> +
> +if ! expr "$vi" : '^[a-z]*$' >/dev/null
> +then
> +	vi=
> +fi
> +
> +for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
> ...
> -for i in vi EDITOR VISUAL core_editor GIT_EDITOR
> +for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
>  do
>  	echo "Edited by $i" >expect
>  	unset EDITOR VISUAL GIT_EDITOR
> @@ -78,7 +91,7 @@ done
>  
>  unset EDITOR VISUAL GIT_EDITOR
>  git config --unset-all core.editor
> -for i in vi EDITOR VISUAL core_editor GIT_EDITOR
> +for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
>  do

Beautiful ;-)

[PATCH v4 0/9] Default pager and editor

[Jonathan Nieder]

Junio C Hamano wrote: 

> * jn/editor-pager (2009-10-30) 8 commits
>  - Provide a build time default-pager setting
>  - Provide a build time default-editor setting
>  - am -i, git-svn: use "git var GIT_PAGER"
>  - add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR"
>  - Teach git var about GIT_PAGER
>  - Teach git var about GIT_EDITOR
>  - Do not use VISUAL editor on dumb terminals
>  - Handle more shell metacharacters in editor names
> 
> Any comments?

Here’s a reroll.  The interdiff is very small:

diff --git a/ident.c b/ident.c
index 99f1c85..26409b2 100644
--- a/ident.c
+++ b/ident.c
@@ -205,7 +205,7 @@ const char *fmt_ident(const char *name, const char *email,
 		if ((warn_on_no_name || error_on_no_name) &&
 		    name == git_default_name && env_hint) {
 			fprintf(stderr, env_hint, au_env, co_env);
-			env_hint = NULL; /* warn only once, for "git var -l" */
+			env_hint = NULL; /* warn only once */
 		}
 		if (error_on_no_name)
 			die("empty ident %s <%s> not allowed", name, email);

and the corresponding hunk in patch 3 could be safely discarded.
Aside from that, patch 3 has been unsquashed from patch 4, since it is
an independent fix that might be worth ejecting; the Signed-off-by
lines on patches 2 and 4 have been fixed; and the commit message for
patch 4 has been expanded to explain more.

In short, nothing of substance has changed.  If you are reminded of
any thoughts on the series, please let me know.

I think it is fair to say every one of these patches except the first
was someone else’s idea.  Thanks, everyone.

Jonathan Nieder (8):
  Handle more shell metacharacters in editor names
  Do not use VISUAL editor on dumb terminals
  Suppress warnings from "git var -l"
  Teach git var about GIT_EDITOR
  Teach git var about GIT_PAGER
  add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR"
  am -i, git-svn: use "git var GIT_PAGER"
  Provide a build time default-editor setting

Junio C Hamano (1):
  Provide a build time default-pager setting

 Documentation/config.txt         |    4 +--
 Documentation/git-commit.txt     |    2 +-
 Documentation/git-send-email.txt |    4 +-
 Documentation/git-var.txt        |   14 +++++++++++
 Makefile                         |   28 ++++++++++++++++++++++
 cache.h                          |    2 +
 contrib/fast-import/git-p4       |    5 +---
 editor.c                         |   32 ++++++++++++++++++-------
 git-add--interactive.perl        |    3 +-
 git-am.sh                        |    5 +++-
 git-send-email.perl              |    3 +-
 git-sh-setup.sh                  |   19 +++++----------
 git-svn.perl                     |   11 +++-----
 ident.c                          |    2 +-
 pager.c                          |   24 ++++++++++++++++---
 t/t7005-editor.sh                |   47 ++++++++++++++++++++++++++++---------
 t/t7501-commit.sh                |    8 +++---
 t/test-lib.sh                    |    8 +++---
 var.c                            |   26 ++++++++++++++++++++-
 19 files changed, 178 insertions(+), 69 deletions(-)

[PATCH 1/9] Handle more shell metacharacters in editor names

[Jonathan Nieder]

Pass the editor name to the shell if it contains any susv3 shell
special character (globs, redirections, variable substitutions,
escapes, etc).  This way, the meaning of some characters will not
meaninglessly change when others are added, and git commands
implemented in C and in shell scripts will interpret editor names
in the same way.

This does not make the GIT_EDITOR setting any more expressive,
since one could always use single quotes to force the editor to
be passed to the shell.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Unchanged from jn/editor-pager, included only for reference.

 editor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/editor.c b/editor.c
index 4d469d0..941c0b2 100644
--- a/editor.c
+++ b/editor.c
@@ -28,7 +28,7 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		const char *args[6];
 		struct strbuf arg0 = STRBUF_INIT;
 
-		if (strcspn(editor, "$ \t'") != len) {
+		if (strcspn(editor, "|&;<>()$`\\\"' \t\n*?[#~=%") != len) {
 			/* there are specials */
 			strbuf_addf(&arg0, "%s \"$@\"", editor);
 			args[i++] = "sh";
-- 
1.6.5.2

[PATCH 2/9] Do not use VISUAL editor on dumb terminals

[Jonathan Nieder]

Refuse to use $VISUAL and fall back to $EDITOR if TERM is unset
or set to "dumb".  Traditionally, VISUAL is set to a screen
editor and EDITOR to a line-based editor, which should be more
useful in that situation.

vim, for example, is happy to assume a terminal supports ANSI
sequences even if TERM is dumb (e.g., when running from a text
editor like Acme).  git already refuses to fall back to vi on a
dumb terminal if GIT_EDITOR, core.editor, VISUAL, and EDITOR are
unset, but without this patch, that check is suppressed by
VISUAL=vi.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Fixes broken sign-off, patch unchanged.

I am personally most interested in this for usage from a text editor,
but vim does not set TERM=dumb like it probably ought to.  A more
realistic everyday example might be "ssh user@domain git commit".

 editor.c          |   12 ++++++------
 t/t7005-editor.sh |   10 ++++++++++
 t/t7501-commit.sh |    8 ++++----
 t/test-lib.sh     |    8 ++++----
 4 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/editor.c b/editor.c
index 941c0b2..3f13751 100644
--- a/editor.c
+++ b/editor.c
@@ -4,19 +4,19 @@
 
 int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
 {
-	const char *editor, *terminal;
+	const char *editor = getenv("GIT_EDITOR");
+	const char *terminal = getenv("TERM");
+	int terminal_is_dumb = !terminal || !strcmp(terminal, "dumb");
 
-	editor = getenv("GIT_EDITOR");
 	if (!editor && editor_program)
 		editor = editor_program;
-	if (!editor)
+	if (!editor && !terminal_is_dumb)
 		editor = getenv("VISUAL");
 	if (!editor)
 		editor = getenv("EDITOR");
 
-	terminal = getenv("TERM");
-	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
-		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
+	if (!editor && terminal_is_dumb)
+		return error("terminal is dumb, but EDITOR unset");
 
 	if (!editor)
 		editor = "vi";
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index b647957..a95fe19 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -42,6 +42,16 @@ test_expect_success 'dumb should error out when falling back on vi' '
 	fi
 '
 
+test_expect_success 'dumb should prefer EDITOR to VISUAL' '
+
+	EDITOR=./e-EDITOR.sh &&
+	VISUAL=./e-VISUAL.sh &&
+	export EDITOR VISUAL &&
+	git commit --amend &&
+	test "$(git show -s --format=%s)" = "Edited by EDITOR"
+
+'
+
 TERM=vt100
 export TERM
 for i in vi EDITOR VISUAL core_editor GIT_EDITOR
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d2de576..a603f6d 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -86,7 +86,7 @@ chmod 755 editor
 
 test_expect_success \
 	"amend commit" \
-	"VISUAL=./editor git commit --amend"
+	"EDITOR=./editor git commit --amend"
 
 test_expect_success \
 	"passing -m and -F" \
@@ -107,7 +107,7 @@ chmod 755 editor
 test_expect_success \
 	"editing message from other commit" \
 	"echo 'hula hula' >file && \
-	 VISUAL=./editor git commit -c HEAD^ -a"
+	 EDITOR=./editor git commit -c HEAD^ -a"
 
 test_expect_success \
 	"message from stdin" \
@@ -141,10 +141,10 @@ EOF
 test_expect_success \
 	'editor not invoked if -F is given' '
 	 echo "moo" >file &&
-	 VISUAL=./editor git commit -a -F msg &&
+	 EDITOR=./editor git commit -a -F msg &&
 	 git show -s --pretty=format:"%s" | grep -q good &&
 	 echo "quack" >file &&
-	 echo "Another good message." | VISUAL=./editor git commit -a -F - &&
+	 echo "Another good message." | EDITOR=./editor git commit -a -F - &&
 	 git show -s --pretty=format:"%s" | grep -q good
 	 '
 # We could just check the head sha1, but checking each commit makes it
diff --git a/t/test-lib.sh b/t/test-lib.sh
index f2ca536..ec3336a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -30,7 +30,7 @@ TZ=UTC
 TERM=dumb
 export LANG LC_ALL PAGER TERM TZ
 EDITOR=:
-VISUAL=:
+unset VISUAL
 unset GIT_EDITOR
 unset AUTHOR_DATE
 unset AUTHOR_EMAIL
@@ -58,7 +58,7 @@ GIT_MERGE_VERBOSITY=5
 export GIT_MERGE_VERBOSITY
 export GIT_AUTHOR_EMAIL GIT_AUTHOR_NAME
 export GIT_COMMITTER_EMAIL GIT_COMMITTER_NAME
-export EDITOR VISUAL
+export EDITOR
 GIT_TEST_CMP=${GIT_TEST_CMP:-diff -u}
 
 # Protect ourselves from common misconfiguration to export
@@ -207,8 +207,8 @@ trap 'die' EXIT
 test_set_editor () {
 	FAKE_EDITOR="$1"
 	export FAKE_EDITOR
-	VISUAL='"$FAKE_EDITOR"'
-	export VISUAL
+	EDITOR='"$FAKE_EDITOR"'
+	export EDITOR
 }
 
 test_tick () {
-- 
1.6.5.2

[PATCH 3/9] Suppress warnings from "git var -l"

[Jonathan Nieder]

For scripts using "git var -l" to read all logical variables at
once, not all per-variable warnings will be relevant.  So suppress
them.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This is a separate issue that might even deserve to be ejected
from the series.

Changes from jn/editor-pager: unsquashed from the next patch, added
back comment-changing hunk.  Of course, there’s no harm in omitting
the comment change, but it describes a change in reality: before this
patch, that code gets run multiple times by "git var -l"; afterwards,
by no one (except possible out-of-tree users).

 ident.c |    2 +-
 var.c   |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ident.c b/ident.c
index 99f1c85..26409b2 100644
--- a/ident.c
+++ b/ident.c
@@ -205,7 +205,7 @@ const char *fmt_ident(const char *name, const char *email,
 		if ((warn_on_no_name || error_on_no_name) &&
 		    name == git_default_name && env_hint) {
 			fprintf(stderr, env_hint, au_env, co_env);
-			env_hint = NULL; /* warn only once, for "git var -l" */
+			env_hint = NULL; /* warn only once */
 		}
 		if (error_on_no_name)
 			die("empty ident %s <%s> not allowed", name, email);
diff --git a/var.c b/var.c
index 125c0d1..dacbaab 100644
--- a/var.c
+++ b/var.c
@@ -22,7 +22,7 @@ static void list_vars(void)
 {
 	struct git_var *ptr;
 	for (ptr = git_vars; ptr->read; ptr++)
-		printf("%s=%s\n", ptr->name, ptr->read(IDENT_WARN_ON_NO_NAME));
+		printf("%s=%s\n", ptr->name, ptr->read(0));
 }
 
 static const char *read_var(const char *var)
-- 
1.6.5.2

[PATCH 4/9] Teach git var about GIT_EDITOR

[Jonathan Nieder]

Expose the command used by launch_editor() for scripts to use.
This should allow one to avoid searching for a proper editor
separately in each command.

git_editor(void) uses the logic to decide which editor to use
that used to live in launch_editor().  The function returns NULL
if there is no suitable editor; the caller is expected to issue
an error message when appropriate.

launch_editor() uses git_editor() and gives the error message the
same way as before when EDITOR is not set.

"git var GIT_EDITOR" gives the editor name, or an error message
when there is no appropriate one.

"git var -l" gives GIT_EDITOR=name only if there is an
appropriate editor.

Originally-submitted-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Changes from the version in pu:
 * unsquashed with the previous patch;
 * replaces Hannes’s sign-off with something more descriptive (see
<http://thread.gmane.org/gmane.comp.version-control.git/131471/focus=131851>);
 * nicer commit message based on Junio’s summary.

 Documentation/git-var.txt |    8 ++++++++
 cache.h                   |    1 +
 editor.c                  |   14 ++++++++++++--
 var.c                     |   16 +++++++++++++++-
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index e2f4c09..89e4b4f 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -36,6 +36,14 @@ GIT_AUTHOR_IDENT::
 GIT_COMMITTER_IDENT::
     The person who put a piece of code into git.
 
+GIT_EDITOR::
+    Text editor for use by git commands.  The value is meant to be
+    interpreted by the shell when it is used.  Examples: `~/bin/vi`,
+    `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
+    --nofork`.  The order of preference is the `$GIT_EDITOR`
+    environment variable, then `core.editor` configuration, then
+    `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 96840c7..311cfe1 100644
--- a/cache.h
+++ b/cache.h
@@ -750,6 +750,7 @@ extern const char *git_author_info(int);
 extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
+extern const char *git_editor(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/editor.c b/editor.c
index 3f13751..70618f1 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
 	const char *terminal = getenv("TERM");
@@ -16,11 +16,21 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 		editor = getenv("EDITOR");
 
 	if (!editor && terminal_is_dumb)
-		return error("terminal is dumb, but EDITOR unset");
+		return NULL;
 
 	if (!editor)
 		editor = "vi";
 
+	return editor;
+}
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor = git_editor();
+
+	if (!editor)
+		return error("Terminal is dumb, but EDITOR unset");
+
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
diff --git a/var.c b/var.c
index dacbaab..b502487 100644
--- a/var.c
+++ b/var.c
@@ -8,6 +8,16 @@
 
 static const char var_usage[] = "git var [-l | <variable>]";
 
+static const char *editor(int flag)
+{
+	const char *pgm = git_editor();
+
+	if (!pgm && flag & IDENT_ERROR_ON_NO_NAME)
+		die("Terminal is dumb, but EDITOR unset");
+
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -15,14 +25,18 @@ struct git_var {
 static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
+	{ "GIT_EDITOR", editor },
 	{ "", NULL },
 };
 
 static void list_vars(void)
 {
 	struct git_var *ptr;
+	const char *val;
+
 	for (ptr = git_vars; ptr->read; ptr++)
-		printf("%s=%s\n", ptr->name, ptr->read(0));
+		if ((val = ptr->read(0)))
+			printf("%s=%s\n", ptr->name, val);
 }
 
 static const char *read_var(const char *var)
-- 
1.6.5.2

[PATCH 5/9] Teach git var about GIT_PAGER

[Jonathan Nieder]

Expose the command found by setup_pager() for scripts to use.
Scripts can use this to avoid repeating the logic to look for a
proper pager in each command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
The rest of the series is unchanged from pu.

 Documentation/git-var.txt |    6 ++++++
 cache.h                   |    1 +
 pager.c                   |   18 +++++++++++++++---
 var.c                     |   10 ++++++++++
 4 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 89e4b4f..ef6aa81 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt
@@ -44,6 +44,12 @@ GIT_EDITOR::
     environment variable, then `core.editor` configuration, then
     `$VISUAL`, then `$EDITOR`, and then finally 'vi'.
 
+GIT_PAGER::
+    Text viewer for use by git commands (e.g., 'less').  The value
+    is meant to be interpreted by the shell.  The order of preference
+    is the `$GIT_PAGER` environment variable, then `core.pager`
+    configuration, then `$PAGER`, and then finally 'less'.
+
 Diagnostics
 -----------
 You don't exist. Go away!::
diff --git a/cache.h b/cache.h
index 311cfe1..5aaa4ba 100644
--- a/cache.h
+++ b/cache.h
@@ -751,6 +751,7 @@ extern const char *git_committer_info(int);
 extern const char *fmt_ident(const char *name, const char *email, const char *date_str, int);
 extern const char *fmt_name(const char *name, const char *email);
 extern const char *git_editor(void);
+extern const char *git_pager(void);
 
 struct checkout {
 	const char *base_dir;
diff --git a/pager.c b/pager.c
index 86facec..0b63d99 100644
--- a/pager.c
+++ b/pager.c
@@ -44,12 +44,14 @@ static void wait_for_pager_signal(int signo)
 	raise(signo);
 }
 
-void setup_pager(void)
+const char *git_pager(void)
 {
-	const char *pager = getenv("GIT_PAGER");
+	const char *pager;
 
 	if (!isatty(1))
-		return;
+		return NULL;
+
+	pager = getenv("GIT_PAGER");
 	if (!pager) {
 		if (!pager_program)
 			git_config(git_default_config, NULL);
@@ -60,6 +62,16 @@ void setup_pager(void)
 	if (!pager)
 		pager = "less";
 	else if (!*pager || !strcmp(pager, "cat"))
+		pager = NULL;
+
+	return pager;
+}
+
+void setup_pager(void)
+{
+	const char *pager = git_pager();
+
+	if (!pager)
 		return;
 
 	spawned_pager = 1; /* means we are emitting to terminal */
diff --git a/var.c b/var.c
index b502487..d9892f8 100644
--- a/var.c
+++ b/var.c
@@ -18,6 +18,15 @@ static const char *editor(int flag)
 	return pgm;
 }
 
+static const char *pager(int flag)
+{
+	const char *pgm = git_pager();
+
+	if (!pgm)
+		pgm = "cat";
+	return pgm;
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);
@@ -26,6 +35,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_COMMITTER_IDENT", git_committer_info },
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
 	{ "GIT_EDITOR", editor },
+	{ "GIT_PAGER", pager },
 	{ "", NULL },
 };
 
-- 
1.6.5.2

[PATCH 6/9] add -i, send-email, svn, p4, etc: use "git var GIT_EDITOR"

[Jonathan Nieder]

Use the new "git var GIT_EDITOR" feature to decide what editor to
use, instead of duplicating its logic elsewhere.  This should make
the behavior of commands in edge cases (e.g., editor names with
spaces) a little more consistent.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Unchanged from pu.

 Documentation/config.txt         |    4 +---
 Documentation/git-commit.txt     |    2 +-
 Documentation/git-send-email.txt |    4 ++--
 contrib/fast-import/git-p4       |    5 +----
 git-add--interactive.perl        |    3 +--
 git-send-email.perl              |    3 ++-
 git-sh-setup.sh                  |   19 ++++++-------------
 git-svn.perl                     |    5 ++---
 8 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d1e2120..5181b77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -387,9 +387,7 @@ core.editor::
 	Commands such as `commit` and `tag` that lets you edit
 	messages by launching an editor uses the value of this
 	variable when it is set, and the environment variable
-	`GIT_EDITOR` is not set.  The order of preference is
-	`GIT_EDITOR` environment, `core.editor`, `VISUAL` and
-	`EDITOR` environment variables and then finally `vi`.
+	`GIT_EDITOR` is not set.  See linkgit:git-var[1].
 
 core.pager::
 	The command that git will use to paginate output.  Can
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0578a40..3ea80c8 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -323,7 +323,7 @@ ENVIRONMENT AND CONFIGURATION VARIABLES
 The editor used to edit the commit log message will be chosen from the
 GIT_EDITOR environment variable, the core.editor configuration variable, the
 VISUAL environment variable, or the EDITOR environment variable (in that
-order).
+order).  See linkgit:git-var[1] for details.
 
 HOOKS
 -----
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 767cf4d..c85d7f4 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -60,8 +60,8 @@ The --bcc option must be repeated for each user you want on the bcc list.
 The --cc option must be repeated for each user you want on the cc list.
 
 --compose::
-	Use $GIT_EDITOR, core.editor, $VISUAL, or $EDITOR to edit an
-	introductory message for the patch series.
+	Invoke a text editor (see GIT_EDITOR in linkgit:git-var[1])
+	to edit an introductory message for the patch series.
 +
 When '--compose' is used, git send-email will use the From, Subject, and
 In-Reply-To headers specified in the message. If the body of the message
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index e710219..48059d0 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -729,13 +729,10 @@ class P4Submit(Command):
             tmpFile.write(submitTemplate + separatorLine + diff + newdiff)
             tmpFile.close()
             mtime = os.stat(fileName).st_mtime
-            defaultEditor = "vi"
-            if platform.system() == "Windows":
-                defaultEditor = "notepad"
             if os.environ.has_key("P4EDITOR"):
                 editor = os.environ.get("P4EDITOR")
             else:
-                editor = os.environ.get("EDITOR", defaultEditor);
+                editor = read_pipe("git var GIT_EDITOR")
             system(editor + " " + fileName)
 
             response = "y"
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 69aeaf0..0c74e5c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -987,8 +987,7 @@ sub edit_hunk_manually {
 EOF
 	close $fh;
 
-	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
-		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	chomp(my $editor = run_cmd_pipe(qw(git var GIT_EDITOR)));
 	system('sh', '-c', $editor.' "$@"', $editor, $hunkfile);
 
 	if ($? != 0) {
diff --git a/git-send-email.perl b/git-send-email.perl
index a0279de..4f5da4e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -162,7 +162,8 @@ my $compose_filename;
 
 # Handle interactive edition of files.
 my $multiedit;
-my $editor = $ENV{GIT_EDITOR} || Git::config(@repo, "core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+my $editor = Git::command_oneline('var', 'GIT_EDITOR');
+
 sub do_edit {
 	if (defined($multiedit) && !$multiedit) {
 		map {
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index c41c2f7..99cceeb 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -99,19 +99,12 @@ set_reflog_action() {
 }
 
 git_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,"
-		echo >&2 "or EDITOR. Tried to fall back to vi but terminal is dumb."
-		echo >&2 "Please set one of these variables to an appropriate"
-		echo >&2 "editor or run $0 with options that will not cause an"
-		echo >&2 "editor to be invoked (e.g., -m or -F for git-commit)."
-		exit 1
-		;;
-	esac
-	eval "${GIT_EDITOR:=vi}" '"$@"'
+	if test -z "${GIT_EDITOR:+set}"
+	then
+		GIT_EDITOR="$(git var GIT_EDITOR)" || return $?
+	fi
+
+	eval "$GIT_EDITOR" '"$@"'
 }
 
 is_bare_repository () {
diff --git a/git-svn.perl b/git-svn.perl
index 6a3b501..42c9a72 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -1321,9 +1321,8 @@ sub get_commit_entry {
 	close $log_fh or croak $!;
 
 	if ($_edit || ($type eq 'tree')) {
-		my $editor = $ENV{VISUAL} || $ENV{EDITOR} || 'vi';
-		# TODO: strip out spaces, comments, like git-commit.sh
-		system($editor, $commit_editmsg);
+		chomp(my $editor = command_oneline(qw(var GIT_EDITOR)));
+		system('sh', '-c', $editor.' "$@"', $editor, $commit_editmsg);
 	}
 	rename $commit_editmsg, $commit_msg or croak $!;
 	{
-- 
1.6.5.2

[PATCH 7/9] am -i, git-svn: use "git var GIT_PAGER"

[Jonathan Nieder]

Use the new "git var GIT_PAGER" command to ask what pager to use.

Without this change, the core.pager configuration is ignored by
these commands.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Unchanged.

 git-am.sh    |    5 ++++-
 git-svn.perl |    6 ++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/git-am.sh b/git-am.sh
index c132f50..2649487 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -649,7 +649,10 @@ do
 		[eE]*) git_editor "$dotest/final-commit"
 		       action=again ;;
 		[vV]*) action=again
-		       LESS=-S ${PAGER:-less} "$dotest/patch" ;;
+		       : ${GIT_PAGER=$(git var GIT_PAGER)}
+		       : ${LESS=-FRSX}
+		       export LESS
+		       $GIT_PAGER "$dotest/patch" ;;
 		*)     action=again ;;
 		esac
 	    done
diff --git a/git-svn.perl b/git-svn.perl
index 42c9a72..c4ca548 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5171,10 +5171,8 @@ sub git_svn_log_cmd {
 
 # adapted from pager.c
 sub config_pager {
-	$pager ||= $ENV{GIT_PAGER} || $ENV{PAGER};
-	if (!defined $pager) {
-		$pager = 'less';
-	} elsif (length $pager == 0 || $pager eq 'cat') {
+	chomp(my $pager = command_oneline(qw(var GIT_PAGER)));
+	if ($pager eq 'cat') {
 		$pager = undef;
 	}
 	$ENV{GIT_PAGER_IN_USE} = defined($pager);
-- 
1.6.5.2

[PATCH 8/9] Provide a build time default-editor setting

[Jonathan Nieder]

Provide a DEFAULT_EDITOR knob to allow setting the fallback
editor to use instead of vi (when VISUAL, EDITOR, and GIT_EDITOR
are unset).  The value can be set at build time according to a
system’s policy.  For example, on Debian systems, the default
editor should be the 'editor' command.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Unchanged.

 Makefile          |   17 +++++++++++++++++
 editor.c          |    6 +++++-
 t/t7005-editor.sh |   37 +++++++++++++++++++++++++------------
 3 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/Makefile b/Makefile
index 268aede..625866c 100644
--- a/Makefile
+++ b/Makefile
@@ -200,6 +200,14 @@ all::
 # memory allocators with the nedmalloc allocator written by Niall Douglas.
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
+#
+# Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
+# want to use something different.  The value will be interpreted by the shell
+# if necessary when it is used.  Examples:
+#
+#   DEFAULT_EDITOR='~/bin/vi',
+#   DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
+#   DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
 	@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1363,6 +1371,15 @@ BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
 	$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
+# Quote for C
+
+ifdef DEFAULT_EDITOR
+DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
+DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/editor.c b/editor.c
index 70618f1..615f575 100644
--- a/editor.c
+++ b/editor.c
@@ -2,6 +2,10 @@
 #include "strbuf.h"
 #include "run-command.h"
 
+#ifndef DEFAULT_EDITOR
+#define DEFAULT_EDITOR "vi"
+#endif
+
 const char *git_editor(void)
 {
 	const char *editor = getenv("GIT_EDITOR");
@@ -19,7 +23,7 @@ const char *git_editor(void)
 		return NULL;
 
 	if (!editor)
-		editor = "vi";
+		editor = DEFAULT_EDITOR;
 
 	return editor;
 }
diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
index a95fe19..5257f4d 100755
--- a/t/t7005-editor.sh
+++ b/t/t7005-editor.sh
@@ -4,7 +4,21 @@ test_description='GIT_EDITOR, core.editor, and stuff'
 
 . ./test-lib.sh
 
-for i in GIT_EDITOR core_editor EDITOR VISUAL vi
+unset EDITOR VISUAL GIT_EDITOR
+
+test_expect_success 'determine default editor' '
+
+	vi=$(TERM=vt100 git var GIT_EDITOR) &&
+	test -n "$vi"
+
+'
+
+if ! expr "$vi" : '^[a-z]*$' >/dev/null
+then
+	vi=
+fi
+
+for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
 do
 	cat >e-$i.sh <<-EOF
 	#!$SHELL_PATH
@@ -12,19 +26,18 @@ do
 	EOF
 	chmod +x e-$i.sh
 done
-unset vi
-mv e-vi.sh vi
-unset EDITOR VISUAL GIT_EDITOR
+
+if ! test -z "$vi"
+then
+	mv e-$vi.sh $vi
+fi
 
 test_expect_success setup '
 
-	msg="Hand edited" &&
+	msg="Hand-edited" &&
+	test_commit "$msg" &&
 	echo "$msg" >expect &&
-	git add vi &&
-	test_tick &&
-	git commit -m "$msg" &&
-	git show -s --pretty=oneline |
-	sed -e "s/^[0-9a-f]* //" >actual &&
+	git show -s --format=%s > actual &&
 	diff actual expect
 
 '
@@ -54,7 +67,7 @@ test_expect_success 'dumb should prefer EDITOR to VISUAL' '
 
 TERM=vt100
 export TERM
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	unset EDITOR VISUAL GIT_EDITOR
@@ -78,7 +91,7 @@ done
 
 unset EDITOR VISUAL GIT_EDITOR
 git config --unset-all core.editor
-for i in vi EDITOR VISUAL core_editor GIT_EDITOR
+for i in $vi EDITOR VISUAL core_editor GIT_EDITOR
 do
 	echo "Edited by $i" >expect
 	case "$i" in
-- 
1.6.5.2

[PATCH 9/9] Provide a build time default-pager setting

[Jonathan Nieder]

Provide a DEFAULT_PAGER knob so packagers can set the fallback
pager to something appropriate during the build.

Examples:

On (old) solaris systems, /usr/bin/less (typically the first less
found) doesn't understand the default arguments (FXRS), which
forces users to alter their environment (PATH, GIT_PAGER, LESS,
etc) or have a local or global gitconfig before paging works as
expected.

On Debian systems, by policy packages must fall back to the
'pager' command, so that changing the target of the
/usr/bin/pager symlink changes the default pager for all packages
at once.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
As in pu.

 Makefile |   11 +++++++++++
 pager.c  |    6 +++++-
 2 files changed, 16 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index 625866c..18fc50a 100644
--- a/Makefile
+++ b/Makefile
@@ -201,6 +201,10 @@ all::
 #
 # Define NO_REGEX if you have no or inferior regex support in your C library.
 #
+# Define DEFAULT_PAGER to a sensible pager command (defaults to "less") if
+# you want to use something different.  The value will be interpreted by the
+# shell at runtime when it is used.
+#
 # Define DEFAULT_EDITOR to a sensible editor command (defaults to "vi") if you
 # want to use something different.  The value will be interpreted by the shell
 # if necessary when it is used.  Examples:
@@ -1380,6 +1384,13 @@ DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
 BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
 endif
 
+ifdef DEFAULT_PAGER
+DEFAULT_PAGER_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_PAGER)))"
+DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
+
+BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
+endif
+
 ALL_CFLAGS += $(BASIC_CFLAGS)
 ALL_LDFLAGS += $(BASIC_LDFLAGS)
 
diff --git a/pager.c b/pager.c
index 0b63d99..92c03f6 100644
--- a/pager.c
+++ b/pager.c
@@ -2,6 +2,10 @@
 #include "run-command.h"
 #include "sigchain.h"
 
+#ifndef DEFAULT_PAGER
+#define DEFAULT_PAGER "less"
+#endif
+
 /*
  * This is split up from the rest of git so that we can do
  * something different on Windows.
@@ -60,7 +64,7 @@ const char *git_pager(void)
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!pager)
-		pager = "less";
+		pager = DEFAULT_PAGER;
 	else if (!*pager || !strcmp(pager, "cat"))
 		pager = NULL;
 
-- 
1.6.5.2

Re: [PATCH v4 0/9] Default pager and editor

[Junio C Hamano]

Thanks, re-queued.

I've been sick and in bed for the past few days, so apologies for a late
reply.