Hi Junio

On 31/07/2025 22:17, Junio C Hamano wrote:

Phillip Wood [off-list ref] writes:

quoted

  #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

quoted

+static const char* comment_key_name(unsigned id)

The asterisk sticks to the identifier, not type.

Oops I'll fix that

quoted

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

quoted

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

quoted

+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