[PATCH 0/7] fmt-merge-msg improvements

[PATCH 0/7] fmt-merge-msg improvements

[Stephen Boyd]

The first one might be considered a bug, although triggering it is
probably impossible. Not much of a price to pay for correctness
though.

The next two patches modernize and update the test a bit.

The next 3 patches try and reduce the code size by libifying(?)
fmt-merge-msg.

The final patch is just a "while I'm here" thing and might be
unnecessary if we decide to just remove the summary option.

Stephen Boyd (7):
  fmt-merge-msg: be quiet if nothing to merge
  t6200: modernize with test_tick
  t6200: test fmt-merge-msg more
  fmt-merge-msg: use pretty.c routines
  string-list: add unsorted_string_list_lookup()
  fmt-merge-msg: remove custom string_list implementation
  fmt-merge-msg: hide summary option

 Documentation/technical/api-string-list.txt |    6 +-
 builtin/fmt-merge-msg.c                     |  159 ++++++++--------------
 string-list.c                               |   13 ++-
 string-list.h                               |    3 +-
 t/t6200-fmt-merge-msg.sh                    |  196 ++++++++++++++++++++++-----
 5 files changed, 235 insertions(+), 142 deletions(-)

[PATCH 1/7] fmt-merge-msg: be quiet if nothing to merge

[Stephen Boyd]

