[PATCH 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

[PATCH 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

Converts the bundle.c code to use the string-list.c API, getting rid
of some duplication in the codebase, while doing that stop the bundle
command-line tool and its API from leaking memory in some common
cases.

Ævar Arnfjörð Bjarmason (3):
  bundle cmd: stop leaking memory from parse_options_cmd_bundle()
  bundle.c: use a temporary variable for OIDs and names
  bundle: remove "ref_list" in favor of string-list.c API

 builtin/bundle.c | 91 ++++++++++++++++++++++++++++++++----------------
 bundle.c         | 72 +++++++++++++++++++++-----------------
 bundle.h         | 20 +++++------
 transport.c      | 11 ++++--
 4 files changed, 119 insertions(+), 75 deletions(-)

-- 
2.32.0.571.gdba276db2c

[PATCH 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).

As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 79 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 24 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index ea6948110b..7778297277 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,
 		const char* prefix,
 		const char * const usagestr[],
 		const struct option options[],
-		const char **bundle_file) {
+		char **bundle_file) {
 	int newargc;
 	newargc = parse_options(argc, argv, NULL, options, usagestr,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,8 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	int progress = isatty(STDERR_FILENO);
 	struct strvec pack_opts;
 	int version = -1;
-
+	int die_no_repo = 0;
+	int ret;
 	struct option options[] = {
 		OPT_SET_INT('q', "quiet", &progress,
 			    N_("do not show progress meter"), 0),
@@ -76,7 +77,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 			    N_("specify bundle format version")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
@@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	if (progress && all_progress_implied)
 		strvec_push(&pack_opts, "--all-progress-implied");
 
-	if (!startup_info->have_repository)
+	if (!startup_info->have_repository) {
+		die_no_repo = 1;
+		goto cleanup;
+	}
+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+cleanup:
+	free(bundle_file);
+	if (die_no_repo)
 		die(_("Need a repository to create a bundle."));
-	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	return ret;
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
 	int quiet = 0;
-
+	int ret;
 	struct option options[] = {
 		OPT_BOOL('q', "quiet", &quiet,
 			    N_("do not show bundle details")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	if (verify_bundle(the_repository, &header, !quiet))
-		return 1;
+	if (verify_bundle(the_repository, &header, !quiet)) {
+		ret = 1;
+		goto cleanup;
+	}
+
 	fprintf(stderr, _("%s is okay\n"), bundle_file);
-	return 0;
+	ret = 0;
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	return !!list_bundle_refs(&header, argc, argv);
+	ret = !!list_bundle_refs(&header, argc, argv);
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int die_no_repo = 0;
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
-	if (!startup_info->have_repository)
-		die(_("Need a repository to unbundle."));
-	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
+	if (!startup_info->have_repository) {
+		die_no_repo = 1;
+		goto cleanup;
+	}
+	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+cleanup:
+	if (die_no_repo)
+		die(_("Need a repository to unbundle."));
+	free(bundle_file);
+	return ret;
 }
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
-- 
2.32.0.571.gdba276db2c

[PATCH 2/3] bundle.c: use a temporary variable for OIDs and names

[Ævar Arnfjörð Bjarmason]

In preparation for moving away from accessing the OID and name via the
"oid" and "name" slots in a subsequent commit, change the code that
accesses it to use named variables. This makes the subsequent change
smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bundle.c    | 22 +++++++++++++++-------
 transport.c |  3 ++-
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/bundle.c b/bundle.c
index 693d619551..621708f40e 100644
--- a/bundle.c
+++ b/bundle.c
@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 	int i;
 
 	for (i = 0; i < r->nr; i++) {
+		struct object_id *oid;
+		const char *name;
+
 		if (argc > 1) {
 			int j;
 			for (j = 1; j < argc; j++)
@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 			if (j == argc)
 				continue;
 		}
-		printf("%s %s\n", oid_to_hex(&r->list[i].oid),
-				r->list[i].name);
+
+		oid = &r->list[i].oid;
+		name = r->list[i].name;
+		printf("%s %s\n", oid_to_hex(oid), name);
 	}
 	return 0;
 }
@@ -194,7 +199,8 @@ int verify_bundle(struct repository *r,
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		struct object *o = parse_object(r, &e->oid);
+		struct object_id *oid = &e->oid;
+		struct object *o = parse_object(r, oid);
 		if (o) {
 			o->flags |= PREREQ_MARK;
 			add_pending_object(&revs, o, e->name);
@@ -202,7 +208,7 @@ int verify_bundle(struct repository *r,
 		}
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(&e->oid), e->name);
+		error("%s %s", oid_to_hex(oid), e->name);
 	}
 	if (revs.pending.nr != p->nr)
 		return ret;
@@ -219,19 +225,21 @@ int verify_bundle(struct repository *r,
 
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		struct object *o = parse_object(r, &e->oid);
+		struct object_id *oid = &e->oid;
+		struct object *o = parse_object(r, oid);
 		assert(o); /* otherwise we'd have returned early */
 		if (o->flags & SHOWN)
 			continue;
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(&e->oid), e->name);
+		error("%s %s", oid_to_hex(oid), e->name);
 	}
 
 	/* Clean up objects used, as they will be reused. */
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		commit = lookup_commit_reference_gently(r, &e->oid, 1);
+		struct object_id *oid = &e->oid;
+		commit = lookup_commit_reference_gently(r, oid, 1);
 		if (commit)
 			clear_commit_marks(commit, ALL_REV_FLAGS);
 	}
diff --git a/transport.c b/transport.c
index 50f5830eb6..9d601c8c95 100644
--- a/transport.c
+++ b/transport.c
@@ -149,7 +149,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 	for (i = 0; i < data->header.references.nr; i++) {
 		struct ref_list_entry *e = data->header.references.list + i;
 		struct ref *ref = alloc_ref(e->name);
-		oidcpy(&ref->old_oid, &e->oid);
+		struct object_id *oid = &e->oid;
+		oidcpy(&ref->old_oid, oid);
 		ref->next = result;
 		result = ref;
 	}
-- 
2.32.0.571.gdba276db2c

[PATCH 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.

That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)

We need to change "name" to "string" and "oid" to "util" to make this
conversion, but other than that the APIs are pretty much identical for
what bundle.c made use of.

Let's also replace the memset(..,0,...) pattern with a more idiomatic
"INIT" macro, and finally add a *_release() function so to free the
allocated memory.

Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 12 ++++-----
 bundle.c         | 64 ++++++++++++++++++++++++------------------------
 bundle.h         | 20 +++++++--------
 transport.c      | 10 +++++---
 4 files changed, 55 insertions(+), 51 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 7778297277..8d82255280 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -106,7 +106,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int quiet = 0;
 	int ret;
@@ -121,7 +121,6 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -136,11 +135,12 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	ret = 0;
 cleanup:
 	free(bundle_file);
+	bundle_header_release(&header);
 	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
 	struct option options[] = {
@@ -152,7 +152,6 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -161,11 +160,12 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 	ret = !!list_bundle_refs(&header, argc, argv);
 cleanup:
 	free(bundle_file);
+	bundle_header_release(&header);
 	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int die_no_repo = 0;
 	int ret;
@@ -178,7 +178,6 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -189,6 +188,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+	bundle_header_release(&header);
 cleanup:
 	if (die_no_repo)
 		die(_("Need a repository to unbundle."));
diff --git a/bundle.c b/bundle.c
index 621708f40e..d36eeee1a5 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,15 +23,6 @@ static struct {
 	{ 3, v3_bundle_signature },
 };
 
-static void add_to_ref_list(const struct object_id *oid, const char *name,
-		struct ref_list *list)
-{
-	ALLOC_GROW(list->list, list->nr + 1, list->alloc);
-	oidcpy(&list->list[list->nr].oid, oid);
-	list->list[list->nr].name = xstrdup(name);
-	list->nr++;
-}
-
 static int parse_capability(struct bundle_header *header, const char *capability)
 {
 	const char *arg;
@@ -79,7 +70,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	/* The bundle header ends with an empty line */
 	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
 	       buf.len && buf.buf[0] != '\n') {
-		struct object_id oid;
+		struct object_id *oid;
 		int is_prereq = 0;
 		const char *p;
 
@@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 		 * Prerequisites have object name that is optionally
 		 * followed by SP and subject line.
 		 */
-		if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||
+		oid = xmalloc(sizeof(struct object_id));
+		if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||
 		    (*p && !isspace(*p)) ||
 		    (!is_prereq && !*p)) {
 			if (report_path)
 				error(_("unrecognized header: %s%s (%d)"),
 				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
 			status = -1;
+			free(oid);
 			break;
 		} else {
-			if (is_prereq)
-				add_to_ref_list(&oid, "", &header->prerequisites);
-			else
-				add_to_ref_list(&oid, p + 1, &header->references);
+			const char *string = is_prereq ? "" : p + 1;
+			struct string_list *list = is_prereq
+				? &header->prerequisites
+				: &header->references;
+			string_list_append(list, string)->util = oid;
 		}
 	}
 
@@ -139,19 +133,19 @@ int read_bundle_header(const char *path, struct bundle_header *header)
 
 int is_bundle(const char *path, int quiet)
 {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int fd = open(path, O_RDONLY);
 
 	if (fd < 0)
 		return 0;
-	memset(&header, 0, sizeof(header));
 	fd = parse_bundle_header(fd, &header, quiet ? NULL : path);
 	if (fd >= 0)
 		close(fd);
+	bundle_header_release(&header);
 	return (fd >= 0);
 }
 
-static int list_refs(struct ref_list *r, int argc, const char **argv)
+static int list_refs(struct string_list *r, int argc, const char **argv)
 {
 	int i;
 
@@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 		if (argc > 1) {
 			int j;
 			for (j = 1; j < argc; j++)
-				if (!strcmp(r->list[i].name, argv[j]))
+				if (!strcmp(r->items[i].string, argv[j]))
 					break;
 			if (j == argc)
 				continue;
 		}
 
-		oid = &r->list[i].oid;
-		name = r->list[i].name;
+		oid = r->items[i].util;
+		name = r->items[i].string;
 		printf("%s %s\n", oid_to_hex(oid), name);
 	}
 	return 0;
@@ -186,7 +180,7 @@ int verify_bundle(struct repository *r,
 	 * Do fast check, then if any prereqs are missing then go line by line
 	 * to be verbose about the errors
 	 */
-	struct ref_list *p = &header->prerequisites;
+	struct string_list *p = &header->prerequisites;
 	struct rev_info revs;
 	const char *argv[] = {NULL, "--all", NULL};
 	struct commit *commit;
@@ -198,17 +192,17 @@ int verify_bundle(struct repository *r,
 
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		struct object_id *oid = e->util;
 		struct object *o = parse_object(r, oid);
 		if (o) {
 			o->flags |= PREREQ_MARK;
-			add_pending_object(&revs, o, e->name);
+			add_pending_object(&revs, o, e->string);
 			continue;
 		}
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(oid), e->name);
+		error("%s %s", oid_to_hex(oid), e->string);
 	}
 	if (revs.pending.nr != p->nr)
 		return ret;
@@ -224,28 +218,28 @@ int verify_bundle(struct repository *r,
 			i--;
 
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		const struct object_id *oid = e->util;
 		struct object *o = parse_object(r, oid);
 		assert(o); /* otherwise we'd have returned early */
 		if (o->flags & SHOWN)
 			continue;
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(oid), e->name);
+		error("%s %s", oid_to_hex(oid), e->string);
 	}
 
 	/* Clean up objects used, as they will be reused. */
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		struct object_id *oid = e->util;
 		commit = lookup_commit_reference_gently(r, oid, 1);
 		if (commit)
 			clear_commit_marks(commit, ALL_REV_FLAGS);
 	}
 
 	if (verbose) {
-		struct ref_list *r;
+		struct string_list *r;
 
 		r = &header->references;
 		printf_ln(Q_("The bundle contains this ref:",
@@ -582,3 +576,9 @@ int unbundle(struct repository *r, struct bundle_header *header,
 		return error(_("index-pack died"));
 	return 0;
 }
+
+void bundle_header_release(struct bundle_header *header)
+{
+	string_list_clear(&header->prerequisites, 1);
+	string_list_clear(&header->references, 1);
+}
diff --git a/bundle.h b/bundle.h
index f9e2d1c8ef..f98c1d24d9 100644
--- a/bundle.h
+++ b/bundle.h
@@ -3,22 +3,21 @@
 
 #include "strvec.h"
 #include "cache.h"
-
-struct ref_list {
-	unsigned int nr, alloc;
-	struct ref_list_entry {
-		struct object_id oid;
-		char *name;
-	} *list;
-};
+#include "string-list.h"
 
 struct bundle_header {
 	unsigned version;
-	struct ref_list prerequisites;
-	struct ref_list references;
+	struct string_list prerequisites;
+	struct string_list references;
 	const struct git_hash_algo *hash_algo;
 };
 
+#define BUNDLE_HEADER_INIT \
+{ \
+	.prerequisites = STRING_LIST_INIT_DUP, \
+	.references = STRING_LIST_INIT_DUP, \
+}
+
 int is_bundle(const char *path, int quiet);
 int read_bundle_header(const char *path, struct bundle_header *header);
 int create_bundle(struct repository *r, const char *path,
@@ -30,5 +29,6 @@ int unbundle(struct repository *r, struct bundle_header *header,
 	     int bundle_fd, int flags);
 int list_bundle_refs(struct bundle_header *header,
 		int argc, const char **argv);
+void bundle_header_release(struct bundle_header *header);
 
 #endif
diff --git a/transport.c b/transport.c
index 9d601c8c95..667c4e0cc6 100644
--- a/transport.c
+++ b/transport.c
@@ -147,13 +147,14 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 	transport->hash_algo = data->header.hash_algo;
 
 	for (i = 0; i < data->header.references.nr; i++) {
-		struct ref_list_entry *e = data->header.references.list + i;
-		struct ref *ref = alloc_ref(e->name);
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = data->header.references.items + i;
+		struct ref *ref = alloc_ref(e->string);
+		const struct object_id *oid = e->util;
 		oidcpy(&ref->old_oid, oid);
 		ref->next = result;
 		result = ref;
 	}
+	string_list_clear(&data->header.references, 1);
 	return result;
 }
 
@@ -176,6 +177,7 @@ static int close_bundle(struct transport *transport)
 	struct bundle_transport_data *data = transport->data;
 	if (data->fd > 0)
 		close(data->fd);
+	bundle_header_release(&data->header);
 	free(data);
 	return 0;
 }
@@ -1082,6 +1084,8 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		die(_("git-over-rsync is no longer supported"));
 	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
+		string_list_init(&data->header.prerequisites, 1);
+		string_list_init(&data->header.references, 1);
 		transport_check_allowed("file");
 		ret->data = data;
 		ret->vtable = &bundle_vtable;
-- 
2.32.0.571.gdba276db2c

Re: [PATCH 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Andrei Rybak]

On 17/06/2021 13:21, Ævar Arnfjörð Bjarmason wrote:
> ---
>   builtin/bundle.c | 12 ++++-----
>   bundle.c         | 64 ++++++++++++++++++++++++------------------------
>   bundle.h         | 20 +++++++--------
>   transport.c      | 10 +++++---
>   4 files changed, 55 insertions(+), 51 deletions(-)
> 

[snip]

> diff --git a/bundle.c b/bundle.c
> index 621708f40e..d36eeee1a5 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
>   		if (argc > 1) {
>   			int j;
>   			for (j = 1; j < argc; j++)
> -				if (!strcmp(r->list[i].name, argv[j]))
> +				if (!strcmp(r->items[i].string, argv[j]))
>   					break;
>   			if (j == argc)
>   				continue;
>   		}
>   
> -		oid = &r->list[i].oid;
> -		name = r->list[i].name;
> +		oid = r->items[i].util;
> +		name = r->items[i].string;
>   		printf("%s %s\n", oid_to_hex(oid), name);

In function `list_refs` variable `name` that is used in a call to printf
has been extracted by the previous patch.

>   	}
>   	return 0;
> @@ -186,7 +180,7 @@ int verify_bundle(struct repository *r,
>   	 * Do fast check, then if any prereqs are missing then go line by line
>   	 * to be verbose about the errors
>   	 */
> -	struct ref_list *p = &header->prerequisites;
> +	struct string_list *p = &header->prerequisites;
>   	struct rev_info revs;
>   	const char *argv[] = {NULL, "--all", NULL};
>   	struct commit *commit;
> @@ -198,17 +192,17 @@ int verify_bundle(struct repository *r,
>   
>   	repo_init_revisions(r, &revs, NULL);
>   	for (i = 0; i < p->nr; i++) {
> -		struct ref_list_entry *e = p->list + i;
> -		struct object_id *oid = &e->oid;
> +		struct string_list_item *e = p->items + i;
> +		struct object_id *oid = e->util;
>   		struct object *o = parse_object(r, oid);
>   		if (o) {
>   			o->flags |= PREREQ_MARK;
> -			add_pending_object(&revs, o, e->name);
> +			add_pending_object(&revs, o, e->string);
>   			continue;
>   		}
>   		if (++ret == 1)
>   			error("%s", message);
> -		error("%s %s", oid_to_hex(oid), e->name);
> +		error("%s %s", oid_to_hex(oid), e->string);
>   	}
>   	if (revs.pending.nr != p->nr)
>   		return ret;
> @@ -224,28 +218,28 @@ int verify_bundle(struct repository *r,
>   			i--;
>   
>   	for (i = 0; i < p->nr; i++) {
> -		struct ref_list_entry *e = p->list + i;
> -		struct object_id *oid = &e->oid;
> +		struct string_list_item *e = p->items + i;
> +		const struct object_id *oid = e->util;
>   		struct object *o = parse_object(r, oid);
>   		assert(o); /* otherwise we'd have returned early */
>   		if (o->flags & SHOWN)
>   			continue;
>   		if (++ret == 1)
>   			error("%s", message);
> -		error("%s %s", oid_to_hex(oid), e->name);
> +		error("%s %s", oid_to_hex(oid), e->string);

However, `e->name` in three places in function `verify_bundle` for two
different instances of `struct ref_list_entry *` wasn't extracted into
a variable by the previous patch. Could you please clarify this
discrepancy?

[snip]

> diff --git a/transport.c b/transport.c
> index 9d601c8c95..667c4e0cc6 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -147,13 +147,14 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
>   	transport->hash_algo = data->header.hash_algo;
>   
>   	for (i = 0; i < data->header.references.nr; i++) {
> -		struct ref_list_entry *e = data->header.references.list + i;
> -		struct ref *ref = alloc_ref(e->name);
> -		struct object_id *oid = &e->oid;
> +		struct string_list_item *e = data->header.references.items + i;
> +		struct ref *ref = alloc_ref(e->string);

Similar question for `e->name` here.

> +		const struct object_id *oid = e->util;
>   		oidcpy(&ref->old_oid, oid);
>   		ref->next = result;
>   		result = ref;
>   	}
> +	string_list_clear(&data->header.references, 1);
>   	return result;
>   }
>   
> @@ -176,6 +177,7 @@ static int close_bundle(struct transport *transport)
>   	struct bundle_transport_data *data = transport->data;
>   	if (data->fd > 0)
>   		close(data->fd);
> +	bundle_header_release(&data->header);
>   	free(data);
>   	return 0;
>   }
> @@ -1082,6 +1084,8 @@ struct transport *transport_get(struct remote *remote, const char *url)
>   		die(_("git-over-rsync is no longer supported"));
>   	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {
>   		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
> +		string_list_init(&data->header.prerequisites, 1);
> +		string_list_init(&data->header.references, 1);
>   		transport_check_allowed("file");
>   		ret->data = data;
>   		ret->vtable = &bundle_vtable;
> 

[PATCH v2 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

This v2 addresses an omission noted by Andrei Rybak[1]. I didn't
factor out the "name" into a variable in 2/3 for ease of reading
3/3. That's now done.

1. https://lore.kernel.org/git/23c4ce5f-7769-1d2c-3a97-ac9733f73a25@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  bundle cmd: stop leaking memory from parse_options_cmd_bundle()
  bundle.c: use a temporary variable for OIDs and names
  bundle: remove "ref_list" in favor of string-list.c API

 builtin/bundle.c | 91 ++++++++++++++++++++++++++++++++----------------
 bundle.c         | 74 ++++++++++++++++++++++-----------------
 bundle.h         | 20 +++++------
 transport.c      | 12 +++++--
 4 files changed, 122 insertions(+), 75 deletions(-)

Range-diff against v1:
1:  f4191088ac3 = 1:  932c0883ce0 bundle cmd: stop leaking memory from parse_options_cmd_bundle()
2:  f297fd0432a ! 2:  7e0d57951e5 bundle.c: use a temporary variable for OIDs and names
    @@ bundle.c: int verify_bundle(struct repository *r,
      	for (i = 0; i < p->nr; i++) {
      		struct ref_list_entry *e = p->list + i;
     -		struct object *o = parse_object(r, &e->oid);
    ++		const char *name = e->name;
     +		struct object_id *oid = &e->oid;
     +		struct object *o = parse_object(r, oid);
      		if (o) {
      			o->flags |= PREREQ_MARK;
    - 			add_pending_object(&revs, o, e->name);
    -@@ bundle.c: int verify_bundle(struct repository *r,
    +-			add_pending_object(&revs, o, e->name);
    ++			add_pending_object(&revs, o, name);
    + 			continue;
      		}
      		if (++ret == 1)
      			error("%s", message);
     -		error("%s %s", oid_to_hex(&e->oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->name);
    ++		error("%s %s", oid_to_hex(oid), name);
      	}
      	if (revs.pending.nr != p->nr)
      		return ret;
    @@ bundle.c: int verify_bundle(struct repository *r,
      	for (i = 0; i < p->nr; i++) {
      		struct ref_list_entry *e = p->list + i;
     -		struct object *o = parse_object(r, &e->oid);
    ++		const char *name = e->name;
     +		struct object_id *oid = &e->oid;
     +		struct object *o = parse_object(r, oid);
      		assert(o); /* otherwise we'd have returned early */
    @@ bundle.c: int verify_bundle(struct repository *r,
      		if (++ret == 1)
      			error("%s", message);
     -		error("%s %s", oid_to_hex(&e->oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->name);
    ++		error("%s %s", oid_to_hex(oid), name);
      	}
      
      	/* Clean up objects used, as they will be reused. */
    @@ bundle.c: int verify_bundle(struct repository *r,
     
      ## transport.c ##
     @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,
    + 
      	for (i = 0; i < data->header.references.nr; i++) {
      		struct ref_list_entry *e = data->header.references.list + i;
    - 		struct ref *ref = alloc_ref(e->name);
    +-		struct ref *ref = alloc_ref(e->name);
     -		oidcpy(&ref->old_oid, &e->oid);
    ++		const char *name = e->name;
    ++		struct ref *ref = alloc_ref(name);
     +		struct object_id *oid = &e->oid;
     +		oidcpy(&ref->old_oid, oid);
      		ref->next = result;
3:  887313d3b02 ! 3:  9d9cb5aaf9e bundle: remove "ref_list" in favor of string-list.c API
    @@ bundle.c: int verify_bundle(struct repository *r,
      	repo_init_revisions(r, &revs, NULL);
      	for (i = 0; i < p->nr; i++) {
     -		struct ref_list_entry *e = p->list + i;
    +-		const char *name = e->name;
     -		struct object_id *oid = &e->oid;
     +		struct string_list_item *e = p->items + i;
    ++		const char *name = e->string;
     +		struct object_id *oid = e->util;
      		struct object *o = parse_object(r, oid);
      		if (o) {
      			o->flags |= PREREQ_MARK;
    --			add_pending_object(&revs, o, e->name);
    -+			add_pending_object(&revs, o, e->string);
    - 			continue;
    - 		}
    - 		if (++ret == 1)
    - 			error("%s", message);
    --		error("%s %s", oid_to_hex(oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->string);
    - 	}
    - 	if (revs.pending.nr != p->nr)
    - 		return ret;
     @@ bundle.c: int verify_bundle(struct repository *r,
      			i--;
      
      	for (i = 0; i < p->nr; i++) {
     -		struct ref_list_entry *e = p->list + i;
    +-		const char *name = e->name;
     -		struct object_id *oid = &e->oid;
     +		struct string_list_item *e = p->items + i;
    ++		const char *name = e->string;
     +		const struct object_id *oid = e->util;
      		struct object *o = parse_object(r, oid);
      		assert(o); /* otherwise we'd have returned early */
      		if (o->flags & SHOWN)
    - 			continue;
    - 		if (++ret == 1)
    - 			error("%s", message);
    --		error("%s %s", oid_to_hex(oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->string);
    - 	}
    +@@ bundle.c: int verify_bundle(struct repository *r,
      
      	/* Clean up objects used, as they will be reused. */
      	for (i = 0; i < p->nr; i++) {
    @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport
      
      	for (i = 0; i < data->header.references.nr; i++) {
     -		struct ref_list_entry *e = data->header.references.list + i;
    --		struct ref *ref = alloc_ref(e->name);
    --		struct object_id *oid = &e->oid;
    +-		const char *name = e->name;
     +		struct string_list_item *e = data->header.references.items + i;
    -+		struct ref *ref = alloc_ref(e->string);
    -+		const struct object_id *oid = e->util;
    ++		const char *name = e->string;
    + 		struct ref *ref = alloc_ref(name);
    +-		struct object_id *oid = &e->oid;
    ++		struct object_id *oid = e->util;
      		oidcpy(&ref->old_oid, oid);
      		ref->next = result;
      		result = ref;
-- 
2.32.0.599.g3967b4fa4ac

[PATCH v2 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).

As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 79 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 55 insertions(+), 24 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index ea6948110b0..7778297277a 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,
 		const char* prefix,
 		const char * const usagestr[],
 		const struct option options[],
-		const char **bundle_file) {
+		char **bundle_file) {
 	int newargc;
 	newargc = parse_options(argc, argv, NULL, options, usagestr,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,8 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	int progress = isatty(STDERR_FILENO);
 	struct strvec pack_opts;
 	int version = -1;
-
+	int die_no_repo = 0;
+	int ret;
 	struct option options[] = {
 		OPT_SET_INT('q', "quiet", &progress,
 			    N_("do not show progress meter"), 0),
@@ -76,7 +77,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 			    N_("specify bundle format version")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
@@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	if (progress && all_progress_implied)
 		strvec_push(&pack_opts, "--all-progress-implied");
 
-	if (!startup_info->have_repository)
+	if (!startup_info->have_repository) {
+		die_no_repo = 1;
+		goto cleanup;
+	}
+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+cleanup:
+	free(bundle_file);
+	if (die_no_repo)
 		die(_("Need a repository to create a bundle."));
-	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	return ret;
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
 	int quiet = 0;
-
+	int ret;
 	struct option options[] = {
 		OPT_BOOL('q', "quiet", &quiet,
 			    N_("do not show bundle details")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	if (verify_bundle(the_repository, &header, !quiet))
-		return 1;
+	if (verify_bundle(the_repository, &header, !quiet)) {
+		ret = 1;
+		goto cleanup;
+	}
+
 	fprintf(stderr, _("%s is okay\n"), bundle_file);
-	return 0;
+	ret = 0;
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	return !!list_bundle_refs(&header, argc, argv);
+	ret = !!list_bundle_refs(&header, argc, argv);
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int die_no_repo = 0;
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
-	if (!startup_info->have_repository)
-		die(_("Need a repository to unbundle."));
-	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
+	if (!startup_info->have_repository) {
+		die_no_repo = 1;
+		goto cleanup;
+	}
+	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+cleanup:
+	if (die_no_repo)
+		die(_("Need a repository to unbundle."));
+	free(bundle_file);
+	return ret;
 }
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
-- 
2.32.0.599.g3967b4fa4ac

[PATCH v2 2/3] bundle.c: use a temporary variable for OIDs and names

[Ævar Arnfjörð Bjarmason]

In preparation for moving away from accessing the OID and name via the
"oid" and "name" slots in a subsequent commit, change the code that
accesses it to use named variables. This makes the subsequent change
smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bundle.c    | 26 ++++++++++++++++++--------
 transport.c |  6 ++++--
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/bundle.c b/bundle.c
index 693d6195514..7210e5e7105 100644
--- a/bundle.c
+++ b/bundle.c
@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 	int i;
 
 	for (i = 0; i < r->nr; i++) {
+		struct object_id *oid;
+		const char *name;
+
 		if (argc > 1) {
 			int j;
 			for (j = 1; j < argc; j++)
@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 			if (j == argc)
 				continue;
 		}
-		printf("%s %s\n", oid_to_hex(&r->list[i].oid),
-				r->list[i].name);
+
+		oid = &r->list[i].oid;
+		name = r->list[i].name;
+		printf("%s %s\n", oid_to_hex(oid), name);
 	}
 	return 0;
 }
@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		struct object *o = parse_object(r, &e->oid);
+		const char *name = e->name;
+		struct object_id *oid = &e->oid;
+		struct object *o = parse_object(r, oid);
 		if (o) {
 			o->flags |= PREREQ_MARK;
-			add_pending_object(&revs, o, e->name);
+			add_pending_object(&revs, o, name);
 			continue;
 		}
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(&e->oid), e->name);
+		error("%s %s", oid_to_hex(oid), name);
 	}
 	if (revs.pending.nr != p->nr)
 		return ret;
@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,
 
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		struct object *o = parse_object(r, &e->oid);
+		const char *name = e->name;
+		struct object_id *oid = &e->oid;
+		struct object *o = parse_object(r, oid);
 		assert(o); /* otherwise we'd have returned early */
 		if (o->flags & SHOWN)
 			continue;
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(&e->oid), e->name);
+		error("%s %s", oid_to_hex(oid), name);
 	}
 
 	/* Clean up objects used, as they will be reused. */
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		commit = lookup_commit_reference_gently(r, &e->oid, 1);
+		struct object_id *oid = &e->oid;
+		commit = lookup_commit_reference_gently(r, oid, 1);
 		if (commit)
 			clear_commit_marks(commit, ALL_REV_FLAGS);
 	}
diff --git a/transport.c b/transport.c
index 50f5830eb6b..95c1138e9ae 100644
--- a/transport.c
+++ b/transport.c
@@ -148,8 +148,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 
 	for (i = 0; i < data->header.references.nr; i++) {
 		struct ref_list_entry *e = data->header.references.list + i;
-		struct ref *ref = alloc_ref(e->name);
-		oidcpy(&ref->old_oid, &e->oid);
+		const char *name = e->name;
+		struct ref *ref = alloc_ref(name);
+		struct object_id *oid = &e->oid;
+		oidcpy(&ref->old_oid, oid);
 		ref->next = result;
 		result = ref;
 	}
-- 
2.32.0.599.g3967b4fa4ac

[PATCH v2 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.

That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)

We need to change "name" to "string" and "oid" to "util" to make this
conversion, but other than that the APIs are pretty much identical for
what bundle.c made use of.

Let's also replace the memset(..,0,...) pattern with a more idiomatic
"INIT" macro, and finally add a *_release() function so to free the
allocated memory.

Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 12 +++++-----
 bundle.c         | 62 ++++++++++++++++++++++++------------------------
 bundle.h         | 20 ++++++++--------
 transport.c      | 10 +++++---
 4 files changed, 54 insertions(+), 50 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 7778297277a..8d822552808 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -106,7 +106,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int quiet = 0;
 	int ret;
@@ -121,7 +121,6 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -136,11 +135,12 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	ret = 0;
 cleanup:
 	free(bundle_file);
+	bundle_header_release(&header);
 	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
 	struct option options[] = {
@@ -152,7 +152,6 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -161,11 +160,12 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 	ret = !!list_bundle_refs(&header, argc, argv);
 cleanup:
 	free(bundle_file);
+	bundle_header_release(&header);
 	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int die_no_repo = 0;
 	int ret;
@@ -178,7 +178,6 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -189,6 +188,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 	}
 	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+	bundle_header_release(&header);
 cleanup:
 	if (die_no_repo)
 		die(_("Need a repository to unbundle."));
diff --git a/bundle.c b/bundle.c
index 7210e5e7105..1c3ae9e02ea 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,15 +23,6 @@ static struct {
 	{ 3, v3_bundle_signature },
 };
 
-static void add_to_ref_list(const struct object_id *oid, const char *name,
-		struct ref_list *list)
-{
-	ALLOC_GROW(list->list, list->nr + 1, list->alloc);
-	oidcpy(&list->list[list->nr].oid, oid);
-	list->list[list->nr].name = xstrdup(name);
-	list->nr++;
-}
-
 static int parse_capability(struct bundle_header *header, const char *capability)
 {
 	const char *arg;
@@ -79,7 +70,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 	/* The bundle header ends with an empty line */
 	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
 	       buf.len && buf.buf[0] != '\n') {
-		struct object_id oid;
+		struct object_id *oid;
 		int is_prereq = 0;
 		const char *p;
 
@@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 		 * Prerequisites have object name that is optionally
 		 * followed by SP and subject line.
 		 */
-		if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||
+		oid = xmalloc(sizeof(struct object_id));
+		if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||
 		    (*p && !isspace(*p)) ||
 		    (!is_prereq && !*p)) {
 			if (report_path)
 				error(_("unrecognized header: %s%s (%d)"),
 				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
 			status = -1;
+			free(oid);
 			break;
 		} else {
-			if (is_prereq)
-				add_to_ref_list(&oid, "", &header->prerequisites);
-			else
-				add_to_ref_list(&oid, p + 1, &header->references);
+			const char *string = is_prereq ? "" : p + 1;
+			struct string_list *list = is_prereq
+				? &header->prerequisites
+				: &header->references;
+			string_list_append(list, string)->util = oid;
 		}
 	}
 
@@ -139,19 +133,19 @@ int read_bundle_header(const char *path, struct bundle_header *header)
 
 int is_bundle(const char *path, int quiet)
 {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int fd = open(path, O_RDONLY);
 
 	if (fd < 0)
 		return 0;
-	memset(&header, 0, sizeof(header));
 	fd = parse_bundle_header(fd, &header, quiet ? NULL : path);
 	if (fd >= 0)
 		close(fd);
+	bundle_header_release(&header);
 	return (fd >= 0);
 }
 
-static int list_refs(struct ref_list *r, int argc, const char **argv)
+static int list_refs(struct string_list *r, int argc, const char **argv)
 {
 	int i;
 
@@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 		if (argc > 1) {
 			int j;
 			for (j = 1; j < argc; j++)
-				if (!strcmp(r->list[i].name, argv[j]))
+				if (!strcmp(r->items[i].string, argv[j]))
 					break;
 			if (j == argc)
 				continue;
 		}
 
-		oid = &r->list[i].oid;
-		name = r->list[i].name;
+		oid = r->items[i].util;
+		name = r->items[i].string;
 		printf("%s %s\n", oid_to_hex(oid), name);
 	}
 	return 0;
@@ -186,7 +180,7 @@ int verify_bundle(struct repository *r,
 	 * Do fast check, then if any prereqs are missing then go line by line
 	 * to be verbose about the errors
 	 */
-	struct ref_list *p = &header->prerequisites;
+	struct string_list *p = &header->prerequisites;
 	struct rev_info revs;
 	const char *argv[] = {NULL, "--all", NULL};
 	struct commit *commit;
@@ -198,9 +192,9 @@ int verify_bundle(struct repository *r,
 
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		const char *name = e->name;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		const char *name = e->string;
+		struct object_id *oid = e->util;
 		struct object *o = parse_object(r, oid);
 		if (o) {
 			o->flags |= PREREQ_MARK;
@@ -225,9 +219,9 @@ int verify_bundle(struct repository *r,
 			i--;
 
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		const char *name = e->name;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		const char *name = e->string;
+		const struct object_id *oid = e->util;
 		struct object *o = parse_object(r, oid);
 		assert(o); /* otherwise we'd have returned early */
 		if (o->flags & SHOWN)
@@ -239,15 +233,15 @@ int verify_bundle(struct repository *r,
 
 	/* Clean up objects used, as they will be reused. */
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		struct object_id *oid = e->util;
 		commit = lookup_commit_reference_gently(r, oid, 1);
 		if (commit)
 			clear_commit_marks(commit, ALL_REV_FLAGS);
 	}
 
 	if (verbose) {
-		struct ref_list *r;
+		struct string_list *r;
 
 		r = &header->references;
 		printf_ln(Q_("The bundle contains this ref:",
@@ -584,3 +578,9 @@ int unbundle(struct repository *r, struct bundle_header *header,
 		return error(_("index-pack died"));
 	return 0;
 }
+
+void bundle_header_release(struct bundle_header *header)
+{
+	string_list_clear(&header->prerequisites, 1);
+	string_list_clear(&header->references, 1);
+}
diff --git a/bundle.h b/bundle.h
index f9e2d1c8ef5..f98c1d24d9a 100644
--- a/bundle.h
+++ b/bundle.h
@@ -3,22 +3,21 @@
 
 #include "strvec.h"
 #include "cache.h"
-
-struct ref_list {
-	unsigned int nr, alloc;
-	struct ref_list_entry {
-		struct object_id oid;
-		char *name;
-	} *list;
-};
+#include "string-list.h"
 
 struct bundle_header {
 	unsigned version;
-	struct ref_list prerequisites;
-	struct ref_list references;
+	struct string_list prerequisites;
+	struct string_list references;
 	const struct git_hash_algo *hash_algo;
 };
 
+#define BUNDLE_HEADER_INIT \
+{ \
+	.prerequisites = STRING_LIST_INIT_DUP, \
+	.references = STRING_LIST_INIT_DUP, \
+}
+
 int is_bundle(const char *path, int quiet);
 int read_bundle_header(const char *path, struct bundle_header *header);
 int create_bundle(struct repository *r, const char *path,
@@ -30,5 +29,6 @@ int unbundle(struct repository *r, struct bundle_header *header,
 	     int bundle_fd, int flags);
 int list_bundle_refs(struct bundle_header *header,
 		int argc, const char **argv);
+void bundle_header_release(struct bundle_header *header);
 
 #endif
diff --git a/transport.c b/transport.c
index 95c1138e9ae..963655c772c 100644
--- a/transport.c
+++ b/transport.c
@@ -147,14 +147,15 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 	transport->hash_algo = data->header.hash_algo;
 
 	for (i = 0; i < data->header.references.nr; i++) {
-		struct ref_list_entry *e = data->header.references.list + i;
-		const char *name = e->name;
+		struct string_list_item *e = data->header.references.items + i;
+		const char *name = e->string;
 		struct ref *ref = alloc_ref(name);
-		struct object_id *oid = &e->oid;
+		struct object_id *oid = e->util;
 		oidcpy(&ref->old_oid, oid);
 		ref->next = result;
 		result = ref;
 	}
+	string_list_clear(&data->header.references, 1);
 	return result;
 }
 
@@ -177,6 +178,7 @@ static int close_bundle(struct transport *transport)
 	struct bundle_transport_data *data = transport->data;
 	if (data->fd > 0)
 		close(data->fd);
+	bundle_header_release(&data->header);
 	free(data);
 	return 0;
 }
@@ -1083,6 +1085,8 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		die(_("git-over-rsync is no longer supported"));
 	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
+		string_list_init(&data->header.prerequisites, 1);
+		string_list_init(&data->header.references, 1);
 		transport_check_allowed("file");
 		ret->data = data;
 		ret->vtable = &bundle_vtable;
-- 
2.32.0.599.g3967b4fa4ac

Re: [PATCH v2 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Jeff King]

On Mon, Jun 21, 2021 at 05:16:12PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Fix a memory leak from the prefix_filename() function introduced with
> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
> 2017-03-20).
> 
> As noted in that commit the leak was intentional as a part of being
> sloppy about freeing resources just before we exit, I'm changing this
> because I'll be fixing other memory leaks in the bundle API (including
> the library version) in subsequent commits. It's easier to reason
> about those fixes if valgrind runs cleanly at the end without any
> leaks whatsoever.

Looking at that old commit, it seems like this is a good candidate for
just inserting a single UNLEAK(bundle_file) into cmd_bundle(). But it
looks like the allocation has now migrated into all of the individual
sub-command functions, so we have to deal with it multiple times. They
could still use UNLEAK() if you want to avoid the "ret = foo(); free();
return ret" dance in each one, though.

We should avoid UNLEAK() in library-ish functions, but sub-commands that
are just one step away from cmd_bundle() returning are OK uses, IMHO.

> @@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>  	if (progress && all_progress_implied)
>  		strvec_push(&pack_opts, "--all-progress-implied");
>  
> -	if (!startup_info->have_repository)
> +	if (!startup_info->have_repository) {
> +		die_no_repo = 1;
> +		goto cleanup;
> +	}
> +	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
> +cleanup:
> +	free(bundle_file);
> +	if (die_no_repo)
>  		die(_("Need a repository to create a bundle."));
> -	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
> +	return ret;
>  }

This die_no_repo stuff confused me at first. But I think you are trying
to make sure we call free(bundle_file) before die? There is no point in
spending any effort on that, I think. When we exit() via die(), the
variable is still on the stack, and hence not leaked. And there are
probably a zillion other places we can hit a die() inside
create_bundle() anyway, which would produce the same effect. There's not
much point treating this one specially.

-Peff

Re: [PATCH v2 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Jeff King]

On Mon, Jun 21, 2021 at 05:16:14PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Move away from the "struct ref_list" in bundle.c in favor of the
> almost identical string-list.c API.
> 
> That API fits this use-case perfectly, but did not exist in its
> current form when this code was added in 2e0afafebd (Add git-bundle:
> move objects and references by archive, 2007-02-22), with hindsight we
> could have used the path-list API, which later got renamed to
> string-list. See 8fd2cb4069 (Extract helper bits from
> c-merge-recursive work, 2006-07-25)

I think this is a good direction, and I didn't see any errors in the
code. It's slightly sad that we end up with more lines than we started
with, but I think that's mostly because you're actually freeing the
memory now.

Two small nitpicks:

> @@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
>  		 * Prerequisites have object name that is optionally
>  		 * followed by SP and subject line.
>  		 */
> -		if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||
> +		oid = xmalloc(sizeof(struct object_id));
> +		if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||
>  		    (*p && !isspace(*p)) ||
>  		    (!is_prereq && !*p)) {
>  			if (report_path)
>  				error(_("unrecognized header: %s%s (%d)"),
>  				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
>  			status = -1;
> +			free(oid);
>  			break;
>  		} else {

This would be slightly simpler if you kept a local "struct object_id",
and then called:

  string_list_append(list, string)->util = oiddup(&oid);

later when you know you want to save it. And then you don't have to
worry about the extra cleanup here. That does require an extra oidcpy()
under the hood, but I suspect that is lost in the noise.

I'm OK with it either way.

> -			if (is_prereq)
> -				add_to_ref_list(&oid, "", &header->prerequisites);
> -			else
> -				add_to_ref_list(&oid, p + 1, &header->references);
> +			const char *string = is_prereq ? "" : p + 1;
> +			struct string_list *list = is_prereq
> +				? &header->prerequisites
> +				: &header->references;
> +			string_list_append(list, string)->util = oid;

I'm usually a big fan of the ternary operator, and using variable
indirection to make it clear that we always call a function.  But here I
think it makes things more confusing.  The two sides of the if/else are
sufficiently simple that it's easy to see they both make the same
function call. And because there are two variables, we check is_prereq
twice, making it much harder to see the two cases.

I.e., I think:

  if (is_prereq)
          string_list_append(&header->prerequisites, "")->util = oid;
  else
          string_list_append(&header->references, p + 1)->util = oid;

is much more obvious.

-Peff

Re: [PATCH v2 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

[Jeff King]

On Mon, Jun 21, 2021 at 05:16:11PM +0200, Ævar Arnfjörð Bjarmason wrote:

> This v2 addresses an omission noted by Andrei Rybak[1]. I didn't
> factor out the "name" into a variable in 2/3 for ease of reading
> 3/3. That's now done.

This all looks OK to me. I left a few small comments on the patches
themselves.

The UNLEAK() thing I suggested for patch 1 does make that patch much
smaller and easier to read, but I suspect makes patch 3 harder (i.e.,
you are reusing the "cleanup" sections there to do the bundle header
release. That could _also_ get UNLEAKed, but at some point it becomes
more clear to actually clean up after ourselves, and I think patch 3
probably crosses that point). So I'm OK to ignore that.

I would prefer to see the "die()" thing I mentioned there addressed, as
well as the ternary thing from patch 3. But neither of them is incorrect
as-is; it's just a style/preference thing.

-Peff

Re: [PATCH v2 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 24 2021, Jeff King wrote:

> On Mon, Jun 21, 2021 at 05:16:12PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Fix a memory leak from the prefix_filename() function introduced with
>> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
>> 2017-03-20).
>> 
>> As noted in that commit the leak was intentional as a part of being
>> sloppy about freeing resources just before we exit, I'm changing this
>> because I'll be fixing other memory leaks in the bundle API (including
>> the library version) in subsequent commits. It's easier to reason
>> about those fixes if valgrind runs cleanly at the end without any
>> leaks whatsoever.
>
> Looking at that old commit, it seems like this is a good candidate for
> just inserting a single UNLEAK(bundle_file) into cmd_bundle(). But it
> looks like the allocation has now migrated into all of the individual
> sub-command functions, so we have to deal with it multiple times. They
> could still use UNLEAK() if you want to avoid the "ret = foo(); free();
> return ret" dance in each one, though.
>
> We should avoid UNLEAK() in library-ish functions, but sub-commands that
> are just one step away from cmd_bundle() returning are OK uses, IMHO.

I'll change it if you feel strongly about it, but I always read UNLEAK()
as "ok, this is too hard, we won't bother, it's just a one-off
built-in", and not necessarily a recommendation for a desired pattern.

I think it's nice to have e.g. valgrind be able to report no leaks in
the binaries we build by default, not just if you compile with
-DSUPPRESS_ANNOTATED_LEAKS.

>> @@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
>>  	if (progress && all_progress_implied)
>>  		strvec_push(&pack_opts, "--all-progress-implied");
>>  
>> -	if (!startup_info->have_repository)
>> +	if (!startup_info->have_repository) {
>> +		die_no_repo = 1;
>> +		goto cleanup;
>> +	}
>> +	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
>> +cleanup:
>> +	free(bundle_file);
>> +	if (die_no_repo)
>>  		die(_("Need a repository to create a bundle."));
>> -	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
>> +	return ret;
>>  }
>
> This die_no_repo stuff confused me at first. But I think you are trying
> to make sure we call free(bundle_file) before die? There is no point in
> spending any effort on that, I think. When we exit() via die(), the
> variable is still on the stack, and hence not leaked. And there are
> probably a zillion other places we can hit a die() inside
> create_bundle() anyway, which would produce the same effect. There's not
> much point treating this one specially.

Right, it's there just for the free(), and yeah, there's a bunch of
places we'll leak anyway.

I do think per the above with valgrind that there's value in making the
common non-dying codepaths not leak though.

Re: [PATCH v2 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

On Thu, Jun 24 2021, Jeff King wrote:

> On Mon, Jun 21, 2021 at 05:16:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Move away from the "struct ref_list" in bundle.c in favor of the
>> almost identical string-list.c API.
>> 
>> That API fits this use-case perfectly, but did not exist in its
>> current form when this code was added in 2e0afafebd (Add git-bundle:
>> move objects and references by archive, 2007-02-22), with hindsight we
>> could have used the path-list API, which later got renamed to
>> string-list. See 8fd2cb4069 (Extract helper bits from
>> c-merge-recursive work, 2006-07-25)
>
> I think this is a good direction, and I didn't see any errors in the
> code. It's slightly sad that we end up with more lines than we started
> with, but I think that's mostly because you're actually freeing the
> memory now.
>
> Two small nitpicks:
>
>> @@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
>>  		 * Prerequisites have object name that is optionally
>>  		 * followed by SP and subject line.
>>  		 */
>> -		if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||
>> +		oid = xmalloc(sizeof(struct object_id));
>> +		if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||
>>  		    (*p && !isspace(*p)) ||
>>  		    (!is_prereq && !*p)) {
>>  			if (report_path)
>>  				error(_("unrecognized header: %s%s (%d)"),
>>  				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
>>  			status = -1;
>> +			free(oid);
>>  			break;
>>  		} else {
>
> This would be slightly simpler if you kept a local "struct object_id",
> and then called:
>
>   string_list_append(list, string)->util = oiddup(&oid);
>
> later when you know you want to save it. And then you don't have to
> worry about the extra cleanup here. That does require an extra oidcpy()
> under the hood, but I suspect that is lost in the noise.
>
> I'm OK with it either way.

That sounds simpler indeed, thanks.

>> -			if (is_prereq)
>> -				add_to_ref_list(&oid, "", &header->prerequisites);
>> -			else
>> -				add_to_ref_list(&oid, p + 1, &header->references);
>> +			const char *string = is_prereq ? "" : p + 1;
>> +			struct string_list *list = is_prereq
>> +				? &header->prerequisites
>> +				: &header->references;
>> +			string_list_append(list, string)->util = oid;
>
> I'm usually a big fan of the ternary operator, and using variable
> indirection to make it clear that we always call a function.  But here I
> think it makes things more confusing.  The two sides of the if/else are
> sufficiently simple that it's easy to see they both make the same
> function call. And because there are two variables, we check is_prereq
> twice, making it much harder to see the two cases.
>
> I.e., I think:
>
>   if (is_prereq)
>           string_list_append(&header->prerequisites, "")->util = oid;
>   else
>           string_list_append(&header->references, p + 1)->util = oid;
>
> is much more obvious.

Hah, that's actually the exact code I wrote to begin with, before
thinking "hrm, someone will probably say I should just use a ternary
here". I'll change it back :)

Re: [PATCH v2 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think this is a good direction, and I didn't see any errors in the
> code. It's slightly sad that we end up with more lines than we started
> with, but I think that's mostly because you're actually freeing the
> memory now.
> ...
> I.e., I think:
>
>   if (is_prereq)
>           string_list_append(&header->prerequisites, "")->util = oid;
>   else
>           string_list_append(&header->references, p + 1)->util = oid;
>
> is much more obvious.

Nicely done and nicely reviewed and improved.

Together with the "no point in freeing just before dying" on the
earlier step, polishing this topic to incorporate the suggested
changes should be fairly an easy task.  Let's not leave too many
loose ends hanging around and close this one with the last final
reroll (hopefully without "I did this too while at it" that meets
"oh, well, that is a bit controversi8al" to drag it unnecessarily
out).

Thanks.

[PATCH v3 0/3]

[Ævar Arnfjörð Bjarmason]

Refactor the bundle API to use the string_list API instead of its own
version of a similar API. See [1] for v2.

Addresses comments by Jeff King about us being too overzelous in
trying not to leak memory (the 'die_no_repo' is gone), and other
flow/style comments of his.

I also added a bundle_header_init() function for use in transport.c,
and noticed a redundant call to string_list_clear() there.

1. https://lore.kernel.org/git/cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  bundle cmd: stop leaking memory from parse_options_cmd_bundle()
  bundle.c: use a temporary variable for OIDs and names
  bundle: remove "ref_list" in favor of string-list.c API

 builtin/bundle.c | 74 ++++++++++++++++++++++++++++++------------------
 bundle.c         | 65 ++++++++++++++++++++++++++----------------
 bundle.h         | 21 +++++++-------
 transport.c      | 10 +++++--
 4 files changed, 105 insertions(+), 65 deletions(-)

Range-diff against v2:
1:  6a8b20a7cf3 < -:  ----------- upload-pack: run is_repository_shallow() before setup_revisions()
2:  d88b2c04102 < -:  ----------- revision.h: unify "disable_stdin" and "read_from_stdin"
3:  d433d7b24a3 < -:  ----------- pack-objects.c: do stdin parsing via revision.c's API
4:  e59a06c3148 < -:  ----------- pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS
5:  f4191088ac3 ! 1:  3d0d7a8e8b5 bundle cmd: stop leaking memory from parse_options_cmd_bundle()
    @@ Commit message
         about those fixes if valgrind runs cleanly at the end without any
         leaks whatsoever.
     
    +    An earlier version of this change went out of its way to not leak
    +    memory on the die() codepaths here, but that was deemed too verbose to
    +    worry about in a built-in that's dying anyway. The only reason we'd
    +    need that is to appease a mode like SANITIZE=leak within the scope of
    +    an entire test file.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/bundle.c ##
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      	struct strvec pack_opts;
      	int version = -1;
     -
    -+	int die_no_repo = 0;
     +	int ret;
      	struct option options[] = {
      		OPT_SET_INT('q', "quiet", &progress,
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      	argc = parse_options_cmd_bundle(argc, argv, prefix,
      			builtin_bundle_create_usage, options, &bundle_file);
     @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
    - 	if (progress && all_progress_implied)
    - 		strvec_push(&pack_opts, "--all-progress-implied");
      
    --	if (!startup_info->have_repository)
    -+	if (!startup_info->have_repository) {
    -+		die_no_repo = 1;
    -+		goto cleanup;
    -+	}
    -+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
    -+cleanup:
    -+	free(bundle_file);
    -+	if (die_no_repo)
    + 	if (!startup_info->have_repository)
      		die(_("Need a repository to create a bundle."));
     -	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
    ++	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
    ++	free(bundle_file);
     +	return ret;
      }
      
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      	struct bundle_header header;
      	int bundle_fd = -1;
     -
    -+	int die_no_repo = 0;
     +	int ret;
      	struct option options[] = {
      		OPT_END()
    @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons
      	memset(&header, 0, sizeof(header));
     -	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
     -		return 1;
    --	if (!startup_info->have_repository)
    --		die(_("Need a repository to unbundle."));
    --	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
     +	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
     +		ret = 1;
     +		goto cleanup;
     +	}
    -+	if (!startup_info->have_repository) {
    -+		die_no_repo = 1;
    -+		goto cleanup;
    -+	}
    + 	if (!startup_info->have_repository)
    + 		die(_("Need a repository to unbundle."));
    +-	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
     +	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
      		list_bundle_refs(&header, argc, argv);
     +cleanup:
    -+	if (die_no_repo)
    -+		die(_("Need a repository to unbundle."));
     +	free(bundle_file);
     +	return ret;
      }
6:  f297fd0432a ! 2:  e47646d3a98 bundle.c: use a temporary variable for OIDs and names
    @@ bundle.c: int verify_bundle(struct repository *r,
      	for (i = 0; i < p->nr; i++) {
      		struct ref_list_entry *e = p->list + i;
     -		struct object *o = parse_object(r, &e->oid);
    ++		const char *name = e->name;
     +		struct object_id *oid = &e->oid;
     +		struct object *o = parse_object(r, oid);
      		if (o) {
      			o->flags |= PREREQ_MARK;
    - 			add_pending_object(&revs, o, e->name);
    -@@ bundle.c: int verify_bundle(struct repository *r,
    +-			add_pending_object(&revs, o, e->name);
    ++			add_pending_object(&revs, o, name);
    + 			continue;
      		}
      		if (++ret == 1)
      			error("%s", message);
     -		error("%s %s", oid_to_hex(&e->oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->name);
    ++		error("%s %s", oid_to_hex(oid), name);
      	}
      	if (revs.pending.nr != p->nr)
      		return ret;
    @@ bundle.c: int verify_bundle(struct repository *r,
      	for (i = 0; i < p->nr; i++) {
      		struct ref_list_entry *e = p->list + i;
     -		struct object *o = parse_object(r, &e->oid);
    ++		const char *name = e->name;
     +		struct object_id *oid = &e->oid;
     +		struct object *o = parse_object(r, oid);
      		assert(o); /* otherwise we'd have returned early */
    @@ bundle.c: int verify_bundle(struct repository *r,
      		if (++ret == 1)
      			error("%s", message);
     -		error("%s %s", oid_to_hex(&e->oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->name);
    ++		error("%s %s", oid_to_hex(oid), name);
      	}
      
      	/* Clean up objects used, as they will be reused. */
    @@ bundle.c: int verify_bundle(struct repository *r,
     
      ## transport.c ##
     @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,
    + 
      	for (i = 0; i < data->header.references.nr; i++) {
      		struct ref_list_entry *e = data->header.references.list + i;
    - 		struct ref *ref = alloc_ref(e->name);
    +-		struct ref *ref = alloc_ref(e->name);
     -		oidcpy(&ref->old_oid, &e->oid);
    ++		const char *name = e->name;
    ++		struct ref *ref = alloc_ref(name);
     +		struct object_id *oid = &e->oid;
     +		oidcpy(&ref->old_oid, oid);
      		ref->next = result;
7:  887313d3b02 ! 3:  f1066ee1b9a bundle: remove "ref_list" in favor of string-list.c API
    @@ builtin/bundle.c: static int cmd_bundle_list_heads(int argc, const char **argv,
     -	struct bundle_header header;
     +	struct bundle_header header = BUNDLE_HEADER_INIT;
      	int bundle_fd = -1;
    - 	int die_no_repo = 0;
      	int ret;
    + 	struct option options[] = {
     @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
      			builtin_bundle_unbundle_usage, options, &bundle_file);
      	/* bundle internals use argv[1] as further parameters */
    @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, co
      		ret = 1;
      		goto cleanup;
     @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
    - 	}
    + 		die(_("Need a repository to unbundle."));
      	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
      		list_bundle_refs(&header, argc, argv);
     +	bundle_header_release(&header);
      cleanup:
    - 	if (die_no_repo)
    - 		die(_("Need a repository to unbundle."));
    + 	free(bundle_file);
    + 	return ret;
     
      ## bundle.c ##
     @@ bundle.c: static struct {
    @@ bundle.c: static struct {
      
     -static void add_to_ref_list(const struct object_id *oid, const char *name,
     -		struct ref_list *list)
    --{
    ++void bundle_header_init(struct bundle_header *header)
    + {
     -	ALLOC_GROW(list->list, list->nr + 1, list->alloc);
     -	oidcpy(&list->list[list->nr].oid, oid);
     -	list->list[list->nr].name = xstrdup(name);
     -	list->nr++;
    --}
    --
    - static int parse_capability(struct bundle_header *header, const char *capability)
    - {
    - 	const char *arg;
    -@@ bundle.c: static int parse_bundle_header(int fd, struct bundle_header *header,
    - 	/* The bundle header ends with an empty line */
    - 	while (!strbuf_getwholeline_fd(&buf, fd, '\n') &&
    - 	       buf.len && buf.buf[0] != '\n') {
    --		struct object_id oid;
    -+		struct object_id *oid;
    - 		int is_prereq = 0;
    - 		const char *p;
    ++	memset(header, 0, sizeof(*header));
    ++	string_list_init(&header->prerequisites, 1);
    ++	string_list_init(&header->references, 1);
    ++}
    ++
    ++void bundle_header_release(struct bundle_header *header)
    ++{
    ++	string_list_clear(&header->prerequisites, 1);
    ++	string_list_clear(&header->references, 1);
    + }
      
    + static int parse_capability(struct bundle_header *header, const char *capability)
     @@ bundle.c: static int parse_bundle_header(int fd, struct bundle_header *header,
    - 		 * Prerequisites have object name that is optionally
    - 		 * followed by SP and subject line.
    - 		 */
    --		if (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||
    -+		oid = xmalloc(sizeof(struct object_id));
    -+		if (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||
    - 		    (*p && !isspace(*p)) ||
    - 		    (!is_prereq && !*p)) {
    - 			if (report_path)
    - 				error(_("unrecognized header: %s%s (%d)"),
    - 				      (is_prereq ? "-" : ""), buf.buf, (int)buf.len);
      			status = -1;
    -+			free(oid);
      			break;
      		} else {
    --			if (is_prereq)
    ++			struct object_id *dup = oiddup(&oid);
    + 			if (is_prereq)
     -				add_to_ref_list(&oid, "", &header->prerequisites);
    --			else
    ++				string_list_append(&header->prerequisites, "")->util = dup;
    + 			else
     -				add_to_ref_list(&oid, p + 1, &header->references);
    -+			const char *string = is_prereq ? "" : p + 1;
    -+			struct string_list *list = is_prereq
    -+				? &header->prerequisites
    -+				: &header->references;
    -+			string_list_append(list, string)->util = oid;
    ++				string_list_append(&header->references, p + 1)->util = dup;
      		}
      	}
      
    @@ bundle.c: int verify_bundle(struct repository *r,
      	repo_init_revisions(r, &revs, NULL);
      	for (i = 0; i < p->nr; i++) {
     -		struct ref_list_entry *e = p->list + i;
    +-		const char *name = e->name;
     -		struct object_id *oid = &e->oid;
     +		struct string_list_item *e = p->items + i;
    ++		const char *name = e->string;
     +		struct object_id *oid = e->util;
      		struct object *o = parse_object(r, oid);
      		if (o) {
      			o->flags |= PREREQ_MARK;
    --			add_pending_object(&revs, o, e->name);
    -+			add_pending_object(&revs, o, e->string);
    - 			continue;
    - 		}
    - 		if (++ret == 1)
    - 			error("%s", message);
    --		error("%s %s", oid_to_hex(oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->string);
    - 	}
    - 	if (revs.pending.nr != p->nr)
    - 		return ret;
     @@ bundle.c: int verify_bundle(struct repository *r,
      			i--;
      
      	for (i = 0; i < p->nr; i++) {
     -		struct ref_list_entry *e = p->list + i;
    +-		const char *name = e->name;
     -		struct object_id *oid = &e->oid;
     +		struct string_list_item *e = p->items + i;
    ++		const char *name = e->string;
     +		const struct object_id *oid = e->util;
      		struct object *o = parse_object(r, oid);
      		assert(o); /* otherwise we'd have returned early */
      		if (o->flags & SHOWN)
    - 			continue;
    - 		if (++ret == 1)
    - 			error("%s", message);
    --		error("%s %s", oid_to_hex(oid), e->name);
    -+		error("%s %s", oid_to_hex(oid), e->string);
    - 	}
    +@@ bundle.c: int verify_bundle(struct repository *r,
      
      	/* Clean up objects used, as they will be reused. */
      	for (i = 0; i < p->nr; i++) {
    @@ bundle.c: int verify_bundle(struct repository *r,
      
      		r = &header->references;
      		printf_ln(Q_("The bundle contains this ref:",
    -@@ bundle.c: int unbundle(struct repository *r, struct bundle_header *header,
    - 		return error(_("index-pack died"));
    - 	return 0;
    - }
    -+
    -+void bundle_header_release(struct bundle_header *header)
    -+{
    -+	string_list_clear(&header->prerequisites, 1);
    -+	string_list_clear(&header->references, 1);
    -+}
     
      ## bundle.h ##
     @@
    @@ bundle.h
     +	.prerequisites = STRING_LIST_INIT_DUP, \
     +	.references = STRING_LIST_INIT_DUP, \
     +}
    ++void bundle_header_init(struct bundle_header *header);
    ++void bundle_header_release(struct bundle_header *header);
     +
      int is_bundle(const char *path, int quiet);
      int read_bundle_header(const char *path, struct bundle_header *header);
      int create_bundle(struct repository *r, const char *path,
    -@@ bundle.h: int unbundle(struct repository *r, struct bundle_header *header,
    - 	     int bundle_fd, int flags);
    - int list_bundle_refs(struct bundle_header *header,
    - 		int argc, const char **argv);
    -+void bundle_header_release(struct bundle_header *header);
    - 
    - #endif
     
      ## transport.c ##
     @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,
    @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport
      
      	for (i = 0; i < data->header.references.nr; i++) {
     -		struct ref_list_entry *e = data->header.references.list + i;
    --		struct ref *ref = alloc_ref(e->name);
    --		struct object_id *oid = &e->oid;
    +-		const char *name = e->name;
     +		struct string_list_item *e = data->header.references.items + i;
    -+		struct ref *ref = alloc_ref(e->string);
    -+		const struct object_id *oid = e->util;
    ++		const char *name = e->string;
    + 		struct ref *ref = alloc_ref(name);
    +-		struct object_id *oid = &e->oid;
    ++		struct object_id *oid = e->util;
      		oidcpy(&ref->old_oid, oid);
      		ref->next = result;
      		result = ref;
    - 	}
    -+	string_list_clear(&data->header.references, 1);
    - 	return result;
    - }
    - 
     @@ transport.c: static int close_bundle(struct transport *transport)
      	struct bundle_transport_data *data = transport->data;
      	if (data->fd > 0)
    @@ transport.c: struct transport *transport_get(struct remote *remote, const char *
      		die(_("git-over-rsync is no longer supported"));
      	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {
      		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
    -+		string_list_init(&data->header.prerequisites, 1);
    -+		string_list_init(&data->header.references, 1);
    ++		bundle_header_init(&data->header);
      		transport_check_allowed("file");
      		ret->data = data;
      		ret->vtable = &bundle_vtable;
-- 
2.32.0.613.g8e17abc2eb

[PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).

As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.

An earlier version of this change went out of its way to not leak
memory on the die() codepaths here, but that was deemed too verbose to
worry about in a built-in that's dying anyway. The only reason we'd
need that is to appease a mode like SANITIZE=leak within the scope of
an entire test file.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 62 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index ea6948110b0..15e2bd61965 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,
 		const char* prefix,
 		const char * const usagestr[],
 		const struct option options[],
-		const char **bundle_file) {
+		char **bundle_file) {
 	int newargc;
 	newargc = parse_options(argc, argv, NULL, options, usagestr,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	int progress = isatty(STDERR_FILENO);
 	struct strvec pack_opts;
 	int version = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_SET_INT('q', "quiet", &progress,
 			    N_("do not show progress meter"), 0),
@@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 			    N_("specify bundle format version")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
@@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
-	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
 	int quiet = 0;
-
+	int ret;
 	struct option options[] = {
 		OPT_BOOL('q', "quiet", &quiet,
 			    N_("do not show bundle details")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	if (verify_bundle(the_repository, &header, !quiet))
-		return 1;
+	if (verify_bundle(the_repository, &header, !quiet)) {
+		ret = 1;
+		goto cleanup;
+	}
+
 	fprintf(stderr, _("%s is okay\n"), bundle_file);
-	return 0;
+	ret = 0;
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	return !!list_bundle_refs(&header, argc, argv);
+	ret = !!list_bundle_refs(&header, argc, argv);
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
-	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
-- 
2.32.0.613.g8e17abc2eb

[PATCH v3 2/3] bundle.c: use a temporary variable for OIDs and names

[Ævar Arnfjörð Bjarmason]

In preparation for moving away from accessing the OID and name via the
"oid" and "name" slots in a subsequent commit, change the code that
accesses it to use named variables. This makes the subsequent change
smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bundle.c    | 26 ++++++++++++++++++--------
 transport.c |  6 ++++--
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/bundle.c b/bundle.c
index 693d6195514..7210e5e7105 100644
--- a/bundle.c
+++ b/bundle.c
@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 	int i;
 
 	for (i = 0; i < r->nr; i++) {
+		struct object_id *oid;
+		const char *name;
+
 		if (argc > 1) {
 			int j;
 			for (j = 1; j < argc; j++)
@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 			if (j == argc)
 				continue;
 		}
-		printf("%s %s\n", oid_to_hex(&r->list[i].oid),
-				r->list[i].name);
+
+		oid = &r->list[i].oid;
+		name = r->list[i].name;
+		printf("%s %s\n", oid_to_hex(oid), name);
 	}
 	return 0;
 }
@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		struct object *o = parse_object(r, &e->oid);
+		const char *name = e->name;
+		struct object_id *oid = &e->oid;
+		struct object *o = parse_object(r, oid);
 		if (o) {
 			o->flags |= PREREQ_MARK;
-			add_pending_object(&revs, o, e->name);
+			add_pending_object(&revs, o, name);
 			continue;
 		}
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(&e->oid), e->name);
+		error("%s %s", oid_to_hex(oid), name);
 	}
 	if (revs.pending.nr != p->nr)
 		return ret;
@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,
 
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		struct object *o = parse_object(r, &e->oid);
+		const char *name = e->name;
+		struct object_id *oid = &e->oid;
+		struct object *o = parse_object(r, oid);
 		assert(o); /* otherwise we'd have returned early */
 		if (o->flags & SHOWN)
 			continue;
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(&e->oid), e->name);
+		error("%s %s", oid_to_hex(oid), name);
 	}
 
 	/* Clean up objects used, as they will be reused. */
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		commit = lookup_commit_reference_gently(r, &e->oid, 1);
+		struct object_id *oid = &e->oid;
+		commit = lookup_commit_reference_gently(r, oid, 1);
 		if (commit)
 			clear_commit_marks(commit, ALL_REV_FLAGS);
 	}
diff --git a/transport.c b/transport.c
index 50f5830eb6b..95c1138e9ae 100644
--- a/transport.c
+++ b/transport.c
@@ -148,8 +148,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 
 	for (i = 0; i < data->header.references.nr; i++) {
 		struct ref_list_entry *e = data->header.references.list + i;
-		struct ref *ref = alloc_ref(e->name);
-		oidcpy(&ref->old_oid, &e->oid);
+		const char *name = e->name;
+		struct ref *ref = alloc_ref(name);
+		struct object_id *oid = &e->oid;
+		oidcpy(&ref->old_oid, oid);
 		ref->next = result;
 		result = ref;
 	}
-- 
2.32.0.613.g8e17abc2eb

[PATCH v3 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.

That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)

We need to change "name" to "string" and "oid" to "util" to make this
conversion, but other than that the APIs are pretty much identical for
what bundle.c made use of.

Let's also replace the memset(..,0,...) pattern with a more idiomatic
"INIT" macro, and finally add a *_release() function so to free the
allocated memory.

Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 12 +++++------
 bundle.c         | 53 ++++++++++++++++++++++++++----------------------
 bundle.h         | 21 ++++++++++---------
 transport.c      |  8 +++++---
 4 files changed, 51 insertions(+), 43 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 15e2bd61965..053a51bea1b 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -100,7 +100,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int quiet = 0;
 	int ret;
@@ -115,7 +115,6 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -130,11 +129,12 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	ret = 0;
 cleanup:
 	free(bundle_file);
+	bundle_header_release(&header);
 	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
 	struct option options[] = {
@@ -146,7 +146,6 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -155,11 +154,12 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 	ret = !!list_bundle_refs(&header, argc, argv);
 cleanup:
 	free(bundle_file);
+	bundle_header_release(&header);
 	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
 	struct option options[] = {
@@ -171,7 +171,6 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -180,6 +179,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 		die(_("Need a repository to unbundle."));
 	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+	bundle_header_release(&header);
 cleanup:
 	free(bundle_file);
 	return ret;
diff --git a/bundle.c b/bundle.c
index 7210e5e7105..cd5356a66f3 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,13 +23,17 @@ static struct {
 	{ 3, v3_bundle_signature },
 };
 
-static void add_to_ref_list(const struct object_id *oid, const char *name,
-		struct ref_list *list)
+void bundle_header_init(struct bundle_header *header)
 {
-	ALLOC_GROW(list->list, list->nr + 1, list->alloc);
-	oidcpy(&list->list[list->nr].oid, oid);
-	list->list[list->nr].name = xstrdup(name);
-	list->nr++;
+	memset(header, 0, sizeof(*header));
+	string_list_init(&header->prerequisites, 1);
+	string_list_init(&header->references, 1);
+}
+
+void bundle_header_release(struct bundle_header *header)
+{
+	string_list_clear(&header->prerequisites, 1);
+	string_list_clear(&header->references, 1);
 }
 
 static int parse_capability(struct bundle_header *header, const char *capability)
@@ -112,10 +116,11 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 			status = -1;
 			break;
 		} else {
+			struct object_id *dup = oiddup(&oid);
 			if (is_prereq)
-				add_to_ref_list(&oid, "", &header->prerequisites);
+				string_list_append(&header->prerequisites, "")->util = dup;
 			else
-				add_to_ref_list(&oid, p + 1, &header->references);
+				string_list_append(&header->references, p + 1)->util = dup;
 		}
 	}
 
@@ -139,19 +144,19 @@ int read_bundle_header(const char *path, struct bundle_header *header)
 
 int is_bundle(const char *path, int quiet)
 {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int fd = open(path, O_RDONLY);
 
 	if (fd < 0)
 		return 0;
-	memset(&header, 0, sizeof(header));
 	fd = parse_bundle_header(fd, &header, quiet ? NULL : path);
 	if (fd >= 0)
 		close(fd);
+	bundle_header_release(&header);
 	return (fd >= 0);
 }
 
-static int list_refs(struct ref_list *r, int argc, const char **argv)
+static int list_refs(struct string_list *r, int argc, const char **argv)
 {
 	int i;
 
@@ -162,14 +167,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 		if (argc > 1) {
 			int j;
 			for (j = 1; j < argc; j++)
-				if (!strcmp(r->list[i].name, argv[j]))
+				if (!strcmp(r->items[i].string, argv[j]))
 					break;
 			if (j == argc)
 				continue;
 		}
 
-		oid = &r->list[i].oid;
-		name = r->list[i].name;
+		oid = r->items[i].util;
+		name = r->items[i].string;
 		printf("%s %s\n", oid_to_hex(oid), name);
 	}
 	return 0;
@@ -186,7 +191,7 @@ int verify_bundle(struct repository *r,
 	 * Do fast check, then if any prereqs are missing then go line by line
 	 * to be verbose about the errors
 	 */
-	struct ref_list *p = &header->prerequisites;
+	struct string_list *p = &header->prerequisites;
 	struct rev_info revs;
 	const char *argv[] = {NULL, "--all", NULL};
 	struct commit *commit;
@@ -198,9 +203,9 @@ int verify_bundle(struct repository *r,
 
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		const char *name = e->name;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		const char *name = e->string;
+		struct object_id *oid = e->util;
 		struct object *o = parse_object(r, oid);
 		if (o) {
 			o->flags |= PREREQ_MARK;
@@ -225,9 +230,9 @@ int verify_bundle(struct repository *r,
 			i--;
 
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		const char *name = e->name;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		const char *name = e->string;
+		const struct object_id *oid = e->util;
 		struct object *o = parse_object(r, oid);
 		assert(o); /* otherwise we'd have returned early */
 		if (o->flags & SHOWN)
@@ -239,15 +244,15 @@ int verify_bundle(struct repository *r,
 
 	/* Clean up objects used, as they will be reused. */
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		struct object_id *oid = e->util;
 		commit = lookup_commit_reference_gently(r, oid, 1);
 		if (commit)
 			clear_commit_marks(commit, ALL_REV_FLAGS);
 	}
 
 	if (verbose) {
-		struct ref_list *r;
+		struct string_list *r;
 
 		r = &header->references;
 		printf_ln(Q_("The bundle contains this ref:",
diff --git a/bundle.h b/bundle.h
index f9e2d1c8ef5..1927d8cd6a4 100644
--- a/bundle.h
+++ b/bundle.h
@@ -3,22 +3,23 @@
 
 #include "strvec.h"
 #include "cache.h"
-
-struct ref_list {
-	unsigned int nr, alloc;
-	struct ref_list_entry {
-		struct object_id oid;
-		char *name;
-	} *list;
-};
+#include "string-list.h"
 
 struct bundle_header {
 	unsigned version;
-	struct ref_list prerequisites;
-	struct ref_list references;
+	struct string_list prerequisites;
+	struct string_list references;
 	const struct git_hash_algo *hash_algo;
 };
 
+#define BUNDLE_HEADER_INIT \
+{ \
+	.prerequisites = STRING_LIST_INIT_DUP, \
+	.references = STRING_LIST_INIT_DUP, \
+}
+void bundle_header_init(struct bundle_header *header);
+void bundle_header_release(struct bundle_header *header);
+
 int is_bundle(const char *path, int quiet);
 int read_bundle_header(const char *path, struct bundle_header *header);
 int create_bundle(struct repository *r, const char *path,
diff --git a/transport.c b/transport.c
index 95c1138e9ae..745ffa22474 100644
--- a/transport.c
+++ b/transport.c
@@ -147,10 +147,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 	transport->hash_algo = data->header.hash_algo;
 
 	for (i = 0; i < data->header.references.nr; i++) {
-		struct ref_list_entry *e = data->header.references.list + i;
-		const char *name = e->name;
+		struct string_list_item *e = data->header.references.items + i;
+		const char *name = e->string;
 		struct ref *ref = alloc_ref(name);
-		struct object_id *oid = &e->oid;
+		struct object_id *oid = e->util;
 		oidcpy(&ref->old_oid, oid);
 		ref->next = result;
 		result = ref;
@@ -177,6 +177,7 @@ static int close_bundle(struct transport *transport)
 	struct bundle_transport_data *data = transport->data;
 	if (data->fd > 0)
 		close(data->fd);
+	bundle_header_release(&data->header);
 	free(data);
 	return 0;
 }
@@ -1083,6 +1084,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		die(_("git-over-rsync is no longer supported"));
 	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
+		bundle_header_init(&data->header);
 		transport_check_allowed("file");
 		ret->data = data;
 		ret->vtable = &bundle_vtable;
-- 
2.32.0.613.g8e17abc2eb

Re: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Jeff King]

On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Fix a memory leak from the prefix_filename() function introduced with
> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
> 2017-03-20).
> 
> As noted in that commit the leak was intentional as a part of being
> sloppy about freeing resources just before we exit, I'm changing this
> because I'll be fixing other memory leaks in the bundle API (including
> the library version) in subsequent commits. It's easier to reason
> about those fixes if valgrind runs cleanly at the end without any
> leaks whatsoever.

Thanks, this looks good to me.

One thing, though...

> An earlier version of this change went out of its way to not leak
> memory on the die() codepaths here, but that was deemed too verbose to
> worry about in a built-in that's dying anyway. The only reason we'd
> need that is to appease a mode like SANITIZE=leak within the scope of
> an entire test file.

Obviously you changed this as I asked, but this final sentence makes me
think we're not on the same page with respect to die(). I don't think
any kind of mode matters here. When we call die(), whatever we have on
the stack is _not_ a leak, by LSan's or valgrind's standards. Because we
still have access to those bytes. And nor can we ever get rid of such
cases. If we ever do:

  void foo(const char *str)
  {
	char *x = xstrdup(str);
	bar(x);
	free(x);
  }

  void bar(const char *x)
  {
	if (!strcmp(x, "foo"))
		die("whatever");
  }

Then "x" will always still be allocated when we die(). We cannot free it
in bar(), where it is read-only. We cannot free it in foo() before we
call bar(), because it is needed there. But control never returns to the
free() statement.

So this code is perfectly fine and unavoidable. In the case you were
touching it was foo() that was calling die() directly, so we could work
around it with some conditionals. But from the leak-checker's
perspective the two cases are the same.

-Peff

Re: [PATCH v3 0/3]

[Jeff King]

On Wed, Jun 30, 2021 at 04:06:13PM +0200, Ævar Arnfjörð Bjarmason wrote:

> Refactor the bundle API to use the string_list API instead of its own
> version of a similar API. See [1] for v2.
> 
> Addresses comments by Jeff King about us being too overzelous in
> trying not to leak memory (the 'die_no_repo' is gone), and other
> flow/style comments of his.
> 
> I also added a bundle_header_init() function for use in transport.c,
> and noticed a redundant call to string_list_clear() there.

Thanks, all three look good to me.

As an aside, having to have a separate bundle_header_init() and
BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
date with each other), but quite common in our code base. I wonder if
writing:

  void bundle_header_init(struct bundle_header *header)
  {
	struct bundle_header blank = BUNDLE_HEADER_INIT;
	memcpy(header, &blank, sizeof(*header));
  }

would let a smart enough compiler just init "header" in place without
the extra copy (the performance of a single bundle_header almost
certainly doesn't matter, but it might for other types).

Just musing. ;)

-Peff

Re: [PATCH v3 0/3]

[Jeff King]

On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:

> As an aside, having to have a separate bundle_header_init() and
> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
> date with each other), but quite common in our code base. I wonder if
> writing:
> 
>   void bundle_header_init(struct bundle_header *header)
>   {
> 	struct bundle_header blank = BUNDLE_HEADER_INIT;
> 	memcpy(header, &blank, sizeof(*header));
>   }
> 
> would let a smart enough compiler just init "header" in place without
> the extra copy (the performance of a single bundle_header almost
> certainly doesn't matter, but it might for other types).
> 
> Just musing. ;)

For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9

With "gcc -O2" the memcpy goes away and we init "header" directly.

If we want to start using this technique widely, I don't think it should
be part of your series, though. This probably applies to quite a few
data structures, so it would make more sense to have a series which
converts several.

-Peff

Re: [PATCH v3 0/3]

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 30 2021, Jeff King wrote:

> On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:
>
>> As an aside, having to have a separate bundle_header_init() and
>> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
>> date with each other), but quite common in our code base. I wonder if
>> writing:
>> 
>>   void bundle_header_init(struct bundle_header *header)
>>   {
>> 	struct bundle_header blank = BUNDLE_HEADER_INIT;
>> 	memcpy(header, &blank, sizeof(*header));
>>   }
>> 
>> would let a smart enough compiler just init "header" in place without
>> the extra copy (the performance of a single bundle_header almost
>> certainly doesn't matter, but it might for other types).
>> 
>> Just musing. ;)
>
> For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9
>
> With "gcc -O2" the memcpy goes away and we init "header" directly.
>
> If we want to start using this technique widely, I don't think it should
> be part of your series, though. This probably applies to quite a few
> data structures, so it would make more sense to have a series which
> converts several.

That's cool, yeah that would make quite a lot of code better. Thanks!

Re: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 30 2021, Jeff King wrote:

> On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Fix a memory leak from the prefix_filename() function introduced with
>> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
>> 2017-03-20).
>> 
>> As noted in that commit the leak was intentional as a part of being
>> sloppy about freeing resources just before we exit, I'm changing this
>> because I'll be fixing other memory leaks in the bundle API (including
>> the library version) in subsequent commits. It's easier to reason
>> about those fixes if valgrind runs cleanly at the end without any
>> leaks whatsoever.
>
> Thanks, this looks good to me.
>
> One thing, though...
>
>> An earlier version of this change went out of its way to not leak
>> memory on the die() codepaths here, but that was deemed too verbose to
>> worry about in a built-in that's dying anyway. The only reason we'd
>> need that is to appease a mode like SANITIZE=leak within the scope of
>> an entire test file.
>
> Obviously you changed this as I asked, but this final sentence makes me
> think we're not on the same page with respect to die(). I don't think
> any kind of mode matters here. When we call die(), whatever we have on
> the stack is _not_ a leak, by LSan's or valgrind's standards. Because we
> still have access to those bytes. And nor can we ever get rid of such
> cases. If we ever do:
>
>   void foo(const char *str)
>   {
> 	char *x = xstrdup(str);
> 	bar(x);
> 	free(x);
>   }
>
>   void bar(const char *x)
>   {
> 	if (!strcmp(x, "foo"))
> 		die("whatever");
>   }
>
> Then "x" will always still be allocated when we die(). We cannot free it
> in bar(), where it is read-only. We cannot free it in foo() before we
> call bar(), because it is needed there. But control never returns to the
> free() statement.
>
> So this code is perfectly fine and unavoidable. In the case you were
> touching it was foo() that was calling die() directly, so we could work
> around it with some conditionals. But from the leak-checker's
> perspective the two cases are the same.

I've got you to blame for introducing SANITIZE=*. Now I've got these
leak checkers all mixed up :)

Yes you're right, FWIW I (re-)wrote this commit message just before
sending and should have said "a heap leak checker" instead of
"SANITIZE=leak", I really meant valgrind.

I originally ended with the "we are about to die" tracking because I was
tracing things with valgrind, which does complain about this sort of
thing. I.e.:
    
     24 bytes in 1 blocks are still reachable in loss record 8 of 21
        at 0x48356AF: malloc (vg_replace_malloc.c:298)
        by 0x4837DE7: realloc (vg_replace_malloc.c:826)
        by 0x3C06F1: xrealloc (wrapper.c:126)
        by 0x380EC9: strbuf_grow (strbuf.c:98)
        by 0x381A14: strbuf_add (strbuf.c:297)
        by 0x20ADC5: strbuf_addstr (strbuf.h:304)
        by 0x20B66D: prefix_filename (abspath.c:277)
        by 0x13CDC6: parse_options_cmd_bundle (bundle.c:55)
        by 0x13D565: cmd_bundle_unbundle (bundle.c:170)
        by 0x13D829: cmd_bundle (bundle.c:214)
        by 0x1279F4: run_builtin (git.c:461)
        by 0x127DFB: handle_builtin (git.c:714)

Re what I mentioned/asked in
https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ I was
experimenting with doing leak checking in the tests.

In this case we have 21 in total under --show-leak-kinds=all (and it was
20 in v2 of this series).

I've found that only including the file tho builtin is in cuts down on
it to a meaningful set of leaks. I.e. to throw out everything not
including /\bbundle\.c:/. We leak in a lot of things we call from
common-main.c, git.c, exec-cmd.c etc.

Maybe if we do end up with a test mode for this we shouldn't care about
checkers like valgrind and only cater to the SANITIZE=leak mode.

I'm still partial to the idea that we'll get the most win out of
something that we can run in the tests by default, i.e. we'll only need
to have a valgrind on the system & have someone run "make test" to run a
(limited set of) tests with this.

Whereas SANITIZE=leak is always a dev-only feature people may not have
on all the time.

Re: [PATCH v3 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Move away from the "struct ref_list" in bundle.c in favor of the
> almost identical string-list.c API.

Nice.

Re: [PATCH v3 0/3]

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 30 2021, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Jun 30 2021, Jeff King wrote:
>
>> On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:
>>
>>> As an aside, having to have a separate bundle_header_init() and
>>> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to
>>> date with each other), but quite common in our code base. I wonder if
>>> writing:
>>> 
>>>   void bundle_header_init(struct bundle_header *header)
>>>   {
>>> 	struct bundle_header blank = BUNDLE_HEADER_INIT;
>>> 	memcpy(header, &blank, sizeof(*header));
>>>   }
>>> 
>>> would let a smart enough compiler just init "header" in place without
>>> the extra copy (the performance of a single bundle_header almost
>>> certainly doesn't matter, but it might for other types).
>>> 
>>> Just musing. ;)
>>
>> For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9
>>
>> With "gcc -O2" the memcpy goes away and we init "header" directly.
>>
>> If we want to start using this technique widely, I don't think it should
>> be part of your series, though. This probably applies to quite a few
>> data structures, so it would make more sense to have a series which
>> converts several.
>
> That's cool, yeah that would make quite a lot of code better. Thanks!

Just for future reference: I submitted a small series to make use of
this suggested idiom:
https://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/

Re: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Jeff King]

On Wed, Jun 30, 2021 at 08:00:50PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > So this code is perfectly fine and unavoidable. In the case you were
> > touching it was foo() that was calling die() directly, so we could work
> > around it with some conditionals. But from the leak-checker's
> > perspective the two cases are the same.
> 
> I've got you to blame for introducing SANITIZE=*. Now I've got these
> leak checkers all mixed up :)
> 
> Yes you're right, FWIW I (re-)wrote this commit message just before
> sending and should have said "a heap leak checker" instead of
> "SANITIZE=leak", I really meant valgrind.

Ah, OK, that makes more sense.

I think the distinction here isn't "heap leak checker" (since valgrind
does still look at the whole memory space to find references), but
rather the treatment of "still reachable" items. I think LSan/ASan
ignore these entirely.

> I originally ended with the "we are about to die" tracking because I was
> tracing things with valgrind, which does complain about this sort of
> thing. I.e.:
>     
>      24 bytes in 1 blocks are still reachable in loss record 8 of 21
>         at 0x48356AF: malloc (vg_replace_malloc.c:298)
>         by 0x4837DE7: realloc (vg_replace_malloc.c:826)
>         by 0x3C06F1: xrealloc (wrapper.c:126)
>         by 0x380EC9: strbuf_grow (strbuf.c:98)
>         by 0x381A14: strbuf_add (strbuf.c:297)
>         by 0x20ADC5: strbuf_addstr (strbuf.h:304)
>         by 0x20B66D: prefix_filename (abspath.c:277)
>         by 0x13CDC6: parse_options_cmd_bundle (bundle.c:55)
>         by 0x13D565: cmd_bundle_unbundle (bundle.c:170)
>         by 0x13D829: cmd_bundle (bundle.c:214)
>         by 0x1279F4: run_builtin (git.c:461)
>         by 0x127DFB: handle_builtin (git.c:714)
> 
> Re what I mentioned/asked in
> https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ I was
> experimenting with doing leak checking in the tests.
> 
> In this case we have 21 in total under --show-leak-kinds=all (and it was
> 20 in v2 of this series).

IMHO these "still reachable" leaks are not interesting. They'll fall
into one of two categories:

  - allocations still on the stack when we exit() without unwinding.
    These are always uninteresting, since by definition we are exiting
    the process.

  - globals that hang on to allocations (which will still be present
    even if we exit the program by returning up through main()). Most of
    these will be items we intend to last for the program length (e.g.,
    fields in the_repository).

    It's _possible_ that some of these could be interesting. E.g., a
    program might have two phases: in the first, we rely on some
    subsystem which uses global variables to cache some data (say,
    parsed config values in remote.c or userdiff.c), and in the second
    phase we no longer use that subsystem. We could be instructing the
    subsystem to free up the memory after the first phase. But in
    practice this is awkward, because the program-level code doesn't
    know about subsystem allocations (or even which subsystems might
    have been recursively triggered).

    And while still-reachable tracking could be used to find
    opportunities like this, I think there's a better approach to
    finding these. If subsystems avoid global-variable caches and
    instead stick their allocations into context structs, then the
    memory is associated with those structs. So for example, if
    userdiff attached its storage to a diff_options, which is attached
    to a rev_info, then any "leaking" is all predicated on the rev_info
    (which the main program either cleans up, or annotates with UNLEAK).

> Maybe if we do end up with a test mode for this we shouldn't care about
> checkers like valgrind and only cater to the SANITIZE=leak mode.

I do find SANITIZE=leak to be a more useful tool in general, but I'm not
at all against using valgrind if people find it convenient. I just think
"reachable" leaks are not interesting enough to be part of what we're
searching for when we run the test suite.

> I'm still partial to the idea that we'll get the most win out of
> something that we can run in the tests by default, i.e. we'll only need
> to have a valgrind on the system & have someone run "make test" to run a
> (limited set of) tests with this.
> 
> Whereas SANITIZE=leak is always a dev-only feature people may not have
> on all the time.

That is exactly why I think "SANITIZE=leak" is better: it is easier for
people to run. valgrind is _extremely_ slow. It's also not available
everywhere; ASan/LSan aren't either, but they're pretty standard in
clang and gcc these days.

It is nice that valgrind can run on an un-instrumented binary, but I
sort of assume that anybody running "make test" will be able to build
the binary, since "make" is going to want to do that. I.e., I think
"make SANITIZE=leak test" is already doing what we want (we just need to
further annotate known-failures).

-Peff

[PATCH v4 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

This re-roll of v3 changes the discussion in the 1/3 commit message,
it incorrectly referred to SANITIZE=leak when I meant valgrind.

I also changed the bundle_header_init() pattern to use the same
"memcpy() a blank" as in my parallel series to do that more generally.

v3 at:
https://lore.kernel.org/git/cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (3):
  bundle cmd: stop leaking memory from parse_options_cmd_bundle()
  bundle.c: use a temporary variable for OIDs and names
  bundle: remove "ref_list" in favor of string-list.c API

 builtin/bundle.c | 74 ++++++++++++++++++++++++++++++------------------
 bundle.c         | 64 +++++++++++++++++++++++++----------------
 bundle.h         | 21 +++++++-------
 transport.c      | 10 +++++--
 4 files changed, 104 insertions(+), 65 deletions(-)

Range-diff against v3:
1:  3d0d7a8e8b5 ! 1:  8e1d08113e5 bundle cmd: stop leaking memory from parse_options_cmd_bundle()
    @@ Commit message
         about those fixes if valgrind runs cleanly at the end without any
         leaks whatsoever.
     
    -    An earlier version of this change went out of its way to not leak
    -    memory on the die() codepaths here, but that was deemed too verbose to
    -    worry about in a built-in that's dying anyway. The only reason we'd
    -    need that is to appease a mode like SANITIZE=leak within the scope of
    -    an entire test file.
    +    An earlier version of this change[1] went out of its way to not leak
    +    memory on the die() codepaths here, but doing so will only avoid
    +    reports of potential leaks under heap-only leak trackers such as
    +    valgrind, not the SANITIZE=leak mode.
    +
    +    Avoiding those leaks as well might be useful to enable us to run
    +    cleanly under the likes of valgrind in the future. But for now the
    +    relative verbosity of the resulting code, and the fact that we don't
    +    have some valgrind or SANITIZE=leak mode as part of our CI (it's only
    +    run ad-hoc, see [2]), means we're not worrying about that for now.
    +
    +    1. https://lore.kernel.org/git/87v95vdxrc.fsf@evledraar.gmail.com/
    +    2. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
2:  e47646d3a98 = 2:  5ce376682b3 bundle.c: use a temporary variable for OIDs and names
3:  f1066ee1b9a ! 3:  3e5972e4184 bundle: remove "ref_list" in favor of string-list.c API
    @@ Commit message
         Before this the add_to_ref_list() would leak memory, now e.g. "bundle
         list-heads" reports no memory leaks at all under valgrind.
     
    +    In the bundle_header_init() function we're using a clever trick to
    +    memcpy() what we'd get from the corresponding
    +    BUNDLE_HEADER_INIT. There is a concurrent series to make use of that
    +    pattern more generally, see [1].
    +
    +    1. https://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/bundle.c ##
    @@ bundle.c: static struct {
     -	oidcpy(&list->list[list->nr].oid, oid);
     -	list->list[list->nr].name = xstrdup(name);
     -	list->nr++;
    -+	memset(header, 0, sizeof(*header));
    -+	string_list_init(&header->prerequisites, 1);
    -+	string_list_init(&header->references, 1);
    ++	struct bundle_header blank = BUNDLE_HEADER_INIT;
    ++	memcpy(header, &blank, sizeof(*header));
     +}
     +
     +void bundle_header_release(struct bundle_header *header)
-- 
2.32.0.632.g49a94b9226d

[PATCH v4 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()

[Ævar Arnfjörð Bjarmason]

Fix a memory leak from the prefix_filename() function introduced with
its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,
2017-03-20).

As noted in that commit the leak was intentional as a part of being
sloppy about freeing resources just before we exit, I'm changing this
because I'll be fixing other memory leaks in the bundle API (including
the library version) in subsequent commits. It's easier to reason
about those fixes if valgrind runs cleanly at the end without any
leaks whatsoever.

An earlier version of this change[1] went out of its way to not leak
memory on the die() codepaths here, but doing so will only avoid
reports of potential leaks under heap-only leak trackers such as
valgrind, not the SANITIZE=leak mode.

Avoiding those leaks as well might be useful to enable us to run
cleanly under the likes of valgrind in the future. But for now the
relative verbosity of the resulting code, and the fact that we don't
have some valgrind or SANITIZE=leak mode as part of our CI (it's only
run ad-hoc, see [2]), means we're not worrying about that for now.

1. https://lore.kernel.org/git/87v95vdxrc.fsf@evledraar.gmail.com/
2. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 62 ++++++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 21 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index ea6948110b0..15e2bd61965 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,
 		const char* prefix,
 		const char * const usagestr[],
 		const struct option options[],
-		const char **bundle_file) {
+		char **bundle_file) {
 	int newargc;
 	newargc = parse_options(argc, argv, NULL, options, usagestr,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
@@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	int progress = isatty(STDERR_FILENO);
 	struct strvec pack_opts;
 	int version = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_SET_INT('q', "quiet", &progress,
 			    N_("do not show progress meter"), 0),
@@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 			    N_("specify bundle format version")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_create_usage, options, &bundle_file);
@@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 
 	if (!startup_info->have_repository)
 		die(_("Need a repository to create a bundle."));
-	return !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	ret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
 	int quiet = 0;
-
+	int ret;
 	struct option options[] = {
 		OPT_BOOL('q', "quiet", &quiet,
 			    N_("do not show bundle details")),
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	if (verify_bundle(the_repository, &header, !quiet))
-		return 1;
+	if (verify_bundle(the_repository, &header, !quiet)) {
+		ret = 1;
+		goto cleanup;
+	}
+
 	fprintf(stderr, _("%s is okay\n"), bundle_file);
-	return 0;
+	ret = 0;
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	close(bundle_fd);
-	return !!list_bundle_refs(&header, argc, argv);
+	ret = !!list_bundle_refs(&header, argc, argv);
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header;
 	int bundle_fd = -1;
-
+	int ret;
 	struct option options[] = {
 		OPT_END()
 	};
-	const char* bundle_file;
+	char *bundle_file;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
 	memset(&header, 0, sizeof(header));
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)
-		return 1;
+	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+		ret = 1;
+		goto cleanup;
+	}
 	if (!startup_info->have_repository)
 		die(_("Need a repository to unbundle."));
-	return !!unbundle(the_repository, &header, bundle_fd, 0) ||
+	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+cleanup:
+	free(bundle_file);
+	return ret;
 }
 
 int cmd_bundle(int argc, const char **argv, const char *prefix)
-- 
2.32.0.632.g49a94b9226d

[PATCH v4 2/3] bundle.c: use a temporary variable for OIDs and names

[Ævar Arnfjörð Bjarmason]

In preparation for moving away from accessing the OID and name via the
"oid" and "name" slots in a subsequent commit, change the code that
accesses it to use named variables. This makes the subsequent change
smaller.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bundle.c    | 26 ++++++++++++++++++--------
 transport.c |  6 ++++--
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/bundle.c b/bundle.c
index 693d6195514..7210e5e7105 100644
--- a/bundle.c
+++ b/bundle.c
@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 	int i;
 
 	for (i = 0; i < r->nr; i++) {
+		struct object_id *oid;
+		const char *name;
+
 		if (argc > 1) {
 			int j;
 			for (j = 1; j < argc; j++)
@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 			if (j == argc)
 				continue;
 		}
-		printf("%s %s\n", oid_to_hex(&r->list[i].oid),
-				r->list[i].name);
+
+		oid = &r->list[i].oid;
+		name = r->list[i].name;
+		printf("%s %s\n", oid_to_hex(oid), name);
 	}
 	return 0;
 }
@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		struct object *o = parse_object(r, &e->oid);
+		const char *name = e->name;
+		struct object_id *oid = &e->oid;
+		struct object *o = parse_object(r, oid);
 		if (o) {
 			o->flags |= PREREQ_MARK;
-			add_pending_object(&revs, o, e->name);
+			add_pending_object(&revs, o, name);
 			continue;
 		}
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(&e->oid), e->name);
+		error("%s %s", oid_to_hex(oid), name);
 	}
 	if (revs.pending.nr != p->nr)
 		return ret;
@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,
 
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		struct object *o = parse_object(r, &e->oid);
+		const char *name = e->name;
+		struct object_id *oid = &e->oid;
+		struct object *o = parse_object(r, oid);
 		assert(o); /* otherwise we'd have returned early */
 		if (o->flags & SHOWN)
 			continue;
 		if (++ret == 1)
 			error("%s", message);
-		error("%s %s", oid_to_hex(&e->oid), e->name);
+		error("%s %s", oid_to_hex(oid), name);
 	}
 
 	/* Clean up objects used, as they will be reused. */
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
-		commit = lookup_commit_reference_gently(r, &e->oid, 1);
+		struct object_id *oid = &e->oid;
+		commit = lookup_commit_reference_gently(r, oid, 1);
 		if (commit)
 			clear_commit_marks(commit, ALL_REV_FLAGS);
 	}
diff --git a/transport.c b/transport.c
index 50f5830eb6b..95c1138e9ae 100644
--- a/transport.c
+++ b/transport.c
@@ -148,8 +148,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 
 	for (i = 0; i < data->header.references.nr; i++) {
 		struct ref_list_entry *e = data->header.references.list + i;
-		struct ref *ref = alloc_ref(e->name);
-		oidcpy(&ref->old_oid, &e->oid);
+		const char *name = e->name;
+		struct ref *ref = alloc_ref(name);
+		struct object_id *oid = &e->oid;
+		oidcpy(&ref->old_oid, oid);
 		ref->next = result;
 		result = ref;
 	}
-- 
2.32.0.632.g49a94b9226d

[PATCH v4 3/3] bundle: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

Move away from the "struct ref_list" in bundle.c in favor of the
almost identical string-list.c API.

That API fits this use-case perfectly, but did not exist in its
current form when this code was added in 2e0afafebd (Add git-bundle:
move objects and references by archive, 2007-02-22), with hindsight we
could have used the path-list API, which later got renamed to
string-list. See 8fd2cb4069 (Extract helper bits from
c-merge-recursive work, 2006-07-25)

We need to change "name" to "string" and "oid" to "util" to make this
conversion, but other than that the APIs are pretty much identical for
what bundle.c made use of.

Let's also replace the memset(..,0,...) pattern with a more idiomatic
"INIT" macro, and finally add a *_release() function so to free the
allocated memory.

Before this the add_to_ref_list() would leak memory, now e.g. "bundle
list-heads" reports no memory leaks at all under valgrind.

In the bundle_header_init() function we're using a clever trick to
memcpy() what we'd get from the corresponding
BUNDLE_HEADER_INIT. There is a concurrent series to make use of that
pattern more generally, see [1].

1. https://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bundle.c | 12 +++++------
 bundle.c         | 52 ++++++++++++++++++++++++++----------------------
 bundle.h         | 21 +++++++++----------
 transport.c      |  8 +++++---
 4 files changed, 50 insertions(+), 43 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index 15e2bd61965..053a51bea1b 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -100,7 +100,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 }
 
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int quiet = 0;
 	int ret;
@@ -115,7 +115,6 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -130,11 +129,12 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	ret = 0;
 cleanup:
 	free(bundle_file);
+	bundle_header_release(&header);
 	return ret;
 }
 
 static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
 	struct option options[] = {
@@ -146,7 +146,6 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -155,11 +154,12 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 	ret = !!list_bundle_refs(&header, argc, argv);
 cleanup:
 	free(bundle_file);
+	bundle_header_release(&header);
 	return ret;
 }
 
 static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
 	int ret;
 	struct option options[] = {
@@ -171,7 +171,6 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	memset(&header, 0, sizeof(header));
 	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
 		ret = 1;
 		goto cleanup;
@@ -180,6 +179,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 		die(_("Need a repository to unbundle."));
 	ret = !!unbundle(the_repository, &header, bundle_fd, 0) ||
 		list_bundle_refs(&header, argc, argv);
+	bundle_header_release(&header);
 cleanup:
 	free(bundle_file);
 	return ret;
diff --git a/bundle.c b/bundle.c
index 7210e5e7105..ab63f402261 100644
--- a/bundle.c
+++ b/bundle.c
@@ -23,13 +23,16 @@ static struct {
 	{ 3, v3_bundle_signature },
 };
 
-static void add_to_ref_list(const struct object_id *oid, const char *name,
-		struct ref_list *list)
+void bundle_header_init(struct bundle_header *header)
 {
-	ALLOC_GROW(list->list, list->nr + 1, list->alloc);
-	oidcpy(&list->list[list->nr].oid, oid);
-	list->list[list->nr].name = xstrdup(name);
-	list->nr++;
+	struct bundle_header blank = BUNDLE_HEADER_INIT;
+	memcpy(header, &blank, sizeof(*header));
+}
+
+void bundle_header_release(struct bundle_header *header)
+{
+	string_list_clear(&header->prerequisites, 1);
+	string_list_clear(&header->references, 1);
 }
 
 static int parse_capability(struct bundle_header *header, const char *capability)
@@ -112,10 +115,11 @@ static int parse_bundle_header(int fd, struct bundle_header *header,
 			status = -1;
 			break;
 		} else {
+			struct object_id *dup = oiddup(&oid);
 			if (is_prereq)
-				add_to_ref_list(&oid, "", &header->prerequisites);
+				string_list_append(&header->prerequisites, "")->util = dup;
 			else
-				add_to_ref_list(&oid, p + 1, &header->references);
+				string_list_append(&header->references, p + 1)->util = dup;
 		}
 	}
 
@@ -139,19 +143,19 @@ int read_bundle_header(const char *path, struct bundle_header *header)
 
 int is_bundle(const char *path, int quiet)
 {
-	struct bundle_header header;
+	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int fd = open(path, O_RDONLY);
 
 	if (fd < 0)
 		return 0;
-	memset(&header, 0, sizeof(header));
 	fd = parse_bundle_header(fd, &header, quiet ? NULL : path);
 	if (fd >= 0)
 		close(fd);
+	bundle_header_release(&header);
 	return (fd >= 0);
 }
 
-static int list_refs(struct ref_list *r, int argc, const char **argv)
+static int list_refs(struct string_list *r, int argc, const char **argv)
 {
 	int i;
 
@@ -162,14 +166,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)
 		if (argc > 1) {
 			int j;
 			for (j = 1; j < argc; j++)
-				if (!strcmp(r->list[i].name, argv[j]))
+				if (!strcmp(r->items[i].string, argv[j]))
 					break;
 			if (j == argc)
 				continue;
 		}
 
-		oid = &r->list[i].oid;
-		name = r->list[i].name;
+		oid = r->items[i].util;
+		name = r->items[i].string;
 		printf("%s %s\n", oid_to_hex(oid), name);
 	}
 	return 0;
@@ -186,7 +190,7 @@ int verify_bundle(struct repository *r,
 	 * Do fast check, then if any prereqs are missing then go line by line
 	 * to be verbose about the errors
 	 */
-	struct ref_list *p = &header->prerequisites;
+	struct string_list *p = &header->prerequisites;
 	struct rev_info revs;
 	const char *argv[] = {NULL, "--all", NULL};
 	struct commit *commit;
@@ -198,9 +202,9 @@ int verify_bundle(struct repository *r,
 
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		const char *name = e->name;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		const char *name = e->string;
+		struct object_id *oid = e->util;
 		struct object *o = parse_object(r, oid);
 		if (o) {
 			o->flags |= PREREQ_MARK;
@@ -225,9 +229,9 @@ int verify_bundle(struct repository *r,
 			i--;
 
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		const char *name = e->name;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		const char *name = e->string;
+		const struct object_id *oid = e->util;
 		struct object *o = parse_object(r, oid);
 		assert(o); /* otherwise we'd have returned early */
 		if (o->flags & SHOWN)
@@ -239,15 +243,15 @@ int verify_bundle(struct repository *r,
 
 	/* Clean up objects used, as they will be reused. */
 	for (i = 0; i < p->nr; i++) {
-		struct ref_list_entry *e = p->list + i;
-		struct object_id *oid = &e->oid;
+		struct string_list_item *e = p->items + i;
+		struct object_id *oid = e->util;
 		commit = lookup_commit_reference_gently(r, oid, 1);
 		if (commit)
 			clear_commit_marks(commit, ALL_REV_FLAGS);
 	}
 
 	if (verbose) {
-		struct ref_list *r;
+		struct string_list *r;
 
 		r = &header->references;
 		printf_ln(Q_("The bundle contains this ref:",
diff --git a/bundle.h b/bundle.h
index f9e2d1c8ef5..1927d8cd6a4 100644
--- a/bundle.h
+++ b/bundle.h
@@ -3,22 +3,23 @@
 
 #include "strvec.h"
 #include "cache.h"
-
-struct ref_list {
-	unsigned int nr, alloc;
-	struct ref_list_entry {
-		struct object_id oid;
-		char *name;
-	} *list;
-};
+#include "string-list.h"
 
 struct bundle_header {
 	unsigned version;
-	struct ref_list prerequisites;
-	struct ref_list references;
+	struct string_list prerequisites;
+	struct string_list references;
 	const struct git_hash_algo *hash_algo;
 };
 
+#define BUNDLE_HEADER_INIT \
+{ \
+	.prerequisites = STRING_LIST_INIT_DUP, \
+	.references = STRING_LIST_INIT_DUP, \
+}
+void bundle_header_init(struct bundle_header *header);
+void bundle_header_release(struct bundle_header *header);
+
 int is_bundle(const char *path, int quiet);
 int read_bundle_header(const char *path, struct bundle_header *header);
 int create_bundle(struct repository *r, const char *path,
diff --git a/transport.c b/transport.c
index 95c1138e9ae..745ffa22474 100644
--- a/transport.c
+++ b/transport.c
@@ -147,10 +147,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,
 	transport->hash_algo = data->header.hash_algo;
 
 	for (i = 0; i < data->header.references.nr; i++) {
-		struct ref_list_entry *e = data->header.references.list + i;
-		const char *name = e->name;
+		struct string_list_item *e = data->header.references.items + i;
+		const char *name = e->string;
 		struct ref *ref = alloc_ref(name);
-		struct object_id *oid = &e->oid;
+		struct object_id *oid = e->util;
 		oidcpy(&ref->old_oid, oid);
 		ref->next = result;
 		result = ref;
@@ -177,6 +177,7 @@ static int close_bundle(struct transport *transport)
 	struct bundle_transport_data *data = transport->data;
 	if (data->fd > 0)
 		close(data->fd);
+	bundle_header_release(&data->header);
 	free(data);
 	return 0;
 }
@@ -1083,6 +1084,7 @@ struct transport *transport_get(struct remote *remote, const char *url)
 		die(_("git-over-rsync is no longer supported"));
 	} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {
 		struct bundle_transport_data *data = xcalloc(1, sizeof(*data));
+		bundle_header_init(&data->header);
 		transport_check_allowed("file");
 		ret->data = data;
 		ret->vtable = &bundle_vtable;
-- 
2.32.0.632.g49a94b9226d

Re: [PATCH v4 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

[Jeff King]

On Fri, Jul 02, 2021 at 11:57:29AM +0200, Ævar Arnfjörð Bjarmason wrote:

> This re-roll of v3 changes the discussion in the 1/3 commit message,
> it incorrectly referred to SANITIZE=leak when I meant valgrind.
> 
> I also changed the bundle_header_init() pattern to use the same
> "memcpy() a blank" as in my parallel series to do that more generally.

Thanks, this looks good to me.

I'd probably word the discussion about die() a bit differently, but you've
already seen my expositions on leak-checking, and it's all tangent here.
So let's move forward with this, and we can let leak-checking
philosophies iron themselves out as we fix more cases. :)

-Peff

Re: [PATCH v4 0/3] bundle.c: remove "ref_list" in favor of string-list.c API

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 03 2021, Jeff King wrote:

> On Fri, Jul 02, 2021 at 11:57:29AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> This re-roll of v3 changes the discussion in the 1/3 commit message,
>> it incorrectly referred to SANITIZE=leak when I meant valgrind.
>> 
>> I also changed the bundle_header_init() pattern to use the same
>> "memcpy() a blank" as in my parallel series to do that more generally.
>
> Thanks, this looks good to me.
>
> I'd probably word the discussion about die() a bit differently, but you've
> already seen my expositions on leak-checking, and it's all tangent here.
> So let's move forward with this, and we can let leak-checking
> philosophies iron themselves out as we fix more cases. :)

Thanks for the detailed review over multiple rounds.

Re: [PATCH v3 0/3]

[Doug Evans]

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

On Thu, Feb 11, 2021 at 1:12 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> [...]
>
> I think the key useful part to keep common impl for is the handling
> of the [] brackets for IPv6 raw addrs. I'd suggest we try to pull the
> "address:port" part out into a new inet_addr_parse() helper that can be
> called from inet_pase and from slirp.
>
> inet_parse() can split on the first ",", and then call inet_addr_parse
> on the first segment.
>
> slirp can split on "-", and call inet_addr_parse with both segments.
>


v4 here:
https://lists.nongnu.org/archive/html/qemu-devel/2021-02/msg06011.html

[-- Attachment #2: Type: text/html, Size: 1213 bytes --]

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 10, 2021 at 02:40:17PM -0800, Doug Evans wrote:
> On Wed, Feb 10, 2021 at 8:49 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> > > On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> > > wrote:
> > >
> > > > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > > > >
> > > > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <
> > berrange@redhat.com
> > > > >
> > > > > > wrote:
> > > > > >
> > > > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > > > >> > Add support for ipv6 host forwarding
> > > > > >> >
> > > > > >> > This patchset takes the original patch from Maxim,
> > > > > >> >
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > > > > >> > and updates it.
> > > > > >> >
> > > > > >> > New option: -ipv6-hostfwd
> > > > > >> >
> > > > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > > > >> >
> > > > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > > > >>
> > > > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > > > >> that we don't need to add any new commands/options. We can
> > > > > >> use existing inet_parse() helper function to parse the address
> > > > > >> info and transparently support IPv4/6 in the existing commands
> > > > > >> and options. This matches normal practice elsewhere in QEMU
> > > > > >> for IP dual stack.
> > > > > >>
> > > > > >
> > > > > > I'm all for this, fwiw.
> > > > > >
> > > > >
> > > > >
> > > > > I should say I'm all for not adding new commands/options.
> > > > > Looking at inet_parse() it cannot be used as-is.
> > > > > The question then becomes: Will refactoring it buy enough?
> > > >
> > > > What's the problem your hitting with inet_parse ?
> > > >
> > >
> > >
> > > First, this is the inet_parse() function we're talking about, right?
> > >
> > > int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> > >
> > >
> > https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618
> >
> > Yes, that's right.
> >
> 
> 
> Thanks, just wanted to be sure.
> 
> The syntax it supports is not the same as what's needed for host forwarding.
> inet_parse: address:port,option1,option2 (where options are to=,ipv4,etc).
> hostfwd: address:port-address:port
> If we wanted to have a utility that parsed "address:port" for v4+v6 then
> we'd have to split the "address:port" part out of inet_parse.
> 
> Plus the way inet_parse() parses the address, which is fine for its
> purposes, is with sscanf.
> Alas the terminating character is not the same (',' vs '-').
> IWBN to retain passing sscanf a constant format string so that the compiler
> can catch various errors,
> and if one keeps that then any kind of refactor loses some appeal.
> [Though one could require all callers to accept either ',' or '-' as the
> delimiter.]

I think the key useful part to keep common impl for is the handling
of the [] brackets for IPv6 raw addrs. I'd suggest we try to pull the
"address:port" part out into a new inet_addr_parse() helper that can be
called from inet_pase and from slirp.

inet_parse() can split on the first ",", and then call inet_addr_parse
on the first segment.

slirp can split on "-", and call inet_addr_parse with both segments.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3]

[Doug Evans]

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

On Wed, Feb 10, 2021 at 8:49 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> > On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > > >
> > > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <
> berrange@redhat.com
> > > >
> > > > > wrote:
> > > > >
> > > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > > >> > Add support for ipv6 host forwarding
> > > > >> >
> > > > >> > This patchset takes the original patch from Maxim,
> > > > >> >
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > > > >> > and updates it.
> > > > >> >
> > > > >> > New option: -ipv6-hostfwd
> > > > >> >
> > > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > > >> >
> > > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > > >>
> > > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > > >> that we don't need to add any new commands/options. We can
> > > > >> use existing inet_parse() helper function to parse the address
> > > > >> info and transparently support IPv4/6 in the existing commands
> > > > >> and options. This matches normal practice elsewhere in QEMU
> > > > >> for IP dual stack.
> > > > >>
> > > > >
> > > > > I'm all for this, fwiw.
> > > > >
> > > >
> > > >
> > > > I should say I'm all for not adding new commands/options.
> > > > Looking at inet_parse() it cannot be used as-is.
> > > > The question then becomes: Will refactoring it buy enough?
> > >
> > > What's the problem your hitting with inet_parse ?
> > >
> >
> >
> > First, this is the inet_parse() function we're talking about, right?
> >
> > int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> >
> >
> https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618
>
> Yes, that's right.
>


Thanks, just wanted to be sure.

The syntax it supports is not the same as what's needed for host forwarding.
inet_parse: address:port,option1,option2 (where options are to=,ipv4,etc).
hostfwd: address:port-address:port
If we wanted to have a utility that parsed "address:port" for v4+v6 then
we'd have to split the "address:port" part out of inet_parse.

Plus the way inet_parse() parses the address, which is fine for its
purposes, is with sscanf.
Alas the terminating character is not the same (',' vs '-').
IWBN to retain passing sscanf a constant format string so that the compiler
can catch various errors,
and if one keeps that then any kind of refactor loses some appeal.
[Though one could require all callers to accept either ',' or '-' as the
delimiter.]

[-- Attachment #2: Type: text/html, Size: 5012 bytes --]

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 10, 2021 at 08:31:40AM -0800, Doug Evans wrote:
> On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> > >
> > > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com
> > >
> > > > wrote:
> > > >
> > > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > > >> > Add support for ipv6 host forwarding
> > > >> >
> > > >> > This patchset takes the original patch from Maxim,
> > > >> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > > >> > and updates it.
> > > >> >
> > > >> > New option: -ipv6-hostfwd
> > > >> >
> > > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > > >> >
> > > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > > >>
> > > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > > >> that we don't need to add any new commands/options. We can
> > > >> use existing inet_parse() helper function to parse the address
> > > >> info and transparently support IPv4/6 in the existing commands
> > > >> and options. This matches normal practice elsewhere in QEMU
> > > >> for IP dual stack.
> > > >>
> > > >
> > > > I'm all for this, fwiw.
> > > >
> > >
> > >
> > > I should say I'm all for not adding new commands/options.
> > > Looking at inet_parse() it cannot be used as-is.
> > > The question then becomes: Will refactoring it buy enough?
> >
> > What's the problem your hitting with inet_parse ?
> >
> 
> 
> First, this is the inet_parse() function we're talking about, right?
> 
> int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)
> 
> https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618

Yes, that's right.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3]

[Doug Evans]

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

On Wed, Feb 10, 2021 at 1:31 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> > On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> >
> > > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com
> >
> > > wrote:
> > >
> > >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > >> > Add support for ipv6 host forwarding
> > >> >
> > >> > This patchset takes the original patch from Maxim,
> > >> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > >> > and updates it.
> > >> >
> > >> > New option: -ipv6-hostfwd
> > >> >
> > >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> > >> >
> > >> > These are the ipv6 equivalents of their ipv4 counterparts.
> > >>
> > >> Before I noticed this v3, I send a reply to your v2 sugesting
> > >> that we don't need to add any new commands/options. We can
> > >> use existing inet_parse() helper function to parse the address
> > >> info and transparently support IPv4/6 in the existing commands
> > >> and options. This matches normal practice elsewhere in QEMU
> > >> for IP dual stack.
> > >>
> > >
> > > I'm all for this, fwiw.
> > >
> >
> >
> > I should say I'm all for not adding new commands/options.
> > Looking at inet_parse() it cannot be used as-is.
> > The question then becomes: Will refactoring it buy enough?
>
> What's the problem your hitting with inet_parse ?
>


First, this is the inet_parse() function we're talking about, right?

int inet_parse(InetSocketAddress *addr, const char *str, Error **errp)

https://gitlab.com/qemu-project/qemu/-/blob/master/util/qemu-sockets.c#L618

[-- Attachment #2: Type: text/html, Size: 3171 bytes --]

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Tue, Feb 09, 2021 at 06:16:57PM -0800, Doug Evans wrote:
> On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:
> 
> > On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> >> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> >> > Add support for ipv6 host forwarding
> >> >
> >> > This patchset takes the original patch from Maxim,
> >> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> >> > and updates it.
> >> >
> >> > New option: -ipv6-hostfwd
> >> >
> >> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> >> >
> >> > These are the ipv6 equivalents of their ipv4 counterparts.
> >>
> >> Before I noticed this v3, I send a reply to your v2 sugesting
> >> that we don't need to add any new commands/options. We can
> >> use existing inet_parse() helper function to parse the address
> >> info and transparently support IPv4/6 in the existing commands
> >> and options. This matches normal practice elsewhere in QEMU
> >> for IP dual stack.
> >>
> >
> > I'm all for this, fwiw.
> >
> 
> 
> I should say I'm all for not adding new commands/options.
> Looking at inet_parse() it cannot be used as-is.
> The question then becomes: Will refactoring it buy enough?

What's the problem your hitting with inet_parse ?

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3]

[Doug Evans]

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

On Thu, Feb 4, 2021 at 10:25 AM Doug Evans <dje@google.com> wrote:

> On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
>> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
>> > Add support for ipv6 host forwarding
>> >
>> > This patchset takes the original patch from Maxim,
>> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
>> > and updates it.
>> >
>> > New option: -ipv6-hostfwd
>> >
>> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
>> >
>> > These are the ipv6 equivalents of their ipv4 counterparts.
>>
>> Before I noticed this v3, I send a reply to your v2 sugesting
>> that we don't need to add any new commands/options. We can
>> use existing inet_parse() helper function to parse the address
>> info and transparently support IPv4/6 in the existing commands
>> and options. This matches normal practice elsewhere in QEMU
>> for IP dual stack.
>>
>
> I'm all for this, fwiw.
>


I should say I'm all for not adding new commands/options.
Looking at inet_parse() it cannot be used as-is.
The question then becomes: Will refactoring it buy enough?

[-- Attachment #2: Type: text/html, Size: 2174 bytes --]

Re: [PATCH v3 0/3]

[Doug Evans]

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

On Thu, Feb 4, 2021 at 2:03 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> > Add support for ipv6 host forwarding
> >
> > This patchset takes the original patch from Maxim,
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> > and updates it.
> >
> > New option: -ipv6-hostfwd
> >
> > New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> >
> > These are the ipv6 equivalents of their ipv4 counterparts.
>
> Before I noticed this v3, I send a reply to your v2 sugesting
> that we don't need to add any new commands/options. We can
> use existing inet_parse() helper function to parse the address
> info and transparently support IPv4/6 in the existing commands
> and options. This matches normal practice elsewhere in QEMU
> for IP dual stack.
>

I'm all for this, fwiw.

[-- Attachment #2: Type: text/html, Size: 1458 bytes --]

Re: [PATCH v3 0/3]

[Daniel P. Berrangé]

On Wed, Feb 03, 2021 at 03:35:36PM -0800, dje--- via wrote:
> Add support for ipv6 host forwarding
> 
> This patchset takes the original patch from Maxim,
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
> and updates it.
> 
> New option: -ipv6-hostfwd
> 
> New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove
> 
> These are the ipv6 equivalents of their ipv4 counterparts.

Before I noticed this v3, I send a reply to your v2 sugesting
that we don't need to add any new commands/options. We can
use existing inet_parse() helper function to parse the address
info and transparently support IPv4/6 in the existing commands
and options. This matches normal practice elsewhere in QEMU
for IP dual stack.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/3]

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210203233539.1990032-1-dje@google.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210203233539.1990032-1-dje@google.com
Subject: [PATCH v3 0/3]

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210203213729.1940893-1-dje@google.com -> patchew/20210203213729.1940893-1-dje@google.com
 * [new tag]         patchew/20210203233539.1990032-1-dje@google.com -> patchew/20210203233539.1990032-1-dje@google.com
Switched to a new branch 'test'
998dd12 net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands
e6a45e5 net/slirp.c: Refactor address parsing
6faf9a9 slirp: Placeholder for libslirp ipv6 hostfwd support

=== OUTPUT BEGIN ===
1/3 Checking commit 6faf9a93b1cf (slirp: Placeholder for libslirp ipv6 hostfwd support)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 2 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/3 Checking commit e6a45e50689f (net/slirp.c: Refactor address parsing)
3/3 Checking commit 998dd12e51aa (net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands)
ERROR: line over 90 characters
#145: FILE: net/slirp.c:748:
+        *errmsg = g_strdup_printf("Missing %s address separator after IPv6 address", kind);

total: 1 errors, 0 warnings, 250 lines checked

Patch 3/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210203233539.1990032-1-dje@google.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

[PATCH v3 0/3]

[dje--- via]

Add support for ipv6 host forwarding

This patchset takes the original patch from Maxim,
https://www.mail-archive.com/qemu-devel@nongnu.org/msg569573.html
and updates it.

New option: -ipv6-hostfwd

New commands: ipv6_hostfwd_add, ipv6_hostfwd_remove

These are the ipv6 equivalents of their ipv4 counterparts.

The libslirp part of the patch has been committed upstream,
and will require adding it to qemu.
https://gitlab.freedesktop.org/slirp/libslirp/-/commit/0624fbe7d39b5433d7084a5096d1effc1df74e39
[plus the subsequent merge commit]

Changes from v2:
- split out libslirp commit
- clarify spelling of ipv6 addresses in docs
- tighten parsing of ipv6 addresses

Change from v1:
- libslirp part is now upstream
- net/slirp.c changes split into two pieces (refactor, add ipv6)
- added docs

Doug Evans (3):
  slirp: Placeholder for libslirp ipv6 hostfwd support
  net/slirp.c: Refactor address parsing
  net: Add -ipv6-hostfwd option, ipv6_hostfwd_add/remove commands

 hmp-commands.hx     |  32 +++++
 include/net/slirp.h |   2 +
 net/slirp.c         | 310 +++++++++++++++++++++++++++++++++++---------
 qapi/net.json       |   4 +
 slirp               |   2 +-
 5 files changed, 285 insertions(+), 65 deletions(-)

-- 
2.30.0.365.g02bc693789-goog

[PATCH v3 0/3]

[Yordan Karadzhov (VMware)]

The patch-set implements new commandline options for KernelShark that
can be used to pre-select the CPU and Task plots to be shown, before
opening the GUI. The idea was suggested by Julia Lawall.

Changes is v2:
 - Fixing a bug in the parsing of the string representing the list of
   IDs, given to the command line options.

 - New Patch 3/3 that limits the number of CPU plots shown by default
   when KernelShark starts. 

Changes is v3:
 - Switching to using comma instead of space for separating the
   Ids (Pid or CPU) of the plots. This way we don't need to worry
   about quoting the arguments.

 - Adding to patch [2/3] logic to handle the case when the user uses
   the "--cpu" or "--pid" options multiple times.

Yordan Karadzhov (VMware) (3):
  kernel-shark: Add methods for selecting the plots to be shown
  kernel-shark: Add command line options for selecting plots to be shown
  kernel-shark: Set a maximum number of plots to be shown by default

 kernel-shark/src/KsGLWidget.cpp   | 13 +++++++++--
 kernel-shark/src/KsMainWindow.cpp | 39 +++++++++++++++++++++++++++++++
 kernel-shark/src/KsMainWindow.hpp |  4 ++++
 kernel-shark/src/KsUtils.cpp      | 24 +++++++++++++++++++
 kernel-shark/src/KsUtils.hpp      |  2 ++
 kernel-shark/src/kernelshark.cpp  | 34 ++++++++++++++++++++++++---
 6 files changed, 111 insertions(+), 5 deletions(-)

-- 
2.20.1

Re: [PATCH v3 0/3]

[Jeff King]

On Sun, Dec 16, 2018 at 01:57:56PM -0800, nbelakovski@gmail.com wrote:

> Finally got around to submitting latest changes.
> 
> I think this addresses all the feedback
> 
> The atom now returns the worktree path instead of '+'

Thanks, I think patch 1 is definitely going in the right direction.
There are a few issues I found in its implementation, but they should be
easy to fix (and won't affect the other patches).

I don't really have a strong opinion on the behavior of the other
patches.

-Peff

[PATCH v3 0/3]

[nbelakovski]

From: Nickolai Belakovski <nbelakovski@gmail.com>

Finally got around to submitting latest changes.

I think this addresses all the feedback

The atom now returns the worktree path instead of '+'

I stuck to cyan for the coloring, since it seemed most popular

I added one more change to display the worktree path in cyan for git branch -vvv
Not sure if it's in the best place, but it seemed like it would be nice to add
the path in the same color so that there's some visibility as to why a particular
branch is colored in cyan. If it proves to be controversial, I wouldn't want it to
hold up this series, we can skip it and I can move discussion to a separate thread
(or just forget it, as the case may be)

Travis CI results: https://travis-ci.org/nbelakovski/git/builds/468569102

Nickolai Belakovski (3):
  ref-filter: add worktreepath atom
  branch: Mark and color a branch differently if it is checked out in a
    linked worktree
  branch: Add an extra verbose output displaying worktree path for refs
    checked out in a linked worktree

 Documentation/git-for-each-ref.txt |  4 +++
 builtin/branch.c                   | 16 ++++++---
 ref-filter.c                       | 70 ++++++++++++++++++++++++++++++++++++++
 t/t3200-branch.sh                  |  8 ++---
 t/t3203-branch-output.sh           | 21 ++++++++++++
 t/t6302-for-each-ref-filter.sh     | 15 ++++++++
 6 files changed, 126 insertions(+), 8 deletions(-)

-- 
2.14.2

Re: [PATCH v3 0/3]

[Andy Shevchenko]

On Fri, 2016-08-19 at 14:26 +0800, Kefeng Wang wrote:
> 
> Make dw8250_set_termios() as the default set_termios callback
> > > > for 8250
> > > > dw uart, correct me
> > > > if I am wrong.
> > > > 
> > > > Then add ACPI support for uart on Hisilicon Hip05 soc, be
> > > > careful that
> > > > it is not 16500
> > > > compatible. Meanwhile, set dw8250_serial_out32 to keep
> > > > consistent
> > > > between serial_out
> > > > and serial_in in ACPI.
> > > 
> > > I will review it perhaps late this week.
> > 
> 
> Hi Andy, kindly ping...

Sorry for being a bit late, but here we are.


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 0/3]

[Kefeng Wang]

On 2016/8/10 13:36, Kefeng Wang wrote:
> 
> 
> On 2016/8/10 0:01, Andy Shevchenko wrote:
>> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>>> Make dw8250_set_termios() as the default set_termios callback for 8250
>>> dw uart, correct me
>>> if I am wrong.
>>>
>>> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
>>> it is not 16500
>>> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
>>> between serial_out
>>> and serial_in in ACPI.
>>
>> I will review it perhaps late this week.
> 

Hi Andy, kindly ping...


> Thanks.
> 
>> Though have a quick question, did you test it? I hope you have done.
> 
> Yes, I have already tested the patchset on D02 board.
> 
> Kefeng
> 
>>
>>>
>>> Change since v2:
>>> - Add a new patch to use new var dev in probe
>>> - Use built-in device properties to set device parameters for existing
>>> device probed by acpi,
>>>   suggested by Andy Shevchenko 
>>>
>>>
>>> Change since v1:
>>> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
>>> to the device
>>>   being probed and not a global search for whole DSDT (pointed by grae
>>> me.gregory@linaro.org)
>>>
>>> Kefeng Wang (3):
>>>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>>>     callback
>>>   serial: 8250_dw: Use new dev variable in probe
>>>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
>>>
>>>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
>>> -----------
>>>  1 file changed, 41 insertions(+), 30 deletions(-)
>>>
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 

Re: [PATCH v3 0/3]

[Kefeng Wang]

On 2016/8/10 0:01, Andy Shevchenko wrote:
> On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
>> Make dw8250_set_termios() as the default set_termios callback for 8250
>> dw uart, correct me
>> if I am wrong.
>>
>> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
>> it is not 16500
>> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
>> between serial_out
>> and serial_in in ACPI.
> 
> I will review it perhaps late this week.

Thanks.

> Though have a quick question, did you test it? I hope you have done.

Yes, I have already tested the patchset on D02 board.

Kefeng

> 
>>
>> Change since v2:
>> - Add a new patch to use new var dev in probe
>> - Use built-in device properties to set device parameters for existing
>> device probed by acpi,
>>   suggested by Andy Shevchenko 
>>
>>
>> Change since v1:
>> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
>> to the device
>>   being probed and not a global search for whole DSDT (pointed by grae
>> me.gregory@linaro.org)
>>
>> Kefeng Wang (3):
>>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>>     callback
>>   serial: 8250_dw: Use new dev variable in probe
>>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
>>
>>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
>> -----------
>>  1 file changed, 41 insertions(+), 30 deletions(-)
>>
> 

Re: [PATCH v3 0/3]

[Andy Shevchenko]

On Fri, 2016-07-15 at 19:01 +0800, Kefeng Wang wrote:
> Make dw8250_set_termios() as the default set_termios callback for 8250
> dw uart, correct me
> if I am wrong.
> 
> Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that
> it is not 16500
> compatible. Meanwhile, set dw8250_serial_out32 to keep consistent
> between serial_out
> and serial_in in ACPI.

I will review it perhaps late this week.
Though have a quick question, did you test it? I hope you have done.

> 
> Change since v2:
> - Add a new patch to use new var dev in probe
> - Use built-in device properties to set device parameters for existing
> device probed by acpi,
>   suggested by Andy Shevchenko 
> 
> 
> Change since v1:
> - Use acpi_match_device() instead of acpi_dev_found(), limit the check
> to the device
>   being probed and not a global search for whole DSDT (pointed by grae
> me.gregory@linaro.org)
> 
> Kefeng Wang (3):
>   serial: 8250_dw: make dw8250_set_termios as default set_termios
>     callback
>   serial: 8250_dw: Use new dev variable in probe
>   serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc
> 
>  drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++------
> -----------
>  1 file changed, 41 insertions(+), 30 deletions(-)
> 

-- 

Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH v3 0/3]

[Kefeng Wang]

Make dw8250_set_termios() as the default set_termios callback for 8250 dw uart, correct me
if I am wrong.

Then add ACPI support for uart on Hisilicon Hip05 soc, be careful that it is not 16500
compatible. Meanwhile, set dw8250_serial_out32 to keep consistent between serial_out
and serial_in in ACPI.

Change since v2:
- Add a new patch to use new var dev in probe
- Use built-in device properties to set device parameters for existing device probed by acpi,
  suggested by Andy Shevchenko 


Change since v1:
- Use acpi_match_device() instead of acpi_dev_found(), limit the check to the device
  being probed and not a global search for whole DSDT (pointed by graeme.gregory@linaro.org)

Kefeng Wang (3):
  serial: 8250_dw: make dw8250_set_termios as default set_termios
    callback
  serial: 8250_dw: Use new dev variable in probe
  serial: 8250_dw: add ACPI support for uart on Hisilicon Hip05 soc

 drivers/tty/serial/8250/8250_dw.c | 71 ++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 30 deletions(-)

-- 
1.7.12.4

[PATCH v3 0/3]

[Jacob Pan]

Minor syntax change based on Joe Perches comments.
https://lkml.org/lkml/2012/11/13/27
Missed the actual changes in v2.

We have done some experiment with idle injection on Intel platforms.
The idea is to use the increasingly power efficient package level
C-states for power capping and passive thermal control.

Documentation is included in the patch to explain the theory of
operation, performance implication, calibration, scalability, and user
interface. Please refer to the following file for more details.

Documentation/thermal/intel_powerclamp.txt

Arjan van de Ven created the original idea and driver, I have been
refining driver in hope that they can be to be useful beyond a proof
of concept.


Jacob Pan (3):
  tick: export nohz tick idle symbols for module use
  x86/nmi: export local_touch_nmi() symbol for modules
  PM: Introduce Intel PowerClamp Driver

 Documentation/thermal/intel_powerclamp.txt |  307 +++++++++++
 arch/x86/kernel/nmi.c                      |    1 +
 drivers/thermal/Kconfig                    |   10 +
 drivers/thermal/Makefile                   |    1 +
 drivers/thermal/intel_powerclamp.c         |  766 ++++++++++++++++++++++++++++
 kernel/time/tick-sched.c                   |    2 +
 6 files changed, 1087 insertions(+)
 create mode 100644 Documentation/thermal/intel_powerclamp.txt
 create mode 100644 drivers/thermal/intel_powerclamp.c

-- 
1.7.9.5

Re: [PATCH v3 0/3]

[Jeff King]

On Fri, Feb 17, 2012 at 11:27:26PM -0800, Junio C Hamano wrote:

> > I'd be happy if we just ignored SIGPIPE everywhere, but turned it on for
> > the log family.
> 
> Hmmmm, now you confused me...  What is special about the log family?  Do
> you mean "when we use pager"?  But then we are writing into the pager,
> which the user can make it exit, which in turn causes us to write into the
> pipe, so I would expect that we would want to ignore SIGPIPE --- ah, then
> we explicitly catch error in xwrite() and say die() which we do not want.

Sort of. I mentioned the log family because those are the commands that
tend to generate large amounts of input, and which are likely to hit
SIGPIPE. But let me elaborate on my thinking a bit.

There are basically three types of writing callsites in git:

  1. Careful calls to write() or fwrite().

     These are the majority of calls. In these, we check the return
     value of write() and die, return the error code, or whatever is
     appropriate for the callsite. SIGPIPE kills the program before the
     careful code gets the chance to make a decision about what to do.
     At best, this is a nuisance; even if the program is going to die(),
     it's likely that the custom code could produce a useful error
     message. At worst it causes bugs. For example, we may write to a
     subprocess that only needs part of our input to make a decision
     (e.g., some hooks and credential helpers). With SIGPIPE, we end up
     dying even though no error has occurred. Worse, these are often
     annoying heisenbugs that depend on the timing of write() and
     close() between the two processes.

  2. Non-careful calls of small, fixed-size output.

     Things like "git remote" will write some output to stdout via
     printf and not bother checking the return code. As the main purpose
     of the program is to create output, it's OK to be killed by
     SIGPIPE if it happens due to a write to stdout. But respecting
     SIGPIPE doesn't buy as all that much. It might save us from making
     a few write() calls that will go to nowhere, but most of the work
     has already been done by the time we are outputting.

     And it has a cost to the other careful write calls in the same
     program, because respecting SIGPIPE is a per-process thing. So for
     something like "git remote show foo", while we would like SIGPIPE
     to kick in for output to stdout, we would not want it for a pipe we
     opened to talk to ls-remote.

  3. Non-careful calls of large, streaming data.

     Commands like "git log" will non-carefully output to stdout, as
     well, but they will generate tons of data, consuming possibly
     minutes of CPU time (e.g., "git log -p"). If whoever is reading the
     output stops doing so, we really want to kill the program to avoid
     wasting CPU time.  In this instance, SIGPIPE is a big win.

     It still has the downside that careful calls in the same program
     will be subject to using SIGPIPE. For "log" and friends, this is
     probably OK with the current code, as we don't make a lot of pipes.
     But that is somewhat of an implementation detail. E.g., "git log
     -p" with external diff or textconv writes to a tempfile, and then
     runs a subprocess with the tempfile as input. But we could just as
     easily have used pipes, and may choose to do so in the future. You
     may even be able to trigger a convert_to_git filter in the current
     code, which does use pipes.

So basically, I find SIGPIPE to be a simplistic too-heavy hammer that
ends up affecting all writes to pipes, when in reality we are only
interested in affecting ones to some "main" output (usually stdout).
That works OK for small Unix-y programs like "head", but is overly
simplistic for something as big as git.

In an ideal world, we could set a per-descriptor version of SIGPIPE, and
just turn it on for stdout (or somehow find out which descriptor caused
the SIGPIPE after the fact). But that's not possible.

So our next best thing would be to actually check the results of
these non-careful writes. Unfortunately, this means either:

  a. wrapping every printf with a function that will appropriately die
     on error. This makes the code more cumbersome.

  or

  b. occasionally checking ferror(stdout) while doing long streams
     (e.g., checking it after each commit is written in git log, and
     aborting if we saw a write error). This is less cumbersome, but it
     does mean that errno may or may not still be accurate by the time
     we notice the error. So it's hard to reliably differentiate EPIPE
     from other errors. It would be nice, for example, to have git log
     exit silently on EPIPE, but properly print an error for something
     like ENOSPC. But perhaps that isn't a big deal, as I believe right
     now that we would silently ignore something like ENOSPC.

Less robust than that is to just ignore SIGPIPE in most git programs
(which don't benefit from it, and where it is only a liability), but
then manually enable it for the few that care (the log family, and
perhaps diff. Maybe things like "git tag -l", though that output tends
to be pretty small). But I think it would work OK in practice, because
those commands don't tend to make pipes other than the "main" output.
And it's quite easy to implement.

> So you want to let SIGPIPE silently kill us when the pager is in use; is
> that it?

That's an OK heuristic, as the pager being in use is a good sign that we
will generate long, streaming output. It has two downsides, though. One
is that it suffers from the too-heavy hammer of the preceding paragraph.
The other is that it only catches when _we_ create the pipe. You would
also want to catch something like:

  git rev-list | head

and stop the traversal. So I think it is less about whether a pager is
in use and more about whether we are creating long output that would
benefit from being cut off early. The pager is an indicator of that, but
it's not a perfect one; I think we'd do better to mark those spots
manually (i.e., by re-enabling SIGPIPE in commands that we deem
appropriate).

-Peff

Re: [PATCH v3 0/3]

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'd be happy if we just ignored SIGPIPE everywhere, but turned it on for
> the log family.

Hmmmm, now you confused me...  What is special about the log family?  Do
you mean "when we use pager"?  But then we are writing into the pager,
which the user can make it exit, which in turn causes us to write into the
pipe, so I would expect that we would want to ignore SIGPIPE --- ah, then
we explicitly catch error in xwrite() and say die() which we do not want.

So you want to let SIGPIPE silently kill us when the pager is in use; is
that it?

Re: [PATCH v3 0/3]

[Jeff King]

On Fri, Feb 17, 2012 at 03:28:51PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Thomas Rast <trast@student.ethz.ch> writes:
> > ...
> > I seem to be getting intermittent test failures, and every time the
> > failing tests are different, when these three are queued to 'pu'. I didn't
> > look for what goes wrong and how.
> 
> False alarm. I suspect that it is jb/required-filter topic that is causing
> intermittent failures from convert.c depending on the timing of how fast
> filter subprocess dies vs how fast we consume its result or something.
> 
> Repeatedly running t0021 like this:
> 
>     $ cd t
>     $ while sh t0021-conversion.sh ; do :; done
> 
> under load seems to make it fail every once in a while.
> 
> test_must_fail: died by signal: git add test.fc
> 
> Are we dying on SIGPIPE or something?

I would be unsurprised if that is the case. Joey Hess mentioned similar
issues with hooks a month or two ago. And I have been seeing
intermittent failures of t5541 under load that I traced back to SIGPIPE.
I've been meaning to dig further and come up with a good solution.
Here's some previous discussion:

  http://article.gmane.org/gmane.comp.version-control.git/186291

I'd be happy if we just ignored SIGPIPE everywhere, but turned it on for
the log family.

-Peff

Re: [PATCH v3 0/3]

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Thomas Rast <trast@student.ethz.ch> writes:
> ...
> I seem to be getting intermittent test failures, and every time the
> failing tests are different, when these three are queued to 'pu'. I didn't
> look for what goes wrong and how.

False alarm. I suspect that it is jb/required-filter topic that is causing
intermittent failures from convert.c depending on the timing of how fast
filter subprocess dies vs how fast we consume its result or something.

Repeatedly running t0021 like this:

    $ cd t
    $ while sh t0021-conversion.sh ; do :; done

under load seems to make it fail every once in a while.

test_must_fail: died by signal: git add test.fc

Are we dying on SIGPIPE or something?

Re: [PATCH v3 0/3]

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> There were actually more mistakes lurking :-( so I am resending the
> whole series.

Ok, will requeue.  The diff you attached to this cover letter looked at
least halfway sane, compared to the previous round ;-), though it is not
exactly clear what goes to lib-test-functions and what goes to lib-test
(for example, you moved test_expect_success to 'test-functions', but it
calls test_ok_ that is in 'test-lib', and test_ok_ is directly used by
test_perf in the new 't/perf/perf-lib.sh'), making it harder for people to
decide where to put their additions to the test infrastructure from now
on.  There needs a bit of description in the first patch to guide them.

I seem to be getting intermittent test failures, and every time the
failing tests are different, when these three are queued to 'pu'. I didn't
look for what goes wrong and how.

Thanks.

[PATCH v3 0/3]

[Thomas Rast]

Junio C Hamano wrote:
> > ---
> >  t/test-lib-functions.sh |  835 +++++++++++++++++++++++++++++++++++++++++++++++
> >  t/test-lib.sh           |  552 +-------------------------------
> >  2 files changed, 840 insertions(+), 547 deletions(-)
> >  create mode 100644 t/test-lib-functions.sh
> 
> I would have expected from the log description that the number of deleted
> lines would be about the same as the number of added lines, and the
> difference would primarily come from the addition of "include" aka "dot"
> ". ./test-lib-functions.sh" that becomes necessary in t/test-lib.sh, some
> boilerplate material at the beginning of the new file e.g. "#!/bin/sh",
> and copying (not moving) the same Copyright block to the new file.

There were actually more mistakes lurking :-( so I am resending the
whole series.  I also put in the copyright that you asked for.  I
verified the results by looking at the diff between a reverse git-show
for test-lib.sh and a forward git-show for test-lib-functions.sh,
which looks as follows:

  --- /dev/fd/63	2012-02-17 10:55:32.994197654 +0100
  +++ /dev/fd/62	2012-02-17 10:55:32.994197654 +0100
  @@ -9,17 +9,29 @@
       
       Signed-off-by: Thomas Rast <trast@student.ethz.ch>
   
  -diff --git b/t/test-lib.sh a/t/test-lib.sh
  -index 1da3f40..e28d5fd 100644
  ---- b/t/test-lib.sh
  -+++ a/t/test-lib.sh
  -@@ -223,9 +223,248 @@ die () {
  - GIT_EXIT_OK=
  - trap 'die' EXIT
  - 
  --# The user-facing functions are loaded from a separate file so that
  --# test_perf subshells can have them too
  --. "${TEST_DIRECTORY:-.}"/test-lib-functions.sh
  +diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
  +new file mode 100644
  +index 0000000..7b3b4be
  +--- /dev/null
  ++++ b/t/test-lib-functions.sh
  +@@ -0,0 +1,565 @@
  ++#!/bin/sh
  ++#
  ++# Copyright (c) 2005 Junio C Hamano
  ++#
  ++# This program is free software: you can redistribute it and/or modify
  ++# it under the terms of the GNU General Public License as published by
  ++# the Free Software Foundation, either version 2 of the License, or
  ++# (at your option) any later version.
  ++#
  ++# This program is distributed in the hope that it will be useful,
  ++# but WITHOUT ANY WARRANTY; without even the implied warranty of
  ++# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  ++# GNU General Public License for more details.
  ++#
  ++# You should have received a copy of the GNU General Public License
  ++# along with this program.  If not, see http://www.gnu.org/licenses/ .
  ++
   +# The semantics of the editor variables are that of invoking
   +# sh -c "$EDITOR \"$@\"" files ...
   +#
  @@ -192,7 +204,6 @@
   +	git config "$@"
   +}
   +
  -+
   +test_config_global () {
   +	test_when_finished "test_unconfig --global '$1'" &&
   +	git config --global "$@"
  @@ -262,13 +273,7 @@
   +	esac
   +	return 1
   +}
  - 
  - # You are not expected to call test_ok_ and test_failure_ directly, use
  - # the text_expect_* functions instead.
  -@@ -313,6 +552,313 @@ test_skip () {
  - 	esac
  - }
  - 
  ++
   +test_expect_failure () {
   +	test "$#" = 3 && { test_prereq=$1; shift; } || test_prereq=
   +	test "$#" = 2 ||
  @@ -575,7 +580,3 @@
   +		mv .git/hooks .git/hooks-disabled
   +	) || exit
   +}
  -+
  - test_done () {
  - 	GIT_EXIT_OK=t
  - 



Thomas Rast (3):
  Move the user-facing test library to test-lib-functions.sh
  Introduce a performance testing framework
  Add a performance test for git-grep

 Makefile                        |   22 +-
 t/Makefile                      |   43 ++-
 t/perf/.gitignore               |    2 +
 t/perf/Makefile                 |   15 +
 t/perf/README                   |  146 ++++++++++
 t/perf/aggregate.perl           |  166 +++++++++++
 t/perf/min_time.perl            |   21 ++
 t/perf/p0000-perf-lib-sanity.sh |   41 +++
 t/perf/p0001-rev-list.sh        |   17 ++
 t/perf/p7810-grep.sh            |   23 ++
 t/perf/perf-lib.sh              |  198 ++++++++++++++
 t/perf/run                      |   82 ++++++
 t/test-lib-functions.sh         |  565 ++++++++++++++++++++++++++++++++++++++
 t/test-lib.sh                   |  574 ++-------------------------------------
 14 files changed, 1363 insertions(+), 552 deletions(-)
 create mode 100644 t/perf/.gitignore
 create mode 100644 t/perf/Makefile
 create mode 100644 t/perf/README
 create mode 100755 t/perf/aggregate.perl
 create mode 100755 t/perf/min_time.perl
 create mode 100755 t/perf/p0000-perf-lib-sanity.sh
 create mode 100755 t/perf/p0001-rev-list.sh
 create mode 100755 t/perf/p7810-grep.sh
 create mode 100644 t/perf/perf-lib.sh
 create mode 100755 t/perf/run
 create mode 100644 t/test-lib-functions.sh

-- 
1.7.9.1.365.ge223f