[PATCH] Simplify the code and avoid an attribution.

[PATCH] Simplify the code and avoid an attribution.

[André Goddard Rosa]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

Hi, all!

    Please cc: me as I'm not subscribed. I'm sending the patch inline
only for review, probably it is mangled.
    Please use the attached patch if you agree with it. Sorry about
sending it attached.

>From cd0cc6995684e2801011910735146052e5b59ccc Mon Sep 17 00:00:00 2001
From: Andre Goddard Rosa <andre.goddard@gmail.com>
Date: Tue, 27 Nov 2007 10:22:46 -0200
Subject: [PATCH] Simplify the code and avoid an attribution.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
---
 config.c          |   19 ++++++++++---------
 mailmap.c         |    2 +-
 xdiff-interface.c |    2 +-
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 56e99fc..7c9fcdd 100644
--- a/config.c
+++ b/config.c
@@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const
char *filename)
 	int ret;
 	FILE *f = fopen(filename, "r");

-	ret = -1;
-	if (f) {
-		config_file = f;
-		config_file_name = filename;
-		config_linenr = 1;
-		ret = git_parse_file(fn);
-		fclose(f);
-		config_file_name = NULL;
-	}
+	if (!f)
+		return -1;
+
+	config_file = f;
+	config_file_name = filename;
+	config_linenr = 1;
+	ret = git_parse_file(fn);
+	fclose(f);
+	config_file_name = NULL;
+
 	return ret;
 }

diff --git a/mailmap.c b/mailmap.c
index 8714167..0c13ecd 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -7,7 +7,7 @@ int read_mailmap(struct path_list *map, const char
*filename, char **repo_abbrev
 	char buffer[1024];
 	FILE *f = fopen(filename, "r");

-	if (f == NULL)
+	if (!f)
 		return 1;
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
 		char *end_of_name, *left_bracket, *right_bracket;
diff --git a/xdiff-interface.c b/xdiff-interface.c
index be866d1..9dd1f3b 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -111,7 +111,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)

 	if (stat(filename, &st))
 		return error("Could not stat %s", filename);
-	if ((f = fopen(filename, "rb")) == NULL)
+	if (!(f = fopen(filename, "rb")))
 		return error("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
 	ptr->ptr = xmalloc(sz);
-- 
1.5.3.6.861.gd794-dirty

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0004-Simplify-the-code-and-avoid-an-attribution.patch --]
[-- Type: text/x-patch; name=0004-Simplify-the-code-and-avoid-an-attribution.patch, Size: 1912 bytes --]

From cd0cc6995684e2801011910735146052e5b59ccc Mon Sep 17 00:00:00 2001
From: Andre Goddard Rosa <andre.goddard@gmail.com>
Date: Tue, 27 Nov 2007 10:22:46 -0200
Subject: [PATCH] Simplify the code and avoid an attribution.

Signed-off-by: Andre Goddard Rosa <andre.goddard@gmail.com>
---
 config.c          |   19 ++++++++++---------
 mailmap.c         |    2 +-
 xdiff-interface.c |    2 +-
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/config.c b/config.c
index 56e99fc..7c9fcdd 100644
--- a/config.c
+++ b/config.c
@@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const char *filename)
 	int ret;
 	FILE *f = fopen(filename, "r");
 
-	ret = -1;
-	if (f) {
-		config_file = f;
-		config_file_name = filename;
-		config_linenr = 1;
-		ret = git_parse_file(fn);
-		fclose(f);
-		config_file_name = NULL;
-	}
+	if (!f)
+		return -1;
+
+	config_file = f;
+	config_file_name = filename;
+	config_linenr = 1;
+	ret = git_parse_file(fn);
+	fclose(f);
+	config_file_name = NULL;
+
 	return ret;
 }
 
