Re: [PATCH] parse_object: check if buffer is non-NULL before freeing it

[Junio C Hamano <junkio@cox.net>]

Jonas Fonseca <fonseca@diku.dk> writes:

> Two code paths may cause this and other places in the source have the
> courtesy to do the check.

Eh, free(NULL) should work just fine.  It is "other places" that
is misguided and needs to be fixed.

- >8 -
[PATCH] free(NULL) is perfectly valid.

Jonas noticed some places say "if (X) free(X)" which is totally
unnecessary.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-apply.c         |    6 ++----
 builtin-fmt-merge-msg.c |    3 +--
 builtin-repo-config.c   |    6 ++----
 builtin-rev-list.c      |    6 ++----
 config.c                |    6 ++----
 fetch.c                 |    3 +--
 http-fetch.c            |   13 ++++---------
 http-push.c             |   13 ++++---------
 path-list.c             |    3 +--
 refs.c                  |    6 ++----
 10 files changed, 21 insertions(+), 44 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index b47ccac..1a1deaf 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1131,8 +1131,7 @@ static struct fragment *parse_binary_hun
 	return frag;
 
  corrupt:
-	if (data)
-		free(data);
+	free(data);
 	*status_p = -1;
 	error("corrupt binary patch at line %d: %.*s",
 	      linenr-1, llen-1, buffer);
@@ -1329,8 +1328,7 @@ static void show_stats(struct patch *pat
 		printf(" %s%-*s |%5d %.*s%.*s\n", prefix,
 		       len, name, patch->lines_added + patch->lines_deleted,
 		       add, pluses, del, minuses);
-	if (qname)
-		free(qname);
+	free(qname);
 }
 
 static int read_old_data(struct stat *st, const char *path, void *buf, unsigned long size)
diff --git a/builtin-fmt-merge-msg.c b/builtin-fmt-merge-msg.c
index a5ed8db..76d22b4 100644
--- a/builtin-fmt-merge-msg.c
+++ b/builtin-fmt-merge-msg.c
@@ -55,8 +55,7 @@ static void free_list(struct list *list)
 
 	for (i = 0; i < list->nr; i++) {
 		free(list->list[i]);
-		if (list->payload[i])
-			free(list->payload[i]);
+		free(list->payload[i]);
 	}
 	free(list->list);
 	free(list->payload);
diff --git a/builtin-repo-config.c b/builtin-repo-config.c
index c416480..6560cf1 100644
--- a/builtin-repo-config.c
+++ b/builtin-repo-config.c
@@ -122,10 +122,8 @@ static int get_value(const char* key_, c
 		ret =  (seen == 1) ? 0 : 1;
 
 free_strings:
-	if (repo_config)
-		free(repo_config);
-	if (global)
-		free(global);
+	free(repo_config);
+	free(global);
 	return ret;
 }
 
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index bc48a3e..7f3e1fc 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -93,10 +93,8 @@ static void show_commit(struct commit *c
 		free_commit_list(commit->parents);
 		commit->parents = NULL;
 	}
-	if (commit->buffer) {
-		free(commit->buffer);
-		commit->buffer = NULL;
-	}
+	free(commit->buffer);
+	commit->buffer = NULL;
 }
 
 static void process_blob(struct blob *blob,
diff --git a/config.c b/config.c
index 82b3562..d9f2b78 100644
--- a/config.c
+++ b/config.c
@@ -361,8 +361,7 @@ int git_config(config_fn_t fn)
 	}
 
 	ret += git_config_from_file(fn, filename);
-	if (repo_config)
-		free(repo_config);
+	free(repo_config);
 	return ret;
 }
 
