[PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This series implements the plan to deprecate and remove support for
core.commentChar=auto outlined in [1]. This feature has been the
source of a couple of bug reports recently [2,3] and as explained in
the first patch the design is tricky to fix. When git sees the
deprecated config setting it will print advice like the example below
to help the user either remove the setting or set a custom comment
string.

hint: Support for 'core.commentChar=auto' is deprecated and will be removed in git 3.0
hint:
hint: To use the default comment string (#) please run
hint:
hint:     git config unset --file ~/.config/git/config --all core.commentString
hint:     git config unset --file ~/.config/git/config core.commentChar
hint:     git config unset --global core.commentChar
hint:
hint: To set a custom comment string please run
hint:
hint:     git config set --global core.commentChar <comment string>
hint:
hint: where '<comment string>' is the string you wish to use.

[1] https://lore.kernel.org/git/6a3154e0-e7bc-45ae-b554-67ccab18727a@gmail.com
[2] https://lore.kernel.org/git/20250315140913.577404-1-oswald.buddenhagen@gmx.de
[3] https://lore.kernel.org/git/20250626132233.414789-1-ayu.chandekar@gmail.com

Base-Commit: f0135a9047ca37d4d117dcf21f7e3e89fad85d00
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fremove-auto-comment-char%2Fv1
View-Changes-At: https://github.com/phillipwood/git/compare/f0135a904...83d0d3ece
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/remove-auto-comment-char/v1


Phillip Wood (2):
  breaking-changes: deprecate support for core.commentString=auto
  commit: print advice when core.commentString=auto

 Documentation/BreakingChanges.adoc |   4 +
 Documentation/config/core.adoc     |  20 ++-
 builtin/commit.c                   | 192 +++++++++++++++++++++++++++++
 config.c                           |   4 +
 environment.c                      |   2 +
 environment.h                      |   2 +
 t/t3404-rebase-interactive.sh      |   2 +-
 t/t7502-commit-porcelain.sh        |  32 ++++-
 8 files changed, 252 insertions(+), 6 deletions(-)

-- 
2.49.0.897.gfad3eb7d210

[PATCH 1/2] breaking-changes: deprecate support for core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When "core.commentString" is set to "auto" then "git commit"
will automatically select the comment character ensuring that it
does not the first character on any of the lines in the commit
message. This was introduced by commit 84c9dc2c5a2 (commit: allow
core.commentChar=auto for character auto selection, 2014-05-17) The
motivation seems to be to avoid commenting out lines from the existing
message when amending a commit that was created with a message from
a file.

Unfortunately this feature does not work with:

 * commit message templates that contain comments.

 * prepare-commit-msg hooks that introduce comments.

 * "git commit --cleanup=strip --edit -F <file>" which means that it
   is incompatible with

   - the "fixup" and "squash" commands of "git rebase -i" as the
     comments added by those commands are then treated as part of the
     commit message.

   - the conflict comments added to the commit message by "git
     cherry-pick", "git rebase" etc. as these comments are then treated
     as part of the commit message.

It is also ignored by "git notes" when amending a note.

The issues with comments coming from a template, hook or file are a
consequence of the design of this feature and are therefore hard to
fix.

As the costs of this feature outweigh the benefits deprecate it and
remove it in Git 3.0. If someone comes up with some patches that fix all
the issues in a maintainable way then I'd be happy to see this change
reverted.

The next commit will add some advice for users on how they can update
their config settings.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/BreakingChanges.adoc |  4 ++++
 Documentation/config/core.adoc     | 20 ++++++++++++++++++--
 builtin/commit.c                   |  4 ++++
 config.c                           |  4 ++++
 environment.c                      |  2 ++
 environment.h                      |  2 ++
 t/t3404-rebase-interactive.sh      |  2 +-
 t/t7502-commit-porcelain.sh        |  4 ++--
 8 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index 61bdd586b9e..f38ba1de6e4 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -183,6 +183,10 @@ These features will be removed.
   timeframe, in preference to its synonym "--annotate-stdin".  Git 3.0
   removes the support for "--stdin" altogether.
 
+* Support for `core.commentString=auto` has been deprecated and will
+  be removed in Git 3.0.
++
+cf. <xmqqa59i45wc.fsf@gitster.g>
 
 == Superseded features that will not be deprecated
 
diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
index 9fde1ab63a7..7133f00c38b 100644
--- a/Documentation/config/core.adoc
+++ b/Documentation/config/core.adoc
@@ -531,9 +531,25 @@ core.commentString::
 	commented, and removes them after the editor returns
 	(default '#').
 +
-If set to "auto", `git-commit` would select a character that is not
+ifndef::with-breaking-changes[]
+If set to "auto", `git-commit` will select a character that is not
 the beginning character of any line in existing commit messages.
-+
+Support for this value is deprecated and will be removed in Git 3.0
+due to the following limitations:
++
+--
+* It is incompatible with adding comments in a commit message
+  template. This includes the conflicts comments added to
+  the commit message by `cherry-pick`, `merge`, `rebase` and
+  `revert`.
+* It is incompatible with adding comments to the commit message
+  in the `prepare-commit-msg` hook.
+* It is incompatible with the `fixup` and `squash` commands when
+  rebasing,
+* It is not respected by `git notes`
+--
++
+endif::with-breaking-changes[]
 Note that these two variables are aliases of each other, and in modern
 versions of Git you are free to use a string (e.g., `//` or `⁑⁕⁑`) with
 `commentChar`. Versions of Git prior to v2.45.0 will ignore
diff --git a/builtin/commit.c b/builtin/commit.c
index fba0dded64a..8794b24572b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -683,6 +683,7 @@ static int author_date_is_interesting(void)
 	return author_message || force_date;
 }
 
+#ifndef WITH_BREAKING_CHANGES
 static void adjust_comment_line_char(const struct strbuf *sb)
 {
 	char candidates[] = "#;@!$%^&|:";
@@ -716,6 +717,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	free(comment_line_str_to_free);
 	comment_line_str = comment_line_str_to_free = xstrfmt("%c", *p);
 }
+#endif /* WITH_BREAKING_CHANGES */
 
 static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
 				struct pretty_print_context *ctx)
@@ -912,8 +914,10 @@ 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"));
 
+#ifndef WITH_BREAKING_CHANGES
 	if (auto_comment_line_char)
 		adjust_comment_line_char(&sb);
+#endif /* WITH_BREAKING_CHANGES */
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
diff --git a/config.c b/config.c
index eb60c293ab3..f99496b16c0 100644
--- a/config.c
+++ b/config.c
@@ -1537,14 +1537,18 @@ static int git_default_core_config(const char *var, const char *value,
 	    !strcmp(var, "core.commentstring")) {
 		if (!value)
 			return config_error_nonbool(var);
+#ifndef WITH_BREAKING_CHANGES
 		else if (!strcasecmp(value, "auto"))
 			auto_comment_line_char = 1;
+#endif /* WITH_BREAKING_CHANGES */
 		else if (value[0]) {
 			if (strchr(value, '\n'))
 				return error(_("%s cannot contain newline"), var);
 			comment_line_str = value;
 			FREE_AND_NULL(comment_line_str_to_free);
+#ifndef WITH_BREAKING_CHANGES
 			auto_comment_line_char = 0;
+#endif /* WITH_BREAKING_CHANGES */
 		} else
 			return error(_("%s must have at least one character"), var);
 		return 0;
diff --git a/environment.c b/environment.c
index 7bf0390a335..6804380889f 100644
--- a/environment.c
+++ b/environment.c
@@ -111,7 +111,9 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT;
  */
 const char *comment_line_str = "#";
 char *comment_line_str_to_free;
+#ifndef WITH_BREAKING_CHANGES
 int auto_comment_line_char;
+#endif /* WITH_BREAKING_CHANGES */
 
 /* This is set by setup_git_directory_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
diff --git a/environment.h b/environment.h
index 9a3d05d414a..871596afcef 100644
--- a/environment.h
+++ b/environment.h
@@ -207,7 +207,9 @@ extern char *excludes_file;
  */
 extern const char *comment_line_str;
 extern char *comment_line_str_to_free;
+#ifndef WITH_BREAKING_CHANGES
 extern int auto_comment_line_char;
+#endif /* WITH_BREAKING_CHANGES */
 
 # endif /* USE_THE_REPOSITORY_VARIABLE */
 #endif /* ENVIRONMENT_H */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6bac217ed35..ce0aebb9a7e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_success 'rebase -i respects core.commentchar=auto' '
+test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=auto' '
 	test_config core.commentchar auto &&
 	write_script copy-edit-script.sh <<-\EOF &&
 	cp "$1" edit-script
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b37e2018a74..65b4519a715 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -956,13 +956,13 @@ test_expect_success 'commit --status with custom comment character' '
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'switch core.commentchar' '
+test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
 	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'switch core.commentchar but out of options' '
+test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar but out of options' '
 	cat >text <<\EOF &&
 # 1
 ; 2
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH 1/2] breaking-changes: deprecate support for core.commentString=auto

[Ayush Chandekar]

Hi Phillip,

On Tue, Jul 8, 2025 at 7:27 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When "core.commentString" is set to "auto" then "git commit"
> will automatically select the comment character ensuring that it
> does not the first character on any of the lines in the commit
> message. This was introduced by commit 84c9dc2c5a2 (commit: allow
> core.commentChar=auto for character auto selection, 2014-05-17) The
> motivation seems to be to avoid commenting out lines from the existing
> message when amending a commit that was created with a message from
> a file.
>

s/that it does not the first character/that it does not appear on the
first character?

> Unfortunately this feature does not work with:
>
>  * commit message templates that contain comments.
>
>  * prepare-commit-msg hooks that introduce comments.
>
>  * "git commit --cleanup=strip --edit -F <file>" which means that it
>    is incompatible with
>
>    - the "fixup" and "squash" commands of "git rebase -i" as the
>      comments added by those commands are then treated as part of the
>      commit message.
>
>    - the conflict comments added to the commit message by "git
>      cherry-pick", "git rebase" etc. as these comments are then treated
>      as part of the commit message.
>
> It is also ignored by "git notes" when amending a note.
>
> The issues with comments coming from a template, hook or file are a
> consequence of the design of this feature and are therefore hard to
> fix.
>
> As the costs of this feature outweigh the benefits deprecate it and
> remove it in Git 3.0. If someone comes up with some patches that fix all
> the issues in a maintainable way then I'd be happy to see this change
> reverted.
>
Nit: s/benefits deprecate/benefits, deprecate.

