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

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

[Michael J Gruber]

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.

Signed-off-by: Michael J Gruber <michaeljgruber+gmane@fastmail.fm>
---
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 "<>".

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

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

Note that the implementation is different from git-svn's author file on
purpose because it serves a different purpose.

Michael

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 ;) 

 builtin-commit.c |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 64 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 649c8be..d90e2f4 100644
--- 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;
+};
+static struct user **users;
+static int users_alloc;
+static int users_nr;
 /*
  * The default commit message cleanup mode will remove the lines
  * beginning with # (shell comments) and leading and trailing
@@ -406,6 +412,7 @@ static const char sign_off_header[] = "Signed-off-by: ";
 static void determine_author_info(void)
 {
 	char *name, *email, *date;
+	int i;
 
 	name = getenv("GIT_AUTHOR_NAME");
 	email = getenv("GIT_AUTHOR_EMAIL");
@@ -429,10 +436,22 @@ static void determine_author_info(void)
 		date = xstrndup(rb + 2, eol - (rb + 2));
 	}
 
+	author_date = date;
+
 	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++) {
+				if (!strcmp(force_author, users[i]->name)) {
+					author_name = users[i]->full_name;
+					author_email = users[i]->email;
+					return;
+				}
+			}
+		}
+
 		if (!lb || !rb)
 			die("malformed --author parameter");
 		name = xstrndup(force_author, lb - force_author);
@@ -441,7 +460,6 @@ static void determine_author_info(void)
 
 	author_name = name;
 	author_email = email;
-	author_date = date;
 }
 
 static int prepare_to_commit(const char *index_file, const char *prefix)
@@ -888,11 +906,56 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	}
 }
 
+static struct user *make_user(const char *name, int len)
+{
+	struct user *ret;
+	int i;
+
+	for (i = 0; i < users_nr; i++) {
+		if (len ? (!strncmp(name, users[i]->name, len) &&
+			   !users[i]->name[len]) :
+		    !strcmp(name, users[i]->name))
+			return users[i];
+	}
+
+	ALLOC_GROW(users, users_nr + 1, users_alloc);
+	ret = xcalloc(1, sizeof(struct user));
+	users[users_nr++] = ret;
+	if (len)
+		ret->name = xstrndup(name, len);
+	else
+		ret->name = xstrdup(name);
+
+	return ret;
+}
+
 static int git_commit_config(const char *k, const char *v, void *cb)
 {
+	const char *name;
+	const char *subkey;
+	struct user *user;
+
 	if (!strcmp(k, "commit.template"))
 		return git_config_string(&template_file, k, v);
 
+	if (!prefixcmp(k, "user.")) {
+		name = k + 5;
+		subkey = strrchr(name, '.');
+		if (!subkey)
+			return 0;
+		user = make_user(name, subkey - name);
+		if (!strcmp(subkey, ".name")) {
+			if (!v)
+				return config_error_nonbool(k);
+			user->full_name = xstrdup(v);
+		} else if (!strcmp(subkey, ".email")) {
+			if (!v)
+				return config_error_nonbool(k);
+			user->email = xstrdup(v);
+		}
+		return 0;
+	}
+
 	return git_status_config(k, v, cb);
 }
 
-- 
1.6.0

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

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 248 bytes --]

On Thu, Aug 21, 2008 at 11:19:41AM +0200, Michael J Gruber <michaeljgruber+gmane@fastmail.fm> wrote:
> If there's interest in this patch I'll follow up with a documentation patch.

See http://article.gmane.org/gmane.comp.version-control.git/92913.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

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

[Michael J Gruber]

Miklos Vajna venit, vidit, dixit 21.08.2008 15:49:
> On Thu, Aug 21, 2008 at 11:19:41AM +0200, Michael J Gruber <michaeljgruber+gmane@fastmail.fm> wrote:
>> If there's interest in this patch I'll follow up with a documentation patch.
> 
> See http://article.gmane.org/gmane.comp.version-control.git/92913.

I've read the post you quote but I'm not sure you've read that AND my
post. I clearly described/documented what my patch does and why I think
it's useful, in the way it's often done here: after the commit message
and before the diffstat. It is documented (as required in the post you
cite), it just doesn't contain a documentation patch.

Documentation/SubmittingPatches in all its length doesn't contain the
requirement you're reading into Junio's post. Maybe it should, if that's
what is meant.

Michael

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

[Alex Riesen]

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

Isn't there existing well-known formats for mail aliases?
For instance, Mutt uses simple text file:

    alias nickname1 Author Name <mail@address>
    alias nickname2 "Author Name 2" <mail2@address>

I don't know how well-known this is, but is surely more known than
git's config (and there are aliases in that format already).
Maybe just reference such files in git's config?

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

[Alex Riesen]

Alex Riesen, Thu, Aug 21, 2008 19:41:18 +0200:
> Michael J Gruber, Thu, Aug 21, 2008 11:19:41 +0200:
> > 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.
> 
> Isn't there existing well-known formats for mail aliases?
> For instance, Mutt uses simple text file:
> 
>     alias nickname1 Author Name <mail@address>
>     alias nickname2 "Author Name 2" <mail2@address>
> 
> I don't know how well-known this is, but is surely more known than
> git's config (and there are aliases in that format already).
> Maybe just reference such files in git's config?
> 

Oh, and you may consider using .mailmap files (look into the Git's
one for example): the user part of mail address is very often a good
alias (and sometimes famous nickname) of a person: junio, tytso,
davem, alan, viro, hpa... You'll have to define some rules for
duplications, of course (first wins seems to be popular).

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

[Jeff King]

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". The reason I think Junio (and other reviewers) find the "I'll
document this if it is accepted" so frustrating is that it puts them in
an awkward position.

When reviewing, you are trying to say "is this patch OK?". And clearly
it isn't, because it lacks documentation. Now Junio could queue your
patch and wait for the documentation, but sometimes the followup doc
patches aren't as easily forthcoming, and then he has to deal with it
later.

Furthermore, it is sort of a good faith effort. It shows that you put
the work into cleaning up the patch for presenting to the community,
which encourages the community to take a look. Saying "this is half of
the work, and I will do the other half if you like this" makes reviewers
wonder how cleaned up and ready the patch is.

All of that being said, I think in this instance it is less about the
patch and more in the words you picked. If you said "I am thinking about
this feature, and here is how I think the interface should work, and
here is the patch I have so far. I don't want to document the interface
until it is settled, so please comment on that and I will work up a
final patch" then that would have gone over very well. But as it
happens, you chose the magic pet peeve words. ;)

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

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 this
uses the existing strings. Is this going to accidentally free() from the
users list, or are we just leaking those other strings now?

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

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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:

In short, I'm in agreement with almost everything you said in your
response, in that I think (1) this is a reasonable thing to want to do,
(2) this should use an external mail-alias file, not set of in-config
values, possibly sharing the database with send-email, (3) committer
should be treated the same way (shouldn't the effort be the same?
otherwise there is something wrong in the existing code structure).

>> In an ideal word, all my collaborators would exchange changes as git 
>> ...
>> --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?

Absolutely.  What the change does is already visible in "log -p".  The
reason behind the change, "Why", is much more important, and Michael's
justification was very well written.  It should have been in the proposed
commit log message.

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

[Michael J Gruber]

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:
> 
>> 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. :)

>> --- 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?

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

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

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

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

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

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.

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

[Jeff King]

[oops, this accidentally got taken off the list, so here is a repost to
the list and all interested parties]

On Fri, Aug 22, 2008 at 10:27:24AM +0200, Michael J Gruber wrote:

> 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 agree with your analysis here. The mapping done by mailmap and git-svn
aren't the same. The ones for send-email are, but there is simply an
implementation hurdle.

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

I think this depends on your situation. In your case, it sounds like you
want to configure a few names that frequently have --author fields for
your specific workflow. For me, even though only 1% of the people in my
mua's alias file might send me patches, 99% of the people I would want
to use --author on are in my mua's alias file.

So while there are may only be a few needed entries, they are already
there for me. Of course, I don't really use --author much, since most
people I talk to are already git users. ;) So I am extrapolating a bit.

> >> 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 ;)

Heh.

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

I think that is sensible.

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

OK, I see. I wonder if it is worth xstrdup'ing them _anyway_, so that
determine_author_info produces a consistent result, and the person who
later does the free() cleanup won't get a nasty surprise. But the
leakage is probably not enough to really care about in this instance.

> 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? ]

No, no need to attribute in this case, I think.

I think the way you have done the config is fine, unless somebody else
has a major style objection (and yes, there are examples of similar
styles).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> 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.
>
> I think this depends on your situation. In your case, it sounds like you
> want to configure a few names that frequently have --author fields for
> your specific workflow. For me, even though only 1% of the people in my
> mua's alias file might send me patches, 99% of the people I would want
> to use --author on are in my mua's alias file.
>
> So while there are may only be a few needed entries, they are already
> there for me. Of course, I don't really use --author much, since most
> people I talk to are already git users. ;) So I am extrapolating a bit.

Another potential source of this information is the existing commits.  If
you are communicating with the same set of people already, you already
have the information in your repository.  I suspect Michael's "selected
few co-workers that would comfortably fit in a small list of config
entries without need for any external text file" use case would be better
served by an approach to look into existing commits.

I often use "git who Jeff" alias to fill the recipient of my e-mails with
this alias:

    [alias]
        who = "!sh -c 'git log -1 --pretty=\"format:%an <%ae>\" --author=\"$1\"' -"
        one = "!sh -c 'git show -s --pretty=\"format:%h (%s, %ai\" \"$@\" | sed -e \"s/ [012][0-9]:[0-5][0-9]:[0-5][0-9] [-+][0-9][0-9][0-9][0-9]$/)/\"' -"

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

[Jeff King]

On Fri, Aug 22, 2008 at 02:09:35PM -0700, Junio C Hamano wrote:

> I often use "git who Jeff" alias to fill the recipient of my e-mails with
> this alias:
> 
>     [alias]
>         who = "!sh -c 'git log -1 --pretty=\"format:%an <%ae>\" --author=\"$1\"' -"

Very clever, I like it. And it also solves the problem I sometimes _do_
have, which is pulling aliases into my mua from git.

-Peff

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

[Michael J Gruber]

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.

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

Signed-off-by: Michael J Gruber <michaeljgruber+gmane@fastmail.fm>
---

I tried to apply everything I've learned from this thread:
- Justification in commit message rather than cover
- minor style adjustments
- xstrdup two more strings to spare future leakage cleanup-a-thons a few
  unpleasant surprises
- comes with documentation patch now

I think the relation to and distinction from "git-svn -A" and ".mailmap"
has become clear through the discussion (should a summary go in the commit
message?).
I really like Junio's alias (git who). It's certainly helpful. For the
case of "git commit --author key" I think we should not simply go by the
first, possibly non-unique match returned by "git show". Also, being able
to say "git commit --author=nitpicker" may make some days brighter ;)

 Documentation/config.txt     |    8 +++++
 Documentation/git-commit.txt |    5 ++-
 builtin-commit.c             |   67 +++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9020675..9bea3a3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1107,6 +1107,14 @@ user.signingkey::
 	unchanged to gpg's --local-user parameter, so you may specify a key
 	using any method that gpg supports.
 
+user.<author>.email::
+	The email address to be recorded in a newly created commit if you
+	specify the option \--author=<author> to linkgit:git-commit[1].
+
+user.<author>.name::
+	The full name to be recorded in a newly created commit if you
+	specify the option \--author=<author> to linkgit:git-commit[1].
+
 imap::
 	The configuration variables in the 'imap' section are described
 	in linkgit:git-imap-send[1].
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index 0e25bb8..1685cf6 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -76,7 +76,10 @@ OPTIONS
 
 --author=<author>::
 	Override the author name used in the commit.  Use
-	`A U Thor <author@example.com>` format.
+	`A U Thor <author@example.com>` format. Alternatively, if
+	<author> does not contain `<>` then the configuration
+	variables `user.<author>.name` and `user.<author>.email`
+	are used if present (see linkgit:git-config[1]).
 
 -m <msg>::
 --message=<msg>::
diff --git a/builtin-commit.c b/builtin-commit.c
index 649c8be..c36e60f 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -53,6 +53,14 @@ 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;
+	char *full_name;
+	char *email;
+};
+static struct user **users;
+static int users_alloc;
+static int users_nr;
 /*
  * The default commit message cleanup mode will remove the lines
  * beginning with # (shell comments) and leading and trailing
@@ -406,6 +414,7 @@ static const char sign_off_header[] = "Signed-off-by: ";
 static void determine_author_info(void)
 {
 	char *name, *email, *date;
+	int i;
 
 	name = getenv("GIT_AUTHOR_NAME");
 	email = getenv("GIT_AUTHOR_EMAIL");
@@ -429,10 +438,22 @@ static void determine_author_info(void)
 		date = xstrndup(rb + 2, eol - (rb + 2));
 	}
 
+	author_date = date;
+
 	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++) {
+				if (!strcmp(force_author, users[i]->name)) {
+					author_name = xstrdup(users[i]->full_name);
+					author_email = xstrdup(users[i]->email);
+					return;
+				}
+			}
+		}
+
 		if (!lb || !rb)
 			die("malformed --author parameter");
 		name = xstrndup(force_author, lb - force_author);
@@ -441,7 +462,6 @@ static void determine_author_info(void)
 
 	author_name = name;
 	author_email = email;
-	author_date = date;
 }
 
 static int prepare_to_commit(const char *index_file, const char *prefix)
@@ -888,11 +908,56 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	}
 }
 
+static struct user *make_user(const char *name, int len)
+{
+	struct user *ret;
+	int i;
+
+	for (i = 0; i < users_nr; i++) {
+		if (len ? (!strncmp(name, users[i]->name, len) &&
+			   !users[i]->name[len]) :
+		    !strcmp(name, users[i]->name))
+			return users[i];
+	}
+
+	ALLOC_GROW(users, users_nr + 1, users_alloc);
+	ret = xcalloc(1, sizeof(struct user));
+	users[users_nr++] = ret;
+	if (len)
+		ret->name = xstrndup(name, len);
+	else
+		ret->name = xstrdup(name);
+
+	return ret;
+}
+
 static int git_commit_config(const char *k, const char *v, void *cb)
 {
+	const char *name;
+	const char *subkey;
+	struct user *user;
+
 	if (!strcmp(k, "commit.template"))
 		return git_config_string(&template_file, k, v);
 
+	if (!prefixcmp(k, "user.")) {
+		name = k + 5;
+		subkey = strrchr(name, '.');
+		if (!subkey)
+			return 0;
+		user = make_user(name, subkey - name);
+		if (!strcmp(subkey, ".name")) {
+			if (!v)
+				return config_error_nonbool(k);
+			user->full_name = xstrdup(v);
+		} else if (!strcmp(subkey, ".email")) {
+			if (!v)
+				return config_error_nonbool(k);
+			user->email = xstrdup(v);
+		}
+		return 0;
+	}
+
 	return git_status_config(k, v, cb);
 }
 
-- 
1.6.0

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

[Junio C Hamano]

Michael J Gruber <michaeljgruber+gmane@fastmail.fm> 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?

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 27.08.2008 08:13:
> Jeff King <peff@peff.net> writes:
> 
>> On Tue, Aug 26, 2008 at 04:31:30PM -0700, Junio C Hamano 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.
>>> 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 <author@example.com> format right.

Michael

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

[Jeff King]

[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 <author@example.com>

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

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

[Jeff King]

On Wed, Aug 27, 2008 at 10:15:19AM -0700, Junio C Hamano wrote:

> > 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.
> 
> In order to prove unique match you would need an exhaustive check, don't
> you?

Yes, though you also do exhaustive check in the worst case already (when
the name doesn't match anything). It takes about .7s on a warm cache on
my git.git.

Anyway, I think it is already not a good idea because of the semantics,
let alone the performance.

-Peff

PS Your message also didn't go to git@vger, so I think you are having
the same problem with Michael's message that I am.

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

[Michael J Gruber]

Jeff King venit, vidit, dixit 27.08.2008 19:18:
> On Wed, Aug 27, 2008 at 10:15:19AM -0700, Junio C Hamano wrote:
> 
>>> 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.
>> In order to prove unique match you would need an exhaustive check, don't
>> you?
> 
> Yes, though you also do exhaustive check in the worst case already (when
> the name doesn't match anything). It takes about .7s on a warm cache on
> my git.git.
> 
> Anyway, I think it is already not a good idea because of the semantics,
> let alone the performance.

By "it" you are referring to
- checking for uniqueness or
- the whole approach combing through commits?

I'd be happy with the patch as is (+"-i" maybe) now that I understand
the template... (I tried --author=key with a key expanding to the
(default) committer, in which case the commit template does not show
author nor committer. Duh.)

> -Peff
> 
> PS Your message also didn't go to git@vger, so I think you are having
> the same problem with Michael's message that I am.

OK: I send this To: Jeff, Cc: Junio, Nntp:
gmane.comp.version-control.git (using Thunderbird 2). (This is the
result of hitting "reply all" and deleting git@vger, because it's
duplicated by gmane...git.)

Could the two of you please tell me what you are receiving? I'm sorry
for this, but if this is a systematic problem with gmane I should switch
(and others should be warned); if it's a TB thing I will cope.

Michael

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

[Jeff King]

On Thu, Aug 28, 2008 at 10:53:23AM +0200, Michael J Gruber wrote:

> > Anyway, I think it is already not a good idea because of the semantics,
> > let alone the performance.
> 
> By "it" you are referring to
> - checking for uniqueness or
> - the whole approach combing through commits?

Sorry, I meant "checking for uniqueness." I think combing through is a
fine idea, but checking for uniqueness will come up with false
positives.

> I'd be happy with the patch as is (+"-i" maybe) now that I understand
> the template... (I tried --author=key with a key expanding to the
> (default) committer, in which case the commit template does not show
> author nor committer. Duh.)

You could pull the first from either (I think you will have to look at
the revision.c code at a little bit lower level -- look at how --author
adds grep fields). In a patch-by-mail repository like git.git, Junio is
almost always the committer. But other projects will have different
workflows.

> > PS Your message also didn't go to git@vger, so I think you are having
> > the same problem with Michael's message that I am.
> 
> OK: I send this To: Jeff, Cc: Junio, Nntp:
> gmane.comp.version-control.git (using Thunderbird 2). (This is the
> result of hitting "reply all" and deleting git@vger, because it's
> duplicated by gmane...git.)

Ah, OK. So your message ends up in my mailbox without a mention of
git@vger at all. It of course has a "newsgroups" header, but that is not
helpful for people who do not use gmane at all. And given that this is
primarily a mailing list which has a newsgroup interface, and not vice
versa, I think it makes sense to give priority to the mail interface.

Is there a reason to post through gmane at all? Why not just leave the
git@vger cc, and kill the nntp post?

-Peff

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

[Jeff King]

On Thu, Aug 28, 2008 at 09:52:02AM +0200, Michael J Gruber wrote:

> Junio C Hamano venit, vidit, dixit 27.08.2008 19:13:
> > Michael J Gruber <michaeljgruber+gmane@fastmail.fm> writes:
> > 
> >> 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 ;)
> > 
> > That attitude is Ok for an experimental piece of software.  Perhaps it was
> > Ok for git 18 months ago as well, but not anymore.
> 
> I probably should have put the ;) in emphasis. This is not my attitude.

Hmm. It sounds like we your interest is moving towards Junio's approach,
so maybe this doesn't matter. But I actually think your statement above
made some sense. I think we will be providing multiple sources of alias
information in the long run anyway, so this becomes just another source.
As a source, it has some advantages (it is simple to setup in your
existing git config, and does not require an extra file), and some
disadvantages (it does not scale as well as some other solutions).

> P.S.: This is "reply all" to a mail sent off-list probably meant for the
> list, but I didn't want to cc: the list without your consent (since I'm
> quoting you). I'm sorry for this confusion. I'm sure it's not your MUAs
> and confident it's not mine, which leaves gmane..

I am putting it back on-list. :)

-Peff

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

[Jeff King]

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

I thought about it after writing the one you saw on the list, but from the
beginning I was planning to do this merely as a demonstration patch that
somebody who is interested in the feature can polish and resubmit with
test and documentation.  I didn't bother adding such frills --- that is
part of "polish and resubmit" cycle.

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

[Pedro Melo]

Hi,

On Aug 22, 2008, at 10:09 PM, Junio C Hamano wrote:
> Another potential source of this information is the existing  
> commits.  If
> you are communicating with the same set of people already, you already
> have the information in your repository.  I suspect Michael's  
> "selected
> few co-workers that would comfortably fit in a small list of config
> entries without need for any external text file" use case would be  
> better
> served by an approach to look into existing commits.
>
> I often use "git who Jeff" alias to fill the recipient of my e- 
> mails with
> this alias:
>
>     [alias]
>         who = "!sh -c 'git log -1 --pretty=\"format:%an <%ae>\" -- 
> author=\"$1\"' -"

Nice:)

>         one = "!sh -c 'git show -s --pretty=\"format:%h (%s, %ai\"  
> \"$@\" | sed -e \"s/ [012][0-9]:[0-5][0-9]:[0-5][0-9] [-+][0-9][0-9] 
> [0-9][0-9]$/)/\"' -"

Can you explain this one? It seems a bit like git describe, but it  
misses a single char at the beggining?

git (master) $ git one
2ebc02d (Start 1.6.1 cycle, 2008-08-17)

git (master) $ git describe
v1.6.0-2-g2ebc02d

Best regards,
-- 
Pedro Melo
Blog: http://www.simplicidade.org/notes/
XMPP ID: melo@simplicidade.org
Use XMPP!

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

[Jeff King]

On Sun, Aug 24, 2008 at 10:19:20AM +0100, Pedro Melo wrote:

>>         one = "!sh -c 'git show -s --pretty=\"format:%h (%s, %ai\"  
>> \"$@\" | sed -e \"s/ [012][0-9]:[0-5][0-9]:[0-5][0-9] [-+][0-9][0-9] 
>> [0-9][0-9]$/)/\"' -"
>
> Can you explain this one? It seems a bit like git describe, but it misses 
> a single char at the beggining?
>
> git (master) $ git one
> 2ebc02d (Start 1.6.1 cycle, 2008-08-17)
>
> git (master) $ git describe
> v1.6.0-2-g2ebc02d

The 'g' character is not part of the sha1, but just a prefix used by git
describe. The point of this alias is to refer (in email or other
writing) to commits. Obviously just the sha1 would be sufficient, but
the subject and date of the commit gives the reader some context without
them having to plug it into git-show.

-Peff

[PATCH] fix "git log -i --grep"

[Jeff King]

On Fri, Aug 22, 2008 at 02:09:35PM -0700, Junio C Hamano wrote:

>     [alias]
>         who = "!sh -c 'git log -1 --pretty=\"format:%an <%ae>\" --author=\"$1\"' -"

I have two improvements for this, and one of them caused me to find a
git bug, for which the fix is below. :)

  1. I tried this with --no-pager, which made it obvious that this
     should be using --pretty=tformat to append a newline.

  2. I use it with "-i" so I don't have to hit the shift key. And that's
     what revealed the bug.

-- >8 --
fix "git log -i --grep"

This has been broken in v1.6.0 due to the reorganization of
the revision option parsing code. The "-i" is completely
ignored, but works fine in "git log --grep -i".

What happens is that the code for "-i" looks for
revs->grep_filter; if it is NULL, we do nothing, since there
are no grep filters. But that is obviously not correct,
since we want it to influence the later --grep option. Doing
it the other way around works, since "-i" just impacts the
existing grep_filter option.

The fix is to allocate the grep_filter member whenever we
get _any_ grep information, be it actual filters or just
flags. Thus checking for non-NULL revs->grep_filter is no
longer sufficient to know that we have patterns; in
commit_match we must actually check that the pattern list is
not empty.

Signed-off-by: Jeff King <peff@peff.net>
---
I didn't bother bisecting, but I'm pretty sure this was a fallout from
Pierre's revision option parsing rewrite.

This was generated with -U5 to make the first hunk easier to read.

We could potentially make revs->grep_filter a part of the struct, rather
than malloc'ing it (since we have to look inside grep_filter anyway to
see if there are any patterns). But that still doesn't save us from a
setup_grep call, since we have to initialize some values inside it.
Potentially this setup (which is not very costly) could just be done
when initializing the rev_info struct, and then we could just assume
that grep_filter was always valid.

I went with the less intrusive change in this case, but I am happy to
work it up the other way.

 revision.c     |   25 +++++++++++++++----------
 t/t4202-log.sh |   22 ++++++++++++++++++++++
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/revision.c b/revision.c
index 8cd39da..a73612f 100644
--- a/revision.c
+++ b/revision.c
@@ -942,19 +942,24 @@ void read_revisions_from_stdin(struct rev_info *revs)
 		if (handle_revision_arg(line, revs, 0, 1))
 			die("bad revision '%s'", line);
 	}
 }
 
-static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
+static void setup_grep(struct rev_info *revs)
 {
 	if (!revs->grep_filter) {
 		struct grep_opt *opt = xcalloc(1, sizeof(*opt));
 		opt->status_only = 1;
 		opt->pattern_tail = &(opt->pattern_list);
 		opt->regflags = REG_NEWLINE;
 		revs->grep_filter = opt;
 	}
+}
+
+static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
+{
+	setup_grep(revs);
 	append_grep_pattern(revs->grep_filter, ptn,
 			    "command line", 0, what);
 }
 
 static void add_header_grep(struct rev_info *revs, const char *field, const char *pattern)
@@ -1167,21 +1172,21 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!prefixcmp(arg, "--committer=")) {
 		add_header_grep(revs, "committer", arg+12);
 	} else if (!prefixcmp(arg, "--grep=")) {
 		add_message_grep(revs, arg+7);
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
-		if (revs->grep_filter)
-			revs->grep_filter->regflags |= REG_EXTENDED;
+		setup_grep(revs);
+		revs->grep_filter->regflags |= REG_EXTENDED;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
-		if (revs->grep_filter)
-			revs->grep_filter->regflags |= REG_ICASE;
+		setup_grep(revs);
+		revs->grep_filter->regflags |= REG_ICASE;
 	} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
-		if (revs->grep_filter)
-			revs->grep_filter->fixed = 1;
+		setup_grep(revs);
+		revs->grep_filter->fixed = 1;
 	} else if (!strcmp(arg, "--all-match")) {
-		if (revs->grep_filter)
-			revs->grep_filter->all_match = 1;
+		setup_grep(revs);
+		revs->grep_filter->all_match = 1;
 	} else if (!prefixcmp(arg, "--encoding=")) {
 		arg += 11;
 		if (strcmp(arg, "none"))
 			git_log_output_encoding = xstrdup(arg);
 		else
@@ -1647,11 +1652,11 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
 	return 0;
 }
 
 static int commit_match(struct commit *commit, struct rev_info *opt)
 {
-	if (!opt->grep_filter)
+	if (!opt->grep_filter || !opt->grep_filter->pattern_list)
 		return 1;
 	return grep_buffer(opt->grep_filter,
 			   NULL, /* we say nothing, not even filename */
 			   commit->buffer, strlen(commit->buffer));
 }
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 4c8af45..0ab925c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -67,9 +67,31 @@ test_expect_success 'diff-filter=D' '
 		false
 	}
 
 '
 
