Re: [PATCH] allow user aliases for the --author parameter

First of all: Thanks for all your responses. I think I've learned a lot
through them, and hopefully I'll be able to give evidence with an
upcoming patch...

Jeff King venit, vidit, dixit 21.08.2008 22:02:

On Thu, Aug 21, 2008 at 11:19:41AM +0200, Michael J Gruber wrote:

quoted

This allows the use of author abbreviations when specifying commit 
authors via the --author option to git commit. "--author=$key" is 
resolved by looking up "user.$key.name" and "user.$key.email" in
the config.

This seems like a reasonable feature to me, though two high-level 
questions:

- Is it worth supporting external alias sources, as Alex mentioned? I
 think that would make more sense for many people. Even if you are 
not personally interested in writing it, it would be nice to keep it 
in mind as a future expansion when doing this work. For example, 
maybe it makes more sense for the config to point to a (type, file) 
pair instead of placing directly into the config. Or maybe this 
should just live in conjunction with that feature, if somebody cares 
to implement it.

- Is user.$key the right namespace? It precludes a few particular 
aliases, and it might clash with future user.* config. Perhaps 
user.alias.* would be a better place (or, as above, just referencing 
an external file).

- git-send-email already looks at some alias files. Maybe this is an 
opportunity to refactor and centralize (although perhaps it is not 
worth the effort, because of the different implementation languages).

There's also git svn.
I think all of these serve different purposes, and have different
typical numbers of entries.

- mailmap maps email addresses to full names, for display purposes only.
Typically a long list.

- git svn's author file maps usernames to fullname <email>. But for
every svn repo I need a file following their chosen keys (usernames),
rather than abbreviations I would remember.

- alias files for send-email map keys to fullname <email>. That indeed
is a mapping and a purpose similar to my intention for git commit
--author. Problem here is that it's in perl and supports various
different formats.

I think for send-email you would typically use your mua's alias file.

For git commit --author abbreviations at least I would typically need
only very few entries (be it per repo or globally), which means they can
be much shorter (than my mua aliases) in order to be unique, and I don't
really want an extra file for that.

So, while in fact I wouldn't have been able to implement it differently
anyways, there are other good reasons as well. :)

quoted

--- In an ideal word, all my collaborators would exchange changes

as git patches (or even via pull/push). In the real world, they
send new versions which I integrate (after dealing with their
whitespace and encoding changes...). Therefore, being able to say 
"git commit --author=mickey" and having git translate "mickey" into
"Mickey Mouse [off-list ref]" is a real time saver. The patch
accomplishes this by reading config keys "user.mickey.name" and
"user.mickey.email" when encountering an --author argument without
"<>".

This justification should probably go into the commit message, not
the cover letter. When you are writing it, think about the reader who
will bisect or blame to your commit a year from now. Will they want
to see just _what_ you did, or _why_ you did it?

OK. I think I'm still thinking in terms of "change log style" commit
messages. I haven't completely switched from svn to git yet, neither
technically nor intellectually, it seems. Read "brain rotten" ;)

quoted

If there's interest in this patch I'll follow up with a
documentation patch.

I think Miklos already yelled at you for this. The message he
referenced doesn't quite apply, because you did include some
discussion of the "why".

He didn't mean to yell, we corresponded off-list, all is well.

[snip]

final patch" then that would have gone over very well. But as it 
happens, you chose the magic pet peeve words. ;)

Newcomer's luck, I'm fine with that.

quoted

The "--committer" argument to git commit is not treated because I
don't consider it worthwhile.

I managed to fool everyone, including myself. There is no --committer
option. I feel in good company now ;)

There is GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL, and likewise for
author. My patch does not use any of these, it only deals with (the)
option argument(s). Explicitely set *_{NAME,EMAIL} should be respected
as is.

If you are introducing a new source of alias mappings, it would make 
sense to me to support it everywhere for the sake of consistency.
That means --committer should look at it, too (and should only be a
few lines, I would think), and probably git-send-email.

quoted

P.S.: That's my first patch here. Yes, I've read
Doc/SubmittingPatches. So, if something's wrong, please be gentle
but not overly so ;)

I hope this is the right amount of gentleness. ;)

quoted

--- a/builtin-commit.c +++ b/builtin-commit.c @@ -53,6 +53,12 @@

static char *author_name, *author_email, *author_date; static int
all, edit_flag, also, interactive, only, amend, signoff; static int
quiet, verbose, no_verify, allow_empty; static char
*untracked_files_arg; +struct user { +	char *name, *full_name,
*email; +};

Others may disagree, but style-wise I think we usually put each
struct member on its own line.

quoted

if (force_author) { const char *lb = strstr(force_author, " <"); 
const char *rb = strchr(force_author, '>');

+		if (!lb && !rb) { +			for (i=0; i < users_nr; i++) {

Style: "i = 0"

quoted

+				if (!strcmp(force_author, users[i]->name)) { +					author_name
= users[i]->full_name; +					author_email = users[i]->email; +
return; +				}

I haven't traced all of the uses of author_name and author_email, but
 all of the other codepaths seem to allocate a new string, whereas

..because they need to make a local (for the function) string global
(for the file)...

this uses the existing strings.

...because they are (file) global already.

Is this going to accidentally free()
from the users list, or are we just leaking those other strings now?

Same as branches in remote.c, see below. They're not freed accidentally
in builtin-commit.c

quoted

+	ALLOC_GROW(users, users_nr + 1, users_alloc);

Yay, a first-time submitter bothered to use ALLOC_GROW! :)

quoted

+	ret = xcalloc(1, sizeof(struct user)); +	users[users_nr++] = ret;
 +	if (len) +		ret->name = xstrndup(name, len); +	else +		ret->name
= xstrdup(name); + +	return ret; +}

This is the not the most git-ish way of using the config[1]. Usually
we avoid reading big lists into memory, but rather just call
git_config with the appropriate callback when we find we need to look
up the user alias.

[1] However, I don't necessarily agree with this. We can end up
parsing the config (which may be split across 3 files) several times
per command, so it is probably better to just parse and store it in
one go. So I will let Junio comment on the preferred method.

I was looking all over the existing code for a function which would do
what "git config --get $key" does, and didn't find any. I ended up
copying the logic (and code) from remote.c's parsing of "branch.*.*".
[Should I have attributed this somehow? ]

I understand there are good reasons for this (the way the config is
parsed): a generic central config parser wouldn't be able to verify the
entries when reading the config.
OTOH, verifying an entry when using it wouldn't be that much later. So,
reading the complete config once and storing it in a global struct
should be an alternative which would provide a central place for all
parsing. Judging the implications is way above my current understanding
of the codebase, not to mention implementing it.

Cheers
Michael

P.S.: I should have split this up. Next post will be shorter.