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

Michael J Gruber [off-list ref] writes:

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.

Maybe it is just me, but I am hesitant about the contamination of user.*
configuration namespace.  This patch as a general solution does not scale
well, once you start working with more than a few dozen people.

Why was it insufficient to use an external shortname-to-fullname mapping
file like git-svn and git-cvsimport does, again?

On Tue, Aug 26, 2008 at 04:31:30PM -0700, Junio C Hamano 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.

Maybe it is just me, but I am hesitant about the contamination of user.*
configuration namespace.  This patch as a general solution does not scale
well, once you start working with more than a few dozen people.

It is not just you. I think this version of the patch is much improved,
but I am still against user.$key.*. At the very least, it needs its own
namespace.

I think if somebody cares, reading external files of various formats
would be nice (and a simple "alias, space, expansion, newline" format
could be introduced), but since I am not volunteering to implement that,
this even simpler implementation is acceptable to me, as long as it is
user.alias.$key.* or similar.

-Peff

Jeff King [off-list ref] writes:

On Tue, Aug 26, 2008 at 04:31:30PM -0700, Junio C Hamano wrote:

quoted

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.

Maybe it is just me, but I am hesitant about the contamination of user.*
configuration namespace.  This patch as a general solution does not scale
well, once you start working with more than a few dozen people.

It is not just you. I think this version of the patch is much improved,
but I am still against user.$key.*. At the very least, it needs its own
namespace.

It's not just that.  Having many of these in .git/config will slow down
any unrelated thing that needs to read from config.

I am not married to the "reuse existing information" idea, but doing it
the way this sample patch does at least makes only people who uses this
feature to pay the price and only when they use it.

Not extensively tested, beyond the usual test suite, and using it for real
only once to commit this with "git commit --author=Jeff".  I wanted to say
"Michael J" instead, but there is this little chicken-and-egg problem ;-)

 builtin-commit.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git c/builtin-commit.c w/builtin-commit.c
index 649c8be..8aae906 100644
--- c/builtin-commit.c
+++ w/builtin-commit.c

@@ -710,6 +710,30 @@ static int message_is_empty(struct strbuf *sb, int start)
 	return 1;
 }
 
+static const char *find_author_by_nickname(const char *name)
+{
+	struct rev_info revs;
+	struct commit *commit;
+	struct strbuf buf = STRBUF_INIT;
+	const char *av[20];
+	int ac = 0;
+
+	init_revisions(&revs, NULL);
+	strbuf_addf(&buf, "--author=%s", name);
+	av[++ac] = "--all";
+	av[++ac] = buf.buf;
+	av[++ac] = NULL;
+	setup_revisions(ac, av, &revs, NULL);
+	prepare_revision_walk(&revs);
+	commit = get_revision(&revs);
+	if (commit) {
+		strbuf_release(&buf);
+		format_commit_message(commit, "%an <%ae>", &buf);
+		return strbuf_detach(&buf, NULL);
+	}
+	die("No existing author found with '%s'", name);
+}
+
 static int parse_and_validate_options(int argc, const char *argv[],
 				      const char * const usage[],
 				      const char *prefix)

@@ -720,6 +744,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	logfile = parse_options_fix_filename(prefix, logfile);
 	template_file = parse_options_fix_filename(prefix, template_file);
 
+	if (force_author && !strchr(force_author, '>'))
+		force_author = find_author_by_nickname(force_author);
+
 	if (logfile || message.len || use_message)
 		use_editor = 0;
 	if (edit_flag)

Junio C Hamano venit, vidit, dixit 27.08.2008 08:13:

Jeff King [off-list ref] writes:

quoted

On Tue, Aug 26, 2008 at 04:31:30PM -0700, Junio C Hamano wrote:

quoted

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.

Maybe it is just me, but I am hesitant about the contamination of user.*
configuration namespace.  This patch as a general solution does not scale
well, once you start working with more than a few dozen people.

It is not just you. I think this version of the patch is much improved,
but I am still against user.$key.*. At the very least, it needs its own
namespace.

It's not just that.  Having many of these in .git/config will slow down
any unrelated thing that needs to read from config.

I don't see a namespace problem as long as nobody uses "name" or "email"
as $key. That said I'd suggest useralias.$key.{name,email} then which
gives a cleaner separation and leaves the possibility to

