git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Ariadne Conill <ariadne@dereferenced.org>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v1 0/2] document deprecation of log.mailmap=false default
Date: Fri, 12 Jul 2019 13:00:51 -0500	[thread overview]
Message-ID: <CAAOiGNzcNhWmmr+COXhv2p9=KF5k4hHLEsNsfA1H+P0JQOTTqg@mail.gmail.com> (raw)
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:
>
> 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.

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

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

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

> 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

  reply	other threads:[~2019-07-12 18:01 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-12 15:58 [PATCH v1 0/2] document deprecation of log.mailmap=false default Ariadne Conill
2019-07-12 15:59 ` [PATCH v1 1/2] log: add warning for unspecified log.mailmap setting Ariadne Conill
2019-07-12 15:59 ` [PATCH v1 2/2] documentation: mention --no-use-mailmap and log.mailmap false setting Ariadne Conill
2019-07-12 17:49 ` [PATCH v1 0/2] document deprecation of log.mailmap=false default Junio C Hamano
2019-07-12 18:00   ` Ariadne Conill [this message]
2019-07-12 19:30     ` Junio C Hamano

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAOiGNzcNhWmmr+COXhv2p9=KF5k4hHLEsNsfA1H+P0JQOTTqg@mail.gmail.com' \
    --to=ariadne@dereferenced.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).