> The next commit will add some advice for users on how they can update
> their config settings.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/BreakingChanges.adoc |  4 ++++
>  Documentation/config/core.adoc     | 20 ++++++++++++++++++--
>  builtin/commit.c                   |  4 ++++
>  config.c                           |  4 ++++
>  environment.c                      |  2 ++
>  environment.h                      |  2 ++
>  t/t3404-rebase-interactive.sh      |  2 +-
>  t/t7502-commit-porcelain.sh        |  4 ++--
>  8 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
> index 61bdd586b9e..f38ba1de6e4 100644
> --- a/Documentation/BreakingChanges.adoc
> +++ b/Documentation/BreakingChanges.adoc
> @@ -183,6 +183,10 @@ These features will be removed.
>    timeframe, in preference to its synonym "--annotate-stdin".  Git 3.0
>    removes the support for "--stdin" altogether.
>
> +* Support for `core.commentString=auto` has been deprecated and will
> +  be removed in Git 3.0.
> ++
> +cf. <xmqqa59i45wc.fsf@gitster.g>
>
>  == Superseded features that will not be deprecated
>
> diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
> index 9fde1ab63a7..7133f00c38b 100644
> --- a/Documentation/config/core.adoc
> +++ b/Documentation/config/core.adoc
> @@ -531,9 +531,25 @@ core.commentString::
>         commented, and removes them after the editor returns
>         (default '#').
>  +
> -If set to "auto", `git-commit` would select a character that is not
> +ifndef::with-breaking-changes[]
> +If set to "auto", `git-commit` will select a character that is not
>  the beginning character of any line in existing commit messages.
> -+
> +Support for this value is deprecated and will be removed in Git 3.0
> +due to the following limitations:
> ++
> +--
> +* It is incompatible with adding comments in a commit message
> +  template. This includes the conflicts comments added to
> +  the commit message by `cherry-pick`, `merge`, `rebase` and
> +  `revert`.
> +* It is incompatible with adding comments to the commit message
> +  in the `prepare-commit-msg` hook.
> +* It is incompatible with the `fixup` and `squash` commands when
> +  rebasing,
> +* It is not respected by `git notes`
> +--
> ++
> +endif::with-breaking-changes[]
>  Note that these two variables are aliases of each other, and in modern
>  versions of Git you are free to use a string (e.g., `//` or `⁑⁕⁑`) with
>  `commentChar`. Versions of Git prior to v2.45.0 will ignore
> diff --git a/builtin/commit.c b/builtin/commit.c
> index fba0dded64a..8794b24572b 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -683,6 +683,7 @@ static int author_date_is_interesting(void)
>         return author_message || force_date;
>  }
>
> +#ifndef WITH_BREAKING_CHANGES
>  static void adjust_comment_line_char(const struct strbuf *sb)
>  {
>         char candidates[] = "#;@!$%^&|:";
> @@ -716,6 +717,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
>         free(comment_line_str_to_free);
>         comment_line_str = comment_line_str_to_free = xstrfmt("%c", *p);
>  }
> +#endif /* WITH_BREAKING_CHANGES */
>
>  static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
>                                 struct pretty_print_context *ctx)
> @@ -912,8 +914,10 @@ 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"));
>
> +#ifndef WITH_BREAKING_CHANGES
>         if (auto_comment_line_char)
>                 adjust_comment_line_char(&sb);
> +#endif /* WITH_BREAKING_CHANGES */
>         strbuf_release(&sb);
>
>         /* This checks if committer ident is explicitly given */
> diff --git a/config.c b/config.c
> index eb60c293ab3..f99496b16c0 100644
> --- a/config.c
> +++ b/config.c
> @@ -1537,14 +1537,18 @@ static int git_default_core_config(const char *var, const char *value,
>             !strcmp(var, "core.commentstring")) {
>                 if (!value)
>                         return config_error_nonbool(var);
> +#ifndef WITH_BREAKING_CHANGES
>                 else if (!strcasecmp(value, "auto"))
>                         auto_comment_line_char = 1;
> +#endif /* WITH_BREAKING_CHANGES */
>                 else if (value[0]) {
>                         if (strchr(value, '\n'))
>                                 return error(_("%s cannot contain newline"), var);
>                         comment_line_str = value;
>                         FREE_AND_NULL(comment_line_str_to_free);
> +#ifndef WITH_BREAKING_CHANGES
>                         auto_comment_line_char = 0;
> +#endif /* WITH_BREAKING_CHANGES */
>                 } else
>                         return error(_("%s must have at least one character"), var);
>                 return 0;
> diff --git a/environment.c b/environment.c
> index 7bf0390a335..6804380889f 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -111,7 +111,9 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT;
>   */
>  const char *comment_line_str = "#";
>  char *comment_line_str_to_free;
> +#ifndef WITH_BREAKING_CHANGES
>  int auto_comment_line_char;
> +#endif /* WITH_BREAKING_CHANGES */
>
>  /* This is set by setup_git_directory_gently() and/or git_default_config() */
>  char *git_work_tree_cfg;
> diff --git a/environment.h b/environment.h
> index 9a3d05d414a..871596afcef 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -207,7 +207,9 @@ extern char *excludes_file;
>   */
>  extern const char *comment_line_str;
>  extern char *comment_line_str_to_free;
> +#ifndef WITH_BREAKING_CHANGES
>  extern int auto_comment_line_char;
> +#endif /* WITH_BREAKING_CHANGES */
>
>  # endif /* USE_THE_REPOSITORY_VARIABLE */
>  #endif /* ENVIRONMENT_H */
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 6bac217ed35..ce0aebb9a7e 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1176,7 +1176,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
>         test B = $(git cat-file commit HEAD^ | sed -ne \$p)
>  '
>
> -test_expect_success 'rebase -i respects core.commentchar=auto' '
> +test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=auto' '
>         test_config core.commentchar auto &&
>         write_script copy-edit-script.sh <<-\EOF &&
>         cp "$1" edit-script
> diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
> index b37e2018a74..65b4519a715 100755
> --- a/t/t7502-commit-porcelain.sh
> +++ b/t/t7502-commit-porcelain.sh
> @@ -956,13 +956,13 @@ test_expect_success 'commit --status with custom comment character' '
>         test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
>  '
>
> -test_expect_success 'switch core.commentchar' '
> +test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
>         test_commit "#foo" foo &&
>         GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
>         test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
>  '
>
> -test_expect_success 'switch core.commentchar but out of options' '
> +test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar but out of options' '
>         cat >text <<\EOF &&
>  # 1
>  ; 2
> --
> 2.49.0.897.gfad3eb7d210
>

Thanks for initiating this topic.

These changes look good to me.

Ayush

Re: [PATCH 1/2] breaking-changes: deprecate support for core.commentString=auto

[Phillip Wood]

Hi Ayush

On 08/07/2025 16:28, Ayush Chandekar wrote:
> On Tue, Jul 8, 2025 at 7:27 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> When "core.commentString" is set to "auto" then "git commit"
>> will automatically select the comment character ensuring that it
>> does not the first character on any of the lines in the commit
>> message. This was introduced by commit 84c9dc2c5a2 (commit: allow
>> core.commentChar=auto for character auto selection, 2014-05-17) The
>> motivation seems to be to avoid commenting out lines from the existing
>> message when amending a commit that was created with a message from
>> a file.
>>
> 
> s/that it does not the first character/that it does not appear on the
> first character?

Well spotted - I was trying to change it say "that it is not the first 
character" but edited the message badly

>> Unfortunately this feature does not work with:
>>
>>   * commit message templates that contain comments.
>>
>>   * prepare-commit-msg hooks that introduce comments.
>>
>>   * "git commit --cleanup=strip --edit -F <file>" which means that it
>>     is incompatible with
>>
>>     - the "fixup" and "squash" commands of "git rebase -i" as the
>>       comments added by those commands are then treated as part of the
>>       commit message.
>>
>>     - the conflict comments added to the commit message by "git
>>       cherry-pick", "git rebase" etc. as these comments are then treated
>>       as part of the commit message.
>>
>> It is also ignored by "git notes" when amending a note.
>>
>> The issues with comments coming from a template, hook or file are a
>> consequence of the design of this feature and are therefore hard to
>> fix.
>>
>> As the costs of this feature outweigh the benefits deprecate it and
>> remove it in Git 3.0. If someone comes up with some patches that fix all
>> the issues in a maintainable way then I'd be happy to see this change
>> reverted.
>>
> Nit: s/benefits deprecate/benefits, deprecate.

Good idea

Thanks

Phillip

[PATCH 2/2] commit: print advice when core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Support for this config setting was deprecated in the last commit
so print some advice to help the user update their config settings
when they are using this setting. The advice message explains that the
setting is deprecated and will be removed in future. It also shows the
commands that the user needs to run to either unset core.commentChar
and core.commentString completely or to change the current setting
to a fixed comment string.

In order to generate this advice we need to parse the config with a
callback that records each file where either of the keys is set and
whether a key occurs more that once in a given file. This lets us
generate the list of commands to remove all the keys and also tells us
which key the "auto" setting comes from. The hard coding of some
filenames in add_config_scope_arg() is unfortunate but as this temporary
code that will be removed when Git 3.0 is released I decided it wasn't
worth adding functions to get the name of the local and worktree config
files.

As we want the user to update their config we do not provide a way for
this advice to be disabled other than changing the value of
core.commentChar or core.commentString.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c            | 188 ++++++++++++++++++++++++++++++++++++
 t/t7502-commit-porcelain.sh |  28 +++++-
 2 files changed, 215 insertions(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8794b24572b..21839db7fce 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -41,6 +41,8 @@
 #include "commit-reach.h"
 #include "commit-graph.h"
 #include "pretty.h"
+#include "quote.h"
+#include "strmap.h"
 #include "trailer.h"
 
 static const char * const builtin_commit_usage[] = {
@@ -684,12 +686,198 @@ static int author_date_is_interesting(void)
 }
 
 #ifndef WITH_BREAKING_CHANGES
+struct comment_char_cfg {
+	unsigned last_key_id;
+	int auto_set_in_file;
+	struct strintmap key_flags;
+	size_t alloc, nr;
+	struct comment_char_cfg_item {
+		unsigned key_id;
+		char *path;
+		enum config_scope scope;
+	} *item;
+};
+
+#define COMMENT_CHAR_CFG_INIT { .key_flags = STRINTMAP_INIT }
+
+static void comment_char_cfg_release(struct comment_char_cfg *cfg)
+{
+	strintmap_clear(&cfg->key_flags);
+	for (size_t i = 0; i < cfg->nr; i++)
+		free(cfg->item[i].path);
+	free(cfg->item);
+}
+
+/* Used to track whether the key occurs more than once in a given file */
+#define KEY_SEEN_ONCE 1u
+#define KEY_SEEN_TWICE 2u
+#define COMMENT_KEY_SHIFT(id) (2 * (id))
+#define COMMENT_KEY_MASK(id) (3u << COMMENT_KEY_SHIFT(id))
+
+static void set_comment_key_flags(struct comment_char_cfg *cfg,
+				  const char *path, unsigned id, unsigned value)
+{
+	unsigned old = strintmap_get(&cfg->key_flags, path);
+	unsigned new = (old & ~COMMENT_KEY_MASK(id)) |
+				value << COMMENT_KEY_SHIFT(id);
+
+	strintmap_set(&cfg->key_flags, path, new);
+}
+
+static unsigned get_comment_key_flags(struct comment_char_cfg *cfg,
+				      const char *path, unsigned id)
+{
+	unsigned value = strintmap_get(&cfg->key_flags, path);
+
+	return (value & COMMENT_KEY_MASK(id)) >> COMMENT_KEY_SHIFT(id);
+}
+
+static const char* comment_key_name(unsigned id)
+{
+	static const char *name[] = {
+		"core.commentChar", "core.commentString",
+	};
+
+	if (id >= ARRAY_SIZE(name))
+		BUG("invalid comment key id");
+
+	return name[id];
+}
+
+static int comment_char_config_cb(const char *key, const char *value,
+				  const struct config_context *ctx, void *data)
+{
+	struct comment_char_cfg *cfg = data;
+	const struct key_value_info *kvi = ctx->kvi;
+	unsigned key_id;
+
+	if (!strcmp(key, "core.commentchar"))
+		key_id = 0;
+	else if (!strcmp(key, "core.commentstring"))
+		key_id = 1;
+	else
+		return 0;
+
+	cfg->last_key_id = key_id;
+	if (!kvi->path) {
+		return 0;
+	} else if (get_comment_key_flags(cfg, kvi->path, key_id)) {
+		set_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_TWICE);
+	} else {
+		struct comment_char_cfg_item *item;
+
+		ALLOC_GROW_BY(cfg->item, cfg->nr, 1, cfg->alloc);
+		item = &cfg->item[cfg->nr - 1];
+		item->key_id = key_id;
+		item->scope = kvi->scope;
+		item->path = xstrdup(kvi->path);
+		set_comment_key_flags(cfg, kvi->path, key_id, KEY_SEEN_ONCE);
+	}
+	cfg->auto_set_in_file =	value && !strcmp(value, "auto");
+
+	return 0;
+}
+
+static void add_config_scope_arg(struct strbuf *buf,
+				 struct comment_char_cfg_item *item)
+{
+	char *global_config = git_global_config();
+	char *system_config = git_system_config();
+
+	if (fspatheq(item->path, system_config)) {
+		strbuf_addstr(buf, "--system ");
+	} else if (fspatheq(item->path, global_config)) {
+		strbuf_addstr(buf, "--global ");
+	} else if (fspatheq(item->path,
+			    mkpath("%s/config",
+				   repo_get_git_dir(the_repository)))) {
+		; /* --local is the default */
+	} else if (fspatheq(item->path,
+			    mkpath("%s/config.worktree",
+				   repo_get_common_dir(the_repository)))) {
+		strbuf_addstr(buf, "--worktree ");
+	} else {
+		const char *path = item->path;
+		const char *home = getenv("HOME");
+
+		strbuf_addstr(buf, "--file ");
+		if (home && !fspathncmp(path, home, strlen(home))) {
+			path += strlen(home);
+			if (!fspathncmp(path, "/", 1))
+				path++;
+			strbuf_addstr(buf, "~/");
+		}
+		sq_quote_buf_pretty(buf, path);
+		strbuf_addch(buf, ' ');
+	}
+
+	free(global_config);
+	free(system_config);
+}
+
+static void add_optional_comment_char_advice(struct comment_char_cfg *cfg)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct comment_char_cfg_item *item;
+	/* TRANSLATORS this is a place holder for the value of core.commentString */
+	const char *placeholder = _("<comment string>");
+
+	/*
+	 * If auto is set in the last file that we saw advise the user how to
+	 * update their config.
+	 */
+	if (!cfg->auto_set_in_file)
+		return;
+
+	for (size_t i = 0; i < cfg->nr; i++) {
+		item = &cfg->item[i];
+
+		strbuf_addstr(&buf, "    git config unset ");
+		add_config_scope_arg(&buf, item);
+		if (get_comment_key_flags(cfg, item->path, item->key_id) == KEY_SEEN_TWICE)
+			strbuf_addstr(&buf, "--all ");
+		strbuf_addf(&buf, "%s\n", comment_key_name(item->key_id));
+	}
+	advise(_("\nTo use the default comment string (#) please run\n\n%s"),
+	       buf.buf);
+
+	item = &cfg->item[cfg->nr - 1];
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, "    git config set ");
+	add_config_scope_arg(&buf, item);
+	strbuf_addf(&buf, "%s %s\n", comment_key_name(item->key_id),
+		    placeholder);
+	advise(_("\nTo set a custom comment string please run\n\n"
+		 "%s\nwhere '%s' is the string you wish to use.\n"),
+	       buf.buf, placeholder);
+	strbuf_release(&buf);
+}
+
+static void advise_auto_comment_char(void)
+{
+	struct comment_char_cfg cfg = COMMENT_CHAR_CFG_INIT;
+	struct config_options opts = {
+		.commondir = repo_get_common_dir(the_repository),
+		.git_dir = repo_get_git_dir(the_repository),
+		.respect_includes = 1,
+	};
+
+	config_with_options(comment_char_config_cb, &cfg, NULL, the_repository,
+			    &opts);
+	advise(_("Support for '%s=auto' is deprecated and will be removed in "
+		 "git 3.0\n"), comment_key_name(cfg.last_key_id));
+	add_optional_comment_char_advice(&cfg);
+	comment_char_cfg_release(&cfg);
+}
+
 static void adjust_comment_line_char(const struct strbuf *sb)
 {
 	char candidates[] = "#;@!$%^&|:";
 	char *candidate;
 	const char *p;
 
+	advise_auto_comment_char();
+
 	if (!memchr(sb->buf, candidates[0], sb->len)) {
 		free(comment_line_str_to_free);
 		comment_line_str = comment_line_str_to_free =
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 65b4519a715..c8c00d316be 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -958,7 +958,33 @@ test_expect_success 'commit --status with custom comment character' '
 
 test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
-	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
+	cat >config-include <<-\EOF &&
+	[core]
+	    commentString=:
+	    commentString=%
+	    commentChar=auto
+	EOF
+	test_when_finished "rm config-include" &&
+	test_config include.path "$(pwd)/config-include" &&
+	test_config core.commentChar ! &&
+	GIT_EDITOR=.git/FAKE_EDITOR git commit --amend 2>err &&
+	sed -n "/^hint: *\$/s///p; /^hint: /s///p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+	    git config unset --file ~/config-include --all core.commentString
+	    git config unset --file ~/config-include core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set --file ~/config-include core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
+	EOF
+	test_cmp expect actual &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This series implements the plan to deprecate and remove support for
> core.commentChar=auto outlined in [1]. This feature has been the
> source of a couple of bug reports recently [2,3] and as explained in
> the first patch the design is tricky to fix. When git sees the
> deprecated config setting it will print advice like the example below
> to help the user either remove the setting or set a custom comment
> string.
>
> hint: Support for 'core.commentChar=auto' is deprecated and will be removed in git 3.0
> hint:
> hint: To use the default comment string (#) please run
> hint:
> hint:     git config unset --file ~/.config/git/config --all core.commentString
> hint:     git config unset --file ~/.config/git/config core.commentChar
> hint:     git config unset --global core.commentChar

We'd need to clear both variants from all scopes, wouldn't we?

	for scope in "" --local --global --worktree
	do
		for variant in commentString commentChar
		do
			git config unset $scope --all core.$variant
		done
	done

> hint:
> hint: To set a custom comment string please run
> hint:
> hint:     git config set --global core.commentChar <comment string>
> hint:
> hint: where '<comment string>' is the string you wish to use.

I do not particulary find it sensible to nudge users to use the same
commentChar across all projects with possibly different project
conventions by suggesting use of the --global option here.

It would be necessary to special case "auto" after 3.0 boundary
anyway, whether we (1) die when we notice the value is set to
"auto", and refuse to work until the user chooses a comment char, or
(2) use "#" or something hardcoded.  Either would be better than
using literal string "auto" as comment char.

So, a simpler approach might be to treat literal string "auto" as if
"#" was specified under WITH_BREAKING_CHANGES so that the end-user
does not have to do anything when they want to "revert" to the
default comment string.  Then we do not have to give any large text
like the above.  We can instead say something like

	The 'auto' setting of core.commentChar (or core.commentString) 
	will change its meaning in Git 3.0 and later and will always
	use the default '#'.

Hmm?

> [1] https://lore.kernel.org/git/6a3154e0-e7bc-45ae-b554-67ccab18727a@gmail.com
> [2] https://lore.kernel.org/git/20250315140913.577404-1-oswald.buddenhagen@gmx.de
> [3] https://lore.kernel.org/git/20250626132233.414789-1-ayu.chandekar@gmail.com
>
> Base-Commit: f0135a9047ca37d4d117dcf21f7e3e89fad85d00
> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fremove-auto-comment-char%2Fv1
> View-Changes-At: https://github.com/phillipwood/git/compare/f0135a904...83d0d3ece
> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/remove-auto-comment-char/v1
>
>
> Phillip Wood (2):
>   breaking-changes: deprecate support for core.commentString=auto
>   commit: print advice when core.commentString=auto
>
>  Documentation/BreakingChanges.adoc |   4 +
>  Documentation/config/core.adoc     |  20 ++-
>  builtin/commit.c                   | 192 +++++++++++++++++++++++++++++
>  config.c                           |   4 +
>  environment.c                      |   2 +
>  environment.h                      |   2 +
>  t/t3404-rebase-interactive.sh      |   2 +-
>  t/t7502-commit-porcelain.sh        |  32 ++++-
>  8 files changed, 252 insertions(+), 6 deletions(-)

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

On 08/07/2025 19:51, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> This series implements the plan to deprecate and remove support for
>> core.commentChar=auto outlined in [1]. This feature has been the
>> source of a couple of bug reports recently [2,3] and as explained in
>> the first patch the design is tricky to fix. When git sees the
>> deprecated config setting it will print advice like the example below
>> to help the user either remove the setting or set a custom comment
>> string.

With hindsight I should have been clearer here that the advice given is 
based on the user's config settings. In this case the files look like

~/.gitconfig:
     [core]
         commentChar = auto

~/$XDG_COMFIG/HOME/git/config
     [core]
         commentString = %
         commentString = !
         commentChar = auto

>> hint: Support for 'core.commentChar=auto' is deprecated and will be removed in git 3.0
>> hint:
>> hint: To use the default comment string (#) please run
>> hint:
>> hint:     git config unset --file ~/.config/git/config --all core.commentString
>> hint:     git config unset --file ~/.config/git/config core.commentChar
>> hint:     git config unset --global core.commentChar
> 
> We'd need to clear both variants from all scopes, wouldn't we?
> 
> 	for scope in "" --local --global --worktree
> 	do
> 		for variant in commentString commentChar
> 		do
> 			git config unset $scope --all core.$variant
> 		done
> 	done

The advice includes commands to clear all the scopes that are set. It 
correctly handles include files and correctly handles cases where both 
.gitconfig and $XDG_CONFIG_HOME/git/config exist (where "git config 
unset --global <key>" will only unset <key> in ~/.gitconfig)

>> hint:
>> hint: To set a custom comment string please run
>> hint:
>> hint:     git config set --global core.commentChar <comment string>
>> hint:
>> hint: where '<comment string>' is the string you wish to use.
> 
> I do not particulary find it sensible to nudge users to use the same
> commentChar across all projects with possibly different project
> conventions by suggesting use of the --global option here.

The advice will recommend a command that updates commentChar in the 
scope where it is currently set so if it is set globally it will not 
prompt you to set it locally in each repository and if it is set locally 
it will prompt you to update it there.
> It would be necessary to special case "auto" after 3.0 boundary
> anyway, whether we (1) die when we notice the value is set to
> "auto", and refuse to work until the user chooses a comment char, or
> (2) use "#" or something hardcoded.  Either would be better than
> using literal string "auto" as comment char.

We can do that if you've changed your view from 
<xmqqfrj6vfsn.fsf@gitster.g>

> So, a simpler approach might be to treat literal string "auto" as if
> "#" was specified under WITH_BREAKING_CHANGES so that the end-user
> does not have to do anything when they want to "revert" to the
> default comment string.  Then we do not have to give any large text
> like the above.  We can instead say something like
> 
> 	The 'auto' setting of core.commentChar (or core.commentString)
> 	will change its meaning in Git 3.0 and later and will always
> 	use the default '#'.

That's certainly simpler for us but it does not help the user to update 
their config. Presumably they're using the auto commentchar because '#' 
does not work for them.

Thanks

Phillip
> Hmm?
> 
>> [1] https://lore.kernel.org/git/6a3154e0-e7bc-45ae-b554-67ccab18727a@gmail.com
>> [2] https://lore.kernel.org/git/20250315140913.577404-1-oswald.buddenhagen@gmx.de
>> [3] https://lore.kernel.org/git/20250626132233.414789-1-ayu.chandekar@gmail.com
>>
>> Base-Commit: f0135a9047ca37d4d117dcf21f7e3e89fad85d00
>> Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fremove-auto-comment-char%2Fv1
>> View-Changes-At: https://github.com/phillipwood/git/compare/f0135a904...83d0d3ece
>> Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/remove-auto-comment-char/v1
>>
>>
>> Phillip Wood (2):
>>    breaking-changes: deprecate support for core.commentString=auto
>>    commit: print advice when core.commentString=auto
>>
>>   Documentation/BreakingChanges.adoc |   4 +
>>   Documentation/config/core.adoc     |  20 ++-
>>   builtin/commit.c                   | 192 +++++++++++++++++++++++++++++
>>   config.c                           |   4 +
>>   environment.c                      |   2 +
>>   environment.h                      |   2 +
>>   t/t3404-rebase-interactive.sh      |   2 +-
>>   t/t7502-commit-porcelain.sh        |  32 ++++-
>>   8 files changed, 252 insertions(+), 6 deletions(-)

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> With hindsight I should have been clearer here that the advice given
> is based on the user's config settings.

Ahh, OK.  If the "hint" advice message gets generated with custom
sequence of commands, that explains why the sample looked so uneven.
Disregard what I said about clearing every variant from every scope.

> The advice will recommend a command that updates commentChar in the
> scope where it is currently set so if it is set globally it will not
> prompt you to set it locally in each repository and if it is set
> locally it will prompt you to update it there.

Again, I misunderstood the set-up that would lead to the sample
output.  If the user has "auto" in ~/.gitconfig, replacing it at the
same place may make sense.

If the "auto" comes from /etc/gitconfig then we'd recommend
changing it there, instead of overriding it per-user in ~/.gitconfig?

>> It would be necessary to special case "auto" after 3.0 boundary
>> anyway, whether we (1) die when we notice the value is set to
>> "auto", and refuse to work until the user chooses a comment char, or
>> (2) use "#" or something hardcoded.  Either would be better than
>> using literal string "auto" as comment char.
>
> We can do that if you've changed your view from
> <xmqqfrj6vfsn.fsf@gitster.g>

Yeah, I think using "auto " as comment line prefix is simply a
nonsense.  Thanks.

>> So, a simpler approach might be to treat literal string "auto" as if
>> "#" was specified under WITH_BREAKING_CHANGES so that the end-user
>> does not have to do anything when they want to "revert" to the
>> default comment string.  Then we do not have to give any large text
>> like the above.  We can instead say something like
>> 	The 'auto' setting of core.commentChar (or core.commentString)
>> 	will change its meaning in Git 3.0 and later and will always
>> 	use the default '#'.
>
> That's certainly simpler for us but it does not help the user to
> update their config. Presumably they're using the auto commentchar
> because '#' does not work for them.

OK.  But those with "auto" because '#' did not work for them are
setting "auto" not because '#' does not work, but because none of
these "#;@!$%^&|:" work for them, no?

As you said earlier, the "auto" setting cannot fundamentally work at
all if we let a third-party inject any commented material into the
editor buffer.  The comment we inject ourselves we can control (and
notice), and perhaps back in the simpler days when "auto" setting
was invented, it was sufficient.  But that may be no longer true, so
it may not be just "tricky to fix" but simply "unworkable".  From
that point of view, as long as the reason clearly is explained to
end-users, I am fine with "'auto' stops Git and you'd need to unset
or set it to something else at the 3.0 boundary".

Thanks.

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

On 09/07/2025 17:20, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> With hindsight I should have been clearer here that the advice given
>> is based on the user's config settings.
> 
> Ahh, OK.  If the "hint" advice message gets generated with custom
> sequence of commands, that explains why the sample looked so uneven.
> Disregard what I said about clearing every variant from every scope.
> 
>> The advice will recommend a command that updates commentChar in the
>> scope where it is currently set so if it is set globally it will not
>> prompt you to set it locally in each repository and if it is set
>> locally it will prompt you to update it there.
> 
> Again, I misunderstood the set-up that would lead to the sample
> output.  If the user has "auto" in ~/.gitconfig, replacing it at the
> same place may make sense.
> 
> If the "auto" comes from /etc/gitconfig then we'd recommend
> changing it there, instead of overriding it per-user in ~/.gitconfig?

Yes, though I'm on the fence about that. I wonder if we should recommend 
~/.gitconfig instead if the user account that git is running under does 
not have write access to /etc/gitconfig. That also raises the question 
of what advice we should give about clearing settings in the system 
config file if the user does not have write access to it. It is possible 
the human user has write access to the system config even if the user 
account that git is running under does not but we have no way of finding 
that out.

>>> It would be necessary to special case "auto" after 3.0 boundary
>>> anyway, whether we (1) die when we notice the value is set to
>>> "auto", and refuse to work until the user chooses a comment char, or
>>> (2) use "#" or something hardcoded.  Either would be better than
>>> using literal string "auto" as comment char.

I'm leaning towards dying to avoid any nasty surprises when the commit 
message contains lines beginning with '#'.

I'll try and re-roll next week

Thanks

Phillip

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> If the "auto" comes from /etc/gitconfig then we'd recommend
>> changing it there, instead of overriding it per-user in ~/.gitconfig?
>
> Yes, though I'm on the fence about that. I wonder if we should
> recommend ~/.gitconfig instead if the user account that git is running
> under does not have write access to /etc/gitconfig. That also raises
> the question of what advice we should give about clearing settings in
> the system config file if the user does not have write access to
> it. It is possible the human user has write access to the system
> config even if the user account that git is running under does not but
> we have no way of finding that out.

Isn't it last-one-wins?  How about just telling them to do without
any "git config unset" and just do a single "git config set", either
to the repository (when the "auto" we saw came from the repository)
or to the per-user configuration (when the "auto" we saw came from
elsewhere, either per-user, or system-wide)?

>>>> It would be necessary to special case "auto" after 3.0 boundary
>>>> anyway, whether we (1) die when we notice the value is set to
>>>> "auto", and refuse to work until the user chooses a comment char, or
>>>> (2) use "#" or something hardcoded.  Either would be better than
>>>> using literal string "auto" as comment char.
>
> I'm leaning towards dying to avoid any nasty surprises when the commit
> message contains lines beginning with '#'.
>
> I'll try and re-roll next week

Thanks!

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Oswald Buddenhagen]

On Fri, Jul 11, 2025 at 10:07:01AM -0700, Junio C Hamano wrote:
>Isn't it last-one-wins?  How about just telling them to do without
>any "git config unset" [...]
>
i wouldn't bother suggesting specific fixes, and just suggest using `git 
config list --show-scope` to figure out where the config comes from.  
waaaay simpler, and avoids the pesky policy questions.

i'm also in favor of just refusing to operate when the 'auto' setting is 
encountered, as that also is the simplest and fail-safe approach.

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> On Fri, Jul 11, 2025 at 10:07:01AM -0700, Junio C Hamano wrote:
>>Isn't it last-one-wins?  How about just telling them to do without
>>any "git config unset" [...]
>>
> i wouldn't bother suggesting specific fixes, and just suggest using
> `git config list --show-scope` to figure out where the config comes
> from.  waaaay simpler, and avoids the pesky policy questions.

Yes, making it explicitly a responsibility of the end-users to
figure out what is the best approach to take would always work.

The approach taken by the patch gives series of commands that can be
copied and pasted without thinking, which is, even though it does
imply that we make a policy decision for those who do not want to
think for themselves, easy to use, though.

> i'm also in favor of just refusing to operate when the 'auto' setting
> is encountered, as that also is the simplest and fail-safe approach.

Yes, I agree that it is a very sensible thing to do.

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I'm leaning towards dying to avoid any nasty surprises when the commit
> message contains lines beginning with '#'.
>
> I'll try and re-roll next week

What's the current state of this effort?

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

On 27/07/2025 00:15, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I'm leaning towards dying to avoid any nasty surprises when the commit
>> message contains lines beginning with '#'.
>>
>> I'll try and re-roll next week
> 
> What's the current state of this effort?

It's getting there, unfortunately it has taken longer than I thought it 
would. I reasonably confident that I'll have something to post later 
this week.

Thanks

Phillip

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> This series implements the plan to deprecate and remove support for
> core.commentChar=auto outlined in [1]. This feature has been the
> source of a couple of bug reports recently [2,3] and as explained in
> the first patch the design is tricky to fix.

FWIW, this fails some tests that expect "# commented lines" by
treating "auto" too literally.

https://github.com/git/git/actions/runs/16157263228/job/45602188411#step:10:2970

I wonder if our braincycles are better spent to actually perform the
"tricky"[*] fix than deprecating the feature and then perfecting the
deprecation process (which does not seem to be without cost either).

 - We can and should keep the "auto" magic and use '#' when it gets
   specified, if we really wanted to do this deprecation.  I am not
   a huge fan of it, though.

 - Or leave it as a known-broken feature in certain corner cases,
   which may motivate some future developers to tackle these
   "tricky" code paths.  If we were to go this route, the first step
   would be to document what works and what does not as "known
   limitations".  I am slightly more in favor of this than "we punt,
   because we cannot fix it", but not by a large margin.

So, I dunno.

Thanks.

[Footnote]

 * Essentially we would need to collect all information (like hook
   output and template files) before we produce our own message to
   be commented out because we need to know what symbol is
   available.  Such a change may mean a major reshuffling of some
   code paths (or worse, some code paths may have to be made to fail
   and retry).  As long as the damage is limited to the case where
   "auto" setting is used, such a "solution" is acceptable.

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Ayush Chandekar]

On Wed, Jul 9, 2025 at 6:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > This series implements the plan to deprecate and remove support for
> > core.commentChar=auto outlined in [1]. This feature has been the
> > source of a couple of bug reports recently [2,3] and as explained in
> > the first patch the design is tricky to fix.
>
> FWIW, this fails some tests that expect "# commented lines" by
> treating "auto" too literally.
>
> https://github.com/git/git/actions/runs/16157263228/job/45602188411#step:10:2970
>

The failing test is a test which I added in the bug-fix patch: [1]
I don't understand what you meant by "treating auto too literally".

> I wonder if our braincycles are better spent to actually perform the
> "tricky"[*] fix than deprecating the feature and then perfecting the
> deprecation process (which does not seem to be without cost either).
>
>  - We can and should keep the "auto" magic and use '#' when it gets
>    specified, if we really wanted to do this deprecation.  I am not
>    a huge fan of it, though.
>
>  - Or leave it as a known-broken feature in certain corner cases,
>    which may motivate some future developers to tackle these
>    "tricky" code paths.  If we were to go this route, the first step
>    would be to document what works and what does not as "known
>    limitations".  I am slightly more in favor of this than "we punt,
>    because we cannot fix it", but not by a large margin.
>
> So, I dunno.
>
> Thanks.
>
> [Footnote]
>
>  * Essentially we would need to collect all information (like hook
>    output and template files) before we produce our own message to
>    be commented out because we need to know what symbol is
>    available.  Such a change may mean a major reshuffling of some
>    code paths (or worse, some code paths may have to be made to fail
>    and retry).  As long as the damage is limited to the case where
>    "auto" setting is used, such a "solution" is acceptable.

[1]: https://lore.kernel.org/git/20250630182527.69167-1-ayu.chandekar@gmail.com

Re: [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

On 09/07/2025 02:27, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> This series implements the plan to deprecate and remove support for
>> core.commentChar=auto outlined in [1]. This feature has been the
>> source of a couple of bug reports recently [2,3] and as explained in
>> the first patch the design is tricky to fix.
> 
> FWIW, this fails some tests that expect "# commented lines" by
> treating "auto" too literally.
> 
> https://github.com/git/git/actions/runs/16157263228/job/45602188411#step:10:2970

That's a semantic conflict between this series and seen - the test 
should be marked !WITH_BREAKING_CHANGES as it is testing 
core.commentChar=auto
> I wonder if our braincycles are better spent to actually perform the
> "tricky"[*] fix than deprecating the feature and then perfecting the
> deprecation process (which does not seem to be without cost either).
> 
>   - We can and should keep the "auto" magic and use '#' when it gets
>     specified, if we really wanted to do this deprecation.  I am not
>     a huge fan of it, though.
> 
>   - Or leave it as a known-broken feature in certain corner cases,
>     which may motivate some future developers to tackle these
>     "tricky" code paths.  If we were to go this route, the first step
>     would be to document what works and what does not as "known
>     limitations".  I am slightly more in favor of this than "we punt,
>     because we cannot fix it", but not by a large margin.

I've thought about fixing it but I don't see a good way for templates 
and hooks to say "lines that begin with this character are comments" - 
for a hook the choice of comment char needs to be based on the message 
so we cannot use a fixed config setting. Unless we have a way of doing 
that it is never going to work. The cherry-pick/rebase issues could be 
addressed by writing some state that is then read by "git commit" though 
goodness knows how we'd make that work with the prepare-commit-msg hook 
that wants to introduce its own comments. I don't really see the point 
of just fixing the rebase case if templates and hooks are still broken.

Now that the commentChar can be an arbitrary string rather than just a 
single byte I think is much easier for users to pick something unique 
that isn't going to be at the start of a line in their commit messages.

Best Wishes

Phillip

[PATCH v2 0/3] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Thanks to Ayush, Junio and Oswald for their comments on V1.
This series implements the plan to deprecate and remove support for
core.commentChar=auto outlined in [1]. This feature has been the
source of a couple of bug reports recently [2,3] and it is hard to
see how the design can be fixed as it is incompatible with preparing
a commit message template containing comments. When git sees the
deprecated config setting it will print advice based on the user's
config setting to help the user either remove the setting or set a
custom comment string. In the example below core.commentString is set
multiple times in $XDG_CONFIG_HOME/git/config and core.commentChar
is set in ~/.gitconfig and $XDG_CONFIG_HOME/git/config.

warning: Support for 'core.commentChar=auto' is deprecated and will be removed in Git 3.0
hint:
hint: To use the default comment string (#) please run
hint:
hint:     git config unset --file ~/.config/git/config --all core.commentString
hint:     git config unset --file ~/.config/git/config core.commentChar
hint:     git config unset --global core.commentChar
hint:
hint: To set a custom comment string please run
hint:
hint:     git config set --global core.commentChar <comment string>
hint:
hint: where '<comment string>' is the string you wish to use.

[1] https://lore.kernel.org/git/6a3154e0-e7bc-45ae-b554-67ccab18727a@gmail.com
[2] https://lore.kernel.org/git/20250315140913.577404-1-oswald.buddenhagen@gmx.de
[3] https://lore.kernel.org/git/20250626132233.414789-1-ayu.chandekar@gmail.com

Changes since V1:
 - Rebased onto a merge of 'ps/config-wo-the-repository' and 'master'
 - Reworded commit messages
 - What was patch 2 has been split into two separate patches and
   reworked to die when core.commentChar=auto and WITH_BREAKING_CHANGES
   is enabled.

Base-Commit: 1ae5bd276bdf101e37c1a8f2904a2eae05fbb744
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fremove-auto-comment-char%2Fv2
View-Changes-At: https://github.com/phillipwood/git/compare/1ae5bd276...0e7c08b15
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/remove-auto-comment-char/v2


Phillip Wood (3):
  breaking-changes: deprecate support for core.commentString=auto
  config: warn on core.commentString=auto
  commit: print advice when core.commentString=auto

 Documentation/BreakingChanges.adoc |   5 +
 Documentation/config/core.adoc     |  20 +-
 builtin/commit.c                   |   7 +
 builtin/merge.c                    |   3 +
 builtin/rebase.c                   |   3 +
 builtin/revert.c                   |   7 +
 config.c                           | 297 ++++++++++++++++++++++++++++-
 environment.c                      |  11 +-
 environment.h                      |   3 +
 repository.c                       |   1 +
 repository.h                       |   3 +
 t/t3404-rebase-interactive.sh      |  19 +-
 t/t3418-rebase-continue.sh         |   2 +-
 t/t7502-commit-porcelain.sh        |  52 ++++-
 14 files changed, 421 insertions(+), 12 deletions(-)

Range-diff against v1:
1:  3747a1f77f0 < -:  ----------- breaking-changes: deprecate support for core.commentString=auto
2:  83d0d3ece86 < -:  ----------- commit: print advice when core.commentString=auto
-:  ----------- > 1:  a6355451d4b breaking-changes: deprecate support for core.commentString=auto
-:  ----------- > 2:  8b575980426 config: warn on core.commentString=auto
-:  ----------- > 3:  0e7c08b15e5 commit: print advice when core.commentString=auto
-- 
2.49.0.897.gfad3eb7d210

[PATCH v2 1/3] breaking-changes: deprecate support for core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When "core.commentString" is set to "auto" then "git commit" will
automatically select the comment character ensuring that it is not the
first character on any of the lines in the commit message. This was
introduced by commit 84c9dc2c5a2 (commit: allow core.commentChar=auto
for character auto selection, 2014-05-17) The motivation seems to be
to avoid commenting out lines from the existing message when amending
a commit that was created with a message from a file.

Unfortunately this feature does not work with:

 * commit message templates that contain comments.

 * prepare-commit-msg hooks that introduce comments.

 * "git commit --cleanup=strip --edit -F <file>" which means that it
   is incompatible with

   - the "fixup" and "squash" commands of "git rebase -i" as the
     comments added by those commands are then treated as part of
     the commit message.

   - the conflict comments added to the commit message by "git
     cherry-pick", "git rebase" etc. as these comments are then
     treated as part of the commit message.

It is also ignored by "git notes" when amending a note.

The issues with comments coming from a template, hook or file are a
consequence of the design of this feature and are therefore hard to
fix.

As the costs of this feature outweigh the benefits deprecate it and
remove it in Git 3.0. If someone comes up with some patches that fix
all the issues in a maintainable way then I'd be happy to see this
change reverted.

The next commits will add a warning and some advice for users on how
they can update their config settings.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/BreakingChanges.adoc |  5 +++++
 Documentation/config/core.adoc     | 20 ++++++++++++++++++--
 builtin/commit.c                   |  4 ++++
 environment.c                      | 10 ++++++++--
 environment.h                      |  2 ++
 t/t3404-rebase-interactive.sh      |  2 +-
 t/t3418-rebase-continue.sh         |  2 +-
 t/t7502-commit-porcelain.sh        |  4 ++--
 8 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index f8d2eba061c..344ce500603 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -239,6 +239,11 @@ These features will be removed.
 +
 The command will be removed.
 
+* Support for `core.commentString=auto` has been deprecated and will
+  be removed in Git 3.0.
++
+cf. <xmqqa59i45wc.fsf@gitster.g>
+
 == Superseded features that will not be deprecated
 
 Some features have gained newer replacements that aim to improve the design in
diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
index 9fde1ab63a7..7133f00c38b 100644
--- a/Documentation/config/core.adoc
+++ b/Documentation/config/core.adoc
@@ -531,9 +531,25 @@ core.commentString::
 	commented, and removes them after the editor returns
 	(default '#').
 +
-If set to "auto", `git-commit` would select a character that is not
+ifndef::with-breaking-changes[]
+If set to "auto", `git-commit` will select a character that is not
 the beginning character of any line in existing commit messages.
-+
+Support for this value is deprecated and will be removed in Git 3.0
+due to the following limitations:
++
+--
+* It is incompatible with adding comments in a commit message
+  template. This includes the conflicts comments added to
+  the commit message by `cherry-pick`, `merge`, `rebase` and
+  `revert`.
+* It is incompatible with adding comments to the commit message
+  in the `prepare-commit-msg` hook.
+* It is incompatible with the `fixup` and `squash` commands when
+  rebasing,
+* It is not respected by `git notes`
+--
++
+endif::with-breaking-changes[]
 Note that these two variables are aliases of each other, and in modern
 versions of Git you are free to use a string (e.g., `//` or `⁑⁕⁑`) with
 `commentChar`. Versions of Git prior to v2.45.0 will ignore
diff --git a/builtin/commit.c b/builtin/commit.c
index 757f51eac82..d25cc07a355 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -683,6 +683,7 @@ static int author_date_is_interesting(void)
 	return author_message || force_date;
 }
 
+#ifndef WITH_BREAKING_CHANGES
 static void adjust_comment_line_char(const struct strbuf *sb)
 {
 	char candidates[] = "#;@!$%^&|:";
@@ -720,6 +721,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	free(comment_line_str_to_free);
 	comment_line_str = comment_line_str_to_free = xstrfmt("%c", *p);
 }
+#endif /* !WITH_BREAKING_CHANGES */
 
 static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
 				struct pretty_print_context *ctx)
@@ -916,8 +918,10 @@ 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"));
 
+#ifndef WITH_BREAKING_CHANGES
 	if (auto_comment_line_char)
 		adjust_comment_line_char(&sb);
+#endif /* !WITH_BREAKING_CHANGES */
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
diff --git a/environment.c b/environment.c
index a0ac5934b37..4c87876d483 100644
--- a/environment.c
+++ b/environment.c
@@ -122,7 +122,9 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT;
  */
 const char *comment_line_str = "#";
 char *comment_line_str_to_free;
+#ifndef WITH_BREAKING_CHANGES
 int auto_comment_line_char;
+#endif /* !WITH_BREAKING_CHANGES */
 
 /* This is set by setup_git_directory_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
@@ -459,18 +461,22 @@ static int git_default_core_config(const char *var, const char *value,
 
 	if (!strcmp(var, "core.commentchar") ||
 	    !strcmp(var, "core.commentstring")) {
-		if (!value)
+		if (!value) {
 			return config_error_nonbool(var);
-		else if (!strcasecmp(value, "auto")) {
+#ifndef WITH_BREAKING_CHANGES
+		} else if (!strcasecmp(value, "auto")) {
 			auto_comment_line_char = 1;
 			FREE_AND_NULL(comment_line_str_to_free);
 			comment_line_str = "#";
+#endif /* !WITH_BREAKING_CHANGES */
 		} else if (value[0]) {
 			if (strchr(value, '\n'))
 				return error(_("%s cannot contain newline"), var);
 			comment_line_str = value;
 			FREE_AND_NULL(comment_line_str_to_free);
+#ifndef WITH_BREAKING_CHANGES
 			auto_comment_line_char = 0;
+#endif /* !WITH_BREAKING_CHANGES */
 		} else
 			return error(_("%s must have at least one character"), var);
 		return 0;
diff --git a/environment.h b/environment.h
index 8cfce41015b..e75c4abb388 100644
--- a/environment.h
+++ b/environment.h
@@ -208,7 +208,9 @@ extern char *excludes_file;
  */
 extern const char *comment_line_str;
 extern char *comment_line_str_to_free;
+#ifndef WITH_BREAKING_CHANGES
 extern int auto_comment_line_char;
+#endif /* !WITH_BREAKING_CHANGES */
 
 # endif /* USE_THE_REPOSITORY_VARIABLE */
 #endif /* ENVIRONMENT_H */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6bac217ed35..ce0aebb9a7e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_success 'rebase -i respects core.commentchar=auto' '
+test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=auto' '
 	test_config core.commentchar auto &&
 	write_script copy-edit-script.sh <<-\EOF &&
 	cp "$1" edit-script
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index b8a8dd77e74..f9b8999db50 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -328,7 +328,7 @@ test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas
 	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
 '
 
-test_expect_success 'no change in comment character due to conflicts markers with core.commentChar=auto' '
+test_expect_success !WITH_BREAKING_CHANGES 'no change in comment character due to conflicts markers with core.commentChar=auto' '
 	git checkout -b branch-a &&
 	test_commit A F1 &&
 	git checkout -b branch-b HEAD^ &&
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b37e2018a74..65b4519a715 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -956,13 +956,13 @@ test_expect_success 'commit --status with custom comment character' '
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'switch core.commentchar' '
+test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
 	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'switch core.commentchar but out of options' '
+test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar but out of options' '
 	cat >text <<\EOF &&
 # 1
 ; 2
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH v2 1/3] breaking-changes: deprecate support for core.commentString=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> When "core.commentString" is set to "auto" then "git commit" will
> automatically select the comment character ensuring that it is not the
> first character on any of the lines in the commit message. This was
> introduced by commit 84c9dc2c5a2 (commit: allow core.commentChar=auto
> for character auto selection, 2014-05-17) The motivation seems to be

