threads / patch / 16856

patchbuiltin-shortlog.c: use string_list_append() instead of duplicating its code

Subject: [PATCH] builtin-shortlog.c: use string_list_append() instead of duplicating its code

## tl;dr

4 messages between Dec 24, 2008 and Dec 30, 2008. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Adeodato Simó· Dec 24, 2008, 16:34 UTC · lore

Also, when clearing the "onelines" string lists, do not free the "util" member: with string_list_append() is not initialized to any value (and was being initialized to NULL previously anyway).

NB: The duplicated code in builtin-shortlog.c predated the appearance of
string_list_append().

Signed-off-by: Adeodato Simó <dato@net.com.org.es>
---
 builtin-shortlog.c |   14 ++------------
 1 files changed, 2 insertions(+), 12 deletions(-)

Show changes to builtin-shortlog.c +2 −12

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index d03f14f..4c5d761 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -36,7 +36,6 @@ static void insert_one_record(struct shortlog *log,
 	const char *dot3 = log->common_repo_prefix;
 	char *buffer, *p;
 	struct string_list_item *item;
-	struct string_list *onelines;
 	char namebuf[1024];
 	size_t len;
 	const char *eol;
@@ -104,16 +103,7 @@ static void insert_one_record(struct shortlog *log,
 		}
 	}
 
-	onelines = item->util;
-	if (onelines->nr >= onelines->alloc) {
-		onelines->alloc = alloc_nr(onelines->nr);
-		onelines->items = xrealloc(onelines->items,
-				onelines->alloc
-				* sizeof(struct string_list_item));
-	}
-
-	onelines->items[onelines->nr].util = NULL;
-	onelines->items[onelines->nr++].string = buffer;
+	string_list_append(buffer, item->util);
 }
 
 static void read_from_stdin(struct shortlog *log)
@@ -323,7 +313,7 @@ void shortlog_output(struct shortlog *log)
 		}
 
 		onelines->strdup_strings = 1;
-		string_list_clear(onelines, 1);
+		string_list_clear(onelines, 0);
 		free(onelines);
 		log->list.items[i].util = NULL;
 	}

-- 
1.6.0.4

Johannes Schindelin· Dec 30, 2008, 12:20 UTC · re: Adeodato Simó · lore

Re: [PATCH] builtin-shortlog.c: use string_list_append() instead of duplicating its code

Hi,

On Wed, 24 Dec 2008, Adeodato Simó wrote:

Show 8 quoted lines

> Also, when clearing the "onelines" string lists, do not free the "util"
> member: with string_list_append() is not initialized to any value (and
> was being initialized to NULL previously anyway).
> 
> NB: The duplicated code in builtin-shortlog.c predated the appearance of
> string_list_append().
> 
> Signed-off-by: Adeodato Simó <dato@net.com.org.es>

FWIW I like the patch, but would like it even more if the strdup() removal was squashed in (with an explanation in the commit message).

Ciao, Dscho

Adeodato Simó· Dec 30, 2008, 20:25 UTC · re: Johannes Schindelin · lore

[PATCH v2] builtin-shortlog.c: use string_list_append(), and don't strdup unnecessarily

Make cleanup in insert_one_record() use string_list_append(), instead of duplicating its code. Because of this, do not free the "util" member when clearing the "onelines" string lists: with the new code path it is not initialized to any value (was being initialized to NULL previously).

Also, avoid unnecessary strdup() calls when inserting names in log->list. This list always has "strdup_strings" activated, hence strdup'ing namebuf is unnecessary. This change also removes a latent memory leak in the old code.

NB: The duplicated code mentioned above predated the appearance of
string_list_append().

Signed-off-by: Adeodato Simó <dato@net.com.org.es>
---

> FWIW I like the patch, but would like it even more if the strdup() removal 
> was squashed in (with an explanation in the commit message).

Ok, I myself prefer it the other way, but here it is. :-)

 builtin-shortlog.c |   19 +++----------------
 1 files changed, 3 insertions(+), 16 deletions(-)

