[PATCH] docs: clarify meaning of core.commentString=auto

[PATCH] docs: clarify meaning of core.commentString=auto

[Oswald Buddenhagen]

I had to read the source to make sense of the feature, which is clearly
not an acceptable state. Make the docu more specific and less
misleading.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>

---

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

given the rather crippling limitations of this feature, does anyone
actually use it?
---
 Documentation/config/core.adoc | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc
index 8f6d8e7754..b470da72ba 100644
--- a/Documentation/config/core.adoc
+++ b/Documentation/config/core.adoc
@@ -526,8 +526,11 @@ core.commentString::
 	commented, and removes them after the editor returns
 	(default '#').
 +
-If set to "auto", `git-commit` would select a character that is not
-the beginning character of any line in existing commit messages.
+If set to "auto", `git-commit` will select the first character
+from the set "#;@!$%^&|:" that does not appear at the beginning
+of any line in the prepared commit message prior to editing.
+Note that this makes it impossible to include comments in the
+prepare-commit-msg hook's output or the commit message template.
 +
 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
-- 
2.49.0.416.g2f302f2ef0.dirty

Re: [PATCH] docs: clarify meaning of core.commentString=auto

[Junio C Hamano]

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

> -If set to "auto", `git-commit` would select a character that is not
> -the beginning character of any line in existing commit messages.

This is so far in the past but I suspect this was deliberately left
vague so that we can add (or subtract) the set of possible letters
to use.

> +If set to "auto", `git-commit` will select the first character
> +from the set "#;@!$%^&|:" that does not appear at the beginning
> +of any line in the prepared commit message prior to editing.

So I am not sure if this is an improvement.

> +Note that this makes it impossible to include comments in the
> +prepare-commit-msg hook's output or the commit message template.

Care to rephrase?  There are degrees of possibilities and "makes it
impossible" is being overly broad.

I suspect you are saying that it is not nice to make it the
responsibility of the end-user who chooses "auto" to ensure that
they adjust the default '#' comments injected from the template or
hook output when

 - they have a line that begins with '#' in their message;
 - the "auto" mechanism chooses to use ';' as the comment character;
 - the template is written assuming '#' as the comment character and
   has comments.

before making a commit.  But "this makes it impossible" does not
quite convey that to casual readers.

Thanks.

Re: [PATCH] docs: clarify meaning of core.commentString=auto

[Taylor Blau]

On Mon, Mar 17, 2025 at 01:17:54PM -0700, Junio C Hamano wrote:
> Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
> > -If set to "auto", `git-commit` would select a character that is not
> > -the beginning character of any line in existing commit messages.
>
> This is so far in the past but I suspect this was deliberately left
> vague so that we can add (or subtract) the set of possible letters
> to use.
>
> > +If set to "auto", `git-commit` will select the first character
> > +from the set "#;@!$%^&|:" that does not appear at the beginning
> > +of any line in the prepared commit message prior to editing.
>
> So I am not sure if this is an improvement.

I had a similar thought while reading. The vague wording of the existing
text gives us freedom to change that set of characters in the code
without the possibility of the documentation becoming stale.

That's pretty academic, though, so I don't have a strong feeling against
this portion of the patch, but I do vaguely prefer the existing wording.

Thanks,
Taylor

Re: [PATCH] docs: clarify meaning of core.commentString=auto

[Oswald Buddenhagen]

On Mon, Mar 17, 2025 at 01:17:54PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> -If set to "auto", `git-commit` would select a character that is not
>> -the beginning character of any line in existing commit messages.
>
>This is so far in the past but I suspect this was deliberately left
>vague so that we can add (or subtract) the set of possible letters
>to use.
>
no such consideration was voiced at any point.
https://lore.kernel.org/git/CALy3b+m7YkYB+mPEnAQnjKFAwUS_PqCUFtuxzN7hwhmNfMrw3Q@mail.gmail.com/T/#u

On Mon, Mar 17, 2025 at 05:34:12PM -0400, Taylor Blau wrote:
>I had a similar thought while reading. The vague wording of the
>existing
>text gives us freedom to change that set of characters in the code
>without the possibility of the documentation becoming stale.
>
>That's pretty academic, though, so I don't have a strong feeling against
>this portion of the patch, but I do vaguely prefer the existing wording.
>
apart from changing it being academic, the feature is also formally
useless without documenting the candidate comment characters. formally,
because in practice the user would just guess, but that doesn't make the
omission a good thing.

On Mon, Mar 17, 2025 at 01:17:54PM -0700, Junio C Hamano wrote:
>> +Note that this makes it impossible to include comments in the
>> +prepare-commit-msg hook's output or the commit message template.
>
>Care to rephrase?  There are degrees of possibilities and "makes it
>impossible" is being overly broad.
>
>I suspect you are saying that it is not nice to make it the
>responsibility of the end-user who chooses "auto" to ensure that
>they adjust the default '#' comments injected from the template or
>hook output when
>
> - they have a line that begins with '#' in their message;
> - the "auto" mechanism chooses to use ';' as the comment character;
> - the template is written assuming '#' as the comment character and
>   has comments.
>
>before making a commit.  But "this makes it impossible" does not
>quite convey that to casual readers.
>
no, i meant what i wrote: it makes it _literally_ impossible. it follows
from the preceding sentence that _whatever_ is in the template will NOT
be the comment char. the commit that introduced that feature (84c9dc2c5)
already mentioned that limitation.

