From: Phillip Wood <phillip.wood123@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: git@vger•kernel.org, Ayush Chandekar <ayu.chandekar@gmail•com>,
Oswald Buddenhagen <oswald.buddenhagen@gmx•de>,
Taylor Blau <me@ttaylorr•com>,
Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail•com>
Subject: Re: [PATCH v2 2/3] config: warn on core.commentString=auto
Date: Fri, 1 Aug 2025 11:37:24 +0100 [thread overview]
Message-ID: <34905f47-5600-4728-8611-4abc166d199f@gmail.com> (raw)
In-Reply-To: <xmqqa54koyb2.fsf@gitster.g>
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
next prev parent reply other threads:[~2025-08-01 10:37 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-07-08 13:56 [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto Phillip Wood
2025-07-08 13:56 ` [PATCH 1/2] breaking-changes: deprecate support for core.commentString=auto Phillip Wood
2025-07-08 15:28 ` Ayush Chandekar
2025-07-09 9:40 ` Phillip Wood
2025-07-08 13:56 ` [PATCH 2/2] commit: print advice when core.commentString=auto Phillip Wood
2025-07-08 18:51 ` [PATCH 0/2] breaking-changes: deprecate support for core.commentChar=auto Junio C Hamano
2025-07-09 9:38 ` Phillip Wood
2025-07-09 16:20 ` Junio C Hamano
2025-07-11 15:09 ` Phillip Wood
2025-07-11 17:07 ` Junio C Hamano
2025-07-12 8:01 ` Oswald Buddenhagen
2025-07-12 14:06 ` Junio C Hamano
2025-07-26 23:15 ` Junio C Hamano
2025-07-27 15:46 ` Phillip Wood
2025-07-09 1:27 ` Junio C Hamano
2025-07-09 1:52 ` Ayush Chandekar
2025-07-09 9:38 ` Phillip Wood
2025-07-31 15:21 ` [PATCH v2 0/3] " Phillip Wood
2025-07-31 15:21 ` [PATCH v2 1/3] breaking-changes: deprecate support for core.commentString=auto Phillip Wood
2025-07-31 20:49 ` Junio C Hamano
2025-07-31 15:21 ` [PATCH v2 2/3] config: warn on core.commentString=auto Phillip Wood
2025-07-31 21:17 ` Junio C Hamano
2025-08-01 10:37 ` Phillip Wood [this message]
2025-08-01 14:36 ` Oswald Buddenhagen
2025-07-31 15:21 ` [PATCH v2 3/3] commit: print advice when core.commentString=auto Phillip Wood
2025-08-01 15:18 ` Oswald Buddenhagen
2025-08-01 17:19 ` Junio C Hamano
2025-08-26 13:33 ` Phillip Wood
2025-08-27 8:19 ` Oswald Buddenhagen
2025-08-27 16:39 ` Junio C Hamano
2025-08-27 22:38 ` Oswald Buddenhagen
2025-08-01 3:50 ` [PATCH v2 0/3] breaking-changes: deprecate support for core.commentChar=auto Junio C Hamano
2025-08-01 10:36 ` Phillip Wood
2025-08-01 16:41 ` Junio C Hamano
2025-08-26 13:35 ` [PATCH v3 " Phillip Wood
2025-08-26 13:35 ` [PATCH v3 1/3] breaking-changes: deprecate support for core.commentString=auto Phillip Wood
2025-08-26 13:35 ` [PATCH v3 2/3] config: warn on core.commentString=auto Phillip Wood
2025-08-26 15:52 ` Junio C Hamano
2025-08-27 15:29 ` Phillip Wood
2025-08-27 18:55 ` Junio C Hamano
2025-08-26 13:35 ` [PATCH v3 3/3] commit: print advice when core.commentString=auto Phillip Wood
2025-08-27 15:27 ` [PATCH v4 0/3] breaking-changes: deprecate support for core.commentChar=auto Phillip Wood
2025-08-27 15:27 ` [PATCH v4 1/3] breaking-changes: deprecate support for core.commentString=auto Phillip Wood
2025-08-27 15:27 ` [PATCH v4 2/3] config: warn on core.commentString=auto Phillip Wood
2025-08-27 15:27 ` [PATCH v4 3/3] commit: print advice when core.commentString=auto Phillip Wood
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=34905f47-5600-4728-8611-4abc166d199f@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=ayu.chandekar@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
--cc=oswald.buddenhagen@gmx$(echo .)de \
--cc=phillip.wood@dunelm$(echo .)org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox