{"thread":{"id":"32817","subject":"[PATCH 2/3] combine-diff: suppress a clang warning","startedAt":"2013-02-03T14:37:08Z","lastAt":"2013-02-07T08:41:06Z","messageCount":20,"participants":["John Keeping","Antoine Pelisse","Tay Ray Chuan","Jonathan Nieder","Junio C Hamano","Miles Bader"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"208544","messageId":"cover.1359901732.git.john@keeping.me.uk","threadId":"32817","inReplyTo":null,"subject":"[PATCH 0/3] Make Git compile warning-free with Clang","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-03T14:37:08Z","receivedAt":"2013-02-03T14:37:08Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"The first two patches here were sent to the list before but seem to have\ngot lost in the noise [1][2].  The final one is new but was prompted by\ndiscussion in the same thread.\n\nAfter applying all of these patches, I don't see any warnings compiling\nGit with Clang 3.2.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/213817\n[2] http://article.gmane.org/gmane.comp.version-control.git/213849\n\nAntoine Pelisse (1):\n  fix clang -Wtautological-compare with unsigned enum\n\nJohn Keeping (2):\n  combine-diff: suppress a clang warning\n  builtin/apply: tighten (dis)similarity index parsing\n\n builtin/apply.c | 10 ++++++----\n combine-diff.c  |  2 +-\n grep.c          |  3 ++-\n grep.h          |  3 ++-\n 4 files changed, 11 insertions(+), 7 deletions(-)\n\n-- \n1.8.1.2\n"},{"id":"208545","messageId":"a9fe675ed9b34d3c15f4678ee13e90cddaa36055.1359901732.git.john@keeping.me.uk","threadId":"32817","inReplyTo":"cover.1359901732.git.john@keeping.me.uk","subject":"[PATCH 1/3] fix clang -Wtautological-compare with unsigned enum","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-03T14:37:09Z","receivedAt":"2013-02-03T14:37:09Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"From: Antoine Pelisse <apelisse@gmail.com>\n\nCreate a GREP_HEADER_FIELD_MIN so we can check that the field value is\nsane and silent the clang warning.\n\nClang warning happens because the enum is unsigned (this is\nimplementation-defined, and there is no negative fields) and the check\nis then tautological.\n\nSigned-off-by: Antoine Pelisse <apelisse@gmail.com>\n---\n grep.c | 3 ++-\n grep.h | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 4bd1b8b..bb548ca 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n \tfor (p = opt->header_list; p; p = p->next) {\n \t\tif (p->token != GREP_PATTERN_HEAD)\n \t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n-\t\tif (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n+\t\tif (p->field < GREP_HEADER_FIELD_MIN ||\n+\t\t    GREP_HEADER_FIELD_MAX <= p->field)\n \t\t\tdie(\"bug: unknown header field %d\", p->field);\n \t\tcompile_regexp(p, opt);\n \t}\ndiff --git a/grep.h b/grep.h\nindex 8fc854f..e4a1df5 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -28,7 +28,8 @@ enum grep_context {\n };\n \n enum grep_header_field {\n-\tGREP_HEADER_AUTHOR = 0,\n+\tGREP_HEADER_FIELD_MIN = 0,\n+\tGREP_HEADER_AUTHOR = GREP_HEADER_FIELD_MIN,\n \tGREP_HEADER_COMMITTER,\n \tGREP_HEADER_REFLOG,\n \n-- \n1.8.1.2\n"},{"id":"208543","messageId":"6995fd5e4d9cb3320ab80c983f1b25ae8a399284.1359901732.git.john@keeping.me.uk","threadId":"32817","inReplyTo":"cover.1359901732.git.john@keeping.me.uk","subject":"[PATCH 2/3] combine-diff: suppress a clang warning","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-03T14:37:10Z","receivedAt":"2013-02-03T14:37:10Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When compiling combine-diff.c, clang 3.2 says:\n\n    combine-diff.c:1006:19: warning: adding 'int' to a string does not\n\t    append to the string [-Wstring-plus-int]\n\t\tprefix = COLONS + offset;\n\t\t\t ~~~~~~~^~~~~~~~\n    combine-diff.c:1006:19: note: use array indexing to silence this warning\n\t\tprefix = COLONS + offset;\n\t\t\t\t^\n\t\t\t &      [       ]\n\nSuppress this by making the suggested change.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n combine-diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex bb1cc96..dba4748 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \t\toffset = strlen(COLONS) - num_parent;\n \t\tif (offset < 0)\n \t\t\toffset = 0;\n-\t\tprefix = COLONS + offset;\n+\t\tprefix = &COLONS[offset];\n \n \t\t/* Show the modes */\n \t\tfor (i = 0; i < num_parent; i++) {\n-- \n1.8.1.2\n"},{"id":"208546","messageId":"2cac21192f79f9fbb5822417775954eba29064fa.1359901732.git.john@keeping.me.uk","threadId":"32817","inReplyTo":"cover.1359901732.git.john@keeping.me.uk","subject":"[PATCH 3/3] builtin/apply: tighten (dis)similarity index parsing","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-03T14:37:11Z","receivedAt":"2013-02-03T14:37:11Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This was prompted by an incorrect warning issued by clang [1], and a\nsuggestion by Linus to restrict the range to check for values greater\nthan INT_MAX since these will give bogus output after casting to int.\n\nIn fact the (dis)similarity index is a percentage, so reject values\ngreater than 100.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/213857\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n builtin/apply.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 6c11e8b..4745e75 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1041,15 +1041,17 @@ static int gitdiff_renamedst(const char *line, struct patch *patch)\n \n static int gitdiff_similarity(const char *line, struct patch *patch)\n {\n-\tif ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n-\t\tpatch->score = 0;\n+\tunsigned long val = strtoul(line, NULL, 10);\n+\tif (val <= 100)\n+\t\tpatch->score = val;\n \treturn 0;\n }\n \n static int gitdiff_dissimilarity(const char *line, struct patch *patch)\n {\n-\tif ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n-\t\tpatch->score = 0;\n+\tunsigned long val = strtoul(line, NULL, 10);\n+\tif (val <= 100)\n+\t\tpatch->score = val;\n \treturn 0;\n }\n \n-- \n1.8.1.2\n"},{"id":"208550","messageId":"CALWbr2wz2yEP9bBxS5UG1abtsRR-BdaP1vFLMp7JL2jwCkoFFA@mail.gmail.com","threadId":"32817","inReplyTo":"cover.1359901732.git.john@keeping.me.uk","subject":"Re: [PATCH 0/3] Make Git compile warning-free with Clang","fromName":"Antoine Pelisse","fromEmail":"apelisse@gmail.com","sentAt":"2013-02-03T17:13:39Z","receivedAt":"2013-02-03T17:13:39Z","isPatch":true,"sender":{"key":"apelisse@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1929644?v=4"},"body":"Thanks John,\nI couldn't find any time to send that \"sum-up\" series.\n\nOn Sun, Feb 3, 2013 at 3:37 PM, John Keeping <john@keeping.me.uk> wrote:\n> The first two patches here were sent to the list before but seem to have\n> got lost in the noise [1][2].  The final one is new but was prompted by\n> discussion in the same thread.\n"},{"id":"208553","messageId":"CALUzUxowrh53g50ZxkXSjLfOrSgX-YiZEB2MJXbLwxmwNB187A@mail.gmail.com","threadId":"32817","inReplyTo":"6995fd5e4d9cb3320ab80c983f1b25ae8a399284.1359901732.git.john@keeping.me.uk","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2013-02-03T18:20:06Z","receivedAt":"2013-02-03T18:20:06Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"On Sun, Feb 3, 2013 at 10:37 PM, John Keeping <john@keeping.me.uk> wrote:\n> When compiling combine-diff.c, clang 3.2 says:\n>\n>     combine-diff.c:1006:19: warning: adding 'int' to a string does not\n>             append to the string [-Wstring-plus-int]\n>                 prefix = COLONS + offset;\n>                          ~~~~~~~^~~~~~~~\n>     combine-diff.c:1006:19: note: use array indexing to silence this warning\n>                 prefix = COLONS + offset;\n>                                 ^\n>                          &      [       ]\n>\n> Suppress this by making the suggested change.\n>\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n>  combine-diff.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/combine-diff.c b/combine-diff.c\n> index bb1cc96..dba4748 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n>                 offset = strlen(COLONS) - num_parent;\n>                 if (offset < 0)\n>                         offset = 0;\n> -               prefix = COLONS + offset;\n> +               prefix = &COLONS[offset];\n>\n>                 /* Show the modes */\n>                 for (i = 0; i < num_parent; i++) {\n> --\n\nHmm, does\n\n               prefix = (const char *) COLONS + offset;\n\nsuppress the warning?\n\n--\nCheers,\nRay Chuan\n"},{"id":"208555","messageId":"20130203190621.GT1342@serenity.lan","threadId":"32817","inReplyTo":"CALUzUxowrh53g50ZxkXSjLfOrSgX-YiZEB2MJXbLwxmwNB187A@mail.gmail.com","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-03T19:06:21Z","receivedAt":"2013-02-03T19:06:21Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Feb 04, 2013 at 02:20:06AM +0800, Tay Ray Chuan wrote:\n> On Sun, Feb 3, 2013 at 10:37 PM, John Keeping <john@keeping.me.uk> wrote:\n> > When compiling combine-diff.c, clang 3.2 says:\n> >\n> >     combine-diff.c:1006:19: warning: adding 'int' to a string does not\n> >             append to the string [-Wstring-plus-int]\n> >                 prefix = COLONS + offset;\n> >                          ~~~~~~~^~~~~~~~\n> >     combine-diff.c:1006:19: note: use array indexing to silence this warning\n> >                 prefix = COLONS + offset;\n> >                                 ^\n> >                          &      [       ]\n> >\n> > Suppress this by making the suggested change.\n> >\n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> > ---\n> >  combine-diff.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/combine-diff.c b/combine-diff.c\n> > index bb1cc96..dba4748 100644\n> > --- a/combine-diff.c\n> > +++ b/combine-diff.c\n> > @@ -1003,7 +1003,7 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n> >                 offset = strlen(COLONS) - num_parent;\n> >                 if (offset < 0)\n> >                         offset = 0;\n> > -               prefix = COLONS + offset;\n> > +               prefix = &COLONS[offset];\n> >\n> >                 /* Show the modes */\n> >                 for (i = 0; i < num_parent; i++) {\n> \n> Hmm, does\n> \n>                prefix = (const char *) COLONS + offset;\n> \n> suppress the warning?\n\nIt does, but it turns out that the following also suppresses the\nwarning:\n\n-- >8 --\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex bb1cc96..a07d329 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -982,7 +982,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tfree(sline);\n }\n \n-#define COLONS \"::::::::::::::::::::::::::::::::\"\n+static const char COLONS[] = \"::::::::::::::::::::::::::::::::\";\n \n static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)\n {\n\nI think that's a nicer change than the original suggestion.\n\n\nJohn\n"},{"id":"208556","messageId":"20130203193816.GA3221@elie.Belkin","threadId":"32817","inReplyTo":"a9fe675ed9b34d3c15f4678ee13e90cddaa36055.1359901732.git.john@keeping.me.uk","subject":"Re: [PATCH 1/3] fix clang -Wtautological-compare with unsigned enum","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-03T19:38:28Z","receivedAt":"2013-02-03T19:38:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"John Keeping wrote:\n\n> From: Antoine Pelisse <apelisse@gmail.com>\n>\n> Create a GREP_HEADER_FIELD_MIN so we can check that the field value is\n> sane and silent the clang warning.\n\nThanks.  Looks good to me.\n\n[...]\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -625,7 +625,8 @@ static struct grep_expr *prep_header_patterns(struct grep_opt *opt)\n>  \tfor (p = opt->header_list; p; p = p->next) {\n>  \t\tif (p->token != GREP_PATTERN_HEAD)\n>  \t\t\tdie(\"bug: a non-header pattern in grep header list.\");\n> -\t\tif (p->field < 0 || GREP_HEADER_FIELD_MAX <= p->field)\n> +\t\tif (p->field < GREP_HEADER_FIELD_MIN ||\n> +\t\t    GREP_HEADER_FIELD_MAX <= p->field)\n>  \t\t\tdie(\"bug: unknown header field %d\", p->field);\n\nI also think it would be fine to drop this test or replace it with an\n\n\tassert((unsigned) p->field < ARRAY_SIZE(header_field));\n\nbecause we know the test never trips.\n"},{"id":"208560","messageId":"7vwqup890o.fsf@alter.siamese.dyndns.org","threadId":"32817","inReplyTo":"6995fd5e4d9cb3320ab80c983f1b25ae8a399284.1359901732.git.john@keeping.me.uk","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-03T19:58:15Z","receivedAt":"2013-02-03T19:58:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> When compiling combine-diff.c, clang 3.2 says:\n>\n>     combine-diff.c:1006:19: warning: adding 'int' to a string does not\n> \t    append to the string [-Wstring-plus-int]\n> \t\tprefix = COLONS + offset;\n> \t\t\t ~~~~~~~^~~~~~~~\n>     combine-diff.c:1006:19: note: use array indexing to silence this warning\n> \t\tprefix = COLONS + offset;\n> \t\t\t\t^\n> \t\t\t &      [       ]\n>\n> Suppress this by making the suggested change.\n>\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n\nThis was not lost in the noise.\n\nI thought that this wasn't a serious patch, but your attempt to\ndemonstrate to others why patches trying to squelch clang warnings\nare not necessarily a good thing to do.\n\nWho is that compiler trying to help with such a warning message?\nAfter all, we are writing in C, and clang is supposed to be a C\ncompiler.  And adding integer to a pointer to (const) char is a\nstraight-forward way to look at the trailing part of a given string.\n\n> -\t\tprefix = COLONS + offset;\n> +\t\tprefix = &COLONS[offset];\n\nIn other words, both are perfectly valid C.  Why should we make it\nless readable to avoid a stupid compiler warning?\n"},{"id":"208564","messageId":"20130203203150.GU1342@serenity.lan","threadId":"32817","inReplyTo":"7vwqup890o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-03T20:31:50Z","receivedAt":"2013-02-03T20:31:50Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Feb 03, 2013 at 11:58:15AM -0800, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > When compiling combine-diff.c, clang 3.2 says:\n> >\n> >     combine-diff.c:1006:19: warning: adding 'int' to a string does not\n> > \t    append to the string [-Wstring-plus-int]\n> > \t\tprefix = COLONS + offset;\n> > \t\t\t ~~~~~~~^~~~~~~~\n> >     combine-diff.c:1006:19: note: use array indexing to silence this warning\n> > \t\tprefix = COLONS + offset;\n> > \t\t\t\t^\n> > \t\t\t &      [       ]\n> >\n> > Suppress this by making the suggested change.\n> >\n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> > ---\n> \n> This was not lost in the noise.\n> \n> I thought that this wasn't a serious patch, but your attempt to\n> demonstrate to others why patches trying to squelch clang warnings\n> are not necessarily a good thing to do.\n>\n> Who is that compiler trying to help with such a warning message?\n> After all, we are writing in C, and clang is supposed to be a C\n> compiler.  And adding integer to a pointer to (const) char is a\n> straight-forward way to look at the trailing part of a given string.\n\nA quick search turned up the original thread where this feature was\nadded to Clang [1].  It seems that it does find genuine bugs where\npeople try to log values by doing:\n\n    log(\"failed to handle error: \" + errno);\n\n[1] http://thread.gmane.org/gmane.comp.compilers.clang.scm/47203\n\n> > -\t\tprefix = COLONS + offset;\n> > +\t\tprefix = &COLONS[offset];\n> \n> In other words, both are perfectly valid C.  Why should we make it\n> less readable to avoid a stupid compiler warning?\n\nAre you happy to change COLONS to a const char[] instead of a #define?\nThat also suppresses the warning.\n\nSince Git is warning-free on GCC and so close to being warning-free on\nrecent Clang I think it is worthwhile to fix the remaining two issues\nwhich do seem to be intentional diagnostics rather than Clang bugs.\n\n\nJohn\n"},{"id":"208566","messageId":"7vd2wh86m4.fsf@alter.siamese.dyndns.org","threadId":"32817","inReplyTo":"2cac21192f79f9fbb5822417775954eba29064fa.1359901732.git.john@keeping.me.uk","subject":"Re: [PATCH 3/3] builtin/apply: tighten (dis)similarity index parsing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-03T20:50:11Z","receivedAt":"2013-02-03T20:50:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 6c11e8b..4745e75 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -1041,15 +1041,17 @@ static int gitdiff_renamedst(const char *line, struct patch *patch)\n>  \n>  static int gitdiff_similarity(const char *line, struct patch *patch)\n>  {\n> -\tif ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n> -\t\tpatch->score = 0;\n> +\tunsigned long val = strtoul(line, NULL, 10);\n> +\tif (val <= 100)\n> +\t\tpatch->score = val;\n>  \treturn 0;\n>  }\n>  \n>  static int gitdiff_dissimilarity(const char *line, struct patch *patch)\n>  {\n> -\tif ((patch->score = strtoul(line, NULL, 10)) == ULONG_MAX)\n> -\t\tpatch->score = 0;\n> +\tunsigned long val = strtoul(line, NULL, 10);\n> +\tif (val <= 100)\n> +\t\tpatch->score = val;\n>  \treturn 0;\n>  }\n\nThis makes sort of sense; .score is used only for display and not\nfor making any decision, so as long as you know it is initialized to\nzero when the call to this function is made, it should be OK.\n\nThanks.\n"},{"id":"208567","messageId":"7v8v7585sr.fsf@alter.siamese.dyndns.org","threadId":"32817","inReplyTo":"20130203203150.GU1342@serenity.lan","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-03T21:07:48Z","receivedAt":"2013-02-03T21:07:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> A quick search turned up the original thread where this feature was\n> added to Clang [1].  It seems that it does find genuine bugs where\n> people try to log values by doing:\n>\n>     log(\"failed to handle error: \" + errno);\n\nTo be perfectly honest, anybody who writes such a code should be\nsent back to school before trying to touch out code ever again ;-).\nIt is not even valid Python, Perl nor Java, I would think.\n\n> Are you happy to change COLONS to a const char[] instead of a #define?\n\nHappy?  Not really.\n\nIt could be a good change for entirely different reason. We will\nsave space if we ever need to use it in multiple places.  But the\nentire \"COLONS + offset\" thing was a hack we did, knowing that it\nwill break when we end up showing a muiti-way diff for more than 32\nblobs.\n\nIf we were to be touching that area of code, I'd rather see a change\nto make it more robust against such a corner case.  If it results in\nsquelching misguided clang warnings against programmers who should\nnot be writing in C, that is a nice side effect, but I loathe to see\nany change whose primary purpose is to squelch pointless warnings.\n\n combine-diff.c | 21 +++++++--------------\n 1 file changed, 7 insertions(+), 14 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex bb1cc96..7f6187f 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -982,14 +982,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tfree(sline);\n }\n \n-#define COLONS \"::::::::::::::::::::::::::::::::\"\n-\n static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)\n {\n \tstruct diff_options *opt = &rev->diffopt;\n-\tint i, offset;\n-\tconst char *prefix;\n-\tint line_termination, inter_name_termination;\n+\tint line_termination, inter_name_termination, i;\n \n \tline_termination = opt->line_termination;\n \tinter_name_termination = '\\t';\n@@ -1000,17 +996,14 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \t\tshow_log(rev);\n \n \tif (opt->output_format & DIFF_FORMAT_RAW) {\n-\t\toffset = strlen(COLONS) - num_parent;\n-\t\tif (offset < 0)\n-\t\t\toffset = 0;\n-\t\tprefix = COLONS + offset;\n+\t\t/* As many colons as there are parents */\n+\t\tfor (i = 0; i < num_parent; i++)\n+\t\t\tputchar(':');\n \n \t\t/* Show the modes */\n-\t\tfor (i = 0; i < num_parent; i++) {\n-\t\t\tprintf(\"%s%06o\", prefix, p->parent[i].mode);\n-\t\t\tprefix = \" \";\n-\t\t}\n-\t\tprintf(\"%s%06o\", prefix, p->mode);\n+\t\tfor (i = 0; i < num_parent; i++)\n+\t\t\tprintf(\"%06o \", p->parent[i].mode);\n+\t\tprintf(\"%06o\", p->mode);\n \n \t\t/* Show sha1's */\n \t\tfor (i = 0; i < num_parent; i++)\n"},{"id":"208574","messageId":"20130203231549.GV1342@serenity.lan","threadId":"32817","inReplyTo":"7v8v7585sr.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-03T23:15:49Z","receivedAt":"2013-02-03T23:15:49Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Feb 03, 2013 at 01:07:48PM -0800, Junio C Hamano wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> \n> > A quick search turned up the original thread where this feature was\n> > added to Clang [1].  It seems that it does find genuine bugs where\n> > people try to log values by doing:\n> >\n> >     log(\"failed to handle error: \" + errno);\n> \n> To be perfectly honest, anybody who writes such a code should be\n> sent back to school before trying to touch out code ever again ;-).\n\nYeah, I can't see that getting through review here :-).\n\n> It is not even valid Python, Perl nor Java, I would think.\n\nIt is valid Java, although I can't think of any other languages that let\nyou do that.\n\n> > Are you happy to change COLONS to a const char[] instead of a #define?\n> \n> Happy?  Not really.\n> \n> It could be a good change for entirely different reason. We will\n> save space if we ever need to use it in multiple places.  But the\n> entire \"COLONS + offset\" thing was a hack we did, knowing that it\n> will break when we end up showing a muiti-way diff for more than 32\n> blobs.\n> \n> If we were to be touching that area of code, I'd rather see a change\n> to make it more robust against such a corner case.  If it results in\n> squelching misguided clang warnings against programmers who should\n> not be writing in C, that is a nice side effect, but I loathe to see\n> any change whose primary purpose is to squelch pointless warnings.\n\nThis seems like a sensible change.\n\nI generally like to get rid of the pointless warnings so that the useful\nones can't hide in the noise.  Perhaps \"CFLAGS += -Wno-string-plus-int\"\nwould be better for this particular warning, but when there's only one\nbit of code that triggers it, tweaking that seemed simpler.\n\n>  combine-diff.c | 21 +++++++--------------\n>  1 file changed, 7 insertions(+), 14 deletions(-)\n> \n> diff --git a/combine-diff.c b/combine-diff.c\n> index bb1cc96..7f6187f 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -982,14 +982,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n>  \tfree(sline);\n>  }\n>  \n> -#define COLONS \"::::::::::::::::::::::::::::::::\"\n> -\n>  static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)\n>  {\n>  \tstruct diff_options *opt = &rev->diffopt;\n> -\tint i, offset;\n> -\tconst char *prefix;\n> -\tint line_termination, inter_name_termination;\n> +\tint line_termination, inter_name_termination, i;\n>  \n>  \tline_termination = opt->line_termination;\n>  \tinter_name_termination = '\\t';\n> @@ -1000,17 +996,14 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n>  \t\tshow_log(rev);\n>  \n>  \tif (opt->output_format & DIFF_FORMAT_RAW) {\n> -\t\toffset = strlen(COLONS) - num_parent;\n> -\t\tif (offset < 0)\n> -\t\t\toffset = 0;\n> -\t\tprefix = COLONS + offset;\n> +\t\t/* As many colons as there are parents */\n> +\t\tfor (i = 0; i < num_parent; i++)\n> +\t\t\tputchar(':');\n>  \n>  \t\t/* Show the modes */\n> -\t\tfor (i = 0; i < num_parent; i++) {\n> -\t\t\tprintf(\"%s%06o\", prefix, p->parent[i].mode);\n> -\t\t\tprefix = \" \";\n> -\t\t}\n> -\t\tprintf(\"%s%06o\", prefix, p->mode);\n> +\t\tfor (i = 0; i < num_parent; i++)\n> +\t\t\tprintf(\"%06o \", p->parent[i].mode);\n> +\t\tprintf(\"%06o\", p->mode);\n>  \n>  \t\t/* Show sha1's */\n>  \t\tfor (i = 0; i < num_parent; i++)\n"},{"id":"208577","messageId":"7vip696i3v.fsf@alter.siamese.dyndns.org","threadId":"32817","inReplyTo":"20130203231549.GV1342@serenity.lan","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-04T00:24:52Z","receivedAt":"2013-02-04T00:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n>> If we were to be touching that area of code, I'd rather see a change\n>> to make it more robust against such a corner case.  If it results in\n>> squelching misguided clang warnings against programmers who should\n>> not be writing in C, that is a nice side effect, but I loathe to see\n>> any change whose primary purpose is to squelch pointless warnings.\n>\n> This seems like a sensible change.\n>\n> I generally like to get rid of the pointless warnings so that the useful\n> ones can't hide in the noise.  Perhaps \"CFLAGS += -Wno-string-plus-int\"\n> would be better for this particular warning, but when there's only one\n> bit of code that triggers it, tweaking that seemed simpler.\n\nThanks for a sanity check.  Ideally it should also have test cases\nto show \"git diff --cc --raw blob1 blob2...blob$n\" for n=4 and n=40\n(or any two values clearly below and above the old hardcoded limit)\nbehave sensibly, exposing the old breakage, which I'll leave as a\nLHF (low-hanging-fruit).  Hint, hint...\n\n-- >8 --\nSubject: [PATCH] combine-diff: lift 32-way limit of combined diff\n\nThe \"raw\" format of combine-diff output is supposed to have as many\ncolons as there are parents at the beginning, then blob modes for\nthese parents, and then object names for these parents.\n\nWe weren't however prepared to handle a more than 32-way merge and\ndid not show the correct number of colons in such a case.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n combine-diff.c | 21 +++++++--------------\n 1 file changed, 7 insertions(+), 14 deletions(-)\n\ndiff --git a/combine-diff.c b/combine-diff.c\nindex bb1cc96..7f6187f 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -982,14 +982,10 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \tfree(sline);\n }\n \n-#define COLONS \"::::::::::::::::::::::::::::::::\"\n-\n static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)\n {\n \tstruct diff_options *opt = &rev->diffopt;\n-\tint i, offset;\n-\tconst char *prefix;\n-\tint line_termination, inter_name_termination;\n+\tint line_termination, inter_name_termination, i;\n \n \tline_termination = opt->line_termination;\n \tinter_name_termination = '\\t';\n@@ -1000,17 +996,14 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re\n \t\tshow_log(rev);\n \n \tif (opt->output_format & DIFF_FORMAT_RAW) {\n-\t\toffset = strlen(COLONS) - num_parent;\n-\t\tif (offset < 0)\n-\t\t\toffset = 0;\n-\t\tprefix = COLONS + offset;\n+\t\t/* As many colons as there are parents */\n+\t\tfor (i = 0; i < num_parent; i++)\n+\t\t\tputchar(':');\n \n \t\t/* Show the modes */\n-\t\tfor (i = 0; i < num_parent; i++) {\n-\t\t\tprintf(\"%s%06o\", prefix, p->parent[i].mode);\n-\t\t\tprefix = \" \";\n-\t\t}\n-\t\tprintf(\"%s%06o\", prefix, p->mode);\n+\t\tfor (i = 0; i < num_parent; i++)\n+\t\t\tprintf(\"%06o \", p->parent[i].mode);\n+\t\tprintf(\"%06o\", p->mode);\n \n \t\t/* Show sha1's */\n \t\tfor (i = 0; i < num_parent; i++)\n-- \n1.8.1.2.628.geb8a6d5\n"},{"id":"208759","messageId":"20130205202558.GX1342@serenity.lan","threadId":"32817","inReplyTo":"7vip696i3v.fsf@alter.siamese.dyndns.org","subject":"[PATCH] t4038: add tests for \"diff --cc --raw <trees>\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-05T20:25:58Z","receivedAt":"2013-02-05T20:25:58Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Signed-off-by: John Keeping <john@keeping.me.uk>\n---\nOn Sun, Feb 03, 2013 at 04:24:52PM -0800, Junio C Hamano wrote:\n>                             Ideally it should also have test cases\n> to show \"git diff --cc --raw blob1 blob2...blob$n\" for n=4 and n=40\n> (or any two values clearly below and above the old hardcoded limit)\n> behave sensibly, exposing the old breakage, which I'll leave as a\n> LHF (low-hanging-fruit).  Hint, hint...\n\nHint taken ;-)\n\ngit-diff uses a different code path for blobs, so I've had to use trees\nto trigger this.  The last test fails without\njc/combine-diff-many-parents and passes with it.\n\n t/t4038-diff-combined.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\nindex 40277c7..a0701bc 100755\n--- a/t/t4038-diff-combined.sh\n+++ b/t/t4038-diff-combined.sh\n@@ -89,4 +89,33 @@ test_expect_success 'diagnose truncated file' '\n \tgrep \"diff --cc file\" out\n '\n \n+test_expect_success 'setup for --cc --raw' '\n+\tblob=$(echo file |git hash-object --stdin -w) &&\n+\tbase_tree=$(echo \"100644 blob $blob\tfile\" | git mktree) &&\n+\ttrees= &&\n+\tfor i in `test_seq 1 40`\n+\tdo\n+\t\tblob=$(echo file$i |git hash-object --stdin -w) &&\n+\t\ttrees=\"$trees $(echo \"100644 blob $blob\tfile\" |git mktree)\"\n+\tdone\n+'\n+\n+test_expect_success 'check --cc --raw with four trees' '\n+\tfour_trees=$(echo \"$trees\" |awk -e \"{\n+\t\tprint \\$1\n+\t\tprint \\$2\n+\t\tprint \\$3\n+\t\tprint \\$4\n+\t}\") &&\n+\tgit diff --cc --raw $four_trees $base_tree >out &&\n+\t# Check for four leading colons in the output:\n+\tgrep \"^::::[^:]\" out\n+'\n+\n+test_expect_success 'check --cc --raw with forty trees' '\n+\tgit diff --cc --raw $trees $base_tree >out &&\n+\t# Check for forty leading colons in the output:\n+\tgrep \"^::::::::::::::::::::::::::::::::::::::::[^:]\" out\n+'\n+\n test_done\n-- \n1.8.1.2\n"},{"id":"208762","messageId":"7v8v72sczp.fsf@alter.siamese.dyndns.org","threadId":"32817","inReplyTo":"20130205202558.GX1342@serenity.lan","subject":"Re: [PATCH] t4038: add tests for \"diff --cc --raw <trees>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-05T20:48:58Z","receivedAt":"2013-02-05T20:48:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n> ...\n> diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\n> index 40277c7..a0701bc 100755\n> --- a/t/t4038-diff-combined.sh\n> +++ b/t/t4038-diff-combined.sh\n> @@ -89,4 +89,33 @@ test_expect_success 'diagnose truncated file' '\n>  \tgrep \"diff --cc file\" out\n>  '\n>  \n> +test_expect_success 'setup for --cc --raw' '\n> +\tblob=$(echo file |git hash-object --stdin -w) &&\n> +\tbase_tree=$(echo \"100644 blob $blob\tfile\" | git mktree) &&\n> +\ttrees= &&\n> +\tfor i in `test_seq 1 40`\n> +\tdo\n> +\t\tblob=$(echo file$i |git hash-object --stdin -w) &&\n> +\t\ttrees=\"$trees $(echo \"100644 blob $blob\tfile\" |git mktree)\"\n\nPlease have a SP after each of these '|' pipes.\n\nIf you collect trees this way:\n\n\ttrees=\"$trees$(echo ... | git mktree)$LF\"\n\nthen ...\n\n> +\tdone\n> +'\n> +\n> +test_expect_success 'check --cc --raw with four trees' '\n> +\tfour_trees=$(echo \"$trees\" |awk -e \"{\n> +\t\tprint \\$1\n> +\t\tprint \\$2\n> +\t\tprint \\$3\n> +\t\tprint \\$4\n> +\t}\") &&\n\n(What's \"awk -e\"?)\n\n... you can do\n\n\techo \"$trees\" | sed -e 4q\n\nwhich is less repetitive.\n\nThanks.\n"},{"id":"208767","messageId":"20130205213949.GY1342@serenity.lan","threadId":"32817","inReplyTo":"7v8v72sczp.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] t4038: add tests for \"diff --cc --raw <trees>\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-05T21:39:49Z","receivedAt":"2013-02-05T21:39:49Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Signed-off-by: John Keeping <john@keeping.me.uk>\n\n---\nChanges since v1:\n\n- more spaces around '|'\n- create trees with line feeds and use 'sed -e 4q'\n---\n t/t4038-diff-combined.sh | 24 ++++++++++++++++++++++++\n 1 file changed, 24 insertions(+)\n\ndiff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh\nindex 40277c7..614425a 100755\n--- a/t/t4038-diff-combined.sh\n+++ b/t/t4038-diff-combined.sh\n@@ -89,4 +89,28 @@ test_expect_success 'diagnose truncated file' '\n \tgrep \"diff --cc file\" out\n '\n \n+test_expect_success 'setup for --cc --raw' '\n+\tblob=$(echo file | git hash-object --stdin -w) &&\n+\tbase_tree=$(echo \"100644 blob $blob\tfile\" | git mktree) &&\n+\ttrees= &&\n+\tfor i in `test_seq 1 40`\n+\tdo\n+\t\tblob=$(echo file$i | git hash-object --stdin -w) &&\n+\t\ttrees=\"$trees$(echo \"100644 blob $blob\tfile\" | git mktree)$LF\"\n+\tdone\n+'\n+\n+test_expect_success 'check --cc --raw with four trees' '\n+\tfour_trees=$(echo \"$trees\" | sed -e 4q) &&\n+\tgit diff --cc --raw $four_trees $base_tree >out &&\n+\t# Check for four leading colons in the output:\n+\tgrep \"^::::[^:]\" out\n+'\n+\n+test_expect_success 'check --cc --raw with forty trees' '\n+\tgit diff --cc --raw $trees $base_tree >out &&\n+\t# Check for forty leading colons in the output:\n+\tgrep \"^::::::::::::::::::::::::::::::::::::::::[^:]\" out\n+'\n+\n test_done\n-- \n1.8.1.2.689.g36c777b\n"},{"id":"208772","messageId":"7vehguqtvi.fsf@alter.siamese.dyndns.org","threadId":"32817","inReplyTo":"20130205213949.GY1342@serenity.lan","subject":"Re: [PATCH v2] t4038: add tests for \"diff --cc --raw <trees>\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-05T22:27:13Z","receivedAt":"2013-02-05T22:27:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"208893","messageId":"876224sqwk.fsf@catnip.gol.com","threadId":"32817","inReplyTo":"20130203231549.GV1342@serenity.lan","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2013-02-07T04:12:59Z","receivedAt":"2013-02-07T04:12:59Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"John Keeping <john@keeping.me.uk> writes:\n> I generally like to get rid of the pointless warnings so that the useful\n> ones can't hide in the noise.  Perhaps \"CFLAGS += -Wno-string-plus-int\"\n> would be better for this particular warning, but when there's only one\n> bit of code that triggers it, tweaking that seemed simpler.\n\nAn even better approach would be to file a bug against clang ... it\nreally is a very ill-considered warning -- PTR + OFFS is not just\nvalid C, it's _idiomatic_ in C for getting interior pointers into\narrays -- and such a warning should never be enabled by default, or by\nany standard warning options.\n\n-miles \n\n-- \n永日の　澄んだ紺から　永遠へ\n"},{"id":"208900","messageId":"20130207084106.GB1342@serenity.lan","threadId":"32817","inReplyTo":"876224sqwk.fsf@catnip.gol.com","subject":"Re: [PATCH 2/3] combine-diff: suppress a clang warning","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-07T08:41:06Z","receivedAt":"2013-02-07T08:41:06Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Feb 07, 2013 at 01:12:59PM +0900, Miles Bader wrote:\n> John Keeping <john@keeping.me.uk> writes:\n> > I generally like to get rid of the pointless warnings so that the useful\n> > ones can't hide in the noise.  Perhaps \"CFLAGS += -Wno-string-plus-int\"\n> > would be better for this particular warning, but when there's only one\n> > bit of code that triggers it, tweaking that seemed simpler.\n> \n> An even better approach would be to file a bug against clang ... it\n> really is a very ill-considered warning -- PTR + OFFS is not just\n> valid C, it's _idiomatic_ in C for getting interior pointers into\n> arrays -- and such a warning should never be enabled by default, or by\n> any standard warning options.\n\nIt doesn't warn of PTR + OFFS, only STRING_LITERAL + OFFS.  I agree that\nit's not a particularly useful warning but it was clearly introduced\nintentionally and appears to find real bugs [1] so I don't intend to\nargue about it with the Clang developers.\n\n[1] http://article.gmane.org/gmane.comp.compilers.clang.scm/47203\n\n\nJohn\n"}]}