When FETCH_HEAD contains only 'not-for-merge' entries fmt-merge-msg
still outputs "Merge" (and if the branch isn't master " into <branch>").
In this case fmt-merge-msg is outputting junk and should really just
be quiet. Fix it.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin/fmt-merge-msg.c  |    3 +++
 t/t6200-fmt-merge-msg.sh |   19 +++++++++++++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 9d52400..9bb2625 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -281,6 +281,9 @@ int fmt_merge_msg(int merge_summary, struct strbuf *in, struct strbuf *out) {
 			die ("Error in line %d: %.*s", i, len, p);
 	}
 
+	if (!srcs.nr)
+		return 0;
+
 	strbuf_addstr(out, "Merge ");
 	for (i = 0; i < srcs.nr; i++) {
 		struct src_data *src_data = srcs.payload[i];
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 42f6fff..ade209a 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -21,6 +21,8 @@ test_expect_success setup '
 	setdate &&
 	git commit -m "Initial" &&
 
+	git clone . remote &&
+
 	echo uno >one &&
 	echo dos >two &&
 	git add two &&
@@ -240,4 +242,21 @@ test_expect_success 'merge-msg -F in subdirectory' '
 	test_cmp expected actual
 '
 
+test_expect_success 'merge-msg with nothing to merge' '
+
+	git config --unset-all merge.log
+	git config --unset-all merge.summary
+	git config merge.summary yes &&
+
+	(
+		cd remote &&
+		git checkout -b unrelated &&
+		setdate &&
+		git fetch origin &&
+		git fmt-merge-msg <.git/FETCH_HEAD >../actual
+	) &&
+
+	test_cmp /dev/null actual
+'
+
 test_done
-- 
1.7.0.3.254.g4503b

[PATCH 2/7] t6200: modernize with test_tick

[Stephen Boyd]

This test defines its own version of test_tick. Get rid of it.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 t/t6200-fmt-merge-msg.sh |   64 +++++++++++++++++++++-------------------------
 1 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index ade209a..b24c5bf 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -7,18 +7,10 @@ test_description='fmt-merge-msg test'
 
 . ./test-lib.sh
 
-datestamp=1151939923
-setdate () {
-	GIT_COMMITTER_DATE="$datestamp +0200"
-	GIT_AUTHOR_DATE="$datestamp +0200"
-	datestamp=`expr "$datestamp" + 1`
-	export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
-}
-
 test_expect_success setup '
 	echo one >one &&
 	git add one &&
-	setdate &&
+	test_tick &&
 	git commit -m "Initial" &&
 
 	git clone . remote &&
@@ -26,46 +18,48 @@ test_expect_success setup '
 	echo uno >one &&
 	echo dos >two &&
 	git add two &&
-	setdate &&
+	test_tick &&
 	git commit -a -m "Second" &&
 
 	git checkout -b left &&
 
-	echo $datestamp >one &&
-	setdate &&
+	echo "c1" >one &&
+	test_tick &&
 	git commit -a -m "Common #1" &&
 
-	echo $datestamp >one &&
-	setdate &&
+	echo "c2" >one &&
+	test_tick &&
 	git commit -a -m "Common #2" &&
 
 	git branch right &&
 
-	echo $datestamp >two &&
-	setdate &&
+	echo "l3" >two &&
+	test_tick &&
 	git commit -a -m "Left #3" &&
 
-	echo $datestamp >two &&
-	setdate &&
+	echo "l4" >two &&
+	test_tick &&
 	git commit -a -m "Left #4" &&
 
-	echo $datestamp >two &&
-	setdate &&
+	echo "l5" >two &&
+	test_tick &&
 	git commit -a -m "Left #5" &&
+	git tag tag-l5 &&
 
 	git checkout right &&
 
-	echo $datestamp >three &&
+	echo "r3" >three &&
 	git add three &&
-	setdate &&
+	test_tick &&
 	git commit -a -m "Right #3" &&
+	git tag tag-r3 &&
 
-	echo $datestamp >three &&
-	setdate &&
+	echo "r4" >three &&
+	test_tick &&
 	git commit -a -m "Right #4" &&
 
-	echo $datestamp >three &&
-	setdate &&
+	echo "r5" >three &&
+	test_tick &&
 	git commit -a -m "Right #5" &&
 
 	git show-branch
@@ -115,7 +109,7 @@ test_expect_success 'merge-msg test #3-1' '
 	git config merge.log true &&
 
 	git checkout master &&
-	setdate &&
+	test_tick &&
 	git fetch . left &&
 
 	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
@@ -129,7 +123,7 @@ test_expect_success 'merge-msg test #3-2' '
 	git config merge.summary true &&
 
 	git checkout master &&
-	setdate &&
+	test_tick &&
 	git fetch . left &&
 
 	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
@@ -161,7 +155,7 @@ test_expect_success 'merge-msg test #4-1' '
 	git config merge.log true &&
 
 	git checkout master &&
-	setdate &&
+	test_tick &&
 	git fetch . left right &&
 
 	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
@@ -175,7 +169,7 @@ test_expect_success 'merge-msg test #4-2' '
 	git config merge.summary true &&
 
 	git checkout master &&
-	setdate &&
+	test_tick &&
 	git fetch . left right &&
 
 	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
@@ -189,7 +183,7 @@ test_expect_success 'merge-msg test #5-1' '
 	git config merge.log yes &&
 
 	git checkout master &&
-	setdate &&
+	test_tick &&
 	git fetch . left right &&
 
 	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
@@ -203,7 +197,7 @@ test_expect_success 'merge-msg test #5-2' '
 	git config merge.summary yes &&
 
 	git checkout master &&
-	setdate &&
+	test_tick &&
 	git fetch . left right &&
 
 	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
@@ -217,7 +211,7 @@ test_expect_success 'merge-msg -F' '
 	git config merge.summary yes &&
 
 	git checkout master &&
-	setdate &&
+	test_tick &&
 	git fetch . left right &&
 
 	git fmt-merge-msg -F .git/FETCH_HEAD >actual &&
@@ -231,7 +225,7 @@ test_expect_success 'merge-msg -F in subdirectory' '
 	git config merge.summary yes &&
 
 	git checkout master &&
-	setdate &&
+	test_tick &&
 	git fetch . left right &&
 	mkdir sub &&
 	cp .git/FETCH_HEAD sub/FETCH_HEAD &&
@@ -251,7 +245,7 @@ test_expect_success 'merge-msg with nothing to merge' '
 	(
 		cd remote &&
 		git checkout -b unrelated &&
-		setdate &&
+		test_tick &&
 		git fetch origin &&
 		git fmt-merge-msg <.git/FETCH_HEAD >../actual
 	) &&
-- 
1.7.0.3.254.g4503b

[PATCH 3/7] t6200: test fmt-merge-msg more

[Stephen Boyd]

Add some more tests so we don't break behavior upon modernizing
fmt-merge-msg.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 t/t6200-fmt-merge-msg.sh |  115 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index b24c5bf..42f8ece 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -62,6 +62,14 @@ test_expect_success setup '
 	test_tick &&
 	git commit -a -m "Right #5" &&
 
+	git checkout -b long &&
+	i=0 &&
+	while test $i -lt 30
+	do
+		test_commit $i one &&
+		i=$(($i+1))
+	done &&
+
 	git show-branch
 '
 
@@ -253,4 +261,111 @@ test_expect_success 'merge-msg with nothing to merge' '
 	test_cmp /dev/null actual
 '
 
+cat >expected <<\EOF
+Merge tag 'tag-r3'
+
+* tag 'tag-r3':
+  Right #3
+  Common #2
+  Common #1
+EOF
+
+test_expect_success 'merge-msg tag' '
+
+	git config --unset-all merge.log
+	git config --unset-all merge.summary
+	git config merge.summary yes &&
+
+	git checkout master &&
+	test_tick &&
+	git fetch . tag tag-r3 &&
+
+	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+Merge tags 'tag-r3' and 'tag-l5'
+
+* tag 'tag-r3':
+  Right #3
+  Common #2
+  Common #1
+
+* tag 'tag-l5':
+  Left #5
+  Left #4
+  Left #3
+  Common #2
+  Common #1
+EOF
+
+test_expect_success 'merge-msg two tags' '
+
+	git config --unset-all merge.log
+	git config --unset-all merge.summary
+	git config merge.summary yes &&
+
+	git checkout master &&
+	test_tick &&
+	git fetch . tag tag-r3 tag tag-l5 &&
+
+	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+Merge branch 'left', tag 'tag-r3'
+
+* tag 'tag-r3':
+  Right #3
+  Common #2
+  Common #1
+
+* left:
+  Left #5
+  Left #4
+  Left #3
+  Common #2
+  Common #1
+EOF
+
+test_expect_success 'merge-msg tag and branch' '
+
+	git config --unset-all merge.log
+	git config --unset-all merge.summary
+	git config merge.summary yes &&
+
+	git checkout master &&
+	test_tick &&
+	git fetch . tag tag-r3 left &&
+
+	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+Merge branch 'long'
+
+* long: (35 commits)
+EOF
+
+test_expect_success 'merge-msg lots of commits' '
+
+	git checkout master &&
+	test_tick &&
+	git fetch . long &&
+
+	i=29 &&
+	while test $i -gt 9
+	do
+		echo "  $i" &&
+		i=$(($i-1))
+	done >>expected &&
+	echo "  ..." >>expected
+
+	git fmt-merge-msg <.git/FETCH_HEAD >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.7.0.3.254.g4503b

[PATCH 4/7] fmt-merge-msg: use pretty.c routines

[Stephen Boyd]

This command duplicates functionality of the '%s' pretty format.
Simplify the code a bit by using the pretty printing routine
instead of open-coding it here.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin/fmt-merge-msg.c |   29 ++++++++---------------------
 1 files changed, 8 insertions(+), 21 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 9bb2625..44b74f4 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -185,6 +185,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 	struct object *branch;
 	struct list subjects = { NULL, NULL, 0, 0 };
 	int flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;
+	struct strbuf sb = STRBUF_INIT;
 
 	branch = deref_tag(parse_object(sha1), sha1_to_hex(sha1), 40);
 	if (!branch || branch->type != OBJ_COMMIT)
@@ -198,7 +199,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 	if (prepare_revision_walk(rev))
 		die("revision walk setup failed");
 	while ((commit = get_revision(rev)) != NULL) {
-		char *oneline, *bol, *eol;
+		struct pretty_print_context ctx = {0};
 
 		/* ignore merges */
 		if (commit->parents && commit->parents->next)
@@ -208,30 +209,16 @@ static void shortlog(const char *name, unsigned char *sha1,
 		if (subjects.nr > limit)
 			continue;
 
-		bol = strstr(commit->buffer, "\n\n");
-		if (bol) {
-			unsigned char c;
-			do {
-				c = *++bol;
-			} while (isspace(c));
-			if (!c)
-				bol = NULL;
-		}
+		format_commit_message(commit, "%s", &sb, &ctx);
+		strbuf_ltrim(&sb);
 
-		if (!bol) {
+		if (!sb.len)
 			append_to_list(&subjects, xstrdup(sha1_to_hex(
 							commit->object.sha1)),
 					NULL);
-			continue;
-		}
-
-		eol = strchr(bol, '\n');
-		if (eol) {
-			oneline = xmemdupz(bol, eol - bol);
-		} else {
-			oneline = xstrdup(bol);
-		}
-		append_to_list(&subjects, oneline, NULL);
+		else
+			append_to_list(&subjects, strbuf_detach(&sb, NULL),
+					NULL);
 	}
 
 	if (count > limit)
-- 
1.7.0.3.254.g4503b

[PATCH 5/7] string-list: add unsorted_string_list_lookup()

[Stephen Boyd]

Sometimes users need to lookup a string in an unsorted string_list. In
that case they should use this function instead of the version for
sorted strings.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 Documentation/technical/api-string-list.txt |    6 +++++-
 string-list.c                               |   13 ++++++++++---
 string-list.h                               |    3 ++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
index 293bb15..6d8c24b 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -104,8 +104,12 @@ write `string_list_insert(...)->util = ...;`.
 `unsorted_string_list_has_string`::
 
 	It's like `string_list_has_string()` but for unsorted lists.
+
+`unsorted_string_list_lookup`::
+
+	It's like `string_list_lookup()` but for unsorted lists.
 +
-This function needs to look through all items, as opposed to its
+The above two functions need to look through all items, as opposed to their
 counterpart for sorted lists, which performs a binary search.
 
 Data structures
diff --git a/string-list.c b/string-list.c
index 1ac536e..c9ad7fc 100644
--- a/string-list.c
+++ b/string-list.c
@@ -168,12 +168,19 @@ void sort_string_list(struct string_list *list)
 	qsort(list->items, list->nr, sizeof(*list->items), cmp_items);
 }
 
-int unsorted_string_list_has_string(struct string_list *list, const char *string)
+struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
+						     const char *string)
 {
 	int i;
 	for (i = 0; i < list->nr; i++)
 		if (!strcmp(string, list->items[i].string))
-			return 1;
-	return 0;
+			return list->items + i;
+	return NULL;
+}
+
+int unsorted_string_list_has_string(struct string_list *list,
+				    const char *string)
+{
+	return unsorted_string_list_lookup(list, string) != NULL;
 }
 
diff --git a/string-list.h b/string-list.h
index 6569cf6..63b69c8 100644
--- a/string-list.h
+++ b/string-list.h
@@ -38,5 +38,6 @@ struct string_list_item *string_list_lookup(const char *string, struct string_li
 struct string_list_item *string_list_append(const char *string, struct string_list *list);
 void sort_string_list(struct string_list *list);
 int unsorted_string_list_has_string(struct string_list *list, const char *string);
-
+struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
+						     const char *string);
 #endif /* STRING_LIST_H */
-- 
1.7.0.3.254.g4503b

[PATCH 6/7] fmt-merge-msg: remove custom string_list implementation

[Stephen Boyd]

This command uses a custom version of string list when it could
just as easily use the string_list API. Convert it to use string_list
and reduce the code size a bit.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

Anyone else annoyed that string_list_append() is the only function
that takes doesn't take the string list as the first argument?

On another note, git-compat-util.h:115 says:

  #undef _ALL_SOURCE /* AIX 5.3L defines a struct list with...

which may be unnecessary now that struct list is gone. Need someone
on AIX 5.3L to test that though.

Finally, this doesn't free some of the lists since I wanted it to be a
straight conversion. I'm feeling lazy right now but I'll think about it.

 builtin/fmt-merge-msg.c |  127 ++++++++++++++++------------------------------
 1 files changed, 44 insertions(+), 83 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 44b74f4..0fb4014 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -4,6 +4,7 @@
 #include "diff.h"
 #include "revision.h"
 #include "tag.h"
+#include "string-list.h"
 
 static const char * const fmt_merge_msg_usage[] = {
 	"git fmt-merge-msg [--log|--no-log] [--file <file>]",
@@ -24,58 +25,21 @@ static int fmt_merge_msg_config(const char *key, const char *value, void *cb)
 	return 0;
 }
 
-struct list {
-	char **list;
-	void **payload;
-	unsigned nr, alloc;
+struct src_data {
+	struct string_list branch, tag, r_branch, generic;
+	int head_status;
 };
 
-static void append_to_list(struct list *list, char *value, void *payload)
-{
-	if (list->nr == list->alloc) {
-		list->alloc += 32;
-		list->list = xrealloc(list->list, sizeof(char *) * list->alloc);
-		list->payload = xrealloc(list->payload,
-				sizeof(char *) * list->alloc);
-	}
-	list->payload[list->nr] = payload;
-	list->list[list->nr++] = value;
-}
-
-static int find_in_list(struct list *list, char *value)
-{
-	int i;
-
-	for (i = 0; i < list->nr; i++)
-		if (!strcmp(list->list[i], value))
-			return i;
-
-	return -1;
-}
-
-static void free_list(struct list *list)
+void init_src_data(struct src_data *data)
 {
-	int i;
-
-	if (list->alloc == 0)
-		return;
-
-	for (i = 0; i < list->nr; i++) {
-		free(list->list[i]);
-		free(list->payload[i]);
-	}
-	free(list->list);
-	free(list->payload);
-	list->nr = list->alloc = 0;
+	data->branch.strdup_strings = 1;
+	data->tag.strdup_strings = 1;
+	data->r_branch.strdup_strings = 1;
+	data->generic.strdup_strings = 1;
 }
 
-struct src_data {
-	struct list branch, tag, r_branch, generic;
-	int head_status;
-};
-
-static struct list srcs = { NULL, NULL, 0, 0};
-static struct list origins = { NULL, NULL, 0, 0};
+static struct string_list srcs = { NULL, 0, 0, 1 };
+static struct string_list origins = { NULL, 0, 0, 1 };
 
 static int handle_line(char *line)
 {
@@ -83,6 +47,7 @@ static int handle_line(char *line)
 	unsigned char *sha1;
 	char *src, *origin;
 	struct src_data *src_data;
+	struct string_list_item *item;
 	int pulling_head = 0;
 
 	if (len < 43 || line[40] != '\t')
@@ -115,64 +80,62 @@ static int handle_line(char *line)
 		pulling_head = 1;
 	}
 
-	i = find_in_list(&srcs, src);
-	if (i < 0) {
-		i = srcs.nr;
-		append_to_list(&srcs, xstrdup(src),
-				xcalloc(1, sizeof(struct src_data)));
+	item = unsorted_string_list_lookup(&srcs, src);
+	if (!item) {
+		item = string_list_append(src, &srcs);
+		item->util = xcalloc(1, sizeof(struct src_data));
+		init_src_data(item->util);
 	}
-	src_data = srcs.payload[i];
+	src_data = item->util;
 
 	if (pulling_head) {
-		origin = xstrdup(src);
+		origin = src;
 		src_data->head_status |= 1;
 	} else if (!prefixcmp(line, "branch ")) {
-		origin = xstrdup(line + 7);
-		append_to_list(&src_data->branch, origin, NULL);
+		origin = line + 7;
+		string_list_append(origin, &src_data->branch);
 		src_data->head_status |= 2;
 	} else if (!prefixcmp(line, "tag ")) {
 		origin = line;
-		append_to_list(&src_data->tag, xstrdup(origin + 4), NULL);
+		string_list_append(origin + 4, &src_data->tag);
 		src_data->head_status |= 2;
 	} else if (!prefixcmp(line, "remote branch ")) {
-		origin = xstrdup(line + 14);
-		append_to_list(&src_data->r_branch, origin, NULL);
+		origin = line + 14;
+		string_list_append(origin, &src_data->r_branch);
 		src_data->head_status |= 2;
 	} else {
-		origin = xstrdup(src);
-		append_to_list(&src_data->generic, xstrdup(line), NULL);
+		origin = src;
+		string_list_append(line, &src_data->generic);
 		src_data->head_status |= 2;
 	}
 
 	if (!strcmp(".", src) || !strcmp(src, origin)) {
 		int len = strlen(origin);
-		if (origin[0] == '\'' && origin[len - 1] == '\'') {
+		if (origin[0] == '\'' && origin[len - 1] == '\'')
 			origin = xmemdupz(origin + 1, len - 2);
-		} else {
-			origin = xstrdup(origin);
-		}
 	} else {
 		char *new_origin = xmalloc(strlen(origin) + strlen(src) + 5);
 		sprintf(new_origin, "%s of %s", origin, src);
 		origin = new_origin;
 	}
-	append_to_list(&origins, origin, sha1);
+	string_list_append(origin, &origins)->util = sha1;
 	return 0;
 }
 
 static void print_joined(const char *singular, const char *plural,
-		struct list *list, struct strbuf *out)
+		struct string_list *list, struct strbuf *out)
 {
 	if (list->nr == 0)
 		return;
 	if (list->nr == 1) {
-		strbuf_addf(out, "%s%s", singular, list->list[0]);
+		strbuf_addf(out, "%s%s", singular, list->items[0].string);
 	} else {
 		int i;
 		strbuf_addstr(out, plural);
 		for (i = 0; i < list->nr - 1; i++)
-			strbuf_addf(out, "%s%s", i > 0 ? ", " : "", list->list[i]);
-		strbuf_addf(out, " and %s", list->list[list->nr - 1]);
+			strbuf_addf(out, "%s%s", i > 0 ? ", " : "",
+				    list->items[i].string);
+		strbuf_addf(out, " and %s", list->items[list->nr - 1].string);
 	}
 }
 
@@ -183,7 +146,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 	int i, count = 0;
 	struct commit *commit;
 	struct object *branch;
-	struct list subjects = { NULL, NULL, 0, 0 };
+	struct string_list subjects = { NULL, 0, 0, 1 };
 	int flags = UNINTERESTING | TREESAME | SEEN | SHOWN | ADDED;
 	struct strbuf sb = STRBUF_INIT;
 
@@ -213,12 +176,10 @@ static void shortlog(const char *name, unsigned char *sha1,
 		strbuf_ltrim(&sb);
 
 		if (!sb.len)
-			append_to_list(&subjects, xstrdup(sha1_to_hex(
-							commit->object.sha1)),
-					NULL);
+			string_list_append(sha1_to_hex(commit->object.sha1),
+					   &subjects);
 		else
-			append_to_list(&subjects, strbuf_detach(&sb, NULL),
-					NULL);
+			string_list_append(strbuf_detach(&sb, NULL), &subjects);
 	}
 
 	if (count > limit)
@@ -230,7 +191,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 		if (i >= limit)
 			strbuf_addf(out, "  ...\n");
 		else
-			strbuf_addf(out, "  %s\n", subjects.list[i]);
+			strbuf_addf(out, "  %s\n", subjects.items[i].string);
 
 	clear_commit_marks((struct commit *)branch, flags);
 	clear_commit_marks(head, flags);
@@ -238,7 +199,7 @@ static void shortlog(const char *name, unsigned char *sha1,
 	rev->commits = NULL;
 	rev->pending.nr = 0;
 
-	free_list(&subjects);
+	string_list_clear(&subjects, 0);
 }
 
 int fmt_merge_msg(int merge_summary, struct strbuf *in, struct strbuf *out) {
@@ -273,14 +234,14 @@ int fmt_merge_msg(int merge_summary, struct strbuf *in, struct strbuf *out) {
 
 	strbuf_addstr(out, "Merge ");
 	for (i = 0; i < srcs.nr; i++) {
-		struct src_data *src_data = srcs.payload[i];
+		struct src_data *src_data = srcs.items[i].util;
 		const char *subsep = "";
 
 		strbuf_addstr(out, sep);
 		sep = "; ";
 
 		if (src_data->head_status == 1) {
-			strbuf_addstr(out, srcs.list[i]);
+			strbuf_addstr(out, srcs.items[i].string);
 			continue;
 		}
 		if (src_data->head_status == 3) {
@@ -309,8 +270,8 @@ int fmt_merge_msg(int merge_summary, struct strbuf *in, struct strbuf *out) {
 			print_joined("commit ", "commits ", &src_data->generic,
 					out);
 		}
-		if (strcmp(".", srcs.list[i]))
-			strbuf_addf(out, " of %s", srcs.list[i]);
+		if (strcmp(".", srcs.items[i].string))
+			strbuf_addf(out, " of %s", srcs.items[i].string);
 	}
 
 	if (!strcmp("master", current_branch))
@@ -329,7 +290,7 @@ int fmt_merge_msg(int merge_summary, struct strbuf *in, struct strbuf *out) {
 		rev.limited = 1;
 
 		for (i = 0; i < origins.nr; i++)
-			shortlog(origins.list[i], origins.payload[i],
+			shortlog(origins.items[i].string, origins.items[i].util,
 					head, &rev, limit, out);
 	}
 	return 0;
-- 
1.7.0.3.254.g4503b

[PATCH 7/7] fmt-merge-msg: hide summary option

[Stephen Boyd]

The --summary command line option has been deprecated in favor of --log.
Hide the option from the help message to further discourage the use of
this option.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

When is this option going to be removed? It's approaching 2 years
since the deprecation occured in 6cd9cfe (fmt-merge-msg: add
'--(no-)log' options and 'merge.log' config variable, 2008-04-06)

 builtin/fmt-merge-msg.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 0fb4014..379a031 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -301,7 +301,9 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix)
 	const char *inpath = NULL;
 	struct option options[] = {
 		OPT_BOOLEAN(0, "log",     &merge_summary, "populate log with the shortlog"),
-		OPT_BOOLEAN(0, "summary", &merge_summary, "alias for --log"),
+		{ OPTION_BOOLEAN, 0, "summary", &merge_summary, NULL,
+		  "alias for --log (deprecated)",
+		  PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
 		OPT_FILENAME('F', "file", &inpath, "file to read from"),
 		OPT_END()
 	};
-- 
1.7.0.3.254.g4503b

Re: [PATCH 1/7] fmt-merge-msg: be quiet if nothing to merge

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> When FETCH_HEAD contains only 'not-for-merge' entries fmt-merge-msg
> still outputs "Merge" (and if the branch isn't master " into <branch>").
> In this case fmt-merge-msg is outputting junk and should really just
> be quiet. Fix it.

Hmm, doesn't pull fail in such a case anyway?  Is there a real damage?

I am just trying asses if this is with a maint-worthy urgency.  The patch
looks sane.

Thanks.

Re: [PATCH 7/7] fmt-merge-msg: hide summary option

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> When is this option going to be removed? It's approaching 2 years
> since the deprecation occured in 6cd9cfe (fmt-merge-msg: add
> '--(no-)log' options and 'merge.log' config variable, 2008-04-06)

We could do that sometime in this autumn timeframe if we start making
noises when they are used, just like we did during the 1.7.0 transition
against soon-to-be-deprecated "features".

Are there other ancient features we have been passively advertising as
deprecated that we should now start the removal process?

Re: [PATCH 1/7] fmt-merge-msg: be quiet if nothing to merge

[Stephen Boyd]

On 03/24/2010 07:39 PM, Junio C Hamano wrote:
>
> Hmm, doesn't pull fail in such a case anyway?  Is there a real damage?
>
> I am just trying asses if this is with a maint-worthy urgency.  The patch
> looks sane.

I don't think this is ever triggered with pull so probably not
maint-worthy. I just ordered it that way in case you figured it as such.

Re: [PATCH 7/7] fmt-merge-msg: hide summary option

[Stephen Boyd]

On 03/24/2010 07:45 PM, Junio C Hamano wrote:
> We could do that sometime in this autumn timeframe if we start making
> noises when they are used, just like we did during the 1.7.0 transition
> against soon-to-be-deprecated "features".

We already make noises when the option is used. Probably need to make
some noises in release notes though.

> Are there other ancient features we have been passively advertising as
> deprecated that we should now start the removal process?

grepping for deprecated shows mostly deprecated commands:

    git lost-found
    git tar-tree
    git peek-remote (synonym to git ls-remote)
    git init-db (synonym to git init)
    git repo-config (synonym to git config)

I'm pretty sure we don't want to remove them anytime soon though, right?

The only option of interest is 'git reset --mixed' with paths. I don't
really know the backstory on that but it looks like it was deprecated
when the command was made into a builtin 0e5a7fa (Make "git reset" a
builtin., 2007-09-11). Maybe it can become a die now?

Oh and git notes has a deprecation warning but I think we'll probably
have to revisit that one in a few years.

Re: [PATCH 7/7] fmt-merge-msg: hide summary option

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> On 03/24/2010 07:45 PM, Junio C Hamano wrote:
>> We could do that sometime in this autumn timeframe if we start making
>> noises when they are used, just like we did during the 1.7.0 transition
>> against soon-to-be-deprecated "features".
>
> We already make noises when the option is used. Probably need to make
> some noises in release notes though.

Then it would be one of the 1.7.2 items, I guess, as we haven't shipped
1.7.1 yet.

Re: [PATCH 6/7] fmt-merge-msg: remove custom string_list implementation

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> This command uses a custom version of string list when it could
> just as easily use the string_list API. Convert it to use string_list
> and reduce the code size a bit.

Thanks, although the above is a bit unfair.  Back when 00449f9 (Make
git-fmt-merge-msg a builtin, 2006-07-03) was written, there was no such
thing like string-list (nor path-list, its predecessor).

> Anyone else annoyed that string_list_append() is the only function
> that takes doesn't take the string list as the first argument?

Yes.

> On another note, git-compat-util.h:115 says:
>
>   #undef _ALL_SOURCE /* AIX 5.3L defines a struct list with...
>
> which may be unnecessary now that struct list is gone. Need someone
> on AIX 5.3L to test that though.

Yeah, finding testers for less common platforms is always the hardest
part.

> Finally, this doesn't free some of the lists since I wanted it to be a
> straight conversion. I'm feeling lazy right now but I'll think about it.

Thanks.

Re: [PATCH 6/7] fmt-merge-msg: remove custom string_list implementation

[Stephen Boyd]

On 03/29/2010 10:18 PM, Junio C Hamano wrote:
> Stephen Boyd <bebarino@gmail.com> writes:
>   
>> This command uses a custom version of string list when it could
>> just as easily use the string_list API. Convert it to use string_list
>> and reduce the code size a bit.
>>     
> Thanks, although the above is a bit unfair.  Back when 00449f9 (Make
> git-fmt-merge-msg a builtin, 2006-07-03) was written, there was no such
> thing like string-list (nor path-list, its predecessor).
>   

Sorry, I didn't mean to be misleading. Maybe something like:

This command uses a custom version of string_list since back when it
was written 00449f9 (Make git-fmt-merge-msg a builtin, 2006-07-03)
there was no string_list API (nor path_list, its predecessor). Convert
it to use string_list and reduce the code size a bit.

[PATCH 8/7] Make string_list_append() consistent with everything else

[Stephen Boyd]

string_list_append() is the only function in the string_list API which takes
the struct string_list as the second argument instead of the first. Swap the
ordering to make the string_list API consistent.

This was mostly done with

 $ git grep -lz 'string_list_append' |
   xargs -0 perl -pi -e
   's/string_list_append\((.*), ([^)]+)\)/string_list_append\($2, $1)/'

and then some minor fixups and manual verification.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---

 On 03/29/2010 10:18 PM, Junio C Hamano wrote:
 > Stephen Boyd <bebarino@gmail.com> writes:
 >
 >> Anyone else annoyed that string_list_append() is the only function
 >> that takes doesn't take the string list as the first argument?
 >
 > Yes.

 This swaps it.

 Documentation/technical/api-string-list.txt |    4 +-
 builtin/apply.c                             |    2 +-
 builtin/fast-export.c                       |    4 +-
 builtin/fetch.c                             |    8 ++--
 builtin/fmt-merge-msg.c                     |   18 +++++-----
 builtin/log.c                               |   20 ++++++------
 builtin/remote.c                            |   46 +++++++++++++-------------
 builtin/rerere.c                            |    2 +-
 builtin/shortlog.c                          |    2 +-
 notes.c                                     |    6 ++--
 remote.c                                    |    2 +-
 revision.c                                  |    4 +-
 string-list.c                               |    2 +-
 string-list.h                               |    2 +-
 14 files changed, 61 insertions(+), 61 deletions(-)

diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
index 6d8c24b..3f575bd 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -38,8 +38,8 @@ struct string_list list;
 int i;
 
 memset(&list, 0, sizeof(struct string_list));
-string_list_append("foo", &list);
-string_list_append("bar", &list);
+string_list_append(&list, "foo");
+string_list_append(&list, "bar");
 for (i = 0; i < list.nr; i++)
 	printf("%s\n", list.items[i].string)
 ----
diff --git a/builtin/apply.c b/builtin/apply.c
index 7ca9047..2d02d17 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c
@@ -3404,7 +3404,7 @@ static void add_name_limit(const char *name, int exclude)
 {
 	struct string_list_item *it;
 
-	it = string_list_append(name, &limit_by_name);
+	it = string_list_append(&limit_by_name, name);
 	it->util = exclude ? NULL : (void *) 1;
 }
 
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index b0a4029..b353861 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -438,7 +438,7 @@ static void get_tags_and_duplicates(struct object_array *pending,
 			/* handle nested tags */
 			while (tag && tag->object.type == OBJ_TAG) {
 				parse_object(tag->object.sha1);
-				string_list_append(full_name, extra_refs)->util = tag;
+				string_list_append(extra_refs, full_name)->util = tag;
 				tag = (struct tag *)tag->tagged;
 			}
 			if (!tag)
@@ -464,7 +464,7 @@ static void get_tags_and_duplicates(struct object_array *pending,
 		}
 		if (commit->util)
 			/* more than one name for the same object */
-			string_list_append(full_name, extra_refs)->util = commit;
+			string_list_append(extra_refs, full_name)->util = commit;
 		else
 			commit->util = full_name;
 	}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 957be9f..e29fff0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -745,7 +745,7 @@ static int get_one_remote_for_fetch(struct remote *remote, void *priv)
 {
 	struct string_list *list = priv;
 	if (!remote->skip_default_update)
-		string_list_append(remote->name, list);
+		string_list_append(list, remote->name);
 	return 0;
 }
 
@@ -764,8 +764,8 @@ static int get_remote_group(const char *key, const char *value, void *priv)
 		int space = strcspn(value, " \t\n");
 		while (*value) {
 			if (space > 1) {
-				string_list_append(xstrndup(value, space),
-						   g->list);
+				string_list_append(g->list,
+						   xstrndup(value, space));
 			}
 			value += space + (value[space] != '\0');
 			space = strcspn(value, " \t\n");
@@ -786,7 +786,7 @@ static int add_remote_or_group(const char *name, struct string_list *list)
 		if (!remote_is_configured(name))
 			return 0;
 		remote = remote_get(name);
-		string_list_append(remote->name, list);
+		string_list_append(list, remote->name);
 	}
 	return 1;
 }
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 379a031..6012012 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -82,7 +82,7 @@ static int handle_line(char *line)
 
 	item = unsorted_string_list_lookup(&srcs, src);
 	if (!item) {
-		item = string_list_append(src, &srcs);
+		item = string_list_append(&srcs, src);
 		item->util = xcalloc(1, sizeof(struct src_data));
 		init_src_data(item->util);
 	}
@@ -93,19 +93,19 @@ static int handle_line(char *line)
 		src_data->head_status |= 1;
 	} else if (!prefixcmp(line, "branch ")) {
 		origin = line + 7;
-		string_list_append(origin, &src_data->branch);
+		string_list_append(&src_data->branch, origin);
 		src_data->head_status |= 2;
 	} else if (!prefixcmp(line, "tag ")) {
 		origin = line;
-		string_list_append(origin + 4, &src_data->tag);
+		string_list_append(&src_data->tag, origin + 4);
 		src_data->head_status |= 2;
 	} else if (!prefixcmp(line, "remote branch ")) {
 		origin = line + 14;
-		string_list_append(origin, &src_data->r_branch);
+		string_list_append(&src_data->r_branch, origin);
 		src_data->head_status |= 2;
 	} else {
 		origin = src;
-		string_list_append(line, &src_data->generic);
+		string_list_append(&src_data->generic, line);
 		src_data->head_status |= 2;
 	}
 
@@ -118,7 +118,7 @@ static int handle_line(char *line)
 		sprintf(new_origin, "%s of %s", origin, src);
 		origin = new_origin;
 	}
-	string_list_append(origin, &origins)->util = sha1;
+	string_list_append(&origins, origin)->util = sha1;
 	return 0;
 }
 
@@ -176,10 +176,10 @@ static void shortlog(const char *name, unsigned char *sha1,
 		strbuf_ltrim(&sb);
 
 		if (!sb.len)
-			string_list_append(sha1_to_hex(commit->object.sha1),
-					   &subjects);
+			string_list_append(&subjects,
+					   sha1_to_hex(commit->object.sha1));
 		else
-			string_list_append(strbuf_detach(&sb, NULL), &subjects);
+			string_list_append(&subjects, strbuf_detach(&sb, NULL));
 	}
 
 	if (count > limit)
diff --git a/builtin/log.c b/builtin/log.c
index a8dd8c9..8e9be47 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -497,13 +497,13 @@ static void add_header(const char *value)
 		len--;
 
 	if (!strncasecmp(value, "to: ", 4)) {
-		item = string_list_append(value + 4, &extra_to);
+		item = string_list_append(&extra_to, value + 4);
 		len -= 4;
 	} else if (!strncasecmp(value, "cc: ", 4)) {
-		item = string_list_append(value + 4, &extra_cc);
+		item = string_list_append(&extra_cc, value + 4);
 		len -= 4;
 	} else {
-		item = string_list_append(value, &extra_hdr);
+		item = string_list_append(&extra_hdr, value);
 	}
 
 	item->string[len] = '\0';
@@ -527,13 +527,13 @@ static int git_format_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "format.to")) {
 		if (!value)
 			return config_error_nonbool(var);
-		string_list_append(value, &extra_to);
+		string_list_append(&extra_to, value);
 		return 0;
 	}
 	if (!strcmp(var, "format.cc")) {
 		if (!value)
 			return config_error_nonbool(var);
-		string_list_append(value, &extra_cc);
+		string_list_append(&extra_cc, value);
 		return 0;
 	}
 	if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) {
@@ -911,7 +911,7 @@ static int to_callback(const struct option *opt, const char *arg, int unset)
 	if (unset)
 		string_list_clear(&extra_to, 0);
 	else
-		string_list_append(arg, &extra_to);
+		string_list_append(&extra_to, arg);
 	return 0;
 }
 
