threads / patch / 22260

patchcommit: match explicit-ident semantics for summary and template

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

## tl;dr

6 messages between Jan 17, 2010 and Jan 18, 2010. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Jeff King· Jan 17, 2010, 19:34 UTC · lore

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(-)
Show changes to builtin-commit.c +1 −1
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
Johannes Sixt· Jan 17, 2010, 20:53 UTC · re: Jeff King · lore

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

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
Jeff King· Jan 17, 2010, 21:00 UTC · re: Johannes Sixt · lore

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

On Sun, Jan 17, 2010 at 09:53:44PM +0100, Johannes Sixt wrote:
Show 11 quoted lines
> 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:

Show changes to builtin-commit.c +1 −1
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",
Junio C Hamano· Jan 17, 2010, 22:03 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> writes:
Show 11 quoted lines
> 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(-)
Show changes to 3 files +19 −6

builtin-commit.c, cache.h, ident.c

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< --
Jeff King· Jan 18, 2010, 01:47 UTC · re: Junio C Hamano · lore

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

On Sun, Jan 17, 2010 at 02:03:48PM -0800, Junio C Hamano wrote:
Show 6 quoted lines
> 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
Junio C Hamano· Jan 18, 2010, 03:34 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> 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.

← back to recent threads