"5-17) The" -> "5-17). The".

> to avoid commenting out lines from the existing message when amending
> a commit that was created with a message from a file.
>
> Unfortunately this feature does not work with:
>
>  * commit message templates that contain comments.
>
>  * prepare-commit-msg hooks that introduce comments.
>
>  * "git commit --cleanup=strip --edit -F <file>" which means that it
>    is incompatible with
>
>    - the "fixup" and "squash" commands of "git rebase -i" as the
>      comments added by those commands are then treated as part of
>      the commit message.
>
>    - the conflict comments added to the commit message by "git
>      cherry-pick", "git rebase" etc. as these comments are then
>      treated as part of the commit message.
>
> It is also ignored by "git notes" when amending a note.
>
> The issues with comments coming from a template, hook or file are a
> consequence of the design of this feature and are therefore hard to
> fix.
>
> As the costs of this feature outweigh the benefits deprecate it and

"the benefits deprecate" -> "the benefits, deprecate".

> remove it in Git 3.0. If someone comes up with some patches that fix
> all the issues in a maintainable way then I'd be happy to see this
> change reverted.
>
> The next commits will add a warning and some advice for users on how
> they can update their config settings.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  +
> -If set to "auto", `git-commit` would select a character that is not
> +ifndef::with-breaking-changes[]
> +If set to "auto", `git-commit` will select a character that is not
>  the beginning character of any line in existing commit messages.
> -+
> +Support for this value is deprecated and will be removed in Git 3.0
> +due to the following limitations:
> ++
> +--
> +* It is incompatible with adding comments in a commit message
> +  template. This includes the conflicts comments added to
> +  the commit message by `cherry-pick`, `merge`, `rebase` and
> +  `revert`.
> +* It is incompatible with adding comments to the commit message
> +  in the `prepare-commit-msg` hook.
> +* It is incompatible with the `fixup` and `squash` commands when
> +  rebasing,
> +* It is not respected by `git notes`
> +--
> ++
> +endif::with-breaking-changes[]

The above is shown to everybody before the 3.0 happens (and if you
opt into 3.0 early, you will stop seeing it earlier than others).

The rest of the patch looks good to me.

[PATCH v2 2/3] config: warn on core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As support for this setting was deprecated in the last commit print a
warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
When printing a warning avoid bombarding the user by only printing it
when running commands commands that run "git commit" and only only
once per command. Some scaffolding is added to repo_read_config()
to allow it to detect deprecated config settings and warn about
them. As both "core.commentChar" and "core.commentString" set the
comment character we record which one of them is used and tailor the
warning message appropriately.