@@ -734,8 +733,7 @@ int git_config_set_multivar(const char* 
 out_free:
 	if (0 <= fd)
 		close(fd);
-	if (config_filename)
-		free(config_filename);
+	free(config_filename);
 	if (lock_file) {
 		unlink(lock_file);
 		free(lock_file);
diff --git a/fetch.c b/fetch.c
index ef60b04..7d3812c 100644
--- a/fetch.c
+++ b/fetch.c
@@ -302,8 +302,7 @@ int pull(int targets, char **target, con
 		if (ret)
 			goto unlock_and_fail;
 	}
-	if (msg)
-		free(msg);
+	free(msg);
 
 	return 0;
 
diff --git a/http-fetch.c b/http-fetch.c
index 7619b33..6806f36 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -696,10 +696,8 @@ xml_start_tag(void *userData, const char
 	strcat(ctx->name, ".");
 	strcat(ctx->name, c);
 
-	if (ctx->cdata) {
-		free(ctx->cdata);
-		ctx->cdata = NULL;
-	}
+	free(ctx->cdata);
+	ctx->cdata = NULL;
 
 	ctx->userFunc(ctx, 0);
 }
@@ -726,8 +724,7 @@ static void
 xml_cdata(void *userData, const XML_Char *s, int len)
 {
 	struct xml_ctx *ctx = (struct xml_ctx *)userData;
-	if (ctx->cdata)
-		free(ctx->cdata);
+	free(ctx->cdata);
 	ctx->cdata = xmalloc(len + 1);
 	strlcpy(ctx->cdata, s, len + 1);
 }
@@ -765,9 +762,7 @@ static void handle_remote_ls_ctx(struct 
 			ls->dentry_flags |= IS_DIR;
 		}
 	} else if (!strcmp(ctx->name, DAV_PROPFIND_RESP)) {
-		if (ls->dentry_name) {
-			free(ls->dentry_name);
-		}
+		free(ls->dentry_name);
 		ls->dentry_name = NULL;
 		ls->dentry_flags = 0;
 	}
diff --git a/http-push.c b/http-push.c
index 04cb238..7814666 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1238,10 +1238,8 @@ xml_start_tag(void *userData, const char
 	strcat(ctx->name, ".");
 	strcat(ctx->name, c);
 
-	if (ctx->cdata) {
-		free(ctx->cdata);
-		ctx->cdata = NULL;
-	}
+	free(ctx->cdata);
+	ctx->cdata = NULL;
 
 	ctx->userFunc(ctx, 0);
 }
@@ -1268,8 +1266,7 @@ static void
 xml_cdata(void *userData, const XML_Char *s, int len)
 {
 	struct xml_ctx *ctx = (struct xml_ctx *)userData;
-	if (ctx->cdata)
-		free(ctx->cdata);
+	free(ctx->cdata);
 	ctx->cdata = xmalloc(len + 1);
 	strlcpy(ctx->cdata, s, len + 1);
 }
@@ -1518,9 +1515,7 @@ static void handle_remote_ls_ctx(struct 
 			ls->dentry_flags |= IS_DIR;
 		}
 	} else if (!strcmp(ctx->name, DAV_PROPFIND_RESP)) {
-		if (ls->dentry_name) {
-			free(ls->dentry_name);
-		}
+		free(ls->dentry_name);
 		ls->dentry_name = NULL;
 		ls->dentry_flags = 0;
 	}
diff --git a/path-list.c b/path-list.c
index f15a10d..b1ee72d 100644
--- a/path-list.c
+++ b/path-list.c
@@ -85,8 +85,7 @@ void path_list_clear(struct path_list *l
 			for (i = 0; i < list->nr; i++) {
 				if (list->strdup_paths)
 					free(list->items[i].path);
-				if (list->items[i].util)
-					free(list->items[i].util);
+				free(list->items[i].util);
 			}
 		free(list->items);
 	}
diff --git a/refs.c b/refs.c
index e70ef0a..aab14fc 100644
--- a/refs.c
+++ b/refs.c
@@ -348,10 +348,8 @@ void unlock_ref(struct ref_lock *lock)
 		if (lock->lk)
 			rollback_lock_file(lock->lk);
 	}
-	if (lock->ref_file)
-		free(lock->ref_file);
-	if (lock->log_file)
-		free(lock->log_file);
+	free(lock->ref_file);
+	free(lock->log_file);
 	free(lock);
 }
 
-- 
1.4.2.g2f76