+test_expect_success 'setup case sensitivity tests' '
+	echo case >one &&
+	test_tick &&
+	git commit -a -m Second
+'
+
+test_expect_success 'log --grep' '
+	echo second >expect &&
+	git log -1 --pretty="tformat:%s" --grep=sec >actual &&
+	test_cmp expect actual
+'
 
+test_expect_success 'log -i --grep' '
+	echo Second >expect &&
+	git log -1 --pretty="tformat:%s" -i --grep=sec >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --grep -i' '
+	echo Second >expect &&
+	git log -1 --pretty="tformat:%s" --grep=sec -i >actual &&
+	test_cmp expect actual
+'
 
 test_done
 
-- 
1.6.0.150.gc3242.dirty

[PATCH] format-patch: use default diff format even with patch options

[Jeff King]

On Sun, Aug 24, 2008 at 09:38:37PM -0400, Jeff King wrote:

> This was generated with -U5 to make the first hunk easier to read.

And while doing that, I detected another bug. Or maybe a feature,
depending on your perspective.

-- >8 --
format-patch: use default diff format even with patch options

Previously, running "git format-patch -U5" would cause the
low-level diff machinery to change the diff output format
from "not specified" to "patch". This meant that
format-patch thought we explicitly specified a diff output
format, and would not use the default format. The resulting
message lacked both the diffstat and the summary, as well as
the separating "---".

