Fwd: [Bug] - Processing commit message after amend

Fwd: [Bug] - Processing commit message after amend

[Michal Stasa]

Hi,

I have stumbled on a weird bug. At work, we use redmine as an issue
tracker and its task are marked by a number starting with #. When I
commit some work and write #1234 in the message, it works. However,
later on when I remember that I forgot to add some files and amend the
commit, vim appears and I cannot perform the commit because the
message starts with # which is a comment in vim and thus I get an
error that my commit message is empty.

Steps to reproduce:
1) commit a file
git commit File1.txt -m "#1234 documentation added"

2) amend previous commit
git commit File2.txt -- amend

3) go for :wq right away

4) an error that the message is empty appears
"Aborting commit due to empty commit message"

However, if you use amend and no edit option, it works
git commit --amend --no-edit

We use git for Windows downloaded here:
http://git-scm.com/downloads

The problem appears in Windows command line. I have not tested it
anywhere else. The OS is Windows Server 2008 R2 Datacenter.

Cheers from cloudy Prague
Michal Staša

Santhos.net
www.santhos.net

+420 773 454 793
michal.stasa@santhos.net

Re: [Bug] - Processing commit message after amend

[Duy Nguyen]

On Fri, May 16, 2014 at 5:18 PM, Michal Stasa <michal.stasa@gmail.com> wrote:
> Hi,
>
> I have stumbled on a weird bug. At work, we use redmine as an issue
> tracker and its task are marked by a number starting with #. When I
> commit some work and write #1234 in the message, it works. However,
> later on when I remember that I forgot to add some files and amend the
> commit, vim appears and I cannot perform the commit because the
> message starts with # which is a comment in vim and thus I get an
> error that my commit message is empty.

A workaround would be "git -c core.commentChar=@ <command> ..." (@
could be some other character). But maybe git should detect that the
current commit message has leading '#' and automatically switch to
another character..
-- 
Duy

Re: Fwd: [Bug] - Processing commit message after amend

[David Kastrup]

Michal Stasa <michal.stasa@gmail.com> writes:

> I have stumbled on a weird bug. At work, we use redmine as an issue
> tracker and its task are marked by a number starting with #. When I
> commit some work and write #1234 in the message, it works. However,
> later on when I remember that I forgot to add some files and amend the
> commit, vim appears and I cannot perform the commit because the
> message starts with # which is a comment in vim and thus I get an
> error that my commit message is empty.
>
> Steps to reproduce:
> 1) commit a file
> git commit File1.txt -m "#1234 documentation added"
>
> 2) amend previous commit
> git commit File2.txt -- amend
>
> 3) go for :wq right away

git commit --amend -C HEAD File2.txt

should do the trick without starting the editor.

> However, if you use amend and no edit option, it works
> git commit --amend --no-edit

Ah, so you got your solution.  It's not like you could add that sort of
commit message interactively to start with, so it's not all that
surprising that you won't be able to amend it interactively.

-- 
David Kastrup

Re: [Bug] - Processing commit message after amend

[Michal Stasa]

It sounds like a good workaround but I think there could be a problem.
When vim opens there is the message on the first line and two lines
below is a commented text which uses # as comment char. Does the char
change when you change the comment char?
Michal Staša

Santhos.net
www.santhos.net

+420 773 454 793
michal.stasa@santhos.net


On 16 May 2014 12:28, Duy Nguyen <pclouds@gmail.com> wrote:
> On Fri, May 16, 2014 at 5:18 PM, Michal Stasa <michal.stasa@gmail.com> wrote:
>> Hi,
>>
>> I have stumbled on a weird bug. At work, we use redmine as an issue
>> tracker and its task are marked by a number starting with #. When I
>> commit some work and write #1234 in the message, it works. However,
>> later on when I remember that I forgot to add some files and amend the
>> commit, vim appears and I cannot perform the commit because the
>> message starts with # which is a comment in vim and thus I get an
>> error that my commit message is empty.
>
> A workaround would be "git -c core.commentChar=@ <command> ..." (@
> could be some other character). But maybe git should detect that the
> current commit message has leading '#' and automatically switch to
> another character..
> --
> Duy

[PATCH] commit: switch core.commentChar if it's found in existing commit

[Nguyễn Thái Ngọc Duy]

