{"thread":{"id":"22260","subject":"[PATCH] commit: match explicit-ident semantics for summary and template","startedAt":"2010-01-17T19:34:01Z","lastAt":"2010-01-18T03:34:55Z","messageCount":6,"participants":["Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"131959","messageId":"20100117193401.GA28448@coredump.intra.peff.net","threadId":"22260","inReplyTo":null,"subject":"[PATCH] commit: match explicit-ident semantics for summary and template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-17T19:34:01Z","receivedAt":"2010-01-17T19:34:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Currently the commit summary will show the committer only if\nneither the name or email was set explicitly. However, the\ncommit message template will show the committer unless\n_both_ are set explicitly. This patch gives the same\nbehavior.\n\nThe difference came about because the two topics (one\nenhancing the template semantics, and the other adding the\nsummary warning) were developed independently.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis goes on top of 'next' as a result of merging jc/ident and\njk/warn-author-committer-after-commit. It's not correct on top of either\ntopic individually.\n\n builtin-commit.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 37c902c..d4eef6d 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -1099,7 +1099,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)\n \t\tstrbuf_addstr(&format, \"\\n Author: \");\n \t\tstrbuf_addbuf_percentquote(&format, &author_ident);\n \t}\n-\tif (!user_ident_explicitly_given) {\n+\tif (user_ident_explicitly_given != IDENT_ALL_GIVEN) {\n \t\tstrbuf_addstr(&format, \"\\n Committer: \");\n \t\tstrbuf_addbuf_percentquote(&format, &committer_ident);\n \t\tif (advice_implicit_identity) {\n-- \n1.6.6.418.gd5443\n"},{"id":"131966","messageId":"201001172153.44413.j6t@kdbg.org","threadId":"22260","inReplyTo":"20100117193401.GA28448@coredump.intra.peff.net","subject":"Re: [PATCH] commit: match explicit-ident semantics for summary and template","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2010-01-17T20:53:44Z","receivedAt":"2010-01-17T20:53:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Sonntag, 17. Januar 2010, Jeff King wrote:\n> -\tif (!user_ident_explicitly_given) {\n> +\tif (user_ident_explicitly_given != IDENT_ALL_GIVEN) {\n>  \t\tstrbuf_addstr(&format, \"\\n Committer: \");\n\nSorry for chiming in so late, but this new condition worries me a bit. On all \nof my machines I have the GECOS field filled in with \"Johannes Sixt\", i.e., I \ndo not need user.name. But of course the automatically derived email address \nis nonsense, so I've set up only user.email. Now I would always this hint, \nwouldn't I? Do most others fill in GECOS in ways that are inappropriate for \ngit?\n\n-- Hannes\n"},{"id":"131967","messageId":"20100117210038.GA3935@coredump.intra.peff.net","threadId":"22260","inReplyTo":"201001172153.44413.j6t@kdbg.org","subject":"Re: [PATCH] commit: match explicit-ident semantics for summary and template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-17T21:00:38Z","receivedAt":"2010-01-17T21:00:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 17, 2010 at 09:53:44PM +0100, Johannes Sixt wrote:\n\n> On Sonntag, 17. Januar 2010, Jeff King wrote:\n> > -\tif (!user_ident_explicitly_given) {\n> > +\tif (user_ident_explicitly_given != IDENT_ALL_GIVEN) {\n> >  \t\tstrbuf_addstr(&format, \"\\n Committer: \");\n> \n> Sorry for chiming in so late, but this new condition worries me a bit. On all \n> of my machines I have the GECOS field filled in with \"Johannes Sixt\", i.e., I \n> do not need user.name. But of course the automatically derived email address \n> is nonsense, so I've set up only user.email. Now I would always this hint, \n> wouldn't I? Do most others fill in GECOS in ways that are inappropriate for \n> git?\n\nYes, you are correctly analyzing the situation. I don't personally have\na preference (even though my GECOS field is correct, it always seemed\nsensible to me to just explicitly set user.name along with user.email.\nBut presumably a user who has set one is clueful enough to know whether\nthey need to set the other one, too).\n\nI do, however, think the summary behavior should match the behavior for\nthe commit message template, which was actually changed in 91c38a21.\nEither this patch should be applied, or the other behavior should be\ntweaked as below:\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex d4eef6d..ec1415e 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -656,7 +656,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\tauthor_ident);\n \t\tfree(author_ident);\n \n-\t\tif (user_ident_explicitly_given != IDENT_ALL_GIVEN)\n+\t\tif (!user_ident_explicitly_given)\n \t\t\tfprintf(fp,\n \t\t\t\t\"%s\"\n \t\t\t\t\"# Committer: %s\\n\",\n"},{"id":"131991","messageId":"7viqb0xubf.fsf@alter.siamese.dyndns.org","threadId":"22260","inReplyTo":"201001172153.44413.j6t@kdbg.org","subject":"Re: [PATCH] commit: match explicit-ident semantics for summary and template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-17T22:03:48Z","receivedAt":"2010-01-17T22:03:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> On Sonntag, 17. Januar 2010, Jeff King wrote:\n>> -\tif (!user_ident_explicitly_given) {\n>> +\tif (user_ident_explicitly_given != IDENT_ALL_GIVEN) {\n>>  \t\tstrbuf_addstr(&format, \"\\n Committer: \");\n>\n> Sorry for chiming in so late, but this new condition worries me a bit. On all \n> of my machines I have the GECOS field filled in with \"Johannes Sixt\", i.e., I \n> do not need user.name. But of course the automatically derived email address \n> is nonsense, so I've set up only user.email. Now I would always this hint, \n> wouldn't I? Do most others fill in GECOS in ways that are inappropriate for \n> git?\n\nWe could say something like\n\n\tif (!(user_ident_explicitly_given & IDENT_EMAIL_GIVEN))\n\nand it probably is a safer change on platforms with GECOS available, but\nthen wouldn't msysgit folks have to fork this code?\n\nBelow are _two_ overlapping patches that apply on top of individual\ntopics; they will conflict when the topics are merged but hopefully in a\nway that is trivially understandable, and I think can supersede Jeff's\npatch.\n\n-- >8 -- to jc/ident topic -- >8 --\n\n builtin-commit.c |    2 +-\n cache.h          |    1 +\n ident.c          |    9 +++++++++\n 3 files changed, 11 insertions(+), 1 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex f4974b5..b76f327 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -624,7 +624,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\tauthor_ident);\n \t\tfree(author_ident);\n \n-\t\tif (user_ident_explicitly_given != IDENT_ALL_GIVEN)\n+\t\tif (!user_ident_sufficiently_given())\n \t\t\tfprintf(fp,\n \t\t\t\t\"%s\"\n \t\t\t\t\"# Committer: %s\\n\",\ndiff --git a/cache.h b/cache.h\nindex 16c8e8d..f7a287c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -929,6 +929,7 @@ extern char git_default_name[MAX_GITNAME];\n #define IDENT_MAIL_GIVEN 02\n #define IDENT_ALL_GIVEN (IDENT_NAME_GIVEN|IDENT_MAIL_GIVEN)\n extern int user_ident_explicitly_given;\n+extern int user_ident_sufficiently_given(void);\n \n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\ndiff --git a/ident.c b/ident.c\nindex d4f6145..96b56e6 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -259,3 +259,12 @@ const char *git_committer_info(int flag)\n \t\t\t getenv(\"GIT_COMMITTER_DATE\"),\n \t\t\t flag);\n }\n+\n+int user_ident_sufficiently_given(void)\n+{\n+#ifndef WINDOWS\n+\treturn (user_ident_explicitly_given & IDENT_MAIL_GIVEN);\n+#else\n+\treturn (user_ident_explicitly_given == IDENT_ALL_GIVEN);\n+#endif\n+}\n\n-- 8< -- to jc/ident topic -- 8< --\n\n-- >8 -- to jk/warn-author-committer-after-commit topic -- >8 --\n builtin-commit.c |    4 ++--\n cache.h          |    1 +\n ident.c          |    5 +++++\n 3 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-commit.c b/builtin-commit.c\nindex 7f61e87..29dc3df 100644\n--- a/builtin-commit.c\n+++ b/builtin-commit.c\n@@ -602,7 +602,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\tauthor_ident);\n \t\tfree(author_ident);\n \n-\t\tif (!user_ident_explicitly_given)\n+\t\tif (!user_ident_sufficiently_given())\n \t\t\tfprintf(fp,\n \t\t\t\t\"%s\"\n \t\t\t\t\"# Committer: %s\\n\",\n@@ -991,7 +991,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)\n \t\tstrbuf_addstr(&format, \"\\n Author: \");\n \t\tstrbuf_addbuf_percentquote(&format, &author_ident);\n \t}\n-\tif (!user_ident_explicitly_given) {\n+\tif (!user_ident_sufficiently_given()) {\n \t\tstrbuf_addstr(&format, \"\\n Committer: \");\n \t\tstrbuf_addbuf_percentquote(&format, &committer_ident);\n \t\tif (advice_implicit_identity) {\ndiff --git a/cache.h b/cache.h\nindex bf468e5..63e0701 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -926,6 +926,7 @@ extern const char *config_exclusive_filename;\n extern char git_default_email[MAX_GITNAME];\n extern char git_default_name[MAX_GITNAME];\n extern int user_ident_explicitly_given;\n+extern int user_ident_sufficiently_given(void);\n \n extern const char *git_commit_encoding;\n extern const char *git_log_output_encoding;\ndiff --git a/ident.c b/ident.c\nindex 26409b2..248f769 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -259,3 +259,8 @@ const char *git_committer_info(int flag)\n \t\t\t getenv(\"GIT_COMMITTER_DATE\"),\n \t\t\t flag);\n }\n+\n+int user_ident_sufficiently_given(void)\n+{\n+\treturn user_ident_explicitly_given;\n+}\n\n-- 8< -- to jk/warn-author-committer-after-commit topic -- 8< --\n"},{"id":"132001","messageId":"20100118014735.GA6831@coredump.intra.peff.net","threadId":"22260","inReplyTo":"7viqb0xubf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] commit: match explicit-ident semantics for summary and template","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-18T01:47:35Z","receivedAt":"2010-01-18T01:47:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 17, 2010 at 02:03:48PM -0800, Junio C Hamano wrote:\n\n> We could say something like\n> \n> \tif (!(user_ident_explicitly_given & IDENT_EMAIL_GIVEN))\n> \n> and it probably is a safer change on platforms with GECOS available, but\n> then wouldn't msysgit folks have to fork this code?\n\nI hadn't thought to be specific to \"email must be given\". That is, I had\nassumed if you gave a name but not email, you would also be considered\ncompetent enough to avoid the warning. But I really can't see anybody\ndoing that, so the semantics you suggest above are fine by me.\n\nAs far as Windows goes, I have no opinion on the correct behavior. But\nif they are going to do something different, your encapsulation makes\nsense to me.\n\n-Peff\n"},{"id":"132010","messageId":"7vfx64uluo.fsf@alter.siamese.dyndns.org","threadId":"22260","inReplyTo":"20100118014735.GA6831@coredump.intra.peff.net","subject":"Re: [PATCH] commit: match explicit-ident semantics for summary and template","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-18T03:34:55Z","receivedAt":"2010-01-18T03:34:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Sun, Jan 17, 2010 at 02:03:48PM -0800, Junio C Hamano wrote:\n>\n>> We could say something like\n>> \n>> \tif (!(user_ident_explicitly_given & IDENT_EMAIL_GIVEN))\n>> \n>> and it probably is a safer change on platforms with GECOS available, but\n>> then wouldn't msysgit folks have to fork this code?\n>\n> I hadn't thought to be specific to \"email must be given\". That is, I had\n> assumed if you gave a name but not email, you would also be considered\n> competent enough to avoid the warning. But I really can't see anybody\n> doing that, so the semantics you suggest above are fine by me.\n\nIt is fine if we keep insisting on getting both explicitly, but as you\nsaid, I think if we have an explicit user.email, it is much more likely\nthat the user are happy with what we get from the GECOS than the user is\nunhappy with GECOS but hasn't learnt user.name configuration.\n\nAlso if you have neither user.name nor user.email on your fresh box, the\nchance that GECOS gives us the name you desire is much much more likely\nthan the chance the output from `whoami`@`hostname -f` happens to match\nyour desired e-mail identity.  I think checking MAIL only would probably\nbe the best heuristics.\n"}]}