@@ -920,7 +920,7 @@ static int cc_callback(const struct option *opt, const char *arg, int unset)
 	if (unset)
 		string_list_clear(&extra_cc, 0);
 	else
-		string_list_append(arg, &extra_cc);
+		string_list_append(&extra_cc, arg);
 	return 0;
 }
 
@@ -1194,7 +1194,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
 	if (in_reply_to) {
 		const char *msgid = clean_message_id(in_reply_to);
-		string_list_append(msgid, rev.ref_message_ids);
+		string_list_append(rev.ref_message_ids, msgid);
 	}
 	rev.numbered_files = numbered_files;
 	rev.patch_suffix = fmt_patch_suffix;
@@ -1241,8 +1241,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 				    && (!cover_letter || rev.nr > 1))
 					free(rev.message_id);
 				else
-					string_list_append(rev.message_id,
-							   rev.ref_message_ids);
+					string_list_append(rev.ref_message_ids,
+							   rev.message_id);
 			}
 			gen_message_id(&rev, sha1_to_hex(commit->object.sha1));
 		}
diff --git a/builtin/remote.c b/builtin/remote.c
index 277765b..cc7247a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -87,7 +87,7 @@ static int opt_parse_track(const struct option *opt, const char *arg, int not)
 	if (not)
 		string_list_clear(list, 0);
 	else
