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

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 30, 2025, 11:26 UTC
Message-ID: <aNu-cTrJwPdcW6hq@pks.im>
In-Reply-To: <20250930091411.14179-1-gitmlko@not-evil.de>

On Tue, Sep 30, 2025 at 11:14:11AM +0200, gitmlko@not-evil.de wrote:

> From: Michael Lohmann <git@lohmann.sh>

Is there any specific reason why the originating mail address and the author information disagree with one another? It makes it hard for us to verify that these mail addresses actually map to the same person.

> Previously, the reflog write command only recognized the environment
> variables GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL. It did not account
> for user.name and user.email settings from the Git configuration.

Nit: we typically write our commit messages in the following style:

  - We describe the problem in the present tense.

  - We then proceed to describe why that problem exists. In our context,
    it's also relevant to explain why our tests didn't catch the issue.

  - Finally, we use imperative mood to say how the problem is fixed, as
    if instructing the code to change.

Happy to provide more guidance here as needed.

Show 5 quoted lines

> Since the test suite always sets these variables, it was unnoticed that
> not present, it would use the system config as defaults instead of the
> git config.
> 
> Co-Authored-By: Patrick Steinhardt <ps@pks.im>

Another tiny nit: This should be "Co-authored-by", with everything except the leading "C" being lower-case.

Show 24 quoted lines

> diff --git a/t/t1421-reflog-write.sh b/t/t1421-reflog-write.sh
> index 46df64c176..65fc4cae93 100755
> --- a/t/t1421-reflog-write.sh
> +++ b/t/t1421-reflog-write.sh
> @@ -101,10 +101,19 @@ test_expect_success 'simple writes' '
>  		EOF
>  
>  		git reflog write refs/heads/something $COMMIT_OID $COMMIT_OID second &&
> -		test_reflog_matches . refs/heads/something <<-EOF
> +		test_reflog_matches . refs/heads/something <<-EOF &&
>  		$ZERO_OID $COMMIT_OID $SIGNATURE	first
>  		$COMMIT_OID $COMMIT_OID $SIGNATURE	second
>  		EOF
> +
> +		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_new $ZERO_OID $COMMIT_OID first &&
> +		test_reflog_matches . refs/heads/something_new <<-EOF
> +		$ZERO_OID $COMMIT_OID Author <a@uth.or> 1112911993 -0700	first
> +		EOF
>  	)
>  '

I think it would be preferable to make this a separate test case.

Thanks for working on this!

Patrick