Re: Remove help advice text from git editors for interactive rebase and reword

Kirill Likhodedov [off-list ref] writes:

My motivation is the following: I'm improving the Git client
inside of IntelliJ IDEA IDE and I would like to provide only the
plain commit message text to the user (any hints can be shown
separately, not inside the editor).

Who is running "git commit --amend" and "git rebase -i" in the
workflow of a user of your tool?  Is it the end user who types these
commands to the shell command prompt, or does your tool formulate
the command line and does an equivalent of system(3) to run it?

I am assuming that the answer is the latter in my response.

If there is no way to do it now, do you think it makes sense to
provide a configuration variable for this, e.g. to introduce more
advice.* config variables in addition to existing ones?

Not at all interested, as that would mean your tool will tell its
users to set such a configuration variable and their interactive use
of Git outside your tool will behave differently from other people
who use vanilla Git, and they will complain to us.

But I do not think adding a new command line option that only is
passed by a tool like yours when it runs "git rebase -i" via
system(3) equivalent would introduce such an issue, so that may be
workable.

But stepping back a bit, as you said in the parentheses, your tool
would need to grab these "hints" from Git, instead of having a
separate hardcoded hints that will go stale while the underlying Git
command improves, to be able to show them "separately".  Which means
to me that you would need to get the output Git would normally show
to the end user and do your own splitting and parsing anyway.  Which
in turn would mean that a configuration or a command line option to
squelch these, which would rob your tool the ability to read what
Git would have told to your users, would be a bad idea and not a
useful addition to the overall system.  So...

On 24 Jul 2017, at 01:09 , Junio C Hamano [off-list ref] wrote:

Who is running "git commit --amend" and "git rebase -i" in the
workflow of a user of your tool?  Is it the end user who types these
commands to the shell command prompt, or does your tool formulate
the command line and does an equivalent of system(3) to run it?

I am assuming that the answer is the latter in my response.

Yes, it is the latter case: the tool formulates the command line and forks a process.

Not at all interested, as that would mean your tool will tell its
users to set such a configuration variable and their interactive use
of Git outside your tool will behave differently from other people
who use vanilla Git, and they will complain to us.

That's not true, since the tool can (and would) use the `git -c config.var=value rebase -i` syntax to set the configuration variable just for this particular command, without affecting the environment.

Btw, if my proposal is so uninteresting, why the existing advice.* variables were previously introduced? I don't know the motivation, but assume that it was about making Git less wordy for experienced users. So I don't see any difference here.

But stepping back a bit, as you said in the parentheses, your tool
would need to grab these "hints" from Git, instead of having a
separate hardcoded hints that will go stale while the underlying Git
command improves, to be able to show them "separately".  

There is no need to call Git to get these "hints". They are quite obvious, well-known and can be hardcoded. However, I don't plan to use these hints anyway, since they are a bit foreign to the GUI of the tool I develop. For instance, for reword I'd like to show an editor containing just the plain commit message that the user is about to change. 

On Mon, Jul 24, 2017 at 01:26:09AM +0300, Kirill Likhodedov wrote:

quoted

Not at all interested, as that would mean your tool will tell its
users to set such a configuration variable and their interactive use
of Git outside your tool will behave differently from other people
who use vanilla Git, and they will complain to us.