Now format-patch explicitly checks for this condition and
uses the default. That means that "git format-patch -p" will
now have the "-p" ignored.

Signed-off-by: Jeff King <peff@peff.net>
---
Maybe this is intentional, and that by asking for "-U" I am explicitly
saying "I really want the patch format, not the default." But I think
this more reasonably maps to what the user expects.

I am a little uncomfortable hurting anyone who thought that
"format-patch -p" was a good idea. OTOH:

  1. I have to question why they were using format-patch in the first
     place. Probably git-log --pretty=email would be a better fit.

  2. Their mails were already broken, since the presence of the diffstat
     is what triggers the "---" divider.

 builtin-log.c           |    3 ++-
 t/t4014-format-patch.sh |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/builtin-log.c b/builtin-log.c
index 9204ffd..1d3c5cb 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -932,7 +932,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	if (argc > 1)
 		die ("unrecognized argument: %s", argv[1]);
 
-	if (!rev.diffopt.output_format)
+	if (!rev.diffopt.output_format
+		|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)
 		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;
 
 	if (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 7fe853c..9d99dc2 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -230,4 +230,29 @@ test_expect_success 'shortlog of cover-letter wraps overly-long onelines' '
 
 '
 
+cat > expect << EOF
+---
+ file |   16 ++++++++++++++++
+ 1 files changed, 16 insertions(+), 0 deletions(-)
+
+diff --git a/file b/file
+index 40f36c6..2dc5c23 100644
+--- a/file
++++ b/file
+@@ -13,4 +13,20 @@ C
+ 10
+ D
+ E
+ F
++5
+EOF
+
+test_expect_success 'format-patch respects -U' '
+
+	git format-patch -U4 -2 &&
+	sed -e "1,/^$/d" -e "/^+5/q" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&
+	test_cmp expect output
+
+'
+
 test_done
-- 
1.6.0.150.gc3242.dirty

Re: [PATCH] format-patch: use default diff format even with patch options

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I am a little uncomfortable hurting anyone who thought that
> "format-patch -p" was a good idea. OTOH:
>
>   1. I have to question why they were using format-patch in the first
>      place. Probably git-log --pretty=email would be a better fit.
>
>   2. Their mails were already broken, since the presence of the diffstat
>      is what triggers the "---" divider.
>
>  builtin-log.c           |    3 ++-
>  t/t4014-format-patch.sh |   25 +++++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletions(-)
>
> diff --git a/builtin-log.c b/builtin-log.c
> index 9204ffd..1d3c5cb 100644
> --- a/builtin-log.c
> +++ b/builtin-log.c
> @@ -932,7 +932,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	if (argc > 1)
>  		die ("unrecognized argument: %s", argv[1]);
>  
> -	if (!rev.diffopt.output_format)
> +	if (!rev.diffopt.output_format
> +		|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)
>  		rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;
>  
>  	if (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)

I think this is the right thing to do.  The only unusual option somebody
might want to use would be "format-patch --stat $range" to send out commit
log e-mails with diffstat summary but without the actual patch, but your
change does not break that use case either.

Re: [PATCH] fix "git log -i --grep"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Aug 22, 2008 at 02:09:35PM -0700, Junio C Hamano wrote:
>
>>     [alias]
>>         who = "!sh -c 'git log -1 --pretty=\"format:%an <%ae>\" --author=\"$1\"' -"
>
> I have two improvements for this, and one of them caused me to find a
> git bug, for which the fix is below. :)
>
>   1. I tried this with --no-pager, which made it obvious that this
>      should be using --pretty=tformat to append a newline.

