Re: [PATCH v2] builtin/reflog: respect user config in "write" subcommand

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2025, 17:13 UTC
Message-ID: <xmqqplb750f2.fsf@gitster.g>
In-Reply-To: <20250930143741.18331-1-git@lohmann.sh>

git@lohmann.sh writes:

Show 5 quoted lines

> From: Michael Lohmann <git@lohmann.sh>
>
> The reflog write recognizes only GIT_COMMITTER_NAME and
> GIT_COMMITTER_EMAIL environment variables, ignoring the user.name and
> user.email settings from the Git configuration.

Rephrasing ", ignoring" and everything after the sentence to something like

    ..., but forgot to honor the user.name and user.email
    configuration variables, due to lack of repo_config() call to
    grab these values from the configuration files.

would make it more obvious to readers what the right correction would be.

Show 7 quoted lines

> The test suite sets these variables, so this behavior was unnoticed.
>
> Ensure that the reflog write also uses the values of user.name and
> user.email if set in the Git configuration.
>
> Co-authored-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Michael Lohmann <git@lohmann.sh>

This is not in general the right place to make repo_config() call, even though in the current shape of the program it happens to work.

This will start to matter once we start adding command line options to the program. We want the configured values read from the configuration to populate the in-core variables first, and then call parse_options() to allow command line arguments to override them.

In short, move the call above parse_options().

Another thing to consider is if we want to do this inside cmd_reflog(), not here. Once we add another subcommand that also records who did what, other than "write", to the "reflog" command, this starts to matter.

Show 24 quoted lines

>  	ref = argv[0];
>  	if (!is_root_ref(ref) && check_refname_format(ref, 0))
>  		die(_("invalid reference name: %s"), ref);
> diff --git a/t/t1421-reflog-write.sh b/t/t1421-reflog-write.sh
> index 46df64c176..cf0e8608fe 100755
> --- a/t/t1421-reflog-write.sh
> +++ b/t/t1421-reflog-write.sh
> @@ -108,6 +108,25 @@ test_expect_success 'simple writes' '
>  	)
>  '
>  
> +test_expect_success 'uses user.name and user.email config' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit initial &&
> +		COMMIT_OID=$(git rev-parse HEAD) &&
> +
> +		sane_unset GIT_COMMITTER_NAME &&
> +		sane_unset GIT_COMMITTER_EMAIL &&
> +		git config --local user.name "Author" &&
> +		git config --local user.email "a@uth.or" &&
> +		git reflog write refs/heads/something $ZERO_OID $COMMIT_OID first &&

It certainly is good to test _without_ environment. Shouldn't we also make sure that _with_ environment variables, these configured values are overriden with a separate test?

> +		test_reflog_matches . refs/heads/something <<-EOF
> +		$ZERO_OID $COMMIT_OID Author <a@uth.or> 1112911993 -0700	first

This timestamp is from the above "test_commit", presumably? If we later add more tests _before_ this step and they used test_commit, would that screw this test up? It may make sense to say $GIT_COMMITTER_DATE instead of that timestamp here.

Show 9 quoted lines

> +		EOF
> +	)
> +'
> +
>  test_expect_success 'can write to root ref' '
>  	test_when_finished "rm -rf repo" &&
>  	git init repo &&
>
> base-commit: 821f583da6d30a84249f75f33501504d597bc16b

Thanks.