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

> On Thu, Aug 21, 2008 at 11:19:41AM +0200, Michael J Gruber wrote:
> 
>> 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).

>> --- 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 <mickey@ducktown.us>" 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?

>> 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".

> 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.
>> 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. ;)
> 
>> --- 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.
> 
>> 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"
> 
>> +				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

> 
>> +	ALLOC_GROW(users, users_nr + 1, users_alloc);
> 
> Yay, a first-time submitter bothered to use ALLOC_GROW! :)
> 
>> +	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.