If we need to use core.commentChar and it's already in the prepared
message, find another char among a small subset. This should stop
surprises because git strips some lines unexpectedly. Of course if
candicate characters happen to be all out, this change does not help.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 On Fri, May 16, 2014 at 5:28 PM, Duy Nguyen <pclouds@gmail.com> wrote:
 > But maybe git should detect that the
 > current commit message has leading '#' and automatically switch to
 > another character..

 Something like this. Lightly tested.. I know there's a small bug..

 builtin/commit.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..70ceb61 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -593,6 +593,32 @@ static char *cut_ident_timestamp_part(char *string)
 	return ket;
 }
 
+static void adjust_comment_line_char(const struct strbuf *sb)
+{
+	char candidates[] = " !@#$%^&|:;~";
+	char *candidate;
+	const char *p;
+	if (!sb->len)
+		return;
+	candidates[0] = comment_line_char;
+	p = sb->buf;
+	do {
+		candidate = strchr(candidates, *p);
+		if (candidate)
+			*candidate = ' ';
+		p = strchrnul(p, '\n');
+		if (*p)
+			p++;
+	} while (*p);
+	if (strchr(candidates, comment_line_char)) {
+		p = candidates;
+		while (*p && *p == ' ')
+			p++;
+		if (*p)
+			comment_line_char = *p;
+	}
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -748,6 +774,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
 
+	if (use_editor && include_status)
+		adjust_comment_line_char(&sb);
+
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
-- 
1.9.1.346.ga2b5940

[PATCH 1/2] config: be strict on core.commentChar

[Nguyễn Thái Ngọc Duy]

We don't support comment _strings_ (at least not yet). And multi-byte
character encoding could also be misinterpreted.

The test with two commas is deleted because it violates this. It's
added with the patch that introduces core.commentChar in eff80a9
(Allow custom "comment char" - 2013-01-16). It's not clear to me _why_
that behavior is wanted.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 config.c          | 8 ++++++--
 t/t7508-status.sh | 6 ------
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/config.c b/config.c
index a30cb5c..05d909b 100644
--- a/config.c
+++ b/config.c
@@ -826,8 +826,12 @@ static int git_default_core_config(const char *var, const char *value)
 	if (!strcmp(var, "core.commentchar")) {
 		const char *comment;
 		int ret = git_config_string(&comment, var, value);
-		if (!ret)
-			comment_line_char = comment[0];
+		if (!ret) {
+			if (comment[0] && !comment[1])
+				comment_line_char = comment[0];
+			else
+				return error("core.commentChar should only be one character");
+		}
 		return ret;
 	}
 
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c987b5e..98a9990 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1348,12 +1348,6 @@ test_expect_success "status (core.commentchar with submodule summary)" '
 	test_i18ncmp expect output
 '
 
-test_expect_success "status (core.commentchar with two chars with submodule summary)" '
-	test_config core.commentchar ";;" &&
-	git -c status.displayCommentPrefix=true status >output &&
-	test_i18ncmp expect output
-'
-
 test_expect_success "--ignore-submodules=all suppresses submodule summary" '
 	cat > expect << EOF &&
 On branch master
-- 
1.9.1.346.ga2b5940

[PATCH 2/2] commit: allow core.commentChar=auto for character auto selection

[Nguyễn Thái Ngọc Duy]

core.commentChar starts with '#' as in default but if it's already in
the prepared message, find another one among a small subset. This
should stop surprises because git strips some lines unexpectedly.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt |  3 +++
 builtin/commit.c         | 36 ++++++++++++++++++++++++++++++++++++
 cache.h                  |  1 +
 config.c                 |  2 ++
 environment.c            |  1 +
 t/t7502-commit.sh        | 25 +++++++++++++++++++++++++
 6 files changed, 68 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..d5bf4d0 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -544,6 +544,9 @@ core.commentchar::
 	messages consider a line that begins with this character
 	commented, and removes them after the editor returns
 	(default '#').
++
+If set to "auto", `git-commit` would select a character that is not
+the beginning character of any line of existing commit messages.
 
 sequence.editor::
 	Text editor used by `git rebase -i` for editing the rebase instruction file.
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..039b426 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -594,6 +594,40 @@ static char *cut_ident_timestamp_part(char *string)
 	return ket;
 }
 
