Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection
On Fri, May 16, 2014 at 11:40 PM, Jonathan Nieder 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection
Jonathan Nieder 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] commit: allow core.commentChar=auto for character auto selection
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
