[PATCH] commit: match explicit-ident semantics for summary and template

[PATCH] commit: match explicit-ident semantics for summary and template

[Jeff King]

Currently the commit summary will show the committer only if
neither the name or email was set explicitly. However, the
commit message template will show the committer unless
_both_ are set explicitly. This patch gives the same
behavior.

The difference came about because the two topics (one
enhancing the template semantics, and the other adding the
summary warning) were developed independently.

Signed-off-by: Jeff King <peff@peff.net>
---
This goes on top of 'next' as a result of merging jc/ident and
jk/warn-author-committer-after-commit. It's not correct on top of either
topic individually.

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

diff --git a/builtin-commit.c b/builtin-commit.c
index 37c902c..d4eef6d 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1099,7 +1099,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		strbuf_addstr(&format, "\n Author: ");
 		strbuf_addbuf_percentquote(&format, &author_ident);
 	}
-	if (!user_ident_explicitly_given) {
+	if (user_ident_explicitly_given != IDENT_ALL_GIVEN) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
 		if (advice_implicit_identity) {
-- 
1.6.6.418.gd5443

Re: [PATCH] commit: match explicit-ident semantics for summary and template

[Johannes Sixt]

On Sonntag, 17. Januar 2010, Jeff King wrote:
> -	if (!user_ident_explicitly_given) {
> +	if (user_ident_explicitly_given != IDENT_ALL_GIVEN) {
>  		strbuf_addstr(&format, "\n Committer: ");

Sorry for chiming in so late, but this new condition worries me a bit. On all 
of my machines I have the GECOS field filled in with "Johannes Sixt", i.e., I 
do not need user.name. But of course the automatically derived email address 
is nonsense, so I've set up only user.email. Now I would always this hint, 
wouldn't I? Do most others fill in GECOS in ways that are inappropriate for 
git?

-- Hannes

Re: [PATCH] commit: match explicit-ident semantics for summary and template

[Jeff King]

On Sun, Jan 17, 2010 at 09:53:44PM +0100, Johannes Sixt wrote:

> On Sonntag, 17. Januar 2010, Jeff King wrote:
> > -	if (!user_ident_explicitly_given) {
> > +	if (user_ident_explicitly_given != IDENT_ALL_GIVEN) {
> >  		strbuf_addstr(&format, "\n Committer: ");
> 
> Sorry for chiming in so late, but this new condition worries me a bit. On all 
> of my machines I have the GECOS field filled in with "Johannes Sixt", i.e., I 
> do not need user.name. But of course the automatically derived email address 
> is nonsense, so I've set up only user.email. Now I would always this hint, 
> wouldn't I? Do most others fill in GECOS in ways that are inappropriate for 
> git?

Yes, you are correctly analyzing the situation. I don't personally have
a preference (even though my GECOS field is correct, it always seemed
sensible to me to just explicitly set user.name along with user.email.
But presumably a user who has set one is clueful enough to know whether
they need to set the other one, too).

I do, however, think the summary behavior should match the behavior for
the commit message template, which was actually changed in 91c38a21.
Either this patch should be applied, or the other behavior should be
tweaked as below:

diff --git a/builtin-commit.c b/builtin-commit.c
index d4eef6d..ec1415e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -656,7 +656,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				author_ident);
 		free(author_ident);
 
-		if (user_ident_explicitly_given != IDENT_ALL_GIVEN)
+		if (!user_ident_explicitly_given)
 			fprintf(fp,
 				"%s"
 				"# Committer: %s\n",

Re: [PATCH] commit: match explicit-ident semantics for summary and template

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Sonntag, 17. Januar 2010, Jeff King wrote:
>> -	if (!user_ident_explicitly_given) {
>> +	if (user_ident_explicitly_given != IDENT_ALL_GIVEN) {
>>  		strbuf_addstr(&format, "\n Committer: ");
>
> Sorry for chiming in so late, but this new condition worries me a bit. On all 
> of my machines I have the GECOS field filled in with "Johannes Sixt", i.e., I 
> do not need user.name. But of course the automatically derived email address 
> is nonsense, so I've set up only user.email. Now I would always this hint, 
> wouldn't I? Do most others fill in GECOS in ways that are inappropriate for 
> git?

We could say something like

	if (!(user_ident_explicitly_given & IDENT_EMAIL_GIVEN))

and it probably is a safer change on platforms with GECOS available, but
then wouldn't msysgit folks have to fork this code?

Below are _two_ overlapping patches that apply on top of individual
topics; they will conflict when the topics are merged but hopefully in a
way that is trivially understandable, and I think can supersede Jeff's
patch.

-- >8 -- to jc/ident topic -- >8 --

 builtin-commit.c |    2 +-
 cache.h          |    1 +
 ident.c          |    9 +++++++++
 3 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index f4974b5..b76f327 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 != IDENT_ALL_GIVEN)
+		if (!user_ident_sufficiently_given())
 			fprintf(fp,
 				"%s"
 				"# Committer: %s\n",
diff --git a/cache.h b/cache.h
index 16c8e8d..f7a287c 100644
--- a/cache.h
+++ b/cache.h
@@ -929,6 +929,7 @@ extern char git_default_name[MAX_GITNAME];
 #define IDENT_MAIL_GIVEN 02
 #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)
 extern int user_ident_explicitly_given;
+extern int user_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
diff --git a/ident.c b/ident.c
index d4f6145..96b56e6 100644
--- a/ident.c
+++ b/ident.c
@@ -259,3 +259,12 @@ const char *git_committer_info(int flag)
 			 getenv("GIT_COMMITTER_DATE"),
 			 flag);
 }
+
+int user_ident_sufficiently_given(void)
+{
+#ifndef WINDOWS
+	return (user_ident_explicitly_given & IDENT_MAIL_GIVEN);
+#else
+	return (user_ident_explicitly_given == IDENT_ALL_GIVEN);
+#endif
+}

-- 8< -- to jc/ident topic -- 8< --

-- >8 -- to jk/warn-author-committer-after-commit topic -- >8 --
 builtin-commit.c |    4 ++--
 cache.h          |    1 +
 ident.c          |    5 +++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/builtin-commit.c b/builtin-commit.c
index 7f61e87..29dc3df 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -602,7 +602,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_sufficiently_given())
 			fprintf(fp,
 				"%s"
 				"# Committer: %s\n",
@@ -991,7 +991,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 		strbuf_addstr(&format, "\n Author: ");
 		strbuf_addbuf_percentquote(&format, &author_ident);
 	}
-	if (!user_ident_explicitly_given) {
+	if (!user_ident_sufficiently_given()) {
 		strbuf_addstr(&format, "\n Committer: ");
 		strbuf_addbuf_percentquote(&format, &committer_ident);
 		if (advice_implicit_identity) {
diff --git a/cache.h b/cache.h
index bf468e5..63e0701 100644
--- a/cache.h
+++ b/cache.h
@@ -926,6 +926,7 @@ extern const char *config_exclusive_filename;
 extern char git_default_email[MAX_GITNAME];
 extern char git_default_name[MAX_GITNAME];
 extern int user_ident_explicitly_given;
+extern int user_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
diff --git a/ident.c b/ident.c
index 26409b2..248f769 100644
--- a/ident.c
+++ b/ident.c
@@ -259,3 +259,8 @@ const char *git_committer_info(int flag)
 			 getenv("GIT_COMMITTER_DATE"),
 			 flag);
 }
+
+int user_ident_sufficiently_given(void)
+{
+	return user_ident_explicitly_given;
+}

-- 8< -- to jk/warn-author-committer-after-commit topic -- 8< --

Re: [PATCH] commit: match explicit-ident semantics for summary and template

[Jeff King]

On Sun, Jan 17, 2010 at 02:03:48PM -0800, Junio C Hamano wrote:

> We could say something like
> 
> 	if (!(user_ident_explicitly_given & IDENT_EMAIL_GIVEN))
> 
> and it probably is a safer change on platforms with GECOS available, but
> then wouldn't msysgit folks have to fork this code?

I hadn't thought to be specific to "email must be given". That is, I had
assumed if you gave a name but not email, you would also be considered
competent enough to avoid the warning. But I really can't see anybody
doing that, so the semantics you suggest above are fine by me.

As far as Windows goes, I have no opinion on the correct behavior. But
if they are going to do something different, your encapsulation makes
sense to me.

-Peff

Re: [PATCH] commit: match explicit-ident semantics for summary and template

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jan 17, 2010 at 02:03:48PM -0800, Junio C Hamano wrote:
>
>> We could say something like
>> 
>> 	if (!(user_ident_explicitly_given & IDENT_EMAIL_GIVEN))
>> 
>> and it probably is a safer change on platforms with GECOS available, but
>> then wouldn't msysgit folks have to fork this code?
>
> I hadn't thought to be specific to "email must be given". That is, I had
> assumed if you gave a name but not email, you would also be considered
> competent enough to avoid the warning. But I really can't see anybody
> doing that, so the semantics you suggest above are fine by me.

It is fine if we keep insisting on getting both explicitly, but as you
said, I think if we have an explicit user.email, it is much more likely
that the user are happy with what we get from the GECOS than the user is
unhappy with GECOS but hasn't learnt user.name configuration.

Also if you have neither user.name nor user.email on your fresh box, the
chance that GECOS gives us the name you desire is much much more likely
than the chance the output from `whoami`@`hostname -f` happens to match
your desired e-mail identity.  I think checking MAIL only would probably
be the best heuristics.