Strict reading of POSIX suggests that you are not supposed to send an
input that has incomplete line to "sed", so tformat may be the right thing
to use for that reason as well.

However.

My sed is non POSIX in a good sense and does not have problem handing such
an input, and my use case is to say "\C-u \M-! git who Jeff <ENTER>" while
typing e-mail message, and I do _not_ want an extra newline after the
input.  That is why I use format: (not tformat:) there.

> The fix is to allocate the grep_filter member whenever we
> get _any_ grep information, be it actual filters or just
> flags. Thus checking for non-NULL revs->grep_filter is no
> longer sufficient to know that we have patterns; in
> commit_match we must actually check that the pattern list is
> not empty.

Well spotted, and thanks for the fix.

As you suggested, making the grep option structure embedded in rev_info
may not be a bad idea.  We used to keep track of the sub-options
separately while we encounter, and updated grep_filter at the end of the
loop, but the conversion to use parse-options broke it.

The only issue I still have, which I suspect your fix has made it easier
to address, is to complain if sub-options to grep like -i and -E are given
without --grep.  That's not something v1.5.6 series did, though.

Re: [PATCH] fix "git log -i --grep"

[Jeff King]

On Sun, Aug 24, 2008 at 10:12:34PM -0700, Junio C Hamano wrote:

> My sed is non POSIX in a good sense and does not have problem handing such
> an input, and my use case is to say "\C-u \M-! git who Jeff <ENTER>" while
> typing e-mail message, and I do _not_ want an extra newline after the
> input.  That is why I use format: (not tformat:) there.

Ah. I would have expected whatever you pulled the output into to eat the
newline. But there is no point in nitpicking, as this is a personal
alias. Mine uses tformat. :)

> > The fix is to allocate the grep_filter member whenever we
> > get _any_ grep information, be it actual filters or just
> > flags. Thus checking for non-NULL revs->grep_filter is no
> > longer sufficient to know that we have patterns; in
> > commit_match we must actually check that the pattern list is
> > not empty.
> 
> Well spotted, and thanks for the fix.

Actually, there is one spot missing from my previous patch. Rev-list
sets "save_commit_buffer" based on the value of grep_filter. So it must
also check grep_filter->pattern_list.

> As you suggested, making the grep option structure embedded in rev_info
> may not be a bad idea.  We used to keep track of the sub-options
> separately while we encounter, and updated grep_filter at the end of the
> loop, but the conversion to use parse-options broke it.

I worked up this patch, and it is below. However, I think it may not be
a good idea, because...

> The only issue I still have, which I suspect your fix has made it easier
> to address, is to complain if sub-options to grep like -i and -E are given
> without --grep.  That's not something v1.5.6 series did, though.