+static void adjust_comment_line_char(const struct strbuf *sb)
+{
+	char candidates[] = " @!#$%^&|:;~";
+	char *candidate;
+	const char *p;
+
+	if (!sb->len)
+		return;
+
+	if (!strchr(candidates, comment_line_char))
+		candidates[0] = comment_line_char;
+	p = sb->buf;
+	candidate = strchr(candidates, *p);
+	if (candidate)
+		*candidate = ' ';
+	for (p = sb->buf; *p; p++) {
+		if ((p[0] == '\n' || p[0] == '\r') && p[1]) {
+			candidate = strchr(candidates, p[1]);
+			if (candidate)
+				*candidate = ' ';
+		}
+	}
+
+	if (candidates[0] == comment_line_char)
+		return;
+	for (p = candidates; *p == ' '; p++)
+		;
+	if (!*p)
+		die(_("the comment character '%c' exists in the commit message\n"
+		      "Please choose another character for core.commentChar"),
+		    comment_line_char);
+	comment_line_char = *p;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -748,6 +782,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
 
+	if (auto_comment_line_char)
+		adjust_comment_line_char(&sb);
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
diff --git a/cache.h b/cache.h
index 107ac61..646fb81 100644
--- a/cache.h
+++ b/cache.h
@@ -602,6 +602,7 @@ extern int precomposed_unicode;
  * that is subject to stripspace.
  */
 extern char comment_line_char;
+extern int auto_comment_line_char;
 
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
diff --git a/config.c b/config.c
index 05d909b..5ec3520 100644
--- a/config.c
+++ b/config.c
@@ -829,6 +829,8 @@ static int git_default_core_config(const char *var, const char *value)
 		if (!ret) {
 			if (comment[0] && !comment[1])
 				comment_line_char = comment[0];
+			else if (!strcasecmp(comment, "auto"))
+				auto_comment_line_char = 1;
 			else
 				return error("core.commentChar should only be one character");
 		}
diff --git a/environment.c b/environment.c
index 5c4815d..f2de1ee 100644
--- a/environment.c
+++ b/environment.c
@@ -69,6 +69,7 @@ unsigned long pack_size_limit_cfg;
  * that is subject to stripspace.
  */
 char comment_line_char = '#';
+int auto_comment_line_char;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 9a3f3a1..5cff300 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -563,4 +563,29 @@ test_expect_success 'commit --status with custom comment character' '
 	test_i18ngrep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
+test_expect_success 'switch core.commentchar' '
+	test_commit "#foo" foo &&
+	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
+	test_i18ngrep "^@ Changes to be committed:" .git/COMMIT_EDITMSG
+'
+
+test_expect_success 'switch core.commentchar but out of options' '
+	cat >text <<\EOF &&
+# 1
+@ 2
+! 3
+$ 4
+% 5
+^ 6
+& 7
+| 8
+: 9
+; 10
+~ 11
+EOF
+	git commit --amend -F text &&
+	GIT_EDITOR=.git/FAKE_EDITOR test_must_fail \
+		git -c core.commentChar=auto commit --amend
+'
+
 test_done
-- 
1.9.1.346.ga2b5940

RE: [PATCH 1/2] config: be strict on core.commentChar

[Felipe Contreras]

Nguyễn Thái Ngọc Duy wrote:
> We don't support comment _strings_ (at least not yet). And multi-byte
> character encoding could also be misinterpreted.
> 
> The test with two commas is deleted because it violates this. It's
> added with the patch that introduces core.commentChar in eff80a9
> (Allow custom "comment char" - 2013-01-16). It's not clear to me _why_
> that behavior is wanted.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  config.c          | 8 ++++++--
>  t/t7508-status.sh | 6 ------
>  2 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/config.c b/config.c
> index a30cb5c..05d909b 100644
> --- a/config.c
> +++ b/config.c
> @@ -826,8 +826,12 @@ static int git_default_core_config(const char *var, const char *value)
>  	if (!strcmp(var, "core.commentchar")) {
>  		const char *comment;
>  		int ret = git_config_string(&comment, var, value);
> -		if (!ret)
> -			comment_line_char = comment[0];
> +		if (!ret) {
> +			if (comment[0] && !comment[1])
> +				comment_line_char = comment[0];
> +			else
> +				return error("core.commentChar should only be one character");
> +		}

Small nit:

  if (ret)
	  return ret;
  if (comment[0] && !comment[1])
	  comment_line_char = comment[0];
  else
	  return error("core.commentChar should only be one character");

-- 
Felipe Contreras

Re: [PATCH 1/2] config: be strict on core.commentChar

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> --- a/config.c
> +++ b/config.c
> @@ -826,8 +826,12 @@ static int git_default_core_config(const char *var, const char *value)
>  	if (!strcmp(var, "core.commentchar")) {
>  		const char *comment;
>  		int ret = git_config_string(&comment, var, value);
> -		if (!ret)
> -			comment_line_char = comment[0];
> +		if (!ret) {
> +			if (comment[0] && !comment[1])
> +				comment_line_char = comment[0];
> +			else
> +				return error("core.commentChar should only be one character");
> +		}

Perhaps, to decrease indentation a little:

		if (ret)
			return ret;
		if (comment[0] && !comment[1])
			comment_line_char = comment[0];
		else
			return error(...);
		return 0;

[...]
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1348,12 +1348,6 @@ test_expect_success "status (core.commentchar with submodule summary)" '
>  	test_i18ncmp expect output
>  '
>  
> -test_expect_success "status (core.commentchar with two chars with submodule summary)" '
> -	test_config core.commentchar ";;" &&
> -	git -c status.displayCommentPrefix=true status >output &&
> -	test_i18ncmp expect output

Could keep the test to avoid regressions:

	test_config core.commentchar ";;" &&
	test_must_fail git -c status.displayCommentPrefix=true status

Thanks,
Jonathan

Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

> core.commentChar starts with '#' as in default but if it's already in
> the prepared message, find another one among a small subset. This
> should stop surprises because git strips some lines unexpectedly.

Probably worth mentioning this only kicks in if someone explicitly
configures [core] commentchar = auto.

Would it be a goal to make 'auto' the default eventually if people
turn out to like it?

[...]
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -594,6 +594,40 @@ static char *cut_ident_timestamp_part(char *string)
>  	return ket;
>  }
>  
> +static void adjust_comment_line_char(const struct strbuf *sb)
> +{
> +	char candidates[] = " @!#$%^&|:;~";

This prefers '@' over '#'.  Intended?

[...]
> +	char *candidate;
> +	const char *p;
> +
> +	if (!sb->len)
> +		return;
> +
> +	if (!strchr(candidates, comment_line_char))
> +		candidates[0] = comment_line_char;

Could do

	if (!memchr(sb->buf, comment_line_char, sb->len))
		return;

to solve the precedence problem.  The comment_line_char not appearing
in the message is the usual case and handling it separately means it
gets handled faster.

[...]
> --- a/config.c
> +++ b/config.c
> @@ -829,6 +829,8 @@ static int git_default_core_config(const char *var, const char *value)
>  		if (!ret) {
>  			if (comment[0] && !comment[1])
>  				comment_line_char = comment[0];
> +			else if (!strcasecmp(comment, "auto"))
> +				auto_comment_line_char = 1;

Is there a way to disable 'auto' if 'auto' is already set?

comment_line_char still can be set and matters when 'auto' is set.
Should they be separate settings?

> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -563,4 +563,29 @@ test_expect_success 'commit --status with custom comment character' '
[...]
> +	GIT_EDITOR=.git/FAKE_EDITOR test_must_fail \

Shells make it obnoxiously hard to set a one-shot envvar while
calling a function without the setting leaking into later commands.

	(
		test_set_editor .git/FAKE_EDITOR &&
		test_must_fail ...
	)

or

	test_must_fail env GIT_EDITOR=.git/FAKE_EDITOR ...

should do the trick.

Thanks and hope that helps,
Jonathan

Re: [PATCH] commit: switch core.commentChar if it's found in existing commit

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> If we need to use core.commentChar and it's already in the prepared
> message, find another char among a small subset. This should stop
> surprises because git strips some lines unexpectedly. Of course if
> candicate characters happen to be all out, this change does not help.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  On Fri, May 16, 2014 at 5:28 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>  > But maybe git should detect that the
>  > current commit message has leading '#' and automatically switch to
>  > another character..
>
>  Something like this. Lightly tested.. I know there's a small bug..
>
>  builtin/commit.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6ab4605..70ceb61 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -593,6 +593,32 @@ static char *cut_ident_timestamp_part(char *string)
>  	return ket;
>  }
>  
> +static void adjust_comment_line_char(const struct strbuf *sb)
> +{
> +	char candidates[] = " !@#$%^&|:;~";

Did you really mean to add a SP to the candidates?

> +	char *candidate;
> +	const char *p;
> +	if (!sb->len)
> +		return;
> +	candidates[0] = comment_line_char;
> +	p = sb->buf;
> +	do {
> +		candidate = strchr(candidates, *p);
> +		if (candidate)
> +			*candidate = ' ';
> +		p = strchrnul(p, '\n');
> +		if (*p)
> +			p++;
> +	} while (*p);
> +	if (strchr(candidates, comment_line_char)) {
> +		p = candidates;
> +		while (*p && *p == ' ')
> +			p++;
> +		if (*p)
> +			comment_line_char = *p;
> +	}
> +}
> +
>  static int prepare_to_commit(const char *index_file, const char *prefix,
>  			     struct commit *current_head,
>  			     struct wt_status *s,
> @@ -748,6 +774,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
>  		die_errno(_("could not write commit template"));
>  
> +	if (use_editor && include_status)
> +		adjust_comment_line_char(&sb);
> +
>  	strbuf_release(&sb);
>  
>  	/* This checks if committer ident is explicitly given */

Re: [PATCH 1/2] config: be strict on core.commentChar

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Nguyễn Thái Ngọc Duy wrote:
>
>> --- a/config.c
>> +++ b/config.c
>> @@ -826,8 +826,12 @@ static int git_default_core_config(const char *var, const char *value)
>>  	if (!strcmp(var, "core.commentchar")) {
>>  		const char *comment;
>>  		int ret = git_config_string(&comment, var, value);
>> -		if (!ret)
>> -			comment_line_char = comment[0];
>> +		if (!ret) {
>> +			if (comment[0] && !comment[1])
>> +				comment_line_char = comment[0];
>> +			else
>> +				return error("core.commentChar should only be one character");
>> +		}
>
> Perhaps, to decrease indentation a little:
>
> 		if (ret)
> 			return ret;
> 		if (comment[0] && !comment[1])
> 			comment_line_char = comment[0];
> 		else
> 			return error(...);
> 		return 0;
>
> [...]
>> --- a/t/t7508-status.sh
>> +++ b/t/t7508-status.sh
>> @@ -1348,12 +1348,6 @@ test_expect_success "status (core.commentchar with submodule summary)" '
>>  	test_i18ncmp expect output
>>  '
>>  
>> -test_expect_success "status (core.commentchar with two chars with submodule summary)" '
>> -	test_config core.commentchar ";;" &&
>> -	git -c status.displayCommentPrefix=true status >output &&
>> -	test_i18ncmp expect output
>
> Could keep the test to avoid regressions:
>
> 	test_config core.commentchar ";;" &&
> 	test_must_fail git -c status.displayCommentPrefix=true status

All good points, including your other review message.

Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Nguyễn Thái Ngọc Duy wrote:
>
>> core.commentChar starts with '#' as in default but if it's already in
>> the prepared message, find another one among a small subset. This
>> should stop surprises because git strips some lines unexpectedly.
>
> Probably worth mentioning this only kicks in if someone explicitly
> configures [core] commentchar = auto.
>
> Would it be a goal to make 'auto' the default eventually if people
> turn out to like it?
>
> [...]
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -594,6 +594,40 @@ static char *cut_ident_timestamp_part(char *string)
>>  	return ket;
>>  }
>>  
>> +static void adjust_comment_line_char(const struct strbuf *sb)
>> +{
>> +	char candidates[] = " @!#$%^&|:;~";
>
> This prefers '@' over '#'.  Intended?

I think the candidates[0] will almost always be overridden with "#"
so it probably does not matter in practice, but I tend to agree that
"#" (and probably ";") should come before all others.

Re: [PATCH] commit: switch core.commentChar if it's found in existing commit

[Junio C Hamano]

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

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> If we need to use core.commentChar and it's already in the prepared
>> message, find another char among a small subset. This should stop
>> surprises because git strips some lines unexpectedly. Of course if
>> candicate characters happen to be all out, this change does not help.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  On Fri, May 16, 2014 at 5:28 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>  > But maybe git should detect that the
>>  > current commit message has leading '#' and automatically switch to
>>  > another character..
>>
>>  Something like this. Lightly tested.. I know there's a small bug..
>>
>>  builtin/commit.c | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 6ab4605..70ceb61 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -593,6 +593,32 @@ static char *cut_ident_timestamp_part(char *string)
>>  	return ket;
>>  }
>>  
>> +static void adjust_comment_line_char(const struct strbuf *sb)
>> +{
>> +	char candidates[] = " !@#$%^&|:;~";
>
> Did you really mean to add a SP to the candidates?

Please ignore this noise.  I realized that the SP is there only to
be overwritten in the meantime, and this is an old message sent out
before that realization, just emerging from my mail provider's
outbox.

Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection

[Duy Nguyen]

On Fri, May 16, 2014 at 11:40 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyễn Thái Ngọc Duy wrote:
>
>> core.commentChar starts with '#' as in default but if it's already in
>> the prepared message, find another one among a small subset. This
>> should stop surprises because git strips some lines unexpectedly.
>
> Probably worth mentioning this only kicks in if someone explicitly
> configures [core] commentchar = auto.
>
> Would it be a goal to make 'auto' the default eventually if people
> turn out to like it?

No. I started this with a patch that does this automatically without a
config knob. It broke git-commit with custom templates. It broke
--cleanup=strip -e -F.. So people may want to put this in ~/.gitconfig
but it's their decision.

>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -594,6 +594,40 @@ static char *cut_ident_timestamp_part(char *string)
>>       return ket;
>>  }
>>
>> +static void adjust_comment_line_char(const struct strbuf *sb)
>> +{
>> +     char candidates[] = " @!#$%^&|:;~";
>
> This prefers '@' over '#'.  Intended?

It used to be the order of keys 1, 2, 3... on qwerty keyboard, but I
was afraid ! may become history expansion in bash so I made @
preferred over !. Will make # and ; higher priority.

>> +     char *candidate;
>> +     const char *p;
>> +
>> +     if (!sb->len)
>> +             return;
>> +
>> +     if (!strchr(candidates, comment_line_char))
>> +             candidates[0] = comment_line_char;
>
> Could do
>
>         if (!memchr(sb->buf, comment_line_char, sb->len))
>                 return;
>
> to solve the precedence problem.  The comment_line_char not appearing
> in the message is the usual case and handling it separately means it
> gets handled faster.

Now that we use "auto" to turn this on, the placeholder candidates[0]
could probably be removed, we know comment_line_char is '#' at this
point.

>> --- a/config.c
>> +++ b/config.c
>> @@ -829,6 +829,8 @@ static int git_default_core_config(const char *var, const char *value)
>>               if (!ret) {
>>                       if (comment[0] && !comment[1])
>>                               comment_line_char = comment[0];
>> +                     else if (!strcasecmp(comment, "auto"))
>> +                             auto_comment_line_char = 1;
>
> Is there a way to disable 'auto' if 'auto' is already set?
>
> comment_line_char still can be set and matters when 'auto' is set.
> Should they be separate settings?

I think the next core.commentChar should override the old one, so
auto_comment_line_char should be clear when we set new value to
comment_line_char.
-- 
Duy

[PATCH v2 1/2] config: be strict on core.commentChar

[Nguyễn Thái Ngọc Duy]

We don't support comment _strings_ (at least not yet). And multi-byte
character encoding could also be misinterpreted.

The test with two commas is updated because it violates this. It's
added with the patch that introduces core.commentChar in eff80a9
(Allow custom "comment char" - 2013-01-16). It's not clear to me _why_
that behavior is wanted.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 config.c          | 8 ++++++--
 t/t7508-status.sh | 3 +--
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index a30cb5c..491a905 100644
--- a/config.c
+++ b/config.c
@@ -826,9 +826,13 @@ static int git_default_core_config(const char *var, const char *value)
 	if (!strcmp(var, "core.commentchar")) {
 		const char *comment;
 		int ret = git_config_string(&comment, var, value);
-		if (!ret)
+		if (ret)
+			return ret;
+		else if (comment[0] && !comment[1]) {
 			comment_line_char = comment[0];
-		return ret;
+		} else
+			return error("core.commentChar should only be one character");
+		return 0;
 	}
 
 	if (!strcmp(var, "core.askpass"))
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c987b5e..148ab9e 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -1350,8 +1350,7 @@ test_expect_success "status (core.commentchar with submodule summary)" '
 
 test_expect_success "status (core.commentchar with two chars with submodule summary)" '
 	test_config core.commentchar ";;" &&
-	git -c status.displayCommentPrefix=true status >output &&
-	test_i18ncmp expect output
+	test_must_fail git -c status.displayCommentPrefix=true status
 '
 
 test_expect_success "--ignore-submodules=all suppresses submodule summary" '
-- 
1.9.1.346.ga2b5940

[PATCH v2 2/2] commit: allow core.commentChar=auto for character auto selection

[Nguyễn Thái Ngọc Duy]

When core.commentChar is "auto", the comment char starts with '#' as
in default but if it's already in the prepared message, find another
char in a small subset. This should stop surprises because git strips
some lines unexpectedly.

Note that git is not smart enough to recognize '#' as the comment char
in custom templates and convert it if the final comment char is
different. It thinks '#' lines in custom templates as part of the
commit message. So don't use this with custom templates.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt |  3 +++
 builtin/commit.c         | 32 ++++++++++++++++++++++++++++++++
 cache.h                  |  1 +
 config.c                 |  3 +++
 environment.c            |  1 +
 t/t7502-commit.sh        | 26 ++++++++++++++++++++++++++
 6 files changed, 66 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..9f3ce06 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -544,6 +544,9 @@ core.commentchar::
 	messages consider a line that begins with this character
 	commented, and removes them after the editor returns
 	(default '#').
++
+If set to "auto", `git-commit` would select a character that is not
+the beginning character of any line in existing commit messages.
 
 sequence.editor::
 	Text editor used by `git rebase -i` for editing the rebase instruction file.
diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..515c4c4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -594,6 +594,36 @@ static char *cut_ident_timestamp_part(char *string)
 	return ket;
 }
 
+static void adjust_comment_line_char(const struct strbuf *sb)
+{
+	char candidates[] = "#;@!$%^&|:";
+	char *candidate;
+	const char *p;
+
+	comment_line_char = candidates[0];
+	if (!memchr(sb->buf, comment_line_char, sb->len))
+		return;
+
+	p = sb->buf;
+	candidate = strchr(candidates, *p);
+	if (candidate)
+		*candidate = ' ';
+	for (p = sb->buf; *p; p++) {
+		if ((p[0] == '\n' || p[0] == '\r') && p[1]) {
+			candidate = strchr(candidates, p[1]);
+			if (candidate)
+				*candidate = ' ';
+		}
+	}
+
+	for (p = candidates; *p == ' '; p++)
+		;
+	if (!*p)
+		die(_("unable to select a comment character that is not used\n"
+		      "in the current commit message"));
+	comment_line_char = *p;
+}
+
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
@@ -748,6 +778,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
 
+	if (auto_comment_line_char)
+		adjust_comment_line_char(&sb);
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
diff --git a/cache.h b/cache.h
index 107ac61..646fb81 100644
--- a/cache.h
+++ b/cache.h
@@ -602,6 +602,7 @@ extern int precomposed_unicode;
  * that is subject to stripspace.
  */
 extern char comment_line_char;
+extern int auto_comment_line_char;
 
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
diff --git a/config.c b/config.c
index 491a905..d29c733 100644
--- a/config.c
+++ b/config.c
@@ -828,8 +828,11 @@ static int git_default_core_config(const char *var, const char *value)
 		int ret = git_config_string(&comment, var, value);
 		if (ret)
 			return ret;
+		else if (!strcasecmp(comment, "auto"))
+			auto_comment_line_char = 1;
 		else if (comment[0] && !comment[1]) {
 			comment_line_char = comment[0];
+			auto_comment_line_char = 0;
 		} else
 			return error("core.commentChar should only be one character");
 		return 0;
diff --git a/environment.c b/environment.c
index 5c4815d..f2de1ee 100644
--- a/environment.c
+++ b/environment.c
@@ -69,6 +69,7 @@ unsigned long pack_size_limit_cfg;
  * that is subject to stripspace.
  */
 char comment_line_char = '#';
+int auto_comment_line_char;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 9a3f3a1..30e4688 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -563,4 +563,30 @@ test_expect_success 'commit --status with custom comment character' '
 	test_i18ngrep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
+test_expect_success 'switch core.commentchar' '
+	test_commit "#foo" foo &&
+	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
+	test_i18ngrep "^; Changes to be committed:" .git/COMMIT_EDITMSG
+'
+
+test_expect_success 'switch core.commentchar but out of options' '
+	cat >text <<\EOF &&
+# 1
+; 2
+@ 3
+! 4
+$ 5
+% 6
+^ 7
+& 8
+| 9
+: 10
+EOF
+	git commit --amend -F text &&
+	(
+		test_set_editor .git/FAKE_EDITOR &&
+		test_must_fail git -c core.commentChar=auto commit --amend
+	)
+'
+
 test_done
-- 
1.9.1.346.ga2b5940