[PATCH 0/3] Add support for git's mailmap.

[PATCH 0/3] Add support for git's mailmap.

[smithj4]

Rebased patch to resolve some minor conflicts with the master branch.

Jason A. Smith (1):
   Add support for git's mailmap.

John Keeping (2):
   filter: introduce cgit_open_email_filter() wrapper
   Remove angle brackets from {author,committer}_email

  cgit.h      |  5 +++++
  filter.c    | 13 +++++++++++++
  parsing.c   | 27 ++++++++++++++++++++++-----
  ui-atom.c   | 31 ++++++++++++++-----------------
  ui-commit.c | 28 ++++++++++++++++++++--------
  ui-log.c    | 11 +++++++++--
  ui-refs.c   | 32 ++++++++++++++++++++++++++------
  ui-stats.c  | 13 ++++++++++---
  ui-tag.c    | 17 ++++++++++++-----
  9 files changed, 131 insertions(+), 46 deletions(-)

[PATCH 0/3] Add support for git's mailmap.

[smithj4]

Seeing a lot of recent activity I checked the upstream and see that the 
patch I first submitted over a year ago has still not been merged. I 
already had to rebase it once several months ago to fix conflicts, I 
don't want to have to do that again. Is there something wrong with this 
patch? Please let me know so I can fix it.

Thanks,
~Jason


On 02/24/2017 11:18 AM, Jason A. Smith wrote:
> 
> Rebased patch to resolve some minor conflicts with the master branch.
> 
> Jason A. Smith (1):
>  ? Add support for git's mailmap.
> 
> John Keeping (2):
>  ? filter: introduce cgit_open_email_filter() wrapper
>  ? Remove angle brackets from {author,committer}_email
> 
>  ?cgit.h????? |? 5 +++++
>  ?filter.c??? | 13 +++++++++++++
>  ?parsing.c?? | 27 ++++++++++++++++++++++-----
>  ?ui-atom.c?? | 31 ++++++++++++++-----------------
>  ?ui-commit.c | 28 ++++++++++++++++++++--------
>  ?ui-log.c??? | 11 +++++++++--
>  ?ui-refs.c?? | 32 ++++++++++++++++++++++++++------
>  ?ui-stats.c? | 13 ++++++++++---
>  ?ui-tag.c??? | 17 ++++++++++++-----
>  ?9 files changed, 131 insertions(+), 46 deletions(-)
> 

[PATCH 0/3] Add support for git's mailmap.

[john]

On Tue, Oct 24, 2017 at 10:55:21AM -0400, Jason A. Smith wrote:
> Seeing a lot of recent activity I checked the upstream and see that the 
> patch I first submitted over a year ago has still not been merged. I 
> already had to rebase it once several months ago to fix conflicts, I 
> don't want to have to do that again. Is there something wrong with this 
> patch? Please let me know so I can fix it.

I've taken another look at this patch and one thing that stands out is
that we end up inserting code to map users in a lot of different places.
I haven't audited the code, but after the final patch, is there anywhere
that calls cgit_parse_commit() and doesn't go on to map the users?
(Even if not all callers do map the user, can we push cgit_map_user()
into cgit_parse_commit() and use a flag parameter to enable it?)

But I think that points to a deeper philosophical question of whether we
should be mapping the user in all cases (and I suspect this is why the
patch has sat on the list for so long - there are arguments in both
directions).

As I understand it, mailmap was originally introduced so that
git-shortlog would coalesce commits from the same author but with
different email addresses.  When using the git command, mailmap is
enabled by default for git-shortlog and git-blame but not for git-log,
git-show, and other commands that print commits.

Extending this to CGit, it seems that there's a clear argument for
enabling mailmap in ui-stats and possibly the new ui-blame, but when we
print a commit in ui-commit or ui-log is it always correct to apply the
mailmap or should we show the commit headers as-is?

It would be interesting to know what others on the list think about
this.  If there isn't a consensus then we may want a new config option
to allow this to be enabled according to the preferences of individual
sites.

[PATCH 0/3] Add support for git's mailmap.

[smithj4]

Hi John,

I always thought that by going through the trouble of collecting the 
duplicate email addresses and creating the .mailmap file that you wanted 
this coalescing behavior.

Personally, I don't really want to have to remember everyone's old email 
address and variation of their name that they might have used several 
years ago to push a commit. I don't really care what email & name they 
used a long time ago, I am more interested in just knowing who made the 
commit, which is why the .mailmap coalesces all of their old identities 
into their current one for display.

If people don't need or want this coalescing behavior and want to see 
each individual identity then they don't need to make the .mailmap file 
to begin with.

Just my opinion on .mailmap.

~Jason


On 10/28/2017 07:36 AM, John Keeping wrote:
> On Tue, Oct 24, 2017 at 10:55:21AM -0400, Jason A. Smith wrote:
>> Seeing a lot of recent activity I checked the upstream and see that the
>> patch I first submitted over a year ago has still not been merged. I
>> already had to rebase it once several months ago to fix conflicts, I
>> don't want to have to do that again. Is there something wrong with this
>> patch? Please let me know so I can fix it.
> 
> I've taken another look at this patch and one thing that stands out is
> that we end up inserting code to map users in a lot of different places.
> I haven't audited the code, but after the final patch, is there anywhere
> that calls cgit_parse_commit() and doesn't go on to map the users?
> (Even if not all callers do map the user, can we push cgit_map_user()
> into cgit_parse_commit() and use a flag parameter to enable it?)
> 
> But I think that points to a deeper philosophical question of whether we
> should be mapping the user in all cases (and I suspect this is why the
> patch has sat on the list for so long - there are arguments in both
> directions).
> 
> As I understand it, mailmap was originally introduced so that
> git-shortlog would coalesce commits from the same author but with
> different email addresses.  When using the git command, mailmap is
> enabled by default for git-shortlog and git-blame but not for git-log,
> git-show, and other commands that print commits.
> 
> Extending this to CGit, it seems that there's a clear argument for
> enabling mailmap in ui-stats and possibly the new ui-blame, but when we
> print a commit in ui-commit or ui-log is it always correct to apply the
> mailmap or should we show the commit headers as-is?
> 
> It would be interesting to know what others on the list think about
> this.  If there isn't a consensus then we may want a new config option
> to allow this to be enabled according to the preferences of individual
> sites.
> 