diff --git a/mailmap.c b/mailmap.c
index 8714167..0c13ecd 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -7,7 +7,7 @@ int read_mailmap(struct path_list *map, const char *filename, char **repo_abbrev
 	char buffer[1024];
 	FILE *f = fopen(filename, "r");
 
-	if (f == NULL)
+	if (!f)
 		return 1;
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
 		char *end_of_name, *left_bracket, *right_bracket;
diff --git a/xdiff-interface.c b/xdiff-interface.c
index be866d1..9dd1f3b 100644
--- a/xdiff-interface.c
+++ b/xdiff-interface.c
@@ -111,7 +111,7 @@ int read_mmfile(mmfile_t *ptr, const char *filename)
 
 	if (stat(filename, &st))
 		return error("Could not stat %s", filename);
-	if ((f = fopen(filename, "rb")) == NULL)
+	if (!(f = fopen(filename, "rb")))
 		return error("Could not open %s", filename);
 	sz = xsize_t(st.st_size);
 	ptr->ptr = xmalloc(sz);
-- 
1.5.3.6.861.gd794-dirty

Re: [PATCH] Simplify the code and avoid an attribution.

[Mike Hommey]

On Wed, Nov 21, 2007 at 11:00:02PM -0200, André Goddard Rosa wrote:
> --- a/config.c
> +++ b/config.c
> @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const
> char *filename)
>  	int ret;
>  	FILE *f = fopen(filename, "r");
> 
> -	ret = -1;
> -	if (f) {
> -		config_file = f;
> -		config_file_name = filename;
> -		config_linenr = 1;
> -		ret = git_parse_file(fn);
> -		fclose(f);
> -		config_file_name = NULL;
> -	}
> +	if (!f)
> +		return -1;
> +
> +	config_file = f;
> +	config_file_name = filename;
> +	config_linenr = 1;
> +	ret = git_parse_file(fn);
> +	fclose(f);
> +	config_file_name = NULL;
> +
>  	return ret;
>  }

Actually, since it is more likely that the file has been opened, the
original code is more optimal because it doesn't generate a jump in most
cases. And if you're worried about the ret variable, don't worry, it's
most likely stripped out by the compiler optimizations.

> -	if (f == NULL)
> +	if (!f)

> -	if ((f = fopen(filename, "rb")) == NULL)
> +	if (!(f = fopen(filename, "rb")))

It's a matter of taste

Mike

Re: [PATCH] Simplify the code and avoid an attribution.

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> On Wed, Nov 21, 2007 at 11:00:02PM -0200, André Goddard Rosa wrote:
>> --- a/config.c
>> +++ b/config.c
>> @@ -447,15 +447,16 @@ int git_config_from_file(config_fn_t fn, const
>> char *filename)
>>  	int ret;
>>  	FILE *f = fopen(filename, "r");
>> 
>> -	ret = -1;
>> -	if (f) {
>> -		config_file = f;
>> -		config_file_name = filename;
>> -		config_linenr = 1;
>> -		ret = git_parse_file(fn);
>> -		fclose(f);
>> -		config_file_name = NULL;
>> -	}
>> +	if (!f)
>> +		return -1;
>> +
>> +	config_file = f;
>> +	config_file_name = filename;
>> +	config_linenr = 1;
>> +	ret = git_parse_file(fn);
>> +	fclose(f);
>> +	config_file_name = NULL;
>> +
>>  	return ret;
>>  }
>
> Actually, since it is more likely that the file has been opened, the
> original code is more optimal because it doesn't generate a jump in most
> cases. And if you're worried about the ret variable, don't worry, it's
> most likely stripped out by the compiler optimizations.

I did not comment on this patch partly because I did not know
what "attribution" meant.  I think it is a good change from
readability, not micro-optimization, point of view.

If you structure your code this way,

	do this
        if (there is an error)
        	return error code;
	do that
	do the rest

it is much easier to read than

	do this
        set error code pessimistically
        if (it was successful) {
        	do that
                update error code
                do the rest
	}
        return error code

especially the more likely part that runs the real processing
("do that...do the rest") tends to grow over time, and even
acquire other error checks.