-		string_list_append(arg, list);
+		string_list_append(list, arg);
 	return 0;
 }
 
@@ -149,7 +149,7 @@ static int add(int argc, const char **argv)
 	strbuf_addf(&buf, "remote.%s.fetch", name);
 
 	if (track.nr == 0)
-		string_list_append("*", &track);
+		string_list_append(&track, "*");
 	for (i = 0; i < track.nr; i++) {
 		struct string_list_item *item = track.items + i;
 
@@ -247,11 +247,11 @@ static int config_read_branches(const char *key, const char *value, void *cb)
 			while (space) {
 				char *merge;
 				merge = xstrndup(value, space - value);
-				string_list_append(merge, &info->merge);
+				string_list_append(&info->merge, merge);
 				value = abbrev_branch(space + 1);
 				space = strchr(value, ' ');
 			}
-			string_list_append(xstrdup(value), &info->merge);
+			string_list_append(&info->merge, xstrdup(value));
 		} else
 			info->rebase = git_config_bool(orig_key, value);
 	}
@@ -288,14 +288,14 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
 	for (ref = fetch_map; ref; ref = ref->next) {
 		unsigned char sha1[20];
 		if (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))
-			string_list_append(abbrev_branch(ref->name), &states->new);
+			string_list_append(&states->new, abbrev_branch(ref->name));
 		else