This is trivial with my first patch, but not with the second. With
grep_filter kept as a pointer, we know that if the pointer is non-NULL
but there are no patterns, then the user asked for grep options but
never --grep.

I guess this might be a helpful thing for some users, but I wonder if it
is being too unpredictable for script usage. I.e., a script like:

  git log -E `for i in "$@"; do echo --author=$i`

Anyway, the non-allocating patch is below. Aside from the test case, it
deletes more lines than it adds, which is always nice.

-- >8 --
fix "git log -i --grep"

This has been broken in v1.6.0 due to the reorganization of
the revision option parsing code. The "-i" is completely
ignored, but works fine in "git log --grep -i".

What happens is that the code for "-i" looks for
revs->grep_filter; if it is NULL, we do nothing, since there
are no grep filters. But that is obviously not correct,
since we want it to influence the later --grep option. Doing
it the other way around works, since "-i" just impacts the
existing grep_filter option.

Instead, we now always initialize the grep_filter member and
just fill in options and patterns as we get them. This means
that we can no longer check grep_filter for NULL, but
instead must check the pattern list to see if we have any
actual patterns.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin-rev-list.c |    3 ++-
 revision.c         |   34 ++++++++++++----------------------
 revision.h         |    3 ++-
 t/t4202-log.sh     |   22 ++++++++++++++++++++++
 4 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 893762c..c023003 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -645,7 +645,8 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	    revs.diff)
 		usage(rev_list_usage);
 