Note the odd combination of die_message() followed by die(NULL)
is to allow the next commit to insert a call to advise() in the middle.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c              |   3 +
 builtin/merge.c               |   3 +
 builtin/rebase.c              |   3 +
 builtin/revert.c              |   7 ++
 config.c                      | 116 +++++++++++++++++++++++++++++++++-
 environment.c                 |   1 +
 environment.h                 |   1 +
 repository.c                  |   1 +
 repository.h                  |   3 +
 t/t3404-rebase-interactive.sh |   7 +-
 t/t7502-commit-porcelain.sh   |  17 ++++-
 11 files changed, 158 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d25cc07a355..f821fdcfcc3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1783,6 +1783,9 @@ int cmd_commit(int argc,
 	show_usage_with_options_if_asked(argc, argv,
 					 builtin_commit_usage, builtin_commit_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/merge.c b/builtin/merge.c
index dc4cb8fb14d..794cb7bb269 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1378,6 +1378,9 @@ int cmd_merge(int argc,
 	show_usage_with_options_if_asked(argc, argv,
 					 builtin_merge_usage, builtin_merge_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 72a52bdfb98..962917ec480 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1242,6 +1242,9 @@ int cmd_rebase(int argc,
 					 builtin_rebase_usage,
 					 builtin_rebase_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/revert.c b/builtin/revert.c
index e07c2217fe8..b197848bb0a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -4,6 +4,7 @@
 #include "builtin.h"
 #include "parse-options.h"
 #include "diff.h"
+#include "environment.h"
 #include "gettext.h"
 #include "revision.h"
 #include "rerere.h"
@@ -285,6 +286,9 @@ int cmd_revert(int argc,
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int res;
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	opts.action = REPLAY_REVERT;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, prefix, &opts);
@@ -302,6 +306,9 @@ struct repository *repo UNUSED)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int res;
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, prefix, &opts);
diff --git a/config.c b/config.c
index 97ffef42700..c36ead76005 100644
--- a/config.c
+++ b/config.c
@@ -8,9 +8,11 @@
 
 #include "git-compat-util.h"
 #include "abspath.h"
+#include "advice.h"
 #include "date.h"
 #include "branch.h"
 #include "config.h"
+#include "dir.h"
 #include "parse.h"
 #include "convert.h"
 #include "environment.h"
@@ -1951,10 +1953,110 @@ int git_configset_get_pathname(struct config_set *set, const char *key, char **d
 		return 1;
 }
 
+struct comment_char_config {
+	unsigned last_key_id;
+	bool auto_set;
+};
+
+#define COMMENT_CHAR_CFG_INIT { 0 }
+
+static const char* comment_key_name(unsigned id)
+{
+	static const char *name[] = {
+		"core.commentChar",
+		"core.commentString",
+	};
+
+	if (id >= ARRAY_SIZE(name))
+		BUG("invalid comment key id");
+
+	return name[id];
+}
+
+static void comment_char_callback(const char *key, const char *value,
+				  const struct config_context *ctx UNUSED,
+				  void *data)
+{
+	struct comment_char_config *config = data;
+	unsigned key_id;
+
+	if (!strcmp(key, "core.commentchar"))
+		key_id = 0;
+	else if (!strcmp(key, "core.commentstring"))
+		key_id = 1;
+	else
+		return;
+
+	config->last_key_id = key_id;
+	config->auto_set = value && !strcmp(value, "auto");
+}
+
+struct repo_config {
+	struct repository *repo;
+	struct comment_char_config comment_char_config;
+};
+
+#define REPO_CONFIG_INIT(repo_) {				\
+		.comment_char_config = COMMENT_CHAR_CFG_INIT,	\
+		.repo = repo_,					\
+	};
+
+#ifdef WITH_BREAKING_CHANGES
+static void check_auto_comment_char_config(struct comment_char_config *config)
+{
+	if (!config->auto_set)
+		return;
+
+	die_message(_("Support for '%s=auto' has been removed in Git 3.0"),
+		    comment_key_name(config->last_key_id));
+	die(NULL);
+}
+#else
+static void check_auto_comment_char_config(struct comment_char_config *config)
+{
+	extern bool warn_on_auto_comment_char;
+	const char *DEPRECATED_CONFIG_ENV =
+				"GIT_AUTO_COMMENT_CHAR_CONFIG_WARNING_GIVEN";
+
+	if (!config->auto_set || !warn_on_auto_comment_char)
+		return;
+
+	/*
+	 * Use an environment variable to ensure that subprocesses do not repeat
+	 * the warning.
+	 */
+	if (git_env_bool(DEPRECATED_CONFIG_ENV, false))
+		return;
+
+	setenv(DEPRECATED_CONFIG_ENV, "true", true);
+
+	warning(_("Support for '%s=auto' is deprecated and will be removed in "
+		  "Git 3.0"), comment_key_name(config->last_key_id));
+}
+#endif /* WITH_BREAKING_CHANGES */
+
+static void check_deprecated_config(struct repo_config *config)
+{
+	if (!config->repo->check_deprecated_config)
+			return;
+
+	check_auto_comment_char_config(&config->comment_char_config);
+}
+
+static int repo_config_callback(const char *key, const char *value,
+				const struct config_context *ctx, void *data)
+{
+	struct repo_config *config = data;
+
+	comment_char_callback(key, value, ctx, &config->comment_char_config);
+	return config_set_callback(key, value, ctx, config->repo->config);
+}
+
 /* Functions use to read configuration from a repository */
 static void repo_read_config(struct repository *repo)
 {
 	struct config_options opts = { 0 };
+	struct repo_config config = REPO_CONFIG_INIT(repo);
 
 	opts.respect_includes = 1;
 	opts.commondir = repo->commondir;
@@ -1966,8 +2068,8 @@ static void repo_read_config(struct repository *repo)
 		git_configset_clear(repo->config);
 
 	git_configset_init(repo->config);
-	if (config_with_options(config_set_callback, repo->config, NULL,
-				repo, &opts) < 0)
+	if (config_with_options(repo_config_callback, &config, NULL, repo,
+				&opts) < 0)
 		/*
 		 * config_with_options() normally returns only
 		 * zero, as most errors are fatal, and
@@ -1980,6 +2082,7 @@ static void repo_read_config(struct repository *repo)
 		 * immediately.
 		 */
 		die(_("unknown error occurred while reading the configuration files"));
+	check_deprecated_config(&config);
 }
 
 static void git_config_check_init(struct repository *repo)
@@ -2667,6 +2770,14 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	char *contents = NULL;
 	size_t contents_sz;
 	struct config_store_data store = CONFIG_STORE_INIT;
+	bool saved_check_deprecated_config = r->check_deprecated_config;
+
+	/*
+	 * Do not warn or die if there are deprecated config settings as
+	 * we want the user to be able to change those settings by running
+	 * "git config".
+	 */
+	r->check_deprecated_config = false;
 
 	validate_comment_string(comment);
 
@@ -2898,6 +3009,7 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	if (in_fd >= 0)
 		close(in_fd);
 	config_store_data_clear(&store);
+	r->check_deprecated_config = saved_check_deprecated_config;
 	return ret;
 
 write_err_out:
diff --git a/environment.c b/environment.c
index 4c87876d483..1ffa2ff30b2 100644
--- a/environment.c
+++ b/environment.c
@@ -124,6 +124,7 @@ const char *comment_line_str = "#";
 char *comment_line_str_to_free;
 #ifndef WITH_BREAKING_CHANGES
 int auto_comment_line_char;
+bool warn_on_auto_comment_char;
 #endif /* !WITH_BREAKING_CHANGES */
 
 /* This is set by setup_git_directory_gently() and/or git_default_config() */
diff --git a/environment.h b/environment.h
index e75c4abb388..51898c99cd1 100644
--- a/environment.h
+++ b/environment.h
@@ -210,6 +210,7 @@ extern const char *comment_line_str;
 extern char *comment_line_str_to_free;
 #ifndef WITH_BREAKING_CHANGES
 extern int auto_comment_line_char;
+extern bool warn_on_auto_comment_char;
 #endif /* !WITH_BREAKING_CHANGES */
 
 # endif /* USE_THE_REPOSITORY_VARIABLE */
diff --git a/repository.c b/repository.c
index ecd691181fc..8af73923d34 100644
--- a/repository.c
+++ b/repository.c
@@ -57,6 +57,7 @@ void initialize_repository(struct repository *repo)
 	repo->parsed_objects = parsed_object_pool_new(repo);
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
+	repo->check_deprecated_config = true;
 
 	/*
 	 * When a command runs inside a repository, it learns what
diff --git a/repository.h b/repository.h
index 042dc93f0f2..5808a5d6108 100644
--- a/repository.h
+++ b/repository.h
@@ -161,6 +161,9 @@ struct repository {
 
 	/* Indicate if a repository has a different 'commondir' from 'gitdir' */
 	unsigned different_commondir:1;
+
+	/* Should repo_config() check for deprecated settings */
+	bool check_deprecated_config;
 };
 
 #ifdef USE_THE_REPOSITORY_VARIABLE
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ce0aebb9a7e..3b2a46c25ce 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1184,8 +1184,13 @@ test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=
 	test_when_finished "git rebase --abort || :" &&
 	(
 		test_set_editor "$(pwd)/copy-edit-script.sh" &&
-		git rebase -i HEAD^
+		git rebase -i HEAD^ 2>err
 	) &&
+	sed -n "s/^warning: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+	EOF
+	test_cmp expect actual &&
 	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
 '
 
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 65b4519a715..a9dc1e416d1 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -958,7 +958,12 @@ test_expect_success 'commit --status with custom comment character' '
 
 test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
-	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
+	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend 2>err &&
+	sed -n "s/^warning: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+	EOF
+	test_cmp expect actual &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
@@ -982,4 +987,14 @@ EOF
 	)
 '
 
+test_expect_success WITH_BREAKING_CHANGES 'core.commentChar=auto is rejected' '
+	test_config core.commentChar auto &&
+	test_must_fail git rev-parse --git-dir 2>err &&
+	sed -n "s/^fatal: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} has been removed in Git 3.0
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH v2 2/3] config: warn on core.commentString=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> diff --git a/config.c b/config.c
> index 97ffef42700..c36ead76005 100644
> --- a/config.c
> +++ b/config.c
> @@ -8,9 +8,11 @@
>  
>  #include "git-compat-util.h"
>  #include "abspath.h"
> +#include "advice.h"

Hmph.  Do you still need this?

I do not think a separate advice_if variable is warranted in this
case.  They see a warning that says that their "auto" will not do
anything useful in the future.  They will keep seeing it until they
decide what to use, and once they decide and set a value that is
different from "auto" to core.commentchar, they will stop seeing
the warning.

> +static const char* comment_key_name(unsigned id)

The asterisk sticks to the identifier, not type.

> +static void comment_char_callback(const char *key, const char *value,
> +				  const struct config_context *ctx UNUSED,
> +				  void *data)
> +{
> +	struct comment_char_config *config = data;
> +	unsigned key_id;
> +
> +	if (!strcmp(key, "core.commentchar"))
> +		key_id = 0;
> +	else if (!strcmp(key, "core.commentstring"))
> +		key_id = 1;
> +	else
> +		return;

Yuck.  We cannot help the joy of last-one-wins here X-<.

> +
> +	config->last_key_id = key_id;
> +	config->auto_set = value && !strcmp(value, "auto");
> +}

It probably becomes simpler (and easier to debug) if you made the
type of .last_key_id member "const char *" to point at the variable
name.  You are not switching on the .last_key_id member.  The only
use of that member is to be fed to die().  And by doing so, you can
drop comment_key_name().

> +struct repo_config {
> +	struct repository *repo;
> +	struct comment_char_config comment_char_config;
> +};
> +
> +#define REPO_CONFIG_INIT(repo_) {				\
> +		.comment_char_config = COMMENT_CHAR_CFG_INIT,	\
> +		.repo = repo_,					\
> +	};
> +
> +#ifdef WITH_BREAKING_CHANGES
> +static void check_auto_comment_char_config(struct comment_char_config *config)
> +{
> +	if (!config->auto_set)
> +		return;
> +
> +	die_message(_("Support for '%s=auto' has been removed in Git 3.0"),
> +		    comment_key_name(config->last_key_id));
> +	die(NULL);
> +}
> +#else
> +static void check_auto_comment_char_config(struct comment_char_config *config)
> +{
> +	extern bool warn_on_auto_comment_char;
> +	const char *DEPRECATED_CONFIG_ENV =
> +				"GIT_AUTO_COMMENT_CHAR_CONFIG_WARNING_GIVEN";
> +
> +	if (!config->auto_set || !warn_on_auto_comment_char)
> +		return;
> +
> +	/*
> +	 * Use an environment variable to ensure that subprocesses do not repeat
> +	 * the warning.
> +	 */
> +	if (git_env_bool(DEPRECATED_CONFIG_ENV, false))
> +		return;
> +
> +	setenv(DEPRECATED_CONFIG_ENV, "true", true);

I know this means well, but it might give users a better experience
if we went a much simpler route.  In your top-level project with two
submodules, you may have core.commentchar set to auto in the top-level
and only one of the submodules, and then you let "git" go recursive.
Wouldn't it be simpler for the user to diagnose which one(s) among
the three repositories need fixing, if the stderr said something
like:

    doing X
    warning core.commentChar is set to auto
    going into submodule A
      doing X
    going into submodule B
      doing X
      warning core.commentString is set to auto

I dunno.

> +	warning(_("Support for '%s=auto' is deprecated and will be removed in "
> +		  "Git 3.0"), comment_key_name(config->last_key_id));
> +}
> +#endif /* WITH_BREAKING_CHANGES */
> +
> +static void check_deprecated_config(struct repo_config *config)
> +{
> +	if (!config->repo->check_deprecated_config)
> +			return;
> +
> +	check_auto_comment_char_config(&config->comment_char_config);

The handling of .check_deprecated_config flag is a bit tricky, and
it is great that this design allows us to write a similar
check_foo_config() helper and make a call to it here, without 
having to worry about it again.

Re: [PATCH v2 2/3] config: warn on core.commentString=auto

[Phillip Wood]

Hi Junio

On 31/07/2025 22:17, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>   #include "git-compat-util.h"
>>   #include "abspath.h"
>> +#include "advice.h"
> 
> Hmph.  Do you still need this?

No, it's left over from splitting one patch into two. It should be in 
the next patch that adds a call to advise.

> I do not think a separate advice_if variable is warranted in this
> case.  They see a warning that says that their "auto" will not do
> anything useful in the future.  They will keep seeing it until they
> decide what to use, and once they decide and set a value that is
> different from "auto" to core.commentchar, they will stop seeing
> the warning.

I agree

>> +static const char* comment_key_name(unsigned id)
> 
> The asterisk sticks to the identifier, not type.

Oops I'll fix that

>> +
>> +	config->last_key_id = key_id;
>> +	config->auto_set = value && !strcmp(value, "auto");
>> +}
> 
> It probably becomes simpler (and easier to debug) if you made the
> type of .last_key_id member "const char *" to point at the variable
> name.  You are not switching on the .last_key_id member.  The only
> use of that member is to be fed to die().  And by doing so, you can
> drop comment_key_name().

I agree the integer id is pointless here, but it is useful in the next 
patch for tracking if a key occurs more than once in a given file.

>> +static void check_auto_comment_char_config(struct comment_char_config *config)
>> +{
>> +	extern bool warn_on_auto_comment_char;
>> +	const char *DEPRECATED_CONFIG_ENV =
>> +				"GIT_AUTO_COMMENT_CHAR_CONFIG_WARNING_GIVEN";
>> +
>> +	if (!config->auto_set || !warn_on_auto_comment_char)
>> +		return;
>> +
>> +	/*
>> +	 * Use an environment variable to ensure that subprocesses do not repeat
>> +	 * the warning.
>> +	 */
>> +	if (git_env_bool(DEPRECATED_CONFIG_ENV, false))
>> +		return;
>> +
>> +	setenv(DEPRECATED_CONFIG_ENV, "true", true);
> 
> I know this means well, but it might give users a better experience
> if we went a much simpler route.  In your top-level project with two
> submodules, you may have core.commentchar set to auto in the top-level
> and only one of the submodules, and then you let "git" go recursive.
> Wouldn't it be simpler for the user to diagnose which one(s) among
> the three repositories need fixing, if the stderr said something
> like:
> 
>      doing X
>      warning core.commentChar is set to auto
>      going into submodule A
>        doing X
>      going into submodule B
>        doing X
>        warning core.commentString is set to auto
> 
> I dunno.

There is definitely a trade off here. The motivation was to stop the 
sequencer printing the same warning every time it forked "git commit" 
and to stop each sumbodule warning about config set in the global config 
file. As you say the downside is that it hides warnings from a 
submodule's local config. I did wonder about somehow tracking the local 
config separately to the global and system config when setting the 
environment variable but it felt like it would get quite complicated 
tracking which .git/config we'd already warned about. We'd need to be 
clear about which repository the user needed to run 'git config' in as 
well which adds to the complications.

>> +static void check_deprecated_config(struct repo_config *config)
>> +{
>> +	if (!config->repo->check_deprecated_config)
>> +			return;
>> +
>> +	check_auto_comment_char_config(&config->comment_char_config);
> 
> The handling of .check_deprecated_config flag is a bit tricky, and
> it is great that this design allows us to write a similar
> check_foo_config() helper and make a call to it here, without
> having to worry about it again.

Thanks

Phillip

Re: [PATCH v2 2/3] config: warn on core.commentString=auto

[Oswald Buddenhagen]

On Thu, Jul 31, 2025 at 04:21:54PM +0100, Phillip Wood wrote:
>From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
>As support for this setting was deprecated in the last commit print a
>warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
>When printing a warning avoid bombarding the user by only printing it

>when running commands commands that run "git commit" and only only
>
something is very wrong with this line.

>once per command. Some scaffolding is added to repo_read_config()
>to allow it to detect deprecated config settings and warn about
>them. As both "core.commentChar" and "core.commentString" set the
>comment character we record which one of them is used and tailor the
>warning message appropriately.
>

[PATCH v2 3/3] commit: print advice when core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add some advice on how to change the config settings when
"core.commentString=auto" or "core.commentChar=auto". The advice
includes instructions for clearing the config setting or setting a
fixed comment string. To try and be as specific as possible, the advice
is customized based on the user's config. If "core.commentString=auto"
is set in the system config and the user does not have write
access then the advice omits the instructions to clear the config
and recommends changing the global config instead. An alternative
approach would be to advise the user to run "git config --show-origin"
and leave them to figure out how to fix it themselves but that seems
rather unfriendly. As we're forcing them to update their config we
should try and make that as easy as possible.

In order to generate this advice we need to record each file where
either of the config keys is set and whether a key occurs more that
once in a given file. This lets us generate the list of commands to
remove all the keys and also tells us which key the "auto" setting
comes from.

As we want the user to update their config we do not provide a way
for this advice to be disabled other than changing the value of
"core.commentChar" or "core.commentString".

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 config.c                      | 195 ++++++++++++++++++++++++++++++++--
 t/t3404-rebase-interactive.sh |  12 ++-
 t/t7502-commit-porcelain.sh   |  37 ++++++-
 3 files changed, 233 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index c36ead76005..e04cb386161 100644
--- a/config.c
+++ b/config.c
@@ -1956,9 +1956,51 @@ int git_configset_get_pathname(struct config_set *set, const char *key, char **d
 struct comment_char_config {
 	unsigned last_key_id;
 	bool auto_set;
+	bool auto_set_in_file;
+	struct strintmap key_flags;
+	size_t alloc, nr;
+	struct comment_char_config_item {
+		unsigned key_id;
+		char *path;
+		enum config_scope scope;
+	} *item;
 };
 
-#define COMMENT_CHAR_CFG_INIT { 0 }
+#define COMMENT_CHAR_CFG_INIT {			\
+		.key_flags = STRINTMAP_INIT,	\
+	}
+
+static void comment_char_config_release(struct comment_char_config *config)
+{
+	strintmap_clear(&config->key_flags);
+	for (size_t i = 0; i < config->nr; i++)
+		free(config->item[i].path);
+	free(config->item);
+}
+
+/* Used to track whether the key occurs more than once in a given file */
+#define KEY_SEEN_ONCE 1u
+#define KEY_SEEN_TWICE 2u
+#define COMMENT_KEY_SHIFT(id) (2 * (id))
+#define COMMENT_KEY_MASK(id) (3u << COMMENT_KEY_SHIFT(id))
+
+static void set_comment_key_flags(struct comment_char_config *config,
+				  const char *path, unsigned id, unsigned value)
+{
+	unsigned old = strintmap_get(&config->key_flags, path);
+	unsigned new = (old & ~COMMENT_KEY_MASK(id)) |
+				value << COMMENT_KEY_SHIFT(id);
+
+	strintmap_set(&config->key_flags, path, new);
+}
+
+static unsigned get_comment_key_flags(struct comment_char_config *config,
+				      const char *path, unsigned id)
+{
+	unsigned value = strintmap_get(&config->key_flags, path);
+
+	return (value & COMMENT_KEY_MASK(id)) >> COMMENT_KEY_SHIFT(id);
+}
 
 static const char* comment_key_name(unsigned id)
 {
@@ -1974,10 +2016,10 @@ static const char* comment_key_name(unsigned id)
 }
 
 static void comment_char_callback(const char *key, const char *value,
-				  const struct config_context *ctx UNUSED,
-				  void *data)
+				  const struct config_context *ctx, void *data)
 {
 	struct comment_char_config *config = data;
+	const struct key_value_info *kvi = ctx->kvi;
 	unsigned key_id;
 
 	if (!strcmp(key, "core.commentchar"))
@@ -1989,7 +2031,135 @@ static void comment_char_callback(const char *key, const char *value,
 
 	config->last_key_id = key_id;
 	config->auto_set = value && !strcmp(value, "auto");
-}
+	if (kvi->origin_type != CONFIG_ORIGIN_FILE) {
+		return;
+	} else if (get_comment_key_flags(config, kvi->filename, key_id)) {
+		set_comment_key_flags(config, kvi->filename, key_id,
+				      KEY_SEEN_TWICE);
+	} else {
+		struct comment_char_config_item *item;
+
+		ALLOC_GROW_BY(config->item, config->nr, 1, config->alloc);
+		item = &config->item[config->nr - 1];
+		item->key_id = key_id;
+		item->scope = kvi->scope;
+		item->path = xstrdup(kvi->filename);
+		set_comment_key_flags(config, kvi->filename, key_id,
+				      KEY_SEEN_ONCE);
+	}
+	config->auto_set_in_file = config->auto_set;
+}
+
+static void add_config_scope_arg(struct repository *repo, struct strbuf *buf,
+				 struct comment_char_config_item *item)
+{
+	char *global_config = git_global_config();
+	char *system_config = git_system_config();
+
+	if (item->scope == CONFIG_SCOPE_SYSTEM && access(item->path, W_OK)) {
+		/*
+		 * If the user cannot write to the system config recommend
+		 * setting the global config instead.
+		 */
+		strbuf_addstr(buf, "--global ");
+	} else if (fspatheq(item->path, system_config)) {
+		strbuf_addstr(buf, "--system ");
+	} else if (fspatheq(item->path, global_config)) {
+		strbuf_addstr(buf, "--global ");
+	} else if (fspatheq(item->path,
+			    mkpath("%s/config",
+				   repo_get_git_dir(repo)))) {
+		; /* --local is the default */
+	} else if (fspatheq(item->path,
+			    mkpath("%s/config.worktree",
+				   repo_get_common_dir(repo)))) {
+		strbuf_addstr(buf, "--worktree ");
+	} else {
+		const char *path = item->path;
+		const char *home = getenv("HOME");
+
+		strbuf_addstr(buf, "--file ");
+		if (home && !fspathncmp(path, home, strlen(home))) {
+			path += strlen(home);
+			if (!fspathncmp(path, "/", 1))
+				path++;
+			strbuf_addstr(buf, "~/");
+		}
+		sq_quote_buf_pretty(buf, path);
+		strbuf_addch(buf, ' ');
+	}
+
+	free(global_config);
+	free(system_config);
+}
+
+static bool can_unset_comment_char_config(struct comment_char_config *config)
+{
+	for (size_t i = 0; i < config->nr; i++) {
+		struct comment_char_config_item *item = &config->item[i];
+
+		if (item->scope == CONFIG_SCOPE_SYSTEM &&
+		    access(item->path, W_OK))
+			return false;
+	}
+
+	return true;
+}
+
+static void add_unset_auto_comment_char_advice(struct repository *repo,
+					       struct comment_char_config *config)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!can_unset_comment_char_config(config))
+		return;
+
+	for (size_t i = 0; i < config->nr; i++) {
+		struct comment_char_config_item *item = &config->item[i];
+
+		strbuf_addstr(&buf, "    git config unset ");
+		add_config_scope_arg(repo, &buf, item);
+		if (get_comment_key_flags(config, item->path, item->key_id) == KEY_SEEN_TWICE)
+			strbuf_addstr(&buf, "--all ");
+		strbuf_addf(&buf, "%s\n", comment_key_name(item->key_id));
+	}
+	advise(_("\nTo use the default comment string (#) please run\n\n%s"),
+	       buf.buf);
+	strbuf_release(&buf);
+}
+
+static void add_comment_char_advice(struct repository *repo,
+				    struct comment_char_config *config)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct comment_char_config_item *item;
+	/* TRANSLATORS this is a place holder for the value of core.commentString */
+	const char *placeholder = _("<comment string>");
+
+	/*
+	 * If auto is set in the last file that we saw advise the user how to
+	 * update their config.
+	 */
+	if (!config->auto_set_in_file)
+		return;
+
+	add_unset_auto_comment_char_advice(repo, config);
+	item = &config->item[config->nr - 1];
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, "    git config set ");
+	add_config_scope_arg(repo, &buf, item);
+	strbuf_addf(&buf, "%s %s\n", comment_key_name(item->key_id),
+		    placeholder);
+	advise(_("\nTo set a custom comment string please run\n\n"
+		 "%s\nwhere '%s' is the string you wish to use.\n"),
+	       buf.buf, placeholder);
+	strbuf_release(&buf);
+}
+
+#undef KEY_SEEN_ONCE
+#undef KEY_SEEN_TWICE
+#undef COMMENT_KEY_SHIFT
+#undef COMMENT_KEY_MASK
 
 struct repo_config {
 	struct repository *repo;
@@ -2001,18 +2171,26 @@ struct repo_config {
 		.repo = repo_,					\
 	};
 
+static void repo_config_release(struct repo_config *config)
+{
+	comment_char_config_release(&config->comment_char_config);
+}
+
 #ifdef WITH_BREAKING_CHANGES
-static void check_auto_comment_char_config(struct comment_char_config *config)
+static void check_auto_comment_char_config(struct repository *repo,
+					   struct comment_char_config *config)
 {
 	if (!config->auto_set)
 		return;
 
 	die_message(_("Support for '%s=auto' has been removed in Git 3.0"),
 		    comment_key_name(config->last_key_id));
+	add_comment_char_advice(repo, config);
 	die(NULL);
 }
 #else
-static void check_auto_comment_char_config(struct comment_char_config *config)
+static void check_auto_comment_char_config(struct repository *repo,
+					   struct comment_char_config *config)
 {
 	extern bool warn_on_auto_comment_char;
 	const char *DEPRECATED_CONFIG_ENV =
@@ -2032,6 +2210,7 @@ static void check_auto_comment_char_config(struct comment_char_config *config)
 
 	warning(_("Support for '%s=auto' is deprecated and will be removed in "
 		  "Git 3.0"), comment_key_name(config->last_key_id));
+	add_comment_char_advice(repo, config);
 }
 #endif /* WITH_BREAKING_CHANGES */
 
@@ -2040,7 +2219,8 @@ static void check_deprecated_config(struct repo_config *config)
 	if (!config->repo->check_deprecated_config)
 			return;
 
-	check_auto_comment_char_config(&config->comment_char_config);
+	check_auto_comment_char_config(config->repo,
+				       &config->comment_char_config);
 }
 
 static int repo_config_callback(const char *key, const char *value,
@@ -2083,6 +2263,7 @@ static void repo_read_config(struct repository *repo)
 		 */
 		die(_("unknown error occurred while reading the configuration files"));
 	check_deprecated_config(&config);
+	repo_config_release(&config);
 }
 
 static void git_config_check_init(struct repository *repo)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3b2a46c25ce..cc97628d810 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1186,9 +1186,19 @@ test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=
 		test_set_editor "$(pwd)/copy-edit-script.sh" &&
 		git rebase -i HEAD^ 2>err
 	) &&
