Re: [PATCH] commit: search author pattern against mailmap

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2013, 05:27 UTC
Message-ID: <xmqq1u5hkomf.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20130825165153.GC21092@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

> Exactly. Sample (largely untested) patch is below if you want to use it
> as a starting point. There are probably a few additional cleanups on top
> (e.g., "git log" understands "--mailmap", which should probably be
> centralized to handle_revision_opt).
>
> I'm on the fence. It doesn't actually save that many lines of code, and
> I guess it's possible that somebody would want a custom mailmap in the
> future. Even though you can't do it right now, all it would take is
> exposing read_mailmap_file and read_mailmap_blob outside of mailmap.c.
> Of course, it would be easy to expose map_user_from at the same time.

I am of two minds on this, but if I were forced to pick one _today_, I would have to say that I am moderately negative to the approach.

Having to always specify that you want to use mailmap and make sure you read it is a bit cumbersome from callers' point of view, and using a singleton global may be one attractive way to do so.

It however regresses the "you can choose which mailmap to apply" structure we already have, it would make things less libifiable, and will make it harder to allow a single Git process work on two or more independent repositories (yes, we would need to restructure the object API to allow us to manage multiple object stores, the ref API, etc. in a way similar to how we weaned ourselves away from the single "active_cache" abstraction in the index API). I am personally OK to declare that we should _never_ touch more than one repository in a single process, but submodule support already does this to some extent, so...

I think it is a reasonable tentative solution to hook a singleton instance to something that is commonly used, e.g. the rev_info structure, for large subset of commands that do use the structure chosen to host that singleton instance, but those that do not work based on revision traversal (e.g. "grep") need to also honor mailmap consistently, so we must keep the lower level API that takes an explicit mailmap instance for them anyway.

So...