remove duplicate code and not needed break statement

remove duplicate code and not needed break statement

[Ralf Thielow]

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 <ralf.thielow@googlemail.com>
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;
-- 

Re: remove duplicate code and not needed break statement

[Valeo de Vries]

On 18 July 2010 18:49, Ralf Thielow <ralf.thielow@googlemail.com> 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 <ralf.thielow@googlemail.com>
> 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

Re: remove duplicate code and not needed break statement

[Ralf Thielow]

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 <valeo@valeo.co.cc>:
>> On 18 July 2010 18:49, Ralf Thielow <ralf.thielow@googlemail.com> 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 <ralf.thielow@googlemail.com>
>>> 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
>>
>

Re: remove duplicate code and not needed break statement

[Ralf Thielow]

2010/7/18 Valeo de Vries <valeo@valeo.co.cc>:
> On 18 July 2010 18:49, Ralf Thielow <ralf.thielow@googlemail.com> 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 <ralf.thielow@googlemail.com>
>> 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

Re: remove duplicate code and not needed break statement

[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

Re: remove duplicate code and not needed break statement

[Ralf Thielow]

2010/7/18 Jonathan Nieder <jrnieder@gmail.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
>

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

Re: remove duplicate code and not needed break statement

[Erik Faye-Lund]

On Sun, Jul 18, 2010 at 7:49 PM, Ralf Thielow
<ralf.thielow@googlemail.com> 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 <ralf.thielow@googlemail.com>
> 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

Re: remove duplicate code and not needed break statement

[Jonathan Nieder]

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 <jrnieder@gmail.com>

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

Re: remove duplicate code and not needed break statement

[Ralf Thielow]

From 5b3e7c8f8b81a295b5c58534be250f5a818ccc64 Mon Sep 17 00:00:00 2001
From: Ralf Thielow <ralf.thielow@googlemail.com>
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 <ralf.thielow@googlemail.com>
Reviewed-by: Jonathan Nieder <jrnieder <at> 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