-	sed -n "s/^warning: //p" err >actual &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^warning: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual &&
 	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index a9dc1e416d1..05f6da4ad98 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -958,10 +958,31 @@ test_expect_success 'commit --status with custom comment character' '
 
 test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
-	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend 2>err &&
-	sed -n "s/^warning: //p" err >actual &&
+	cat >config-include <<-\EOF &&
+	[core]
+	    commentString=:
+	    commentString=%
+	    commentChar=auto
+	EOF
+	test_when_finished "rm config-include" &&
+	test_config include.path "$(pwd)/config-include" &&
+	test_config core.commentChar ! &&
+	GIT_EDITOR=.git/FAKE_EDITOR git commit --amend 2>err &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^warning: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+	    git config unset --file ~/config-include --all core.commentString
+	    git config unset --file ~/config-include core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set --file ~/config-include core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
@@ -990,9 +1011,19 @@ EOF
 test_expect_success WITH_BREAKING_CHANGES 'core.commentChar=auto is rejected' '
 	test_config core.commentChar auto &&
 	test_must_fail git rev-parse --git-dir 2>err &&
-	sed -n "s/^fatal: //p" err >actual &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^fatal: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} has been removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual
 '
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH v2 3/3] commit: print advice when core.commentString=auto

[Oswald Buddenhagen]

On Thu, Jul 31, 2025 at 04:21:55PM +0100, Phillip Wood wrote:
>An alternative
>approach would be to advise the user to run "git config --show-origin"
>and leave them to figure out how to fix it themselves but that seems
>rather unfriendly. As we're forcing them to update their config we
>should try and make that as easy as possible.
>
your approach certainly helps the user to fix their acute problem 
quickly, but
- why should it? it's not like leaving it to the user would cause them a 
   huge burden, or that a noteworthy number of users are even going to be 
   affected. i don't think the fact that the update is forced justifies 
   making it a lot more user friendly than git configuration usually is, 
   esp. at this cost in complexity.
- i don't think i'd appreciate the tool lecturing me about trivial usage 
   patterns, when the real question in that situation is why the option 
   was set like that in the first place and whether/how the replacement 
   is actually equivalent or even superior.
- given that it doesn't print the entire decision tree (when 
   encountering read-only files), it doesn't necessarily guide the user 
   towards the best overall solution. that makes it _less_ user-friendly, 
   in a way.

Re: [PATCH v2 3/3] commit: print advice when core.commentString=auto

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> On Thu, Jul 31, 2025 at 04:21:55PM +0100, Phillip Wood wrote:
>>An alternative
>>approach would be to advise the user to run "git config --show-origin"
>>and leave them to figure out how to fix it themselves but that seems
>>rather unfriendly. As we're forcing them to update their config we
>>should try and make that as easy as possible.
>>
> your approach certainly helps the user to fix their acute problem
> quickly, but
> - why should it? it's not like leaving it to the user would cause them
>   a    huge burden, or that a noteworthy number of users are even
>   going to be    affected. i don't think the fact that the update is
>   forced justifies    making it a lot more user friendly than git
>   configuration usually is,    esp. at this cost in complexity.

I tend to agree that I prefer a simpler code that leaves a simple
exception handling to the users ;-)

> - given that it doesn't print the entire decision tree (when
>   encountering read-only files), it doesn't necessarily guide the user
>   towards the best overall solution. that makes it _less_
>   user-friendly,    in a way.

Even though we often do not like it, majority of users prefer to be
told what to do without having to think, so it is acceptable as long
as the suggestion does not take them in a direction that would hurt
them, even if it were not optimal.

Re: [PATCH v2 3/3] commit: print advice when core.commentString=auto

[Phillip Wood]

On 01/08/2025 16:18, Oswald Buddenhagen wrote:
> On Thu, Jul 31, 2025 at 04:21:55PM +0100, Phillip Wood wrote:
>> An alternative
>> approach would be to advise the user to run "git config --show-origin"
>> and leave them to figure out how to fix it themselves but that seems
>> rather unfriendly. As we're forcing them to update their config we
>> should try and make that as easy as possible.
>>
> your approach certainly helps the user to fix their acute problem 
> quickly, but
> - why should it? it's not like leaving it to the user would cause them a 
>    huge burden, or that a noteworthy number of users are even going to 
> be   affected. i don't think the fact that the update is forced 
> justifies   making it a lot more user friendly than git configuration 
> usually is,   esp. at this cost in complexity.

I think the fact that we're forcing the user to update does matter 
because it means they're having to update their config when they 
otherwise would not have to. I'd much rather it gave me a suggestion on 
how to proceed rather than told me to check my config and figure out 
what to do. There is certainly a complexity cost but I don't think it is 
that high. Some of git's reputation for being hard to use is well earned 
and I don't want to add to that.

> - i don't think i'd appreciate the tool lecturing me about trivial usage 
>    patterns, when the real question in that situation is why the option 
>    was set like that in the first place and whether/how the replacement 
>    is actually equivalent or even superior.

I don't think offering a suggestion is "lecturing about trivial usage 
patterns", I see it as offering assistance to users. The reason the 
advice offers two suggestions is because we cannot second guess whether 
the user wants to use the default or set a fixed string - it is up to 
them to decide.

> - given that it doesn't print the entire decision tree (when   
> encountering read-only files), it doesn't necessarily guide the user   
> towards the best overall solution. that makes it _less_ user-friendly,   
> in a way.

It provides a reasonable way of updating the config that we know will 
work when a user does not have write access to the system config. More 
experienced users are of course free to update their config as they see fit.

Thanks

Phillip

Re: [PATCH v2 3/3] commit: print advice when core.commentString=auto

[Oswald Buddenhagen]

On Tue, Aug 26, 2025 at 02:33:10PM +0100, Phillip Wood wrote:
>Some of git's reputation for being hard to use is well earned and I 
>don't want to add to that.
>
i would find that reasoning compelling if we weren't talking about a 
case that is likely to affect only very few, and probably rather 
advanced users (who somehow managed to make the feature actually useful 
for them - others would have quickly reverted the setting, because it 
would have gotten into their way).

Re: [PATCH v2 3/3] commit: print advice when core.commentString=auto

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> On Tue, Aug 26, 2025 at 02:33:10PM +0100, Phillip Wood wrote:
>> Some of git's reputation for being hard to use is well earned and I
>> don't want to add to that.
>>
> i would find that reasoning compelling if we weren't talking about a
> case that is likely to affect only very few, and probably rather
> advanced users (who somehow managed to make the feature actually
> useful for them - others would have quickly reverted the setting,
> because it would have gotten into their way).

So your counter-proposal is just stop at saying (possibly a milder
equivalent of) "nope, auto is no longer available. deal with it"?
Or does it go even stronger and not even special case "auto" that
user sets (i.e. and start commented lines with "auto " prefix)?

A simpler solution that would work for existing users is more
attractive than an overly complex one, of course, but I need to
gauge how simple you want to go.

Thanks.

Re: [PATCH v2 3/3] commit: print advice when core.commentString=auto

[Oswald Buddenhagen]

On Wed, Aug 27, 2025 at 09:39:25AM -0700, Junio C Hamano wrote:
>So your counter-proposal is just stop at saying (possibly a milder
>equivalent of) "nope, auto is no longer available. deal with it"?
>
yes.
though it should probably include "because it was found to have 
fundamentally flawed semantics. re-check the manual for alternatives." 
or something like that.

>Or does it go even stronger and not even special case "auto" that
>user sets (i.e. and start commented lines with "auto " prefix)?
>
no, silent failure would be counter-productive.

Re: [PATCH v2 0/3] breaking-changes: deprecate support for core.commentChar=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Changes since V1:
>  - Rebased onto a merge of 'ps/config-wo-the-repository' and 'master'

OK.  I needed the following merge-fix to make this merge work.