[PATCH 0/3] Add support for git's mailmap.

[i]

Ack and for edge cases a config entry ignore_mailmap = True

On 02.11.2017 21:48, Jason A. Smith wrote:
> If people don't need or want this coalescing behavior and want to see
> each individual identity then they don't need to make the .mailmap file
> to begin with.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20171102/9cccb9fa/attachment.asc>

[PATCH 0/3] Add support for git's mailmap.

[john]

On Thu, Nov 02, 2017 at 04:48:08PM -0400, Jason A. Smith wrote:
> I always thought that by going through the trouble of collecting the 
> duplicate email addresses and creating the .mailmap file that you wanted 
> this coalescing behavior.
> 
> Personally, I don't really want to have to remember everyone's old email 
> address and variation of their name that they might have used several 
> years ago to push a commit. I don't really care what email & name they 
> used a long time ago, I am more interested in just knowing who made the 
> commit, which is why the .mailmap coalesces all of their old identities 
> into their current one for display.
> 
> If people don't need or want this coalescing behavior and want to see 
> each individual identity then they don't need to make the .mailmap file 
> to begin with.

Consider someone working on an open source project who changes employer;
in this case .mailmap allows email to be directed to an address at the
new employer rather than at the previous employer, especially for
auto-generated CC lists from something like Linux's get_maintainer.pl
script.

However, when looking at historic commits, it may be very interesting
that a particular change was committed by someone with an email address
at a cerain company.  Blindly applying .mailmap rules loses that
information (admittedly, in the case of the kernel, DCO rules mean the
relevant information also lives in the S-o-b but that doesn't apply
everywhere).

On the other hand, it might be that some early commits by a developer in
a project use the default "user@(none)" fallback address and .mailmap is
used to change that to a genuine email address.  In that case applying
.mailmap in all cases is desirable.

I don't think we can take a global decision on which behaviour is
correct for all projects, so IMHO there must be a configuration switch
to allow projects to decide what is right for them.

> On 10/28/2017 07:36 AM, John Keeping wrote:
> > On Tue, Oct 24, 2017 at 10:55:21AM -0400, Jason A. Smith wrote:
> >> Seeing a lot of recent activity I checked the upstream and see that the
> >> patch I first submitted over a year ago has still not been merged. I
> >> already had to rebase it once several months ago to fix conflicts, I
> >> don't want to have to do that again. Is there something wrong with this
> >> patch? Please let me know so I can fix it.
> > 
> > I've taken another look at this patch and one thing that stands out is
> > that we end up inserting code to map users in a lot of different places.
> > I haven't audited the code, but after the final patch, is there anywhere
> > that calls cgit_parse_commit() and doesn't go on to map the users?
> > (Even if not all callers do map the user, can we push cgit_map_user()
> > into cgit_parse_commit() and use a flag parameter to enable it?)
> > 
> > But I think that points to a deeper philosophical question of whether we
> > should be mapping the user in all cases (and I suspect this is why the
> > patch has sat on the list for so long - there are arguments in both
> > directions).
> > 
> > As I understand it, mailmap was originally introduced so that
> > git-shortlog would coalesce commits from the same author but with
> > different email addresses.  When using the git command, mailmap is
> > enabled by default for git-shortlog and git-blame but not for git-log,
> > git-show, and other commands that print commits.
> > 
> > Extending this to CGit, it seems that there's a clear argument for
> > enabling mailmap in ui-stats and possibly the new ui-blame, but when we
> > print a commit in ui-commit or ui-log is it always correct to apply the
> > mailmap or should we show the commit headers as-is?
> > 
> > It would be interesting to know what others on the list think about
> > this.  If there isn't a consensus then we may want a new config option
> > to allow this to be enabled according to the preferences of individual
> > sites.

[PATCH 0/3] Add support for git's mailmap.

[smithj4]

Sorry, I was using imap-send and used attachments to keep my
thunderbird mail client from line-wrapping the patches.

Jason A. Smith (1):
  Add support for git's mailmap.

John Keeping (2):
  filter: introduce cgit_open_email_filter() wrapper
  Remove angle brackets from {author,committer}_email

 cgit.h      |  5 +++++
 filter.c    | 13 +++++++++++++
 parsing.c   | 27 ++++++++++++++++++++++-----
 ui-atom.c   | 31 ++++++++++++++-----------------
 ui-commit.c | 28 ++++++++++++++++++++--------
 ui-log.c    | 11 +++++++++--
 ui-refs.c   | 32 ++++++++++++++++++++++++++------
 ui-stats.c  | 13 ++++++++++---
 ui-tag.c    | 17 ++++++++++++-----
 9 files changed, 131 insertions(+), 46 deletions(-)

-- 
2.9.3