Show changes to builtin-shortlog.c +3 −16

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index d03f14f..90e76ae 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -36,7 +36,6 @@ static void insert_one_record(struct shortlog *log,
 	const char *dot3 = log->common_repo_prefix;
 	char *buffer, *p;
 	struct string_list_item *item;
-	struct string_list *onelines;
 	char namebuf[1024];
 	size_t len;
 	const char *eol;
@@ -68,12 +67,9 @@ static void insert_one_record(struct shortlog *log,
 		snprintf(namebuf + len, room, " %.*s", maillen, boemail);
 	}
 
-	buffer = xstrdup(namebuf);
-	item = string_list_insert(buffer, &log->list);
+	item = string_list_insert(namebuf, &log->list);
 	if (item->util == NULL)
 		item->util = xcalloc(1, sizeof(struct string_list));
-	else
-		free(buffer);
 
 	/* Skip any leading whitespace, including any blank lines. */
 	while (*oneline && isspace(*oneline))
@@ -104,16 +100,7 @@ static void insert_one_record(struct shortlog *log,
 		}
 	}
 
-	onelines = item->util;
-	if (onelines->nr >= onelines->alloc) {
-		onelines->alloc = alloc_nr(onelines->nr);
-		onelines->items = xrealloc(onelines->items,
-				onelines->alloc
-				* sizeof(struct string_list_item));
-	}
-
-	onelines->items[onelines->nr].util = NULL;
-	onelines->items[onelines->nr++].string = buffer;
+	string_list_append(buffer, item->util);
 }
 
 static void read_from_stdin(struct shortlog *log)
@@ -323,7 +310,7 @@ void shortlog_output(struct shortlog *log)
 		}
 
 		onelines->strdup_strings = 1;
-		string_list_clear(onelines, 1);
+		string_list_clear(onelines, 0);
 		free(onelines);
 		log->list.items[i].util = NULL;
 	}

-- 
1.6.1.307.g07803

Adeodato Simó· Dec 30, 2008, 21:01 UTC · re: Adeodato Simó · lore

[PATCH v3] builtin-shortlog.c: use string_list_append(), and don't strdup unnecessarily

Make insert_one_record() use string_list_append(), instead of duplicating its code. Because of this, do not free the "util" member when clearing the "onelines" string lists: with the new code path it is not initialized to any value (was being initialized to NULL previously).

NB: The duplicated code mentioned above predated the appearance of
string_list_append().

Signed-off-by: Adeodato Simó <dato@net.com.org.es>
---

Now with the obvious mistake in the log fixed, apologies. (s/cleanup in//)

 builtin-shortlog.c |   19 +++----------------
 1 files changed, 3 insertions(+), 16 deletions(-)

Show changes to builtin-shortlog.c +3 −16

diff --git a/builtin-shortlog.c b/builtin-shortlog.c
index d03f14f..90e76ae 100644
--- a/builtin-shortlog.c
+++ b/builtin-shortlog.c
@@ -36,7 +36,6 @@ static void insert_one_record(struct shortlog *log,
 	const char *dot3 = log->common_repo_prefix;
 	char *buffer, *p;
 	struct string_list_item *item;
-	struct string_list *onelines;
 	char namebuf[1024];
 	size_t len;
 	const char *eol;
@@ -68,12 +67,9 @@ static void insert_one_record(struct shortlog *log,
 		snprintf(namebuf + len, room, " %.*s", maillen, boemail);
 	}
 
-	buffer = xstrdup(namebuf);
-	item = string_list_insert(buffer, &log->list);
+	item = string_list_insert(namebuf, &log->list);
 	if (item->util == NULL)
 		item->util = xcalloc(1, sizeof(struct string_list));
-	else
-		free(buffer);
 
 	/* Skip any leading whitespace, including any blank lines. */
 	while (*oneline && isspace(*oneline))
@@ -104,16 +100,7 @@ static void insert_one_record(struct shortlog *log,
 		}
 	}
 
-	onelines = item->util;
-	if (onelines->nr >= onelines->alloc) {
-		onelines->alloc = alloc_nr(onelines->nr);
-		onelines->items = xrealloc(onelines->items,
-				onelines->alloc
-				* sizeof(struct string_list_item));
-	}
-
-	onelines->items[onelines->nr].util = NULL;
-	onelines->items[onelines->nr++].string = buffer;
+	string_list_append(buffer, item->util);
 }
 
 static void read_from_stdin(struct shortlog *log)
@@ -323,7 +310,7 @@ void shortlog_output(struct shortlog *log)
 		}
 
 		onelines->strdup_strings = 1;
-		string_list_clear(onelines, 1);
+		string_list_clear(onelines, 0);
 		free(onelines);
 		log->list.items[i].util = NULL;
 	}

-- 
1.6.1.307.g07803