edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[Adam Megacz]

Hi, folks.

>From the output of 'git show', it appears that a commit has a few fields
of metadata associated with it in addition to the comment.  These fields
seem to include Author, AuthorDate, Committer, and CommitDate.

  1. Are there other fields aside from these four?

  2. When I invoke 'git commit' without the '-m' argument I'm dropped
     into the cozy $EDITOR of my choice and given the opportunity to
     edit the commit message.  Is there any way to include the metadata
     fields in this editing session?  That way I could both sanity-check
     them as I perform the commit (important) and modify them if they're
     wrong (less important).

     I've been having problems lately with running git on machines where
     I forgot to set up my .gitconfig; I wind up with patches that have
     committers like root@mymachine and so forth.  Being automatically
     shown the committer/author when I make the commit would help me
     avoid these situations.

Thanks,

  - a

Re: edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[Sverre Rabbelier]

Heya,

On Sun, Jan 3, 2010 at 18:32, Adam Megacz <adam@megacz.com> wrote:
>     I've been having problems lately with running git on machines where
>     I forgot to set up my .gitconfig; I wind up with patches that have
>     committers like root@mymachine and so forth.  Being automatically
>     shown the committer/author when I make the commit would help me
>     avoid these situations.

At the very least it should be easy to include these fields as
comments in the message template. But of course you would still be
bitten if you used "git commit -m" :(.

-- 
Cheers,

Sverre Rabbelier

Re: edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[Adam Megacz]

Sverre Rabbelier <srabbelier@gmail.com> writes:
> On Sun, Jan 3, 2010 at 18:32, Adam Megacz <adam@megacz.com> wrote:
>>     I've been having problems lately with running git on machines where
>>     I forgot to set up my .gitconfig; I wind up with patches that have
>>     committers like root@mymachine and so forth.  Being automatically
>>     shown the committer/author when I make the commit would help me
>>     avoid these situations.
>
> At the very least it should be easy to include these fields as
> comments in the message template.

That would be great.

> But of course you would still be bitten if you used "git commit -m"
> :(.

Perhaps a preference (off by default) demanding that they be set
explicitly when "git commit -m" is used?

Some people care more than others about the metadata; this is for the
folks to whom it matters a lot.

  - a

Re: edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[Sverre Rabbelier]

Heya,

On Mon, Jan 4, 2010 at 16:08, Adam Megacz <adam@megacz.com> wrote:
> Perhaps a preference (off by default) demanding that they be set
> explicitly when "git commit -m" is used?

Heh, what use would that be? On a different/new box you would have
neither that setting nor the email set, so that doens't solve the
problem methinks :P.

-- 
Cheers,

Sverre Rabbelier

Re: edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[David Aguilar]

On Mon, Jan 04, 2010 at 05:52:42PM -0500, Sverre Rabbelier wrote:
> Heya,
> 
> On Mon, Jan 4, 2010 at 16:08, Adam Megacz <adam@megacz.com> wrote:
> > Perhaps a preference (off by default) demanding that they be set
> > explicitly when "git commit -m" is used?
> 
> Heh, what use would that be? On a different/new box you would have
> neither that setting nor the email set, so that doens't solve the
> problem methinks :P.
> 
> -- 
> Cheers,
> 
> Sverre Rabbelier

Workaround:

If you use "git commit -s" it includes a Signed-off-by line
which includes your name and email.

Seeing "Signed-off-by: root <root@localhost>" would give you a
hint that you should abort the commit, set the vars, and
try again.


-- 
		David

Re: edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[Nanako Shiraishi]

Quoting Adam Megacz <adam@megacz.com>

> Perhaps a preference (off by default) demanding that they be set
> explicitly when "git commit -m" is used?

Sverre pointed out why this won't work.

> Some people care more than others about the metadata; this is for the
> folks to whom it matters a lot.

So the only workable solution is to check your commits with "git show -s" until you become confident that you configured your new box correctly. Some people unfortunately don't care enough to do so, but it is for the people to whom it matters.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Adam Megacz <adam@megacz.com>
>
>> Perhaps a preference (off by default) demanding that they be set
>> explicitly when "git commit -m" is used?
>
> Sverre pointed out why this won't work.
>
>> Some people care more than others about the metadata; this is for the
>> folks to whom it matters a lot.
>
> So the only workable solution is to check your commits with "git show
> -s" until you become confident that you configured your new box
> correctly. Some people unfortunately don't care enough to do so, but it
> is for the people to whom it matters.

Traditionally, we've only had a minimal sanity check (e.g. to barf when
the name is empty, or something silly like that) and tried to come up with
a reasonable name/email given the available system information.

The approach may have been Ok 10 years ago, back when `whoami`@`hostname`,
at least on systems that were competently maintained, gave a reasonable
mail address for most people, but I don't think it is adequate anymore to
majority of people, especially the ones who work on Open Source projects
as individuals, whose desired public identities are often tied to their
email account at their ISPs or mailbox providers (like gmail), and there
is no way for us to guess what it is from `whoami` nor `hostname` [*1*].

So I don't think anybody minds if we refuse to work if we are going to end
up using a name that we didn't get from an explicit end user configuration
(i.e. GIT_*_EMAIL and GIT_*_NAME environment and user.* configuration
variables).


[Footnote]

*1* Inside corporate environments, `whoami`@`hostname -f` might still be a
reasonable and usable default, though.

Re: edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[Adam Megacz]

Junio C Hamano <gitster@pobox.com> writes:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
>> Quoting Adam Megacz <adam@megacz.com>
>>> Perhaps a preference (off by default) demanding that they be set
>>> explicitly when "git commit -m" is used?

>> Sverre pointed out why this won't work.

I agree; making it a preference will not help.

I propose instead that "git commit -e" cause the metadata headers to be
provided to $EDITOR.  People who care about the metadata can simply get
in the habit of always passing that option when invoking "git commit".

> The approach may have been Ok 10 years ago, back when `whoami`@`hostname`,
> at least on systems that were competently maintained, gave a reasonable
> mail address for most people, but I don't think it is adequate anymore to
> majority of people,

I agree.

> So I don't think anybody minds if we refuse to work if we are going to end
> up using a name that we didn't get from an explicit end user configuration
> (i.e. GIT_*_EMAIL and GIT_*_NAME environment and user.* configuration
> variables).

I support that as well, although I'd still like to be shown the data.  I
wear a few different hats (each with its own email address), and I don't
think I want to pick one of them as the default.

Thanks,

  - a

Re: edit Author/Date metadata as part of 'git commit' $EDITOR invocation?

[Junio C Hamano]

Adam Megacz <adam@megacz.com> writes:

> I propose instead that "git commit -e" cause the metadata headers to be
> provided to $EDITOR.  People who care about the metadata can simply get
> in the habit of always passing that option when invoking "git commit".

That is already done by bb1ae3f (commit: Show committer if automatic,
2008-05-04), so there is no need to propose anything.

I see a bit of room for tightening logic in that ancient commit, though.

[PATCH 1/3] ident.c: remove unused variables

[Junio C Hamano]

d5cc2de (ident.c: Trim hint printed when gecos is empty., 2006-11-28)
reworded the message used as printf() format and dropped "%s" from it;
these two variables that hold the names of GIT_{AUTHOR,COMMITTER}_NAME
environment variables haven't been used since then.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This is an independent clean-up

 ident.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/ident.c b/ident.c
index 26409b2..e6c1798 100644
--- a/ident.c
+++ b/ident.c
@@ -168,8 +168,6 @@ static int copy(char *buf, size_t size, int offset, const char *src)
 	return offset;
 }
 
-static const char au_env[] = "GIT_AUTHOR_NAME";
-static const char co_env[] = "GIT_COMMITTER_NAME";
 static const char *env_hint =
 "\n"
 "*** Please tell me who you are.\n"
@@ -204,7 +202,7 @@ const char *fmt_ident(const char *name, const char *email,
 
 		if ((warn_on_no_name || error_on_no_name) &&
 		    name == git_default_name && env_hint) {
-			fprintf(stderr, env_hint, au_env, co_env);
+			fprintf(stderr, env_hint);
 			env_hint = NULL; /* warn only once */
 		}
 		if (error_on_no_name)
-- 
1.6.6.209.g52296.dirty

[PATCH 2/3] ident.c: check explicit identity for name and email separately

[Junio C Hamano]

bb1ae3f (commit: Show committer if automatic, 2008-05-04) added a logic to
check both name and email were given explicitly by the end user, but it
assumed that fmt_ident() is never called before git_default_user_config()
is called, which was fragile.  The former calls setup_ident() and fills
the "default" name and email, so the check in the config parser would have
mistakenly said both are given even if only user.name was provided.

Make the logic more robust by keeping track of name and email separately.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-commit.c |    2 +-
 cache.h          |    3 +++
 config.c         |    6 ++----
 ident.c          |    7 ++++---
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 073fe90..f4974b5 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -624,7 +624,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				author_ident);
 		free(author_ident);
 
-		if (!user_ident_explicitly_given)
+		if (user_ident_explicitly_given != IDENT_ALL_GIVEN)
 			fprintf(fp,
 				"%s"
 				"# Committer: %s\n",
diff --git a/cache.h b/cache.h
index bf468e5..16c8e8d 100644
--- a/cache.h
+++ b/cache.h
@@ -925,6 +925,9 @@ extern const char *config_exclusive_filename;
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
 extern char git_default_name[MAX_GITNAME];
+#define IDENT_NAME_GIVEN 01
+#define IDENT_MAIL_GIVEN 02
+#define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
 extern int user_ident_explicitly_given;
 
 extern const char *git_commit_encoding;
diff --git a/config.c b/config.c
index 37385ce..fa1a0c0 100644
--- a/config.c
+++ b/config.c
@@ -528,8 +528,7 @@ static int git_default_user_config(const char *var, const char *value)
 		if (!value)
 			return config_error_nonbool(var);
 		strlcpy(git_default_name, value, sizeof(git_default_name));
-		if (git_default_email[0])
-			user_ident_explicitly_given = 1;
+		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
 		return 0;
 	}
 
