{"thread":{"id":"33244","subject":"[PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","startedAt":"2013-03-21T11:03:38Z","lastAt":"2013-03-25T21:55:16Z","messageCount":42,"participants":["Jeff King","Johannes Sixt","Joachim Schmitz","Junio C Hamano","Erik Faye-Lund","Jonathan Nieder","René Scharfe","Torsten Bögershausen"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"211846","messageId":"20130321110338.GA18552@sigill.intra.peff.net","threadId":"33244","inReplyTo":null,"subject":"[PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T11:03:38Z","receivedAt":"2013-03-21T11:03:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was fooling around with clang and noticed that it complains about the\n\"int x = x\" construct under -Wall. That is IMHO a deficiency in clang,\nsince the idiom has a well-defined use in silencing -Wuninitialized\nwarnings. But I've also always been nervous about the idiom, because\nit's easy to get the analysis wrong (after all, the compiler gets these\ncases wrong because they're complex), and it's possible for the code to\nchange later, introducing a new problem.\n\nSo I investigated our uses of the idiom. Many of them are correct and\nstill necessary. Some are correct but no longer necessary with modern\ngcc. And some are technically correct, but the code and its assumptions\ncan be made clearer (to both a reader and the compiler) with a simple\nrewrite. Patches are below for the latter two types.\n\nNote that none of these fixes an actual bug in the current code; this is\npurely maintenance hygiene. Nor do any of the patches depend on each\nother; we can drop any of them that do not look they are providing a net\nbenefit.\n\n [1/4]: wt-status: fix possible use of uninitialized variable\n [2/4]: fast-import: use pointer-to-pointer to keep list tail\n [3/4]: drop some obsolete \"x = x\" compiler warning hacks\n [4/4]: transport: drop \"int cmp = cmp\" hack\n\n-Peff\n"},{"id":"211847","messageId":"20130321110527.GA18819@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130321110338.GA18552@sigill.intra.peff.net","subject":"[PATCH 1/4] wt-status: fix possible use of uninitialized variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T11:05:28Z","receivedAt":"2013-03-21T11:05:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In wt_status_print_change_data, we accept a change_type flag\nthat is meant to be either WT_STATUS_UPDATED or\nWT_STATUS_CHANGED. We then switch() on this value to set\nthe local variable \"status\" for each case, but do not\nprovide a fallback \"default\" label to the switch statement.\n\nAs a result, the compiler realizes that \"status\" might be\nunset, and complains with a warning. To silence this\nwarning, we use the \"int status = status\" trick. This is\ncorrect with the current code, as all callers provide one of\nthe two expected change_type flags. However, it's also a\nmaintenance trap, as there is nothing to prevent future\ncallers from passing another flag, nor to document this\nassumption.\n\nInstead of using the \"x = x\" hack, let's handle the default\ncase in the switch() statement with a die(\"BUG\"). That tells\nthe compiler and any readers of the code exactly what the\nfunction's input assumptions are.\n\nWe could also convert the flag to an enum, which would\nprovide a compile-time check on the function input. However,\nsince these flags are part of a larger enum, that would make\nthe code unnecessarily complex (we would have to make a new\nenum with just the two flags, and then convert it to the old\nenum for passing to sub-functions).\n\nSigned-off-by: Jeff King \n---\n wt-status.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/wt-status.c b/wt-status.c\nindex ef405d0..7555817 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -264,7 +264,7 @@ static void wt_status_print_change_data(struct wt_status *s,\n {\n \tstruct wt_status_change_data *d = it->util;\n \tconst char *c = color(change_type, s);\n-\tint status = status;\n+\tint status;\n \tchar *one_name;\n \tchar *two_name;\n \tconst char *one, *two;\n@@ -292,6 +292,9 @@ static void wt_status_print_change_data(struct wt_status *s,\n \t\t}\n \t\tstatus = d->worktree_status;\n \t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: unhandled change_type %d in wt_status_print_change_data\",\n+\t\t change_type);\n \t}\n \n \tone = quote_path(one_name, -1, &onebuf, s->prefix);\n-- \n1.8.2.rc2.8.g2161951\n"},{"id":"211848","messageId":"20130321110817.GB18819@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130321110338.GA18552@sigill.intra.peff.net","subject":"[PATCH 2/4] fast-import: use pointer-to-pointer to keep list tail","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T11:08:17Z","receivedAt":"2013-03-21T11:08:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is shorter, idiomatic, and it means the compiler does\nnot get confused about whether our \"e\" pointer is valid,\nletting us drop the \"e = e\" hack.\n\nSigned-off-by: Jeff King \n---\nAnd it fixes an instance of Linus's \"people do not understand pointers\"\nfrom here:\n\n http://meta.slashdot.org/story/12/10/11/0030249/linus-torvalds-answers-your-questions\n\n fast-import.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex c2a814e..583a439 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2613,7 +2613,7 @@ static struct hash_list *parse_merge(unsigned int *count)\n \n static struct hash_list *parse_merge(unsigned int *count)\n {\n-\tstruct hash_list *list = NULL, *n, *e = e;\n+\tstruct hash_list *list = NULL, **tail = &list, *n;\n \tconst char *from;\n \tstruct branch *s;\n \n@@ -2641,11 +2641,9 @@ static struct hash_list *parse_merge(unsigned int *count)\n \t\t\tdie(\"Invalid ref name or SHA1 expression: %s\", from);\n \n \t\tn->next = NULL;\n-\t\tif (list)\n-\t\t\te->next = n;\n-\t\telse\n-\t\t\tlist = n;\n-\t\te = n;\n+\t\t*tail = n;\n+\t\ttail = &n->next;\n+\n \t\t(*count)++;\n \t\tread_next_command();\n \t}\n-- \n1.8.2.rc2.8.g2161951\n"},{"id":"211849","messageId":"20130321111028.GC18819@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130321110338.GA18552@sigill.intra.peff.net","subject":"[PATCH 3/4] drop some obsolete \"x = x\" compiler warning hacks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T11:10:28Z","receivedAt":"2013-03-21T11:10:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In cases where the setting and access of a variable are\nprotected by the same conditional flag, older versions of\ngcc would generate a \"might be used unitialized\" warning. We\nsilence the warning by initializing the variable to itself,\na hack that gcc recognizes.\n\nModern versions of gcc are smart enough to get this right,\ngoing back to at least version 4.3.5. gcc 4.1 does get it\nwrong in both cases, but is sufficiently old that we\nprobably don't need to care about it anymore.\n\nSigned-off-by: Jeff King \n---\ngcc 4.2 is conspicuously missing because no current Debian system even\nhas a backwards-compatibility package for it, making it harder to test.\nAnd 4.3 was old enough for me to say \"I do not care if you can run with\n-Wall -Werror or not\", let alone 4.2.\n\n builtin/cat-file.c | 2 +-\n fast-import.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 00528dd..ad29000 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -193,7 +193,7 @@ static int batch_one_object(const char *obj_name, int print_contents)\n \tunsigned char sha1[20];\n \tenum object_type type = 0;\n \tunsigned long size;\n-\tvoid *contents = contents;\n+\tvoid *contents;\n \n \tif (!obj_name)\n \t return 1;\ndiff --git a/fast-import.c b/fast-import.c\nindex 583a439..e12a8b8 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2434,7 +2434,7 @@ static void note_change_n(struct branch *b, unsigned char *old_fanout)\n {\n \tconst char *p = command_buf.buf + 2;\n \tstatic struct strbuf uq = STRBUF_INIT;\n-\tstruct object_entry *oe = oe;\n+\tstruct object_entry *oe;\n \tstruct branch *s;\n \tunsigned char sha1[20], commit_sha1[20];\n \tchar path[60];\n-- \n1.8.2.rc2.8.g2161951\n"},{"id":"211850","messageId":"20130321111333.GD18819@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130321110338.GA18552@sigill.intra.peff.net","subject":"[PATCH 4/4] transport: drop \"int cmp = cmp\" hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T11:13:33Z","receivedAt":"2013-03-21T11:13:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"According to 47ec794, this initialization is meant to\nsquelch an erroneous uninitialized variable warning from gcc\n4.0.1. That version is quite old at this point, and gcc 4.1\nand up handle it fine, with one exception. There seems to be\na regression in gcc 4.6.3, which produces the warning;\nhowever, gcc versions 4.4.7 and 4.7.2 do not.\n\nSigned-off-by: Jeff King \n---\nWe probably _don't_ want to apply this one right now. The regression in\n4.6 means some people on reasonably modern systems probably would still\nsee the warning. Debian stable ships with 4.4, and testing/unstable\ndefaults to 4.7 (though you can install a gcc-4.6 compatibility\npackage). But I have no clue if other distros made releases with 4.6.\n\n transport.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/transport.c b/transport.c\nindex 886ffd8..87b8f14 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -106,7 +106,7 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)\n \t\treturn;\n \n \tfor (;;) {\n-\t\tint cmp = cmp, len;\n+\t\tint cmp, len;\n \n \t\tif (!fgets(buffer, sizeof(buffer), f)) {\n \t\t\tfclose(f);\n-- \n1.8.2.rc2.8.g2161951\n"},{"id":"211851","messageId":"514AF2E1.7020409@viscovery.net","threadId":"33244","inReplyTo":"20130321110338.GA18552@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2013-03-21T11:45:37Z","receivedAt":"2013-03-21T11:45:37Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 3/21/2013 12:03, schrieb Jeff King:\n> I was fooling around with clang and noticed that it complains about the\n> \"int x = x\" construct under -Wall. That is IMHO a deficiency in clang,\n> since the idiom has a well-defined use in silencing -Wuninitialized\n> warnings.\n\nIMO, that's a myth. The construct invokes undefined behavior at least\nsince C99, and the compilers are right to complain about it.\n\nBut you might just say that standards are not worth the paper they are\nprinted on, and you may possibly be right for practical reasons. But I\nstill consider it a myth that \"int x = x\" is an idiom. I'm in the C\nbusiness since more than 25 years, and the first time I saw the \"idiom\"\nwas in git code. Is there any evidence that the construct is used\nelsewhere? Have I been in the wrong corner of the C world for such a long\ntime?\n\n-- Hannes\n"},{"id":"211852","messageId":"20130321115545.GB21319@sigill.intra.peff.net","threadId":"33244","inReplyTo":"514AF2E1.7020409@viscovery.net","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T11:55:45Z","receivedAt":"2013-03-21T11:55:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 21, 2013 at 12:45:37PM +0100, Johannes Sixt wrote:\n\n> Am 3/21/2013 12:03, schrieb Jeff King:\n> > I was fooling around with clang and noticed that it complains about the\n> > \"int x = x\" construct under -Wall. That is IMHO a deficiency in clang,\n> > since the idiom has a well-defined use in silencing -Wuninitialized\n> > warnings.\n> \n> IMO, that's a myth. The construct invokes undefined behavior at least\n> since C99, and the compilers are right to complain about it.\n\nWhile undefined behavior does leave the compiler free to do anything,\nincluding nasal demons, it would be a very poor implementation that did\nanything except leave random bytes in the value. And it also means that\ngcc is free to take it as a hint to silence the warning; given that\nclang tries to be compatible with gcc, I'd think it would want to do the\nsame. But I may be wrong that the behavior from gcc is intentional or\ncommon (see below).\n\n> But you might just say that standards are not worth the paper they are\n> printed on, and you may possibly be right for practical reasons. But I\n> still consider it a myth that \"int x = x\" is an idiom. I'm in the C\n> business since more than 25 years, and the first time I saw the \"idiom\"\n> was in git code. Is there any evidence that the construct is used\n> elsewhere? Have I been in the wrong corner of the C world for such a long\n> time?\n\nGit code was my introduction to it, too, and I was led to believe it was\nidiomatic, so I can't speak further on that. I think it was Junio who\nintroduced me to it, so maybe he can shed more light on the history.\n\n-Peff\n"},{"id":"211866","messageId":"kif2so$2ug$1@ger.gmane.org","threadId":"33244","inReplyTo":"514AF2E1.7020409@viscovery.net","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-03-21T13:44:51Z","receivedAt":"2013-03-21T13:44:51Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Johannes Sixt wrote:\n> Am 3/21/2013 12:03, schrieb Jeff King:\n>> I was fooling around with clang and noticed that it complains about\n>> the \"int x = x\" construct under -Wall. That is IMHO a deficiency in\n>> clang, since the idiom has a well-defined use in silencing\n>> -Wuninitialized warnings.\n>\n> IMO, that's a myth. The construct invokes undefined behavior at least\n> since C99, and the compilers are right to complain about it.\n\nAnd I complained about this a couple months ago, as the compiler on \nHP-NonStop stumbles across this too (by emitting a warning)\n\nBye, Jojo \n"},{"id":"211868","messageId":"kif3ig$au5$1@ger.gmane.org","threadId":"33244","inReplyTo":"kif2so$2ug$1@ger.gmane.org","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-03-21T13:56:28Z","receivedAt":"2013-03-21T13:56:28Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Joachim Schmitz wrote:\n> Johannes Sixt wrote:\n>> Am 3/21/2013 12:03, schrieb Jeff King:\n>>> I was fooling around with clang and noticed that it complains about\n>>> the \"int x = x\" construct under -Wall. That is IMHO a deficiency in\n>>> clang, since the idiom has a well-defined use in silencing\n>>> -Wuninitialized warnings.\n>> \n>> IMO, that's a myth. The construct invokes undefined behavior at least\n>> since C99, and the compilers are right to complain about it.\n> \n> And I complained about this a couple months ago, as the compiler on\n\nActually on August 20th, 2012...\n\n> HP-NonStop stumbles across this too (by emitting a warning)\n\n\nBye, Jojo\n"},{"id":"211870","messageId":"7vppysbxzo.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"20130321115545.GB21319@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-21T14:58:51Z","receivedAt":"2013-03-21T14:58:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Git code was my introduction to it, too, and I was led to believe it was\n> idiomatic, so I can't speak further on that. I think it was Junio who\n> introduced me to it, so maybe he can shed more light on the history.\n\nI think we picked the convention up from the kernel folks. At least\nthat is how I first met the construct. The uninitialized_var(x)\nmacro was (and still is) used to mark these \"The compiler is too\ndumb to realize, but we know what we are doing\" cases:\n\n $ git grep '#define uninitialized_var' include/\n include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x\n include/linux/compiler-intel.h:#define uninitialized_var(x) x\n\nbut they recently had a discussion, e.g.\n\n http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705\n\nso...\n"},{"id":"211872","messageId":"CABPQNSadzAFqJq8=zi36BdL9Qcoi-WsoEj0yhdAZ4GvvkyHfVQ@mail.gmail.com","threadId":"33244","inReplyTo":"20130321111028.GC18819@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] drop some obsolete \"x = x\" compiler warning hacks","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2013-03-21T15:16:00Z","receivedAt":"2013-03-21T15:16:00Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Thu, Mar 21, 2013 at 12:10 PM, Jeff King wrote:\n> In cases where the setting and access of a variable are\n> protected by the same conditional flag, older versions of\n> gcc would generate a \"might be used unitialized\" warning. We\n> silence the warning by initializing the variable to itself,\n> a hack that gcc recognizes.\n>\n> Modern versions of gcc are smart enough to get this right,\n> going back to at least version 4.3.5. gcc 4.1 does get it\n> wrong in both cases, but is sufficiently old that we\n> probably don't need to care about it anymore.\n>\n> Signed-off-by: Jeff King \n> ---\n> gcc 4.2 is conspicuously missing because no current Debian system even\n> has a backwards-compatibility package for it, making it harder to test.\n> And 4.3 was old enough for me to say \"I do not care if you can run with\n> -Wall -Werror or not\", let alone 4.2.\n\nJust a data-point. This is the version we use in msysGit:\n\n$ gcc --version\ngcc.exe (TDM-1 mingw32) 4.4.0\n\nSo yeah, it's not going to increase false positives here, I guess.\n"},{"id":"211873","messageId":"7vhak4bx0w.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"7vppysbxzo.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-21T15:19:43Z","receivedAt":"2013-03-21T15:19:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> Git code was my introduction to it, too, and I was led to believe it was\n>> idiomatic, so I can't speak further on that. I think it was Junio who\n>> introduced me to it, so maybe he can shed more light on the history.\n>\n> I think we picked the convention up from the kernel folks. At least\n> that is how I first met the construct. The uninitialized_var(x)\n> macro was (and still is) used to mark these \"The compiler is too\n> dumb to realize, but we know what we are doing\" cases:\n>\n> $ git grep '#define uninitialized_var' include/\n> include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x\n> include/linux/compiler-intel.h:#define uninitialized_var(x) x\n>\n> but they recently had a discussion, e.g.\n>\n> http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705\n>\n> so...\n\nWhile flipping the paragraphs around before sending the message out\nI managed to lose the important one. Here is roughly what I wrote:\n\n I am for dropping \"= x\" and leaving it uninitialized at the\n declaration site, or explicitly initializing it to some\n reasonable starting value (e.g. NULL if it is a pointer) and\n adding a comment to say that the initialization is to squelch\n compiler warnings.\n"},{"id":"211876","messageId":"20130321154402.GA25907@sigill.intra.peff.net","threadId":"33244","inReplyTo":"7vhak4bx0w.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T15:44:02Z","receivedAt":"2013-03-21T15:44:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 21, 2013 at 08:19:43AM -0700, Junio C Hamano wrote:\n\n> > $ git grep '#define uninitialized_var' include/\n> > include/linux/compiler-gcc.h:#define uninitialized_var(x) x = x\n> > include/linux/compiler-intel.h:#define uninitialized_var(x) x\n> >\n> > but they recently had a discussion, e.g.\n> >\n> > http://thread.gmane.org/gmane.linux.kernel.openipmi/1998/focus=1383705\n> >\n> > so...\n> \n> While flipping the paragraphs around before sending the message out\n> I managed to lose the important one. Here is roughly what I wrote:\n> \n> I am for dropping \"= x\" and leaving it uninitialized at the\n> declaration site, or explicitly initializing it to some\n> reasonable starting value (e.g. NULL if it is a pointer) and\n> adding a comment to say that the initialization is to squelch\n> compiler warnings.\n\nI'd be in favor of that, too. In many cases, I think the fact that gcc\ncannot trace the control flow is a good indication that it is hard for a\nhuman to trace it, too. And in those cases we would be better off\nrestructuring the code slightly to make it more obvious to both types of\nreaders.\n\nTwo patches to follow.\n\n [5/4]: fast-import: clarify \"inline\" logic in file_change_m\n [6/4]: run-command: always set failed_errno in start_command\n\n-Peff\n"},{"id":"211877","messageId":"20130321154439.GA2075@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130321154402.GA25907@sigill.intra.peff.net","subject":"[PATCH 5/4] fast-import: clarify \"inline\" logic in file_change_m","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T15:44:39Z","receivedAt":"2013-03-21T15:44:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we read a fast-import line like:\n\n M 100644 :1 foo.c\n\nwe point the local object_entry variable \"oe\" to the object\nnamed by the mark \":1\". When the input uses the \"inline\"\nconstruct, however, we do not have such an object_entry.\n\nThe current code is careful not to access \"oe\" in the inline\ncase, but we can make the assumption even more obvious (and\ncatch violations of it) by setting oe to NULL and adding a\ncomment. As a bonus, this also squelches an over-zealous gcc\n-Wuninitialized warning, which means we can drop the \"oe =\noe\" initialization hack.\n\nSigned-off-by: Jeff King \n---\n fast-import.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex e12a8b8..a0c2c2f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2265,7 +2265,7 @@ static void file_change_m(struct branch *b)\n \tconst char *p = command_buf.buf + 2;\n \tstatic struct strbuf uq = STRBUF_INIT;\n \tconst char *endp;\n-\tstruct object_entry *oe = oe;\n+\tstruct object_entry *oe;\n \tunsigned char sha1[20];\n \tuint16_t mode, inline_data = 0;\n \n@@ -2292,6 +2292,7 @@ static void file_change_m(struct branch *b)\n \t\thashcpy(sha1, oe->idx.sha1);\n \t} else if (!prefixcmp(p, \"inline \")) {\n \t\tinline_data = 1;\n+\t\toe = NULL; /* not used with inline_data, but makes gcc happy */\n \t\tp += strlen(\"inline\"); /* advance to space */\n \t} else {\n \t\tif (get_sha1_hex(p, sha1))\n-- \n1.8.2.rc2.8.g2161951\n"},{"id":"211878","messageId":"20130321154500.GB2075@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130321154402.GA25907@sigill.intra.peff.net","subject":"[PATCH 6/4] run-command: always set failed_errno in start_command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-21T15:45:00Z","receivedAt":"2013-03-21T15:45:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we fail to fork, we set the failed_errno variable to\nthe value of errno so it is not clobbered by later syscalls.\nHowever, we do so in a conditional, and it is hard to see\nlater under what conditions the variable has a valid value.\n\nInstead of setting it only when fork fails, let's just\nalways set it after forking. This is more obvious for human\nreaders (as we are no longer setting it as a side effect of\na strerror call), and it is more obvious to gcc, which no\nlonger generates a spurious -Wuninitialized warning. It also\nhappens to match what the WIN32 half of the #ifdef does.\n\nSigned-off-by: Jeff King \n---\n run-command.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 07e27ff..765c2ce 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -273,7 +273,7 @@ int start_command(struct child_process *cmd)\n {\n \tint need_in, need_out, need_err;\n \tint fdin[2], fdout[2], fderr[2];\n-\tint failed_errno = failed_errno;\n+\tint failed_errno;\n \tchar *str;\n \n \t/*\n@@ -341,6 +341,7 @@ fail_pipe:\n \t\tnotify_pipe[0] = notify_pipe[1] = -1;\n \n \tcmd->pid = fork();\n+\tfailed_errno = errno;\n \tif (!cmd->pid) {\n \t\t/*\n \t\t * Redirect the channel to write syscall error messages to\n@@ -420,7 +421,7 @@ fail_pipe:\n \t}\n \tif (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n-\t\t\tstrerror(failed_errno = errno));\n+\t\t\tstrerror(errno));\n \telse if (cmd->clean_on_exit)\n \t\tmark_child_for_cleanup(cmd->pid);\n \n-- \n1.8.2.rc2.8.g2161951\n"},{"id":"211887","messageId":"20130321194949.GG29311@google.com","threadId":"33244","inReplyTo":"20130321110527.GA18819@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-21T19:49:50Z","receivedAt":"2013-03-21T19:49:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Instead of using the \"x = x\" hack, let's handle the default\n> case in the switch() statement with a die(\"BUG\"). That tells\n> the compiler and any readers of the code exactly what the\n> function's input assumptions are.\n\nSounds reasonable.\n\n> We could also convert the flag to an enum, which would\n> provide a compile-time check on the function input.\n\nUnfortunately C permits out-of-bounds values for enums.\n\n[...]\n> --- a/wt-status.c\n> +++ b/wt-status.c\n> @@ -264,7 +264,7 @@ static void wt_status_print_change_data(struct wt_status *s,\n> {\n> \tstruct wt_status_change_data *d = it->util;\n> \tconst char *c = color(change_type, s);\n> -\tint status = status;\n> +\tint status;\n> \tchar *one_name;\n> \tchar *two_name;\n> \tconst char *one, *two;\n> @@ -292,6 +292,9 @@ static void wt_status_print_change_data(struct wt_status *s,\n> \t\t}\n> \t\tstatus = d->worktree_status;\n> \t\tbreak;\n> +\tdefault:\n> +\t\tdie(\"BUG: unhandled change_type %d in wt_status_print_change_data\",\n> +\t\t change_type);\n\nMicronit: s/unhandled/invalid/.\n\nThanks,\nJonathan\n"},{"id":"211890","messageId":"7vip4ka5pb.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"20130321194949.GG29311@google.com","subject":"Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-21T19:55:12Z","receivedAt":"2013-03-21T19:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jeff King wrote:\n>\n>> Instead of using the \"x = x\" hack, let's handle the default\n>> case in the switch() statement with a die(\"BUG\"). That tells\n>> the compiler and any readers of the code exactly what the\n>> function's input assumptions are.\n>\n> Sounds reasonable.\n>\n>> We could also convert the flag to an enum, which would\n>> provide a compile-time check on the function input.\n>\n> Unfortunately C permits out-of-bounds values for enums.\n>\n> [...]\n>> --- a/wt-status.c\n>> +++ b/wt-status.c\n>> @@ -264,7 +264,7 @@ static void wt_status_print_change_data(struct wt_status *s,\n>> {\n>> \tstruct wt_status_change_data *d = it->util;\n>> \tconst char *c = color(change_type, s);\n>> -\tint status = status;\n>> +\tint status;\n>> \tchar *one_name;\n>> \tchar *two_name;\n>> \tconst char *one, *two;\n>> @@ -292,6 +292,9 @@ static void wt_status_print_change_data(struct wt_status *s,\n>> \t\t}\n>> \t\tstatus = d->worktree_status;\n>> \t\tbreak;\n>> +\tdefault:\n>> +\t\tdie(\"BUG: unhandled change_type %d in wt_status_print_change_data\",\n>> +\t\t change_type);\n>\n> Micronit: s/unhandled/invalid/.\n\nI actually think \"unhandled\" is more correct for this one; we may\nadd new change_type later in the caller, and we do not want to\nforget to add a new case arm that handles the new value.\n"},{"id":"211892","messageId":"20130321195804.GI29311@google.com","threadId":"33244","inReplyTo":"7vip4ka5pb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-21T19:58:04Z","receivedAt":"2013-03-21T19:58:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>> Jeff King wrote:\n\n>>> +\tdefault:\n>>> +\t\tdie(\"BUG: unhandled change_type %d in wt_status_print_change_data\",\n>>> +\t\t change_type);\n>>\n>> Micronit: s/unhandled/invalid/.\n>\n> I actually think \"unhandled\" is more correct for this one; we may\n> add new change_type later in the caller, and we do not want to\n> forget to add a new case arm that handles the new value.\n\nOk. Makes sense.\n"},{"id":"211897","messageId":"20130321204355.GK29311@google.com","threadId":"33244","inReplyTo":"20130321110817.GB18819@sigill.intra.peff.net","subject":"Re: [PATCH 2/4] fast-import: use pointer-to-pointer to keep list tail","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-21T20:43:55Z","receivedAt":"2013-03-21T20:43:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> This is shorter, idiomatic, and it means the compiler does\n> not get confused about whether our \"e\" pointer is valid,\n> letting us drop the \"e = e\" hack.\n>\n> Signed-off-by: Jeff King \n> ---\n> And it fixes an instance of Linus's \"people do not understand pointers\"\n\nHeh. Yes, looks correct. For what it's worth,\nReviewed-by: Jonathan Nieder \n"},{"id":"211898","messageId":"20130321204709.GL29311@google.com","threadId":"33244","inReplyTo":"20130321111028.GC18819@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] drop some obsolete \"x = x\" compiler warning hacks","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-21T20:47:09Z","receivedAt":"2013-03-21T20:47:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> And 4.3 was old enough for me to say \"I do not care if you can run with\n> -Wall -Werror or not\", let alone 4.2.\n\nChanges like this can only reveal bugs (in git or optimizers) that\nwere hidden before, without regressing actual runtime behavior, so for\nwhat it's worth I like them.\n\nI think perhaps we should encourage people to use\n-Wno-error=uninitialized, in addition to cleaning up our code where\nreasonably recent optimizers reveal it to be confusing.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"211899","messageId":"20130321205948.GM29311@google.com","threadId":"33244","inReplyTo":"20130321111333.GD18819@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] transport: drop \"int cmp = cmp\" hack","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-21T20:59:48Z","receivedAt":"2013-03-21T20:59:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> We probably _don't_ want to apply this one right now.\n\nI think we should. gcc 4.6.y warning bugs should be fixed --- there's\nno need for git to work around them. And anyone affected can easily\nstop using -Werror (-Werror is not meant for use by non-developers in\nproduction).\n\nSo fwiw\nReviewed-by: Jonathan Nieder \n"},{"id":"211900","messageId":"20130321210235.GN29311@google.com","threadId":"33244","inReplyTo":"20130321154402.GA25907@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-03-21T21:02:35Z","receivedAt":"2013-03-21T21:02:35Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Two patches to follow.\n>\n> [5/4]: fast-import: clarify \"inline\" logic in file_change_m\n\nThis one is clearly a bug / missing feature in gcc's control flow\nanalysis, but your workaround looks reasonable.\n\n> [6/4]: run-command: always set failed_errno in start_command\n\nVery sane. Thanks.\n"},{"id":"211952","messageId":"20130322161540.GF3083@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130321194949.GG29311@google.com","subject":"Re: [PATCH 1/4] wt-status: fix possible use of uninitialized variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-22T16:15:41Z","receivedAt":"2013-03-22T16:15:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 21, 2013 at 12:49:50PM -0700, Jonathan Nieder wrote:\n\n> > We could also convert the flag to an enum, which would\n> > provide a compile-time check on the function input.\n> \n> Unfortunately C permits out-of-bounds values for enums.\n\nTrue, although I would think that most compilers take the hint for\nswitch() statements that handling all defined constants for an enum is\nenough (certainly gcc does it with the \"some enum constants not handled\"\nwarning, but I did not actually check whether it does so in the\nuninitialized-warning control flow checker).\n\nStill, I'm happy enough with the die(\"BUG\") that I posted, so we don't\nneed to worry about it.\n\n-Peff\n"},{"id":"211953","messageId":"20130322161837.GG3083@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130321154402.GA25907@sigill.intra.peff.net","subject":"Re: [PATCH 0/4] drop some \"int x = x\" hacks to silence gcc warnings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-22T16:18:37Z","receivedAt":"2013-03-22T16:18:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 21, 2013 at 11:44:02AM -0400, Jeff King wrote:\n\n> > I am for dropping \"= x\" and leaving it uninitialized at the\n> > declaration site, or explicitly initializing it to some\n> > reasonable starting value (e.g. NULL if it is a pointer) and\n> > adding a comment to say that the initialization is to squelch\n> > compiler warnings.\n> \n> I'd be in favor of that, too. In many cases, I think the fact that gcc\n> cannot trace the control flow is a good indication that it is hard for a\n> human to trace it, too. And in those cases we would be better off\n> restructuring the code slightly to make it more obvious to both types of\n> readers.\n> \n> Two patches to follow.\n> \n> [5/4]: fast-import: clarify \"inline\" logic in file_change_m\n> [6/4]: run-command: always set failed_errno in start_command\n\nAnd here are two more; with these, our code base should be free of \"x =\nx\" initializations (at least according to clang).\n\n [7/4]: submodule: clarify logic in show_submodule_summary\n [8/4]: match-trees: drop \"x = x\" initializations\n\nNot pressing, obviously, but since I had just analyzed the code\nyesterday, I wanted to do it while they were still fresh in my mind.\n\n-Peff\n"},{"id":"211955","messageId":"20130322161955.GA25857@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130322161837.GG3083@sigill.intra.peff.net","subject":"[PATCH 7/4] submodule: clarify logic in show_submodule_summary","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-22T16:19:56Z","receivedAt":"2013-03-22T16:19:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are two uses of the \"left\" and \"right\" commit\nvariables that make it hard to be sure what values they\nhave (both for the reader, and for gcc, which wrongly\ncomplains that they might be used uninitialized).\n\nThe functions starts with a cascading if statement, checking\nthat the input sha1s exist, and finally working up to\npreparing a revision walk. We only prepare the walk if the\ncascading conditional did not find any problems, which we\ncheck by seeing whether it set the \"message\" variable or\nnot. It's simpler and more obvious to just add a condition\nto the end of the cascade.\n\nLater, we check the same \"message\" variable when deciding\nwhether to clear commit marks on the left/right commits; if\nit is set, we presumably never started the walk. This is\nwrong, though; we might have started the walk and munged\ncommit flags, only to encounter an error afterwards. We\nshould always clear the flags on left/right if they exist,\nwhether the walk was successful or not.\n\nSigned-off-by: Jeff King \n---\n submodule.c | 13 ++++++-------\n 1 file changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 9ba1496..975bc87 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -261,7 +261,7 @@ void show_submodule_summary(FILE *f, const char *path,\n \t\tconst char *del, const char *add, const char *reset)\n {\n \tstruct rev_info rev;\n-\tstruct commit *left = left, *right = right;\n+\tstruct commit *left = NULL, *right = NULL;\n \tconst char *message = NULL;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fast_forward = 0, fast_backward = 0;\n@@ -275,10 +275,8 @@ void show_submodule_summary(FILE *f, const char *path,\n \telse if (!(left = lookup_commit_reference(one)) ||\n \t\t !(right = lookup_commit_reference(two)))\n \t\tmessage = \"(commits not present)\";\n-\n-\tif (!message &&\n-\t prepare_submodule_summary(&rev, path, left, right,\n-\t\t\t\t\t&fast_forward, &fast_backward))\n+\telse if (prepare_submodule_summary(&rev, path, left, right,\n+\t\t\t\t\t &fast_forward, &fast_backward))\n \t\tmessage = \"(revision walker failed)\";\n \n \tif (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n@@ -302,11 +300,12 @@ void show_submodule_summary(FILE *f, const char *path,\n \t\tstrbuf_addf(&sb, \"%s:%s\\n\", fast_backward ? \" (rewind)\" : \"\", reset);\n \tfwrite(sb.buf, sb.len, 1, f);\n \n-\tif (!message) {\n+\tif (!message) /* only NULL if we succeeded in setting up the walk */\n \t\tprint_submodule_summary(&rev, f, del, add, reset);\n+\tif (left)\n \t\tclear_commit_marks(left, ~0);\n+\tif (right)\n \t\tclear_commit_marks(right, ~0);\n-\t}\n \n \tstrbuf_release(&sb);\n }\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"211956","messageId":"20130322162155.GB25857@sigill.intra.peff.net","threadId":"33244","inReplyTo":"20130322161837.GG3083@sigill.intra.peff.net","subject":"[PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-22T16:21:55Z","receivedAt":"2013-03-22T16:21:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These nonsense assignments are meant to squelch gcc warnings\nthat the variables might be used uninitialized. However, gcc\ngets it mostly right, realizing that we will either\nextract tree entries from both sides, or we will hit a\n\"continue\" statement and go to the top of the loop.\n\nHowever, while getting this right for the \"elem\" and \"path\"\nvariables, it does not do so for the \"mode\" variables. Let's\ndrop the nonsense initialization where modern gcc does not\nneed them, and just set the modes to \"0\", along with a\ncomment. These values should never be used, but it makes\nboth gcc, as well as any compiler which does not like the \"x\n= x\" initializations, happy.\n\nWhile we're in the area, let's also update the loop\ncondition to use logical-OR rather than bitwise-OR. They should\nbe equivalent in this case, and the use of the latter was\nprobably a typo.\n\nSigned-off-by: Jeff King \n---\nOf the 8 patches, this is the one I find the least satisfying, if only\nbecause I do not think gcc's failure is because of complicated control\nflow, and rearranging the code would only hurt readability. And I'm\nquite curious why it complains about \"mode\", but not about the other\nvariables, which are set in the exact same place (and why it would not\nbe able to handle such a simple control flow at all).\n\nIt makes me wonder if I am missing something, or there is some subtle\nbug. But I can't see it. Other eyes appreciated.\n\n match-trees.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/match-trees.c b/match-trees.c\nindex 26f7ed1..4360f10 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -71,13 +71,13 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)\n \tif (type != OBJ_TREE)\n \t\tdie(\"%s is not a tree\", sha1_to_hex(hash2));\n \tinit_tree_desc(&two, two_buf, size);\n-\twhile (one.size | two.size) {\n-\t\tconst unsigned char *elem1 = elem1;\n-\t\tconst unsigned char *elem2 = elem2;\n-\t\tconst char *path1 = path1;\n-\t\tconst char *path2 = path2;\n-\t\tunsigned mode1 = mode1;\n-\t\tunsigned mode2 = mode2;\n+\twhile (one.size || two.size) {\n+\t\tconst unsigned char *elem1;\n+\t\tconst unsigned char *elem2;\n+\t\tconst char *path1;\n+\t\tconst char *path2;\n+\t\tunsigned mode1 = 0; /* make gcc happy */\n+\t\tunsigned mode2 = 0; /* make gcc happy */\n \t\tint cmp;\n \n \t\tif (one.size)\n-- \n1.8.2.13.g0f18d3c\n"},{"id":"211983","messageId":"7v620j6sz1.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"20130322161955.GA25857@sigill.intra.peff.net","subject":"Re: [PATCH 7/4] submodule: clarify logic in show_submodule_summary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T21:10:42Z","receivedAt":"2013-03-22T21:10:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There are two uses of the \"left\" and \"right\" commit\n> variables that make it hard to be sure what values they\n> have (both for the reader, and for gcc, which wrongly\n> complains that they might be used uninitialized).\n>\n> The functions starts with a cascading if statement, checking\n> that the input sha1s exist, and finally working up to\n> preparing a revision walk. We only prepare the walk if the\n> cascading conditional did not find any problems, which we\n> check by seeing whether it set the \"message\" variable or\n> not. It's simpler and more obvious to just add a condition\n> to the end of the cascade.\n>\n> Later, we check the same \"message\" variable when deciding\n> whether to clear commit marks on the left/right commits; if\n> it is set, we presumably never started the walk. This is\n> wrong, though; we might have started the walk and munged\n> commit flags, only to encounter an error afterwards. We\n> should always clear the flags on left/right if they exist,\n> whether the walk was successful or not.\n>\n> Signed-off-by: Jeff King \n> ---\n\nLooks good. Thanks.\n\n> submodule.c | 13 ++++++-------\n> 1 file changed, 6 insertions(+), 7 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 9ba1496..975bc87 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -261,7 +261,7 @@ void show_submodule_summary(FILE *f, const char *path,\n> \t\tconst char *del, const char *add, const char *reset)\n> {\n> \tstruct rev_info rev;\n> -\tstruct commit *left = left, *right = right;\n> +\tstruct commit *left = NULL, *right = NULL;\n> \tconst char *message = NULL;\n> \tstruct strbuf sb = STRBUF_INIT;\n> \tint fast_forward = 0, fast_backward = 0;\n> @@ -275,10 +275,8 @@ void show_submodule_summary(FILE *f, const char *path,\n> \telse if (!(left = lookup_commit_reference(one)) ||\n> \t\t !(right = lookup_commit_reference(two)))\n> \t\tmessage = \"(commits not present)\";\n> -\n> -\tif (!message &&\n> -\t prepare_submodule_summary(&rev, path, left, right,\n> -\t\t\t\t\t&fast_forward, &fast_backward))\n> +\telse if (prepare_submodule_summary(&rev, path, left, right,\n> +\t\t\t\t\t &fast_forward, &fast_backward))\n> \t\tmessage = \"(revision walker failed)\";\n> \n> \tif (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)\n> @@ -302,11 +300,12 @@ void show_submodule_summary(FILE *f, const char *path,\n> \t\tstrbuf_addf(&sb, \"%s:%s\\n\", fast_backward ? \" (rewind)\" : \"\", reset);\n> \tfwrite(sb.buf, sb.len, 1, f);\n> \n> -\tif (!message) {\n> +\tif (!message) /* only NULL if we succeeded in setting up the walk */\n> \t\tprint_submodule_summary(&rev, f, del, add, reset);\n> +\tif (left)\n> \t\tclear_commit_marks(left, ~0);\n> +\tif (right)\n> \t\tclear_commit_marks(right, ~0);\n> -\t}\n> \n> \tstrbuf_release(&sb);\n> }\n"},{"id":"211987","messageId":"7v1ub76s8c.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"20130322162155.GB25857@sigill.intra.peff.net","subject":"Re: [PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T21:26:43Z","receivedAt":"2013-03-22T21:26:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Of the 8 patches, this is the one I find the least satisfying, if only\n> because I do not think gcc's failure is because of complicated control\n> flow, and rearranging the code would only hurt readability. And I'm\n> quite curious why it complains about \"mode\", but not about the other\n> variables, which are set in the exact same place (and why it would not\n> be able to handle such a simple control flow at all).\n>\n> It makes me wonder if I am missing something, or there is some subtle\n> bug. But I can't see it. Other eyes appreciated.\n\nI obviously am not qualified as \"other eyes\" to catch bugs in this\ncode as this is entirely mine, but I do not see any obvious reason\nthat would make the compiler to think mode[12] less initialized than\nelem[12] or path[12] either.\n\nThese three are all updated by the same tree_entry_extract() call,\nand whenever we use mode[12] we use path[12], so if it decides path1\nis used or assigned, it should be able to tell mode1 is, too.\n\nUnsatisfactory, it surely is...\n\n> match-trees.c | 14 +++++++-------\n> 1 file changed, 7 insertions(+), 7 deletions(-)\n>\n> diff --git a/match-trees.c b/match-trees.c\n> index 26f7ed1..4360f10 100644\n> --- a/match-trees.c\n> +++ b/match-trees.c\n> @@ -71,13 +71,13 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)\n> \tif (type != OBJ_TREE)\n> \t\tdie(\"%s is not a tree\", sha1_to_hex(hash2));\n> \tinit_tree_desc(&two, two_buf, size);\n> -\twhile (one.size | two.size) {\n> -\t\tconst unsigned char *elem1 = elem1;\n> -\t\tconst unsigned char *elem2 = elem2;\n> -\t\tconst char *path1 = path1;\n> -\t\tconst char *path2 = path2;\n> -\t\tunsigned mode1 = mode1;\n> -\t\tunsigned mode2 = mode2;\n> +\twhile (one.size || two.size) {\n> +\t\tconst unsigned char *elem1;\n> +\t\tconst unsigned char *elem2;\n> +\t\tconst char *path1;\n> +\t\tconst char *path2;\n> +\t\tunsigned mode1 = 0; /* make gcc happy */\n> +\t\tunsigned mode2 = 0; /* make gcc happy */\n> \t\tint cmp;\n> \n> \t\tif (one.size)\n"},{"id":"211988","messageId":"7vtxo35dcz.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"7v1ub76s8c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-22T21:33:16Z","receivedAt":"2013-03-22T21:33:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> These three are all updated by the same tree_entry_extract() call,\n> and whenever we use mode[12] we use path[12], so if it decides path1\n> is used or assigned, it should be able to tell mode1 is, too.\n>\n> Unsatisfactory, it surely is...\n\nAnd immediately after I wrote the above, I am greeted by this:\n\n gcc (Debian 4.4.5-8) 4.4.5\n match-trees.c:75: error: 'elem1' may be used uninitialized in this function\n match-trees.c:77: error: 'path1' may be used uninitialized in this function\n\nand this crazy one on top squelches it.\n\nIf you flip the order of four lines that extracts only when size is\nnon-zero to extract first from two into elem2, then the warning is\ngiven for elem2/path2 but not for elem1/path1.\n\nI'll initialize all of them to nonsense values for now.\n\ndiff --git a/match-trees.c b/match-trees.c\nindex 4360f10..88981e8 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -72,9 +72,9 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)\n \t\tdie(\"%s is not a tree\", sha1_to_hex(hash2));\n \tinit_tree_desc(&two, two_buf, size);\n \twhile (one.size || two.size) {\n-\t\tconst unsigned char *elem1;\n+\t\tconst unsigned char *elem1 = NULL;\n \t\tconst unsigned char *elem2;\n-\t\tconst char *path1;\n+\t\tconst char *path1 = NULL;\n \t\tconst char *path2;\n \t\tunsigned mode1 = 0; /* make gcc happy */\n \t\tunsigned mode2 = 0; /* make gcc happy */\n"},{"id":"211989","messageId":"20130322213628.GA7471@sigill.intra.peff.net","threadId":"33244","inReplyTo":"7vtxo35dcz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-22T21:36:28Z","receivedAt":"2013-03-22T21:36:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 22, 2013 at 02:33:16PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > These three are all updated by the same tree_entry_extract() call,\n> > and whenever we use mode[12] we use path[12], so if it decides path1\n> > is used or assigned, it should be able to tell mode1 is, too.\n> >\n> > Unsatisfactory, it surely is...\n> \n> And immediately after I wrote the above, I am greeted by this:\n> \n> gcc (Debian 4.4.5-8) 4.4.5\n> match-trees.c:75: error: 'elem1' may be used uninitialized in this function\n> match-trees.c:77: error: 'path1' may be used uninitialized in this function\n> \n> and this crazy one on top squelches it.\n\nUgh, yeah, I should have tried with more compilers. 4.6 complains, but\n4.7 doesn't (although I still find it really weird that 4.7 gets it\n_half_ right).\n\n> I'll initialize all of them to nonsense values for now.\n\nI think that's sensible.\n\n-Peff\n"},{"id":"212062","messageId":"514DFB1A.8040102@lsrfire.ath.cx","threadId":"33244","inReplyTo":"20130322162155.GB25857@sigill.intra.peff.net","subject":"Re: [PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-03-23T18:57:30Z","receivedAt":"2013-03-23T18:57:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.03.2013 17:21, schrieb Jeff King:\n> Of the 8 patches, this is the one I find the least satisfying, if only\n> because I do not think gcc's failure is because of complicated control\n> flow, and rearranging the code would only hurt readability.\n\nHmm, let's see if we can help the compiler follow the code without\nmaking it harder for people to understand. The patch looks a bit\njumbled, but the resulting code is OK in my biased opinion.\n\n-- >8 --\nThere are two ways we can spot missing entries, i.e. added or removed\nfiles: By reaching the end of one of the trees while the other still\nhas entries, or in the middle of the two lists with base_name_compare().\nMissing files are handled the same in either case, but the code is\nduplicated.\n\nUnify the handling by just setting cmp appropriately when running off\na tree instead of handling the case on the spot. If both trees contain\nentries, call base_name_compare() as usual.\n\nThis make the code slightly shorter, and also helps gcc 4.6 to\nunderstand that none of the variables in the loop are used without\ninitialization. Therefore we can remove the trick to initialize them\nusing themselves, which was used to squelch false warnings.\n\n[Stolen from Jeff King:]\nWhile we're in the area, let's also update the loop\ncondition to use logical-OR rather than bitwise-OR. They should\nbe equivalent in this case, and the use of the latter was\nprobably a typo.\n\nSigned-off-by: Rene Scharfe \n---\n match-trees.c | 36 ++++++++++++++----------------------\n 1 file changed, 14 insertions(+), 22 deletions(-)\n\ndiff --git a/match-trees.c b/match-trees.c\nindex 26f7ed1..c0c66bb 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -71,34 +71,26 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)\n \tif (type != OBJ_TREE)\n \t\tdie(\"%s is not a tree\", sha1_to_hex(hash2));\n \tinit_tree_desc(&two, two_buf, size);\n-\twhile (one.size | two.size) {\n-\t\tconst unsigned char *elem1 = elem1;\n-\t\tconst unsigned char *elem2 = elem2;\n-\t\tconst char *path1 = path1;\n-\t\tconst char *path2 = path2;\n-\t\tunsigned mode1 = mode1;\n-\t\tunsigned mode2 = mode2;\n-\t\tint cmp;\n+\twhile (one.size || two.size) {\n+\t\tconst unsigned char *elem1, *elem2;\n+\t\tconst char *path1, *path2;\n+\t\tunsigned mode1, mode2;\n+\t\tint cmp = 0;\n \n \t\tif (one.size)\n \t\t\telem1 = tree_entry_extract(&one, &path1, &mode1);\n+\t\telse\n+\t\t\t/* two has more entries */\n+\t\t\tcmp = 1;\n \t\tif (two.size)\n \t\t\telem2 = tree_entry_extract(&two, &path2, &mode2);\n-\n-\t\tif (!one.size) {\n-\t\t\t/* two has more entries */\n-\t\t\tscore += score_missing(mode2, path2);\n-\t\t\tupdate_tree_entry(&two);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!two.size) {\n+\t\telse\n \t\t\t/* two lacks this entry */\n-\t\t\tscore += score_missing(mode1, path1);\n-\t\t\tupdate_tree_entry(&one);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tcmp = base_name_compare(path1, strlen(path1), mode1,\n-\t\t\t\t\tpath2, strlen(path2), mode2);\n+\t\t\tcmp = -1;\n+\n+\t\tif (!cmp)\n+\t\t\tcmp = base_name_compare(path1, strlen(path1), mode1,\n+\t\t\t\t\t\tpath2, strlen(path2), mode2);\n \t\tif (cmp < 0) {\n \t\t\t/* path1 does not appear in two */\n \t\t\tscore += score_missing(mode1, path1);\n-- \n1.8.2\n"},{"id":"212068","messageId":"CAPc5daVOksx56js_ascEr348PTLAZB9OeBrf3sELJUpdyB_kMg@mail.gmail.com","threadId":"33244","inReplyTo":"20130321111333.GD18819@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] transport: drop \"int cmp = cmp\" hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-24T04:00:05Z","receivedAt":"2013-03-24T04:00:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Mar 21, 2013 at 4:13 AM, Jeff King wrote:\n>\n> According to 47ec794, this initialization is meant to\n> squelch an erroneous uninitialized variable warning from gcc\n> 4.0.1. That version is quite old at this point, and gcc 4.1\n> and up handle it fine, with one exception. There seems to be\n> a regression in gcc 4.6.3, which produces the warning;\n> however, gcc versions 4.4.7 and 4.7.2 do not.\n>\n\ntransport.c: In function 'get_refs_via_rsync':\ntransport.c:127:29: error: 'cmp' may be used uninitialized in this\nfunction [-Werror=uninitialized]\ntransport.c:109:7: note: 'cmp' was declared here\n\ngcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3\n\n\nSigh...\n"},{"id":"212069","messageId":"7vli9d4crq.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"514DFB1A.8040102@lsrfire.ath.cx","subject":"Re: [PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-24T04:55:53Z","receivedAt":"2013-03-24T04:55:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Hmm, let's see if we can help the compiler follow the code without\n> making it harder for people to understand. The patch looks a bit\n> jumbled, but the resulting code is OK in my biased opinion.\n\nI actually think the result is much better than a mere \"OK\"; the\nduplicated \"at this point we know path1 (or path2) is missing from\nthe other side\" has been bothering me and I was about to suggest a\nsimilar rewrite before I read your message ;-)\n\nHowever, the same compiler still thinks {elem,path,mode}1 can be\nused uninitialized (but not {elem,path,mode}2). The craziness I\nreported in the previous message is also the same. With this patch\non top to swap the side we inspect first, the compiler thinks\n{elem,path,mode}2 can be used uninitialized but not the other three\nvariables X-<.\n\nSo I like your change for readability, but for GCC 4.4.5 we still\nneed the unnecessary initialization.\n\n match-trees.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/match-trees.c b/match-trees.c\nindex c0c66bb..9ea2c80 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -77,16 +77,16 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)\n \t\tunsigned mode1, mode2;\n \t\tint cmp = 0;\n \n-\t\tif (one.size)\n-\t\t\telem1 = tree_entry_extract(&one, &path1, &mode1);\n-\t\telse\n-\t\t\t/* two has more entries */\n-\t\t\tcmp = 1;\n \t\tif (two.size)\n \t\t\telem2 = tree_entry_extract(&two, &path2, &mode2);\n \t\telse\n \t\t\t/* two lacks this entry */\n \t\t\tcmp = -1;\n+\t\tif (one.size)\n+\t\t\telem1 = tree_entry_extract(&one, &path1, &mode1);\n+\t\telse\n+\t\t\t/* two has more entries */\n+\t\t\tcmp = 1;\n \n \t\tif (!cmp)\n \t\t\tcmp = base_name_compare(path1, strlen(path1), mode1,\n"},{"id":"212080","messageId":"514EA886.3090801@web.de","threadId":"33244","inReplyTo":"20130321204709.GL29311@google.com","subject":"Re: [PATCH 3/4] drop some obsolete \"x = x\" compiler warning hacks","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-03-24T07:17:26Z","receivedAt":"2013-03-24T07:17:26Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 21.03.13 21:47, Jonathan Nieder wrote:\n> Jeff King wrote:\n> \n>> And 4.3 was old enough for me to say \"I do not care if you can run with\n>> -Wall -Werror or not\", let alone 4.2.\n> \n> Changes like this can only reveal bugs (in git or optimizers) that\n> were hidden before, without regressing actual runtime behavior, so for\n> what it's worth I like them.\n> \n> I think perhaps we should encourage people to use\n> -Wno-error=uninitialized, in addition to cleaning up our code where\n> reasonably recent optimizers reveal it to be confusing.\n> \n> Reviewed-by: Jonathan Nieder \n\nI got 2 warnings, but reading the comments I feel that\n\nMac OS 10.6 and i686-apple-darwin10-gcc-4.2.1 (GCC) 4.2.1 (Apple Inc. build 5666) (dot 3)\n\nis outdated ;-)\n\n\nbuiltin/cat-file.c: In function ~cmd_cat_file~:\nbuiltin/cat-file.c:196: warning: ~contents~ may be used uninitialized in this function\nbuiltin/cat-file.c:196: note: ~contents~ was declared here\n\n\nfast-import.c: In function ‘parse_new_commit’:\nfast-import.c:2438: warning: ‘oe’ may be used uninitialized in this function\nfast-import.c:2438: note: ‘oe’ was declared here\n"},{"id":"212083","messageId":"20130324093212.GA28234@sigill.intra.peff.net","threadId":"33244","inReplyTo":"CAPc5daVOksx56js_ascEr348PTLAZB9OeBrf3sELJUpdyB_kMg@mail.gmail.com","subject":"Re: [PATCH 4/4] transport: drop \"int cmp = cmp\" hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-24T09:32:13Z","receivedAt":"2013-03-24T09:32:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 23, 2013 at 09:00:05PM -0700, Junio C Hamano wrote:\n\n> On Thu, Mar 21, 2013 at 4:13 AM, Jeff King wrote:\n> >\n> > According to 47ec794, this initialization is meant to\n> > squelch an erroneous uninitialized variable warning from gcc\n> > 4.0.1. That version is quite old at this point, and gcc 4.1\n> > and up handle it fine, with one exception. There seems to be\n> > a regression in gcc 4.6.3, which produces the warning;\n> > however, gcc versions 4.4.7 and 4.7.2 do not.\n> >\n> \n> transport.c: In function 'get_refs_via_rsync':\n> transport.c:127:29: error: 'cmp' may be used uninitialized in this\n> function [-Werror=uninitialized]\n> transport.c:109:7: note: 'cmp' was declared here\n> \n> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3\n\nRight, that's the same version I noted above. Is 4.6.3 the default\ncompiler under a particular release of Ubuntu, or did you use their\ngcc-4.6 package?\n\n-Peff\n"},{"id":"212084","messageId":"20130324100136.GA28884@sigill.intra.peff.net","threadId":"33244","inReplyTo":"7vli9d4crq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-24T10:01:36Z","receivedAt":"2013-03-24T10:01:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 23, 2013 at 09:55:53PM -0700, Junio C Hamano wrote:\n\n> René Scharfe writes:\n> \n> > Hmm, let's see if we can help the compiler follow the code without\n> > making it harder for people to understand. The patch looks a bit\n> > jumbled, but the resulting code is OK in my biased opinion.\n> \n> I actually think the result is much better than a mere \"OK\"; the\n> duplicated \"at this point we know path1 (or path2) is missing from\n> the other side\" has been bothering me and I was about to suggest a\n> similar rewrite before I read your message ;-)\n> \n> However, the same compiler still thinks {elem,path,mode}1 can be\n> used uninitialized (but not {elem,path,mode}2). The craziness I\n> reported in the previous message is also the same. With this patch\n> on top to swap the side we inspect first, the compiler thinks\n> {elem,path,mode}2 can be used uninitialized but not the other three\n> variables X-<.\n\nYeah, I'd agree that the result is more readable, but it does not\naddress the original problem. Junio, do you want to drop my patch,\nsquash the initialization of mode into René's version, and just add a\nnote to the commit message that we still have to deal with the gcc\nwarning?\n\n-Peff\n"},{"id":"212096","messageId":"514F13AC.6000100@web.de","threadId":"33244","inReplyTo":"20130324093212.GA28234@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] transport: drop \"int cmp = cmp\" hack","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-03-24T14:54:36Z","receivedAt":"2013-03-24T14:54:36Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 24.03.13 10:32, Jeff King wrote:\n> On Sat, Mar 23, 2013 at 09:00:05PM -0700, Junio C Hamano wrote:\n> \n>> On Thu, Mar 21, 2013 at 4:13 AM, Jeff King wrote:\n>>>\n>>> According to 47ec794, this initialization is meant to\n>>> squelch an erroneous uninitialized variable warning from gcc\n>>> 4.0.1. That version is quite old at this point, and gcc 4.1\n>>> and up handle it fine, with one exception. There seems to be\n>>> a regression in gcc 4.6.3, which produces the warning;\n>>> however, gcc versions 4.4.7 and 4.7.2 do not.\n>>>\n>>\n>> transport.c: In function 'get_refs_via_rsync':\n>> transport.c:127:29: error: 'cmp' may be used uninitialized in this\n>> function [-Werror=uninitialized]\n>> transport.c:109:7: note: 'cmp' was declared here\n>>\n>> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3\n> \n> Right, that's the same version I noted above. Is 4.6.3 the default\n> compiler under a particular release of Ubuntu, or did you use their\n> gcc-4.6 package?\n> \n> -Peff\nSide question:\nHow much does it hurt to write like this:\n\ndiff --git a/transport.c b/transport.c\nindex 6b2ae94..8020b62 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -106,7 +106,7 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)\n return;\n \n for (;;) {\n- int cmp, len;\n+ int cmp=0, len;\n \n==============\n\nUsing Ubuntu 10.4, using gcc (Ubuntu 4.4.3-4ubuntu5.1) 4.4.3\nthe compiler will add a line like this:\n\n 2e83:\t31 ff \txor %edi,%edi\n\n(Which should not be to slow to execute)\n\nLooking at a later gcc, from upcoming Debian, with gcc (Debian 4.7.2-5) 4.7.2\nthe assembly code is exactly the same ;-)\n"},{"id":"212138","messageId":"514F8244.8070702@lsrfire.ath.cx","threadId":"33244","inReplyTo":"7vli9d4crq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-03-24T22:46:28Z","receivedAt":"2013-03-24T22:46:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 24.03.2013 05:55, schrieb Junio C Hamano:\n> However, the same compiler still thinks {elem,path,mode}1 can be\n> used uninitialized (but not {elem,path,mode}2). The craziness I\n> reported in the previous message is also the same. With this patch\n> on top to swap the side we inspect first, the compiler thinks\n> {elem,path,mode}2 can be used uninitialized but not the other three\n> variables X-<.\n> \n> So I like your change for readability, but for GCC 4.4.5 we still\n> need the unnecessary initialization.\n\nHrm, perhaps we can make it even simpler for the compiler.\n\n-- >8 --\nSubject: match-trees: simplify score_trees() using tree_entry()\n\nConvert the loop in score_trees() to tree_entry(). The code becomes\nshorter and simpler because the calls to update_tree_entry() are not\nneeded any more.\n\nAnother benefit is that we need less variables to track the current\ntree entries; as a side-effect of that the compiler has an easier\njob figuring out the control flow and thus can avoid false warnings\nabout uninitialized variables.\n\nUsing struct name_entry also allows the use of tree_entry_len() for\nfinding the path length instead of strlen(), which may be slightly\nmore efficient.\n\nAlso unify the handling of missing entries in one of the two trees\n(i.e. added or removed files): Just set cmp appropriately first, no\nmatter if we ran off the end of a tree or if we actually have two\nentries to compare, and check its value a bit later without\nduplicating the handler code.\n\nSigned-off-by: Rene Scharfe \n---\nI'm a bit uneasy about this one because we lack proper tests for\nthis code and I don't know how to write ones off the bat.\n\n match-trees.c | 68 ++++++++++++++++++++++++-----------------------------------\n 1 file changed, 28 insertions(+), 40 deletions(-)\n\ndiff --git a/match-trees.c b/match-trees.c\nindex 26f7ed1..2bb734d 100644\n--- a/match-trees.c\n+++ b/match-trees.c\n@@ -47,6 +47,13 @@ static int score_matches(unsigned mode1, unsigned mode2, const char *path)\n \treturn score;\n }\n \n+static int base_name_entries_compare(const struct name_entry *a,\n+\t\t\t\t const struct name_entry *b)\n+{\n+\treturn base_name_compare(a->path, tree_entry_len(a), a->mode,\n+\t\t\t\t b->path, tree_entry_len(b), b->mode);\n+}\n+\n /*\n * Inspect two trees, and give a score that tells how similar they are.\n */\n@@ -71,54 +78,35 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)\n \tif (type != OBJ_TREE)\n \t\tdie(\"%s is not a tree\", sha1_to_hex(hash2));\n \tinit_tree_desc(&two, two_buf, size);\n-\twhile (one.size | two.size) {\n-\t\tconst unsigned char *elem1 = elem1;\n-\t\tconst unsigned char *elem2 = elem2;\n-\t\tconst char *path1 = path1;\n-\t\tconst char *path2 = path2;\n-\t\tunsigned mode1 = mode1;\n-\t\tunsigned mode2 = mode2;\n+\tfor (;;) {\n+\t\tstruct name_entry e1, e2;\n+\t\tint got_entry_from_one = tree_entry(&one, &e1);\n+\t\tint got_entry_from_two = tree_entry(&two, &e2);\n \t\tint cmp;\n \n-\t\tif (one.size)\n-\t\t\telem1 = tree_entry_extract(&one, &path1, &mode1);\n-\t\tif (two.size)\n-\t\t\telem2 = tree_entry_extract(&two, &path2, &mode2);\n-\n-\t\tif (!one.size) {\n-\t\t\t/* two has more entries */\n-\t\t\tscore += score_missing(mode2, path2);\n-\t\t\tupdate_tree_entry(&two);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!two.size) {\n+\t\tif (got_entry_from_one && got_entry_from_two)\n+\t\t\tcmp = base_name_entries_compare(&e1, &e2);\n+\t\telse if (got_entry_from_one)\n \t\t\t/* two lacks this entry */\n-\t\t\tscore += score_missing(mode1, path1);\n-\t\t\tupdate_tree_entry(&one);\n-\t\t\tcontinue;\n-\t\t}\n-\t\tcmp = base_name_compare(path1, strlen(path1), mode1,\n-\t\t\t\t\tpath2, strlen(path2), mode2);\n-\t\tif (cmp < 0) {\n+\t\t\tcmp = -1;\n+\t\telse if (got_entry_from_two)\n+\t\t\t/* two has more entries */\n+\t\t\tcmp = 1;\n+\t\telse\n+\t\t\tbreak;\n+\n+\t\tif (cmp < 0)\n \t\t\t/* path1 does not appear in two */\n-\t\t\tscore += score_missing(mode1, path1);\n-\t\t\tupdate_tree_entry(&one);\n-\t\t\tcontinue;\n-\t\t}\n-\t\telse if (cmp > 0) {\n+\t\t\tscore += score_missing(e1.mode, e1.path);\n+\t\telse if (cmp > 0)\n \t\t\t/* path2 does not appear in one */\n-\t\t\tscore += score_missing(mode2, path2);\n-\t\t\tupdate_tree_entry(&two);\n-\t\t\tcontinue;\n-\t\t}\n-\t\telse if (hashcmp(elem1, elem2))\n+\t\t\tscore += score_missing(e2.mode, e2.path);\n+\t\telse if (hashcmp(e1.sha1, e2.sha1))\n \t\t\t/* they are different */\n-\t\t\tscore += score_differs(mode1, mode2, path1);\n+\t\t\tscore += score_differs(e1.mode, e2.mode, e1.path);\n \t\telse\n \t\t\t/* same subtree or blob */\n-\t\t\tscore += score_matches(mode1, mode2, path1);\n-\t\tupdate_tree_entry(&one);\n-\t\tupdate_tree_entry(&two);\n+\t\t\tscore += score_matches(e1.mode, e2.mode, e1.path);\n \t}\n \tfree(one_buf);\n \tfree(two_buf);\n-- \n1.8.2\n"},{"id":"212175","messageId":"7vzjxrzchj.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"514F8244.8070702@lsrfire.ath.cx","subject":"Re: [PATCH 8/4] match-trees: drop \"x = x\" initializations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T16:10:48Z","receivedAt":"2013-03-25T16:10:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 24.03.2013 05:55, schrieb Junio C Hamano:\n>> So I like your change for readability, but for GCC 4.4.5 we still\n>> need the unnecessary initialization.\n>\n> Hrm, perhaps we can make it even simpler for the compiler.\n\nAnd the result is even simpler for human readers, I'd have to say.\n\n> I'm a bit uneasy about this one because we lack proper tests for\n> this code and I don't know how to write ones off the bat.\n\nThis looks pretty much a straight-forward equivalent rewrite from\nyour earlier one, which was also an obvious equivalent to the\noriginal, at least to me. The first four lines in the original were\nmade into two tree_entry() calls (what a useful helper we haven't\nbeen using!) and that allows us to lose explicit update_tree_entry()\ncalls.\n\n\n\n> match-trees.c | 68 ++++++++++++++++++++++++-----------------------------------\n> 1 file changed, 28 insertions(+), 40 deletions(-)\n>\n> diff --git a/match-trees.c b/match-trees.c\n> index 26f7ed1..2bb734d 100644\n> --- a/match-trees.c\n> +++ b/match-trees.c\n> @@ -47,6 +47,13 @@ static int score_matches(unsigned mode1, unsigned mode2, const char *path)\n> \treturn score;\n> }\n> \n> +static int base_name_entries_compare(const struct name_entry *a,\n> +\t\t\t\t const struct name_entry *b)\n> +{\n> +\treturn base_name_compare(a->path, tree_entry_len(a), a->mode,\n> +\t\t\t\t b->path, tree_entry_len(b), b->mode);\n> +}\n> +\n> /*\n> * Inspect two trees, and give a score that tells how similar they are.\n> */\n> @@ -71,54 +78,35 @@ static int score_trees(const unsigned char *hash1, const unsigned char *hash2)\n> \tif (type != OBJ_TREE)\n> \t\tdie(\"%s is not a tree\", sha1_to_hex(hash2));\n> \tinit_tree_desc(&two, two_buf, size);\n> -\twhile (one.size | two.size) {\n> -\t\tconst unsigned char *elem1 = elem1;\n> -\t\tconst unsigned char *elem2 = elem2;\n> -\t\tconst char *path1 = path1;\n> -\t\tconst char *path2 = path2;\n> -\t\tunsigned mode1 = mode1;\n> -\t\tunsigned mode2 = mode2;\n> +\tfor (;;) {\n> +\t\tstruct name_entry e1, e2;\n> +\t\tint got_entry_from_one = tree_entry(&one, &e1);\n> +\t\tint got_entry_from_two = tree_entry(&two, &e2);\n> \t\tint cmp;\n> \n> -\t\tif (one.size)\n> -\t\t\telem1 = tree_entry_extract(&one, &path1, &mode1);\n> -\t\tif (two.size)\n> -\t\t\telem2 = tree_entry_extract(&two, &path2, &mode2);\n> -\n> -\t\tif (!one.size) {\n> -\t\t\t/* two has more entries */\n> -\t\t\tscore += score_missing(mode2, path2);\n> -\t\t\tupdate_tree_entry(&two);\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tif (!two.size) {\n> +\t\tif (got_entry_from_one && got_entry_from_two)\n> +\t\t\tcmp = base_name_entries_compare(&e1, &e2);\n> +\t\telse if (got_entry_from_one)\n> \t\t\t/* two lacks this entry */\n> -\t\t\tscore += score_missing(mode1, path1);\n> -\t\t\tupdate_tree_entry(&one);\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\tcmp = base_name_compare(path1, strlen(path1), mode1,\n> -\t\t\t\t\tpath2, strlen(path2), mode2);\n> -\t\tif (cmp < 0) {\n> +\t\t\tcmp = -1;\n> +\t\telse if (got_entry_from_two)\n> +\t\t\t/* two has more entries */\n> +\t\t\tcmp = 1;\n> +\t\telse\n> +\t\t\tbreak;\n> +\n> +\t\tif (cmp < 0)\n> \t\t\t/* path1 does not appear in two */\n> -\t\t\tscore += score_missing(mode1, path1);\n> -\t\t\tupdate_tree_entry(&one);\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\telse if (cmp > 0) {\n> +\t\t\tscore += score_missing(e1.mode, e1.path);\n> +\t\telse if (cmp > 0)\n> \t\t\t/* path2 does not appear in one */\n> -\t\t\tscore += score_missing(mode2, path2);\n> -\t\t\tupdate_tree_entry(&two);\n> -\t\t\tcontinue;\n> -\t\t}\n> -\t\telse if (hashcmp(elem1, elem2))\n> +\t\t\tscore += score_missing(e2.mode, e2.path);\n> +\t\telse if (hashcmp(e1.sha1, e2.sha1))\n> \t\t\t/* they are different */\n> -\t\t\tscore += score_differs(mode1, mode2, path1);\n> +\t\t\tscore += score_differs(e1.mode, e2.mode, e1.path);\n> \t\telse\n> \t\t\t/* same subtree or blob */\n> -\t\t\tscore += score_matches(mode1, mode2, path1);\n> -\t\tupdate_tree_entry(&one);\n> -\t\tupdate_tree_entry(&two);\n> +\t\t\tscore += score_matches(e1.mode, e2.mode, e1.path);\n> \t}\n> \tfree(one_buf);\n> \tfree(two_buf);\n"},{"id":"212203","messageId":"7vfvzjxnq9.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"20130324093212.GA28234@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] transport: drop \"int cmp = cmp\" hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T19:50:54Z","receivedAt":"2013-03-25T19:50:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Mar 23, 2013 at 09:00:05PM -0700, Junio C Hamano wrote:\n>\n>> On Thu, Mar 21, 2013 at 4:13 AM, Jeff King wrote:\n>> >\n>> > According to 47ec794, this initialization is meant to\n>> > squelch an erroneous uninitialized variable warning from gcc\n>> > 4.0.1. That version is quite old at this point, and gcc 4.1\n>> > and up handle it fine, with one exception. There seems to be\n>> > a regression in gcc 4.6.3, which produces the warning;\n>> > however, gcc versions 4.4.7 and 4.7.2 do not.\n>> >\n>> \n>> transport.c: In function 'get_refs_via_rsync':\n>> transport.c:127:29: error: 'cmp' may be used uninitialized in this\n>> function [-Werror=uninitialized]\n>> transport.c:109:7: note: 'cmp' was declared here\n>> \n>> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3\n>\n> Right, that's the same version I noted above. Is 4.6.3 the default\n> compiler under a particular release of Ubuntu, or did you use their\n> gcc-4.6 package?\n\nI'll check later with one of my VMs. The copy of U 12.04 I happened\nto have handy has that version installed.\n\nBy the way, I find this piece of code less than pleasant:\n\n * It uses \"struct ref dummy = { NULL }, *tail = &dummy\", and then\n accumulates things by appending to \"&tail\" and then returns\n dummy.next. Why doesn't it do\n\n\tstruct ref *retval = NULL, **tail = &retval;\n\n and pass tail around to append things, like everybody else? Is\n this another instance of \"People do not understand linked list\"\n problem? Perhaps fixing that may unconfuse the compiler?\n\n * Its read_loose_refs() is a recursive function that sorts the\n results from readdir(3) and iterates over them, expecting its\n recursive call to fail _only_ when the entry it read is not a\n directory that it needs to recurse into.\n\n It is not obvious if the resulting list is sorted correctly with\n this loop structure when you have branches \"foo.bar\", \"foo/bar\",\n and \"foo=bar\". I think the loop first reads \"foo\", \"foo.bar\" and\n \"foo=bar\", sorts them in that order, and starts reading\n recursively, ending up with \"foo/bar\" first and then \"foo.bar\"\n and finally \"foo=bar\". Later, the tail of the same list is\n passed to insert_packed_refs(), which does in-place merging of\n this list and the contents of the packed_refs file. These two\n data sources have to be sorted the same way for this merge to\n work correctly, but there is no validating the order of the\n entries it reads from the packed-refs file. At least, it should\n barf when the file is not sorted. It could be lenient and accept\n a mal-sorted input, but I do not think that is advisable.\n\nI'll apply the attached on 'maint' for now, as rsync is not worth\nspending too many cycles on worrying about; I need to go to the\nbathroom to wash my eyes after staring this code for 20 minutes X-<.\n\n transport.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/transport.c b/transport.c\nindex 87b8f14..e6f9346 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -106,7 +106,8 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)\n \t\treturn;\n \n \tfor (;;) {\n-\t\tint cmp, len;\n+\t\tint cmp = 0; /* assigned before used */\n+\t\tint len;\n \n \t\tif (!fgets(buffer, sizeof(buffer), f)) {\n \t\t\tfclose(f);\n"},{"id":"212223","messageId":"20130325210625.GA16386@sigill.intra.peff.net","threadId":"33244","inReplyTo":"7vfvzjxnq9.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 4/4] transport: drop \"int cmp = cmp\" hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-03-25T21:06:25Z","receivedAt":"2013-03-25T21:06:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 25, 2013 at 12:50:54PM -0700, Junio C Hamano wrote:\n\n> >> transport.c: In function 'get_refs_via_rsync':\n> >> transport.c:127:29: error: 'cmp' may be used uninitialized in this\n> >> function [-Werror=uninitialized]\n> >> transport.c:109:7: note: 'cmp' was declared here\n> >> \n> >> gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3\n> >\n> > Right, that's the same version I noted above. Is 4.6.3 the default\n> > compiler under a particular release of Ubuntu, or did you use their\n> > gcc-4.6 package?\n> \n> I'll check later with one of my VMs. The copy of U 12.04 I happened\n> to have handy has that version installed.\n\nAh, if you didn't explicitly run \"gcc-4.6\", then it was probably the\ndefault version in 12.04 (as it was for a while in Debian testing, but\nthey never actually made a release with it, so everybody is now on 4.7\nby default).\n\n> By the way, I find this piece of code less than pleasant:\n> \n> * It uses \"struct ref dummy = { NULL }, *tail = &dummy\", and then\n> accumulates things by appending to \"&tail\" and then returns\n> dummy.next. Why doesn't it do\n> \n> \tstruct ref *retval = NULL, **tail = &retval;\n> \n> and pass tail around to append things, like everybody else? Is\n> this another instance of \"People do not understand linked list\"\n> problem? Perhaps fixing that may unconfuse the compiler?\n\nUgh, that is horrible. At first I thought it was even wrong, as we pass\n&tail and not &dummy.next to read_loose_refs. But two wrongs _do_ make a\nright, because read_loose_refs, rather than do:\n\n *tail = new;\n tail = &new->next;\n\ndoes:\n\n (*tail)->next = new;\n *tail = new;\n\n> Later, the tail of the same list is passed to insert_packed_refs(),\n> which does in-place merging of this list and the contents of the\n> packed_refs file. These two data sources have to be sorted the\n> same way for this merge to work correctly, but there is no\n> validating the order of the entries it reads from the packed-refs\n> file. At least, it should barf when the file is not sorted. It\n> could be lenient and accept a mal-sorted input, but I do not think\n> that is advisable.\n\nActually, it is the head of the loose list (though it is hard to\nrealize, because it is called tail!).\n\n> I'll apply the attached on 'maint' for now, as rsync is not worth\n> spending too many cycles on worrying about; I need to go to the\n> bathroom to wash my eyes after staring this code for 20 minutes X-<.\n\nYeah, it's quite ugly. I really wonder if it is time to drop rsync\nsupport. I'd be really surprised if anybody is actively using it.\n\nI wonder, though, what made you look at this. It did not come up in my\nlist of -Wuninitialized warnings. Did it get triggered by one of the\nother gcc versions?\n\n> diff --git a/transport.c b/transport.c\n> index 87b8f14..e6f9346 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -106,7 +106,8 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)\n> \t\treturn;\n> \n> \tfor (;;) {\n> -\t\tint cmp, len;\n> +\t\tint cmp = 0; /* assigned before used */\n> +\t\tint len;\n> \n> \t\tif (!fgets(buffer, sizeof(buffer), f)) {\n> \t\t\tfclose(f);\n\nI think that's fine.\n\n-Peff\n"},{"id":"212236","messageId":"7vk3ovw3ej.fsf@alter.siamese.dyndns.org","threadId":"33244","inReplyTo":"20130325210625.GA16386@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] transport: drop \"int cmp = cmp\" hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-03-25T21:55:16Z","receivedAt":"2013-03-25T21:55:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wonder, though, what made you look at this. It did not come up in my\n> list of -Wuninitialized warnings. Did it get triggered by one of the\n> other gcc versions?\n\nNo, but the function in question has that questionable construct\nwritten by somebody who does not understand linked list, and it\ndusgusted me enough to look at where that list came from, which\ninevitably made me notice that \"return dummy.next\" that made me go\n\"wat?\"\n\n>\n>> diff --git a/transport.c b/transport.c\n>> index 87b8f14..e6f9346 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -106,7 +106,8 @@ static void insert_packed_refs(const char *packed_refs, struct ref **list)\n>> \t\treturn;\n>> \n>> \tfor (;;) {\n>> -\t\tint cmp, len;\n>> +\t\tint cmp = 0; /* assigned before used */\n>> +\t\tint len;\n>> \n>> \t\tif (!fgets(buffer, sizeof(buffer), f)) {\n>> \t\t\tfclose(f);\n>\n> I think that's fine.\n>\n> -Peff\n"}]}