[PATCH (REVISED)] Add core.editor configuration variable

[PATCH (REVISED)] Add core.editor configuration variable

[Adam Roben]

This variable lets you specify an editor that will be launched in preference to
the EDITOR and VISUAL environment variables.

Signed-off-by: Adam Roben <aroben@apple.com>
---
On Jul 18, 2007, at 11:23 PM, Shawn O. Pearce wrote:
> We only launch an editor for three reasons: commit messages, tag
> messages and git-rebase -i.  If we were to ever add a new editor
> using thingy, odds are the user would want the same editor by
> default for that too.

   Well, it turns out we already do launch an editor in other places, namely
"git am -i" and "git send-email --compose". So, this patch takes care of those
cases as well.

   My original intent had been to have an editor that really was *just* for
commit messages, but I realize now that that is probably something that not too
many people want, whereas core.editor is much more generally useful. Thanks
Junio and Shawn for the advice.

 Documentation/git-commit.txt     |    9 +++++----
 Documentation/git-send-email.txt |    4 ++--
 git-am.sh                        |    2 +-
 git-commit.sh                    |   13 +++++++------
 git-rebase--interactive.sh       |    2 +-
 git-send-email.perl              |    3 +--
 git-tag.sh                       |    2 +-
 7 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f96142f..5caad56 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -244,10 +244,11 @@ on the Subject: line and the rest of the commit in the body.
 
 include::i18n.txt[]
 
-ENVIRONMENT VARIABLES
----------------------
-The command specified by either the VISUAL or EDITOR environment
-variables is used to edit the commit log message.
+ENVIRONMENT AND CONFIGURATION VARIABLES
+---------------------------------------
+The editor used to edit the commit log message will be chosen from the
+core.editor configuration variable, the VISUAL environment variable, or the
+EDITOR environment variable (in that order).
 
 HOOKS
 -----
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index 293686c..e7723c9 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -44,8 +44,8 @@ The --cc option must be repeated for each user you want on the cc list.
 	value; if that is unspecified, default to --chain-reply-to.
 
 --compose::
-	Use $EDITOR to edit an introductory message for the
-	patch series.
+	Use core.editor, $VISUAL, or $EDITOR to edit an introductory message
+	for the patch series.
 
 --from::
 	Specify the sender of the emails.  This will default to
diff --git a/git-am.sh b/git-am.sh
index e5e6f2c..3a651ae 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -364,7 +364,7 @@ do
 		[yY]*) action=yes ;;
 		[aA]*) action=yes interactive= ;;
 		[nN]*) action=skip ;;
-		[eE]*) "${VISUAL:-${EDITOR:-vi}}" "$dotest/final-commit"
+		[eE]*) "$(git config core.editor || echo ${VISUAL:-${EDITOR:-vi}})" "$dotest/final-commit"
 		       action=again ;;
 		[vV]*) action=again
 		       LESS=-S ${PAGER:-less} "$dotest/patch" ;;
diff --git a/git-commit.sh b/git-commit.sh
index 3f3de17..72e4cf0 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -544,18 +544,19 @@ fi
 
 case "$no_edit" in
 '')
-	case "${VISUAL:-$EDITOR},$TERM" in
+	commit_editor=$(git config core.editor || echo ${VISUAL:-$EDITOR})
+	case "$commit_editor,$TERM" in
 	,dumb)
-		echo >&2 "Terminal is dumb but no VISUAL nor EDITOR defined."
-		echo >&2 "Please supply the commit log message using either"
-		echo >&2 "-m or -F option.  A boilerplate log message has"
-		echo >&2 "been prepared in $GIT_DIR/COMMIT_EDITMSG"
+		echo >&2 "Terminal is dumb but core.editor, VISUAL, and EDITOR"
+		echo >&2 "are undefined. Please supply the commit log message"
+		echo >&2 "using either -m or -F option.  A boilerplate log message"
+		echo >&2 "has been prepared in $GIT_DIR/COMMIT_EDITMSG"
 		exit 1
 		;;
 	esac
 	git-var GIT_AUTHOR_IDENT > /dev/null  || die
 	git-var GIT_COMMITTER_IDENT > /dev/null  || die
-	${VISUAL:-${EDITOR:-vi}} "$GIT_DIR/COMMIT_EDITMSG"
+	${commit_editor:-vi} "$GIT_DIR/COMMIT_EDITMSG"
 	;;
 esac
 
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f395076..2e15abb 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -414,7 +414,7 @@ EOF
 			die_abort "Nothing to do"
 
 		cp "$TODO" "$TODO".backup
-		${VISUAL:-${EDITOR:-vi}} "$TODO" ||
+		$(git config core.editor || echo ${VISUAL:-${EDITOR:-vi}}) "$TODO" ||
 			die "Could not execute editor"
 
 		test -z "$(grep -ve '^$' -e '^#' < $TODO)" &&
diff --git a/git-send-email.perl b/git-send-email.perl
index 7552cac..ad17b4e 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -341,8 +341,7 @@ GIT: for the patch you are writing.
 EOT
 	close(C);
 