-			string_list_append(abbrev_branch(ref->name), &states->tracked);
+			string_list_append(&states->tracked, abbrev_branch(ref->name));
 	}
 	stale_refs = get_stale_heads(states->remote, fetch_map);
 	for (ref = stale_refs; ref; ref = ref->next) {
 		struct string_list_item *item =
-			string_list_append(abbrev_branch(ref->name), &states->stale);
+			string_list_append(&states->stale, abbrev_branch(ref->name));
 		item->util = xstrdup(ref->name);
 	}
 	free_refs(stale_refs);
@@ -344,8 +344,8 @@ static int get_push_ref_states(const struct ref *remote_refs,
 			continue;
 		hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
 
-		item = string_list_append(abbrev_branch(ref->peer_ref->name),
-					  &states->push);
+		item = string_list_append(&states->push,
+					  abbrev_branch(ref->peer_ref->name));
 		item->util = xcalloc(sizeof(struct push_info), 1);
 		info = item->util;
 		info->forced = ref->force;
@@ -380,7 +380,7 @@ static int get_push_ref_states_noquery(struct ref_states *states)
 
 	states->push.strdup_strings = 1;
 	if (!remote->push_refspec_nr) {
-		item = string_list_append("(matching)", &states->push);
+		item = string_list_append(&states->push, "(matching)");
 		info = item->util = xcalloc(sizeof(struct push_info), 1);
 		info->status = PUSH_STATUS_NOTQUERIED;
 		info->dest = xstrdup(item->string);
@@ -388,11 +388,11 @@ static int get_push_ref_states_noquery(struct ref_states *states)
 	for (i = 0; i < remote->push_refspec_nr; i++) {
 		struct refspec *spec = remote->push + i;
 		if (spec->matching)
-			item = string_list_append("(matching)", &states->push);
+			item = string_list_append(&states->push, "(matching)");
 		else if (strlen(spec->src))
-			item = string_list_append(spec->src, &states->push);
+			item = string_list_append(&states->push, spec->src);
 		else
-			item = string_list_append("(delete)", &states->push);
+			item = string_list_append(&states->push, "(delete)");
 
 		info = item->util = xcalloc(sizeof(struct push_info), 1);
 		info->forced = spec->force;
@@ -416,7 +416,7 @@ static int get_head_names(const struct ref *remote_refs, struct ref_states *stat
 	matches = guess_remote_head(find_ref_by_name(remote_refs, "HEAD"),
 				    fetch_map, 1);
 	for (ref = matches; ref; ref = ref->next)
-		string_list_append(abbrev_branch(ref->name), &states->heads);
+		string_list_append(&states->heads, abbrev_branch(ref->name));
 
 	free_refs(fetch_map);
 	free_refs(matches);
@@ -480,8 +480,8 @@ static int add_branch_for_removal(const char *refname,
 	if (prefixcmp(refname, "refs/remotes")) {
 		/* advise user how to delete local branches */
 		if (!prefixcmp(refname, "refs/heads/"))
-			string_list_append(abbrev_branch(refname),
-					   branches->skipped);
+			string_list_append(branches->skipped,
+					   abbrev_branch(refname));
 		/* silently skip over other non-remote refs */
 		return 0;
 	}
@@ -490,7 +490,7 @@ static int add_branch_for_removal(const char *refname,
 	if (flags & REF_ISSYMREF)
 		return unlink(git_path("%s", refname));
 
-	item = string_list_append(refname, branches->branches);
+	item = string_list_append(branches->branches, refname);
 	item->util = xmalloc(20);
 	hashcpy(item->util, sha1);
 
@@ -515,7 +515,7 @@ static int read_remote_branches(const char *refname,
 
 	strbuf_addf(&buf, "refs/remotes/%s", rename->old);
 	if (!prefixcmp(refname, buf.buf)) {
-		item = string_list_append(xstrdup(refname), rename->remote_branches);
+		item = string_list_append(rename->remote_branches, xstrdup(refname));
 		symref = resolve_ref(refname, orig_sha1, 1, &flag);
 		if (flag & REF_ISSYMREF)
 			item->util = xstrdup(symref);
@@ -798,7 +798,7 @@ static int append_ref_to_tracked_list(const char *refname,
 	memset(&refspec, 0, sizeof(refspec));
 	refspec.dst = (char *)refname;
 	if (!remote_find_tracking(states->remote, &refspec))
-		string_list_append(abbrev_branch(refspec.src), &states->tracked);
+		string_list_append(&states->tracked, abbrev_branch(refspec.src));
 
 	return 0;
 }
@@ -946,7 +946,7 @@ static int add_push_to_show_info(struct string_list_item *push_item, void *cb_da
 		show_info->width = n;
 	if ((n = strlen(push_info->dest)) > show_info->width2)
 		show_info->width2 = n;
-	item = string_list_append(push_item->string, show_info->list);
+	item = string_list_append(show_info->list, push_item->string);
 	item->util = push_item->util;
 	return 0;
 }
@@ -1360,10 +1360,10 @@ static int get_one_entry(struct remote *remote, void *priv)
 
 	if (remote->url_nr > 0) {
 		strbuf_addf(&url_buf, "%s (fetch)", remote->url[0]);
-		string_list_append(remote->name, list)->util =
+		string_list_append(list, remote->name)->util =
 				strbuf_detach(&url_buf, NULL);
 	} else
-		string_list_append(remote->name, list)->util = NULL;
+		string_list_append(list, remote->name)->util = NULL;
 	if (remote->pushurl_nr) {
 		url = remote->pushurl;
 		url_nr = remote->pushurl_nr;
@@ -1374,7 +1374,7 @@ static int get_one_entry(struct remote *remote, void *priv)
 	for (i = 0; i < url_nr; i++)
 	{
 		strbuf_addf(&url_buf, "%s (push)", url[i]);
-		string_list_append(remote->name, list)->util =
+		string_list_append(list, remote->name)->util =
 				strbuf_detach(&url_buf, NULL);
 	}
 
diff --git a/builtin/rerere.c b/builtin/rerere.c
index 34f9ace..73610b6 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -59,7 +59,7 @@ static void garbage_collect(struct string_list *rr)
 		cutoff = (has_rerere_resolution(e->d_name)
 			  ? cutoff_resolve : cutoff_noresolve);
 		if (then < now - cutoff * 86400)
-			string_list_append(e->d_name, &to_remove);
+			string_list_append(&to_remove, e->d_name);
 	}
 	for (i = 0; i < to_remove.nr; i++)
 		unlink_rr_item(to_remove.items[i].string);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 06320f5..e052c17 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -115,7 +115,7 @@ static void insert_one_record(struct shortlog *log,
 		}
 	}
 
-	string_list_append(buffer, item->util);
+	string_list_append(item->util, buffer);
 }
 
 static void read_from_stdin(struct shortlog *log)
diff --git a/notes.c b/notes.c
index e425e19..6556627 100644
--- a/notes.c
+++ b/notes.c
@@ -838,7 +838,7 @@ static int string_list_add_one_ref(const char *path, const unsigned char *sha1,
 {
 	struct string_list *refs = cb;
 	if (!unsorted_string_list_has_string(refs, path))
-		string_list_append(path, refs);
+		string_list_append(refs, path);
 	return 0;
 }
 
@@ -851,7 +851,7 @@ void string_list_add_refs_by_glob(struct string_list *list, const char *glob)
 		if (get_sha1(glob, sha1))
 			warning("notes ref %s is invalid", glob);
 		if (!unsorted_string_list_has_string(list, glob))
-			string_list_append(glob, list);
+			string_list_append(list, glob);
 	}
 }
 
@@ -983,7 +983,7 @@ void init_display_notes(struct display_notes_opt *opt)
 	assert(!display_notes_trees);
 
 	if (!opt || !opt->suppress_default_notes) {
-		string_list_append(default_notes_ref(), &display_notes_refs);
+		string_list_append(&display_notes_refs, default_notes_ref());
 		display_ref_env = getenv(GIT_NOTES_DISPLAY_REF_ENVIRONMENT);
 		if (display_ref_env) {
 			string_list_add_refs_from_colon_sep(&display_notes_refs,
diff --git a/remote.c b/remote.c
index c70181c..498bf38 100644
--- a/remote.c
+++ b/remote.c
@@ -1709,7 +1709,7 @@ struct ref *get_stale_heads(struct remote *remote, struct ref *fetch_map)
 	info.ref_names = &ref_names;
 	info.stale_refs_tail = &stale_refs;
 	for (ref = fetch_map; ref; ref = ref->next)
-		string_list_append(ref->name, &ref_names);
+		string_list_append(&ref_names, ref->name);
 	sort_string_list(&ref_names);
 	for_each_ref(get_stale_heads_cb, &info);
 	string_list_clear(&ref_names, 0);
diff --git a/revision.c b/revision.c
index f4b8b38..28f1c6d 100644
--- a/revision.c
+++ b/revision.c
@@ -1205,8 +1205,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		else
 			strbuf_addstr(&buf, "refs/notes/");
 		strbuf_addstr(&buf, arg+13);
-		string_list_append(strbuf_detach(&buf, NULL),
-				   revs->notes_opt.extra_notes_refs);
+		string_list_append(revs->notes_opt.extra_notes_refs,
+				   strbuf_detach(&buf, NULL));
 	} else if (!strcmp(arg, "--no-notes")) {
 		revs->show_notes = 0;
 		revs->show_notes_given = 1;
diff --git a/string-list.c b/string-list.c
index c9ad7fc..6d29187 100644
--- a/string-list.c
+++ b/string-list.c
@@ -148,7 +148,7 @@ void print_string_list(const char *text, const struct string_list *p)
 		printf("%s:%p\n", p->items[i].string, p->items[i].util);
 }
 
-struct string_list_item *string_list_append(const char *string, struct string_list *list)
+struct string_list_item *string_list_append(struct string_list *list, const char *string)
 {
 	ALLOC_GROW(list->items, list->nr + 1, list->alloc);
 	list->items[list->nr].string =
diff --git a/string-list.h b/string-list.h
index 63b69c8..814bbd3 100644
--- a/string-list.h
+++ b/string-list.h
@@ -35,7 +35,7 @@ struct string_list_item *string_list_insert_at_index(int insert_at,
 struct string_list_item *string_list_lookup(const char *string, struct string_list *list);
 
 /* Use these functions only on unsorted lists: */
-struct string_list_item *string_list_append(const char *string, struct string_list *list);
+struct string_list_item *string_list_append(struct string_list *list, const char *string);
 void sort_string_list(struct string_list *list);
 int unsorted_string_list_has_string(struct string_list *list, const char *string);
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
-- 
1.7.0.3.313.g87b3c

Re: [PATCH 8/7] Make string_list_append() consistent with everything else

[Julian Phillips]

On Wed, 31 Mar 2010 23:29:39 -0700, Stephen Boyd <bebarino@gmail.com>
wrote:
> string_list_append() is the only function in the string_list API which
> takes
> the struct string_list as the second argument instead of the first. Swap
> the
> ordering to make the string_list API consistent.

The only function?  It doesn't seem that way ...

jp3@rayne: git(pu)>grep '(' string-list.h | grep -v '(\(const \)*struct
string_list'
void print_string_list(const char *text, const struct string_list *p);
typedef void (*string_list_clear_func_t)(void *p, const char *str);
int for_each_string_list(string_list_each_func_t,
struct string_list_item *string_list_insert(const char *string, struct
string_list *list);
struct string_list_item *string_list_insert_at_index(int insert_at,
struct string_list_item *string_list_lookup(const char *string, struct
string_list *list);
struct string_list_item *string_list_append(const char *string, struct
string_list *list);

I don't think that swapping the argument order for append is wrong, but it
doesn't seem to be the only function that doesn't take the list as the
first argument ... or am I missing something? (probably am)

-- 
Julian