[PATCH 1/3] list-objects: add "void *data" parameter to show functions

[PATCH 1/3] list-objects: add "void *data" parameter to show functions

[Christian Couder]

The goal of this patch is to get rid of the "static struct rev_info
revs" static variable in "builtin-rev-list.c".

To do that, we need to pass the revs to the "show_commit" function
in "builtin-rev-list.c" and this in turn means that the
"traverse_commit_list" function in "list-objects.c" must be passed
functions pointers to functions with 2 parameters instead of one.

So we have to change all the callers and all the functions passed
to "traverse_commit_list".

Anyway this makes the code more clean and more generic, so it
should be a good thing in the long run.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin-pack-objects.c |    6 ++--
 builtin-rev-list.c     |   68 ++++++++++++++++++++++++-----------------------
 list-objects.c         |    9 +++---
 list-objects.h         |    6 ++--
 upload-pack.c          |    6 ++--
 5 files changed, 49 insertions(+), 46 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 50f8cc1..8c5eaba 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1901,13 +1901,13 @@ static void read_object_list_from_stdin(void)
 
 #define OBJECT_ADDED (1u<<20)
 
-static void show_commit(struct commit *commit)
+static void show_commit(struct commit *commit, void *data)
 {
 	add_object_entry(commit->object.sha1, OBJ_COMMIT, NULL, 0);
 	commit->object.flags |= OBJECT_ADDED;
 }
 