diff --git w/environment.c c/environment.c
index ae1427bb9e..a0ac5934b3 100644
--- w/environment.c
+++ c/environment.c
@@ -461,9 +461,11 @@ static int git_default_core_config(const char *var, const char *value,
 	    !strcmp(var, "core.commentstring")) {
 		if (!value)
 			return config_error_nonbool(var);
-		else if (!strcasecmp(value, "auto"))
+		else if (!strcasecmp(value, "auto")) {
 			auto_comment_line_char = 1;
-		else if (value[0]) {
+			FREE_AND_NULL(comment_line_str_to_free);
+			comment_line_str = "#";
+		} else if (value[0]) {
 			if (strchr(value, '\n'))
 				return error(_("%s cannot contain newline"), var);
 			comment_line_str = value;

I guess I used to carry an equivalent as a recurrent merge-fix for
your topic branch, but rolling it into the base of the series is
certainly safer (i.e. we have to do a merge and resolve conflicts
just once, and after that we won't even touch it---as opposed to
keep recreating the same conflict and resolving every time we merge
your topic via rerere & merge-fix mechanism).

Thanks.

Re: [PATCH v2 0/3] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

On 01/08/2025 04:50, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> Changes since V1:
>>   - Rebased onto a merge of 'ps/config-wo-the-repository' and 'master'
> 
> OK.  I needed the following merge-fix to make this merge work.
> 
> diff --git w/environment.c c/environment.c
> index ae1427bb9e..a0ac5934b3 100644
> --- w/environment.c
> +++ c/environment.c
> @@ -461,9 +461,11 @@ static int git_default_core_config(const char *var, const char *value,
>   	    !strcmp(var, "core.commentstring")) {
>   		if (!value)
>   			return config_error_nonbool(var);
> -		else if (!strcasecmp(value, "auto"))
> +		else if (!strcasecmp(value, "auto")) {
>   			auto_comment_line_char = 1;
> -		else if (value[0]) {
> +			FREE_AND_NULL(comment_line_str_to_free);
> +			comment_line_str = "#";
> +		} else if (value[0]) {
>   			if (strchr(value, '\n'))
>   				return error(_("%s cannot contain newline"), var);
>   			comment_line_str = value;
> 
> I guess I used to carry an equivalent as a recurrent merge-fix for
> your topic branch, but rolling it into the base of the series is
> certainly safer (i.e. we have to do a merge and resolve conflicts
> just once, and after that we won't even touch it---as opposed to
> keep recreating the same conflict and resolving every time we merge
> your topic via rerere & merge-fix mechanism).

I was hoping that rebasing on master would eliminate the need for a fix 
as 'ac/auto-comment-char-fix' is now in master but in the meantime 
'ps/config-wo-the-repository' came along and moved code from config.c to 
environment.c without those changes. I'd assumed you already had a 
similar fixup when merging 'ps/config-wo-the-repository' into seen.

Thanks

Phillip

Re: [PATCH v2 0/3] breaking-changes: deprecate support for core.commentChar=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> I guess I used to carry an equivalent as a recurrent merge-fix for
>> your topic branch, but rolling it into the base of the series is
>> certainly safer (i.e. we have to do a merge and resolve conflicts
>> just once, and after that we won't even touch it---as opposed to
>> keep recreating the same conflict and resolving every time we merge
>> your topic via rerere & merge-fix mechanism).
>
> I was hoping that rebasing on master would eliminate the need for a
> fix as 'ac/auto-comment-char-fix' is now in master but in the meantime
> 'ps/config-wo-the-repository' came along and moved code from config.c
> to environment.c without those changes. I'd assumed you already had a
> similar fixup when merging 'ps/config-wo-the-repository' into seen.

Exactly.  That is what I just said, in case it was unclear.

[PATCH v3 0/3] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Thanks to Junio and Oswald for their comments on V2.
This series implements the plan to deprecate and remove support for
core.commentChar=auto outlined in [1]. This feature has been the
source of a couple of bug reports recently [2,3] and it is hard to
see how the design can be fixed as it is incompatible with preparing
a commit message template containing comments. When git sees the
deprecated config setting it will print advice based on the user's
config setting to help the user either remove the setting or set a
custom comment string. In the example below core.commentString is set
multiple times in $XDG_CONFIG_HOME/git/config and core.commentChar
is set in ~/.gitconfig and $XDG_CONFIG_HOME/git/config.

warning: Support for 'core.commentChar=auto' is deprecated and will be removed in Git 3.0
hint:
hint: To use the default comment string (#) please run
hint:
hint:     git config unset --file ~/.config/git/config --all core.commentString
hint:     git config unset --file ~/.config/git/config core.commentChar
hint:     git config unset --global core.commentChar
hint:
hint: To set a custom comment string please run
hint:
hint:     git config set --global core.commentChar <comment string>
hint:
hint: where '<comment string>' is the string you wish to use.

[1] https://lore.kernel.org/git/6a3154e0-e7bc-45ae-b554-67ccab18727a@gmail.com
[2] https://lore.kernel.org/git/20250315140913.577404-1-oswald.buddenhagen@gmx.de
[3] https://lore.kernel.org/git/20250626132233.414789-1-ayu.chandekar@gmail.com

Changes since V2:
 - Patch 1: Punctuation fixes
 - Patch 2: Reworded the commit message slightly
            Remove unnecessary include of advice.h
            Fix variable declaration
 - Patch 3: Include advice.h

Changes since V1:
 - Rebased onto a merge of 'ps/config-wo-the-repository' and 'master'
 - Reworded commit messages
 - What was patch 2 has been split into two separate patches and
   reworked to die when core.commentChar=auto and WITH_BREAKING_CHANGES
   is enabled.

Base-Commit: 1ae5bd276bdf101e37c1a8f2904a2eae05fbb744
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fremove-auto-comment-char%2Fv3
View-Changes-At: https://github.com/phillipwood/git/compare/1ae5bd276...ee6cf11a8
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/remove-auto-comment-char/v3


Phillip Wood (3):
  breaking-changes: deprecate support for core.commentString=auto
  config: warn on core.commentString=auto
  commit: print advice when core.commentString=auto

 Documentation/BreakingChanges.adoc |   5 +
 Documentation/config/core.adoc     |  20 +-
 builtin/commit.c                   |   7 +
 builtin/merge.c                    |   3 +
 builtin/rebase.c                   |   3 +
 builtin/revert.c                   |   7 +
 config.c                           | 297 ++++++++++++++++++++++++++++-
 environment.c                      |  11 +-
 environment.h                      |   3 +
 repository.c                       |   1 +
 repository.h                       |   3 +
 t/t3404-rebase-interactive.sh      |  19 +-
 t/t3418-rebase-continue.sh         |   2 +-
 t/t7502-commit-porcelain.sh        |  52 ++++-
 14 files changed, 421 insertions(+), 12 deletions(-)

Range-diff against v2:
1:  a6355451d4b ! 1:  5b921064f1e breaking-changes: deprecate support for core.commentString=auto
    @@ Commit message
         automatically select the comment character ensuring that it is not the
         first character on any of the lines in the commit message. This was
         introduced by commit 84c9dc2c5a2 (commit: allow core.commentChar=auto
    -    for character auto selection, 2014-05-17) The motivation seems to be
    +    for character auto selection, 2014-05-17). The motivation seems to be
         to avoid commenting out lines from the existing message when amending
         a commit that was created with a message from a file.
     
    @@ Commit message
         consequence of the design of this feature and are therefore hard to
         fix.
     
    -    As the costs of this feature outweigh the benefits deprecate it and
    +    As the costs of this feature outweigh the benefits, deprecate it and
         remove it in Git 3.0. If someone comes up with some patches that fix
         all the issues in a maintainable way then I'd be happy to see this
         change reverted.
2:  8b575980426 ! 2:  5dd897c95e6 config: warn on core.commentString=auto
    @@ Commit message
     
         As support for this setting was deprecated in the last commit print a
         warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
    -    When printing a warning avoid bombarding the user by only printing it
    -    when running commands commands that run "git commit" and only only
    -    once per command. Some scaffolding is added to repo_read_config()
    -    to allow it to detect deprecated config settings and warn about
    -    them. As both "core.commentChar" and "core.commentString" set the
    -    comment character we record which one of them is used and tailor the
    -    warning message appropriately.
    +    Avoid bombarding the user with warnings by only printing it (a) when
    +    running commands commands that call "git commit" and (b) only once
    +    per command. Some scaffolding is added to repo_read_config() to allow
    +    it to detect deprecated config settings and warn about them. As both
    +    "core.commentChar" and "core.commentString" set the comment character
    +    we record which one of them is used and tailor the warning message
    +    appropriately.
     
         Note the odd combination of die_message() followed by die(NULL)
         is to allow the next commit to insert a call to advise() in the middle.
    @@ builtin/revert.c: struct repository *repo UNUSED)
     
      ## config.c ##
     @@
    - 
    - #include "git-compat-util.h"
    - #include "abspath.h"
    -+#include "advice.h"
      #include "date.h"
      #include "branch.h"
      #include "config.h"
    @@ config.c: int git_configset_get_pathname(struct config_set *set, const char *key
     +
     +#define COMMENT_CHAR_CFG_INIT { 0 }
     +
    -+static const char* comment_key_name(unsigned id)
    ++static const char *comment_key_name(unsigned id)
     +{
     +	static const char *name[] = {
     +		"core.commentChar",
3:  0e7c08b15e5 ! 3:  ee6cf11a82c commit: print advice when core.commentString=auto
    @@ Commit message
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## config.c ##
    +@@
    + 
    + #include "git-compat-util.h"
    + #include "abspath.h"
    ++#include "advice.h"
    + #include "date.h"
    + #include "branch.h"
    + #include "config.h"
     @@ config.c: int git_configset_get_pathname(struct config_set *set, const char *key, char **d
      struct comment_char_config {
      	unsigned last_key_id;
    @@ config.c: int git_configset_get_pathname(struct config_set *set, const char *key
     +	return (value & COMMENT_KEY_MASK(id)) >> COMMENT_KEY_SHIFT(id);
     +}
      
    - static const char* comment_key_name(unsigned id)
    + static const char *comment_key_name(unsigned id)
      {
    -@@ config.c: static const char* comment_key_name(unsigned id)
    +@@ config.c: static const char *comment_key_name(unsigned id)
      }
      
      static void comment_char_callback(const char *key, const char *value,
-- 
2.49.0.897.gfad3eb7d210

[PATCH v3 1/3] breaking-changes: deprecate support for core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When "core.commentString" is set to "auto" then "git commit" will
automatically select the comment character ensuring that it is not the
first character on any of the lines in the commit message. This was
introduced by commit 84c9dc2c5a2 (commit: allow core.commentChar=auto
for character auto selection, 2014-05-17). The motivation seems to be
to avoid commenting out lines from the existing message when amending
a commit that was created with a message from a file.

Unfortunately this feature does not work with:

 * commit message templates that contain comments.

 * prepare-commit-msg hooks that introduce comments.

 * "git commit --cleanup=strip --edit -F <file>" which means that it
   is incompatible with

   - the "fixup" and "squash" commands of "git rebase -i" as the
     comments added by those commands are then treated as part of
     the commit message.

   - the conflict comments added to the commit message by "git
     cherry-pick", "git rebase" etc. as these comments are then
     treated as part of the commit message.

It is also ignored by "git notes" when amending a note.

The issues with comments coming from a template, hook or file are a
consequence of the design of this feature and are therefore hard to
fix.

As the costs of this feature outweigh the benefits, deprecate it and
remove it in Git 3.0. If someone comes up with some patches that fix
all the issues in a maintainable way then I'd be happy to see this
change reverted.

The next commits will add a warning and some advice for users on how
they can update their config settings.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/BreakingChanges.adoc |  5 +++++
 Documentation/config/core.adoc     | 20 ++++++++++++++++++--
 builtin/commit.c                   |  4 ++++
 environment.c                      | 10 ++++++++--
 environment.h                      |  2 ++
 t/t3404-rebase-interactive.sh      |  2 +-
 t/t3418-rebase-continue.sh         |  2 +-
 t/t7502-commit-porcelain.sh        |  4 ++--
 8 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index f8d2eba061c..344ce500603 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -239,6 +239,11 @@ These features will be removed.
 +
 The command will be removed.
 
+* Support for `core.commentString=auto` has been deprecated and will
+  be removed in Git 3.0.
++
+cf. <xmqqa59i45wc.fsf@gitster.g>
+
 == Superseded features that will not be deprecated
 
 Some features have gained newer replacements that aim to improve the design in
diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
index 9fde1ab63a7..7133f00c38b 100644
--- a/Documentation/config/core.adoc
+++ b/Documentation/config/core.adoc
@@ -531,9 +531,25 @@ core.commentString::
 	commented, and removes them after the editor returns
 	(default '#').
 +
-If set to "auto", `git-commit` would select a character that is not
+ifndef::with-breaking-changes[]
+If set to "auto", `git-commit` will select a character that is not
 the beginning character of any line in existing commit messages.
-+
+Support for this value is deprecated and will be removed in Git 3.0
+due to the following limitations:
++
+--
+* It is incompatible with adding comments in a commit message
+  template. This includes the conflicts comments added to
+  the commit message by `cherry-pick`, `merge`, `rebase` and
+  `revert`.
+* It is incompatible with adding comments to the commit message
+  in the `prepare-commit-msg` hook.
+* It is incompatible with the `fixup` and `squash` commands when
+  rebasing,
+* It is not respected by `git notes`
+--
++
+endif::with-breaking-changes[]
 Note that these two variables are aliases of each other, and in modern
 versions of Git you are free to use a string (e.g., `//` or `⁑⁕⁑`) with
 `commentChar`. Versions of Git prior to v2.45.0 will ignore
diff --git a/builtin/commit.c b/builtin/commit.c
index 757f51eac82..d25cc07a355 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -683,6 +683,7 @@ static int author_date_is_interesting(void)
 	return author_message || force_date;
 }
 
+#ifndef WITH_BREAKING_CHANGES
 static void adjust_comment_line_char(const struct strbuf *sb)
 {
 	char candidates[] = "#;@!$%^&|:";
@@ -720,6 +721,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	free(comment_line_str_to_free);
 	comment_line_str = comment_line_str_to_free = xstrfmt("%c", *p);
 }
+#endif /* !WITH_BREAKING_CHANGES */
 
 static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
 				struct pretty_print_context *ctx)
@@ -916,8 +918,10 @@ 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"));
 
+#ifndef WITH_BREAKING_CHANGES
 	if (auto_comment_line_char)
 		adjust_comment_line_char(&sb);
+#endif /* !WITH_BREAKING_CHANGES */
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
diff --git a/environment.c b/environment.c
index a0ac5934b37..4c87876d483 100644
--- a/environment.c
+++ b/environment.c
@@ -122,7 +122,9 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT;
  */
 const char *comment_line_str = "#";
 char *comment_line_str_to_free;
+#ifndef WITH_BREAKING_CHANGES
 int auto_comment_line_char;
+#endif /* !WITH_BREAKING_CHANGES */
 
 /* This is set by setup_git_directory_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
@@ -459,18 +461,22 @@ static int git_default_core_config(const char *var, const char *value,
 
 	if (!strcmp(var, "core.commentchar") ||
 	    !strcmp(var, "core.commentstring")) {
-		if (!value)
+		if (!value) {
 			return config_error_nonbool(var);
-		else if (!strcasecmp(value, "auto")) {
+#ifndef WITH_BREAKING_CHANGES
+		} else if (!strcasecmp(value, "auto")) {
 			auto_comment_line_char = 1;
 			FREE_AND_NULL(comment_line_str_to_free);
 			comment_line_str = "#";
+#endif /* !WITH_BREAKING_CHANGES */
 		} else if (value[0]) {
 			if (strchr(value, '\n'))
 				return error(_("%s cannot contain newline"), var);
 			comment_line_str = value;
 			FREE_AND_NULL(comment_line_str_to_free);
+#ifndef WITH_BREAKING_CHANGES
 			auto_comment_line_char = 0;
+#endif /* !WITH_BREAKING_CHANGES */
 		} else
 			return error(_("%s must have at least one character"), var);
 		return 0;
diff --git a/environment.h b/environment.h
index 8cfce41015b..e75c4abb388 100644
--- a/environment.h
+++ b/environment.h
@@ -208,7 +208,9 @@ extern char *excludes_file;
  */
 extern const char *comment_line_str;
 extern char *comment_line_str_to_free;
+#ifndef WITH_BREAKING_CHANGES
 extern int auto_comment_line_char;
+#endif /* !WITH_BREAKING_CHANGES */
 
 # endif /* USE_THE_REPOSITORY_VARIABLE */
 #endif /* ENVIRONMENT_H */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6bac217ed35..ce0aebb9a7e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_success 'rebase -i respects core.commentchar=auto' '
+test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=auto' '
 	test_config core.commentchar auto &&
 	write_script copy-edit-script.sh <<-\EOF &&
 	cp "$1" edit-script
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index b8a8dd77e74..f9b8999db50 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -328,7 +328,7 @@ test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas
 	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
 '
 
-test_expect_success 'no change in comment character due to conflicts markers with core.commentChar=auto' '
+test_expect_success !WITH_BREAKING_CHANGES 'no change in comment character due to conflicts markers with core.commentChar=auto' '
 	git checkout -b branch-a &&
 	test_commit A F1 &&
 	git checkout -b branch-b HEAD^ &&
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b37e2018a74..65b4519a715 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -956,13 +956,13 @@ test_expect_success 'commit --status with custom comment character' '
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'switch core.commentchar' '
+test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
 	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'switch core.commentchar but out of options' '
+test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar but out of options' '
 	cat >text <<\EOF &&
 # 1
 ; 2
-- 
2.49.0.897.gfad3eb7d210

[PATCH v3 2/3] config: warn on core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As support for this setting was deprecated in the last commit print a
warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
Avoid bombarding the user with warnings by only printing it (a) when
running commands commands that call "git commit" and (b) only once
per command. Some scaffolding is added to repo_read_config() to allow
it to detect deprecated config settings and warn about them. As both
"core.commentChar" and "core.commentString" set the comment character
we record which one of them is used and tailor the warning message
appropriately.

Note the odd combination of die_message() followed by die(NULL)
is to allow the next commit to insert a call to advise() in the middle.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c              |   3 +
 builtin/merge.c               |   3 +
 builtin/rebase.c              |   3 +
 builtin/revert.c              |   7 +++
 config.c                      | 115 +++++++++++++++++++++++++++++++++-
 environment.c                 |   1 +
 environment.h                 |   1 +
 repository.c                  |   1 +
 repository.h                  |   3 +
 t/t3404-rebase-interactive.sh |   7 ++-
 t/t7502-commit-porcelain.sh   |  17 ++++-
 11 files changed, 157 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d25cc07a355..f821fdcfcc3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1783,6 +1783,9 @@ int cmd_commit(int argc,
 	show_usage_with_options_if_asked(argc, argv,
 					 builtin_commit_usage, builtin_commit_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/merge.c b/builtin/merge.c
index dc4cb8fb14d..794cb7bb269 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1378,6 +1378,9 @@ int cmd_merge(int argc,
 	show_usage_with_options_if_asked(argc, argv,
 					 builtin_merge_usage, builtin_merge_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 72a52bdfb98..962917ec480 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1242,6 +1242,9 @@ int cmd_rebase(int argc,
 					 builtin_rebase_usage,
 					 builtin_rebase_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/revert.c b/builtin/revert.c
index e07c2217fe8..b197848bb0a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -4,6 +4,7 @@
 #include "builtin.h"
 #include "parse-options.h"
 #include "diff.h"
+#include "environment.h"
 #include "gettext.h"
 #include "revision.h"
 #include "rerere.h"
@@ -285,6 +286,9 @@ int cmd_revert(int argc,
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int res;
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	opts.action = REPLAY_REVERT;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, prefix, &opts);
@@ -302,6 +306,9 @@ struct repository *repo UNUSED)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int res;
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, prefix, &opts);
diff --git a/config.c b/config.c
index 97ffef42700..18b42197095 100644
--- a/config.c
+++ b/config.c
@@ -11,6 +11,7 @@
 #include "date.h"
 #include "branch.h"
 #include "config.h"
+#include "dir.h"
 #include "parse.h"
 #include "convert.h"
 #include "environment.h"
@@ -1951,10 +1952,110 @@ int git_configset_get_pathname(struct config_set *set, const char *key, char **d
 		return 1;
 }
 
+struct comment_char_config {
+	unsigned last_key_id;
+	bool auto_set;
+};
+
+#define COMMENT_CHAR_CFG_INIT { 0 }
+
+static const char *comment_key_name(unsigned id)
+{
+	static const char *name[] = {
+		"core.commentChar",
+		"core.commentString",
+	};
+
+	if (id >= ARRAY_SIZE(name))
+		BUG("invalid comment key id");
+
+	return name[id];
+}
+
+static void comment_char_callback(const char *key, const char *value,
+				  const struct config_context *ctx UNUSED,
+				  void *data)
+{
+	struct comment_char_config *config = data;
+	unsigned key_id;
+
+	if (!strcmp(key, "core.commentchar"))
+		key_id = 0;
+	else if (!strcmp(key, "core.commentstring"))
+		key_id = 1;
+	else
+		return;
+
+	config->last_key_id = key_id;
+	config->auto_set = value && !strcmp(value, "auto");
+}
+
+struct repo_config {
+	struct repository *repo;
+	struct comment_char_config comment_char_config;
+};
+
+#define REPO_CONFIG_INIT(repo_) {				\
+		.comment_char_config = COMMENT_CHAR_CFG_INIT,	\
+		.repo = repo_,					\
+	};
+
+#ifdef WITH_BREAKING_CHANGES
+static void check_auto_comment_char_config(struct comment_char_config *config)
+{
+	if (!config->auto_set)
+		return;
+
+	die_message(_("Support for '%s=auto' has been removed in Git 3.0"),
+		    comment_key_name(config->last_key_id));
+	die(NULL);
+}
+#else
+static void check_auto_comment_char_config(struct comment_char_config *config)
+{
+	extern bool warn_on_auto_comment_char;
+	const char *DEPRECATED_CONFIG_ENV =
+				"GIT_AUTO_COMMENT_CHAR_CONFIG_WARNING_GIVEN";
+
+	if (!config->auto_set || !warn_on_auto_comment_char)
+		return;
+
+	/*
+	 * Use an environment variable to ensure that subprocesses do not repeat
+	 * the warning.
+	 */
+	if (git_env_bool(DEPRECATED_CONFIG_ENV, false))
+		return;
+
+	setenv(DEPRECATED_CONFIG_ENV, "true", true);
+
+	warning(_("Support for '%s=auto' is deprecated and will be removed in "
+		  "Git 3.0"), comment_key_name(config->last_key_id));
+}
+#endif /* WITH_BREAKING_CHANGES */
+
+static void check_deprecated_config(struct repo_config *config)
+{
+	if (!config->repo->check_deprecated_config)
+			return;
+
+	check_auto_comment_char_config(&config->comment_char_config);
+}
+
+static int repo_config_callback(const char *key, const char *value,
+				const struct config_context *ctx, void *data)
+{
+	struct repo_config *config = data;
+
+	comment_char_callback(key, value, ctx, &config->comment_char_config);
+	return config_set_callback(key, value, ctx, config->repo->config);
+}
+
 /* Functions use to read configuration from a repository */
 static void repo_read_config(struct repository *repo)
 {
 	struct config_options opts = { 0 };
+	struct repo_config config = REPO_CONFIG_INIT(repo);
 
 	opts.respect_includes = 1;
 	opts.commondir = repo->commondir;
@@ -1966,8 +2067,8 @@ static void repo_read_config(struct repository *repo)
 		git_configset_clear(repo->config);
 
 	git_configset_init(repo->config);
-	if (config_with_options(config_set_callback, repo->config, NULL,
-				repo, &opts) < 0)
+	if (config_with_options(repo_config_callback, &config, NULL, repo,
+				&opts) < 0)
 		/*
 		 * config_with_options() normally returns only
 		 * zero, as most errors are fatal, and
@@ -1980,6 +2081,7 @@ static void repo_read_config(struct repository *repo)
 		 * immediately.
 		 */
 		die(_("unknown error occurred while reading the configuration files"));
+	check_deprecated_config(&config);
 }
 
 static void git_config_check_init(struct repository *repo)
@@ -2667,6 +2769,14 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	char *contents = NULL;
 	size_t contents_sz;
 	struct config_store_data store = CONFIG_STORE_INIT;
+	bool saved_check_deprecated_config = r->check_deprecated_config;
+
+	/*
+	 * Do not warn or die if there are deprecated config settings as
+	 * we want the user to be able to change those settings by running
+	 * "git config".
+	 */
+	r->check_deprecated_config = false;
 
 	validate_comment_string(comment);
 
@@ -2898,6 +3008,7 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	if (in_fd >= 0)
 		close(in_fd);
 	config_store_data_clear(&store);
+	r->check_deprecated_config = saved_check_deprecated_config;
 	return ret;
 
 write_err_out:
diff --git a/environment.c b/environment.c
index 4c87876d483..1ffa2ff30b2 100644
--- a/environment.c
+++ b/environment.c
@@ -124,6 +124,7 @@ const char *comment_line_str = "#";
 char *comment_line_str_to_free;
 #ifndef WITH_BREAKING_CHANGES
 int auto_comment_line_char;
+bool warn_on_auto_comment_char;
 #endif /* !WITH_BREAKING_CHANGES */
 
 /* This is set by setup_git_directory_gently() and/or git_default_config() */
diff --git a/environment.h b/environment.h
index e75c4abb388..51898c99cd1 100644
--- a/environment.h
+++ b/environment.h
@@ -210,6 +210,7 @@ extern const char *comment_line_str;
 extern char *comment_line_str_to_free;
 #ifndef WITH_BREAKING_CHANGES
 extern int auto_comment_line_char;
+extern bool warn_on_auto_comment_char;
 #endif /* !WITH_BREAKING_CHANGES */
 
 # endif /* USE_THE_REPOSITORY_VARIABLE */
diff --git a/repository.c b/repository.c
index ecd691181fc..8af73923d34 100644
--- a/repository.c
+++ b/repository.c
@@ -57,6 +57,7 @@ void initialize_repository(struct repository *repo)
 	repo->parsed_objects = parsed_object_pool_new(repo);
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
+	repo->check_deprecated_config = true;
 
 	/*
 	 * When a command runs inside a repository, it learns what
diff --git a/repository.h b/repository.h
index 042dc93f0f2..5808a5d6108 100644
--- a/repository.h
+++ b/repository.h
@@ -161,6 +161,9 @@ struct repository {
 
 	/* Indicate if a repository has a different 'commondir' from 'gitdir' */
 	unsigned different_commondir:1;
+
+	/* Should repo_config() check for deprecated settings */
+	bool check_deprecated_config;
 };
 
 #ifdef USE_THE_REPOSITORY_VARIABLE
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ce0aebb9a7e..3b2a46c25ce 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1184,8 +1184,13 @@ test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=
 	test_when_finished "git rebase --abort || :" &&
 	(
 		test_set_editor "$(pwd)/copy-edit-script.sh" &&
-		git rebase -i HEAD^
+		git rebase -i HEAD^ 2>err
 	) &&
+	sed -n "s/^warning: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+	EOF
+	test_cmp expect actual &&
 	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
 '
 
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 65b4519a715..a9dc1e416d1 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -958,7 +958,12 @@ test_expect_success 'commit --status with custom comment character' '
 
 test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
-	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
+	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend 2>err &&
+	sed -n "s/^warning: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+	EOF
+	test_cmp expect actual &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
@@ -982,4 +987,14 @@ EOF
 	)
 '
 
+test_expect_success WITH_BREAKING_CHANGES 'core.commentChar=auto is rejected' '
+	test_config core.commentChar auto &&
+	test_must_fail git rev-parse --git-dir 2>err &&
+	sed -n "s/^fatal: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} has been removed in Git 3.0
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.49.0.897.gfad3eb7d210

Re: [PATCH v3 2/3] config: warn on core.commentString=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> As support for this setting was deprecated in the last commit print a
> warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
> Avoid bombarding the user with warnings by only printing it (a) when
> running commands commands that call "git commit" and (b) only once

"commands commands" -> "commands".

> per command.

Re: [PATCH v3 2/3] config: warn on core.commentString=auto

[Phillip Wood]

On 26/08/2025 16:52, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> As support for this setting was deprecated in the last commit print a
>> warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
>> Avoid bombarding the user with warnings by only printing it (a) when
>> running commands commands that call "git commit" and (b) only once
> 
> "commands commands" -> "commands".

Sigh, I removed "only only" which Oswald had pointed out only to add 
another repeated word. I've just sent V4 with the typo fixed.

Thanks

Phillip

Re: [PATCH v3 2/3] config: warn on core.commentString=auto

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 26/08/2025 16:52, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> As support for this setting was deprecated in the last commit print a
>>> warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
>>> Avoid bombarding the user with warnings by only printing it (a) when
>>> running commands commands that call "git commit" and (b) only once
>> "commands commands" -> "commands".
>
> Sigh, I removed "only only" which Oswald had pointed out only to add
> another repeated word. I've just sent V4 with the typo fixed.

Heh, I've locally amended so no need to resend.  I'd rather see the
"what degree of help would our users need, and is it worth trying to
find the (impossible) definition of being 'good enough for most
people'" resolved soon so that we can move forward.  I'll read the
series again, especially the recovery recipe the topic would give
our users, to see if my stance would change from my previous one.

Thanks.

[PATCH v3 3/3] commit: print advice when core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add some advice on how to change the config settings when
"core.commentString=auto" or "core.commentChar=auto". The advice
includes instructions for clearing the config setting or setting a
fixed comment string. To try and be as specific as possible, the advice
is customized based on the user's config. If "core.commentString=auto"
is set in the system config and the user does not have write
access then the advice omits the instructions to clear the config
and recommends changing the global config instead. An alternative
approach would be to advise the user to run "git config --show-origin"
and leave them to figure out how to fix it themselves but that seems
rather unfriendly. As we're forcing them to update their config we
should try and make that as easy as possible.

In order to generate this advice we need to record each file where
either of the config keys is set and whether a key occurs more that
once in a given file. This lets us generate the list of commands to
remove all the keys and also tells us which key the "auto" setting
comes from.

As we want the user to update their config we do not provide a way
for this advice to be disabled other than changing the value of
"core.commentChar" or "core.commentString".

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 config.c                      | 196 ++++++++++++++++++++++++++++++++--
 t/t3404-rebase-interactive.sh |  12 ++-
 t/t7502-commit-porcelain.sh   |  37 ++++++-
 3 files changed, 234 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 18b42197095..18dcf341d58 100644
--- a/config.c
+++ b/config.c
@@ -8,6 +8,7 @@
 
 #include "git-compat-util.h"
 #include "abspath.h"
+#include "advice.h"
 #include "date.h"
 #include "branch.h"
 #include "config.h"
@@ -1955,9 +1956,51 @@ int git_configset_get_pathname(struct config_set *set, const char *key, char **d
 struct comment_char_config {
 	unsigned last_key_id;
 	bool auto_set;
+	bool auto_set_in_file;
+	struct strintmap key_flags;
+	size_t alloc, nr;
+	struct comment_char_config_item {
+		unsigned key_id;
+		char *path;
+		enum config_scope scope;
+	} *item;
 };
 
-#define COMMENT_CHAR_CFG_INIT { 0 }
+#define COMMENT_CHAR_CFG_INIT {			\
+		.key_flags = STRINTMAP_INIT,	\
+	}
+
+static void comment_char_config_release(struct comment_char_config *config)
+{
+	strintmap_clear(&config->key_flags);
+	for (size_t i = 0; i < config->nr; i++)
+		free(config->item[i].path);
+	free(config->item);
+}
+
+/* Used to track whether the key occurs more than once in a given file */
+#define KEY_SEEN_ONCE 1u
+#define KEY_SEEN_TWICE 2u
+#define COMMENT_KEY_SHIFT(id) (2 * (id))
+#define COMMENT_KEY_MASK(id) (3u << COMMENT_KEY_SHIFT(id))
+
+static void set_comment_key_flags(struct comment_char_config *config,
+				  const char *path, unsigned id, unsigned value)
+{
+	unsigned old = strintmap_get(&config->key_flags, path);
+	unsigned new = (old & ~COMMENT_KEY_MASK(id)) |
+				value << COMMENT_KEY_SHIFT(id);
+
+	strintmap_set(&config->key_flags, path, new);
+}
+
+static unsigned get_comment_key_flags(struct comment_char_config *config,
+				      const char *path, unsigned id)
+{
+	unsigned value = strintmap_get(&config->key_flags, path);
+
+	return (value & COMMENT_KEY_MASK(id)) >> COMMENT_KEY_SHIFT(id);
+}
 
 static const char *comment_key_name(unsigned id)
 {
@@ -1973,10 +2016,10 @@ static const char *comment_key_name(unsigned id)
 }
 
 static void comment_char_callback(const char *key, const char *value,
-				  const struct config_context *ctx UNUSED,
-				  void *data)
+				  const struct config_context *ctx, void *data)
 {
 	struct comment_char_config *config = data;
+	const struct key_value_info *kvi = ctx->kvi;
 	unsigned key_id;
 
 	if (!strcmp(key, "core.commentchar"))
@@ -1988,7 +2031,135 @@ static void comment_char_callback(const char *key, const char *value,
 
 	config->last_key_id = key_id;
 	config->auto_set = value && !strcmp(value, "auto");
-}
+	if (kvi->origin_type != CONFIG_ORIGIN_FILE) {
+		return;
+	} else if (get_comment_key_flags(config, kvi->filename, key_id)) {
+		set_comment_key_flags(config, kvi->filename, key_id,
+				      KEY_SEEN_TWICE);
+	} else {
+		struct comment_char_config_item *item;
+
+		ALLOC_GROW_BY(config->item, config->nr, 1, config->alloc);
+		item = &config->item[config->nr - 1];
+		item->key_id = key_id;
+		item->scope = kvi->scope;
+		item->path = xstrdup(kvi->filename);
+		set_comment_key_flags(config, kvi->filename, key_id,
+				      KEY_SEEN_ONCE);
+	}
+	config->auto_set_in_file = config->auto_set;
+}
+
+static void add_config_scope_arg(struct repository *repo, struct strbuf *buf,
+				 struct comment_char_config_item *item)
+{
+	char *global_config = git_global_config();
+	char *system_config = git_system_config();
+
+	if (item->scope == CONFIG_SCOPE_SYSTEM && access(item->path, W_OK)) {
+		/*
+		 * If the user cannot write to the system config recommend
+		 * setting the global config instead.
+		 */
+		strbuf_addstr(buf, "--global ");
+	} else if (fspatheq(item->path, system_config)) {
+		strbuf_addstr(buf, "--system ");
+	} else if (fspatheq(item->path, global_config)) {
+		strbuf_addstr(buf, "--global ");
+	} else if (fspatheq(item->path,
+			    mkpath("%s/config",
+				   repo_get_git_dir(repo)))) {
+		; /* --local is the default */
+	} else if (fspatheq(item->path,
+			    mkpath("%s/config.worktree",
+				   repo_get_common_dir(repo)))) {
+		strbuf_addstr(buf, "--worktree ");
+	} else {
+		const char *path = item->path;
+		const char *home = getenv("HOME");
+
+		strbuf_addstr(buf, "--file ");
+		if (home && !fspathncmp(path, home, strlen(home))) {
+			path += strlen(home);
+			if (!fspathncmp(path, "/", 1))
+				path++;
+			strbuf_addstr(buf, "~/");
+		}
+		sq_quote_buf_pretty(buf, path);
+		strbuf_addch(buf, ' ');
+	}
+
+	free(global_config);
+	free(system_config);
+}
+
+static bool can_unset_comment_char_config(struct comment_char_config *config)
+{
+	for (size_t i = 0; i < config->nr; i++) {
+		struct comment_char_config_item *item = &config->item[i];
+
+		if (item->scope == CONFIG_SCOPE_SYSTEM &&
+		    access(item->path, W_OK))
+			return false;
+	}
+
+	return true;
+}
+
+static void add_unset_auto_comment_char_advice(struct repository *repo,
+					       struct comment_char_config *config)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!can_unset_comment_char_config(config))
+		return;
+
+	for (size_t i = 0; i < config->nr; i++) {
+		struct comment_char_config_item *item = &config->item[i];
+
+		strbuf_addstr(&buf, "    git config unset ");
+		add_config_scope_arg(repo, &buf, item);
+		if (get_comment_key_flags(config, item->path, item->key_id) == KEY_SEEN_TWICE)
+			strbuf_addstr(&buf, "--all ");
+		strbuf_addf(&buf, "%s\n", comment_key_name(item->key_id));
+	}
+	advise(_("\nTo use the default comment string (#) please run\n\n%s"),
+	       buf.buf);
+	strbuf_release(&buf);
+}
+
+static void add_comment_char_advice(struct repository *repo,
+				    struct comment_char_config *config)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct comment_char_config_item *item;
+	/* TRANSLATORS this is a place holder for the value of core.commentString */
+	const char *placeholder = _("<comment string>");
+
+	/*
+	 * If auto is set in the last file that we saw advise the user how to
+	 * update their config.
+	 */
+	if (!config->auto_set_in_file)
+		return;
+
+	add_unset_auto_comment_char_advice(repo, config);
+	item = &config->item[config->nr - 1];
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, "    git config set ");
+	add_config_scope_arg(repo, &buf, item);
+	strbuf_addf(&buf, "%s %s\n", comment_key_name(item->key_id),
+		    placeholder);
+	advise(_("\nTo set a custom comment string please run\n\n"
+		 "%s\nwhere '%s' is the string you wish to use.\n"),
+	       buf.buf, placeholder);
+	strbuf_release(&buf);
+}
+
+#undef KEY_SEEN_ONCE
+#undef KEY_SEEN_TWICE
+#undef COMMENT_KEY_SHIFT
+#undef COMMENT_KEY_MASK
 
 struct repo_config {
 	struct repository *repo;
@@ -2000,18 +2171,26 @@ struct repo_config {
 		.repo = repo_,					\
 	};
 
+static void repo_config_release(struct repo_config *config)
+{
+	comment_char_config_release(&config->comment_char_config);
+}
+
 #ifdef WITH_BREAKING_CHANGES
-static void check_auto_comment_char_config(struct comment_char_config *config)
+static void check_auto_comment_char_config(struct repository *repo,
+					   struct comment_char_config *config)
 {
 	if (!config->auto_set)
 		return;
 
 	die_message(_("Support for '%s=auto' has been removed in Git 3.0"),
 		    comment_key_name(config->last_key_id));
+	add_comment_char_advice(repo, config);
 	die(NULL);
 }
 #else
-static void check_auto_comment_char_config(struct comment_char_config *config)
+static void check_auto_comment_char_config(struct repository *repo,
+					   struct comment_char_config *config)
 {
 	extern bool warn_on_auto_comment_char;
 	const char *DEPRECATED_CONFIG_ENV =
@@ -2031,6 +2210,7 @@ static void check_auto_comment_char_config(struct comment_char_config *config)
 
 	warning(_("Support for '%s=auto' is deprecated and will be removed in "
 		  "Git 3.0"), comment_key_name(config->last_key_id));
+	add_comment_char_advice(repo, config);
 }
 #endif /* WITH_BREAKING_CHANGES */
 
@@ -2039,7 +2219,8 @@ static void check_deprecated_config(struct repo_config *config)
 	if (!config->repo->check_deprecated_config)
 			return;
 
-	check_auto_comment_char_config(&config->comment_char_config);
+	check_auto_comment_char_config(config->repo,
+				       &config->comment_char_config);
 }
 
 static int repo_config_callback(const char *key, const char *value,
@@ -2082,6 +2263,7 @@ static void repo_read_config(struct repository *repo)
 		 */
 		die(_("unknown error occurred while reading the configuration files"));
 	check_deprecated_config(&config);
+	repo_config_release(&config);
 }
 
 static void git_config_check_init(struct repository *repo)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3b2a46c25ce..cc97628d810 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1186,9 +1186,19 @@ test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=
 		test_set_editor "$(pwd)/copy-edit-script.sh" &&
 		git rebase -i HEAD^ 2>err
 	) &&
-	sed -n "s/^warning: //p" err >actual &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^warning: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual &&
 	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index a9dc1e416d1..05f6da4ad98 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -958,10 +958,31 @@ test_expect_success 'commit --status with custom comment character' '
 
 test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
-	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend 2>err &&
-	sed -n "s/^warning: //p" err >actual &&
+	cat >config-include <<-\EOF &&
+	[core]
+	    commentString=:
+	    commentString=%
+	    commentChar=auto
+	EOF
+	test_when_finished "rm config-include" &&
+	test_config include.path "$(pwd)/config-include" &&
+	test_config core.commentChar ! &&
+	GIT_EDITOR=.git/FAKE_EDITOR git commit --amend 2>err &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^warning: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+	    git config unset --file ~/config-include --all core.commentString
+	    git config unset --file ~/config-include core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set --file ~/config-include core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
@@ -990,9 +1011,19 @@ EOF
 test_expect_success WITH_BREAKING_CHANGES 'core.commentChar=auto is rejected' '
 	test_config core.commentChar auto &&
 	test_must_fail git rev-parse --git-dir 2>err &&
-	sed -n "s/^fatal: //p" err >actual &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^fatal: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} has been removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual
 '
-- 
2.49.0.897.gfad3eb7d210

[PATCH v4 0/3] breaking-changes: deprecate support for core.commentChar=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

This series implements the plan to deprecate and remove support for
core.commentChar=auto outlined in [1]. This feature has been the
source of a couple of bug reports recently [2,3] and it is hard to
see how the design can be fixed as it is incompatible with preparing
a commit message template containing comments. When git sees the
deprecated config setting it will print advice based on the user's
config setting to help the user either remove the setting or set a
custom comment string. In the example below core.commentString is set
multiple times in $XDG_CONFIG_HOME/git/config and core.commentChar
is set in ~/.gitconfig and $XDG_CONFIG_HOME/git/config.

warning: Support for 'core.commentChar=auto' is deprecated and will be removed in Git 3.0
hint:
hint: To use the default comment string (#) please run
hint:
hint:     git config unset --file ~/.config/git/config --all core.commentString
hint:     git config unset --file ~/.config/git/config core.commentChar
hint:     git config unset --global core.commentChar
hint:
hint: To set a custom comment string please run
hint:
hint:     git config set --global core.commentChar <comment string>
hint:
hint: where '<comment string>' is the string you wish to use.

[1] https://lore.kernel.org/git/6a3154e0-e7bc-45ae-b554-67ccab18727a@gmail.com
[2] https://lore.kernel.org/git/20250315140913.577404-1-oswald.buddenhagen@gmx.de
[3] https://lore.kernel.org/git/20250626132233.414789-1-ayu.chandekar@gmail.com

Changes since V3:
 - Patch 2: Remove repeated word from commit message

Changes since V2:
 - Patch 1: Punctuation fixes
 - Patch 2: Reworded the commit message slightly
            Remove unnecessary include of advice.h
            Fix variable declaration
 - Patch 3: Include advice.h

Changes since V1:
 - Rebased onto a merge of 'ps/config-wo-the-repository' and 'master'
 - Reworded commit messages
 - What was patch 2 has been split into two separate patches and
   reworked to die when core.commentChar=auto and WITH_BREAKING_CHANGES
   is enabled.

Base-Commit: 1ae5bd276bdf101e37c1a8f2904a2eae05fbb744
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Fremove-auto-comment-char%2Fv4
View-Changes-At: https://github.com/phillipwood/git/compare/1ae5bd276...39d824db4
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/remove-auto-comment-char/v4


Phillip Wood (3):
  breaking-changes: deprecate support for core.commentString=auto
  config: warn on core.commentString=auto
  commit: print advice when core.commentString=auto

 Documentation/BreakingChanges.adoc |   5 +
 Documentation/config/core.adoc     |  20 +-
 builtin/commit.c                   |   7 +
 builtin/merge.c                    |   3 +
 builtin/rebase.c                   |   3 +
 builtin/revert.c                   |   7 +
 config.c                           | 297 ++++++++++++++++++++++++++++-
 environment.c                      |  11 +-
 environment.h                      |   3 +
 repository.c                       |   1 +
 repository.h                       |   3 +
 t/t3404-rebase-interactive.sh      |  19 +-
 t/t3418-rebase-continue.sh         |   2 +-
 t/t7502-commit-porcelain.sh        |  52 ++++-
 14 files changed, 421 insertions(+), 12 deletions(-)

Range-diff against v3:
1:  5b921064f1e = 1:  5b921064f1e breaking-changes: deprecate support for core.commentString=auto
2:  5dd897c95e6 ! 2:  e92511ce21c config: warn on core.commentString=auto
    @@ Commit message
     
         As support for this setting was deprecated in the last commit print a
         warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
    -    Avoid bombarding the user with warnings by only printing it (a) when
    -    running commands commands that call "git commit" and (b) only once
    -    per command. Some scaffolding is added to repo_read_config() to allow
    +    Avoid bombarding the user with warnings by only printing it (a)
    +    when running commands that call "git commit" and (b) only once per
    +    command. Some scaffolding is added to repo_read_config() to allow
         it to detect deprecated config settings and warn about them. As both
         "core.commentChar" and "core.commentString" set the comment character
         we record which one of them is used and tailor the warning message
3:  ee6cf11a82c = 3:  39d824db4ab commit: print advice when core.commentString=auto
-- 
2.49.0.897.gfad3eb7d210

[PATCH v4 1/3] breaking-changes: deprecate support for core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When "core.commentString" is set to "auto" then "git commit" will
automatically select the comment character ensuring that it is not the
first character on any of the lines in the commit message. This was
introduced by commit 84c9dc2c5a2 (commit: allow core.commentChar=auto
for character auto selection, 2014-05-17). The motivation seems to be
to avoid commenting out lines from the existing message when amending
a commit that was created with a message from a file.

Unfortunately this feature does not work with:

 * commit message templates that contain comments.

 * prepare-commit-msg hooks that introduce comments.

 * "git commit --cleanup=strip --edit -F <file>" which means that it
   is incompatible with

   - the "fixup" and "squash" commands of "git rebase -i" as the
     comments added by those commands are then treated as part of
     the commit message.

   - the conflict comments added to the commit message by "git
     cherry-pick", "git rebase" etc. as these comments are then
     treated as part of the commit message.

It is also ignored by "git notes" when amending a note.

The issues with comments coming from a template, hook or file are a
consequence of the design of this feature and are therefore hard to
fix.

As the costs of this feature outweigh the benefits, deprecate it and
remove it in Git 3.0. If someone comes up with some patches that fix
all the issues in a maintainable way then I'd be happy to see this
change reverted.

The next commits will add a warning and some advice for users on how
they can update their config settings.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/BreakingChanges.adoc |  5 +++++
 Documentation/config/core.adoc     | 20 ++++++++++++++++++--
 builtin/commit.c                   |  4 ++++
 environment.c                      | 10 ++++++++--
 environment.h                      |  2 ++
 t/t3404-rebase-interactive.sh      |  2 +-
 t/t3418-rebase-continue.sh         |  2 +-
 t/t7502-commit-porcelain.sh        |  4 ++--
 8 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
index f8d2eba061c..344ce500603 100644
--- a/Documentation/BreakingChanges.adoc
+++ b/Documentation/BreakingChanges.adoc
@@ -239,6 +239,11 @@ These features will be removed.
 +
 The command will be removed.
 
+* Support for `core.commentString=auto` has been deprecated and will
+  be removed in Git 3.0.
++
+cf. <xmqqa59i45wc.fsf@gitster.g>
+
 == Superseded features that will not be deprecated
 
 Some features have gained newer replacements that aim to improve the design in
diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
index 9fde1ab63a7..7133f00c38b 100644
--- a/Documentation/config/core.adoc
+++ b/Documentation/config/core.adoc
@@ -531,9 +531,25 @@ core.commentString::
 	commented, and removes them after the editor returns
 	(default '#').
 +
-If set to "auto", `git-commit` would select a character that is not
+ifndef::with-breaking-changes[]
+If set to "auto", `git-commit` will select a character that is not
 the beginning character of any line in existing commit messages.
-+
+Support for this value is deprecated and will be removed in Git 3.0
+due to the following limitations:
++
+--
+* It is incompatible with adding comments in a commit message
+  template. This includes the conflicts comments added to
+  the commit message by `cherry-pick`, `merge`, `rebase` and
+  `revert`.
+* It is incompatible with adding comments to the commit message
+  in the `prepare-commit-msg` hook.
+* It is incompatible with the `fixup` and `squash` commands when
+  rebasing,
+* It is not respected by `git notes`
+--
++
+endif::with-breaking-changes[]
 Note that these two variables are aliases of each other, and in modern
 versions of Git you are free to use a string (e.g., `//` or `⁑⁕⁑`) with
 `commentChar`. Versions of Git prior to v2.45.0 will ignore
diff --git a/builtin/commit.c b/builtin/commit.c
index 757f51eac82..d25cc07a355 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -683,6 +683,7 @@ static int author_date_is_interesting(void)
 	return author_message || force_date;
 }
 
+#ifndef WITH_BREAKING_CHANGES
 static void adjust_comment_line_char(const struct strbuf *sb)
 {
 	char candidates[] = "#;@!$%^&|:";
@@ -720,6 +721,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	free(comment_line_str_to_free);
 	comment_line_str = comment_line_str_to_free = xstrfmt("%c", *p);
 }
+#endif /* !WITH_BREAKING_CHANGES */
 
 static void prepare_amend_commit(struct commit *commit, struct strbuf *sb,
 				struct pretty_print_context *ctx)
@@ -916,8 +918,10 @@ 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"));
 
+#ifndef WITH_BREAKING_CHANGES
 	if (auto_comment_line_char)
 		adjust_comment_line_char(&sb);
+#endif /* !WITH_BREAKING_CHANGES */
 	strbuf_release(&sb);
 
 	/* This checks if committer ident is explicitly given */
diff --git a/environment.c b/environment.c
index a0ac5934b37..4c87876d483 100644
--- a/environment.c
+++ b/environment.c
@@ -122,7 +122,9 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT;
  */
 const char *comment_line_str = "#";
 char *comment_line_str_to_free;
+#ifndef WITH_BREAKING_CHANGES
 int auto_comment_line_char;
+#endif /* !WITH_BREAKING_CHANGES */
 
 /* This is set by setup_git_directory_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
@@ -459,18 +461,22 @@ static int git_default_core_config(const char *var, const char *value,
 
 	if (!strcmp(var, "core.commentchar") ||
 	    !strcmp(var, "core.commentstring")) {
-		if (!value)
+		if (!value) {
 			return config_error_nonbool(var);
-		else if (!strcasecmp(value, "auto")) {
+#ifndef WITH_BREAKING_CHANGES
+		} else if (!strcasecmp(value, "auto")) {
 			auto_comment_line_char = 1;
 			FREE_AND_NULL(comment_line_str_to_free);
 			comment_line_str = "#";
+#endif /* !WITH_BREAKING_CHANGES */
 		} else if (value[0]) {
 			if (strchr(value, '\n'))
 				return error(_("%s cannot contain newline"), var);
 			comment_line_str = value;
 			FREE_AND_NULL(comment_line_str_to_free);
+#ifndef WITH_BREAKING_CHANGES
 			auto_comment_line_char = 0;
+#endif /* !WITH_BREAKING_CHANGES */
 		} else
 			return error(_("%s must have at least one character"), var);
 		return 0;