-	save_commit_buffer = revs.verbose_header || revs.grep_filter;
+	save_commit_buffer = revs.verbose_header ||
+		revs.grep_filter.pattern_list;
 	if (bisect_list)
 		revs.limited = 1;
 
diff --git a/revision.c b/revision.c
index e75079a..36291b6 100644
--- a/revision.c
+++ b/revision.c
@@ -782,6 +782,10 @@ void init_revisions(struct rev_info *revs, const char *prefix)
 
 	revs->commit_format = CMIT_FMT_DEFAULT;
 
+	revs->grep_filter.status_only = 1;
+	revs->grep_filter.pattern_tail = &(revs->grep_filter.pattern_list);
+	revs->grep_filter.regflags = REG_NEWLINE;
+
 	diff_setup(&revs->diffopt);
 	if (prefix && !revs->diffopt.prefix) {
 		revs->diffopt.prefix = prefix;
@@ -946,15 +950,7 @@ void read_revisions_from_stdin(struct rev_info *revs)
 
 static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
 {
-	if (!revs->grep_filter) {
-		struct grep_opt *opt = xcalloc(1, sizeof(*opt));
-		opt->status_only = 1;
-		opt->pattern_tail = &(opt->pattern_list);
-		opt->regflags = REG_NEWLINE;
-		revs->grep_filter = opt;
-	}
-	append_grep_pattern(revs->grep_filter, ptn,
-			    "command line", 0, what);
+	append_grep_pattern(&revs->grep_filter, ptn, "command line", 0, what);
 }
 
 static void add_header_grep(struct rev_info *revs, const char *field, const char *pattern)
@@ -1164,17 +1160,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	} else if (!prefixcmp(arg, "--grep=")) {
 		add_message_grep(revs, arg+7);
 	} else if (!strcmp(arg, "--extended-regexp") || !strcmp(arg, "-E")) {
-		if (revs->grep_filter)
-			revs->grep_filter->regflags |= REG_EXTENDED;
+		revs->grep_filter.regflags |= REG_EXTENDED;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
-		if (revs->grep_filter)
-			revs->grep_filter->regflags |= REG_ICASE;
+		revs->grep_filter.regflags |= REG_ICASE;
 	} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
