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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 16, 2014, 16:40 UTC
Message-ID: <20140516164053.GB12314@google.com>
In-Reply-To: <1400248283-303-2-git-send-email-pclouds@gmail.com>

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?

[...]

Show 9 quoted lines

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

[...]

Show 8 quoted lines

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

[...]

Show 8 quoted lines

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