{"thread":{"id":"24431","subject":"remove duplicate code and not needed break statement","startedAt":"2010-07-18T17:49:59Z","lastAt":"2010-07-19T16:26:12Z","messageCount":9,"participants":["Ralf Thielow","Valeo de Vries","Jonathan Nieder","Erik Faye-Lund"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"145763","messageId":"1279475399-6081-1-git-send-email-ralf.thielow@googlemail.com","threadId":"24431","inReplyTo":null,"subject":"remove duplicate code and not needed break statement","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@googlemail.com","sentAt":"2010-07-18T17:49:59Z","receivedAt":"2010-07-18T17:49:59Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"Remove duplicate code and not needed break statement.\n\nOn switch statements you can use multiple cases for one\nstatement and on a goto statement you not need a break.\n\n>From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001\nFrom: Ralf Thielow \nDate: Sun, 18 Jul 2010 18:48:58 +0200\nSubject: [PATCH] remove duplicate code and not needed break statement\n\n---\n server-info.c | 3 ---\n 1 files changed, 0 insertions(+), 3 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 4098ca2..9ec744e 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)\n \t\t\t\tgoto out_stale;\n \t\t\tbreak;\n \t\tcase 'D': /* we used to emit D but that was misguided. */\n-\t\t\tgoto out_stale;\n-\t\t\tbreak;\n \t\tcase 'T': /* we used to emit T but nobody uses it. */\n \t\t\tgoto out_stale;\n-\t\t\tbreak;\n \t\tdefault:\n \t\t\terror(\"unrecognized: %s\", line);\n \t\t\tbreak;\n-- \n"},{"id":"145764","messageId":"AANLkTilhNLWEF-40hE9tWdJQX6FXpYWXCu7yhAC11Zvq@mail.gmail.com","threadId":"24431","inReplyTo":"1279475399-6081-1-git-send-email-ralf.thielow@googlemail.com","subject":"Re: remove duplicate code and not needed break statement","fromName":"Valeo de Vries","fromEmail":"valeo@valeo.co.cc","sentAt":"2010-07-18T17:57:12Z","receivedAt":"2010-07-18T17:57:12Z","isPatch":false,"sender":{"key":"valeo@valeo.co.cc","avatar":null},"body":"On 18 July 2010 18:49, Ralf Thielow wrote:\n> Remove duplicate code and not needed break statement.\n>\n> On switch statements you can use multiple cases for one\n> statement and on a goto statement you not need a break.\n>\n> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001\n> From: Ralf Thielow \n> Date: Sun, 18 Jul 2010 18:48:58 +0200\n> Subject: [PATCH] remove duplicate code and not needed break statement\n>\n> ---\n>  server-info.c |    3 ---\n>  1 files changed, 0 insertions(+), 3 deletions(-)\n>\n> diff --git a/server-info.c b/server-info.c\n> index 4098ca2..9ec744e 100644\n> --- a/server-info.c\n> +++ b/server-info.c\n> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)\n>                                goto out_stale;\n>                        break;\n\nYou seem to have missed another, or am I just lacking context? ;)\n\nValeo\n"},{"id":"145769","messageId":"AANLkTimXOhibJyVu7Z8Obye8ph-xjAbyqQ4rCiodYvpx@mail.gmail.com","threadId":"24431","inReplyTo":"AANLkTimuIBe2mvCH15D0RCzlgKTxhHEoX8KEsqiTJgO0@mail.gmail.com","subject":"Re: remove duplicate code and not needed break statement","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@googlemail.com","sentAt":"2010-07-18T18:19:55Z","receivedAt":"2010-07-18T18:19:55Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"There were 2 cases on that switch statement which do the same thing.\nin case 'P' there is an if-statment before.\n\nRalf\n> 2010/7/18 Valeo de Vries :\n>> On 18 July 2010 18:49, Ralf Thielow wrote:\n>>> Remove duplicate code and not needed break statement.\n>>>\n>>> On switch statements you can use multiple cases for one\n>>> statement and on a goto statement you not need a break.\n>>>\n>>> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001\n>>> From: Ralf Thielow \n>>> Date: Sun, 18 Jul 2010 18:48:58 +0200\n>>> Subject: [PATCH] remove duplicate code and not needed break statement\n>>>\n>>> ---\n>>>  server-info.c |    3 ---\n>>>  1 files changed, 0 insertions(+), 3 deletions(-)\n>>>\n>>> diff --git a/server-info.c b/server-info.c\n>>> index 4098ca2..9ec744e 100644\n>>> --- a/server-info.c\n>>> +++ b/server-info.c\n>>> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)\n>>>                                goto out_stale;\n>>>                        break;\n>>\n>> You seem to have missed another, or am I just lacking context? ;)\n>>\n>> Valeo\n>>\n>\n"},{"id":"145776","messageId":"AANLkTinrg2cp0XL0HOApGN25kmuL7xdik5aEo3lfRqz0@mail.gmail.com","threadId":"24431","inReplyTo":"AANLkTilhNLWEF-40hE9tWdJQX6FXpYWXCu7yhAC11Zvq@mail.gmail.com","subject":"Re: remove duplicate code and not needed break statement","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@googlemail.com","sentAt":"2010-07-18T18:29:06Z","receivedAt":"2010-07-18T18:29:06Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"2010/7/18 Valeo de Vries :\n> On 18 July 2010 18:49, Ralf Thielow wrote:\n>> Remove duplicate code and not needed break statement.\n>>\n>> On switch statements you can use multiple cases for one\n>> statement and on a goto statement you not need a break.\n>>\n>> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001\n>> From: Ralf Thielow \n>> Date: Sun, 18 Jul 2010 18:48:58 +0200\n>> Subject: [PATCH] remove duplicate code and not needed break statement\n>>\n>> ---\n>>  server-info.c |    3 ---\n>>  1 files changed, 0 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/server-info.c b/server-info.c\n>> index 4098ca2..9ec744e 100644\n>> --- a/server-info.c\n>> +++ b/server-info.c\n>> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)\n>>                                goto out_stale;\n>>                        break;\n>\n> You seem to have missed another, or am I just lacking context? ;)\n>\n> Valeo\n>\n\nThere were 2 cases on that switch statement which do the same thing.\nin case 'P' there is an if-statment before.\n\nRalf\n"},{"id":"145777","messageId":"20100718184716.GD14367@burratino","threadId":"24431","inReplyTo":"1279475399-6081-1-git-send-email-ralf.thielow@googlemail.com","subject":"Re: remove duplicate code and not needed break statement","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-18T18:47:16Z","receivedAt":"2010-07-18T18:47:16Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Ralf!\n\nRalf Thielow wrote:\n\n> Remove duplicate code and not needed break statement.\n\nThis looks like a reasonable change, with the nice effect of getting\nread_pack_info_file() to almost fit in one screen.\n\nThe commit message is not super clear. I would say:\n\n Subject: update-server-info: Shorten read_pack_info_file()\n\n The correct responses to a D and a T line in .git/objects/info/packs\n are the same, so combine their case arms. In both cases we already\n ‘goto’ out of the switch so while at it, remove a redundant ‘break’\n to avoid yet another line of code.\n\ni.e., impact in the subject line, details in the commit message body.\n\nIt is missing a sign-off. See Documentation/SubmittingPatches for\nwhat that means.\n\nHope that helps,\nJonathan\n"},{"id":"145778","messageId":"AANLkTilU1T5ppfVZ5UXpsRXet0tcffjJ2Bi3q67wnF6L@mail.gmail.com","threadId":"24431","inReplyTo":"20100718184716.GD14367@burratino","subject":"Re: remove duplicate code and not needed break statement","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@googlemail.com","sentAt":"2010-07-18T18:56:33Z","receivedAt":"2010-07-18T18:56:33Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"2010/7/18 Jonathan Nieder :\n> Hi Ralf!\n>\n> Ralf Thielow wrote:\n>\n>> Remove duplicate code and not needed break statement.\n>\n> This looks like a reasonable change, with the nice effect of getting\n> read_pack_info_file() to almost fit in one screen.\n>\n> The commit message is not super clear.  I would say:\n>\n>  Subject: update-server-info: Shorten read_pack_info_file()\n>\n>  The correct responses to a D and a T line in .git/objects/info/packs\n>  are the same, so combine their case arms.  In both cases we already\n>  ‘goto’ out of the switch so while at it, remove a redundant ‘break’\n>  to avoid yet another line of code.\n>\n> i.e., impact in the subject line, details in the commit message body.\n>\n> It is missing a sign-off.  See Documentation/SubmittingPatches for\n> what that means.\n>\n> Hope that helps,\n> Jonathan\n>\n\nThank you,\nthis is my first patch on a mailing list using git, you see.\nIn future I do it better and read the whole documentation\nbefore. :)\n\nRalf\n"},{"id":"145790","messageId":"AANLkTimHHJnvgFh3Kd7jMqTJJFensZjkD7YCU1gdt-FT@mail.gmail.com","threadId":"24431","inReplyTo":"1279475399-6081-1-git-send-email-ralf.thielow@googlemail.com","subject":"Re: remove duplicate code and not needed break statement","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@googlemail.com","sentAt":"2010-07-19T15:43:34Z","receivedAt":"2010-07-19T15:43:34Z","isPatch":false,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Sun, Jul 18, 2010 at 7:49 PM, Ralf Thielow\n wrote:\n> Remove duplicate code and not needed break statement.\n>\n> On switch statements you can use multiple cases for one\n> statement and on a goto statement you not need a break.\n>\n> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001\n> From: Ralf Thielow \n> Date: Sun, 18 Jul 2010 18:48:58 +0200\n> Subject: [PATCH] remove duplicate code and not needed break statement\n>\n> ---\n>  server-info.c |    3 ---\n>  1 files changed, 0 insertions(+), 3 deletions(-)\n>\n> diff --git a/server-info.c b/server-info.c\n> index 4098ca2..9ec744e 100644\n> --- a/server-info.c\n> +++ b/server-info.c\n> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)\n>                                goto out_stale;\n>                        break;\n>                case 'D': /* we used to emit D but that was misguided. */\n> -                       goto out_stale;\n> -                       break;\n>                case 'T': /* we used to emit T but nobody uses it. */\n\nWe have a tendency of adding a comment pointing out fall through\nbetween case-statements. Perhaps you should add one? 'git grep -i\nfall.\\\\?through' shows you some of the places we've got it now...\n\n-- \nErik \"kusma\" Faye-Lund\n"},{"id":"145792","messageId":"20100719160632.GA17526@burratino","threadId":"24431","inReplyTo":"AANLkTimHHJnvgFh3Kd7jMqTJJFensZjkD7YCU1gdt-FT@mail.gmail.com","subject":"Re: remove duplicate code and not needed break statement","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-07-19T16:06:32Z","receivedAt":"2010-07-19T16:06:32Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Erik,\n\nErik Faye-Lund wrote:\n> On Sun, Jul 18, 2010 at 7:49 PM, Ralf Thielow\n\n>>                case 'D': /* we used to emit D but that was misguided. */\n>> -                       goto out_stale;\n>> -                       break;\n>>                case 'T': /* we used to emit T but nobody uses it. */\n>\n> We have a tendency of adding a comment pointing out fall through\n> between case-statements. Perhaps you should add one?\n\nI think in this case that would not be appropriate. It is the\ndifference between:\n\n\tcase 1:\n\tcase 2:\n\tcase 3:\n\t\tputs(\"one to three\");\n\t\tbreak;\n\nand\n\n\tcase 1:\n\t\tputs(\"one\");\n\t\t/* fall through */\n\tcase 2:\n\tcase 3:\n\t\tputs(\"one to three\");\n\t\tbreak;\n\nI admit, I am surprised to see multiple suggestions for improvements\nto details of the patch. I guess I should say, except for the log\nmessage, it is\n\n Reviewed-by: Jonathan Nieder \n\ni.e., I don’t see any low-hanging fruit for improving it without\nactually taking a deeper look at the surrounding code. Ralf, can we\nhave your sign-off? That way maybe it could be applied and people\ncould suggest patches on top.\n\nStill, thanks for looking it over.\n\nJonathan\n"},{"id":"145796","messageId":"AANLkTilB1eNHZWHLoBsVmEOObdSdCZQ0fxzqWc-SkC_a@mail.gmail.com","threadId":"24431","inReplyTo":"20100719160632.GA17526@burratino","subject":"Re: remove duplicate code and not needed break statement","fromName":"Ralf Thielow","fromEmail":"ralf.thielow@googlemail.com","sentAt":"2010-07-19T16:26:12Z","receivedAt":"2010-07-19T16:26:12Z","isPatch":false,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001\nFrom: Ralf Thielow \nDate: Sun, 18 Jul 2010 18:48:58 +0200\nSubject: [PATCH] update-server-info: Shorten read_pack_info_file()\n\nThe correct responses to a D and a T line in .git/objects/info/packs\nare the same, so combine their case arms. In both cases we already\n‘goto’ out of the switch so while at it, remove a redundant ‘break’\nto avoid yet another line of code.\n\n\nSigned-off-by: Ralf Thielow \nReviewed-by: Jonathan Nieder gmail.com>\n---\n server-info.c | 3 ---\n 1 files changed, 0 insertions(+), 3 deletions(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 4098ca2..9ec744e 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile)\n \t\t\t\tgoto out_stale;\n \t\t\tbreak;\n \t\tcase 'D': /* we used to emit D but that was misguided. */\n-\t\t\tgoto out_stale;\n-\t\t\tbreak;\n \t\tcase 'T': /* we used to emit T but nobody uses it. */\n \t\t\tgoto out_stale;\n-\t\t\tbreak;\n \t\tdefault:\n \t\t\terror(\"unrecognized: %s\", line);\n \t\t\tbreak;\n-- \n1.7.0.4\n"}]}