[PATCH] Add commit.infodisplay option to give message editor empty file

[PATCH] Add commit.infodisplay option to give message editor empty file

[James P. Howard, II]

This patch creates commit.infodisplay which causes git commit to
display the status information on the standard output rather
than in the temporary file for the commit message.  By doing
this, it becomes feasible to set core.editor for commit messages
to be a line editor, e.g. ex or ed.

Signed-off-by: James P. Howard, II <jh@jameshoward.us>
---
 Documentation/config.txt |    5 +++++
 builtin-commit.c         |   31 ++++++++++++++++++-------------
 cache.h                  |    1 +
 environment.c            |    3 +++
 4 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a1e36d7..56b3238 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -705,6 +705,11 @@ color.ui::
 	terminal. When more specific variables of color.* are set, they always
 	take precedence over this setting. Defaults to false.
 
+commit.infodisplay::
+	When true and a commit message is not specified on the command line,
+	the status information is not placed in the message template but is
+	printed to the standard output.
+
 commit.template::
 	Specify a file to use as the template for new commit messages.
 	"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
diff --git a/builtin-commit.c b/builtin-commit.c
index e93a647..e4db374 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -463,7 +463,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	int commitable, saved_color_setting;
 	struct strbuf sb = STRBUF_INIT;
 	char *buffer;
-	FILE *fp;
+	FILE *fp, *infofp;
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
 	int ident_shown = 0;
@@ -540,7 +540,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
 		die_errno("could not write commit template");
-
+	if (info_display)
+		infofp = stdout;
+	else {
+		infofp = fp;
+		fprintf(infofp, "\n");
+	}
 	strbuf_release(&sb);
 
 	determine_author_info();
@@ -552,7 +557,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		const char *committer_ident;
 
 		if (in_merge)
