Re: [PATCH v1 0/2] document deprecation of log.mailmap=false default

From: Ariadne Conill <ariadne@dereferenced.org>
Date: Jul 12, 2019, 18:00 UTC
Message-ID: <CAAOiGNzcNhWmmr+COXhv2p9=KF5k4hHLEsNsfA1H+P0JQOTTqg@mail.gmail.com>
In-Reply-To: <xmqqpnmfp2gs.fsf@gitster-ct.c.googlers.com>

Hello,

On Fri, Jul 12, 2019 at 12:50 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> Ariadne Conill <ariadne@dereferenced.org> writes:
>
> > Based on the discussion of the previous patches, we concluded that
> > changing the default will require a transitional period.
>
> OK.
>
> > As such, I have introduced a deprecation warning that is printed when
> > the log builtin commands are used.
>
> ... used when the user did not configure and did not give --[no-]use-mailmap
> option from the command line, right?  Otherwise the warning would be annoying.

Yes, the default state is changed to -1, which using the command line options or explicitly configuring the option would change to either 0 or 1.

> Have you run the test suite before posting?  To me, at least t0203,
> t4212, t7006 and t9700 seem to be having trouble with this change,
> that may need adjusting.

I ran parts of the testsuite, but I can go through the whole thing and will fix the fallout.

Show 29 quoted lines

> 0203.1 expects that "git show" gives nothing to its standard error
> stream.  Perhaps something like this should serve as a good
> workaround.
>
> diff --git a/t/t0203-gettext-setlocale-sanity.sh b/t/t0203-gettext-setlocale-sanity.sh
> index 0ce1f22eff..76f774c8b8 100755
> --- a/t/t0203-gettext-setlocale-sanity.sh
> +++ b/t/t0203-gettext-setlocale-sanity.sh
> @@ -10,7 +10,7 @@ test_description="The Git C functions aren't broken by setlocale(3)"
>  test_expect_success 'git show a ISO-8859-1 commit under C locale' '
>         . "$TEST_DIRECTORY"/t3901/8859-1.txt &&
>         test_commit "iso-c-commit" iso-under-c &&
> -       git show >out 2>err &&
> +       git show --use-mailmap >out 2>err &&
>         test_must_be_empty err &&
>         grep -q "iso-c-commit" out
>  '
>
>
> 4212.3 is the same way.
>
> 4212.4 raises an interesting question.  It wants to see the output
> of this command:
>
>         git log --format="%an+%ae+%ad" broken_email >actual.out 2>actual.err &&
>
> The question is, when the user explicitly asked for the "true"
> identity values (not the mapped one via %aN, %aE and their friends),
> if --use-mailmap should affect the outcome?

I don't think it should.

Show 5 quoted lines

> A secondary question is if we should be issuing a warning against
> this command line in the first place, if the answer to the above
> question is "no" (i.e. --[no-]use-mailmap, and the future default
> switch, do not affect the outcome).  There is no point issuing a
> warning if the command line is already future-proofed.

I will disable the warning if a custom format is specified.

Show 10 quoted lines

> 7006.43 (among others) expects its "colorful" shell helper function
> to be a reliable way to detect if color escape sequence is in use in
> the output but the helper only reads the first line and expects it
> to always begin with the colored "commit 0c9f6db7183a...", which fails
> when an error or a warning message comes first.  I think the best fix
> there should be to make the helper more robust, perhaps by borrowing
> ideas from test-lib-functions.sh::test_decode_color, i.e. find byte
> sequence "\033[m" in there (I think assuming the presence of RESET
> sequence alone is sufficient for the purpose of t7006 tests that
> wants to see "have we tried to color or emit non-colored output?".

The colorful shell helper function irritates me from time to time with that sort of thing. I might look into fixing it, but I think for now it makes more sense to just defang those tests by killing the warning.

A thought I had was to perhaps use isatty(STDERR_FILENO) to determine if stderr is attached to a terminal session and only issue the warning in that case. What do you think? It should fix all of these tests.

Show 15 quoted lines

> A possible workaround for 9007 would be to set log.mailmap=true in
> the set-up step, but there may be a better alternative.
>
> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh
> index 102c133112..a7a336947d 100755
> --- a/t/t9700-perl-git.sh
> +++ b/t/t9700-perl-git.sh
> @@ -36,6 +36,7 @@ test_expect_success \
>       echo "changed file 1" > file1 &&
>       git commit -a -m "second commit" &&
>
> +     git config log.mailmap true &&
>       git config --add color.test.slot1 green &&
>       git config --add test.string value &&
>       git config --add test.dupstring value1 &&

This seems reasonable.

> There may be issues with other tests.  I didn't do any exhaustive
> vetting.

Thanks wholeheartedly for all of your help so far, I know there are many git users who will be very happy once all of this work is done (basically anyone who has changed their name).

Ariadne