diff --git a/environment.h b/environment.h
index 8cfce41015b..e75c4abb388 100644
--- a/environment.h
+++ b/environment.h
@@ -208,7 +208,9 @@ extern char *excludes_file;
  */
 extern const char *comment_line_str;
 extern char *comment_line_str_to_free;
+#ifndef WITH_BREAKING_CHANGES
 extern int auto_comment_line_char;
+#endif /* !WITH_BREAKING_CHANGES */
 
 # endif /* USE_THE_REPOSITORY_VARIABLE */
 #endif /* ENVIRONMENT_H */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 6bac217ed35..ce0aebb9a7e 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1176,7 +1176,7 @@ test_expect_success 'rebase -i respects core.commentchar' '
 	test B = $(git cat-file commit HEAD^ | sed -ne \$p)
 '
 
-test_expect_success 'rebase -i respects core.commentchar=auto' '
+test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=auto' '
 	test_config core.commentchar auto &&
 	write_script copy-edit-script.sh <<-\EOF &&
 	cp "$1" edit-script
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index b8a8dd77e74..f9b8999db50 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -328,7 +328,7 @@ test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas
 	test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
 '
 
-test_expect_success 'no change in comment character due to conflicts markers with core.commentChar=auto' '
+test_expect_success !WITH_BREAKING_CHANGES 'no change in comment character due to conflicts markers with core.commentChar=auto' '
 	git checkout -b branch-a &&
 	test_commit A F1 &&
 	git checkout -b branch-b HEAD^ &&
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index b37e2018a74..65b4519a715 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -956,13 +956,13 @@ test_expect_success 'commit --status with custom comment character' '
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'switch core.commentchar' '
+test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
 	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
-test_expect_success 'switch core.commentchar but out of options' '
+test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar but out of options' '
 	cat >text <<\EOF &&
 # 1
 ; 2
-- 
2.49.0.897.gfad3eb7d210

[PATCH v4 2/3] config: warn on core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

As support for this setting was deprecated in the last commit print a
warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
Avoid bombarding the user with warnings by only printing it (a)
when running commands that call "git commit" and (b) only once per
command. Some scaffolding is added to repo_read_config() to allow
it to detect deprecated config settings and warn about them. As both
"core.commentChar" and "core.commentString" set the comment character
we record which one of them is used and tailor the warning message
appropriately.

