{"thread":{"id":"42975","subject":"[PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","startedAt":"2016-07-30T19:11:26Z","lastAt":"2016-08-08T18:01:57Z","messageCount":12,"participants":["Josh Triplett","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"292659","messageId":"20160730191111.cd6ay3l4hweyjf7f@x","threadId":"42975","inReplyTo":"cover.4d006cadf197f80d899ad7d7d56d8ba41f574adf.1469905775.git-series.josh@joshtriplett.org","subject":"[PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-07-30T19:11:11Z","receivedAt":"2016-07-30T19:11:26Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"This helps users who would prefer format-patch to default to --from, and\nmakes it easier to change the default in the future.\n\nSigned-off-by: Josh Triplett <josh@joshtriplett.org>\n---\n Documentation/config.txt               | 10 ++++++-\n builtin/log.c                          | 46 +++++++++++++++++++++------\n contrib/completion/git-completion.bash |  1 +-\n t/t4014-format-patch.sh                | 40 +++++++++++++++++++++++-\n 4 files changed, 88 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8b1aee4..bd34774 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1253,6 +1253,16 @@ format.attach::\n \tvalue as the boundary.  See the --attach option in\n \tlinkgit:git-format-patch[1].\n \n+format.from::\n+\tProvides the default value for the `--from` option to format-patch.\n+\tAccepts a boolean value, or a name and email address.  If false,\n+\tformat-patch defaults to `--no-from`, using commit authors directly in\n+\tthe \"From:\" field of patch mails.  If true, format-patch defaults to\n+\t`--from`, using your committer identity in the \"From:\" field of patch\n+\tmails and including a \"From:\" field in the body of the patch mail if\n+\tdifferent.  If set to a non-boolean value, format-patch uses that\n+\tvalue instead of your committer identity.  Defaults to false.\n+\n format.numbered::\n \tA boolean which can enable or disable sequence numbers in patch\n \tsubjects.  It defaults to \"auto\" which enables it only if there\ndiff --git a/builtin/log.c b/builtin/log.c\nindex fd1652f..dbd2da7 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -719,6 +719,7 @@ static void add_header(const char *value)\n static int thread;\n static int do_signoff;\n static int base_auto;\n+static char *from;\n static const char *signature = git_version_string;\n static const char *signature_file;\n static int config_cover_letter;\n@@ -731,6 +732,28 @@ enum {\n \tCOVER_AUTO\n };\n \n+enum from {\n+\tFROM_AUTHOR,\n+\tFROM_USER,\n+\tFROM_VALUE,\n+};\n+\n+static void set_from(enum from type, const char *value)\n+{\n+\tfree(from);\n+\tswitch (type) {\n+\tcase FROM_AUTHOR:\n+\t\tfrom = NULL;\n+\t\tbreak;\n+\tcase FROM_USER:\n+\t\tfrom = xstrdup(git_committer_info(IDENT_NO_DATE));\n+\t\tbreak;\n+\tcase FROM_VALUE:\n+\t\tfrom = xstrdup(value);\n+\t\tbreak;\n+\t}\n+}\n+\n static int git_format_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"format.headers\")) {\n@@ -807,6 +830,16 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\tbase_auto = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"format.from\")) {\n+\t\tint b = git_config_maybe_bool(var, value);\n+\t\tif (b < 0)\n+\t\t\tset_from(FROM_VALUE, value);\n+\t\telse if (b)\n+\t\t\tset_from(FROM_USER, NULL);\n+\t\telse\n+\t\t\tset_from(FROM_AUTHOR, NULL);\n+\t\treturn 0;\n+\t}\n \n \treturn git_log_config(var, value, cb);\n }\n@@ -1199,16 +1232,12 @@ static int cc_callback(const struct option *opt, const char *arg, int unset)\n \n static int from_callback(const struct option *opt, const char *arg, int unset)\n {\n-\tchar **from = opt->value;\n-\n-\tfree(*from);\n-\n \tif (unset)\n-\t\t*from = NULL;\n+\t\tset_from(FROM_AUTHOR, NULL);\n \telse if (arg)\n-\t\t*from = xstrdup(arg);\n+\t\tset_from(FROM_VALUE, arg);\n \telse\n-\t\t*from = xstrdup(git_committer_info(IDENT_NO_DATE));\n+\t\tset_from(FROM_USER, NULL);\n \treturn 0;\n }\n \n@@ -1384,7 +1413,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tint quiet = 0;\n \tint reroll_count = -1;\n \tchar *branch_name = NULL;\n-\tchar *from = NULL;\n \tchar *base_commit = NULL;\n \tstruct base_tree_info bases;\n \n@@ -1433,7 +1461,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t    0, to_callback },\n \t\t{ OPTION_CALLBACK, 0, \"cc\", NULL, N_(\"email\"), N_(\"add Cc: header\"),\n \t\t\t    0, cc_callback },\n-\t\t{ OPTION_CALLBACK, 0, \"from\", &from, N_(\"ident\"),\n+\t\t{ OPTION_CALLBACK, 0, \"from\", NULL, N_(\"ident\"),\n \t\t\t    N_(\"set From address to <ident> (or committer ident if absent)\"),\n \t\t\t    PARSE_OPT_OPTARG, from_callback },\n \t\tOPT_STRING(0, \"in-reply-to\", &in_reply_to, N_(\"message-id\"),\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 10f6d52..4393033 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2181,6 +2181,7 @@ _git_config ()\n \t\tformat.attach\n \t\tformat.cc\n \t\tformat.coverLetter\n+\t\tformat.from\n \t\tformat.headers\n \t\tformat.numbered\n \t\tformat.pretty\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 1206c48..b0579dd 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -229,6 +229,46 @@ check_patch () {\n \tgrep -e \"^Subject:\" \"$1\"\n }\n \n+test_expect_success 'format.from=false' '\n+\n+\tgit -c format.from=false format-patch --stdout master..side |\n+\tsed -e \"/^\\$/q\" >patch &&\n+\tcheck_patch patch &&\n+\t! grep \"^From: C O Mitter <committer@example.com>\\$\" patch\n+'\n+\n+test_expect_success 'format.from=true' '\n+\n+\tgit -c format.from=true format-patch --stdout master..side |\n+\tsed -e \"/^\\$/q\" >patch &&\n+\tcheck_patch patch &&\n+\tgrep \"^From: C O Mitter <committer@example.com>\\$\" patch\n+'\n+\n+test_expect_success 'format.from with address' '\n+\n+\tgit -c format.from=\"F R Om <from@example.com>\" format-patch --stdout master..side |\n+\tsed -e \"/^\\$/q\" >patch &&\n+\tcheck_patch patch &&\n+\tgrep \"^From: F R Om <from@example.com>\\$\" patch\n+'\n+\n+test_expect_success '--no-from overrides format.from' '\n+\n+\tgit -c format.from=\"F R Om <from@example.com>\" format-patch --no-from --stdout master..side |\n+\tsed -e \"/^\\$/q\" >patch &&\n+\tcheck_patch patch &&\n+\t! grep \"^From: F R Om <from@example.com>\\$\" patch\n+'\n+\n+test_expect_success '--from overrides format.from' '\n+\n+\tgit -c format.from=\"F R Om <from@example.com>\" format-patch --from --stdout master..side |\n+\tsed -e \"/^\\$/q\" >patch &&\n+\tcheck_patch patch &&\n+\t! grep \"^From: F R Om <from@example.com>\\$\" patch\n+'\n+\n test_expect_success '--no-to overrides config.to' '\n \n \tgit config --replace-all format.to \\\n-- \ngit-series 0.8.7\n"},{"id":"292660","messageId":"20160730191118.u35sq6ctt7psbnho@x","threadId":"42975","inReplyTo":"cover.4d006cadf197f80d899ad7d7d56d8ba41f574adf.1469905775.git-series.josh@joshtriplett.org","subject":"[PATCH v2 2/2] format-patch: Default to --from","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-07-30T19:11:18Z","receivedAt":"2016-07-30T19:11:31Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"This avoids spoofing mails when formatting commits not written by the\nuser.\n\nAdd tests for the new default, and fix tests whose expected output\ndepended on the old default.\n\nSigned-off-by: Josh Triplett <josh@joshtriplett.org>\n---\n Documentation/config.txt |  2 +-\n builtin/log.c            |  1 +\n t/t4014-format-patch.sh  | 28 +++++++++++++++++++++++++---\n 3 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex bd34774..2310877 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1261,7 +1261,7 @@ format.from::\n \t`--from`, using your committer identity in the \"From:\" field of patch\n \tmails and including a \"From:\" field in the body of the patch mail if\n \tdifferent.  If set to a non-boolean value, format-patch uses that\n-\tvalue instead of your committer identity.  Defaults to false.\n+\tvalue instead of your committer identity.  Defaults to true.\n \n format.numbered::\n \tA boolean which can enable or disable sequence numbers in patch\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dbd2da7..bcca974 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1486,6 +1486,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tset_from(FROM_USER, NULL);\n \textra_hdr.strdup_strings = 1;\n \textra_to.strdup_strings = 1;\n \textra_cc.strdup_strings = 1;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex b0579dd..fa35cbe 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -652,7 +652,7 @@ EOF\n \n test_expect_success 'format-patch -p suppresses stat' '\n \n-\tgit format-patch -p -2 &&\n+\tgit format-patch --no-from -p -2 &&\n \tsed -e \"1,/^\\$/d\" -e \"/^+5/q\" < 0001-This-is-an-excessively-long-subject-line-for-a-messa.patch > output &&\n \ttest_cmp expect output\n \n@@ -973,7 +973,7 @@ check_author() {\n \techo content >>file &&\n \tgit add file &&\n \tGIT_AUTHOR_NAME=$1 git commit -m author-check &&\n-\tgit format-patch --stdout -1 >patch &&\n+\tgit format-patch --no-from --stdout -1 >patch &&\n \tsed -n \"/^From: /p; /^ /p; /^$/q\" <patch >actual &&\n \ttest_cmp expect actual\n }\n@@ -1089,6 +1089,18 @@ test_expect_success '--from=ident replaces author' '\n \ttest_cmp expect patch.head\n '\n \n+test_expect_success 'Default uses committer ident' '\n+\tgit format-patch -1 --stdout >patch &&\n+\tcat >expect <<-\\EOF &&\n+\tFrom: C O Mitter <committer@example.com>\n+\n+\tFrom: A U Thor <author@example.com>\n+\n+\tEOF\n+\tsed -ne \"/^From:/p; /^$/p; /^---$/q\" <patch >patch.head &&\n+\ttest_cmp expect patch.head\n+'\n+\n test_expect_success '--from uses committer ident' '\n \tgit format-patch -1 --stdout --from >patch &&\n \tcat >expect <<-\\EOF &&\n@@ -1101,6 +1113,16 @@ test_expect_success '--from uses committer ident' '\n \ttest_cmp expect patch.head\n '\n \n+test_expect_success '--no-from suppresses default --from' '\n+\tgit format-patch -1 --stdout --no-from >patch &&\n+\tcat >expect <<-\\EOF &&\n+\tFrom: A U Thor <author@example.com>\n+\n+\tEOF\n+\tsed -ne \"/^From:/p; /^$/p; /^---$/q\" <patch >patch.head &&\n+\ttest_cmp expect patch.head\n+'\n+\n test_expect_success '--from omits redundant in-body header' '\n \tgit format-patch -1 --stdout --from=\"A U Thor <author@example.com>\" >patch &&\n \tcat >expect <<-\\EOF &&\n@@ -1129,7 +1151,7 @@ test_expect_success 'in-body headers trigger content encoding' '\n append_signoff()\n {\n \tC=$(git commit-tree HEAD^^{tree} -p HEAD) &&\n-\tgit format-patch --stdout --signoff $C^..$C >append_signoff.patch &&\n+\tgit format-patch --no-from --stdout --signoff $C^..$C >append_signoff.patch &&\n \tsed -n -e \"1,/^---$/p\" append_signoff.patch |\n \t\tegrep -n \"^Subject|Sign|^$\"\n }\n-- \ngit-series 0.8.7\n"},{"id":"292766","messageId":"20160801173847.qph2tora75h6ebsk@sigill.intra.peff.net","threadId":"42975","inReplyTo":"20160730191111.cd6ay3l4hweyjf7f@x","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-01T17:38:47Z","receivedAt":"2016-08-01T18:06:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 30, 2016 at 12:11:11PM -0700, Josh Triplett wrote:\n\n> +enum from {\n> +\tFROM_AUTHOR,\n> +\tFROM_USER,\n> +\tFROM_VALUE,\n> +};\n> +\n> +static void set_from(enum from type, const char *value)\n> +{\n> +\tfree(from);\n> +\tswitch (type) {\n> +\tcase FROM_AUTHOR:\n> +\t\tfrom = NULL;\n> +\t\tbreak;\n> +\tcase FROM_USER:\n> +\t\tfrom = xstrdup(git_committer_info(IDENT_NO_DATE));\n> +\t\tbreak;\n> +\tcase FROM_VALUE:\n> +\t\tfrom = xstrdup(value);\n> +\t\tbreak;\n> +\t}\n> +}\n\nThanks for looking into reducing the duplication. TBH, I am not sure it\nis really an improvement, just because of the amount of boilerplate (and\nthis function interface is kind of weird, because of the rules for when\n\"value\" should or should not be NULL).\n\nI guess another way to do it would be:\n\n  #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))\n  void set_from(const char *value)\n  {\n\tif (value == FROM_AUTO_IDENT)\n\t\tvalue = git_committer_info(IDENT_NO_DATE);\n\tfree(from);\n\tfrom = xstrdup_or_null(value);\n  }\n\nbut I think the effort to polish further here is outweighing the\nmagnitude of the patch itself. So I offer that as \"how I would have done\nit\" in case you like it, but again, I am fine with either this version\nor the previous.\n\n-Peff\n"},{"id":"292803","messageId":"xmqqziowgpc8.fsf@gitster.mtv.corp.google.com","threadId":"42975","inReplyTo":"20160730191111.cd6ay3l4hweyjf7f@x","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-01T21:18:47Z","receivedAt":"2016-08-01T21:55:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> Subject: Re: [PATCH v2 1/2] format-patch: Add a config option format.from ...\n\nAt least s/Add/add/; but I would prefer an even shorter\n\n\tformat-patch: format.from gives the default for --from\n\n> +static char *from;\n\nThe same \"this does not quite help the transition\" comment applies\nto this one.\n\n> +enum from {\n> +\tFROM_AUTHOR,\n> +\tFROM_USER,\n> +\tFROM_VALUE,\n\nDrop trailing comma after the last enum definition (trailing comma\nafter the last element in an array is OK, though).\n\n> +static void set_from(enum from type, const char *value)\n> +{\n> +\tfree(from);\n> +\tswitch (type) {\n> +\tcase FROM_AUTHOR:\n> +\t\tfrom = NULL;\n> +\t\tbreak;\n> +\tcase FROM_USER:\n> +\t\tfrom = xstrdup(git_committer_info(IDENT_NO_DATE));\n> +\t\tbreak;\n> +\tcase FROM_VALUE:\n> +\t\tfrom = xstrdup(value);\n> +\t\tbreak;\n> +\t}\n> +}\n\nI tend to agree with what Jeff said; I'd queue 1/2 from the original\nround for now.\n\nThanks.\n"},{"id":"293329","messageId":"20160807225701.ucv2xunq5vs4uedk@x","threadId":"42975","inReplyTo":"20160801173847.qph2tora75h6ebsk@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-08-07T22:57:01Z","receivedAt":"2016-08-07T22:57:25Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Mon, Aug 01, 2016 at 01:38:47PM -0400, Jeff King wrote:\n> On Sat, Jul 30, 2016 at 12:11:11PM -0700, Josh Triplett wrote:\n> \n> > +enum from {\n> > +\tFROM_AUTHOR,\n> > +\tFROM_USER,\n> > +\tFROM_VALUE,\n> > +};\n> > +\n> > +static void set_from(enum from type, const char *value)\n> > +{\n> > +\tfree(from);\n> > +\tswitch (type) {\n> > +\tcase FROM_AUTHOR:\n> > +\t\tfrom = NULL;\n> > +\t\tbreak;\n> > +\tcase FROM_USER:\n> > +\t\tfrom = xstrdup(git_committer_info(IDENT_NO_DATE));\n> > +\t\tbreak;\n> > +\tcase FROM_VALUE:\n> > +\t\tfrom = xstrdup(value);\n> > +\t\tbreak;\n> > +\t}\n> > +}\n> \n> Thanks for looking into reducing the duplication. TBH, I am not sure it\n> is really an improvement, just because of the amount of boilerplate (and\n> this function interface is kind of weird, because of the rules for when\n> \"value\" should or should not be NULL).\n> \n> I guess another way to do it would be:\n> \n>   #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))\n>   void set_from(const char *value)\n>   {\n> \tif (value == FROM_AUTO_IDENT)\n> \t\tvalue = git_committer_info(IDENT_NO_DATE);\n> \tfree(from);\n> \tfrom = xstrdup_or_null(value);\n>   }\n\nI'd actually seriously considered this exact approach, which I preferred\nas well, but I'd discarded it because I figured it'd get rejected.\nGiven your suggestion, and Junio's comment, I'll go with this version.\n\n- Josh Triplett\n"},{"id":"293333","messageId":"xmqqtwewggwi.fsf@gitster.mtv.corp.google.com","threadId":"42975","inReplyTo":"20160807225701.ucv2xunq5vs4uedk@x","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-08T01:59:09Z","receivedAt":"2016-08-08T01:59:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> I'd actually seriously considered this exact approach, which I preferred\n> as well, but I'd discarded it because I figured it'd get rejected.\n> Given your suggestion, and Junio's comment, I'll go with this version.\n\nSorry, but your response is soo delayed that I am not sure what you\nare agreeing with and also am not sure if you are planning to reroll\nwhat has already been happily accepted to 'next', which is not quite\nwelcome.\n\n"},{"id":"293334","messageId":"20160808043458.jrgkoy2i65hxsaeo@x","threadId":"42975","inReplyTo":"xmqqtwewggwi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-08-08T04:34:59Z","receivedAt":"2016-08-08T04:35:17Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Sun, Aug 07, 2016 at 06:59:09PM -0700, Junio C Hamano wrote:\n> Josh Triplett <josh@joshtriplett.org> writes:\n> \n> > I'd actually seriously considered this exact approach, which I preferred\n> > as well, but I'd discarded it because I figured it'd get rejected.\n> > Given your suggestion, and Junio's comment, I'll go with this version.\n> \n> Sorry, but your response is soo delayed that I am not sure what you\n> are agreeing with\n\nI'm on vacation right now.  I was agreeing with your comment that you\ndidn't care for the change in v2 to use an enum.\n\n> and also am not sure if you are planning to reroll\n> what has already been happily accepted to 'next', which is not quite\n> welcome.\n\nI didn't realize you had already taken the patch series into next; I'd\nassumed from the various comments that you expected me to reroll it\nbefore you'd take it.\n\nWould you like me to write something up for the release notes regarding\nplans to change the default?\n\n- Josh Triplett\n"},{"id":"293335","messageId":"20160808044206.ubvaftex3mwbmwdh@x","threadId":"42975","inReplyTo":"xmqqziowgpc8.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-08-08T04:42:07Z","receivedAt":"2016-08-08T04:42:26Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Mon, Aug 01, 2016 at 02:18:47PM -0700, Junio C Hamano wrote:\n> Josh Triplett <josh@joshtriplett.org> writes:\n> > +enum from {\n> > +\tFROM_AUTHOR,\n> > +\tFROM_USER,\n> > +\tFROM_VALUE,\n> \n> Drop trailing comma after the last enum definition (trailing comma\n> after the last element in an array is OK, though).\n\nI realize this code didn't get included in the final version, but for\nfuture reference, what's the rationale for this?  I tend to include a\nfinal comma in cases like these (and likewise for initializers) to avoid\nneeding to change the last line when introducing a new element, reducing\nnoise in diffs.  I hadn't seen anything in any of the coding style\ndocumentation talking about trailing commas (either pro or con).\n"},{"id":"293337","messageId":"20160808045441.duy7ztgdrz7wpvzj@sigill.intra.peff.net","threadId":"42975","inReplyTo":"20160808044206.ubvaftex3mwbmwdh@x","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-08T04:54:41Z","receivedAt":"2016-08-08T04:59:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 07, 2016 at 06:42:07PM -1000, Josh Triplett wrote:\n\n> > Drop trailing comma after the last enum definition (trailing comma\n> > after the last element in an array is OK, though).\n> \n> I realize this code didn't get included in the final version, but for\n> future reference, what's the rationale for this?  I tend to include a\n> final comma in cases like these (and likewise for initializers) to avoid\n> needing to change the last line when introducing a new element, reducing\n> noise in diffs.  I hadn't seen anything in any of the coding style\n> documentation talking about trailing commas (either pro or con).\n\nPortability; some compilers choke on it. C89 allows trailing commas in\narray initialization but _not_ in enums. Most compilers allow it anyway\n(though gcc complains with -Wpedantic).\n\nThis definitely broke the build on real systems early in Git's history\n(I think the AIX compiler was one culprit), but at this point it's\npossible that all of those compilers have died off. It would be nice if\nwe could start using it (for exactly the reasons you give).\nUnfortunately there's not a good way to know except \"introduce it and\nsee if people complain\".\n\n-Peff\n"},{"id":"293338","messageId":"20160808050217.u2pmbu7b7yww4viv@x","threadId":"42975","inReplyTo":"20160808045441.duy7ztgdrz7wpvzj@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Josh Triplett","fromEmail":"josh@joshtriplett.org","sentAt":"2016-08-08T05:02:18Z","receivedAt":"2016-08-08T05:02:29Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"On Mon, Aug 08, 2016 at 12:54:41AM -0400, Jeff King wrote:\n> On Sun, Aug 07, 2016 at 06:42:07PM -1000, Josh Triplett wrote:\n> \n> > > Drop trailing comma after the last enum definition (trailing comma\n> > > after the last element in an array is OK, though).\n> > \n> > I realize this code didn't get included in the final version, but for\n> > future reference, what's the rationale for this?  I tend to include a\n> > final comma in cases like these (and likewise for initializers) to avoid\n> > needing to change the last line when introducing a new element, reducing\n> > noise in diffs.  I hadn't seen anything in any of the coding style\n> > documentation talking about trailing commas (either pro or con).\n> \n> Portability; some compilers choke on it. C89 allows trailing commas in\n> array initialization but _not_ in enums. Most compilers allow it anyway\n> (though gcc complains with -Wpedantic).\n> \n> This definitely broke the build on real systems early in Git's history\n> (I think the AIX compiler was one culprit),\n\nThanks for the explanation.  I assume such compilers also don't accept\nC99?\n\n> but at this point it's\n> possible that all of those compilers have died off. It would be nice if\n> we could start using it (for exactly the reasons you give).\n> Unfortunately there's not a good way to know except \"introduce it and\n> see if people complain\".\n\nFair enough.  I'll let someone else be the test case for that. :)\n\nPerhaps the next Git user survey could ask \"what compiler (including\nversion) do you use to compile Git\", and perhaps \"does it accept the\nfollowing code:\"?\n\n- Josh Triplett\n"},{"id":"293339","messageId":"20160808050614.pirm3qnizrbzbsh4@sigill.intra.peff.net","threadId":"42975","inReplyTo":"20160808050217.u2pmbu7b7yww4viv@x","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-08T05:06:14Z","receivedAt":"2016-08-08T05:06:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 07, 2016 at 07:02:18PM -1000, Josh Triplett wrote:\n\n> > Portability; some compilers choke on it. C89 allows trailing commas in\n> > array initialization but _not_ in enums. Most compilers allow it anyway\n> > (though gcc complains with -Wpedantic).\n> > \n> > This definitely broke the build on real systems early in Git's history\n> > (I think the AIX compiler was one culprit),\n> \n> Thanks for the explanation.  I assume such compilers also don't accept\n> C99?\n\nCorrect. We don't allow other C99 features like variadic macros, either\n(there are some in the code base, but you'll note they can all be\nconditionally disabled).\n\n> Perhaps the next Git user survey could ask \"what compiler (including\n> version) do you use to compile Git\", and perhaps \"does it accept the\n> following code:\"?\n\nMaybe. I'm not sure I would consider a lack of responses there to be a\ndefinite sign. It seems that once every few years people on bizarre\nsystems come out of the woodwork and do a round of portability fixes,\nand then problems accrue, and so on. So I'm not sure that the survey\nwould hit the right people in a timely manner.\n\nI think the breaking point will be just declaring \"look, C99 is N years\nold; if your compiler can't handle it, that's now your problem\". When\nGit started, N was only 6. It's now 17.\n\n-Peff\n"},{"id":"293372","messageId":"xmqqa8gnf8c2.fsf@gitster.mtv.corp.google.com","threadId":"42975","inReplyTo":"20160808043458.jrgkoy2i65hxsaeo@x","subject":"Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-08T18:01:49Z","receivedAt":"2016-08-08T18:01:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@joshtriplett.org> writes:\n\n> I didn't realize you had already taken the patch series into next; I'd\n> assumed from the various comments that you expected me to reroll it\n> before you'd take it.\n>\n> Would you like me to write something up for the release notes regarding\n> plans to change the default?\n\nGiven that we are at week #8 and -rc0 is coming soon, I suspect that\nthat note will happen not in this release but in the next one.\n\nThe patch in question (1/2) is merely a new convenience feature that\ndoes not have to say anything about the future default, so we are\ngood with 1/2 as-is (not v2 version of it, but the original one\nwithout enum), I think.\n\n\n\n\n"}]}