-	my $editor = $ENV{EDITOR};
-	$editor = 'vi' unless defined $editor;
+	my $editor = $repo->config("core.editor") || $ENV{VISUAL} || $ENV{EDITOR} || "vi";
 	system($editor, $compose_filename);
 
 	open(C2,">",$compose_filename . ".final")
diff --git a/git-tag.sh b/git-tag.sh
index 1c25d88..9aa30b4 100755
--- a/git-tag.sh
+++ b/git-tag.sh
@@ -177,7 +177,7 @@ if [ "$annotate" ]; then
         ( echo "#"
           echo "# Write a tag message"
           echo "#" ) > "$GIT_DIR"/TAG_EDITMSG
-        ${VISUAL:-${EDITOR:-vi}} "$GIT_DIR"/TAG_EDITMSG || exit
+        $(git config core.editor || echo ${VISUAL:-${EDITOR:-vi}}) "$GIT_DIR"/TAG_EDITMSG || exit
     else
         printf '%s\n' "$message" >"$GIT_DIR"/TAG_EDITMSG
     fi
-- 
1.5.3.rc2.19.gc4fba-dirty

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Andy Parkins]

On Thursday 2007 July 19, Adam Roben wrote:

>    Well, it turns out we already do launch an editor in other places,
> namely "git am -i" and "git send-email --compose". So, this patch takes
> care of those cases as well.

Perhaps I'm being overly pedantic, but it seems odd to put options that are 
relevant only to porcelain under the "core" section.

core.pager is in the same category - but that's already in.

Would porcelain.editor be a better name for this variable?


Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Johannes Schindelin]

Hi,

On Thu, 19 Jul 2007, Andy Parkins wrote:

> On Thursday 2007 July 19, Adam Roben wrote:
> 
> >    Well, it turns out we already do launch an editor in other places,
> > namely "git am -i" and "git send-email --compose". So, this patch takes
> > care of those cases as well.
> 
> Perhaps I'm being overly pedantic, but it seems odd to put options that are 
> relevant only to porcelain under the "core" section.
> 
> core.pager is in the same category - but that's already in.
> 
> Would porcelain.editor be a better name for this variable?

>From my point of view you can put into "myWonderfulGit.editor".  It does 
not matter.

So why not just leave it in "core", also because -- strictly speaking -- 
those commands in git.git are not really your regular porcelain. They are 
tested with almost every commit, and therefore have a much better chance 
not to be broken inadvertently by changes to the core.

Ciao,
Dscho

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Andy Parkins]

On Thursday 2007 July 19, Johannes Schindelin wrote:
> Hi,
>
> On Thu, 19 Jul 2007, Andy Parkins wrote:
> > On Thursday 2007 July 19, Adam Roben wrote:
> > >    Well, it turns out we already do launch an editor in other places,
> > > namely "git am -i" and "git send-email --compose". So, this patch takes
> > > care of those cases as well.
> >
> > Perhaps I'm being overly pedantic, but it seems odd to put options that
> > are relevant only to porcelain under the "core" section.
> >
> > core.pager is in the same category - but that's already in.
> >
> > Would porcelain.editor be a better name for this variable?
>
> From my point of view you can put into "myWonderfulGit.editor".  It does
> not matter.

By that argument, why do we bother with subsections at all.  In fact why not 
call the variable "xhxhxjjjll.yqlaoospsp"?

They are meant to help the human remembering it.

> So why not just leave it in "core", also because -- strictly speaking --

That's what I said for pager - since it's already there.  However, since the 
editor isn't already in, it's not a question of _leaving_ it anywhere.  
Better that it goes in the right place when it is first introduced.

> those commands in git.git are not really your regular porcelain. They are
> tested with almost every commit, and therefore have a much better chance

So is every other porcelain command - almost by definition, porcelain commands 
are the ones we use.

> not to be broken inadvertently by changes to the core.

I don't understand the point you're making here.  Are you saying that we can 
rely on porcelain not to change or the core not to change?  I'm also 
wondering about these "inadvertent" changes that are getting made to git 
core - I've not noticed them before :-)



Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Johannes Schindelin]

Hi,

On Thu, 19 Jul 2007, Andy Parkins wrote:

> On Thursday 2007 July 19, Johannes Schindelin wrote:
> >
> > On Thu, 19 Jul 2007, Andy Parkins wrote:
> >
> > > Would porcelain.editor be a better name for this variable?
> >
> > From my point of view you can put into "myWonderfulGit.editor".  It 
> > does not matter.
> 
> By that argument, why do we bother with subsections at all.  In fact why 
> not call the variable "xhxhxjjjll.yqlaoospsp"?

No.  I said, and I quote here, "From my point of view".

> They are meant to help the human remembering it.
> 
> > So why not just leave it in "core", also because -- strictly speaking 
> > --
> 
> That's what I said for pager - since it's already there.  However, since 
> the editor isn't already in, it's not a question of _leaving_ it 
> anywhere.  Better that it goes in the right place when it is first 
> introduced.

And how would having "core.pager" but "porcelain.editor" be easier to 
remember?  Nah, not really.