-		if (revs->grep_filter)
-			revs->grep_filter->fixed = 1;
+		revs->grep_filter.fixed = 1;
 	} else if (!strcmp(arg, "--all-match")) {
-		if (revs->grep_filter)
-			revs->grep_filter->all_match = 1;
+		revs->grep_filter.all_match = 1;
 	} else if (!prefixcmp(arg, "--encoding=")) {
 		arg += 11;
 		if (strcmp(arg, "none"))
@@ -1349,9 +1341,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, const ch
 	if (diff_setup_done(&revs->diffopt) < 0)
 		die("diff_setup_done failed");
 
-	if (revs->grep_filter) {
-		compile_grep_patterns(revs->grep_filter);
-	}
+	compile_grep_patterns(&revs->grep_filter);
 
 	if (revs->reverse && revs->reflog_info)
 		die("cannot combine --reverse with --walk-reflogs");
@@ -1492,9 +1482,9 @@ static int rewrite_parents(struct rev_info *revs, struct commit *commit)
 
 static int commit_match(struct commit *commit, struct rev_info *opt)
 {
-	if (!opt->grep_filter)
+	if (!opt->grep_filter.pattern_list)
 		return 1;
-	return grep_buffer(opt->grep_filter,
+	return grep_buffer(&opt->grep_filter,
 			   NULL, /* we say nothing, not even filename */
 			   commit->buffer, strlen(commit->buffer));
 }
diff --git a/revision.h b/revision.h
index 1b04566..91f1944 100644
--- a/revision.h
+++ b/revision.h
@@ -2,6 +2,7 @@
 #define REVISION_H
 
 #include "parse-options.h"
+#include "grep.h"
 
 #define SEEN		(1u<<0)
 #define UNINTERESTING   (1u<<1)
@@ -92,7 +93,7 @@ struct rev_info {
 	int		show_log_size;
 
 	/* Filter by commit log message */
-	struct grep_opt	*grep_filter;
+	struct grep_opt	grep_filter;
 
 	/* Display history graph */
 	struct git_graph *graph;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 4c8af45..0ab925c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -69,7 +69,29 @@ test_expect_success 'diff-filter=D' '
 
 '
 
+test_expect_success 'setup case sensitivity tests' '
+	echo case >one &&
+	test_tick &&
+	git commit -a -m Second
+'
+
+test_expect_success 'log --grep' '
+	echo second >expect &&
+	git log -1 --pretty="tformat:%s" --grep=sec >actual &&
+	test_cmp expect actual
+'
 
+test_expect_success 'log -i --grep' '
+	echo Second >expect &&
+	git log -1 --pretty="tformat:%s" -i --grep=sec >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --grep -i' '
+	echo Second >expect &&
+	git log -1 --pretty="tformat:%s" --grep=sec -i >actual &&
+	test_cmp expect actual
+'
 
 test_done
 
-- 
1.6.0.150.gc3242.dirty

Re: [PATCH] fix "git log -i --grep"

[Jeff King]

On Mon, Aug 25, 2008 at 02:15:05AM -0400, Jeff King wrote:

> > As you suggested, making the grep option structure embedded in rev_info
> > may not be a bad idea.  We used to keep track of the sub-options
> > separately while we encounter, and updated grep_filter at the end of the
> > loop, but the conversion to use parse-options broke it.
> 
> I worked up this patch, and it is below. However, I think it may not be
> a good idea, because...

Actually, let me amend that. While writing the email, I came to the
conclusion that the "complain if -i but not --grep" is probably not a
good idea. So in that case, I think this patch (allocating grep_filter
inside the struct) _is_ a good idea.

But I don't feel too strongly either way.  If you disagree, we can go
with the first one, and I can resend it (with the cleanup I mentioned)
and I can do the trivial complaining patch on top of it.

-Peff

Re: [PATCH] fix "git log -i --grep"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I worked up this patch, and it is below. However, I think it may not be
> a good idea, because...
>
>> The only issue I still have, which I suspect your fix has made it easier
>> to address, is to complain if sub-options to grep like -i and -E are given
>> without --grep.  That's not something v1.5.6 series did, though.
>
> This is trivial with my first patch, but not with the second. With
> grep_filter kept as a pointer, we know that if the pointer is non-NULL
> but there are no patterns, then the user asked for grep options but
> never --grep.

Hmm, that's true --- instead you would need to introduce a new flag in
rev_info that records if you saw any grep sub-options, if we want to check
this condition.

> I guess this might be a helpful thing for some users, but I wonder if it
> is being too unpredictable for script usage. I.e., a script like:
>
>   git log -E `for i in "$@"; do echo --author=$i`

Ok, that's true, so let's not worry about making "log -i without --grep"
an error.

> Anyway, the non-allocating patch is below. Aside from the test case, it
> deletes more lines than it adds, which is always nice.

Yeah, thanks.