Note the odd combination of die_message() followed by die(NULL)
is to allow the next commit to insert a call to advise() in the middle.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 builtin/commit.c              |   3 +
 builtin/merge.c               |   3 +
 builtin/rebase.c              |   3 +
 builtin/revert.c              |   7 +++
 config.c                      | 115 +++++++++++++++++++++++++++++++++-
 environment.c                 |   1 +
 environment.h                 |   1 +
 repository.c                  |   1 +
 repository.h                  |   3 +
 t/t3404-rebase-interactive.sh |   7 ++-
 t/t7502-commit-porcelain.sh   |  17 ++++-
 11 files changed, 157 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d25cc07a355..f821fdcfcc3 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1783,6 +1783,9 @@ int cmd_commit(int argc,
 	show_usage_with_options_if_asked(argc, argv,
 					 builtin_commit_usage, builtin_commit_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/merge.c b/builtin/merge.c
index dc4cb8fb14d..794cb7bb269 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1378,6 +1378,9 @@ int cmd_merge(int argc,
 	show_usage_with_options_if_asked(argc, argv,
 					 builtin_merge_usage, builtin_merge_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 72a52bdfb98..962917ec480 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1242,6 +1242,9 @@ int cmd_rebase(int argc,
 					 builtin_rebase_usage,
 					 builtin_rebase_options);
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
 
diff --git a/builtin/revert.c b/builtin/revert.c
index e07c2217fe8..b197848bb0a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -4,6 +4,7 @@
 #include "builtin.h"
 #include "parse-options.h"
 #include "diff.h"
+#include "environment.h"
 #include "gettext.h"
 #include "revision.h"
 #include "rerere.h"
@@ -285,6 +286,9 @@ int cmd_revert(int argc,
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int res;
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	opts.action = REPLAY_REVERT;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, prefix, &opts);
@@ -302,6 +306,9 @@ struct repository *repo UNUSED)
 	struct replay_opts opts = REPLAY_OPTS_INIT;
 	int res;
 
+#ifndef WITH_BREAKING_CHANGES
+	warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
 	opts.action = REPLAY_PICK;
 	sequencer_init_config(&opts);
 	res = run_sequencer(argc, argv, prefix, &opts);
diff --git a/config.c b/config.c
index 97ffef42700..18b42197095 100644
--- a/config.c
+++ b/config.c
@@ -11,6 +11,7 @@
 #include "date.h"
 #include "branch.h"
 #include "config.h"
+#include "dir.h"
 #include "parse.h"
 #include "convert.h"
 #include "environment.h"
@@ -1951,10 +1952,110 @@ int git_configset_get_pathname(struct config_set *set, const char *key, char **d
 		return 1;
 }
 
+struct comment_char_config {
+	unsigned last_key_id;
+	bool auto_set;
+};
+
+#define COMMENT_CHAR_CFG_INIT { 0 }
+
+static const char *comment_key_name(unsigned id)
+{
+	static const char *name[] = {
+		"core.commentChar",
+		"core.commentString",
+	};
+
+	if (id >= ARRAY_SIZE(name))
+		BUG("invalid comment key id");
+
+	return name[id];
+}
+
+static void comment_char_callback(const char *key, const char *value,
+				  const struct config_context *ctx UNUSED,
+				  void *data)
+{
+	struct comment_char_config *config = data;
+	unsigned key_id;
+
+	if (!strcmp(key, "core.commentchar"))
+		key_id = 0;
+	else if (!strcmp(key, "core.commentstring"))
+		key_id = 1;
+	else
+		return;
+
+	config->last_key_id = key_id;
+	config->auto_set = value && !strcmp(value, "auto");
+}
+
+struct repo_config {
+	struct repository *repo;
+	struct comment_char_config comment_char_config;
+};
+
+#define REPO_CONFIG_INIT(repo_) {				\
+		.comment_char_config = COMMENT_CHAR_CFG_INIT,	\
+		.repo = repo_,					\
+	};
+
+#ifdef WITH_BREAKING_CHANGES
+static void check_auto_comment_char_config(struct comment_char_config *config)
+{
+	if (!config->auto_set)
+		return;
+
+	die_message(_("Support for '%s=auto' has been removed in Git 3.0"),
+		    comment_key_name(config->last_key_id));
+	die(NULL);
+}
+#else
+static void check_auto_comment_char_config(struct comment_char_config *config)
+{
+	extern bool warn_on_auto_comment_char;
+	const char *DEPRECATED_CONFIG_ENV =
+				"GIT_AUTO_COMMENT_CHAR_CONFIG_WARNING_GIVEN";
+
+	if (!config->auto_set || !warn_on_auto_comment_char)
+		return;
+
+	/*
+	 * Use an environment variable to ensure that subprocesses do not repeat
+	 * the warning.
+	 */
+	if (git_env_bool(DEPRECATED_CONFIG_ENV, false))
+		return;
+
+	setenv(DEPRECATED_CONFIG_ENV, "true", true);
+
+	warning(_("Support for '%s=auto' is deprecated and will be removed in "
+		  "Git 3.0"), comment_key_name(config->last_key_id));
+}
+#endif /* WITH_BREAKING_CHANGES */
+
+static void check_deprecated_config(struct repo_config *config)
+{
+	if (!config->repo->check_deprecated_config)
+			return;
+
+	check_auto_comment_char_config(&config->comment_char_config);
+}
+
+static int repo_config_callback(const char *key, const char *value,
+				const struct config_context *ctx, void *data)
+{
+	struct repo_config *config = data;
+
+	comment_char_callback(key, value, ctx, &config->comment_char_config);
+	return config_set_callback(key, value, ctx, config->repo->config);
+}
+
 /* Functions use to read configuration from a repository */
 static void repo_read_config(struct repository *repo)
 {
 	struct config_options opts = { 0 };
+	struct repo_config config = REPO_CONFIG_INIT(repo);
 
 	opts.respect_includes = 1;
 	opts.commondir = repo->commondir;
@@ -1966,8 +2067,8 @@ static void repo_read_config(struct repository *repo)
 		git_configset_clear(repo->config);
 
 	git_configset_init(repo->config);
-	if (config_with_options(config_set_callback, repo->config, NULL,
-				repo, &opts) < 0)
+	if (config_with_options(repo_config_callback, &config, NULL, repo,
+				&opts) < 0)
 		/*
 		 * config_with_options() normally returns only
 		 * zero, as most errors are fatal, and
@@ -1980,6 +2081,7 @@ static void repo_read_config(struct repository *repo)
 		 * immediately.
 		 */
 		die(_("unknown error occurred while reading the configuration files"));
+	check_deprecated_config(&config);
 }
 
 static void git_config_check_init(struct repository *repo)
@@ -2667,6 +2769,14 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	char *contents = NULL;
 	size_t contents_sz;
 	struct config_store_data store = CONFIG_STORE_INIT;
+	bool saved_check_deprecated_config = r->check_deprecated_config;
+
+	/*
+	 * Do not warn or die if there are deprecated config settings as
+	 * we want the user to be able to change those settings by running
+	 * "git config".
+	 */
+	r->check_deprecated_config = false;
 
 	validate_comment_string(comment);
 
@@ -2898,6 +3008,7 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
 	if (in_fd >= 0)
 		close(in_fd);
 	config_store_data_clear(&store);
+	r->check_deprecated_config = saved_check_deprecated_config;
 	return ret;
 
 write_err_out:
diff --git a/environment.c b/environment.c
index 4c87876d483..1ffa2ff30b2 100644
--- a/environment.c
+++ b/environment.c
@@ -124,6 +124,7 @@ const char *comment_line_str = "#";
 char *comment_line_str_to_free;
 #ifndef WITH_BREAKING_CHANGES
 int auto_comment_line_char;
+bool warn_on_auto_comment_char;
 #endif /* !WITH_BREAKING_CHANGES */
 
 /* This is set by setup_git_directory_gently() and/or git_default_config() */
diff --git a/environment.h b/environment.h
index e75c4abb388..51898c99cd1 100644
--- a/environment.h
+++ b/environment.h
@@ -210,6 +210,7 @@ extern const char *comment_line_str;
 extern char *comment_line_str_to_free;
 #ifndef WITH_BREAKING_CHANGES
 extern int auto_comment_line_char;
+extern bool warn_on_auto_comment_char;
 #endif /* !WITH_BREAKING_CHANGES */
 
 # endif /* USE_THE_REPOSITORY_VARIABLE */
diff --git a/repository.c b/repository.c
index ecd691181fc..8af73923d34 100644
--- a/repository.c
+++ b/repository.c
@@ -57,6 +57,7 @@ void initialize_repository(struct repository *repo)
 	repo->parsed_objects = parsed_object_pool_new(repo);
 	ALLOC_ARRAY(repo->index, 1);
 	index_state_init(repo->index, repo);
+	repo->check_deprecated_config = true;
 
 	/*
 	 * When a command runs inside a repository, it learns what
diff --git a/repository.h b/repository.h
index 042dc93f0f2..5808a5d6108 100644
--- a/repository.h
+++ b/repository.h
@@ -161,6 +161,9 @@ struct repository {
 
 	/* Indicate if a repository has a different 'commondir' from 'gitdir' */
 	unsigned different_commondir:1;
+
+	/* Should repo_config() check for deprecated settings */
+	bool check_deprecated_config;
 };
 
 #ifdef USE_THE_REPOSITORY_VARIABLE
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ce0aebb9a7e..3b2a46c25ce 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1184,8 +1184,13 @@ test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=
 	test_when_finished "git rebase --abort || :" &&
 	(
 		test_set_editor "$(pwd)/copy-edit-script.sh" &&
-		git rebase -i HEAD^
+		git rebase -i HEAD^ 2>err
 	) &&
+	sed -n "s/^warning: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+	EOF
+	test_cmp expect actual &&
 	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
 '
 
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 65b4519a715..a9dc1e416d1 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -958,7 +958,12 @@ test_expect_success 'commit --status with custom comment character' '
 
 test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
-	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
+	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend 2>err &&
+	sed -n "s/^warning: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+	EOF
+	test_cmp expect actual &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
@@ -982,4 +987,14 @@ EOF
 	)
 '
 
+test_expect_success WITH_BREAKING_CHANGES 'core.commentChar=auto is rejected' '
+	test_config core.commentChar auto &&
+	test_must_fail git rev-parse --git-dir 2>err &&
+	sed -n "s/^fatal: //p" err >actual &&
+	cat >expect <<-EOF &&
+	Support for ${SQ}core.commentChar=auto${SQ} has been removed in Git 3.0
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.49.0.897.gfad3eb7d210

[PATCH v4 3/3] commit: print advice when core.commentString=auto

[Phillip Wood]

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Add some advice on how to change the config settings when
"core.commentString=auto" or "core.commentChar=auto". The advice
includes instructions for clearing the config setting or setting a
fixed comment string. To try and be as specific as possible, the advice
is customized based on the user's config. If "core.commentString=auto"
is set in the system config and the user does not have write
access then the advice omits the instructions to clear the config
and recommends changing the global config instead. An alternative
approach would be to advise the user to run "git config --show-origin"
and leave them to figure out how to fix it themselves but that seems
rather unfriendly. As we're forcing them to update their config we
should try and make that as easy as possible.

In order to generate this advice we need to record each file where
either of the config keys is set and whether a key occurs more that
once in a given file. This lets us generate the list of commands to
remove all the keys and also tells us which key the "auto" setting
comes from.

As we want the user to update their config we do not provide a way
for this advice to be disabled other than changing the value of
"core.commentChar" or "core.commentString".

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 config.c                      | 196 ++++++++++++++++++++++++++++++++--
 t/t3404-rebase-interactive.sh |  12 ++-
 t/t7502-commit-porcelain.sh   |  37 ++++++-
 3 files changed, 234 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 18b42197095..18dcf341d58 100644
--- a/config.c
+++ b/config.c
@@ -8,6 +8,7 @@
 
 #include "git-compat-util.h"
 #include "abspath.h"
+#include "advice.h"
 #include "date.h"
 #include "branch.h"
 #include "config.h"
@@ -1955,9 +1956,51 @@ int git_configset_get_pathname(struct config_set *set, const char *key, char **d
 struct comment_char_config {
 	unsigned last_key_id;
 	bool auto_set;
+	bool auto_set_in_file;
+	struct strintmap key_flags;
+	size_t alloc, nr;
+	struct comment_char_config_item {
+		unsigned key_id;
+		char *path;
+		enum config_scope scope;
+	} *item;
 };
 
-#define COMMENT_CHAR_CFG_INIT { 0 }
+#define COMMENT_CHAR_CFG_INIT {			\
+		.key_flags = STRINTMAP_INIT,	\
+	}
+
+static void comment_char_config_release(struct comment_char_config *config)
+{
+	strintmap_clear(&config->key_flags);
+	for (size_t i = 0; i < config->nr; i++)
+		free(config->item[i].path);
+	free(config->item);
+}
+
+/* Used to track whether the key occurs more than once in a given file */
+#define KEY_SEEN_ONCE 1u
+#define KEY_SEEN_TWICE 2u
+#define COMMENT_KEY_SHIFT(id) (2 * (id))
+#define COMMENT_KEY_MASK(id) (3u << COMMENT_KEY_SHIFT(id))
+
+static void set_comment_key_flags(struct comment_char_config *config,
+				  const char *path, unsigned id, unsigned value)
+{
+	unsigned old = strintmap_get(&config->key_flags, path);
+	unsigned new = (old & ~COMMENT_KEY_MASK(id)) |
+				value << COMMENT_KEY_SHIFT(id);
+
+	strintmap_set(&config->key_flags, path, new);
+}
+
+static unsigned get_comment_key_flags(struct comment_char_config *config,
+				      const char *path, unsigned id)
+{
+	unsigned value = strintmap_get(&config->key_flags, path);
+
+	return (value & COMMENT_KEY_MASK(id)) >> COMMENT_KEY_SHIFT(id);
+}
 
 static const char *comment_key_name(unsigned id)
 {
@@ -1973,10 +2016,10 @@ static const char *comment_key_name(unsigned id)
 }
 
 static void comment_char_callback(const char *key, const char *value,
-				  const struct config_context *ctx UNUSED,
-				  void *data)
+				  const struct config_context *ctx, void *data)
 {
 	struct comment_char_config *config = data;
+	const struct key_value_info *kvi = ctx->kvi;
 	unsigned key_id;
 
 	if (!strcmp(key, "core.commentchar"))
@@ -1988,7 +2031,135 @@ static void comment_char_callback(const char *key, const char *value,
 
 	config->last_key_id = key_id;
 	config->auto_set = value && !strcmp(value, "auto");
-}
+	if (kvi->origin_type != CONFIG_ORIGIN_FILE) {
+		return;
+	} else if (get_comment_key_flags(config, kvi->filename, key_id)) {
+		set_comment_key_flags(config, kvi->filename, key_id,
+				      KEY_SEEN_TWICE);
+	} else {
+		struct comment_char_config_item *item;
+
+		ALLOC_GROW_BY(config->item, config->nr, 1, config->alloc);
+		item = &config->item[config->nr - 1];
+		item->key_id = key_id;
+		item->scope = kvi->scope;
+		item->path = xstrdup(kvi->filename);
+		set_comment_key_flags(config, kvi->filename, key_id,
+				      KEY_SEEN_ONCE);
+	}
+	config->auto_set_in_file = config->auto_set;
+}
+
+static void add_config_scope_arg(struct repository *repo, struct strbuf *buf,
+				 struct comment_char_config_item *item)
+{
+	char *global_config = git_global_config();
+	char *system_config = git_system_config();
+
+	if (item->scope == CONFIG_SCOPE_SYSTEM && access(item->path, W_OK)) {
+		/*
+		 * If the user cannot write to the system config recommend
+		 * setting the global config instead.
+		 */
+		strbuf_addstr(buf, "--global ");
+	} else if (fspatheq(item->path, system_config)) {
+		strbuf_addstr(buf, "--system ");
+	} else if (fspatheq(item->path, global_config)) {
+		strbuf_addstr(buf, "--global ");
+	} else if (fspatheq(item->path,
+			    mkpath("%s/config",
+				   repo_get_git_dir(repo)))) {
+		; /* --local is the default */
+	} else if (fspatheq(item->path,
+			    mkpath("%s/config.worktree",
+				   repo_get_common_dir(repo)))) {
+		strbuf_addstr(buf, "--worktree ");
+	} else {
+		const char *path = item->path;
+		const char *home = getenv("HOME");
+
+		strbuf_addstr(buf, "--file ");
+		if (home && !fspathncmp(path, home, strlen(home))) {
+			path += strlen(home);
+			if (!fspathncmp(path, "/", 1))
+				path++;
+			strbuf_addstr(buf, "~/");
+		}
+		sq_quote_buf_pretty(buf, path);
+		strbuf_addch(buf, ' ');
+	}
+
+	free(global_config);
+	free(system_config);
+}
+
+static bool can_unset_comment_char_config(struct comment_char_config *config)
+{
+	for (size_t i = 0; i < config->nr; i++) {
+		struct comment_char_config_item *item = &config->item[i];
+
+		if (item->scope == CONFIG_SCOPE_SYSTEM &&
+		    access(item->path, W_OK))
+			return false;
+	}
+
+	return true;
+}
+
+static void add_unset_auto_comment_char_advice(struct repository *repo,
+					       struct comment_char_config *config)
+{
+	struct strbuf buf = STRBUF_INIT;
+
+	if (!can_unset_comment_char_config(config))
+		return;
+
+	for (size_t i = 0; i < config->nr; i++) {
+		struct comment_char_config_item *item = &config->item[i];
+
+		strbuf_addstr(&buf, "    git config unset ");
+		add_config_scope_arg(repo, &buf, item);
+		if (get_comment_key_flags(config, item->path, item->key_id) == KEY_SEEN_TWICE)
+			strbuf_addstr(&buf, "--all ");
+		strbuf_addf(&buf, "%s\n", comment_key_name(item->key_id));
+	}
+	advise(_("\nTo use the default comment string (#) please run\n\n%s"),
+	       buf.buf);
+	strbuf_release(&buf);
+}
+
+static void add_comment_char_advice(struct repository *repo,
+				    struct comment_char_config *config)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct comment_char_config_item *item;
+	/* TRANSLATORS this is a place holder for the value of core.commentString */
+	const char *placeholder = _("<comment string>");
+
+	/*
+	 * If auto is set in the last file that we saw advise the user how to
+	 * update their config.
+	 */
+	if (!config->auto_set_in_file)
+		return;
+
+	add_unset_auto_comment_char_advice(repo, config);
+	item = &config->item[config->nr - 1];
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, "    git config set ");
+	add_config_scope_arg(repo, &buf, item);
+	strbuf_addf(&buf, "%s %s\n", comment_key_name(item->key_id),
+		    placeholder);
+	advise(_("\nTo set a custom comment string please run\n\n"
+		 "%s\nwhere '%s' is the string you wish to use.\n"),
+	       buf.buf, placeholder);
+	strbuf_release(&buf);
+}
+
+#undef KEY_SEEN_ONCE
+#undef KEY_SEEN_TWICE
+#undef COMMENT_KEY_SHIFT
+#undef COMMENT_KEY_MASK
 
 struct repo_config {
 	struct repository *repo;
@@ -2000,18 +2171,26 @@ struct repo_config {
 		.repo = repo_,					\
 	};
 
+static void repo_config_release(struct repo_config *config)
+{
+	comment_char_config_release(&config->comment_char_config);
+}
+
 #ifdef WITH_BREAKING_CHANGES
-static void check_auto_comment_char_config(struct comment_char_config *config)
+static void check_auto_comment_char_config(struct repository *repo,
+					   struct comment_char_config *config)
 {
 	if (!config->auto_set)
 		return;
 
 	die_message(_("Support for '%s=auto' has been removed in Git 3.0"),
 		    comment_key_name(config->last_key_id));
+	add_comment_char_advice(repo, config);
 	die(NULL);
 }
 #else
-static void check_auto_comment_char_config(struct comment_char_config *config)
+static void check_auto_comment_char_config(struct repository *repo,
+					   struct comment_char_config *config)
 {
 	extern bool warn_on_auto_comment_char;
 	const char *DEPRECATED_CONFIG_ENV =
@@ -2031,6 +2210,7 @@ static void check_auto_comment_char_config(struct comment_char_config *config)
 
 	warning(_("Support for '%s=auto' is deprecated and will be removed in "
 		  "Git 3.0"), comment_key_name(config->last_key_id));
+	add_comment_char_advice(repo, config);
 }
 #endif /* WITH_BREAKING_CHANGES */
 
@@ -2039,7 +2219,8 @@ static void check_deprecated_config(struct repo_config *config)
 	if (!config->repo->check_deprecated_config)
 			return;
 
-	check_auto_comment_char_config(&config->comment_char_config);
+	check_auto_comment_char_config(config->repo,
+				       &config->comment_char_config);
 }
 
 static int repo_config_callback(const char *key, const char *value,
@@ -2082,6 +2263,7 @@ static void repo_read_config(struct repository *repo)
 		 */
 		die(_("unknown error occurred while reading the configuration files"));
 	check_deprecated_config(&config);
+	repo_config_release(&config);
 }
 
 static void git_config_check_init(struct repository *repo)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3b2a46c25ce..cc97628d810 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1186,9 +1186,19 @@ test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=
 		test_set_editor "$(pwd)/copy-edit-script.sh" &&
 		git rebase -i HEAD^ 2>err
 	) &&
-	sed -n "s/^warning: //p" err >actual &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^warning: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual &&
 	test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index a9dc1e416d1..05f6da4ad98 100755
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -958,10 +958,31 @@ test_expect_success 'commit --status with custom comment character' '
 
 test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
 	test_commit "#foo" foo &&
-	GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend 2>err &&
-	sed -n "s/^warning: //p" err >actual &&
+	cat >config-include <<-\EOF &&
+	[core]
+	    commentString=:
+	    commentString=%
+	    commentChar=auto
+	EOF
+	test_when_finished "rm config-include" &&
+	test_config include.path "$(pwd)/config-include" &&
+	test_config core.commentChar ! &&
+	GIT_EDITOR=.git/FAKE_EDITOR git commit --amend 2>err &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^warning: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+	    git config unset --file ~/config-include --all core.commentString
+	    git config unset --file ~/config-include core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set --file ~/config-include core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual &&
 	test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
@@ -990,9 +1011,19 @@ EOF
 test_expect_success WITH_BREAKING_CHANGES 'core.commentChar=auto is rejected' '
 	test_config core.commentChar auto &&
 	test_must_fail git rev-parse --git-dir 2>err &&
-	sed -n "s/^fatal: //p" err >actual &&
+	sed -n "s/^hint: *\$//p; s/^hint: //p; s/^fatal: //p" err >actual &&
 	cat >expect <<-EOF &&
 	Support for ${SQ}core.commentChar=auto${SQ} has been removed in Git 3.0
+
+	To use the default comment string (#) please run
+
+	    git config unset core.commentChar
+
+	To set a custom comment string please run
+
+	    git config set core.commentChar <comment string>
+
+	where ${SQ}<comment string>${SQ} is the string you wish to use.
 	EOF
 	test_cmp expect actual
 '
-- 
2.49.0.897.gfad3eb7d210