Ciao,
Dscho

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Andy Parkins]

On Thursday 2007 July 19, Johannes Schindelin wrote:
> Hi,
>
> On Thu, 19 Jul 2007, Andy Parkins wrote:
> > On Thursday 2007 July 19, Johannes Schindelin wrote:
> > > On Thu, 19 Jul 2007, Andy Parkins wrote:
> > > > Would porcelain.editor be a better name for this variable?
> > >
> > > From my point of view you can put into "myWonderfulGit.editor".  It
> > > does not matter.
> >
> > By that argument, why do we bother with subsections at all.  In fact why
> > not call the variable "xhxhxjjjll.yqlaoospsp"?
>
> No.  I said, and I quote here, "From my point of view".

That doesn't change my point - these things are named to give meaning, they 
aren't just arbitrary strings of characters.

> And how would having "core.pager" but "porcelain.editor" be easier to
> remember?  Nah, not really.

If there is no difference, then do you object so strongly?

Besides, memory isn't just about having words, it's about meaning too.  
Categorisation and hierarchy are important.  If I'm searching my brain for a 
function that does something to strings then the fact that it starts 
with "str" gets me a long way there.  The fact that they _all_ start 
with "str" is what's important.

I don't care _that_ strongly; just like you it won't make any difference to me 
personally as I'll cope either way.  I'm trying to think like a noob, and it 
seems that coherency is broken when we make distinctions between porcelain 
and plumbing and then don't stick to them in the config file.

Perhaps I am wrong in my assumption: I have always thought of core.* options 
being those options which apply to plumbing - i.e. if I were a git-guru and 
did everything with plumbing I would still need those options.



Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Brian Gernhardt]

On Jul 19, 2007, at 3:48 AM, Andy Parkins wrote:

> On Thursday 2007 July 19, Adam Roben wrote:
>
>>    Well, it turns out we already do launch an editor in other places,
>> namely "git am -i" and "git send-email --compose". So, this patch  
>> takes
>> care of those cases as well.
>
> Perhaps I'm being overly pedantic, but it seems odd to put options  
> that are
> relevant only to porcelain under the "core" section.
>
> core.pager is in the same category - but that's already in.

Since I'm the one who added that, I feel compelled to add in my $0.02.

First, there wasn't much argument on the list when core.pager was  
added...  But that's not my major point.

To many git users (myself included), claiming that commands like git- 
commit, git-log, and other "porcelain" isn't part of "core" git is  
ridiculous.  These are the commands that I use every day, and are a  
part of the main git package, source, and repository.  If the  
porcelain was maintained separately from the pluming, like cognito  
was, perhaps that would be a more compelling argument to me.

For many people commit is more "core" to their git usage than write- 
tree and commit-tree.  At this point, it's less an extra layer  
porcelain and more the standard interface.  It's a result of the  
wonderful attempts to make git more user friendly.

As far as [core] being only for plumbing, I disagree with that  
premise as well.  Any option that is used across many of the git  
commands is a core (meaning central) option.

~~ Brian

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Andy Parkins]

On Thursday 2007 July 19, Brian Gernhardt wrote:

> As far as [core] being only for plumbing, I disagree with that
> premise as well.  Any option that is used across many of the git
> commands is a core (meaning central) option.

Fair enough.  With the definition core != plumbing; my argument has no weight.


Andy
-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Nicolas Pitre]

On Thu, 19 Jul 2007, Andy Parkins wrote:

> On Thursday 2007 July 19, Johannes Schindelin wrote:
> > And how would having "core.pager" but "porcelain.editor" be easier to
> > remember?  Nah, not really.
> 
> If there is no difference, then do you object so strongly?

If anything I think it is core.pager which is a misnommer.  A pager is 
certainly more porcelainish. by nature.

Given that core.pager already exists, I think it would be yet another 
mistake to put editor in a different section separate from pager.

IMHO both of them should have been user.pager and user.editor.


Nicolas

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Nicolas Pitre]

On Thu, 19 Jul 2007, Brian Gernhardt wrote:

> For many people commit is more "core" to their git usage than write-tree and
> commit-tree.  At this point, it's less an extra layer porcelain and more the
> standard interface.  It's a result of the wonderful attempts to make git more
> user friendly.
> 
> As far as [core] being only for plumbing, I disagree with that premise as
> well.  Any option that is used across many of the git commands is a core
> (meaning central) option.

That pov certainly makes sense to me.


Nicolas

Re: [PATCH (REVISED)] Add core.editor configuration variable

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Thu, 19 Jul 2007, Brian Gernhardt wrote:
>
>> For many people commit is more "core" to their git usage than write-tree and
>> commit-tree.  At this point, it's less an extra layer porcelain and more the
>> standard interface.  It's a result of the wonderful attempts to make git more
>> user friendly.
>> 
>> As far as [core] being only for plumbing, I disagree with that premise as
>> well.  Any option that is used across many of the git commands is a core
>> (meaning central) option.
>
> That pov certainly makes sense to me.

Good to see the discussion converged while I was trapped in the
day job.  So core.editor it is?