@@ -537,8 +536,7 @@ static int git_default_user_config(const char *var, const char *value)
 		if (!value)
 			return config_error_nonbool(var);
 		strlcpy(git_default_email, value, sizeof(git_default_email));
-		if (git_default_name[0])
-			user_ident_explicitly_given = 1;
+		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 		return 0;
 	}
 
diff --git a/ident.c b/ident.c
index e6c1798..e67c5ad 100644
--- a/ident.c
+++ b/ident.c
@@ -249,9 +249,10 @@ const char *git_author_info(int flag)
 
 const char *git_committer_info(int flag)
 {
-	if (getenv("GIT_COMMITTER_NAME") &&
-	    getenv("GIT_COMMITTER_EMAIL"))
-		user_ident_explicitly_given = 1;
+	if (getenv("GIT_COMMITTER_NAME"))
+		user_ident_explicitly_given |= IDENT_NAME_GIVEN;
+	if (getenv("GIT_COMMITTER_EMAIL"))
+		user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
 	return fmt_ident(getenv("GIT_COMMITTER_NAME"),
 			 getenv("GIT_COMMITTER_EMAIL"),
 			 getenv("GIT_COMMITTER_DATE"),
-- 
1.6.6.209.g52296.dirty

[RFC PATCH 3/3] ident.c: treat $EMAIL as giving user.email identity explicitly

[Junio C Hamano]

The environment variable EMAIL has been honored since 28a94f8 (Fall back
to $EMAIL for missing GIT_AUTHOR_EMAIL and GIT_COMMITTER_EMAIL,
2007-04-28) as the end-user's wish to use the address as the identity.
When we use it, we should say we are explicitly given email by the user.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is an RFC as some people would feel strongly about _not_ using
   $EMAIL as their commit identity and would rather override it explicitly
   with user.email; if they weren't told about git using their $EMAIL,
   they will complain.

 ident.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/ident.c b/ident.c
index e67c5ad..d4f6145 100644
--- a/ident.c
+++ b/ident.c
@@ -85,10 +85,11 @@ static void setup_ident(void)
 	if (!git_default_email[0]) {
 		const char *email = getenv("EMAIL");
 
-		if (email && email[0])
+		if (email && email[0]) {
 			strlcpy(git_default_email, email,
 				sizeof(git_default_email));
-		else {
+			user_ident_explicitly_given |= IDENT_MAIL_GIVEN;
+		} else {
 			if (!pw)
 				pw = getpwuid(getuid());
 			if (!pw)
-- 
1.6.6.209.g52296.dirty

Re: [PATCH 2/3] ident.c: check explicit identity for name and email separately

[Santi Béjar]

On Fri, Jan 8, 2010 at 5:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> bb1ae3f (commit: Show committer if automatic, 2008-05-04) added a logic to
> check both name and email were given explicitly by the end user, but it
> assumed that fmt_ident() is never called before git_default_user_config()
> is called, which was fragile.  The former calls setup_ident() and fills
> the "default" name and email, so the check in the config parser would have
> mistakenly said both are given even if only user.name was provided.
>
> Make the logic more robust by keeping track of name and email separately.
>

It's a good improvement, thanks.

Acked-by: Santi Béjar <santi@agolina.net>

Santi

[PATCH] Display author and committer after "git commit"

[Adam Megacz]

Display author (name, email, date) and committer (name, email, date)
after creating a new commit to ensure that the user is alerted in the
event that they are set in an undesirable manner.

This patch seeks to accomplish the following goal: all data included
in the commit which are sha1-protected (and therefore immutable) are
either taken from the working tree or else displayed to the user for
sanity checking purposes.  Since the author/committer information is
immutable and not taken from the working tree, achieving the goal
above requires printing out the author/committer.  The short window of
time after committing a patch and before propagating it is the last
opportunity to modify the data (by deleting and recreating the commit).

This patch is not necessarily meant for inclusion verbatim; it's more
of a starting point for discussion.
---
 commit.h   |    2 ++
 log-tree.c |   15 +++++++++++++++
 pretty.c   |   23 ++++++++++++++++++-----
 3 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/commit.h b/commit.h
index e5332ef..e4222b0 100644
--- a/commit.h
+++ b/commit.h
@@ -59,6 +59,8 @@ enum cmit_fmt {
 	CMIT_FMT_ONELINE,
 	CMIT_FMT_EMAIL,
 	CMIT_FMT_USERFORMAT,
+        CMIT_FMT_COMMITTER_AND_DATE,
+        CMIT_FMT_AUTHOR_AND_DATE,
 
 	CMIT_FMT_UNSPECIFIED,
 };
diff --git a/log-tree.c b/log-tree.c
index 0fdf159..7b399b8 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -160,6 +160,20 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 	strbuf_addch(sb, '\n');
 }
 
+static void append_metadata(struct strbuf *sb,
+                            struct commit *commit,
+                            const struct pretty_print_context *ctx)
+{
+
+	strbuf_addch(sb, '\n');
+	strbuf_addstr(sb, " Author:     ");
+        pretty_print_commit(CMIT_FMT_AUTHOR_AND_DATE, commit, sb, ctx);
+
+	strbuf_addch(sb, '\n');
+	strbuf_addstr(sb, " Committer:  ");
+        pretty_print_commit(CMIT_FMT_COMMITTER_AND_DATE, commit, sb, ctx);
+}
+
 static unsigned int digits_in_number(unsigned int number)
 {
 	unsigned int i = 10, result = 1;
@@ -414,6 +428,7 @@ void show_log(struct rev_info *opt)
 	ctx.reflog_info = opt->reflog_info;
 	pretty_print_commit(opt->commit_format, commit, &msgbuf, &ctx);
 
+        append_metadata(&msgbuf, commit, &ctx);
 	if (opt->add_signoff)
 		append_signoff(&msgbuf, opt->add_signoff);
 	if (opt->show_log_size) {
diff --git a/pretty.c b/pretty.c
index 8f5bd1a..2458509 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1028,16 +1028,26 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	int need_8bit_cte = context->need_8bit_cte;
 
 	if (fmt == CMIT_FMT_USERFORMAT) {
-		format_commit_message(commit, user_format, sb, context);
+                format_commit_message(commit, user_format, sb, context);
 		return;
 	}
+        if (fmt == CMIT_FMT_COMMITTER_AND_DATE) {
+                format_commit_message(commit, "%cn <%ce> %cd", sb, context);
+                return;
+        }
+        if (fmt == CMIT_FMT_AUTHOR_AND_DATE) {
+                format_commit_message(commit, "%an <%ae> %ad", sb, context);
+                return;
+        }
 
 	reencoded = reencode_commit_message(commit, &encoding);
 	if (reencoded) {
 		msg = reencoded;
 	}
 
-	if (fmt == CMIT_FMT_ONELINE || fmt == CMIT_FMT_EMAIL)
+
+	if (fmt == CMIT_FMT_ONELINE || fmt == CMIT_FMT_EMAIL ||
+            fmt == CMIT_FMT_COMMITTER_AND_DATE || CMIT_FMT_AUTHOR_AND_DATE)
 		indent = 0;
 
 	/*
@@ -1078,12 +1088,14 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 			      context->after_subject, encoding, need_8bit_cte);
 
 	beginning_of_body = sb->len;
-	if (fmt != CMIT_FMT_ONELINE)
+	if (fmt != CMIT_FMT_ONELINE &&
+            fmt != CMIT_FMT_COMMITTER_AND_DATE && fmt != CMIT_FMT_AUTHOR_AND_DATE)
 		pp_remainder(fmt, &msg, sb, indent);
 	strbuf_rtrim(sb);
 
 	/* Make sure there is an EOLN for the non-oneline case */
-	if (fmt != CMIT_FMT_ONELINE)
+	if (fmt != CMIT_FMT_ONELINE &&
+            fmt != CMIT_FMT_COMMITTER_AND_DATE && fmt != CMIT_FMT_AUTHOR_AND_DATE)
 		strbuf_addch(sb, '\n');
 
 	/*
@@ -1094,7 +1106,8 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
 	if (fmt == CMIT_FMT_EMAIL && sb->len <= beginning_of_body)
 		strbuf_addch(sb, '\n');
 
-	if (fmt != CMIT_FMT_ONELINE)
+	if (fmt != CMIT_FMT_ONELINE &&
+            fmt != CMIT_FMT_COMMITTER_AND_DATE && fmt != CMIT_FMT_AUTHOR_AND_DATE)
 		get_commit_notes(commit, sb, encoding,
 				 NOTES_SHOW_HEADER | NOTES_INDENT);
 
-- 
1.6.4.4

Re: [PATCH] Display author and committer after "git commit"

[Adam Megacz]

Adam Megacz <adam@megacz.com> writes:
> either taken from the working tree or else displayed to the user for
> sanity checking purposes.  Since the author/committer information is
> immutable and not taken from the working tree,

s/working tree/index/g

Sorry,

  - a

Re: [PATCH] Display author and committer after "git commit"

[Junio C Hamano]

Adam Megacz <adam@megacz.com> writes:

> Display author (name, email, date) and committer (name, email, date)
> after creating a new commit to ensure that the user is alerted in the
> event that they are set in an undesirable manner.

Too much clutter for too little gain, except for a very first few commits
in the repository.

Why isn't the "# Author:" and "# Committer:" information you see along
with "git status" output in the editor "git commit" gives you sufficient
if it is to avoid unconfigured/misconfigured names and e-mail addresses?

> This patch is not necessarily meant for inclusion verbatim;
> ...
> diff --git a/pretty.c b/pretty.c
> index 8f5bd1a..2458509 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1028,16 +1028,26 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
>  	int need_8bit_cte = context->need_8bit_cte;
>  
>  	if (fmt == CMIT_FMT_USERFORMAT) {
> -		format_commit_message(commit, user_format, sb, context);
> +                format_commit_message(commit, user_format, sb, context);

Of course it isn't, with a change like this ;-)

Indentation damages aside, there is a lot more serious issue with this.
You added this cruft *unconditionally* to show_log().  Doesn't it mean
that you made format-patch *unusable*?  Try:

	git am your_patch.mbox
        make install
	for i in 1 2 3 4 5
        do
                git format-patch -1 --stdout >patch.mbox
                git reset --hard HEAD^
                git am patch.mbox
	done        
	git show -s

and weep.

Have you checked --pretty=fuller, by the way?

Re: [PATCH] Display author and committer after "git commit"

[Adam Megacz]

Junio C Hamano <gitster@pobox.com> writes:
> Why isn't the "# Author:" and "# Committer:" information you see along
> with "git status" output in the editor "git commit" gives you sufficient
> if it is to avoid unconfigured/misconfigured names and e-mail
> addresses?

It is sufficient!  But, as others have mentioned, it is not displayed
when "git commit -m" is used.  The patch in this thread rectifies that
omission.

> Too much clutter for too little gain, except for a very first few commits
> in the repository.

Funny, I think of those "+++" "-----" histogram things as clutter.  I
guess it's subjective.

  - a

Re: [PATCH] Display author and committer after "git commit"

[Jeff King]

On Tue, Jan 12, 2010 at 01:51:51AM +0000, Adam Megacz wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> > Why isn't the "# Author:" and "# Committer:" information you see along
> > with "git status" output in the editor "git commit" gives you sufficient
> > if it is to avoid unconfigured/misconfigured names and e-mail
> > addresses?
> 
> It is sufficient!  But, as others have mentioned, it is not displayed
> when "git commit -m" is used.  The patch in this thread rectifies that
> omission.

I think it is sensible to reiterate the information in the summary for
the "interesting" cases, as it does make it available to people who do
not see the template, and as the uncommon case, is not usually
cluttering the output.

But I don't understand why the original patch needed to touch anything
outside of builtin-commit.c:print_summary. Something like this should
work (though see below for why it isn't ready for inclusion):

diff --git a/builtin-commit.c b/builtin-commit.c
index 1e353f6..6ee6b10 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1054,9 +1054,12 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
 	struct commit *commit;
-	static const char *format = "format:%h] %s";
+	struct strbuf format = STRBUF_INIT;
 	unsigned char junk_sha1[20];
 	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+	struct pretty_print_context pctx = {0};
+	struct strbuf author_ident = STRBUF_INIT;
+	struct strbuf committer_ident = STRBUF_INIT;
 
 	commit = lookup_commit(sha1);
 	if (!commit)
@@ -1064,6 +1067,22 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	if (!commit || parse_commit(commit))
 		die("could not parse newly created commit");
 
+	strbuf_addstr(&format, "format:%h] %s");
+
+	format_commit_message(commit, "%an <%ae>", &author_ident, &pctx);
+	format_commit_message(commit, "%cn <%ce>", &committer_ident, &pctx);
+	if (strbuf_cmp(&author_ident, &committer_ident)) {
+		int i;
+		strbuf_addstr(&format, "\n Author: ");
+		for (i = 0; i < author_ident.len; i++) {
+			if (author_ident.buf[i] == '%')
+				strbuf_addch(&format, '%');
+			strbuf_addch(&format, author_ident.buf[i]);
+		}
+	}
+	strbuf_release(&author_ident);
+	strbuf_release(&committer_ident);
+
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 
@@ -1074,7 +1093,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
-	get_commit_format(format, &rev);
+	get_commit_format(format.buf, &rev);
+	strbuf_release(&format);
 	rev.always_show_header = 0;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
@@ -1093,7 +1113,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		struct pretty_print_context ctx = {0};
 		struct strbuf buf = STRBUF_INIT;
 		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format + 7, &buf, &ctx);
+		format_commit_message(commit, format.buf + 7, &buf, &ctx);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}

It's not appropriate for inclusion because:

  - I didn't actually test it beyond "GIT_AUTHOR_NAME=Foo git commit
    -m". I think what the code does is correct, but it may be breaking
    output in the test suite.

  - It tries to quote any percents in the author name, but user formats
    don't actually have a quoting mechanism! Probably we should
    interpret "%%" as "%". Even though it's a behavior change, I
    consider the current behavior buggy.

    Side note: it feels a little hack-ish that I have to actually use a
    user-format to get the author and committer. But we don't seem to
    have any infrastructure for something as simple as "give me a string
    with the author name of this commit".

  - It only handles author != committer as interesting. We should also
    check user_ident_explicitly_given and show the committer in that
    case, as the editor template does.

-Peff

Re: [PATCH] Display author and committer after "git commit"

[Jeff King]

On Tue, Jan 12, 2010 at 09:24:06AM -0500, Jeff King wrote:

>   - It tries to quote any percents in the author name, but user formats
>     don't actually have a quoting mechanism! Probably we should
>     interpret "%%" as "%". Even though it's a behavior change, I
>     consider the current behavior buggy.

Actually, on second thought, they do: you can use %x25 to get the same
effect. I still think we should support '%%' as a more readable and
expected alternative (this is how printf works, and daemon.c's expansion
already does this).

-Peff

Re: [PATCH] Display author and committer after "git commit"

[Jeff King]

On Tue, Jan 12, 2010 at 09:24:05AM -0500, Jeff King wrote:

> But I don't understand why the original patch needed to touch anything
> outside of builtin-commit.c:print_summary. Something like this should
> work (though see below for why it isn't ready for inclusion):

Well, I had originally meant to send this off and try to convince Adam
to fix it up for inclusion, but it seemed easy enough so I just went
ahead and did it.

  [1/3]: strbuf_expand: convert "%%" to "%"
  [2/3]: strbuf: add strbuf_percentquote_buf
  [3/3]: commit: show interesting ident information in summary

-Peff

[PATCH 1/3] strbuf_expand: convert "%%" to "%"

[Jeff King]

The only way to safely quote arbitrary text in a
pretty-print user format is to replace instances of "%" with
"%x25". This is slightly unreadable, and many users would
expect "%%" to produce a single "%", as that is what printf
format specifiers do.

This patch converts "%%" to "%" for all users of
strbuf_expand:

 1. git-daemon interpolated paths

 2. pretty-print user formats

 3. merge driver command lines

Case (1) was already doing the conversion itself outside of
strbuf_expand. Case (2) is the intended beneficiary of this
patch. Case (3) users probably won't notice, but as this is
user-facing behavior, consistently providing the quoting
mechanism makes sense.

Signed-off-by: Jeff King <peff@peff.net>
---
Because of the %x25 thing, this isn't strictly necessary for my series.
I do think it's the right thing to do, though. If you want to drop it
because of the user-visible behavior change, I can re-roll the rest of
my series around %x25 quoting (though it makes the helper in 2/3 less
useful, as the quoting doesn't work for printf anymore).

 Documentation/pretty-formats.txt |    1 +
 daemon.c                         |    1 -
 strbuf.c                         |    6 ++++++
 t/t6006-rev-list-format.sh       |    7 +++++++
 4 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 53a9168..1686a54 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -134,6 +134,7 @@ The placeholders are:
 - '%C(...)': color specification, as described in color.branch.* config option
 - '%m': left, right or boundary mark
 - '%n': newline
+- '%%': a raw '%'
 - '%x00': print a byte from a hex code
 - '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of
   linkgit:git-shortlog[1].
diff --git a/daemon.c b/daemon.c
index 918e560..360635e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -147,7 +147,6 @@ static char *path_ok(char *directory)
 			{ "IP", ip_address },
 			{ "P", tcp_port },
 			{ "D", directory },
-			{ "%", "%" },
 			{ NULL }
 		};
 
diff --git a/strbuf.c b/strbuf.c
index a6153dc..6cbc1fc 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -227,6 +227,12 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 			break;
 		format = percent + 1;
 
+		if (*format == '%') {
+			strbuf_addch(sb, '%');
+			format++;
+			continue;
+		}
+
 		consumed = fn(sb, format, context);
 		if (consumed)
 			format += consumed;
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 5719315..b0047d3 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -19,6 +19,13 @@ test_cmp expect.$1 output.$1
 "
 }
 
+test_format percent %%h <<'EOF'
+commit 131a310eb913d107dd3c09a65d1651175898735d
+%h
+commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
+%h
+EOF
+
 test_format hash %H%n%h <<'EOF'
 commit 131a310eb913d107dd3c09a65d1651175898735d
 131a310eb913d107dd3c09a65d1651175898735d
-- 
1.6.6.138.g309fc.dirty

[PATCH 2/3] strbuf: add strbuf_percentquote_buf

[Jeff King]

This is handy for creating strings which will be fed to
strbuf_expand or printf.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-strbuf.txt |    7 +++++++
 strbuf.c                               |   10 ++++++++++
 strbuf.h                               |    1 +
 3 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index a0e0f85..d5ae3b0 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -214,6 +214,13 @@ which can be used by the programmer of the callback as she sees fit.
 	placeholder and replacement string.  The array needs to be
 	terminated by an entry with placeholder set to NULL.
 
+`strbuf_percentquote_buf`::
+
+	Append the contents of one strbuf to another, quoting any
+	percent signs ("%") into double-percents ("%%") in the
+	destination. This is useful for literal data to be fed to either
+	strbuf_expand or to the *printf family of functions.
+
 `strbuf_addf`::
 
 	Add a formatted string to the buffer.
diff --git a/strbuf.c b/strbuf.c
index 6cbc1fc..b5183c6 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -257,6 +257,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 	return 0;
 }
 
+void strbuf_percentquote_buf(struct strbuf *dest, struct strbuf *src)
+{
+	int i;
+	for (i = 0; i < src->len; i++) {
+		if (src->buf[i] == '%')
+			strbuf_addch(dest, '%');
+		strbuf_addch(dest, src->buf[i]);
+	}
+}
+
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
diff --git a/strbuf.h b/strbuf.h
index fa07ecf..f6bf055 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -116,6 +116,7 @@ struct strbuf_expand_dict_entry {
 	const char *value;
 };
 extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);
+extern void strbuf_percentquote_buf(struct strbuf *dest, struct strbuf *src);
 
 __attribute__((format (printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
-- 
1.6.6.138.g309fc.dirty

[PATCH 3/3] commit: show interesting ident information in summary

[Jeff King]

There are a few cases of user identity information that we
consider interesting:

  1. When the author and committer identities do not match.

  2. When the committer identity was picked automatically
     from the username, hostname and GECOS information.

In these cases, we already show the information in the
commit message template. However, users do not always see
that template because they might use "-m" or "-F". With this
patch, we show these interesting cases after the commit,
along with the subject and change summary. The new output
looks like:

  $ git commit \
      -m "federalist papers" \
      --author='Publius <alexander@hamilton.com>'
  [master 3d226a7] federalist papers
   Author: Publius <alexander@hamilton.com>
   1 files changed, 1 insertions(+), 0 deletions(-)

for case (1), and:

  $ git config --global --unset user.name
  $ git config --global --unset user.email
  $ git commit -m foo
  [master 7c2a927] foo
   Committer: Jeff King <peff@c-71-185-130-222.hsd1.va.comcast.net>
   1 files changed, 1 insertions(+), 0 deletions(-)

for case (2).

Signed-off-by: Jeff King <peff@peff.net>
---
Note that this has a slight semantic conflict with the jc/ident topic in
next. The user_ident_explicitly_given flag needs to be compared to
IDENT_ALL.

I hope the example output in the commit message is not too verbose. I
was recently reviewing somebody's series that made output changes, and
they didn't include sample output anywhere, which made reviewing a lot
more annoying.

Personally I don't care much about case (2) one way or the other, but it
is the one that triggered this thread. I think case (1) is very useful,
though.

I tested case (2) manually, but I didn't include anything in the test
suite; I feel funny testing output created from the hostname and GECOS
(can't it even barf if the user's system isn't set up very well? That
would produce a false negative for the test).

 builtin-commit.c  |   25 ++++++++++++++++++++++---
 t/t7501-commit.sh |    6 +++++-
 2 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 073fe90..279145d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1046,9 +1046,12 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
 	struct commit *commit;
-	static const char *format = "format:%h] %s";
+	struct strbuf format = STRBUF_INIT;
 	unsigned char junk_sha1[20];
 	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+	struct pretty_print_context pctx = {0};
+	struct strbuf author_ident = STRBUF_INIT;
+	struct strbuf committer_ident = STRBUF_INIT;
 
 	commit = lookup_commit(sha1);
 	if (!commit)
@@ -1056,6 +1059,21 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	if (!commit || parse_commit(commit))
 		die("could not parse newly created commit");
 
+	strbuf_addstr(&format, "format:%h] %s");
+
+	format_commit_message(commit, "%an <%ae>", &author_ident, &pctx);
+	format_commit_message(commit, "%cn <%ce>", &committer_ident, &pctx);
+	if (strbuf_cmp(&author_ident, &committer_ident)) {
+		strbuf_addstr(&format, "\n Author: ");
+		strbuf_percentquote_buf(&format, &author_ident);
+	}
+	if (!user_ident_explicitly_given) {
+		strbuf_addstr(&format, "\n Committer: ");
+		strbuf_percentquote_buf(&format, &committer_ident);
+	}
+	strbuf_release(&author_ident);
+	strbuf_release(&committer_ident);
+
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 
@@ -1066,7 +1084,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
-	get_commit_format(format, &rev);
+	get_commit_format(format.buf, &rev);
+	strbuf_release(&format);
 	rev.always_show_header = 0;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
@@ -1085,7 +1104,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		struct pretty_print_context ctx = {0};
 		struct strbuf buf = STRBUF_INIT;
 		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format + 7, &buf, &ctx);
+		format_commit_message(commit, format.buf + 7, &buf, &ctx);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index a529701..7940901 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -117,7 +117,11 @@ test_expect_success \
 test_expect_success \
 	"overriding author from command line" \
 	"echo 'gak' >file && \
-	 git commit -m 'author' --author 'Rubber Duck <rduck@convoy.org>' -a"
+	 git commit -m 'author' --author 'Rubber Duck <rduck@convoy.org>' -a >output 2>&1"
+
+test_expect_success \
+	"commit --author output mentions author" \
+	"grep Rubber.Duck output"
 
 test_expect_success PERL \
 	"interactive add" \
-- 
1.6.6.138.g309fc.dirty

Re: [PATCH 2/3] strbuf: add strbuf_percentquote_buf

[Jeff King]

On Tue, Jan 12, 2010 at 05:19:21PM +0100, Johannes Schindelin wrote:

> > This is handy for creating strings which will be fed to
> > strbuf_expand or printf.
> 
> For printf(), there is always %s%s, so I would not say your patch is 
> useful there, but rather adds churn: first you add a percent, then you 
> strip it away again.

True. It is only useful in either case if you are going to pass the
format specifier through an API that does all of its work at once (e.g.,
in this instance, I would be happy to simply output my strings at the
right moment, but I need to get them _between_ the log format and the
diff summary, which means I need to hide them in the log format
specifier). That tends not to happen with printf-style strings, since we
don't build complex APIs around them.

-Peff

Re: [PATCH 2/3] strbuf: add strbuf_percentquote_buf

[Johannes Schindelin]

Hi,

On Tue, 12 Jan 2010, Jeff King wrote:

> This is handy for creating strings which will be fed to
> strbuf_expand or printf.

For printf(), there is always %s%s, so I would not say your patch is 
useful there, but rather adds churn: first you add a percent, then you 
strip it away again.

Ciao,
Dscho

Re: [PATCH 2/3] strbuf: add strbuf_percentquote_buf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +`strbuf_percentquote_buf`::
> +
> +	Append the contents of one strbuf to another, quoting any
> +	percent signs ("%") into double-percents ("%%") in the
> +	destination. This is useful for literal data to be fed to either
> +	strbuf_expand or to the *printf family of functions.
> +
>  `strbuf_addf`::
>  
>  	Add a formatted string to the buffer.
> diff --git a/strbuf.c b/strbuf.c
> index 6cbc1fc..b5183c6 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -257,6 +257,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
>  	return 0;
>  }
>  
> +void strbuf_percentquote_buf(struct strbuf *dest, struct strbuf *src)
> +{

Just a style thing, but please call that "dst" to be consistent.  You are
already dropping vowels from the other side to spell it "src".

I wondered if the function should be just 1-arg that always quotes
in-place instead, but your [PATCH 3/3] wants to have an appending
semantics from this function, so changing it to be a 1-arg "in-place
quoter" will force the caller to run strbuf_addbuf() on the result, which
is not nice.

Since tucking a p-quoted version of the same string to its original
doesn't make sense at all, perhaps this should:

 (0) be renamed to have "append" somewhere in its name;

 (1) mark the src side as const; and

 (2) perhaps have assert(dst != src).  The loop won't terminate when
     called with src == dst, I think.

There seems to be only one other strbuf function that takes two strbufs in
the suite (strbuf_addbuf), and I think it is unsafe in a different way,
which is trivial to fix.

-- >8 --

Subject: [PATCH] strbuf_addbuf(): allow passing the same buf to dst and src

If sb and sb2 are the same (i.e. doubling the string), the underlying
strbuf_add() will make sb2->buf invalid by calling strbuf_grow(sb) at
the beginning and will read from the freed buffer.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 strbuf.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index fa07ecf..e272359 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -105,7 +105,13 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
 	strbuf_add(sb, s, strlen(s));
 }
 static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) {
-	strbuf_add(sb, sb2->buf, sb2->len);
+	char *buf = sb2->buf;
+	int len = sb2->len;
+	if (sb->buf == sb2->buf) {
+		strbuf_grow(sb, len);
+		buf = sb->buf;
+	}
+	strbuf_add(sb, buf, len);
 }
 extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
 
-- 
1.6.6.280.ge295b7.dirty

Re: [PATCH 3/3] commit: show interesting ident information in summary

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +	if (strbuf_cmp(&author_ident, &committer_ident)) {
> +		strbuf_addstr(&format, "\n Author: ");
> +		strbuf_percentquote_buf(&format, &author_ident);
> +	}
> +	if (!user_ident_explicitly_given) {
> +		strbuf_addstr(&format, "\n Committer: ");
> +		strbuf_percentquote_buf(&format, &committer_ident);
> +	}

This is much better.

We might want an advice message inside the latter case, helping the user
learn how to spell his name correctly.  This is designed to trigger for
people/repositories that are not configured, and by definition the
majority of that target audience are new people.

The extra message will disappear once committer information is explicitly
given, there is no need to protect the advice message with the usual
"advice.*" configuration.

Thanks.

Re: [PATCH 2/3] strbuf: add strbuf_percentquote_buf

[Jeff King]

On Tue, Jan 12, 2010 at 10:55:45PM -0800, Junio C Hamano wrote:

> > +void strbuf_percentquote_buf(struct strbuf *dest, struct strbuf *src)
> > +{
> 
> Just a style thing, but please call that "dst" to be consistent.  You are
> already dropping vowels from the other side to spell it "src".

I personally dislike that spelling, but it certainly is consistent with
the rest of git, so OK.

> I wondered if the function should be just 1-arg that always quotes
> in-place instead, but your [PATCH 3/3] wants to have an appending
> semantics from this function, so changing it to be a 1-arg "in-place
> quoter" will force the caller to run strbuf_addbuf() on the result, which
> is not nice.

Yep. An in-place version would be a bit more complicated to write, and
would make the caller do extra work.

> Since tucking a p-quoted version of the same string to its original
> doesn't make sense at all, perhaps this should:
> 
>  (0) be renamed to have "append" somewhere in its name;

Yeah, I considered this. To follow the existing naming conventions, the
name should indicate:

  1. it's a strbuf function
  2. it's appending (and the pattern is to use "add")
  3. it's appending a strbuf (and the pattern is to call this "buf")
  4. it's percent-quoting

So perhaps following the existing standards, it should be
strbuf_addbuf_percentquote? Long, but I don't think there is any
confusion about what it does (and leaves room for addstr_percentquote).

>  (1) mark the src side as const; and

Oops, good catch.

>  (2) perhaps have assert(dst != src).  The loop won't terminate when
>      called with src == dst, I think.

Oops again. I think it is sensible to protect against this. I thought
about trying to make it magically work in-place, but I don't think there
is a simple way to do so. And since I don't actually need to do that, I
think leaving an assert in-place until somebody does need it and wants
to write it is fine.

> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -105,7 +105,13 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
>  	strbuf_add(sb, s, strlen(s));
>  }
>  static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) {
> -	strbuf_add(sb, sb2->buf, sb2->len);
> +	char *buf = sb2->buf;
> +	int len = sb2->len;
> +	if (sb->buf == sb2->buf) {
> +		strbuf_grow(sb, len);
> +		buf = sb->buf;
> +	}
> +	strbuf_add(sb, buf, len);
>  }

Shouldn't this be "if (sb == sb2)"? Two strbufs in the initial state
will point to the same strbuf_slopbuf, but obviously growing sb will not
impact sb2. Though that would simply provoke a false positive, which I
don't think has any negative consequences.

Also, since reallocating sb will reallocate sb2, can't you just write it
safely like this:

  strbuf_grow(sb, sb2->len);
  strbuf_add(sb, sb2->buf, sb2->len);

The grow will not affect the length of sb2, so that doesn't need to be
saved. And there is no point in deciding whether to point the buf you
pass at sb->buf or sb2->buf. If they are the same, then the grow will
have reallocated sb2 as well as sb, and they are identical. And if they
are not, then sb2->buf is the right thing to pass.

-Peff

Re: [PATCH 3/3] commit: show interesting ident information in summary

[Jeff King]

On Tue, Jan 12, 2010 at 10:57:03PM -0800, Junio C Hamano wrote:

> > +	if (!user_ident_explicitly_given) {
> > +		strbuf_addstr(&format, "\n Committer: ");
> > +		strbuf_percentquote_buf(&format, &committer_ident);
> > +	}
> 
> This is much better.
> 
> We might want an advice message inside the latter case, helping the user
> learn how to spell his name correctly.  This is designed to trigger for
> people/repositories that are not configured, and by definition the
> majority of that target audience are new people.
> 
> The extra message will disappear once committer information is explicitly
> given, there is no need to protect the advice message with the usual
> "advice.*" configuration.

Just adding the "Committer:" reminder is slightly annoying (though
perhaps some people will even like it). Adding a big advice message on
every commit is going to be annoying to everyone who sees it, and is
really crossing the line of "we don't really support implicit identities
anymore", since anyone seeing it is going to want to fix it.

I know there has been some discussion of that area in the last few
months, but I admit I didn't pay any attention. Is that the direction we
want to move in? I don't have a particular problem with it, but I want
to point out that if there _are_ people who really like the implicit
ident feature, we are effectively killing it off for them.

-Peff

Re: [PATCH] Display author and committer after "git commit"

[Jeff King]

On Tue, Jan 12, 2010 at 10:36:56AM -0500, Jeff King wrote:

>   [1/3]: strbuf_expand: convert "%%" to "%"
>   [2/3]: strbuf: add strbuf_percentquote_buf
>   [3/3]: commit: show interesting ident information in summary

And here's a re-roll based on Junio's comments.

  [1/3]: strbuf_expand: convert "%%" to "%"
  [2/3]: strbuf: add strbuf_addbuf_percentquote
  [3/3]: commit: show interesting ident information in summary

-Peff

[PATCH v2 1/3] strbuf_expand: convert "%%" to "%"

[Jeff King]

The only way to safely quote arbitrary text in a
pretty-print user format is to replace instances of "%" with
"%x25". This is slightly unreadable, and many users would
expect "%%" to produce a single "%", as that is what printf
format specifiers do.

This patch converts "%%" to "%" for all users of
strbuf_expand:

 1. git-daemon interpolated paths

 2. pretty-print user formats

 3. merge driver command lines

Case (1) was already doing the conversion itself outside of
strbuf_expand. Case (2) is the intended beneficiary of this
patch. Case (3) users probably won't notice, but as this is
user-facing behavior, consistently providing the quoting
mechanism makes sense.

Signed-off-by: Jeff King <peff@coredump.intra.peff.net>
---
Changes from v1:
  - note change in strbuf api docs

 Documentation/pretty-formats.txt       |    1 +
 Documentation/technical/api-strbuf.txt |    4 ++++
 daemon.c                               |    1 -
 strbuf.c                               |    6 ++++++
 t/t6006-rev-list-format.sh             |    7 +++++++
 5 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 53a9168..1686a54 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -134,6 +134,7 @@ The placeholders are:
 - '%C(...)': color specification, as described in color.branch.* config option
 - '%m': left, right or boundary mark
 - '%n': newline
+- '%%': a raw '%'
 - '%x00': print a byte from a hex code
 - '%w([<w>[,<i1>[,<i2>]]])': switch line wrapping, like the -w option of
   linkgit:git-shortlog[1].
diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index a0e0f85..3b1da10 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -199,6 +199,10 @@ character if the letter `n` appears after a `%`.  The function returns
 the length of the placeholder recognized and `strbuf_expand()` skips
 over it.
 +
+The format `%%` is automatically expanded to a single `%` as a quoting
+mechanism; callers do not need to handle the `%` placeholder themselves,
+and the callback function will not be invoked for this placeholder.
++
 All other characters (non-percent and not skipped ones) are copied
 verbatim to the strbuf.  If the callback returned zero, meaning that the
 placeholder is unknown, then the percent sign is copied, too.
diff --git a/daemon.c b/daemon.c
index 918e560..360635e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -147,7 +147,6 @@ static char *path_ok(char *directory)
 			{ "IP", ip_address },
 			{ "P", tcp_port },
 			{ "D", directory },
-			{ "%", "%" },
 			{ NULL }
 		};
 
diff --git a/strbuf.c b/strbuf.c
index a6153dc..6cbc1fc 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -227,6 +227,12 @@ void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn,
 			break;
 		format = percent + 1;
 
+		if (*format == '%') {
+			strbuf_addch(sb, '%');
+			format++;
+			continue;
+		}
+
 		consumed = fn(sb, format, context);
 		if (consumed)
 			format += consumed;
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index 5719315..b0047d3 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -19,6 +19,13 @@ test_cmp expect.$1 output.$1
 "
 }
 
+test_format percent %%h <<'EOF'
+commit 131a310eb913d107dd3c09a65d1651175898735d
+%h
+commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
+%h
+EOF
+
 test_format hash %H%n%h <<'EOF'
 commit 131a310eb913d107dd3c09a65d1651175898735d
 131a310eb913d107dd3c09a65d1651175898735d
-- 
1.6.6.140.g92e4d.dirty

[PATCH v2 2/3] strbuf: add strbuf_addbuf_percentquote

[Jeff King]

This is handy for creating strings which will be fed to
strbuf_expand or printf.

Signed-off-by: Jeff King <peff@coredump.intra.peff.net>
---
Changes since v1:
  - better name
  - style: s/dest/dst/
  - const src
  - no infinite loop for src == dst

 Documentation/technical/api-strbuf.txt |    7 +++++++
 strbuf.c                               |   11 +++++++++++
 strbuf.h                               |    1 +
 3 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index 3b1da10..afe2759 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -218,6 +218,13 @@ which can be used by the programmer of the callback as she sees fit.
 	placeholder and replacement string.  The array needs to be
 	terminated by an entry with placeholder set to NULL.
 
+`strbuf_addbuf_percentquote`::
+
+	Append the contents of one strbuf to another, quoting any
+	percent signs ("%") into double-percents ("%%") in the
+	destination. This is useful for literal data to be fed to either
+	strbuf_expand or to the *printf family of functions.
+
 `strbuf_addf`::
 
 	Add a formatted string to the buffer.
diff --git a/strbuf.c b/strbuf.c
index 6cbc1fc..0c46054 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -257,6 +257,17 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
 	return 0;
 }
 
+void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src)
+{
+	int i;
+	assert(dst != src);
+	for (i = 0; i < src->len; i++) {
+		if (src->buf[i] == '%')
+			strbuf_addch(dst, '%');
+		strbuf_addch(dst, src->buf[i]);
+	}
+}
+
 size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 {
 	size_t res;
diff --git a/strbuf.h b/strbuf.h
index fa07ecf..84ac942 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -116,6 +116,7 @@ struct strbuf_expand_dict_entry {
 	const char *value;
 };
 extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);
+extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);
 
 __attribute__((format (printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
-- 
1.6.6.140.g92e4d.dirty

[PATCH v2 3/3] commit: show interesting ident information in summary

[Jeff King]

There are a few cases of user identity information that we
consider interesting:

  1. When the author and committer identities do not match.

  2. When the committer identity was picked automatically
     from the username, hostname and GECOS information.

In these cases, we already show the information in the
commit message template. However, users do not always see
that template because they might use "-m" or "-F". With this
patch, we show these interesting cases after the commit,
along with the subject and change summary. The new output
looks like:

  $ git commit \
      -m "federalist papers" \
      --author='Publius <alexander@hamilton.com>'
  [master 3d226a7] federalist papers
   Author: Publius <alexander@hamilton.com>
   1 files changed, 1 insertions(+), 0 deletions(-)

for case (1), and:

  $ git config --global --unset user.name
  $ git config --global --unset user.email
  $ git commit -m foo
  [master 7c2a927] foo
   Committer: Jeff King <peff@c-71-185-130-222.hsd1.va.comcast.net>
  Your name and email address were configured automatically based
  on your username and hostname. Please check that they are accurate.
  You can suppress this message by setting them explicitly:

      git config --global user.name Your Name
      git config --global user.email you@example.com

  If the identity used for this commit is wrong, you can fix it with:

      git commit --amend --author='Your Name <you@example.com>'

   1 files changed, 1 insertions(+), 0 deletions(-)

for case (2).

Signed-off-by: Jeff King <peff@coredump.intra.peff.net>
---
Changes since v1:
 - rebase on v2 2/3 for function name change
 - gigantic warning message

I have mixed feelings on the warning message, as I mentioned elsewhere
in the thread.

Also, if you run the "commit --amend" advice immediately (without fixing
your config), you will still have a bogus committer field. Alternate
advice could be:

  If the identity used for this commit is wrong, you can fix it (after
  having set your identity as above) with:

    git commit --amend --reset-author

I dunno which is better. I went with what I did because it is something
the user can immediately do to fix this commit before they forget (of
course, they probably would just run the "git config" commands,
immediately, too...).

 builtin-commit.c  |   39 ++++++++++++++++++++++++++++++++++++---
 t/t7501-commit.sh |    6 +++++-
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 073fe90..3fa9b39 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -36,6 +36,18 @@ static const char * const builtin_status_usage[] = {
 	NULL
 };
 
+static const char implicit_ident_advice[] =
+"Your name and email address were configured automatically based\n"
+"on your username and hostname. Please check that they are accurate.\n"
+"You can suppress this message by setting them explicitly:\n"
+"\n"
+"    git config --global user.name Your Name\n"
+"    git config --global user.email you@example.com\n"
+"\n"
+"If the identity used for this commit is wrong, you can fix it with:\n"
+"\n"
+"    git commit --amend --author='Your Name <you@example.com>'\n";
+
 static unsigned char head_sha1[20];
 static char *use_message_buffer;
 static const char commit_editmsg[] = "COMMIT_EDITMSG";
@@ -1046,9 +1058,12 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 {
 	struct rev_info rev;
 	struct commit *commit;
-	static const char *format = "format:%h] %s";
+	struct strbuf format = STRBUF_INIT;
 	unsigned char junk_sha1[20];
 	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+	struct pretty_print_context pctx = {0};
+	struct strbuf author_ident = STRBUF_INIT;
+	struct strbuf committer_ident = STRBUF_INIT;
 
 	commit = lookup_commit(sha1);
 	if (!commit)
@@ -1056,6 +1071,23 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	if (!commit || parse_commit(commit))
 		die("could not parse newly created commit");
 
+	strbuf_addstr(&format, "format:%h] %s");
+
+	format_commit_message(commit, "%an <%ae>", &author_ident, &pctx);
+	format_commit_message(commit, "%cn <%ce>", &committer_ident, &pctx);
+	if (strbuf_cmp(&author_ident, &committer_ident)) {
+		strbuf_addstr(&format, "\n Author: ");
+		strbuf_addbuf_percentquote(&format, &author_ident);
+	}
+	if (!user_ident_explicitly_given) {
+		strbuf_addstr(&format, "\n Committer: ");
+		strbuf_addbuf_percentquote(&format, &committer_ident);
+		strbuf_addch(&format, '\n');
+		strbuf_addstr(&format, implicit_ident_advice);
+	}
+	strbuf_release(&author_ident);
+	strbuf_release(&committer_ident);
+
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 
@@ -1066,7 +1098,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
-	get_commit_format(format, &rev);
+	get_commit_format(format.buf, &rev);
+	strbuf_release(&format);
 	rev.always_show_header = 0;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
@@ -1085,7 +1118,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		struct pretty_print_context ctx = {0};
 		struct strbuf buf = STRBUF_INIT;
 		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format + 7, &buf, &ctx);
+		format_commit_message(commit, format.buf + 7, &buf, &ctx);
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index a529701..7940901 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -117,7 +117,11 @@ test_expect_success \
 test_expect_success \
 	"overriding author from command line" \
 	"echo 'gak' >file && \
-	 git commit -m 'author' --author 'Rubber Duck <rduck@convoy.org>' -a"
+	 git commit -m 'author' --author 'Rubber Duck <rduck@convoy.org>' -a >output 2>&1"
+
+test_expect_success \
+	"commit --author output mentions author" \
+	"grep Rubber.Duck output"
 
 test_expect_success PERL \
 	"interactive add" \
-- 
1.6.6.140.g92e4d.dirty

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Wincent Colaiuta]

El 13/01/2010, a las 18:39, Jeff King escribió:

>  $ git config --global --unset user.name
>  $ git config --global --unset user.email
>  $ git commit -m foo
>  [master 7c2a927] foo
>   Committer: Jeff King <peff@c-71-185-130-222.hsd1.va.comcast.net>
>  Your name and email address were configured automatically based
>  on your username and hostname. Please check that they are accurate.
>  You can suppress this message by setting them explicitly:
>
>      git config --global user.name Your Name
>      git config --global user.email you@example.com
>
>  If the identity used for this commit is wrong, you can fix it with:
>
>      git commit --amend --author='Your Name <you@example.com>'
>
>   1 files changed, 1 insertions(+), 0 deletions(-)

I'll never see this message myself, but I think you could (and perhaps  
should) replace almost all of that with:

   Your name and email address were configured automatically.
   See "git config help" for information on setting them explicitly
   or "git commit help" if you wish to amend this commit.

But like I said, seeing as I won't see the message its verbosity won't  
directly affect me.

Cheers,
Wincent

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Jeff King]

On Wed, Jan 13, 2010 at 07:39:47PM +0100, Wincent Colaiuta wrote:

> > Your name and email address were configured automatically based
> > on your username and hostname. Please check that they are accurate.
> > You can suppress this message by setting them explicitly:
> >
> >     git config --global user.name Your Name
> >     git config --global user.email you@example.com
> >
> > If the identity used for this commit is wrong, you can fix it with:
> >
> >     git commit --amend --author='Your Name <you@example.com>'
> >
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> I'll never see this message myself, but I think you could (and
> perhaps should) replace almost all of that with:
> 
>   Your name and email address were configured automatically.
>   See "git config help" for information on setting them explicitly
>   or "git commit help" if you wish to amend this commit.

I don't have a huge problem with your wording, except that it needs
s/(\w+) help/help \1/.

Mainly I was trying to hand-hold because not having this information set
up means it may be your first commit, and you are probably a bit
clueless (the exceptions are people who have been using git, but are
seeing this new behavior in their new version, and people who have git
configured on another machine but are using _this_ machine for the first
time).

As far as reducing verbosity goes, I don't think there is much point.
Both of ours are huge and annoying enough to nag you into setting up
your config, so the user is only likely to see it a few times.

> But like I said, seeing as I won't see the message its verbosity won't
> directly affect me.

I am also in this boat. :)

-Peff

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Wincent Colaiuta]

El 13/01/2010, a las 19:45, Jeff King escribió:

> On Wed, Jan 13, 2010 at 07:39:47PM +0100, Wincent Colaiuta wrote:
>
>>> Your name and email address were configured automatically based
>>> on your username and hostname. Please check that they are accurate.
>>> You can suppress this message by setting them explicitly:
>>>
>>>    git config --global user.name Your Name
>>>    git config --global user.email you@example.com
>>>
>>> If the identity used for this commit is wrong, you can fix it with:
>>>
>>>    git commit --amend --author='Your Name <you@example.com>'
>>>
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> I'll never see this message myself, but I think you could (and
>> perhaps should) replace almost all of that with:
>>
>>  Your name and email address were configured automatically.
>>  See "git config help" for information on setting them explicitly
>>  or "git commit help" if you wish to amend this commit.
>
> I don't have a huge problem with your wording, except that it needs
> s/(\w+) help/help \1/.

Whoops.

> Mainly I was trying to hand-hold because not having this information  
> set
> up means it may be your first commit, and you are probably a bit
> clueless (the exceptions are people who have been using git, but are
> seeing this new behavior in their new version, and people who have git
> configured on another machine but are using _this_ machine for the  
> first
> time).

Fair enough, but I'm sighing here at the thought of people jumping in  
and using git commands without even having looked at _any_ of the  
zillions of "your first 10 minutes with Git" tutorials out there,  
which pretty much _all_ start with how to set up your user.name and  
user.email...

Cheers,
Wincent

Re: [PATCH 2/3] strbuf: add strbuf_percentquote_buf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  (2) perhaps have assert(dst != src).  The loop won't terminate when
>>      called with src == dst, I think.
>
> Oops again. I think it is sensible to protect against this. I thought
> about trying to make it magically work in-place, but I don't think there
> is a simple way to do so.

As I said, I don't think appending p-quoted version of itself to a string
makes much sense, but I don't think in-place is too difficult.

	strbuf_addbuf_pquote(*dst, *src)
        {
		int len = src->len, i;
		for (i = 0; i < len; i++) {
			if (src->buf[i] == '%')
                        	strbuf_addch(dst, '%');
			strbuf_addch(dst, src->buf[i]);
		}
        }

>> --- a/strbuf.h
>> +++ b/strbuf.h
>> @@ -105,7 +105,13 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
>>  	strbuf_add(sb, s, strlen(s));
>>  }
>>  static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) {
>> -	strbuf_add(sb, sb2->buf, sb2->len);
>> +	char *buf = sb2->buf;
>> +	int len = sb2->len;
>> +	if (sb->buf == sb2->buf) {
>> +		strbuf_grow(sb, len);
>> +		buf = sb->buf;
>> +	}
>> +	strbuf_add(sb, buf, len);
>>  }
>
> Shouldn't this be "if (sb == sb2)"? Two strbufs in the initial state
> will point to the same strbuf_slopbuf, but obviously growing sb will not
> impact sb2. Though that would simply provoke a false positive, which I
> don't think has any negative consequences.

Ok, that is a good catch.  And two strbufs that share the same allocated
string is a user error

> Also, since reallocating sb will reallocate sb2, can't you just write it
> safely like this:
>
>   strbuf_grow(sb, sb2->len);
>   strbuf_add(sb, sb2->buf, sb2->len);

I didn't want to worry about a semi-clever (read: broken) compilers doing
semi-clever things assuming sb and sb2 do not alias, but I agree that your
approach is much simpler.

Thanks.

Re: [PATCH 3/3] commit: show interesting ident information in summary

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Just adding the "Committer:" reminder is slightly annoying (though
> perhaps some people will even like it). Adding a big advice message on
> every commit is going to be annoying to everyone who sees it, and is
> really crossing the line of "we don't really support implicit identities
> anymore", since anyone seeing it is going to want to fix it.
>
> I know there has been some discussion of that area in the last few
> months, but I admit I didn't pay any attention. Is that the direction we
> want to move in? I don't have a particular problem with it, but I want
> to point out that if there _are_ people who really like the implicit
> ident feature, we are effectively killing it off for them.

Traditionally, we've only had a minimal sanity check (e.g. to barf when
the name is empty, or something silly like that) and tried to come up with
a reasonable name/email given the available system information.

In olden days, `whoami`@`hostname`, at least on systems that were
competently maintained, gave a reasonable mail address for most people,
but I think it stopped being adequate more than 10 years ago, and it is
not useful anymore to majority of people, especially the ones who work on
Open Source projects as individuals, whose desired public identities are
often tied to their email account at their ISPs or mailbox providers (like
gmail).  There is no way for us to guess, when `whoami`@`hostname -f` is
the only thing we can go by without explicit user configuration.

Inside corporate environments, `whoami`@`hostname -f` might still be a
reasonable and usable default, though.

So I think the safest thing to do would be to give a big advice but make
it squelch-able with advice.howToSetYourIdentity or something.

Re: [PATCH 2/3] strbuf: add strbuf_percentquote_buf

[Jeff King]

On Wed, Jan 13, 2010 at 11:47:18AM -0800, Junio C Hamano wrote:

> As I said, I don't think appending p-quoted version of itself to a string
> makes much sense, but I don't think in-place is too difficult.
> 
> 	strbuf_addbuf_pquote(*dst, *src)
>         {
> 		int len = src->len, i;
> 		for (i = 0; i < len; i++) {
> 			if (src->buf[i] == '%')
>                         	strbuf_addch(dst, '%');
> 			strbuf_addch(dst, src->buf[i]);

Oops, of course. I was still thinking of actually doing a single
in-place conversion, not appending in-place. Of course yours is right.
Can you mark up my patch instead of using the assert?

-Peff

Re: [PATCH 3/3] commit: show interesting ident information in summary

[Jeff King]

On Wed, Jan 13, 2010 at 11:48:53AM -0800, Junio C Hamano wrote:

> In olden days, `whoami`@`hostname`, at least on systems that were
> competently maintained, gave a reasonable mail address for most people,
> but I think it stopped being adequate more than 10 years ago, and it is
> not useful anymore to majority of people, especially the ones who work on
> Open Source projects as individuals, whose desired public identities are
> often tied to their email account at their ISPs or mailbox providers (like
> gmail).  There is no way for us to guess, when `whoami`@`hostname -f` is
> the only thing we can go by without explicit user configuration.

Even outside of competent maintenance or individuals being served by
ISPs, I think it is really that it is no longer the case that the
machines we get our mail on and the machines we do our work on are less
and less the same. Even as an individual, I can afford a Linux
workstation on my desk _and_ one to serve my mail. But I don't advertise
peff@workstation.peff.net as my email.

Which isn't to say there aren't people in the separate situation, like:

> Inside corporate environments, `whoami`@`hostname -f` might still be a
> reasonable and usable default, though.
> 
> So I think the safest thing to do would be to give a big advice but make
> it squelch-able with advice.howToSetYourIdentity or something.

I think that is a good idea. Administrators of competent shared
environments can turn off the advice via /etc/gitconfig if they want,
and everyone else needs to opt into it consciously, which should help
reduce errors. I'm sure there will still be somebody, somewhere, who
complains about having to set the config, but that minority is hopefully
small enough to justify the errors saved by new git users.

Can you apply the patch below to my series as 4/3?

-- >8 --
Subject: [PATCH] commit: allow suppression of implicit identity advice

We now nag the user with a giant warning when their identity
was pulled from the username, hostname, and gecos
information, in case it is not correct. Most users will
suppress this by simply setting up their information
correctly.

However, there may be some users who consciously want to use
that information, because having the value change from host
to host contains useful information. These users can now set
advice.implicitidentity to false to suppress the message.

Signed-off-by: Jeff King <peff@peff.net>
---
Pretty straightforward. The biggest question is whether to suppress the
"Committer: XXX <YYY@ZZZ>" line, too. I kind of think it is useful if
you are intentionally using this feature; by definition if you are using
it intentionally then the information is of some interest to you.

 Documentation/config.txt |    4 ++++
 advice.c                 |    2 ++
 advice.h                 |    1 +
 builtin-commit.c         |    6 ++++--
 4 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9f40955..905076f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -130,6 +130,10 @@ advice.*::
 		Advice shown when linkgit:git-merge[1] refuses to
 		merge to avoid overwritting local changes.
 		Default: true.
+	implicitIdentity::
+		Advice on how to set your identity configuration when
+		your information is guessed from the system username and
+		domain name. Default: true.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index cb666ac..8f7de0e 100644
--- a/advice.c
+++ b/advice.c
@@ -3,6 +3,7 @@
 int advice_push_nonfastforward = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
+int advice_implicit_identity = 1;
 
 static struct {
 	const char *name;
@@ -11,6 +12,7 @@ static struct {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
+	{ "implicitidentity", &advice_implicit_identity },
 };
 
 int git_default_advice_config(const char *var, const char *value)
diff --git a/advice.h b/advice.h
index 3de5000..728ab90 100644
--- a/advice.h
+++ b/advice.h
@@ -4,6 +4,7 @@
 extern int advice_push_nonfastforward;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
+extern int advice_implicit_identity;
 
 int git_default_advice_config(const char *var, const char *value);
 
diff --git a/builtin-commit.c b/builtin-commit.c
index 3fa9b39..d687cf1 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1082,8 +1082,10 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	if (!user_ident_explicitly_given) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
-		strbuf_addch(&format, '\n');
-		strbuf_addstr(&format, implicit_ident_advice);
+		if (advice_implicit_identity) {
+			strbuf_addch(&format, '\n');
+			strbuf_addstr(&format, implicit_ident_advice);
+		}
 	}
 	strbuf_release(&author_ident);
 	strbuf_release(&committer_ident);
-- 
1.6.6.146.gdaab9.dirty

Re: [PATCH 3/3] commit: show interesting ident information in summary

[Jeff King]

On Wed, Jan 13, 2010 at 03:17:08PM -0500, Jeff King wrote:

> Even outside of competent maintenance or individuals being served by
> ISPs, I think it is really that it is no longer the case that the
> machines we get our mail on and the machines we do our work on are less
> and less the same. Even as an individual, I can afford a Linux

Er, re-reading that I think I have too many negatives. But hopefully you
get the point: "it is less and less the case that..." or "it is no
longer the case that..."

-Peff

Re: [PATCH 3/3] commit: show interesting ident information in summary

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 13, 2010 at 03:17:08PM -0500, Jeff King wrote:
>
>> Even outside of competent maintenance or individuals being served by
>> ISPs, I think it is really that it is no longer the case that the
>> machines we get our mail on and the machines we do our work on are less
>> and less the same. Even as an individual, I can afford a Linux
>
> Er, re-reading that I think I have too many negatives. But hopefully you
> get the point: "it is less and less the case that..." or "it is no
> longer the case that..."

Yes, I am in full agreement with everything you wrote in the message you
are responding to, including the comments after three-dash line about
showing the "Committer: " line even when the advice.implicitidentity is
declined.

Thanks.

Re: [PATCH v2 1/3] strbuf_expand: convert "%%" to "%"

[Chris Johnsen]

On 2010 Jan 13, at 11:35, Jeff King wrote:
> Signed-off-by: Jeff King <peff@coredump.intra.peff.net>

The patches of the v2 of this series (well, except "4/3") all use  
this surprising, "extended" hostname in their Signed-off-by lines. I  
suppose you unset user.email while testing the series and sent these  
out before restoring your normal configuration.

Sorry for the noise if this was intentional (a small joke about the  
auto-configured ident info?).

-- 
Chris

Re: [PATCH v2 1/3] strbuf_expand: convert "%%" to "%"

[Jeff King]

On Thu, Jan 14, 2010 at 05:47:09AM -0600, Chris Johnsen wrote:

> On 2010 Jan 13, at 11:35, Jeff King wrote:
> >Signed-off-by: Jeff King <peff@coredump.intra.peff.net>
> 
> The patches of the v2 of this series (well, except "4/3") all use
> this surprising, "extended" hostname in their Signed-off-by lines. I
> suppose you unset user.email while testing the series and sent these
> out before restoring your normal configuration.

Heh. Yes, thank you for noticing. That is exactly what happened. Perhaps
the next series should be a huge nag about implicit ident for S-o-b
lines. ;)

Junio, can you please fix up s/coredump.intra.// in your copies before
pushing out?

-Peff

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Thomas Rast]

Wincent Colaiuta wrote:
> 
> Fair enough, but I'm sighing here at the thought of people jumping in  
> and using git commands without even having looked at _any_ of the  
> zillions of "your first 10 minutes with Git" tutorials out there,  
> which pretty much _all_ start with how to set up your user.name and  
> user.email...

If you really are shocked by that thought, try hanging out on #git for
six hours on any given day...

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Felipe Contreras]

On Thu, Jan 14, 2010 at 5:02 PM, Thomas Rast <trast@student.ethz.ch> wrote:
> Wincent Colaiuta wrote:
>>
>> Fair enough, but I'm sighing here at the thought of people jumping in
>> and using git commands without even having looked at _any_ of the
>> zillions of "your first 10 minutes with Git" tutorials out there,
>> which pretty much _all_ start with how to set up your user.name and
>> user.email...
>
> If you really are shocked by that thought, try hanging out on #git for
> six hours on any given day...

Which is precisely why I was pushing for this:
http://thread.gmane.org/gmane.comp.version-control.git/131150

-- 
Felipe Contreras

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, Jan 14, 2010 at 5:02 PM, Thomas Rast <trast@student.ethz.ch> wrote:
>> Wincent Colaiuta wrote:
>>>
>>> Fair enough, but I'm sighing here at the thought of people jumping in
>>> and using git commands without even having looked at _any_ of the
>>> zillions of "your first 10 minutes with Git" tutorials out there,
>>> which pretty much _all_ start with how to set up your user.name and
>>> user.email...
>>
>> If you really are shocked by that thought, try hanging out on #git for
>> six hours on any given day...
>
> Which is precisely why I was pushing for this:
> http://thread.gmane.org/gmane.comp.version-control.git/131150

I think the point of the message you are responding to is that it has
already been proven that there are users that never reads any of the
zillions of "your first 10 minutes with Git".  How that _could_ ever
possibly be the reason/justification why you would want to push that
change to our documentation?

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Felipe Contreras]

On Thu, Jan 14, 2010 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> I think the point of the message you are responding to is that it has
> already been proven that there are users that never reads any of the
> zillions of "your first 10 minutes with Git".  How that _could_ ever
> possibly be the reason/justification why you would want to push that
> change to our documentation?

Users are lazy.

-- 
Felipe Contreras

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Thu, Jan 14, 2010 at 9:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> I think the point of the message you are responding to is that it has
>> already been proven that there are users that never reads any of the
>> zillions of "your first 10 minutes with Git".  How that _could_ ever
>> possibly be the reason/justification why you would want to push that
>> change to our documentation?
>
> Users are lazy.

And the ones that suffer from the issue discussed in this thread will not
read the manual your patch touches.  When you make changes to the manual,
you should not be targetting them, as they won't read it anyway.  Instead,
the description of the manual should aim to help people who _read_ it.

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Felipe Contreras]

On Thu, Jan 14, 2010 at 9:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Users are lazy.
>
> And the ones that suffer from the issue discussed in this thread will not
> read the manual your patch touches.  When you make changes to the manual,
> you should not be targetting them, as they won't read it anyway.  Instead,
> the description of the manual should aim to help people who _read_ it.

The world is not clear-cut between users who read, and users don't.
Most probably user laziness follows a Pareto distribution:
http://en.wikipedia.org/wiki/File:Pareto_distributionPDF.png

The long tail of users who don't read much is so big that you will
find *a lot* that don't read anything at all, therefore you would also
find many that read a bit, and as a consequence a tiny amount that
actually would read the whole user manual.

Clearly, Thomas' comment implies that some people might need to adjust
their mental model to reflect reality.

-- 
Felipe Contreras

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Adam Megacz]

Wincent Colaiuta <win@wincent.com> writes:
> Fair enough, but I'm sighing here at the thought of people jumping in  
> and using git commands without even having looked at _any_ of the  
> zillions of "your first 10 minutes with Git" tutorials out there,  

I don't think that's what this thread is really about, although helping
those people might be a harmless side-effect.

>> and people who have git configured on another machine but are using
>> _this_ machine for the first time).

For the record, this is what I have been burned by several times now.

I'm now stuck with a bunch of repositories I can no longer fix because
data blindly yanked out of libnss and never shown to me was then SHA-1
signed for all eternity.  It's incredibly frustrating.

Thank you for taking steps to save others from this frustration in the
future.  I appreciate it.

  - a

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> @@ -1046,9 +1058,12 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>  {
>  	struct rev_info rev;
>  	struct commit *commit;
> -	static const char *format = "format:%h] %s";
> +	struct strbuf format = STRBUF_INIT;
>  	unsigned char junk_sha1[20];
>  	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
> +	struct pretty_print_context pctx = {0};
> +	struct strbuf author_ident = STRBUF_INIT;
> +	struct strbuf committer_ident = STRBUF_INIT;
>  
>  	commit = lookup_commit(sha1);
>  	if (!commit)
> @@ -1056,6 +1071,23 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>  	if (!commit || parse_commit(commit))
>  		die("could not parse newly created commit");
>  
> +	strbuf_addstr(&format, "format:%h] %s");
> + ...
> +	if (strbuf_cmp(&author_ident, &committer_ident)) {
> +		strbuf_addstr(&format, "\n Author: ");
> +		strbuf_addbuf_percentquote(&format, &author_ident);
> +	}
> +	if (!user_ident_explicitly_given) {
> +		strbuf_addstr(&format, "\n Committer: ");
> +		strbuf_addbuf_percentquote(&format, &committer_ident);
> +		strbuf_addch(&format, '\n');
> +		strbuf_addstr(&format, implicit_ident_advice);
> +	}
> + ...
> -	get_commit_format(format, &rev);
> +	get_commit_format(format.buf, &rev);
> +	strbuf_release(&format);
>  	rev.always_show_header = 0;
>  	rev.diffopt.detect_rename = 1;
>  	rev.diffopt.rename_limit = 100;

This prepares the user format for log_tree_commit(); get_commit_format()
copies it away in its userformat, so it appears we are done with format
strbuf we built, and we release...

> @@ -1085,7 +1118,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>  		struct pretty_print_context ctx = {0};
>  		struct strbuf buf = STRBUF_INIT;
>  		ctx.date_mode = DATE_NORMAL;
> -		format_commit_message(commit, format + 7, &buf, &ctx);
> +		format_commit_message(commit, format.buf + 7, &buf, &ctx);
>  		printf("%s\n", buf.buf);

But sometimes log_tree_commit() doesn't show the header.  Most notably for
merges.  What string are we using for format_commit_message()?  Oops.

diff --git a/builtin-commit.c b/builtin-commit.c
index a73a532..7f61e87 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1013,7 +1013,6 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
 	get_commit_format(format.buf, &rev);
-	strbuf_release(&format);
 	rev.always_show_header = 0;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
@@ -1036,6 +1035,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		printf("%s\n", buf.buf);
 		strbuf_release(&buf);
 	}
+	strbuf_release(&format);
 }
 
 static int git_commit_config(const char *k, const char *v, void *cb)

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Matthieu Moy]

Wincent Colaiuta <win@wincent.com> writes:

> I'll never see this message myself, but I think you could (and perhaps
> should) replace almost all of that with:
>
>   Your name and email address were configured automatically.
>   See "git config help" for information on setting them explicitly
>   or "git commit help" if you wish to amend this commit.

I don't think this is a good idea. The two main cases when this
information will be shown is:

* Newbies, who didn't read the doc, or read it too fast. They'll
  happily ignore your short message.

  For example, I just started a project with 200 students. The doc we
  give them _starts_ with setting user/email in ~/.gitconfig, right
  before we give them the URL of the repository they'll work on. Out
  of that, 22 email adresses were mis-configured. Don't underestimate
  the ability of newbies not to read doc, even when told to do so.

  If the message is long, it'll be disturbing, and they may end up
  reading it.

* Non-newbies, using a machine for the first time. These users will
  see the message once, so it's not really disturbing, and at least I
  would appreciate the message to be flashy, to make sure I don't miss
  it.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 3/3] commit: show interesting ident information in summary

[Jeff King]

On Sun, Jan 17, 2010 at 12:59:53AM -0800, Junio C Hamano wrote:

> > @@ -1085,7 +1118,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
> >  		struct pretty_print_context ctx = {0};
> >  		struct strbuf buf = STRBUF_INIT;
> >  		ctx.date_mode = DATE_NORMAL;
> > -		format_commit_message(commit, format + 7, &buf, &ctx);
> > +		format_commit_message(commit, format.buf + 7, &buf, &ctx);
> >  		printf("%s\n", buf.buf);
> 
> But sometimes log_tree_commit() doesn't show the header.  Most notably for
> merges.  What string are we using for format_commit_message()?  Oops.

Ugh. Thank you and good catch.

-Peff