# remove duplicate code and not needed break statement 9 messages from 2010-07-18 to 2010-07-19. Participants: Ralf Thielow, Valeo de Vries, Jonathan Nieder, Erik Faye-Lund. Thread: https://gitlist.dev/t/24431 ## Ralf Thielow, 2010-07-18 17:49 Subject: remove duplicate code and not needed break statement Message-ID: <1279475399-6081-1-git-send-email-ralf.thielow@googlemail.com> URL: https://gitlist.dev/e/1279475399-6081-1-git-send-email-ralf.thielow%40googlemail.com ``` Remove duplicate code and not needed break statement. On switch statements you can use multiple cases for one statement and on a goto statement you not need a break. >From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001 From: Ralf Thielow Date: Sun, 18 Jul 2010 18:48:58 +0200 Subject: [PATCH] remove duplicate code and not needed break statement --- server-info.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-) diff --git a/server-info.c b/server-info.c index 4098ca2..9ec744e 100644 --- a/server-info.c +++ b/server-info.c @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile) goto out_stale; break; case 'D': /* we used to emit D but that was misguided. */ - goto out_stale; - break; case 'T': /* we used to emit T but nobody uses it. */ goto out_stale; - break; default: error("unrecognized: %s", line); break; -- ``` ## Valeo de Vries, 2010-07-18 17:57 Subject: Re: remove duplicate code and not needed break statement Message-ID: URL: https://gitlist.dev/e/AANLkTilhNLWEF-40hE9tWdJQX6FXpYWXCu7yhAC11Zvq%40mail.gmail.com In-Reply-To: <1279475399-6081-1-git-send-email-ralf.thielow@googlemail.com> ``` On 18 July 2010 18:49, Ralf Thielow wrote: > Remove duplicate code and not needed break statement. > > On switch statements you can use multiple cases for one > statement and on a goto statement you not need a break. > > From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001 > From: Ralf Thielow > Date: Sun, 18 Jul 2010 18:48:58 +0200 > Subject: [PATCH] remove duplicate code and not needed break statement > > --- >  server-info.c |    3 --- >  1 files changed, 0 insertions(+), 3 deletions(-) > > diff --git a/server-info.c b/server-info.c > index 4098ca2..9ec744e 100644 > --- a/server-info.c > +++ b/server-info.c > @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile) >                                goto out_stale; >                        break; You seem to have missed another, or am I just lacking context? ;) Valeo ``` ## Ralf Thielow, 2010-07-18 18:19 Subject: Re: remove duplicate code and not needed break statement Message-ID: URL: https://gitlist.dev/e/AANLkTimXOhibJyVu7Z8Obye8ph-xjAbyqQ4rCiodYvpx%40mail.gmail.com In-Reply-To: ``` There were 2 cases on that switch statement which do the same thing. in case 'P' there is an if-statment before. Ralf > 2010/7/18 Valeo de Vries : >> On 18 July 2010 18:49, Ralf Thielow wrote: >>> Remove duplicate code and not needed break statement. >>> >>> On switch statements you can use multiple cases for one >>> statement and on a goto statement you not need a break. >>> >>> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001 >>> From: Ralf Thielow >>> Date: Sun, 18 Jul 2010 18:48:58 +0200 >>> Subject: [PATCH] remove duplicate code and not needed break statement >>> >>> --- >>>  server-info.c |    3 --- >>>  1 files changed, 0 insertions(+), 3 deletions(-) >>> >>> diff --git a/server-info.c b/server-info.c >>> index 4098ca2..9ec744e 100644 >>> --- a/server-info.c >>> +++ b/server-info.c >>> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile) >>>                                goto out_stale; >>>                        break; >> >> You seem to have missed another, or am I just lacking context? ;) >> >> Valeo >> > ``` ## Ralf Thielow, 2010-07-18 18:29 Subject: Re: remove duplicate code and not needed break statement Message-ID: URL: https://gitlist.dev/e/AANLkTinrg2cp0XL0HOApGN25kmuL7xdik5aEo3lfRqz0%40mail.gmail.com In-Reply-To: ``` 2010/7/18 Valeo de Vries : > On 18 July 2010 18:49, Ralf Thielow wrote: >> Remove duplicate code and not needed break statement. >> >> On switch statements you can use multiple cases for one >> statement and on a goto statement you not need a break. >> >> From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001 >> From: Ralf Thielow >> Date: Sun, 18 Jul 2010 18:48:58 +0200 >> Subject: [PATCH] remove duplicate code and not needed break statement >> >> --- >>  server-info.c |    3 --- >>  1 files changed, 0 insertions(+), 3 deletions(-) >> >> diff --git a/server-info.c b/server-info.c >> index 4098ca2..9ec744e 100644 >> --- a/server-info.c >> +++ b/server-info.c >> @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile) >>                                goto out_stale; >>                        break; > > You seem to have missed another, or am I just lacking context? ;) > > Valeo > There were 2 cases on that switch statement which do the same thing. in case 'P' there is an if-statment before. Ralf ``` ## Jonathan Nieder, 2010-07-18 18:47 Subject: Re: remove duplicate code and not needed break statement Message-ID: <20100718184716.GD14367@burratino> URL: https://gitlist.dev/e/20100718184716.GD14367%40burratino In-Reply-To: <1279475399-6081-1-git-send-email-ralf.thielow@googlemail.com> ``` Hi Ralf! Ralf Thielow wrote: > Remove duplicate code and not needed break statement. This looks like a reasonable change, with the nice effect of getting read_pack_info_file() to almost fit in one screen. The commit message is not super clear. I would say: Subject: update-server-info: Shorten read_pack_info_file() The correct responses to a D and a T line in .git/objects/info/packs are the same, so combine their case arms. In both cases we already ‘goto’ out of the switch so while at it, remove a redundant ‘break’ to avoid yet another line of code. i.e., impact in the subject line, details in the commit message body. It is missing a sign-off. See Documentation/SubmittingPatches for what that means. Hope that helps, Jonathan ``` ## Ralf Thielow, 2010-07-18 18:56 Subject: Re: remove duplicate code and not needed break statement Message-ID: URL: https://gitlist.dev/e/AANLkTilU1T5ppfVZ5UXpsRXet0tcffjJ2Bi3q67wnF6L%40mail.gmail.com In-Reply-To: <20100718184716.GD14367@burratino> ``` 2010/7/18 Jonathan Nieder : > Hi Ralf! > > Ralf Thielow wrote: > >> Remove duplicate code and not needed break statement. > > This looks like a reasonable change, with the nice effect of getting > read_pack_info_file() to almost fit in one screen. > > The commit message is not super clear.  I would say: > >  Subject: update-server-info: Shorten read_pack_info_file() > >  The correct responses to a D and a T line in .git/objects/info/packs >  are the same, so combine their case arms.  In both cases we already >  ‘goto’ out of the switch so while at it, remove a redundant ‘break’ >  to avoid yet another line of code. > > i.e., impact in the subject line, details in the commit message body. > > It is missing a sign-off.  See Documentation/SubmittingPatches for > what that means. > > Hope that helps, > Jonathan > Thank you, this is my first patch on a mailing list using git, you see. In future I do it better and read the whole documentation before. :) Ralf ``` ## Erik Faye-Lund, 2010-07-19 15:43 Subject: Re: remove duplicate code and not needed break statement Message-ID: URL: https://gitlist.dev/e/AANLkTimHHJnvgFh3Kd7jMqTJJFensZjkD7YCU1gdt-FT%40mail.gmail.com In-Reply-To: <1279475399-6081-1-git-send-email-ralf.thielow@googlemail.com> ``` On Sun, Jul 18, 2010 at 7:49 PM, Ralf Thielow wrote: > Remove duplicate code and not needed break statement. > > On switch statements you can use multiple cases for one > statement and on a goto statement you not need a break. > > From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001 > From: Ralf Thielow > Date: Sun, 18 Jul 2010 18:48:58 +0200 > Subject: [PATCH] remove duplicate code and not needed break statement > > --- >  server-info.c |    3 --- >  1 files changed, 0 insertions(+), 3 deletions(-) > > diff --git a/server-info.c b/server-info.c > index 4098ca2..9ec744e 100644 > --- a/server-info.c > +++ b/server-info.c > @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile) >                                goto out_stale; >                        break; >                case 'D': /* we used to emit D but that was misguided. */ > -                       goto out_stale; > -                       break; >                case 'T': /* we used to emit T but nobody uses it. */ We have a tendency of adding a comment pointing out fall through between case-statements. Perhaps you should add one? 'git grep -i fall.\\?through' shows you some of the places we've got it now... -- Erik "kusma" Faye-Lund ``` ## Jonathan Nieder, 2010-07-19 16:06 Subject: Re: remove duplicate code and not needed break statement Message-ID: <20100719160632.GA17526@burratino> URL: https://gitlist.dev/e/20100719160632.GA17526%40burratino In-Reply-To: ``` Hi Erik, Erik Faye-Lund wrote: > On Sun, Jul 18, 2010 at 7:49 PM, Ralf Thielow >>                case 'D': /* we used to emit D but that was misguided. */ >> -                       goto out_stale; >> -                       break; >>                case 'T': /* we used to emit T but nobody uses it. */ > > We have a tendency of adding a comment pointing out fall through > between case-statements. Perhaps you should add one? I think in this case that would not be appropriate. It is the difference between: case 1: case 2: case 3: puts("one to three"); break; and case 1: puts("one"); /* fall through */ case 2: case 3: puts("one to three"); break; I admit, I am surprised to see multiple suggestions for improvements to details of the patch. I guess I should say, except for the log message, it is Reviewed-by: Jonathan Nieder i.e., I don’t see any low-hanging fruit for improving it without actually taking a deeper look at the surrounding code. Ralf, can we have your sign-off? That way maybe it could be applied and people could suggest patches on top. Still, thanks for looking it over. Jonathan ``` ## Ralf Thielow, 2010-07-19 16:26 Subject: Re: remove duplicate code and not needed break statement Message-ID: URL: https://gitlist.dev/e/AANLkTilB1eNHZWHLoBsVmEOObdSdCZQ0fxzqWc-SkC_a%40mail.gmail.com In-Reply-To: <20100719160632.GA17526@burratino> ``` From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001 From: Ralf Thielow Date: Sun, 18 Jul 2010 18:48:58 +0200 Subject: [PATCH] update-server-info: Shorten read_pack_info_file() The correct responses to a D and a T line in .git/objects/info/packs are the same, so combine their case arms. In both cases we already ‘goto’ out of the switch so while at it, remove a redundant ‘break’ to avoid yet another line of code. Signed-off-by: Ralf Thielow Reviewed-by: Jonathan Nieder gmail.com> --- server-info.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-) diff --git a/server-info.c b/server-info.c index 4098ca2..9ec744e 100644 --- a/server-info.c +++ b/server-info.c @@ -113,11 +113,8 @@ static int read_pack_info_file(const char *infofile) goto out_stale; break; case 'D': /* we used to emit D but that was misguided. */ - goto out_stale; - break; case 'T': /* we used to emit T but nobody uses it. */ goto out_stale; - break; default: error("unrecognized: %s", line); break; -- 1.7.0.4 ```