-			fprintf(fp,
+			fprintf(infofp,
 				"#\n"
 				"# It looks like you may be committing a MERGE.\n"
 				"# If this is not correct, please remove the file\n"
@@ -561,28 +566,27 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				"#\n",
 				git_path("MERGE_HEAD"));
 
-		fprintf(fp,
-			"\n"
+		fprintf(infofp,
 			"# Please enter the commit message for your changes.");
 		if (cleanup_mode == CLEANUP_ALL)
-			fprintf(fp,
+			fprintf(infofp,
 				" Lines starting\n"
 				"# with '#' will be ignored, and an empty"
 				" message aborts the commit.\n");
 		else /* CLEANUP_SPACE, that is. */
-			fprintf(fp,
+			fprintf(infofp,
 				" Lines starting\n"
 				"# with '#' will be kept; you may remove them"
 				" yourself if you want to.\n"
 				"# An empty message aborts the commit.\n");
 		if (only_include_assumed)
-			fprintf(fp, "# %s\n", only_include_assumed);
+			fprintf(infofp, "# %s\n", only_include_assumed);
 
 		author_ident = xstrdup(fmt_name(author_name, author_email));
 		committer_ident = fmt_name(getenv("GIT_COMMITTER_NAME"),
 					   getenv("GIT_COMMITTER_EMAIL"));
 		if (strcmp(author_ident, committer_ident))
-			fprintf(fp,
+			fprintf(infofp,
 				"%s"
 				"# Author:    %s\n",
 				ident_shown++ ? "" : "#\n",
@@ -590,18 +594,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		free(author_ident);
 
 		if (!user_ident_explicitly_given)
-			fprintf(fp,
+			fprintf(infofp,
 				"%s"
 				"# Committer: %s\n",
 				ident_shown++ ? "" : "#\n",
 				committer_ident);
 
 		if (ident_shown)
-			fprintf(fp, "#\n");
+			fprintf(infofp, "#\n");
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
-		commitable = run_status(fp, index_file, prefix, 1, s);
+		commitable = run_status(infofp, index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
 	} else {
 		unsigned char sha1[20];
@@ -1006,7 +1010,8 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 
 	if (!strcmp(k, "commit.template"))
 		return git_config_pathname(&template_file, k, v);
-
+	if (!strcmp(k, "commit.infodisplay"))
+		info_display = git_config_bool(k, v);
 	return git_status_config(k, v, s);
 }
 
diff --git a/cache.h b/cache.h
index bf468e5..2b36fb3 100644
--- a/cache.h
+++ b/cache.h
@@ -529,6 +529,7 @@ extern int auto_crlf;
 extern int read_replace_refs;
 extern int fsync_object_files;
 extern int core_preload_index;
+extern int info_display;
 
 enum safe_crlf {
 	SAFE_CRLF_FALSE = 0,
diff --git a/environment.c b/environment.c
index 5171d9f..ac7cfed 100644
--- a/environment.c
+++ b/environment.c
@@ -55,6 +55,9 @@ int grafts_replace_parents = 1;
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
 
+/* Controls whether commit information is appended to message text or displayed */
+int info_display = 0;
+
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
 static char *work_tree;
-- 
1.6.5.3

Re: [PATCH] Add commit.infodisplay option to give message editor empty file

[Junio C Hamano]

"James P. Howard, II" <jh@jameshoward.us> writes:

> This patch creates commit.infodisplay which causes git commit to
> display the status information on the standard output rather
> than in the temporary file for the commit message.  By doing
> this, it becomes feasible to set core.editor for commit messages
> to be a line editor, e.g. ex or ed.

Two points and a half:

 - Why does info_display need to be visible to everybody else by being in
   cache.h and environment.c?  Shouldn't its scope be similar to that of
   template_file, that is static to builtin-commit.c?

 - A configuration variable that does not allow the users to override from
   the command line is a no-no.  We usually do command line option first
   and then (or at the same time) configuration variable as a typesaver.

 - While the amount of change necessary for this change doesn't look too
   bad, is it really worth it?  What is wrong with "1,$p" while using ed
   as your editor?

Re: [PATCH] Add commit.infodisplay option to give message editor empty file

[James P. Howard, II]

On Fri, Dec 04, 2009 at 11:30:56PM -0800, Junio C Hamano wrote:

> Two points and a half:
> 
>  - Why does info_display need to be visible to everybody else by being in
>    cache.h and environment.c?  Shouldn't its scope be similar to that of
>    template_file, that is static to builtin-commit.c?
> 
>  - A configuration variable that does not allow the users to override from
>    the command line is a no-no.  We usually do command line option first
>    and then (or at the same time) configuration variable as a typesaver.

I can make these changes in the next few days.
 
>  - While the amount of change necessary for this change doesn't look too
>    bad, is it really worth it?  What is wrong with "1,$p" while using ed
>    as your editor?

I tried a few variants of this, and shell script wrappers for ex (and
gate, which is a specizalized text editor that actually works kind of
well for commit message editing), and it worked fairly well.

The real motivation is that this feature ditches all assumptions about
the capabilities of the text editor.  I am not sure where that leads
yet, but I'd rather make it possible.

James

-- 
James P. Howard, II, MPA MBCS
jh@jameshoward.us

Re: [PATCH] Add commit.infodisplay option to give message editor empty file

[Jeff King]

On Sat, Dec 05, 2009 at 10:47:53AM -0500, James P. Howard, II wrote:

> >  - While the amount of change necessary for this change doesn't look too
> >    bad, is it really worth it?  What is wrong with "1,$p" while using ed
> >    as your editor?
> 
> I tried a few variants of this, and shell script wrappers for ex (and
> gate, which is a specizalized text editor that actually works kind of
> well for commit message editing), and it worked fairly well.
> 
> The real motivation is that this feature ditches all assumptions about
> the capabilities of the text editor.  I am not sure where that leads
> yet, but I'd rather make it possible.

Is your problem that your editor doesn't show the template content and
you want to see it, or is it that your editor isn't pleasant to use when
the buffer is pre-filled with the template?

If the former, it seems like just dumping it to stdout isn't all that
satisfying, either. What happens when your editing causes the
information to scroll off the screen and you want to see it again?
Couldn't you get the same thing just by doing "git status; git commit"?

If the latter, I think we would be better served by an option to simply
turn off the template. Then that is also helpful for the case of people
using decent editors, but who don't want to waste the CPU time on
generating the template information (which can be substantial for things
like media repositories).

I suspect your answer will be that it is some of both, but this just
really feels like we are putting hacks into git because of one
featureless editor. Hacks like that would be better suited (IMHO) to a
wrapper script for the editor.

-Peff

Re: [PATCH] Add commit.infodisplay option to give message editor empty file

[James P. Howard, II]

On Sat, Dec 05, 2009 at 11:28:27AM -0500, Jeff King wrote:

> If the latter, I think we would be better served by an option to simply
> turn off the template. Then that is also helpful for the case of people
> using decent editors, but who don't want to waste the CPU time on
> generating the template information (which can be substantial for things
> like media repositories).

Actually, I find this a reasonable solution for both cases and would be
willing to reimplment my change this way, as it meets my needs and would
be useful to others.  The only question I have is, what should variable/
command line option be called?

James

-- 
James P. Howard, II, MPA MBCS
jh@jameshoward.us

Re: [PATCH] Add commit.infodisplay option to give message editor empty file

[Jeff King]

On Sat, Dec 05, 2009 at 06:09:03PM -0500, James P. Howard, II wrote:

> On Sat, Dec 05, 2009 at 11:28:27AM -0500, Jeff King wrote:
> 
> > If the latter, I think we would be better served by an option to simply
> > turn off the template. Then that is also helpful for the case of people
> > using decent editors, but who don't want to waste the CPU time on
> > generating the template information (which can be substantial for things
> > like media repositories).
> 
> Actually, I find this a reasonable solution for both cases and would be
> willing to reimplment my change this way, as it meets my needs and would
> be useful to others.  The only question I have is, what should variable/
> command line option be called?

I would be tempted to call it "--no-template", but I think that is too
confusing. The "--template" option is not really about the git-generated
template, but about a user-defined template that goes on top of the
git-generated one (I would have expected --template=/dev/null to do what
you want, too, but it retains the git template).

Probably "--no-status" would be a good name, as the generated template
is the format generated by "git status".

-Peff

Re: [PATCH] Add commit.infodisplay option to give message editor empty file

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I would be tempted to call it "--no-template", but I think that is too
> confusing. The "--template" option is not really about the git-generated
> template, but about a user-defined template that goes on top of the
> git-generated one (I would have expected --template=/dev/null to do what
> you want, too, but it retains the git template).
>
> Probably "--no-status" would be a good name, as the generated template
> is the format generated by "git status".

I wonder which part is the most expensive in generating the status
output.  Perhaps -suno is sufficient?

Re: [PATCH] Add commit.infodisplay option to give message editor empty file

[Jeff King]

On Sun, Dec 06, 2009 at 12:01:53AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I would be tempted to call it "--no-template", but I think that is too
> > confusing. The "--template" option is not really about the git-generated
> > template, but about a user-defined template that goes on top of the
> > git-generated one (I would have expected --template=/dev/null to do what
> > you want, too, but it retains the git template).
> >
> > Probably "--no-status" would be a good name, as the generated template
> > is the format generated by "git status".
> 
> I wonder which part is the most expensive in generating the status
> output.  Perhaps -suno is sufficient?

Speaking from my experience, it is doing break and rename detection on
large files, which there is currently no way to turn off (I hacked
around it with "$EDITOR msg && git commit --quiet -F msg").

Keep in mind this is one of my gigantic photo repositories, and the
commit in question made a minor change to almost 3G worth of files. So
it is not the end of the world for me to use the hack above on those
rare occasions. But since James wants it for other reasons, and it
should be a trivial patch, I think it would be nice for commit to
support it natively.

It would also make sense to me for "--quiet" to suppress the template,
but that is a behavior change that I suppose some people might not like.

-Peff

[PATCH] Add commit.status, --status, and --no-status

[James P. Howard, II]

This commit provides support for commit.status, --status, and
--no-status, which control whether or not the git status information
is included in the commit message template when using an editor to
prepare the commit message.  It does not affect the effects of a
user's commit.template settings.

Signed-off-by: James P. Howard, II <jh@jameshoward.us>
---
 Documentation/config.txt     |    5 +++++
 Documentation/git-commit.txt |   14 +++++++++++++-
 builtin-commit.c             |    9 +++++++--
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a1e36d7..5561560 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -705,6 +705,11 @@ color.ui::
 	terminal. When more specific variables of color.* are set, they always
 	take precedence over this setting. Defaults to false.
 
+commit.status
+	A boolean to enable/disable inclusion of status information in the
+	commit message template when using an editor to prepare the commit
+	message.  Defaults to true.
+
 commit.template::
 	Specify a file to use as the template for new commit messages.
 	"{tilde}/" is expanded to the value of `$HOME` and "{tilde}user/" to the
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index d227cec..0e53518 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 'git commit' [-a | --interactive] [-s] [-v] [-u<mode>] [--amend] [--dry-run]
 	   [(-c | -C) <commit>] [-F <file> | -m <msg>] [--reset-author]
 	   [--allow-empty] [--no-verify] [-e] [--author=<author>]
-	   [--cleanup=<mode>] [--] [[-i | -o ]<file>...]
+	   [--cleanup=<mode>] [--status | --no-status] [--]
+	   [[-i | -o ]<file>...]
 
 DESCRIPTION
 -----------
@@ -207,6 +208,17 @@ specified.
 	to be committed, paths with local changes that will be left
 	uncommitted and paths that are untracked.
 
+--status::
+	Include the output of linkgit:git-status[1] in the commit
+	message template when using an editor to prepare the commit
+	message.  Defaults to on, but can be used to override
+	configuration variable commit.status.
+
+--no-status::
+	Do not include the output of linkgit:git-status[1] in the
+	commit message template when using an editor to prepare the
+	default commit message.
+
 \--::
 	Do not interpret any more arguments as options.
 
diff --git a/builtin-commit.c b/builtin-commit.c
index e93a647..095c186 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -67,7 +67,7 @@ static enum {
 } cleanup_mode;
 static char *cleanup_arg;
 
-static int use_editor = 1, initial_commit, in_merge;
+static int use_editor = 1, initial_commit, in_merge, include_status = 1;
 static const char *only_include_assumed;
 static struct strbuf message;
 
@@ -97,6 +97,7 @@ static struct option builtin_commit_options[] = {
 	OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
 	OPT_FILENAME('t', "template", &template_file, "use specified template file"),
 	OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
+	OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
 
 	OPT_GROUP("Commit contents options"),
 	OPT_BOOLEAN('a', "all", &all, "commit all changed files"),
@@ -547,7 +548,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 	/* This checks if committer ident is explicitly given */
 	git_committer_info(0);
-	if (use_editor) {
+	if (use_editor && include_status) {
 		char *author_ident;
 		const char *committer_ident;
 
@@ -1006,6 +1007,10 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 
 	if (!strcmp(k, "commit.template"))
 		return git_config_pathname(&template_file, k, v);
+	if (!strcmp(k, "commit.status")) {
+		include_status = git_config_bool(k, v);
+		return 0;
+	}
 
 	return git_status_config(k, v, s);
 }
-- 
1.6.5.3

Re: [PATCH] Add commit.status, --status, and --no-status

[James Pickens]

On Mon, Dec 7, 2009, James P. Howard, II <jh@jameshoward.us> wrote:
> This commit provides support for commit.status, --status, and
> --no-status, which control whether or not the git status information
> is included in the commit message template when using an editor to
> prepare the commit message.  It does not affect the effects of a
> user's commit.template settings.

At the risk of sounding like a curmudgeon, I have to register an objection
to this _as a config option_, for the same reasons that I objected to the
'grep --full-tree' config option [1].  Both options fundamentally change
user visible behavior, both options IMHO will be used by a small minority
of git users, and in both cases, the desired behavior can be attained using
aliases in combination with a new command line option.

Sorry if I'm obstructing progress, but I think that, for the good of the
Git community as a whole, it's best to keep the number of config options
(and to a lesser extent, command line options) as low as possible.

Note that I do not object to adding --no-status as a command line option.

[1] http://article.gmane.org/gmane.comp.version-control.git/133681

James

Re: [PATCH] Add commit.status, --status, and --no-status

[Jeff King]

On Mon, Dec 07, 2009 at 05:45:27PM -0500, James P. Howard, II wrote:

> This commit provides support for commit.status, --status, and
> --no-status, which control whether or not the git status information
> is included in the commit message template when using an editor to
> prepare the commit message.  It does not affect the effects of a
> user's commit.template settings.

Thanks, this looks very cleanly done. The only complaint I would make is
that it should probably include a simple test case.

-Peff

Re: [PATCH] Add commit.status, --status, and --no-status

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Dec 07, 2009 at 05:45:27PM -0500, James P. Howard, II wrote:
>
>> This commit provides support for commit.status, --status, and
>> --no-status, which control whether or not the git status information
>> is included in the commit message template when using an editor to
>> prepare the commit message.  It does not affect the effects of a
>> user's commit.template settings.
>
> Thanks, this looks very cleanly done. The only complaint I would make is
> that it should probably include a simple test case.

Yes.  Also I am a _bit_ worried about the name "status", as the longer
term direction is to make "status" not "a preview of commit", may confuse
people who do read Release Notes.

Re: [PATCH] Add commit.status, --status, and --no-status

[Jeff King]

On Mon, Dec 07, 2009 at 11:13:00PM -0800, Junio C Hamano wrote:

> >> This commit provides support for commit.status, --status, and
> >> --no-status, which control whether or not the git status information
> >> is included in the commit message template when using an editor to
> >> prepare the commit message.  It does not affect the effects of a
> >> user's commit.template settings.
> >
> > Thanks, this looks very cleanly done. The only complaint I would make is
> > that it should probably include a simple test case.
> 
> Yes.  Also I am a _bit_ worried about the name "status", as the longer
> term direction is to make "status" not "a preview of commit", may confuse
> people who do read Release Notes.

I thought about that, but what other name does it have? That text has
always been called "status", and we will continue to support that output
format as "git status" _and_ as "commit --dry-run". So I think
explaining it as "usually we stick the output of 'git status' into the
commit message, but this suppresses it" is not that hard (and that was
how I read the documentation in his patch).

The only trick is that it is not a vanilla "git status", but rather
"status after we have staged things for commit". But I think that is
fairly obvious since you are, after all, calling "commit".

But then again, I am probably way too deep in this topic to provide a
regular git user's perspective of what is obvious.

-Peff

Re: [PATCH] Add commit.status, --status, and --no-status

[James P. Howard, II]

On Tue, Dec 8, 2009 at 02:13, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Mon, Dec 07, 2009 at 05:45:27PM -0500, James P. Howard, II wrote:
>>
>>> This commit provides support for commit.status, --status, and
>>> --no-status, which control whether or not the git status information
>>> is included in the commit message template when using an editor to
>>> prepare the commit message.  It does not affect the effects of a
>>> user's commit.template settings.
>>
>> Thanks, this looks very cleanly done. The only complaint I would make is
>> that it should probably include a simple test case.
>
> Yes.  Also I am a _bit_ worried about the name "status", as the longer
> term direction is to make "status" not "a preview of commit", may confuse
> people who do read Release Notes.

Right now, this option does not affect how the commit message is prepared
in git tag.  If the option were extended to cover that case as well, what would
a sensible name for the option be, then?

James

-- 
James P. Howard, II, MPA MBCS
jh@jameshoward.us

Re: [PATCH] Add commit.infodisplay option to give message editor empty file

[James P. Howard, II]

On Sat, Dec 5, 2009 at 23:22, Jeff King <peff@peff.net> wrote:

> Probably "--no-status" would be a good name, as the generated template
> is the format generated by "git status".

I have produced a patch against master that implements this in, what I
hope, is a better way.

James

-- 
James P. Howard, II, MPA MBCS
jh@jameshoward.us