-static void show_object(struct object_array_entry *p)
+static void show_object(struct object_array_entry *p, void *data)
 {
 	add_preferred_base_object(p->name);
 	add_object_entry(p->item->sha1, p->item->type, p->name, 0);
@@ -2071,7 +2071,7 @@ static void get_object_list(int ac, const char **av)
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
 	mark_edges_uninteresting(revs.commits, &revs, show_edge);
-	traverse_commit_list(&revs, show_commit, show_object);
+	traverse_commit_list(&revs, show_commit, show_object, NULL);
 
 	if (keep_unreachable)
 		add_objects_in_unpacked_packs(&revs);
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index eb34147..cd6f6b8 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -42,72 +42,72 @@ static const char rev_list_usage[] =
 "    --bisect-all"
 ;
 
-static struct rev_info revs;
-
 static int show_timestamp;
 static int hdr_termination;
 static const char *header_prefix;
 
-static void finish_commit(struct commit *commit);
-static void show_commit(struct commit *commit)
+static void finish_commit(struct commit *commit, void *data);
+static void show_commit(struct commit *commit, void *data)
 {
-	graph_show_commit(revs.graph);
+	struct rev_info *revs = data;
+
+	graph_show_commit(revs->graph);
 
 	if (show_timestamp)
 		printf("%lu ", commit->date);
 	if (header_prefix)
 		fputs(header_prefix, stdout);
 
-	if (!revs.graph) {
+	if (!revs->graph) {
 		if (commit->object.flags & BOUNDARY)
 			putchar('-');
 		else if (commit->object.flags & UNINTERESTING)
 			putchar('^');
-		else if (revs.left_right) {
+		else if (revs->left_right) {
 			if (commit->object.flags & SYMMETRIC_LEFT)
 				putchar('<');
 			else
 				putchar('>');
 		}
 	}
-	if (revs.abbrev_commit && revs.abbrev)
-		fputs(find_unique_abbrev(commit->object.sha1, revs.abbrev),
+	if (revs->abbrev_commit && revs->abbrev)
+		fputs(find_unique_abbrev(commit->object.sha1, revs->abbrev),
 		      stdout);
 	else
 		fputs(sha1_to_hex(commit->object.sha1), stdout);
-	if (revs.print_parents) {
+	if (revs->print_parents) {
 		struct commit_list *parents = commit->parents;
 		while (parents) {
 			printf(" %s", sha1_to_hex(parents->item->object.sha1));
 			parents = parents->next;
 		}
 	}
-	if (revs.children.name) {
+	if (revs->children.name) {
 		struct commit_list *children;
 
-		children = lookup_decoration(&revs.children, &commit->object);
+		children = lookup_decoration(&revs->children, &commit->object);
 		while (children) {
 			printf(" %s", sha1_to_hex(children->item->object.sha1));
 			children = children->next;
 		}
 	}
-	show_decorations(&revs, commit);
-	if (revs.commit_format == CMIT_FMT_ONELINE)
+	show_decorations(revs, commit);
+	if (revs->commit_format == CMIT_FMT_ONELINE)
 		putchar(' ');
 	else
 		putchar('\n');
 
-	if (revs.verbose_header && commit->buffer) {
+	if (revs->verbose_header && commit->buffer) {
 		struct strbuf buf = STRBUF_INIT;
-		pretty_print_commit(revs.commit_format, commit,
-				    &buf, revs.abbrev, NULL, NULL,
-				    revs.date_mode, 0);
-		if (revs.graph) {
+		pretty_print_commit(revs->commit_format, commit,
+				    &buf, revs->abbrev, NULL, NULL,
+				    revs->date_mode, 0);
+		if (revs->graph) {
 			if (buf.len) {
-				if (revs.commit_format != CMIT_FMT_ONELINE)
-					graph_show_oneline(revs.graph);
+				if (revs->commit_format != CMIT_FMT_ONELINE)
+					graph_show_oneline(revs->graph);
 
-				graph_show_commit_msg(revs.graph, &buf);
+				graph_show_commit_msg(revs->graph, &buf);
 
 				/*
 				 * Add a newline after the commit message.
@@ -125,7 +125,7 @@ static void show_commit(struct commit *commit)
 				 * format doesn't explicitly end in a newline.)
 				 */
 				if (buf.len && buf.buf[buf.len - 1] == '\n')
-					graph_show_padding(revs.graph);
+					graph_show_padding(revs->graph);
 				putchar('\n');
 			} else {
 				/*
@@ -133,7 +133,7 @@ static void show_commit(struct commit *commit)
 				 * the rest of the graph output for this
 				 * commit.
 				 */
-				if (graph_show_remainder(revs.graph))
+				if (graph_show_remainder(revs->graph))
 					putchar('\n');
 			}
 		} else {
@@ -142,14 +142,14 @@ static void show_commit(struct commit *commit)
 		}
 		strbuf_release(&buf);
 	} else {
-		if (graph_show_remainder(revs.graph))
+		if (graph_show_remainder(revs->graph))
 			putchar('\n');
 	}
 	maybe_flush_or_die(stdout, "stdout");
-	finish_commit(commit);
+	finish_commit(commit, data);
 }
 
-static void finish_commit(struct commit *commit)
+static void finish_commit(struct commit *commit, void *data)
 {
 	if (commit->parents) {
 		free_commit_list(commit->parents);
@@ -159,20 +159,20 @@ static void finish_commit(struct commit *commit)
 	commit->buffer = NULL;
 }
 
-static void finish_object(struct object_array_entry *p)
+static void finish_object(struct object_array_entry *p, void *data)
 {
 	if (p->item->type == OBJ_BLOB && !has_sha1_file(p->item->sha1))
 		die("missing blob object '%s'", sha1_to_hex(p->item->sha1));
 }
 
-static void show_object(struct object_array_entry *p)
+static void show_object(struct object_array_entry *p, void *data)
 {
 	/* An object with name "foo\n0000000..." can be used to
 	 * confuse downstream "git pack-objects" very badly.
 	 */
 	const char *ep = strchr(p->name, '\n');
 
-	finish_object(p);
+	finish_object(p, data);
 	if (ep) {
 		printf("%s %.*s\n", sha1_to_hex(p->item->sha1),
 		       (int) (ep - p->name),
@@ -264,7 +264,7 @@ int show_bisect_vars(struct rev_info *revs, int reaches, int all, int flags)
 		strcpy(hex, sha1_to_hex(revs->commits->item->object.sha1));
 
 	if (flags & BISECT_SHOW_ALL) {
-		traverse_commit_list(revs, show_commit, show_object);
+		traverse_commit_list(revs, show_commit, show_object, revs);
 		printf("------\n");
 	}
 
@@ -297,6 +297,7 @@ int show_bisect_vars(struct rev_info *revs, int reaches, int all, int flags)
 
 int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
+	struct rev_info revs;
 	struct commit_list *list;
 	int i;
 	int read_from_stdin = 0;
@@ -391,8 +392,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	}
 
 	traverse_commit_list(&revs,
-		quiet ? finish_commit : show_commit,
-		quiet ? finish_object : show_object);
+			     quiet ? finish_commit : show_commit,
+			     quiet ? finish_object : show_object,
+			     &revs);
 
 	return 0;
 }
diff --git a/list-objects.c b/list-objects.c
index c8b8375..208a4cb 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -137,8 +137,9 @@ void mark_edges_uninteresting(struct commit_list *list,
 }
 
 void traverse_commit_list(struct rev_info *revs,
-			  void (*show_commit)(struct commit *),
-			  void (*show_object)(struct object_array_entry *))
+			  show_commit_fn show_commit,
+			  show_object_fn show_object,
+			  void *data)
 {
 	int i;
 	struct commit *commit;
@@ -146,7 +147,7 @@ void traverse_commit_list(struct rev_info *revs,
 
 	while ((commit = get_revision(revs)) != NULL) {
 		process_tree(revs, commit->tree, &objects, NULL, "");
-		show_commit(commit);
+		show_commit(commit, data);
 	}
 	for (i = 0; i < revs->pending.nr; i++) {
 		struct object_array_entry *pending = revs->pending.objects + i;
@@ -173,7 +174,7 @@ void traverse_commit_list(struct rev_info *revs,
 		    sha1_to_hex(obj->sha1), name);
 	}
 	for (i = 0; i < objects.nr; i++)
-		show_object(&objects.objects[i]);
+		show_object(&objects.objects[i], data);
 	free(objects.objects);
 	if (revs->pending.nr) {
 		free(revs->pending.objects);
diff --git a/list-objects.h b/list-objects.h
index 0f41391..47fae2e 100644
--- a/list-objects.h
+++ b/list-objects.h
@@ -1,11 +1,11 @@
 #ifndef LIST_OBJECTS_H
 #define LIST_OBJECTS_H
 
-typedef void (*show_commit_fn)(struct commit *);
-typedef void (*show_object_fn)(struct object_array_entry *);
+typedef void (*show_commit_fn)(struct commit *, void *);
+typedef void (*show_object_fn)(struct object_array_entry *, void *);
 typedef void (*show_edge_fn)(struct commit *);
 
-void traverse_commit_list(struct rev_info *revs, show_commit_fn, show_object_fn);
+void traverse_commit_list(struct rev_info *, show_commit_fn, show_object_fn, void *);
 
 void mark_edges_uninteresting(struct commit_list *, struct rev_info *, show_edge_fn);
 
diff --git a/upload-pack.c b/upload-pack.c
index 4d7357f..a28a0c4 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -66,7 +66,7 @@ static ssize_t send_client_data(int fd, const char *data, ssize_t sz)
 }
 
 static FILE *pack_pipe = NULL;
-static void show_commit(struct commit *commit)
+static void show_commit(struct commit *commit, void *data)
 {
 	if (commit->object.flags & BOUNDARY)
 		fputc('-', pack_pipe);
@@ -78,7 +78,7 @@ static void show_commit(struct commit *commit)
 	commit->buffer = NULL;
 }
 
-static void show_object(struct object_array_entry *p)
+static void show_object(struct object_array_entry *p, void *data)
 {
 	/* An object with name "foo\n0000000..." can be used to
 	 * confuse downstream git-pack-objects very badly.
@@ -134,7 +134,7 @@ static int do_rev_list(int fd, void *create_full_pack)
 	if (prepare_revision_walk(&revs))
 		die("revision walk setup failed");
 	mark_edges_uninteresting(revs.commits, &revs, show_edge);
-	traverse_commit_list(&revs, show_commit, show_object);
+	traverse_commit_list(&revs, show_commit, show_object, NULL);
 	fflush(pack_pipe);
 	fclose(pack_pipe);
 	return 0;
-- 
1.6.2.2.537.g3b83b

[PATCH 2/3] rev-list: remove last static vars used in "show_commit"

[Christian Couder]

This patch removes the last static variables that were used in
the "show_commit" function.

To do that, we create a new "rev_list_info" struct that we will pass
in the "void *data" argument to "show_commit".

This means that we have to change the first argument to
"show_bisect_vars" too.

While at it, we also remove a "struct commit_list *list" variable
in "cmd_rev_list" that is not really needed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 bisect.c           |    6 +++++-
 bisect.h           |   14 ++++++++------
 builtin-rev-list.c |   42 +++++++++++++++++++++---------------------
 3 files changed, 34 insertions(+), 28 deletions(-)

diff --git a/bisect.c b/bisect.c
index 69f8860..4d2a150 100644
--- a/bisect.c
+++ b/bisect.c
@@ -535,8 +535,12 @@ static void bisect_rev_setup(struct rev_info *revs, const char *prefix)
 int bisect_next_vars(const char *prefix)
 {
 	struct rev_info revs;
+	struct rev_list_info info;
 	int reaches = 0, all = 0;
 
+	memset(&info, 0, sizeof(info));
+	info.revs = &revs;
+
 	bisect_rev_setup(&revs, prefix);
 
 	if (prepare_revision_walk(&revs))
@@ -547,6 +551,6 @@ int bisect_next_vars(const char *prefix)
 	revs.commits = find_bisection(revs.commits, &reaches, &all,
 				      !!skipped_sha1_nr);
 
-	return show_bisect_vars(&revs, reaches, all,
+	return show_bisect_vars(&info, reaches, all,
 				BISECT_SHOW_TRIED | BISECT_SHOW_STRINGED);
 }
diff --git a/bisect.h b/bisect.h
index f5d1067..b1c334d 100644
--- a/bisect.h
+++ b/bisect.h
@@ -14,12 +14,14 @@ extern struct commit_list *filter_skipped(struct commit_list *list,
 #define BISECT_SHOW_TRIED	(1<<1)
 #define BISECT_SHOW_STRINGED	(1<<2)
 
-/*
- * The flag BISECT_SHOW_ALL should not be set if this function is called
- * from outside "builtin-rev-list.c" as otherwise it would use
- * static "revs" from this file.
- */
-extern int show_bisect_vars(struct rev_info *revs, int reaches, int all,
+struct rev_list_info {
+	struct rev_info *revs;
+	int show_timestamp;
+	int hdr_termination;
+	const char *header_prefix;
+};
+
+extern int show_bisect_vars(struct rev_list_info *info, int reaches, int all,
 			    int flags);
 
 extern int bisect_next_vars(const char *prefix);
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index cd6f6b8..244b73e 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -42,21 +42,18 @@ static const char rev_list_usage[] =
 "    --bisect-all"
 ;
 
-static int show_timestamp;
-static int hdr_termination;
-static const char *header_prefix;
-
 static void finish_commit(struct commit *commit, void *data);
 static void show_commit(struct commit *commit, void *data)
 {
-	struct rev_info *revs = data;
+	struct rev_list_info *info = data;
+	struct rev_info *revs = info->revs;
 
 	graph_show_commit(revs->graph);
 
-	if (show_timestamp)
+	if (info->show_timestamp)
 		printf("%lu ", commit->date);
-	if (header_prefix)
-		fputs(header_prefix, stdout);
+	if (info->header_prefix)
+		fputs(info->header_prefix, stdout);
 
 	if (!revs->graph) {
 		if (commit->object.flags & BOUNDARY)
@@ -138,7 +135,7 @@ static void show_commit(struct commit *commit, void *data)
 			}
 		} else {
 			if (buf.len)
-				printf("%s%c", buf.buf, hdr_termination);
+				printf("%s%c", buf.buf, info->hdr_termination);
 		}
 		strbuf_release(&buf);
 	} else {
@@ -236,11 +233,13 @@ static void show_tried_revs(struct commit_list *tried, int stringed)
 	printf(stringed ? "' &&\n" : "'\n");
 }
 
-int show_bisect_vars(struct rev_info *revs, int reaches, int all, int flags)
+int show_bisect_vars(struct rev_list_info *info, int reaches, int all,
+		     int flags)
 {
 	int cnt;
 	char hex[41] = "", *format;
 	struct commit_list *tried;
+	struct rev_info *revs = info->revs;
 
 	if (!revs->commits && !(flags & BISECT_SHOW_TRIED))
 		return 1;
@@ -264,7 +263,7 @@ int show_bisect_vars(struct rev_info *revs, int reaches, int all, int flags)
 		strcpy(hex, sha1_to_hex(revs->commits->item->object.sha1));
 
 	if (flags & BISECT_SHOW_ALL) {
-		traverse_commit_list(revs, show_commit, show_object, revs);
+		traverse_commit_list(revs, show_commit, show_object, info);
 		printf("------\n");
 	}
 
@@ -298,7 +297,7 @@ int show_bisect_vars(struct rev_info *revs, int reaches, int all, int flags)
 int cmd_rev_list(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info revs;
-	struct commit_list *list;
+	struct rev_list_info info;
 	int i;
 	int read_from_stdin = 0;
 	int bisect_list = 0;
@@ -313,6 +312,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	revs.commit_format = CMIT_FMT_UNSPECIFIED;
 	argc = setup_revisions(argc, argv, &revs, NULL);
 
+	memset(&info, 0, sizeof(info));
+	info.revs = &revs;
+
 	quiet = DIFF_OPT_TST(&revs.diffopt, QUIET);
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
@@ -322,7 +324,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 			continue;
 		}
 		if (!strcmp(arg, "--timestamp")) {
-			show_timestamp = 1;
+			info.show_timestamp = 1;
 			continue;
 		}
 		if (!strcmp(arg, "--bisect")) {
@@ -352,19 +354,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	}
 	if (revs.commit_format != CMIT_FMT_UNSPECIFIED) {
 		/* The command line has a --pretty  */
-		hdr_termination = '\n';
+		info.hdr_termination = '\n';
 		if (revs.commit_format == CMIT_FMT_ONELINE)
-			header_prefix = "";
+			info.header_prefix = "";
 		else
-			header_prefix = "commit ";
+			info.header_prefix = "commit ";
 	}
 	else if (revs.verbose_header)
 		/* Only --header was specified */
 		revs.commit_format = CMIT_FMT_RAW;
 
-	list = revs.commits;
-
-	if ((!list &&
+	if ((!revs.commits &&
 	     (!(revs.tag_objects||revs.tree_objects||revs.blob_objects) &&
 	      !revs.pending.nr)) ||
 	    revs.diff)
@@ -387,14 +387,14 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 					      bisect_find_all);
 
 		if (bisect_show_vars)
-			return show_bisect_vars(&revs, reaches, all,
+			return show_bisect_vars(&info, reaches, all,
 						bisect_show_all ? BISECT_SHOW_ALL : 0);
 	}
 
 	traverse_commit_list(&revs,
 			     quiet ? finish_commit : show_commit,
 			     quiet ? finish_object : show_object,
-			     &revs);
+			     &info);
 
 	return 0;
 }
-- 
1.6.2.2.537.g3b83b

[PATCH 3/3] rev-list: add "int bisect_show_flags" in "struct rev_list_info"

[Christian Couder]

This is a cleanup patch to make it easier to use the
"show_bisect_vars" function and take advantage of the rev_list_info
struct.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 bisect.c           |    4 ++--
 bisect.h           |    6 +++---
 builtin-rev-list.c |   11 ++++-------
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/bisect.c b/bisect.c
index 4d2a150..58f7e6f 100644
--- a/bisect.c
+++ b/bisect.c
@@ -540,6 +540,7 @@ int bisect_next_vars(const char *prefix)
 
 	memset(&info, 0, sizeof(info));
 	info.revs = &revs;
+	info.bisect_show_flags = BISECT_SHOW_TRIED | BISECT_SHOW_STRINGED;
 
 	bisect_rev_setup(&revs, prefix);
 
@@ -551,6 +552,5 @@ int bisect_next_vars(const char *prefix)
 	revs.commits = find_bisection(revs.commits, &reaches, &all,
 				      !!skipped_sha1_nr);
 
-	return show_bisect_vars(&info, reaches, all,
-				BISECT_SHOW_TRIED | BISECT_SHOW_STRINGED);
+	return show_bisect_vars(&info, reaches, all);
 }
diff --git a/bisect.h b/bisect.h
index b1c334d..fdba913 100644
--- a/bisect.h
+++ b/bisect.h
@@ -9,20 +9,20 @@ extern struct commit_list *filter_skipped(struct commit_list *list,
 					  struct commit_list **tried,
 					  int show_all);
 
-/* show_bisect_vars flags */
+/* bisect_show_flags flags in struct rev_list_info */
 #define BISECT_SHOW_ALL		(1<<0)
 #define BISECT_SHOW_TRIED	(1<<1)
 #define BISECT_SHOW_STRINGED	(1<<2)
 
 struct rev_list_info {
 	struct rev_info *revs;
+	int bisect_show_flags;
 	int show_timestamp;
 	int hdr_termination;
 	const char *header_prefix;
 };
 
-extern int show_bisect_vars(struct rev_list_info *info, int reaches, int all,
-			    int flags);
+extern int show_bisect_vars(struct rev_list_info *info, int reaches, int all);
 
 extern int bisect_next_vars(const char *prefix);
 
diff --git a/builtin-rev-list.c b/builtin-rev-list.c
index 244b73e..193993c 100644
--- a/builtin-rev-list.c
+++ b/builtin-rev-list.c
@@ -233,10 +233,9 @@ static void show_tried_revs(struct commit_list *tried, int stringed)
 	printf(stringed ? "' &&\n" : "'\n");
 }
 
-int show_bisect_vars(struct rev_list_info *info, int reaches, int all,
-		     int flags)
+int show_bisect_vars(struct rev_list_info *info, int reaches, int all)
 {
-	int cnt;
+	int cnt, flags = info->bisect_show_flags;
 	char hex[41] = "", *format;
 	struct commit_list *tried;
 	struct rev_info *revs = info->revs;
@@ -303,7 +302,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	int bisect_list = 0;
 	int bisect_show_vars = 0;
 	int bisect_find_all = 0;
-	int bisect_show_all = 0;
 	int quiet = 0;
 
 	git_config(git_default_config, NULL);
@@ -334,7 +332,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--bisect-all")) {
 			bisect_list = 1;
 			bisect_find_all = 1;
-			bisect_show_all = 1;
+			info.bisect_show_flags = BISECT_SHOW_ALL;
 			revs.show_decorations = 1;
 			continue;
 		}
@@ -387,8 +385,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 					      bisect_find_all);
 
 		if (bisect_show_vars)
-			return show_bisect_vars(&info, reaches, all,
-						bisect_show_all ? BISECT_SHOW_ALL : 0);
+			return show_bisect_vars(&info, reaches, all);
 	}
 
 	traverse_commit_list(&revs,
-- 
1.6.2.2.537.g3b83b

Re: [PATCH 1/3] list-objects: add "void *data" parameter to show functions

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> The goal of this patch is to get rid of the "static struct rev_info
> revs" static variable in "builtin-rev-list.c".

Hmm.  If it were a more library-ish file, a removal of such a static
variable might help you to make more than one calls to a library function,
but does it matter in files like builtin-rev-list.c?  Its cmd_rev_list()
is like main() --- it is meant to run once and exit.

So if it is the only goal of this series, I am inclined to say that I do
not have a reason to look at the rest of the series, but as a side effect
does this removal make some other API better?  Perhaps a more library-ish
function is in builtin-rev-list.c and this structure should really needs
to be passed around as a parameter, but I cannot tell solely by reading
the goal above, without reading the patches themselves.

> Anyway this makes the code more clean and more generic, so it
> should be a good thing in the long run.

I wouldn't disagree with that "long run" thing, but the answer to the
above question affects the placement of this series in my prioritized
queue.

Re: [PATCH 1/3] list-objects: add "void *data" parameter to show functions

[Christian Couder]

Le mercredi 8 avril 2009, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > The goal of this patch is to get rid of the "static struct rev_info
> > revs" static variable in "builtin-rev-list.c".
>
> Hmm.  If it were a more library-ish file, a removal of such a static
> variable might help you to make more than one calls to a library
> function, but does it matter in files like builtin-rev-list.c?  Its
> cmd_rev_list() is like main() --- it is meant to run once and exit.
>
> So if it is the only goal of this series, I am inclined to say that I do
> not have a reason to look at the rest of the series, but as a side effect
> does this removal make some other API better?  Perhaps a more library-ish
> function is in builtin-rev-list.c and this structure should really needs
> to be passed around as a parameter, but I cannot tell solely by reading
> the goal above, without reading the patches themselves.

In the cover letter, I wrote that the patch series removes restrictions on 
using the "show_bisect_vars" function.

In fact there was a restriction on the use of the BISECT_SHOW_ALL flag 
because that would use the "show_commit" function that was using static 
variables.

The restriction was described in a comment in "bisect.h" and this comment is 
removed by the series. 

This is the relevant hunk in patch 2/3:

diff --git a/bisect.h b/bisect.h
index f5d1067..b1c334d 100644
--- a/bisect.h
+++ b/bisect.h
@@ -14,12 +14,14 @@ extern struct commit_list *filter_skipped(struct 
commit_list *list,
 #define BISECT_SHOW_TRIED      (1<<1)
 #define BISECT_SHOW_STRINGED   (1<<2)
 
-/*
- * The flag BISECT_SHOW_ALL should not be set if this function is called
- * from outside "builtin-rev-list.c" as otherwise it would use
- * static "revs" from this file.
- */
-extern int show_bisect_vars(struct rev_info *revs, int reaches, int all,
+struct rev_list_info {
+       struct rev_info *revs;
+       int show_timestamp;
+       int hdr_termination;
+       const char *header_prefix;
+};
+
+extern int show_bisect_vars(struct rev_list_info *info, int reaches, int 
all,
                            int flags);
 
 extern int bisect_next_vars(const char *prefix);


Best regards,
Christian.

> > Anyway this makes the code more clean and more generic, so it
> > should be a good thing in the long run.
>
> I wouldn't disagree with that "long run" thing, but the answer to the
> above question affects the placement of this series in my prioritized
> queue.

Re: [PATCH 1/3] list-objects: add "void *data" parameter to show functions

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> This is the relevant hunk in patch 2/3:
>
> diff --git a/bisect.h b/bisect.h
> index f5d1067..b1c334d 100644
> --- a/bisect.h
> +++ b/bisect.h
> @@ -14,12 +14,14 @@ extern struct commit_list *filter_skipped(struct 
> commit_list *list,
>  #define BISECT_SHOW_TRIED      (1<<1)
>  #define BISECT_SHOW_STRINGED   (1<<2)
>  
> -/*
> - * The flag BISECT_SHOW_ALL should not be set if this function is called
> - * from outside "builtin-rev-list.c" as otherwise it would use
> - * static "revs" from this file.
> - */

Thanks for a clarification.

Re: [PATCH 2/3] rev-list: remove last static vars used in "show_commit"

[Paolo Bonzini]

>  	struct rev_info revs;
> +	struct rev_list_info info;
>  	int reaches = 0, all = 0;
>  
> +	memset(&info, 0, sizeof(info));
> +	info.revs = &revs;

Would it make sense to embed the struct rev_info entirely in the new
struct, without going through a pointer?

Paolo

Re: [PATCH 2/3] rev-list: remove last static vars used in "show_commit"

[Christian Couder]

Le mercredi 8 avril 2009, Paolo Bonzini a écrit :
> >  	struct rev_info revs;
> > +	struct rev_list_info info;
> >  	int reaches = 0, all = 0;
> >
> > +	memset(&info, 0, sizeof(info));
> > +	info.revs = &revs;
>
> Would it make sense to embed the struct rev_info entirely in the new
> struct, without going through a pointer?

Perhaps, I will have a look.

The downside is that the struct rev_info may be initialized twice if we 
still use "memset(&info, 0, sizeof(info))" as it is also initialized 
in "init_revisions".

Thanks,
Christian.