That's not true, since the tool can (and would) use the `git -c
config.var=value rebase -i` syntax to set the configuration variable
just for this particular command, without affecting the environment.

Yes, but if you are adding a config variable that is only ever meant to
be used from the command line, it probably makes sense to just add a
command-line option.

Btw, if my proposal is so uninteresting, why the existing advice.*
variables were previously introduced? I don't know the motivation, but
assume that it was about making Git less wordy for experienced users.
So I don't see any difference here.

That is exactly what advice.* is for, but it is about the _user_
deciding that they don't care about seeing that text. Not a tool that is
calling Git deciding that in one particular context, it would like to
suppress the hint text.

So I actually would be OK with having an advice.* option to squelch
rebase and/or commit instructions. But only if users decide they would
never like to see that text. So yes, your tool could piggy-back on that
config option, but it would be a slight abuse of the intent.

quoted

But stepping back a bit, as you said in the parentheses, your tool
would need to grab these "hints" from Git, instead of having a
separate hardcoded hints that will go stale while the underlying Git
command improves, to be able to show them "separately".  

There is no need to call Git to get these "hints". They are quite
obvious, well-known and can be hardcoded. However, I don't plan to use
these hints anyway, since they are a bit foreign to the GUI of the
tool I develop. For instance, for reword I'd like to show an editor
containing just the plain commit message that the user is about to
change.

If this is all scripted anyway, wouldn't it be an option to just process
the commit message in your program?  The format is well-known, with
hints and instructions on lines marked by core.commentChar ("#" by
default).

I'm not sure exactly of the flow in which the user sees the commit
message buffer (i.e., if you are invoking the editor yourself, or if you
are relying on git-commit to do so). But even in the latter case, you
can hook the editor invocation to do whatever you like. For example:

  GIT_EDITOR='f() { sed -i /^#/d "$1"; $EDITOR "$1"; }; f' git commit

That allows you not only to strip out the existing instructions, but to
insert whatever other instructions you choose.

-Peff

So I actually would be OK with having an advice.* option to squelch
rebase and/or commit instructions. But only if users decide they would
never like to see that text. So yes, your tool could piggy-back on that
config option, but it would be a slight abuse of the intent.

I don't mind the advice in the interactive rebase TODO list.  It's at
the end of the file, nothing comes after that, so it's never in the
way.

However, I do care about the advices in the commit message template,
because they are _between_ the commit message I'm writing and the diff
(using commit.verbose=true) I'm writing about.  So I build git
for my own use with the patch below for a couple of years now, but
never submitted it.


On a related note, when committing a merge or cherry-pick the commit
message templates includes this:

  # It looks like you may be committing a merge.
  # If this is not correct, please remove the file
  #       .git/MERGE_HEAD
  # and try again.

This text traces back almost to the dawn of time, to commit 9c065315f
(Make "git commit" work correctly in the presense of a manual merge,
2005-06-08).  Now, I can well imagine that stray MERGE_HEAD files
caused troubles back then, especially with those "manual merges"...  
But is it really an issue with modern git?!  I think this is long
outdated and could be removed.


  -- >8 --

Subject: [PATCH] commit: allow suppression of commit message template advices

The commit message template includes a lot of advices:

  - The default commit message template asks the user nicely to write
    a commit message and tells about comments and how to abort.
  - It includes some outdated hints about merges and cherry-picks.
  - Finally, in case of 'git commit -v' it reminds about the role of
    the scissors line separating the commit message from the diff.

While these reminders are useful for new users, with time they learn
what the score is, and experienced users might find these advices are
just wasting a couple of lines' worth of screen real estate.

Make displaying these advices configurable via the 'advice.commitMsg'
config variable.

Signed-off-by: SZEDER Gábor <redacted>
---
 Documentation/config.txt |  2 ++
 advice.c                 |  2 ++
 advice.h                 |  1 +
 builtin/commit.c         | 69 ++++++++++++++++++++++++++----------------------
 wt-status.c              | 14 +++++-----
 5 files changed, 50 insertions(+), 38 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d5c9c4cab..29c8736b1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -331,6 +331,8 @@ advice.*::
 	commitBeforeMerge::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwriting local changes.
+	commitMsg::
+		Advices shown in the commit message template.
 	resolveConflict::
 		Advice shown by various commands when conflicts
 		prevent the operation from being performed.

diff --git a/advice.c b/advice.c
index d81e1cb74..7851bb20c 100644
--- a/advice.c
+++ b/advice.c

@@ -10,6 +10,7 @@ int advice_push_needs_force = 1;
 int advice_status_hints = 1;
 int advice_status_u_option = 1;
 int advice_commit_before_merge = 1;
+int advice_commit_msg = 1;
 int advice_resolve_conflict = 1;
 int advice_implicit_identity = 1;
 int advice_detached_head = 1;

@@ -31,6 +32,7 @@ static struct {
 	{ "statushints", &advice_status_hints },
 	{ "statusuoption", &advice_status_u_option },
 	{ "commitbeforemerge", &advice_commit_before_merge },
+	{ "commitmsg", &advice_commit_msg },
 	{ "resolveconflict", &advice_resolve_conflict },
 	{ "implicitidentity", &advice_implicit_identity },
 	{ "detachedhead", &advice_detached_head },

diff --git a/advice.h b/advice.h
index c84a44531..92c9937d6 100644
--- a/advice.h
+++ b/advice.h

@@ -12,6 +12,7 @@ extern int advice_push_needs_force;
 extern int advice_status_hints;
 extern int advice_status_u_option;
 extern int advice_commit_before_merge;
+extern int advice_commit_msg;
 extern int advice_resolve_conflict;
 extern int advice_implicit_identity;
 extern int advice_detached_head;

diff --git a/builtin/commit.c b/builtin/commit.c
index 8e9380251..ead7bf5ef 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -810,38 +810,42 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		if (whence != FROM_COMMIT) {
 			if (cleanup_mode == CLEANUP_SCISSORS)
 				wt_status_add_cut_line(s->fp);
-			status_printf_ln(s, GIT_COLOR_NORMAL,
-			    whence == FROM_MERGE
-				? _("\n"
-					"It looks like you may be committing a merge.\n"
-					"If this is not correct, please remove the file\n"
-					"	%s\n"
-					"and try again.\n")
-				: _("\n"
-					"It looks like you may be committing a cherry-pick.\n"
-					"If this is not correct, please remove the file\n"
-					"	%s\n"
-					"and try again.\n"),
-				whence == FROM_MERGE ?
-					git_path_merge_head() :
-					git_path_cherry_pick_head());
+			if (advice_commit_msg)
+				status_printf_ln(s, GIT_COLOR_NORMAL,
+				    whence == FROM_MERGE
+					? _("\n"
+						"It looks like you may be committing a merge.\n"
+						"If this is not correct, please remove the file\n"
+						"	%s\n"
+						"and try again.\n")
+					: _("\n"
+						"It looks like you may be committing a cherry-pick.\n"
+						"If this is not correct, please remove the file\n"
+						"	%s\n"
+						"and try again.\n"),
+					whence == FROM_MERGE ?
+						git_path_merge_head() :
+						git_path_cherry_pick_head());
 		}
 
 		fprintf(s->fp, "\n");
-		if (cleanup_mode == CLEANUP_ALL)
-			status_printf(s, GIT_COLOR_NORMAL,
-				_("Please enter the commit message for your changes."
-				  " Lines starting\nwith '%c' will be ignored, and an empty"
-				  " message aborts the commit.\n"), comment_line_char);
-		else if (cleanup_mode == CLEANUP_SCISSORS && whence == FROM_COMMIT)
+		if (cleanup_mode == CLEANUP_ALL) {
+			if (advice_commit_msg)
+				status_printf(s, GIT_COLOR_NORMAL,
+					_("Please enter the commit message for your changes."
+					  " Lines starting\nwith '%c' will be ignored, and an empty"
+					  " message aborts the commit.\n"), comment_line_char);
+		} else if (cleanup_mode == CLEANUP_SCISSORS && whence == FROM_COMMIT)
 			wt_status_add_cut_line(s->fp);
-		else /* CLEANUP_SPACE, that is. */
-			status_printf(s, GIT_COLOR_NORMAL,
-				_("Please enter the commit message for your changes."
-				  " Lines starting\n"
-				  "with '%c' will be kept; you may remove them"
-				  " yourself if you want to.\n"
-				  "An empty message aborts the commit.\n"), comment_line_char);
+		else /* CLEANUP_SPACE, that is. */ {
+			if (advice_commit_msg)
+				status_printf(s, GIT_COLOR_NORMAL,
+					_("Please enter the commit message for your changes."
+					  " Lines starting\n"
+					  "with '%c' will be kept; you may remove them"
+					  " yourself if you want to.\n"
+					  "An empty message aborts the commit.\n"), comment_line_char);
+		}
 
 		/*
 		 * These should never fail because they come from our own

@@ -856,7 +860,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 				_("%s"
 				"Author:    %.*s <%.*s>"),
-				ident_shown++ ? "" : "\n",
+				ident_shown++ || !advice_commit_msg ? "" : "\n",
 				(int)(ai.name_end - ai.name_begin), ai.name_begin,
 				(int)(ai.mail_end - ai.mail_begin), ai.mail_begin);
 

@@ -864,18 +868,19 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 				_("%s"
 				"Date:      %s"),
-				ident_shown++ ? "" : "\n",
+				ident_shown++ || !advice_commit_msg ? "" : "\n",
 				show_ident_date(&ai, DATE_MODE(NORMAL)));
 
 		if (!committer_ident_sufficiently_given())
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 				_("%s"
 				"Committer: %.*s <%.*s>"),
-				ident_shown++ ? "" : "\n",
+				ident_shown++ || !advice_commit_msg ? "" : "\n",
 				(int)(ci.name_end - ci.name_begin), ci.name_begin,
 				(int)(ci.mail_end - ci.mail_begin), ci.mail_begin);
 
-		status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new line for clarity */
+		if (ident_shown || advice_commit_msg)
+			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new line for clarity */
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;

diff --git a/wt-status.c b/wt-status.c
index 77c27c511..09cb24be9 100644
--- a/wt-status.c
+++ b/wt-status.c

@@ -934,13 +934,15 @@ size_t wt_status_locate_end(const char *s, size_t len)
 
 void wt_status_add_cut_line(FILE *fp)
 {
-	const char *explanation = _("Do not touch the line above.\nEverything below will be removed.");
-	struct strbuf buf = STRBUF_INIT;
-
 	fprintf(fp, "%c %s", comment_line_char, cut_line);
-	strbuf_add_commented_lines(&buf, explanation, strlen(explanation));
-	fputs(buf.buf, fp);
-	strbuf_release(&buf);
+	if (advice_commit_msg) {
+		const char *explanation = _("Do not touch the line above.\nEverything below will be removed.");
+		struct strbuf buf = STRBUF_INIT;
+
+		strbuf_add_commented_lines(&buf, explanation, strlen(explanation));
+		fputs(buf.buf, fp);
+		strbuf_release(&buf);
+	}
 }
 
 static void wt_longstatus_print_verbose(struct wt_status *s)

-- 
2.14.0.rc0.88.ge338f4246