- use the alias for other cases than --author
- use other fields than name, email

at a later time.

I am not married to the "reuse existing information" idea, but doing it
the way this sample patch does at least makes only people who uses this
feature to pay the price and only when they use it.

People who don't use this feature don't have any entries and don't pay
anything.
People who use this feature and have a moderate number of entries don't
pay a recognizable price.
People who use this feature and have a vast amount of entries should be
told to implement an alias file parser ;)

Not extensively tested, beyond the usual test suite, and using it for real
only once to commit this with "git commit --author=Jeff".  I wanted to say
"Michael J" instead, but there is this little chicken-and-egg problem ;-)

[patch snipped]

I'd be happy with that approach as well for my use case. In general it
may suffer from the uniqueness problem: If there's a recent commit
authored by "Michael@Jeff.com" your "--author=Jeff" will resolve
differently from yesterday, and you won't even notice (not even commit
-v tells you). [ A typo is punished by a search through all commits;
that's fine.]

But I won't compete with an alternative patch from The Man, of course ;)

+	die("No existing author found with '%s'", name);
Minor suggestion:
"...or malformed --author parameter"
I foresee questions like "Huh? What does it mean not existing" when
people don't get the A U Thor [off-list ref] format right.

Michael

On Tue, Aug 26, 2008 at 11:13:13PM -0700, Junio C Hamano wrote:

quoted

It is not just you. I think this version of the patch is much improved,
but I am still against user.$key.*. At the very least, it needs its own
namespace.

It's not just that.  Having many of these in .git/config will slow down
any unrelated thing that needs to read from config.

Sure, it can, but so can putting a lot of branch info in your config. My
thinking was that this covers the "I just want to put in a few entries
easily" use case. If somebody wants to do something _big_, then that is
time for the external format.

But then we have two formats which we must support forever, which is
maybe a bad thing.

I am not married to the "reuse existing information" idea, but doing it
the way this sample patch does at least makes only people who uses this
feature to pay the price and only when they use it.

Actually, I like this quite a bit. Almost by definition, the information
is already here (and if it isn't, it is because it is the first time
this person is an author, so you would have to end up typing it once
_anyway_).

My only complaint is:

+	strbuf_addf(&buf, "--author=%s", name);
+	av[++ac] = "--all";
+	av[++ac] = buf.buf;
+	av[++ac] = NULL;

I am too lazy to hit "shift", so I would use "-i".

-Peff

[resend, copy git list. Gah, Michael there is something about your
messages that causes me to keep dropping the git list when I reply. It
looks like maybe you send one message to the author without git@vger
cc'd, and then you send a different one 'to' the git list without the
original 'from' in the cc?]

On Wed, Aug 27, 2008 at 11:36:55AM +0200, Michael J Gruber wrote:

I don't see a namespace problem as long as nobody uses "name" or "email"
as $key.

It also ties our hands for putting more things in user.* later, since
now we will hurt users who have put their arbitrary aliases in user.*
(and who will rightly complain when we break their config).

That said I'd suggest useralias.$key.{name,email} then which gives a
cleaner separation and leaves the possibility to

I would be fine with that. Though I do think Junio's "automatic" version
is even nicer.

- use the alias for other cases than --author - use other fields than
name, email

I think the big user would be send-email; I don't know if that will ever
get converted to C, though.

People who don't use this feature don't have any entries and don't pay
anything.
People who use this feature and have a moderate number of entries don't
pay a recognizable price.
People who use this feature and have a vast amount of entries should be
told to implement an alias file parser ;)

This I agree with. :)

I'd be happy with that approach as well for my use case. In general it
may suffer from the uniqueness problem: If there's a recent commit
authored by "Michael@Jeff.com" your "--author=Jeff" will resolve
differently from yesterday, and you won't even notice (not even commit
-v tells you). [ A typo is punished by a search through all commits;
that's fine.]

The commit message template should say:

  Author: A U Thor [off-list ref]

but of course you won't see that if you are using "-m".

I wonder if there is a good way to warn that we have multiple matches.
Of course we expect many _exact_ matches if the author has multiple
commits, but we could look for distinct matches. However, even that will
turn up false positives, since some authors have multiple email
addresses.

-Peff