Hello, On Thu, Jul 11, 2019 at 12:09 PM Martin Ågren <martin.agren@xxxxxxxxx> wrote: > > Hi Ariadne, > > Welcome to the list! Thanks! > On Thu, 11 Jul 2019 at 18:39, Ariadne Conill <ariadne@xxxxxxxxxxxxxxxx> wrote: > > > > Hello, > > > > On Thu, Jul 11, 2019 at 10:19 AM brian m. carlson > > <sandals@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > On 2019-07-11 at 08:19:58, Ariadne Conill wrote: > > > > The `git log` command shows the author and committer name recorded in > > > > the git repository itself, while other commands respect `.mailmap` > > > > by default. I believe this is a bad design: it causes log entries to > > > > reflect inaccurate information: anyone who changes their name or > > > > e-mail address will not have that change (recorded in mailmap file) > > > > reflected when using `git log` by default. > > > > > > > > Anyone who explicitly wants the current behaviour can clearly request > > > > it by setting the `log.mailmap` setting to `false` in their > > > > `.gitconfig` file. > > It would be useful with some tests for this. That would be a nice way to > showcase how this is meant to work, and to protect this from being > broken in the future. From looking around in t/, it looks like > t4203-mailmap.sh could be a good spot. I agree. I added a regression test to t4203-mailmap.sh in my second go around, already. > > > While I'm in favor of using the mailmap by default, typically we want a > > > way people can override a default setting from the command line. > > > > > > So in this case, we have "--use-mailmap", but we don't have a > > > "--no-use-mailmap" (at least, it's not documented in the manpage). I > > > think we'd want to add such an option so that people can set it if they > > > need non-default behavior. > > > > I agree that there is probably some useful reasons to have this > > option, so I can add an option that forces mailmap usage off to > > supplement the --use-mailmap option. It's no problem. I will > > generate a new patch series in a few minutes. > > There are some tests already in t4203 which test that for example `git > log --use-mailmap ...` does the right thing. If your implementation is > correct, it should be possible to drop `--use-mailmap` from there and > everything should work. It would then be very useful to have tests that > `--no-use-mailmap` does the right thing. There's also some testing > around the `log.mailmap` config, which you should probably adapt and > possibly extend. Alright, I'll adapt the tests as proposed. I agree with your conclusion that dropping `--use-mailmap` should be perfectly safe and not cause any regressions in the testsuite. I already did add a regression test for `--no-use-mailmap` which tests it with `-c log.mailmap=True`. > The documentation for `log.mailmap` will need to be updated, since > right now it says (or at least implies) that the default is "false". Yeah, I just noticed the documentation needs to be updated for this change. That will be resolved in v3, as well. > This looks like one of those very small and simple changes that turn > into quite some work to ensure that the tests are all there and that > the documentation is up to date. :-) Yeah, this is definitely more docs and test oriented I think. Thanks for the review. Ariadne
