Re: [PATCH 3/3] cat-file: add mailmap support

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 30, 2022, 17:07 UTC
Message-ID: <CAP8UFD2tYzoKSr7HnQ+LbCE+YWoNqo3qPQrbr7ZWAMnDt+ajCg@mail.gmail.com>
In-Reply-To: <61074b4c-c48f-da89-5d03-b280b9c4fedf@gmail.com>

Hi Phillip,

On Thu, Jun 30, 2022 at 5:50 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

Show 7 quoted lines

> On 30/06/2022 15:24, Siddharth Asthana wrote:
> > git cat-file is not a plumbing command anymore, especially as it gained
> > more and more high level features like its `--batch-command` mode.
>
> cat-file is definitely a plumbing command as it is intended to be used
> by scripts. It has a number of features that are used by porcelain
> commands but that does not make cat-file itself porcelain.

Ok, so maybe:

"Even if git cat-file is a plumbing command, it has gained more and more high level features like its `--batch-command` mode."

Show 12 quoted lines

> > So
> > tools do use it to get commit and tag contents that are then displayed
> > to users. This content which has author, committer or tagger
> > information, could benefit from passing through the mailmap mechanism,
> > before being sent or displayed.
> >
> > This patch adds --[no-]use-mailmap command line option to the git
> > cat-file command. It also adds --[no-]mailmap option as an alias to
> > --[no-]use-mailmap.
>
> I don't think we need an alias for this option, it'll just end up
> confusing people.

I am not sure if people would be more confused by the alias or by the fact that the "--[no-]mailmap" alias works for `git log` but not `git cat-file`.

Show 6 quoted lines

> > At this time, this patch only adds a command line
> > option, but perhaps a `cat-file.mailmap` config option could be added as
> > well in the same way as for `git log`.
>
> As cat-file is a plumbing command that is used by scripts we should not
> add a config option for this as it would potentially break those scripts.

Yeah, we could either remove this small paragraph or add the explanation you give.

> I like the idea of adding mailmap support to cat-file and I think this
> patch is definitely going in the right direction.

Thanks!

Show 19 quoted lines

> > +test_expect_success '--no-use-mailmap disables mailmap in cat-file' '
> > +     test_when_finished "rm .mailmap" &&
> > +     cat >.mailmap <<-EOF &&
> > +     A U Thor <author@example.com> Orig <orig@example.com>
> > +     EOF
> > +     cat >expect <<-EOF &&
> > +     author Orig <orig@example.com>
> > +     EOF
> > +     git cat-file --no-use-mailmap commit HEAD >log &&
> > +     grep author log >actual &&
> > +     sed -e "/^author/q" actual >log &&
>
> This line does not have any effect on the contents of log
>
> > +     sed -e "s/ [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$//" log >actual &&
>
> I think you can simplify this series of commands to do
>         git cat-file ... >log
>         sed -n "/^author /s/\([^>]*>\).*/\1/p" log >actual

Thanks for the suggestion!