reading through the thread of the original submission, the feature is a
workaround for `commit -m` and `commit --amend` being inconsistent wrt.
message washing. i find it surprising that this patch didn't get any
push-back, even though the thread mentioned the correct way to enforce
consistency (use --amend with --no-edit), and the fact that the user
should have set the commentChar to non-'#' even if his primary method to
create commit messages was with -m. i don't see how (or why) anyone
would integrate this option into any practical workflow, and therefore
consider it a mis-feature that should be done away with. but knowing how
people here react to such proposals, it seems most practical to document
the feature sufficiently well to enable users to easily draw the
conclusion that it is, in fact, nonsense.

Re: [PATCH] docs: clarify meaning of core.commentString=auto

[Junio C Hamano]

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

>>before making a commit.  But "this makes it impossible" does not
>>quite convey that to casual readers.
>>
> no, i meant what i wrote: it makes it _literally_ impossible. it follows
> from the preceding sentence that _whatever_ is in the template will NOT
> be the comment char.

OK, it (i.e. the order in which things happen) would be a good thing
to add to the explanation, to unconfuse readers who (incorrectly)
guess that auto comment character is determined and then template is
read, which is where my comment came from.

> reading through the thread of the original submission, the feature is a
> workaround for `commit -m` and `commit --amend` being inconsistent wrt.
> message washing.

Perhaps somebody can be talked into fixing it ;-)

With a clear explanation, I am OK if somebody wants to advocate to
deprecate (and remove at Git 3.0 boundary) the "auto" support ;-)

Thanks.

Re: [PATCH] docs: clarify meaning of core.commentString=auto

[Oswald Buddenhagen]

On Tue, Mar 18, 2025 at 10:15:15AM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>> no, i meant what i wrote: it makes it _literally_ impossible. it
>> follows
>> from the preceding sentence that _whatever_ is in the template will NOT
>> be the comment char.
>
>OK, it (i.e. the order in which things happen) would be a good thing
>to add to the explanation, to unconfuse readers who (incorrectly)
>guess that auto comment character is determined and then template is
>read, which is where my comment came from.
>
i hoped the formulation "the prepared commit message prior to editing"
would be unambiguous. did i miss anything? if you just made a thinko and
actually agree, then i'd leave the patch as-is, as it doesn't seem worth
expanding _that_ docu any further.

>> reading through the thread of the original submission, the feature is a
>> workaround for `commit -m` and `commit --amend` being inconsistent wrt.
>> message washing.
>
>Perhaps somebody can be talked into fixing it ;-)
>
>With a clear explanation, I am OK if somebody wants to advocate to
>deprecate (and remove at Git 3.0 boundary) the "auto" support ;-)
>
how would we go about this in practice? just a notice in the docu, or
some mechanism which would complain at runtime? under what circumstances
(i.e., how to enable/squelch it)?

Re: [PATCH] docs: clarify meaning of core.commentString=auto

[Phillip Wood]

On 19/03/2025 18:20, Oswald Buddenhagen wrote:
> On Tue, Mar 18, 2025 at 10:15:15AM -0700, Junio C Hamano wrote:
>>> reading through the thread of the original submission, the feature is a
>>> workaround for `commit -m` and `commit --amend` being inconsistent wrt.
>>> message washing.
>>
>> Perhaps somebody can be talked into fixing it ;-)
>>
>> With a clear explanation, I am OK if somebody wants to advocate to
>> deprecate (and remove at Git 3.0 boundary) the "auto" support ;-)

I think that may be best. Looking at the sequencer I don't think 
append_conflicts_hint(), the "fixup" or "squash" commands of "rebase 
-i", or the "--reference" option of "git revert" are compatible with 
core.commentStr=auto. For rebase making it work would mean scanning the 
messages of all the commits to be squash before picking the first one 
which is a pain.

> how would we go about this in practice? just a notice in the docu, or
> some mechanism which would complain at runtime? under what circumstances
> (i.e., how to enable/squelch it)?

I think we'd want to start printing some advice when 
core.commentStr=auto explaining why it has been deprecated and that it 
will be removed when Git 3.0 is released. We should allow that advice to 
be suppressed setting advice.autoCommentStr (other name suggestions 
welcome). We would also want to add an item to BreakingChanges.adoc 
explaining why it is being removed and add "#ifndef 
WITH_BREAKING_CHANGES" around the code that handles core.commentStr=auto 
in builtin/commit.c and guard the documentation with 
"ifdef::with_breaking_changes[]". We may want to make 
core.commentStr=auto an error when breaking changes are enabled as well.

Best Wishes

Phillip

Re: [PATCH] docs: clarify meaning of core.commentString=auto

[Junio C Hamano]

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

> I think we'd want to start printing some advice when
> core.commentStr=auto explaining why it has been deprecated and that it
> will be removed when Git 3.0 is released. We should allow that advice
> to be suppressed setting advice.autoCommentStr (other name suggestions
> welcome). We would also want to add an item to BreakingChanges.adoc
> explaining why it is being removed and add "#ifndef
> WITH_BREAKING_CHANGES" around the code that handles
> core.commentStr=auto in builtin/commit.c and guard the documentation
> with "ifdef::with_breaking_changes[]".

All of these are good action items in a good transition plan, I
would say.

> We may want to make
> core.commentStr=auto an error when breaking changes are enabled as
> well.

I am not so sure.  As commentStr is a random string that is used to
prefix any comment line, "auto" is just a (albeit weird) string, so
not doing anything special would be good enough.