{"thread":{"id":"55951","subject":"[PATCH 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","startedAt":"2021-06-17T11:22:11Z","lastAt":"2021-07-03T11:28:48Z","messageCount":33,"participants":["Ævar Arnfjörð Bjarmason","Andrei Rybak","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"427739","messageId":"patch-1.3-f4191088ac-20210617T111934Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-0000000000-20210617T111934Z-avarab@gmail.com","subject":"[PATCH 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T11:21:47Z","receivedAt":"2021-06-17T11:22:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak from the prefix_filename() function introduced with\nits use in 3b754eedd5 (bundle: use prefix_filename with bundle path,\n2017-03-20).\n\nAs noted in that commit the leak was intentional as a part of being\nsloppy about freeing resources just before we exit, I'm changing this\nbecause I'll be fixing other memory leaks in the bundle API (including\nthe library version) in subsequent commits. It's easier to reason\nabout those fixes if valgrind runs cleanly at the end without any\nleaks whatsoever.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 79 +++++++++++++++++++++++++++++++++---------------\n 1 file changed, 55 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex ea6948110b..7778297277 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,\n \t\tconst char* prefix,\n \t\tconst char * const usagestr[],\n \t\tconst struct option options[],\n-\t\tconst char **bundle_file) {\n+\t\tchar **bundle_file) {\n \tint newargc;\n \tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n@@ -61,7 +61,8 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \tint progress = isatty(STDERR_FILENO);\n \tstruct strvec pack_opts;\n \tint version = -1;\n-\n+\tint die_no_repo = 0;\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n \t\t\t N_(\"do not show progress meter\"), 0),\n@@ -76,7 +77,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \t\t\t N_(\"specify bundle format version\")),\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_create_usage, options, &bundle_file);\n@@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \tif (progress && all_progress_implied)\n \t\tstrvec_push(&pack_opts, \"--all-progress-implied\");\n \n-\tif (!startup_info->have_repository)\n+\tif (!startup_info->have_repository) {\n+\t\tdie_no_repo = 1;\n+\t\tgoto cleanup;\n+\t}\n+\tret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n+cleanup:\n+\tfree(bundle_file);\n+\tif (die_no_repo)\n \t\tdie(_(\"Need a repository to create a bundle.\"));\n-\treturn !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n+\treturn ret;\n }\n \n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n \tint quiet = 0;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_BOOL('q', \"quiet\", &quiet,\n \t\t\t N_(\"do not show bundle details\")),\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tclose(bundle_fd);\n-\tif (verify_bundle(the_repository, &header, !quiet))\n-\t\treturn 1;\n+\tif (verify_bundle(the_repository, &header, !quiet)) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \tfprintf(stderr, _(\"%s is okay\\n\"), bundle_file);\n-\treturn 0;\n+\tret = 0;\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tclose(bundle_fd);\n-\treturn !!list_bundle_refs(&header, argc, argv);\n+\tret = !!list_bundle_refs(&header, argc, argv);\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n-\n+\tint die_no_repo = 0;\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n-\tif (!startup_info->have_repository)\n-\t\tdie(_(\"Need a repository to unbundle.\"));\n-\treturn !!unbundle(the_repository, &header, bundle_fd, 0) ||\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n+\tif (!startup_info->have_repository) {\n+\t\tdie_no_repo = 1;\n+\t\tgoto cleanup;\n+\t}\n+\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n+cleanup:\n+\tif (die_no_repo)\n+\t\tdie(_(\"Need a repository to unbundle.\"));\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n int cmd_bundle(int argc, const char **argv, const char *prefix)\n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427740","messageId":"cover-0.3-0000000000-20210617T111934Z-avarab@gmail.com","threadId":"55951","inReplyTo":null,"subject":"[PATCH 0/3] bundle.c: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T11:21:46Z","receivedAt":"2021-06-17T11:22:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Converts the bundle.c code to use the string-list.c API, getting rid\nof some duplication in the codebase, while doing that stop the bundle\ncommand-line tool and its API from leaking memory in some common\ncases.\n\nÆvar Arnfjörð Bjarmason (3):\n bundle cmd: stop leaking memory from parse_options_cmd_bundle()\n bundle.c: use a temporary variable for OIDs and names\n bundle: remove \"ref_list\" in favor of string-list.c API\n\n builtin/bundle.c | 91 ++++++++++++++++++++++++++++++++----------------\n bundle.c | 72 +++++++++++++++++++++-----------------\n bundle.h | 20 +++++------\n transport.c | 11 ++++--\n 4 files changed, 119 insertions(+), 75 deletions(-)\n\n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427741","messageId":"patch-3.3-887313d3b0-20210617T111934Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-0000000000-20210617T111934Z-avarab@gmail.com","subject":"[PATCH 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T11:21:49Z","receivedAt":"2021-06-17T11:22:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move away from the \"struct ref_list\" in bundle.c in favor of the\nalmost identical string-list.c API.\n\nThat API fits this use-case perfectly, but did not exist in its\ncurrent form when this code was added in 2e0afafebd (Add git-bundle:\nmove objects and references by archive, 2007-02-22), with hindsight we\ncould have used the path-list API, which later got renamed to\nstring-list. See 8fd2cb4069 (Extract helper bits from\nc-merge-recursive work, 2006-07-25)\n\nWe need to change \"name\" to \"string\" and \"oid\" to \"util\" to make this\nconversion, but other than that the APIs are pretty much identical for\nwhat bundle.c made use of.\n\nLet's also replace the memset(..,0,...) pattern with a more idiomatic\n\"INIT\" macro, and finally add a *_release() function so to free the\nallocated memory.\n\nBefore this the add_to_ref_list() would leak memory, now e.g. \"bundle\nlist-heads\" reports no memory leaks at all under valgrind.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 12 ++++-----\n bundle.c | 64 ++++++++++++++++++++++++------------------------\n bundle.h | 20 +++++++--------\n transport.c | 10 +++++---\n 4 files changed, 55 insertions(+), 51 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 7778297277..8d82255280 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -106,7 +106,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n }\n \n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint quiet = 0;\n \tint ret;\n@@ -121,7 +121,6 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -136,11 +135,12 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tret = 0;\n cleanup:\n \tfree(bundle_file);\n+\tbundle_header_release(&header);\n \treturn ret;\n }\n \n static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n \tstruct option options[] = {\n@@ -152,7 +152,6 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -161,11 +160,12 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \tret = !!list_bundle_refs(&header, argc, argv);\n cleanup:\n \tfree(bundle_file);\n+\tbundle_header_release(&header);\n \treturn ret;\n }\n \n static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint die_no_repo = 0;\n \tint ret;\n@@ -178,7 +178,6 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -189,6 +188,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n+\tbundle_header_release(&header);\n cleanup:\n \tif (die_no_repo)\n \t\tdie(_(\"Need a repository to unbundle.\"));\ndiff --git a/bundle.c b/bundle.c\nindex 621708f40e..d36eeee1a5 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -23,15 +23,6 @@ static struct {\n \t{ 3, v3_bundle_signature },\n };\n \n-static void add_to_ref_list(const struct object_id *oid, const char *name,\n-\t\tstruct ref_list *list)\n-{\n-\tALLOC_GROW(list->list, list->nr + 1, list->alloc);\n-\toidcpy(&list->list[list->nr].oid, oid);\n-\tlist->list[list->nr].name = xstrdup(name);\n-\tlist->nr++;\n-}\n-\n static int parse_capability(struct bundle_header *header, const char *capability)\n {\n \tconst char *arg;\n@@ -79,7 +70,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t/* The bundle header ends with an empty line */\n \twhile (!strbuf_getwholeline_fd(&buf, fd, '\\n') &&\n \t buf.len && buf.buf[0] != '\\n') {\n-\t\tstruct object_id oid;\n+\t\tstruct object_id *oid;\n \t\tint is_prereq = 0;\n \t\tconst char *p;\n \n@@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\t * Prerequisites have object name that is optionally\n \t\t * followed by SP and subject line.\n \t\t */\n-\t\tif (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||\n+\t\toid = xmalloc(sizeof(struct object_id));\n+\t\tif (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||\n \t\t (*p && !isspace(*p)) ||\n \t\t (!is_prereq && !*p)) {\n \t\t\tif (report_path)\n \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n \t\t\t\t (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n \t\t\tstatus = -1;\n+\t\t\tfree(oid);\n \t\t\tbreak;\n \t\t} else {\n-\t\t\tif (is_prereq)\n-\t\t\t\tadd_to_ref_list(&oid, \"\", &header->prerequisites);\n-\t\t\telse\n-\t\t\t\tadd_to_ref_list(&oid, p + 1, &header->references);\n+\t\t\tconst char *string = is_prereq ? \"\" : p + 1;\n+\t\t\tstruct string_list *list = is_prereq\n+\t\t\t\t? &header->prerequisites\n+\t\t\t\t: &header->references;\n+\t\t\tstring_list_append(list, string)->util = oid;\n \t\t}\n \t}\n \n@@ -139,19 +133,19 @@ int read_bundle_header(const char *path, struct bundle_header *header)\n \n int is_bundle(const char *path, int quiet)\n {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint fd = open(path, O_RDONLY);\n \n \tif (fd < 0)\n \t\treturn 0;\n-\tmemset(&header, 0, sizeof(header));\n \tfd = parse_bundle_header(fd, &header, quiet ? NULL : path);\n \tif (fd >= 0)\n \t\tclose(fd);\n+\tbundle_header_release(&header);\n \treturn (fd >= 0);\n }\n \n-static int list_refs(struct ref_list *r, int argc, const char **argv)\n+static int list_refs(struct string_list *r, int argc, const char **argv)\n {\n \tint i;\n \n@@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \t\tif (argc > 1) {\n \t\t\tint j;\n \t\t\tfor (j = 1; j < argc; j++)\n-\t\t\t\tif (!strcmp(r->list[i].name, argv[j]))\n+\t\t\t\tif (!strcmp(r->items[i].string, argv[j]))\n \t\t\t\t\tbreak;\n \t\t\tif (j == argc)\n \t\t\t\tcontinue;\n \t\t}\n \n-\t\toid = &r->list[i].oid;\n-\t\tname = r->list[i].name;\n+\t\toid = r->items[i].util;\n+\t\tname = r->items[i].string;\n \t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n \t}\n \treturn 0;\n@@ -186,7 +180,7 @@ int verify_bundle(struct repository *r,\n \t * Do fast check, then if any prereqs are missing then go line by line\n \t * to be verbose about the errors\n \t */\n-\tstruct ref_list *p = &header->prerequisites;\n+\tstruct string_list *p = &header->prerequisites;\n \tstruct rev_info revs;\n \tconst char *argv[] = {NULL, \"--all\", NULL};\n \tstruct commit *commit;\n@@ -198,17 +192,17 @@ int verify_bundle(struct repository *r,\n \n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tstruct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n-\t\t\tadd_pending_object(&revs, o, e->name);\n+\t\t\tadd_pending_object(&revs, o, e->string);\n \t\t\tcontinue;\n \t\t}\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), e->string);\n \t}\n \tif (revs.pending.nr != p->nr)\n \t\treturn ret;\n@@ -224,28 +218,28 @@ int verify_bundle(struct repository *r,\n \t\t\ti--;\n \n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tconst struct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n \t\t\tcontinue;\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), e->string);\n \t}\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tstruct object_id *oid = e->util;\n \t\tcommit = lookup_commit_reference_gently(r, oid, 1);\n \t\tif (commit)\n \t\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n \t}\n \n \tif (verbose) {\n-\t\tstruct ref_list *r;\n+\t\tstruct string_list *r;\n \n \t\tr = &header->references;\n \t\tprintf_ln(Q_(\"The bundle contains this ref:\",\n@@ -582,3 +576,9 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \t\treturn error(_(\"index-pack died\"));\n \treturn 0;\n }\n+\n+void bundle_header_release(struct bundle_header *header)\n+{\n+\tstring_list_clear(&header->prerequisites, 1);\n+\tstring_list_clear(&header->references, 1);\n+}\ndiff --git a/bundle.h b/bundle.h\nindex f9e2d1c8ef..f98c1d24d9 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -3,22 +3,21 @@\n \n #include \"strvec.h\"\n #include \"cache.h\"\n-\n-struct ref_list {\n-\tunsigned int nr, alloc;\n-\tstruct ref_list_entry {\n-\t\tstruct object_id oid;\n-\t\tchar *name;\n-\t} *list;\n-};\n+#include \"string-list.h\"\n \n struct bundle_header {\n \tunsigned version;\n-\tstruct ref_list prerequisites;\n-\tstruct ref_list references;\n+\tstruct string_list prerequisites;\n+\tstruct string_list references;\n \tconst struct git_hash_algo *hash_algo;\n };\n \n+#define BUNDLE_HEADER_INIT \\\n+{ \\\n+\t.prerequisites = STRING_LIST_INIT_DUP, \\\n+\t.references = STRING_LIST_INIT_DUP, \\\n+}\n+\n int is_bundle(const char *path, int quiet);\n int read_bundle_header(const char *path, struct bundle_header *header);\n int create_bundle(struct repository *r, const char *path,\n@@ -30,5 +29,6 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, int flags);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n+void bundle_header_release(struct bundle_header *header);\n \n #endif\ndiff --git a/transport.c b/transport.c\nindex 9d601c8c95..667c4e0cc6 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -147,13 +147,14 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n-\t\tstruct ref_list_entry *e = data->header.references.list + i;\n-\t\tstruct ref *ref = alloc_ref(e->name);\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = data->header.references.items + i;\n+\t\tstruct ref *ref = alloc_ref(e->string);\n+\t\tconst struct object_id *oid = e->util;\n \t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n \t}\n+\tstring_list_clear(&data->header.references, 1);\n \treturn result;\n }\n \n@@ -176,6 +177,7 @@ static int close_bundle(struct transport *transport)\n \tstruct bundle_transport_data *data = transport->data;\n \tif (data->fd > 0)\n \t\tclose(data->fd);\n+\tbundle_header_release(&data->header);\n \tfree(data);\n \treturn 0;\n }\n@@ -1082,6 +1084,8 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tdie(_(\"git-over-rsync is no longer supported\"));\n \t} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n+\t\tstring_list_init(&data->header.prerequisites, 1);\n+\t\tstring_list_init(&data->header.references, 1);\n \t\ttransport_check_allowed(\"file\");\n \t\tret->data = data;\n \t\tret->vtable = &bundle_vtable;\n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427742","messageId":"patch-2.3-f297fd0432-20210617T111934Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-0000000000-20210617T111934Z-avarab@gmail.com","subject":"[PATCH 2/3] bundle.c: use a temporary variable for OIDs and names","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T11:21:48Z","receivedAt":"2021-06-17T11:22:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for moving away from accessing the OID and name via the\n\"oid\" and \"name\" slots in a subsequent commit, change the code that\naccesses it to use named variables. This makes the subsequent change\nsmaller.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bundle.c | 22 +++++++++++++++-------\n transport.c | 3 ++-\n 2 files changed, 17 insertions(+), 8 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 693d619551..621708f40e 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \tint i;\n \n \tfor (i = 0; i < r->nr; i++) {\n+\t\tstruct object_id *oid;\n+\t\tconst char *name;\n+\n \t\tif (argc > 1) {\n \t\t\tint j;\n \t\t\tfor (j = 1; j < argc; j++)\n@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \t\t\tif (j == argc)\n \t\t\t\tcontinue;\n \t\t}\n-\t\tprintf(\"%s %s\\n\", oid_to_hex(&r->list[i].oid),\n-\t\t\t\tr->list[i].name);\n+\n+\t\toid = &r->list[i].oid;\n+\t\tname = r->list[i].name;\n+\t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n \t}\n \treturn 0;\n }\n@@ -194,7 +199,8 @@ int verify_bundle(struct repository *r,\n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object *o = parse_object(r, &e->oid);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n \t\t\tadd_pending_object(&revs, o, e->name);\n@@ -202,7 +208,7 @@ int verify_bundle(struct repository *r,\n \t\t}\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n \t}\n \tif (revs.pending.nr != p->nr)\n \t\treturn ret;\n@@ -219,19 +225,21 @@ int verify_bundle(struct repository *r,\n \n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object *o = parse_object(r, &e->oid);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n \t\t\tcontinue;\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n \t}\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tcommit = lookup_commit_reference_gently(r, &e->oid, 1);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tcommit = lookup_commit_reference_gently(r, oid, 1);\n \t\tif (commit)\n \t\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n \t}\ndiff --git a/transport.c b/transport.c\nindex 50f5830eb6..9d601c8c95 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -149,7 +149,8 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \tfor (i = 0; i < data->header.references.nr; i++) {\n \t\tstruct ref_list_entry *e = data->header.references.list + i;\n \t\tstruct ref *ref = alloc_ref(e->name);\n-\t\toidcpy(&ref->old_oid, &e->oid);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n \t}\n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427934","messageId":"23c4ce5f-7769-1d2c-3a97-ac9733f73a25@gmail.com","threadId":"55951","inReplyTo":"patch-3.3-887313d3b0-20210617T111934Z-avarab@gmail.com","subject":"Re: [PATCH 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2021-06-19T02:12:45Z","receivedAt":"2021-06-19T02:12:55Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 17/06/2021 13:21, Ævar Arnfjörð Bjarmason wrote:\n> ---\n> builtin/bundle.c | 12 ++++-----\n> bundle.c | 64 ++++++++++++++++++++++++------------------------\n> bundle.h | 20 +++++++--------\n> transport.c | 10 +++++---\n> 4 files changed, 55 insertions(+), 51 deletions(-)\n> \n\n[snip]\n\n> diff --git a/bundle.c b/bundle.c\n> index 621708f40e..d36eeee1a5 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n> \t\tif (argc > 1) {\n> \t\t\tint j;\n> \t\t\tfor (j = 1; j < argc; j++)\n> -\t\t\t\tif (!strcmp(r->list[i].name, argv[j]))\n> +\t\t\t\tif (!strcmp(r->items[i].string, argv[j]))\n> \t\t\t\t\tbreak;\n> \t\t\tif (j == argc)\n> \t\t\t\tcontinue;\n> \t\t}\n> \n> -\t\toid = &r->list[i].oid;\n> -\t\tname = r->list[i].name;\n> +\t\toid = r->items[i].util;\n> +\t\tname = r->items[i].string;\n> \t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n\nIn function `list_refs` variable `name` that is used in a call to printf\nhas been extracted by the previous patch.\n\n> \t}\n> \treturn 0;\n> @@ -186,7 +180,7 @@ int verify_bundle(struct repository *r,\n> \t * Do fast check, then if any prereqs are missing then go line by line\n> \t * to be verbose about the errors\n> \t */\n> -\tstruct ref_list *p = &header->prerequisites;\n> +\tstruct string_list *p = &header->prerequisites;\n> \tstruct rev_info revs;\n> \tconst char *argv[] = {NULL, \"--all\", NULL};\n> \tstruct commit *commit;\n> @@ -198,17 +192,17 @@ int verify_bundle(struct repository *r,\n> \n> \trepo_init_revisions(r, &revs, NULL);\n> \tfor (i = 0; i < p->nr; i++) {\n> -\t\tstruct ref_list_entry *e = p->list + i;\n> -\t\tstruct object_id *oid = &e->oid;\n> +\t\tstruct string_list_item *e = p->items + i;\n> +\t\tstruct object_id *oid = e->util;\n> \t\tstruct object *o = parse_object(r, oid);\n> \t\tif (o) {\n> \t\t\to->flags |= PREREQ_MARK;\n> -\t\t\tadd_pending_object(&revs, o, e->name);\n> +\t\t\tadd_pending_object(&revs, o, e->string);\n> \t\t\tcontinue;\n> \t\t}\n> \t\tif (++ret == 1)\n> \t\t\terror(\"%s\", message);\n> -\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n> +\t\terror(\"%s %s\", oid_to_hex(oid), e->string);\n> \t}\n> \tif (revs.pending.nr != p->nr)\n> \t\treturn ret;\n> @@ -224,28 +218,28 @@ int verify_bundle(struct repository *r,\n> \t\t\ti--;\n> \n> \tfor (i = 0; i < p->nr; i++) {\n> -\t\tstruct ref_list_entry *e = p->list + i;\n> -\t\tstruct object_id *oid = &e->oid;\n> +\t\tstruct string_list_item *e = p->items + i;\n> +\t\tconst struct object_id *oid = e->util;\n> \t\tstruct object *o = parse_object(r, oid);\n> \t\tassert(o); /* otherwise we'd have returned early */\n> \t\tif (o->flags & SHOWN)\n> \t\t\tcontinue;\n> \t\tif (++ret == 1)\n> \t\t\terror(\"%s\", message);\n> -\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n> +\t\terror(\"%s %s\", oid_to_hex(oid), e->string);\n\nHowever, `e->name` in three places in function `verify_bundle` for two\ndifferent instances of `struct ref_list_entry *` wasn't extracted into\na variable by the previous patch. Could you please clarify this\ndiscrepancy?\n\n[snip]\n\n> diff --git a/transport.c b/transport.c\n> index 9d601c8c95..667c4e0cc6 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -147,13 +147,14 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n> \ttransport->hash_algo = data->header.hash_algo;\n> \n> \tfor (i = 0; i < data->header.references.nr; i++) {\n> -\t\tstruct ref_list_entry *e = data->header.references.list + i;\n> -\t\tstruct ref *ref = alloc_ref(e->name);\n> -\t\tstruct object_id *oid = &e->oid;\n> +\t\tstruct string_list_item *e = data->header.references.items + i;\n> +\t\tstruct ref *ref = alloc_ref(e->string);\n\nSimilar question for `e->name` here.\n\n> +\t\tconst struct object_id *oid = e->util;\n> \t\toidcpy(&ref->old_oid, oid);\n> \t\tref->next = result;\n> \t\tresult = ref;\n> \t}\n> +\tstring_list_clear(&data->header.references, 1);\n> \treturn result;\n> }\n> \n> @@ -176,6 +177,7 @@ static int close_bundle(struct transport *transport)\n> \tstruct bundle_transport_data *data = transport->data;\n> \tif (data->fd > 0)\n> \t\tclose(data->fd);\n> +\tbundle_header_release(&data->header);\n> \tfree(data);\n> \treturn 0;\n> }\n> @@ -1082,6 +1084,8 @@ struct transport *transport_get(struct remote *remote, const char *url)\n> \t\tdie(_(\"git-over-rsync is no longer supported\"));\n> \t} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {\n> \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n> +\t\tstring_list_init(&data->header.prerequisites, 1);\n> +\t\tstring_list_init(&data->header.references, 1);\n> \t\ttransport_check_allowed(\"file\");\n> \t\tret->data = data;\n> \t\tret->vtable = &bundle_vtable;\n> \n\n"},{"id":"428046","messageId":"cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-0000000000-20210617T111934Z-avarab@gmail.com","subject":"[PATCH v2 0/3] bundle.c: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:16:11Z","receivedAt":"2021-06-21T15:16:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This v2 addresses an omission noted by Andrei Rybak[1]. I didn't\nfactor out the \"name\" into a variable in 2/3 for ease of reading\n3/3. That's now done.\n\n1. https://lore.kernel.org/git/23c4ce5f-7769-1d2c-3a97-ac9733f73a25@gmail.com/\n\nÆvar Arnfjörð Bjarmason (3):\n bundle cmd: stop leaking memory from parse_options_cmd_bundle()\n bundle.c: use a temporary variable for OIDs and names\n bundle: remove \"ref_list\" in favor of string-list.c API\n\n builtin/bundle.c | 91 ++++++++++++++++++++++++++++++++----------------\n bundle.c | 74 ++++++++++++++++++++++-----------------\n bundle.h | 20 +++++------\n transport.c | 12 +++++--\n 4 files changed, 122 insertions(+), 75 deletions(-)\n\nRange-diff against v1:\n1: f4191088ac3 = 1: 932c0883ce0 bundle cmd: stop leaking memory from parse_options_cmd_bundle()\n2: f297fd0432a ! 2: 7e0d57951e5 bundle.c: use a temporary variable for OIDs and names\n @@ bundle.c: int verify_bundle(struct repository *r,\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n -\t\tstruct object *o = parse_object(r, &e->oid);\n ++\t\tconst char *name = e->name;\n +\t\tstruct object_id *oid = &e->oid;\n +\t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n - \t\t\tadd_pending_object(&revs, o, e->name);\n -@@ bundle.c: int verify_bundle(struct repository *r,\n +-\t\t\tadd_pending_object(&revs, o, e->name);\n ++\t\t\tadd_pending_object(&revs, o, name);\n + \t\t\tcontinue;\n \t\t}\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n -\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n -+\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n ++\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \tif (revs.pending.nr != p->nr)\n \t\treturn ret;\n @@ bundle.c: int verify_bundle(struct repository *r,\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n -\t\tstruct object *o = parse_object(r, &e->oid);\n ++\t\tconst char *name = e->name;\n +\t\tstruct object_id *oid = &e->oid;\n +\t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n @@ bundle.c: int verify_bundle(struct repository *r,\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n -\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n -+\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n ++\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \n \t/* Clean up objects used, as they will be reused. */\n @@ bundle.c: int verify_bundle(struct repository *r,\n \n ## transport.c ##\n @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,\n + \n \tfor (i = 0; i < data->header.references.nr; i++) {\n \t\tstruct ref_list_entry *e = data->header.references.list + i;\n - \t\tstruct ref *ref = alloc_ref(e->name);\n +-\t\tstruct ref *ref = alloc_ref(e->name);\n -\t\toidcpy(&ref->old_oid, &e->oid);\n ++\t\tconst char *name = e->name;\n ++\t\tstruct ref *ref = alloc_ref(name);\n +\t\tstruct object_id *oid = &e->oid;\n +\t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n3: 887313d3b02 ! 3: 9d9cb5aaf9e bundle: remove \"ref_list\" in favor of string-list.c API\n @@ bundle.c: int verify_bundle(struct repository *r,\n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n -\t\tstruct ref_list_entry *e = p->list + i;\n +-\t\tconst char *name = e->name;\n -\t\tstruct object_id *oid = &e->oid;\n +\t\tstruct string_list_item *e = p->items + i;\n ++\t\tconst char *name = e->string;\n +\t\tstruct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n --\t\t\tadd_pending_object(&revs, o, e->name);\n -+\t\t\tadd_pending_object(&revs, o, e->string);\n - \t\t\tcontinue;\n - \t\t}\n - \t\tif (++ret == 1)\n - \t\t\terror(\"%s\", message);\n --\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n -+\t\terror(\"%s %s\", oid_to_hex(oid), e->string);\n - \t}\n - \tif (revs.pending.nr != p->nr)\n - \t\treturn ret;\n @@ bundle.c: int verify_bundle(struct repository *r,\n \t\t\ti--;\n \n \tfor (i = 0; i < p->nr; i++) {\n -\t\tstruct ref_list_entry *e = p->list + i;\n +-\t\tconst char *name = e->name;\n -\t\tstruct object_id *oid = &e->oid;\n +\t\tstruct string_list_item *e = p->items + i;\n ++\t\tconst char *name = e->string;\n +\t\tconst struct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n - \t\t\tcontinue;\n - \t\tif (++ret == 1)\n - \t\t\terror(\"%s\", message);\n --\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n -+\t\terror(\"%s %s\", oid_to_hex(oid), e->string);\n - \t}\n +@@ bundle.c: int verify_bundle(struct repository *r,\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n -\t\tstruct ref_list_entry *e = data->header.references.list + i;\n --\t\tstruct ref *ref = alloc_ref(e->name);\n --\t\tstruct object_id *oid = &e->oid;\n +-\t\tconst char *name = e->name;\n +\t\tstruct string_list_item *e = data->header.references.items + i;\n -+\t\tstruct ref *ref = alloc_ref(e->string);\n -+\t\tconst struct object_id *oid = e->util;\n ++\t\tconst char *name = e->string;\n + \t\tstruct ref *ref = alloc_ref(name);\n +-\t\tstruct object_id *oid = &e->oid;\n ++\t\tstruct object_id *oid = e->util;\n \t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428047","messageId":"patch-2.3-7e0d57951e5-20210621T151357Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com","subject":"[PATCH v2 2/3] bundle.c: use a temporary variable for OIDs and names","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:16:13Z","receivedAt":"2021-06-21T15:16:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for moving away from accessing the OID and name via the\n\"oid\" and \"name\" slots in a subsequent commit, change the code that\naccesses it to use named variables. This makes the subsequent change\nsmaller.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bundle.c | 26 ++++++++++++++++++--------\n transport.c | 6 ++++--\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 693d6195514..7210e5e7105 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \tint i;\n \n \tfor (i = 0; i < r->nr; i++) {\n+\t\tstruct object_id *oid;\n+\t\tconst char *name;\n+\n \t\tif (argc > 1) {\n \t\t\tint j;\n \t\t\tfor (j = 1; j < argc; j++)\n@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \t\t\tif (j == argc)\n \t\t\t\tcontinue;\n \t\t}\n-\t\tprintf(\"%s %s\\n\", oid_to_hex(&r->list[i].oid),\n-\t\t\t\tr->list[i].name);\n+\n+\t\toid = &r->list[i].oid;\n+\t\tname = r->list[i].name;\n+\t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n \t}\n \treturn 0;\n }\n@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,\n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object *o = parse_object(r, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n-\t\t\tadd_pending_object(&revs, o, e->name);\n+\t\t\tadd_pending_object(&revs, o, name);\n \t\t\tcontinue;\n \t\t}\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \tif (revs.pending.nr != p->nr)\n \t\treturn ret;\n@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,\n \n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object *o = parse_object(r, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n \t\t\tcontinue;\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tcommit = lookup_commit_reference_gently(r, &e->oid, 1);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tcommit = lookup_commit_reference_gently(r, oid, 1);\n \t\tif (commit)\n \t\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n \t}\ndiff --git a/transport.c b/transport.c\nindex 50f5830eb6b..95c1138e9ae 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -148,8 +148,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n \t\tstruct ref_list_entry *e = data->header.references.list + i;\n-\t\tstruct ref *ref = alloc_ref(e->name);\n-\t\toidcpy(&ref->old_oid, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct ref *ref = alloc_ref(name);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n \t}\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428048","messageId":"patch-1.3-932c0883ce0-20210621T151357Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com","subject":"[PATCH v2 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:16:12Z","receivedAt":"2021-06-21T15:16:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak from the prefix_filename() function introduced with\nits use in 3b754eedd5 (bundle: use prefix_filename with bundle path,\n2017-03-20).\n\nAs noted in that commit the leak was intentional as a part of being\nsloppy about freeing resources just before we exit, I'm changing this\nbecause I'll be fixing other memory leaks in the bundle API (including\nthe library version) in subsequent commits. It's easier to reason\nabout those fixes if valgrind runs cleanly at the end without any\nleaks whatsoever.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 79 +++++++++++++++++++++++++++++++++---------------\n 1 file changed, 55 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex ea6948110b0..7778297277a 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,\n \t\tconst char* prefix,\n \t\tconst char * const usagestr[],\n \t\tconst struct option options[],\n-\t\tconst char **bundle_file) {\n+\t\tchar **bundle_file) {\n \tint newargc;\n \tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n@@ -61,7 +61,8 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \tint progress = isatty(STDERR_FILENO);\n \tstruct strvec pack_opts;\n \tint version = -1;\n-\n+\tint die_no_repo = 0;\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n \t\t\t N_(\"do not show progress meter\"), 0),\n@@ -76,7 +77,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \t\t\t N_(\"specify bundle format version\")),\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_create_usage, options, &bundle_file);\n@@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \tif (progress && all_progress_implied)\n \t\tstrvec_push(&pack_opts, \"--all-progress-implied\");\n \n-\tif (!startup_info->have_repository)\n+\tif (!startup_info->have_repository) {\n+\t\tdie_no_repo = 1;\n+\t\tgoto cleanup;\n+\t}\n+\tret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n+cleanup:\n+\tfree(bundle_file);\n+\tif (die_no_repo)\n \t\tdie(_(\"Need a repository to create a bundle.\"));\n-\treturn !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n+\treturn ret;\n }\n \n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n \tint quiet = 0;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_BOOL('q', \"quiet\", &quiet,\n \t\t\t N_(\"do not show bundle details\")),\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tclose(bundle_fd);\n-\tif (verify_bundle(the_repository, &header, !quiet))\n-\t\treturn 1;\n+\tif (verify_bundle(the_repository, &header, !quiet)) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \tfprintf(stderr, _(\"%s is okay\\n\"), bundle_file);\n-\treturn 0;\n+\tret = 0;\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tclose(bundle_fd);\n-\treturn !!list_bundle_refs(&header, argc, argv);\n+\tret = !!list_bundle_refs(&header, argc, argv);\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n-\n+\tint die_no_repo = 0;\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n-\tif (!startup_info->have_repository)\n-\t\tdie(_(\"Need a repository to unbundle.\"));\n-\treturn !!unbundle(the_repository, &header, bundle_fd, 0) ||\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n+\tif (!startup_info->have_repository) {\n+\t\tdie_no_repo = 1;\n+\t\tgoto cleanup;\n+\t}\n+\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n+cleanup:\n+\tif (die_no_repo)\n+\t\tdie(_(\"Need a repository to unbundle.\"));\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n int cmd_bundle(int argc, const char **argv, const char *prefix)\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428049","messageId":"patch-3.3-9d9cb5aaf9e-20210621T151357Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com","subject":"[PATCH v2 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:16:14Z","receivedAt":"2021-06-21T15:16:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move away from the \"struct ref_list\" in bundle.c in favor of the\nalmost identical string-list.c API.\n\nThat API fits this use-case perfectly, but did not exist in its\ncurrent form when this code was added in 2e0afafebd (Add git-bundle:\nmove objects and references by archive, 2007-02-22), with hindsight we\ncould have used the path-list API, which later got renamed to\nstring-list. See 8fd2cb4069 (Extract helper bits from\nc-merge-recursive work, 2006-07-25)\n\nWe need to change \"name\" to \"string\" and \"oid\" to \"util\" to make this\nconversion, but other than that the APIs are pretty much identical for\nwhat bundle.c made use of.\n\nLet's also replace the memset(..,0,...) pattern with a more idiomatic\n\"INIT\" macro, and finally add a *_release() function so to free the\nallocated memory.\n\nBefore this the add_to_ref_list() would leak memory, now e.g. \"bundle\nlist-heads\" reports no memory leaks at all under valgrind.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 12 +++++-----\n bundle.c | 62 ++++++++++++++++++++++++------------------------\n bundle.h | 20 ++++++++--------\n transport.c | 10 +++++---\n 4 files changed, 54 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 7778297277a..8d822552808 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -106,7 +106,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n }\n \n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint quiet = 0;\n \tint ret;\n@@ -121,7 +121,6 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -136,11 +135,12 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tret = 0;\n cleanup:\n \tfree(bundle_file);\n+\tbundle_header_release(&header);\n \treturn ret;\n }\n \n static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n \tstruct option options[] = {\n@@ -152,7 +152,6 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -161,11 +160,12 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \tret = !!list_bundle_refs(&header, argc, argv);\n cleanup:\n \tfree(bundle_file);\n+\tbundle_header_release(&header);\n \treturn ret;\n }\n \n static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint die_no_repo = 0;\n \tint ret;\n@@ -178,7 +178,6 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -189,6 +188,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t}\n \tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n+\tbundle_header_release(&header);\n cleanup:\n \tif (die_no_repo)\n \t\tdie(_(\"Need a repository to unbundle.\"));\ndiff --git a/bundle.c b/bundle.c\nindex 7210e5e7105..1c3ae9e02ea 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -23,15 +23,6 @@ static struct {\n \t{ 3, v3_bundle_signature },\n };\n \n-static void add_to_ref_list(const struct object_id *oid, const char *name,\n-\t\tstruct ref_list *list)\n-{\n-\tALLOC_GROW(list->list, list->nr + 1, list->alloc);\n-\toidcpy(&list->list[list->nr].oid, oid);\n-\tlist->list[list->nr].name = xstrdup(name);\n-\tlist->nr++;\n-}\n-\n static int parse_capability(struct bundle_header *header, const char *capability)\n {\n \tconst char *arg;\n@@ -79,7 +70,7 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t/* The bundle header ends with an empty line */\n \twhile (!strbuf_getwholeline_fd(&buf, fd, '\\n') &&\n \t buf.len && buf.buf[0] != '\\n') {\n-\t\tstruct object_id oid;\n+\t\tstruct object_id *oid;\n \t\tint is_prereq = 0;\n \t\tconst char *p;\n \n@@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\t * Prerequisites have object name that is optionally\n \t\t * followed by SP and subject line.\n \t\t */\n-\t\tif (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||\n+\t\toid = xmalloc(sizeof(struct object_id));\n+\t\tif (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||\n \t\t (*p && !isspace(*p)) ||\n \t\t (!is_prereq && !*p)) {\n \t\t\tif (report_path)\n \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n \t\t\t\t (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n \t\t\tstatus = -1;\n+\t\t\tfree(oid);\n \t\t\tbreak;\n \t\t} else {\n-\t\t\tif (is_prereq)\n-\t\t\t\tadd_to_ref_list(&oid, \"\", &header->prerequisites);\n-\t\t\telse\n-\t\t\t\tadd_to_ref_list(&oid, p + 1, &header->references);\n+\t\t\tconst char *string = is_prereq ? \"\" : p + 1;\n+\t\t\tstruct string_list *list = is_prereq\n+\t\t\t\t? &header->prerequisites\n+\t\t\t\t: &header->references;\n+\t\t\tstring_list_append(list, string)->util = oid;\n \t\t}\n \t}\n \n@@ -139,19 +133,19 @@ int read_bundle_header(const char *path, struct bundle_header *header)\n \n int is_bundle(const char *path, int quiet)\n {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint fd = open(path, O_RDONLY);\n \n \tif (fd < 0)\n \t\treturn 0;\n-\tmemset(&header, 0, sizeof(header));\n \tfd = parse_bundle_header(fd, &header, quiet ? NULL : path);\n \tif (fd >= 0)\n \t\tclose(fd);\n+\tbundle_header_release(&header);\n \treturn (fd >= 0);\n }\n \n-static int list_refs(struct ref_list *r, int argc, const char **argv)\n+static int list_refs(struct string_list *r, int argc, const char **argv)\n {\n \tint i;\n \n@@ -162,14 +156,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \t\tif (argc > 1) {\n \t\t\tint j;\n \t\t\tfor (j = 1; j < argc; j++)\n-\t\t\t\tif (!strcmp(r->list[i].name, argv[j]))\n+\t\t\t\tif (!strcmp(r->items[i].string, argv[j]))\n \t\t\t\t\tbreak;\n \t\t\tif (j == argc)\n \t\t\t\tcontinue;\n \t\t}\n \n-\t\toid = &r->list[i].oid;\n-\t\tname = r->list[i].name;\n+\t\toid = r->items[i].util;\n+\t\tname = r->items[i].string;\n \t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n \t}\n \treturn 0;\n@@ -186,7 +180,7 @@ int verify_bundle(struct repository *r,\n \t * Do fast check, then if any prereqs are missing then go line by line\n \t * to be verbose about the errors\n \t */\n-\tstruct ref_list *p = &header->prerequisites;\n+\tstruct string_list *p = &header->prerequisites;\n \tstruct rev_info revs;\n \tconst char *argv[] = {NULL, \"--all\", NULL};\n \tstruct commit *commit;\n@@ -198,9 +192,9 @@ int verify_bundle(struct repository *r,\n \n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tconst char *name = e->name;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tconst char *name = e->string;\n+\t\tstruct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n@@ -225,9 +219,9 @@ int verify_bundle(struct repository *r,\n \t\t\ti--;\n \n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tconst char *name = e->name;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tconst char *name = e->string;\n+\t\tconst struct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n@@ -239,15 +233,15 @@ int verify_bundle(struct repository *r,\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tstruct object_id *oid = e->util;\n \t\tcommit = lookup_commit_reference_gently(r, oid, 1);\n \t\tif (commit)\n \t\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n \t}\n \n \tif (verbose) {\n-\t\tstruct ref_list *r;\n+\t\tstruct string_list *r;\n \n \t\tr = &header->references;\n \t\tprintf_ln(Q_(\"The bundle contains this ref:\",\n@@ -584,3 +578,9 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \t\treturn error(_(\"index-pack died\"));\n \treturn 0;\n }\n+\n+void bundle_header_release(struct bundle_header *header)\n+{\n+\tstring_list_clear(&header->prerequisites, 1);\n+\tstring_list_clear(&header->references, 1);\n+}\ndiff --git a/bundle.h b/bundle.h\nindex f9e2d1c8ef5..f98c1d24d9a 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -3,22 +3,21 @@\n \n #include \"strvec.h\"\n #include \"cache.h\"\n-\n-struct ref_list {\n-\tunsigned int nr, alloc;\n-\tstruct ref_list_entry {\n-\t\tstruct object_id oid;\n-\t\tchar *name;\n-\t} *list;\n-};\n+#include \"string-list.h\"\n \n struct bundle_header {\n \tunsigned version;\n-\tstruct ref_list prerequisites;\n-\tstruct ref_list references;\n+\tstruct string_list prerequisites;\n+\tstruct string_list references;\n \tconst struct git_hash_algo *hash_algo;\n };\n \n+#define BUNDLE_HEADER_INIT \\\n+{ \\\n+\t.prerequisites = STRING_LIST_INIT_DUP, \\\n+\t.references = STRING_LIST_INIT_DUP, \\\n+}\n+\n int is_bundle(const char *path, int quiet);\n int read_bundle_header(const char *path, struct bundle_header *header);\n int create_bundle(struct repository *r, const char *path,\n@@ -30,5 +29,6 @@ int unbundle(struct repository *r, struct bundle_header *header,\n \t int bundle_fd, int flags);\n int list_bundle_refs(struct bundle_header *header,\n \t\tint argc, const char **argv);\n+void bundle_header_release(struct bundle_header *header);\n \n #endif\ndiff --git a/transport.c b/transport.c\nindex 95c1138e9ae..963655c772c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -147,14 +147,15 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n-\t\tstruct ref_list_entry *e = data->header.references.list + i;\n-\t\tconst char *name = e->name;\n+\t\tstruct string_list_item *e = data->header.references.items + i;\n+\t\tconst char *name = e->string;\n \t\tstruct ref *ref = alloc_ref(name);\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object_id *oid = e->util;\n \t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n \t}\n+\tstring_list_clear(&data->header.references, 1);\n \treturn result;\n }\n \n@@ -177,6 +178,7 @@ static int close_bundle(struct transport *transport)\n \tstruct bundle_transport_data *data = transport->data;\n \tif (data->fd > 0)\n \t\tclose(data->fd);\n+\tbundle_header_release(&data->header);\n \tfree(data);\n \treturn 0;\n }\n@@ -1083,6 +1085,8 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tdie(_(\"git-over-rsync is no longer supported\"));\n \t} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n+\t\tstring_list_init(&data->header.prerequisites, 1);\n+\t\tstring_list_init(&data->header.references, 1);\n \t\ttransport_check_allowed(\"file\");\n \t\tret->data = data;\n \t\tret->vtable = &bundle_vtable;\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428386","messageId":"YNS4wCjkp4yiLLBQ@coredump.intra.peff.net","threadId":"55951","inReplyTo":"patch-1.3-932c0883ce0-20210621T151357Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T16:54:24Z","receivedAt":"2021-06-24T16:54:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 21, 2021 at 05:16:12PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Fix a memory leak from the prefix_filename() function introduced with\n> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,\n> 2017-03-20).\n> \n> As noted in that commit the leak was intentional as a part of being\n> sloppy about freeing resources just before we exit, I'm changing this\n> because I'll be fixing other memory leaks in the bundle API (including\n> the library version) in subsequent commits. It's easier to reason\n> about those fixes if valgrind runs cleanly at the end without any\n> leaks whatsoever.\n\nLooking at that old commit, it seems like this is a good candidate for\njust inserting a single UNLEAK(bundle_file) into cmd_bundle(). But it\nlooks like the allocation has now migrated into all of the individual\nsub-command functions, so we have to deal with it multiple times. They\ncould still use UNLEAK() if you want to avoid the \"ret = foo(); free();\nreturn ret\" dance in each one, though.\n\nWe should avoid UNLEAK() in library-ish functions, but sub-commands that\nare just one step away from cmd_bundle() returning are OK uses, IMHO.\n\n> @@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n> \tif (progress && all_progress_implied)\n> \t\tstrvec_push(&pack_opts, \"--all-progress-implied\");\n> \n> -\tif (!startup_info->have_repository)\n> +\tif (!startup_info->have_repository) {\n> +\t\tdie_no_repo = 1;\n> +\t\tgoto cleanup;\n> +\t}\n> +\tret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n> +cleanup:\n> +\tfree(bundle_file);\n> +\tif (die_no_repo)\n> \t\tdie(_(\"Need a repository to create a bundle.\"));\n> -\treturn !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n> +\treturn ret;\n> }\n\nThis die_no_repo stuff confused me at first. But I think you are trying\nto make sure we call free(bundle_file) before die? There is no point in\nspending any effort on that, I think. When we exit() via die(), the\nvariable is still on the stack, and hence not leaked. And there are\nprobably a zillion other places we can hit a die() inside\ncreate_bundle() anyway, which would produce the same effect. There's not\nmuch point treating this one specially.\n\n-Peff\n"},{"id":"428389","messageId":"YNS8vzPc1KyRaSR5@coredump.intra.peff.net","threadId":"55951","inReplyTo":"patch-3.3-9d9cb5aaf9e-20210621T151357Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T17:11:27Z","receivedAt":"2021-06-24T17:11:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 21, 2021 at 05:16:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Move away from the \"struct ref_list\" in bundle.c in favor of the\n> almost identical string-list.c API.\n> \n> That API fits this use-case perfectly, but did not exist in its\n> current form when this code was added in 2e0afafebd (Add git-bundle:\n> move objects and references by archive, 2007-02-22), with hindsight we\n> could have used the path-list API, which later got renamed to\n> string-list. See 8fd2cb4069 (Extract helper bits from\n> c-merge-recursive work, 2006-07-25)\n\nI think this is a good direction, and I didn't see any errors in the\ncode. It's slightly sad that we end up with more lines than we started\nwith, but I think that's mostly because you're actually freeing the\nmemory now.\n\nTwo small nitpicks:\n\n> @@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n> \t\t * Prerequisites have object name that is optionally\n> \t\t * followed by SP and subject line.\n> \t\t */\n> -\t\tif (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||\n> +\t\toid = xmalloc(sizeof(struct object_id));\n> +\t\tif (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||\n> \t\t (*p && !isspace(*p)) ||\n> \t\t (!is_prereq && !*p)) {\n> \t\t\tif (report_path)\n> \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n> \t\t\t\t (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n> \t\t\tstatus = -1;\n> +\t\t\tfree(oid);\n> \t\t\tbreak;\n> \t\t} else {\n\nThis would be slightly simpler if you kept a local \"struct object_id\",\nand then called:\n\n string_list_append(list, string)->util = oiddup(&oid);\n\nlater when you know you want to save it. And then you don't have to\nworry about the extra cleanup here. That does require an extra oidcpy()\nunder the hood, but I suspect that is lost in the noise.\n\nI'm OK with it either way.\n\n> -\t\t\tif (is_prereq)\n> -\t\t\t\tadd_to_ref_list(&oid, \"\", &header->prerequisites);\n> -\t\t\telse\n> -\t\t\t\tadd_to_ref_list(&oid, p + 1, &header->references);\n> +\t\t\tconst char *string = is_prereq ? \"\" : p + 1;\n> +\t\t\tstruct string_list *list = is_prereq\n> +\t\t\t\t? &header->prerequisites\n> +\t\t\t\t: &header->references;\n> +\t\t\tstring_list_append(list, string)->util = oid;\n\nI'm usually a big fan of the ternary operator, and using variable\nindirection to make it clear that we always call a function. But here I\nthink it makes things more confusing. The two sides of the if/else are\nsufficiently simple that it's easy to see they both make the same\nfunction call. And because there are two variables, we check is_prereq\ntwice, making it much harder to see the two cases.\n\nI.e., I think:\n\n if (is_prereq)\n string_list_append(&header->prerequisites, \"\")->util = oid;\n else\n string_list_append(&header->references, p + 1)->util = oid;\n\nis much more obvious.\n\n-Peff\n"},{"id":"428390","messageId":"YNS9Yz+EtVzI+Ui7@coredump.intra.peff.net","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/3] bundle.c: remove \"ref_list\" in favor of string-list.c API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-24T17:14:11Z","receivedAt":"2021-06-24T17:14:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 21, 2021 at 05:16:11PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This v2 addresses an omission noted by Andrei Rybak[1]. I didn't\n> factor out the \"name\" into a variable in 2/3 for ease of reading\n> 3/3. That's now done.\n\nThis all looks OK to me. I left a few small comments on the patches\nthemselves.\n\nThe UNLEAK() thing I suggested for patch 1 does make that patch much\nsmaller and easier to read, but I suspect makes patch 3 harder (i.e.,\nyou are reusing the \"cleanup\" sections there to do the bundle header\nrelease. That could _also_ get UNLEAKed, but at some point it becomes\nmore clear to actually clean up after ourselves, and I think patch 3\nprobably crosses that point). So I'm OK to ignore that.\n\nI would prefer to see the \"die()\" thing I mentioned there addressed, as\nwell as the ternary thing from patch 3. But neither of them is incorrect\nas-is; it's just a style/preference thing.\n\n-Peff\n"},{"id":"428421","messageId":"87v963ys9x.fsf@evledraar.gmail.com","threadId":"55951","inReplyTo":"YNS4wCjkp4yiLLBQ@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-24T19:28:24Z","receivedAt":"2021-06-24T19:30:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 24 2021, Jeff King wrote:\n\n> On Mon, Jun 21, 2021 at 05:16:12PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Fix a memory leak from the prefix_filename() function introduced with\n>> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,\n>> 2017-03-20).\n>> \n>> As noted in that commit the leak was intentional as a part of being\n>> sloppy about freeing resources just before we exit, I'm changing this\n>> because I'll be fixing other memory leaks in the bundle API (including\n>> the library version) in subsequent commits. It's easier to reason\n>> about those fixes if valgrind runs cleanly at the end without any\n>> leaks whatsoever.\n>\n> Looking at that old commit, it seems like this is a good candidate for\n> just inserting a single UNLEAK(bundle_file) into cmd_bundle(). But it\n> looks like the allocation has now migrated into all of the individual\n> sub-command functions, so we have to deal with it multiple times. They\n> could still use UNLEAK() if you want to avoid the \"ret = foo(); free();\n> return ret\" dance in each one, though.\n>\n> We should avoid UNLEAK() in library-ish functions, but sub-commands that\n> are just one step away from cmd_bundle() returning are OK uses, IMHO.\n\nI'll change it if you feel strongly about it, but I always read UNLEAK()\nas \"ok, this is too hard, we won't bother, it's just a one-off\nbuilt-in\", and not necessarily a recommendation for a desired pattern.\n\nI think it's nice to have e.g. valgrind be able to report no leaks in\nthe binaries we build by default, not just if you compile with\n-DSUPPRESS_ANNOTATED_LEAKS.\n\n>> @@ -92,77 +93,107 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n>> \tif (progress && all_progress_implied)\n>> \t\tstrvec_push(&pack_opts, \"--all-progress-implied\");\n>> \n>> -\tif (!startup_info->have_repository)\n>> +\tif (!startup_info->have_repository) {\n>> +\t\tdie_no_repo = 1;\n>> +\t\tgoto cleanup;\n>> +\t}\n>> +\tret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n>> +cleanup:\n>> +\tfree(bundle_file);\n>> +\tif (die_no_repo)\n>> \t\tdie(_(\"Need a repository to create a bundle.\"));\n>> -\treturn !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n>> +\treturn ret;\n>> }\n>\n> This die_no_repo stuff confused me at first. But I think you are trying\n> to make sure we call free(bundle_file) before die? There is no point in\n> spending any effort on that, I think. When we exit() via die(), the\n> variable is still on the stack, and hence not leaked. And there are\n> probably a zillion other places we can hit a die() inside\n> create_bundle() anyway, which would produce the same effect. There's not\n> much point treating this one specially.\n\nRight, it's there just for the free(), and yeah, there's a bunch of\nplaces we'll leak anyway.\n\nI do think per the above with valgrind that there's value in making the\ncommon non-dying codepaths not leak though.\n"},{"id":"428422","messageId":"87sg17ys82.fsf@evledraar.gmail.com","threadId":"55951","inReplyTo":"YNS8vzPc1KyRaSR5@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-24T19:31:02Z","receivedAt":"2021-06-24T19:31:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 24 2021, Jeff King wrote:\n\n> On Mon, Jun 21, 2021 at 05:16:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Move away from the \"struct ref_list\" in bundle.c in favor of the\n>> almost identical string-list.c API.\n>> \n>> That API fits this use-case perfectly, but did not exist in its\n>> current form when this code was added in 2e0afafebd (Add git-bundle:\n>> move objects and references by archive, 2007-02-22), with hindsight we\n>> could have used the path-list API, which later got renamed to\n>> string-list. See 8fd2cb4069 (Extract helper bits from\n>> c-merge-recursive work, 2006-07-25)\n>\n> I think this is a good direction, and I didn't see any errors in the\n> code. It's slightly sad that we end up with more lines than we started\n> with, but I think that's mostly because you're actually freeing the\n> memory now.\n>\n> Two small nitpicks:\n>\n>> @@ -103,19 +94,22 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n>> \t\t * Prerequisites have object name that is optionally\n>> \t\t * followed by SP and subject line.\n>> \t\t */\n>> -\t\tif (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||\n>> +\t\toid = xmalloc(sizeof(struct object_id));\n>> +\t\tif (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||\n>> \t\t (*p && !isspace(*p)) ||\n>> \t\t (!is_prereq && !*p)) {\n>> \t\t\tif (report_path)\n>> \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n>> \t\t\t\t (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n>> \t\t\tstatus = -1;\n>> +\t\t\tfree(oid);\n>> \t\t\tbreak;\n>> \t\t} else {\n>\n> This would be slightly simpler if you kept a local \"struct object_id\",\n> and then called:\n>\n> string_list_append(list, string)->util = oiddup(&oid);\n>\n> later when you know you want to save it. And then you don't have to\n> worry about the extra cleanup here. That does require an extra oidcpy()\n> under the hood, but I suspect that is lost in the noise.\n>\n> I'm OK with it either way.\n\nThat sounds simpler indeed, thanks.\n\n>> -\t\t\tif (is_prereq)\n>> -\t\t\t\tadd_to_ref_list(&oid, \"\", &header->prerequisites);\n>> -\t\t\telse\n>> -\t\t\t\tadd_to_ref_list(&oid, p + 1, &header->references);\n>> +\t\t\tconst char *string = is_prereq ? \"\" : p + 1;\n>> +\t\t\tstruct string_list *list = is_prereq\n>> +\t\t\t\t? &header->prerequisites\n>> +\t\t\t\t: &header->references;\n>> +\t\t\tstring_list_append(list, string)->util = oid;\n>\n> I'm usually a big fan of the ternary operator, and using variable\n> indirection to make it clear that we always call a function. But here I\n> think it makes things more confusing. The two sides of the if/else are\n> sufficiently simple that it's easy to see they both make the same\n> function call. And because there are two variables, we check is_prereq\n> twice, making it much harder to see the two cases.\n>\n> I.e., I think:\n>\n> if (is_prereq)\n> string_list_append(&header->prerequisites, \"\")->util = oid;\n> else\n> string_list_append(&header->references, p + 1)->util = oid;\n>\n> is much more obvious.\n\nHah, that's actually the exact code I wrote to begin with, before\nthinking \"hrm, someone will probably say I should just use a ternary\nhere\". I'll change it back :)\n"},{"id":"428654","messageId":"xmqqh7hh4h5e.fsf@gitster.g","threadId":"55951","inReplyTo":"YNS8vzPc1KyRaSR5@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-29T01:02:37Z","receivedAt":"2021-06-29T01:04:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think this is a good direction, and I didn't see any errors in the\n> code. It's slightly sad that we end up with more lines than we started\n> with, but I think that's mostly because you're actually freeing the\n> memory now.\n> ...\n> I.e., I think:\n>\n> if (is_prereq)\n> string_list_append(&header->prerequisites, \"\")->util = oid;\n> else\n> string_list_append(&header->references, p + 1)->util = oid;\n>\n> is much more obvious.\n\nNicely done and nicely reviewed and improved.\n\nTogether with the \"no point in freeing just before dying\" on the\nearlier step, polishing this topic to incorporate the suggested\nchanges should be fairly an easy task. Let's not leave too many\nloose ends hanging around and close this one with the last final\nreroll (hopefully without \"I did this too while at it\" that meets\n\"oh, well, that is a bit controversi8al\" to drag it unnecessarily\nout).\n\nThanks.\n\n"},{"id":"428821","messageId":"cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com","subject":"[PATCH v3 0/3]","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T14:06:13Z","receivedAt":"2021-06-30T14:06:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the bundle API to use the string_list API instead of its own\nversion of a similar API. See [1] for v2.\n\nAddresses comments by Jeff King about us being too overzelous in\ntrying not to leak memory (the 'die_no_repo' is gone), and other\nflow/style comments of his.\n\nI also added a bundle_header_init() function for use in transport.c,\nand noticed a redundant call to string_list_clear() there.\n\n1. https://lore.kernel.org/git/cover-0.3-00000000000-20210621T151357Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (3):\n bundle cmd: stop leaking memory from parse_options_cmd_bundle()\n bundle.c: use a temporary variable for OIDs and names\n bundle: remove \"ref_list\" in favor of string-list.c API\n\n builtin/bundle.c | 74 ++++++++++++++++++++++++++++++------------------\n bundle.c | 65 ++++++++++++++++++++++++++----------------\n bundle.h | 21 +++++++-------\n transport.c | 10 +++++--\n 4 files changed, 105 insertions(+), 65 deletions(-)\n\nRange-diff against v2:\n1: 6a8b20a7cf3 < -: ----------- upload-pack: run is_repository_shallow() before setup_revisions()\n2: d88b2c04102 < -: ----------- revision.h: unify \"disable_stdin\" and \"read_from_stdin\"\n3: d433d7b24a3 < -: ----------- pack-objects.c: do stdin parsing via revision.c's API\n4: e59a06c3148 < -: ----------- pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n5: f4191088ac3 ! 1: 3d0d7a8e8b5 bundle cmd: stop leaking memory from parse_options_cmd_bundle()\n @@ Commit message\n about those fixes if valgrind runs cleanly at the end without any\n leaks whatsoever.\n \n + An earlier version of this change went out of its way to not leak\n + memory on the die() codepaths here, but that was deemed too verbose to\n + worry about in a built-in that's dying anyway. The only reason we'd\n + need that is to appease a mode like SANITIZE=leak within the scope of\n + an entire test file.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/bundle.c ##\n @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons\n \tstruct strvec pack_opts;\n \tint version = -1;\n -\n -+\tint die_no_repo = 0;\n +\tint ret;\n \tstruct option options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons\n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_create_usage, options, &bundle_file);\n @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n - \tif (progress && all_progress_implied)\n - \t\tstrvec_push(&pack_opts, \"--all-progress-implied\");\n \n --\tif (!startup_info->have_repository)\n -+\tif (!startup_info->have_repository) {\n -+\t\tdie_no_repo = 1;\n -+\t\tgoto cleanup;\n -+\t}\n -+\tret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n -+cleanup:\n -+\tfree(bundle_file);\n -+\tif (die_no_repo)\n + \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to create a bundle.\"));\n -\treturn !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n ++\tret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n ++\tfree(bundle_file);\n +\treturn ret;\n }\n \n @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n -\n -+\tint die_no_repo = 0;\n +\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n @@ builtin/bundle.c: static int cmd_bundle_create(int argc, const char **argv, cons\n \tmemset(&header, 0, sizeof(header));\n -\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n -\t\treturn 1;\n --\tif (!startup_info->have_repository)\n --\t\tdie(_(\"Need a repository to unbundle.\"));\n --\treturn !!unbundle(the_repository, &header, bundle_fd, 0) ||\n +\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n +\t\tret = 1;\n +\t\tgoto cleanup;\n +\t}\n -+\tif (!startup_info->have_repository) {\n -+\t\tdie_no_repo = 1;\n -+\t\tgoto cleanup;\n -+\t}\n + \tif (!startup_info->have_repository)\n + \t\tdie(_(\"Need a repository to unbundle.\"));\n +-\treturn !!unbundle(the_repository, &header, bundle_fd, 0) ||\n +\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n +cleanup:\n -+\tif (die_no_repo)\n -+\t\tdie(_(\"Need a repository to unbundle.\"));\n +\tfree(bundle_file);\n +\treturn ret;\n }\n6: f297fd0432a ! 2: e47646d3a98 bundle.c: use a temporary variable for OIDs and names\n @@ bundle.c: int verify_bundle(struct repository *r,\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n -\t\tstruct object *o = parse_object(r, &e->oid);\n ++\t\tconst char *name = e->name;\n +\t\tstruct object_id *oid = &e->oid;\n +\t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n - \t\t\tadd_pending_object(&revs, o, e->name);\n -@@ bundle.c: int verify_bundle(struct repository *r,\n +-\t\t\tadd_pending_object(&revs, o, e->name);\n ++\t\t\tadd_pending_object(&revs, o, name);\n + \t\t\tcontinue;\n \t\t}\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n -\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n -+\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n ++\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \tif (revs.pending.nr != p->nr)\n \t\treturn ret;\n @@ bundle.c: int verify_bundle(struct repository *r,\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n -\t\tstruct object *o = parse_object(r, &e->oid);\n ++\t\tconst char *name = e->name;\n +\t\tstruct object_id *oid = &e->oid;\n +\t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n @@ bundle.c: int verify_bundle(struct repository *r,\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n -\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n -+\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n ++\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \n \t/* Clean up objects used, as they will be reused. */\n @@ bundle.c: int verify_bundle(struct repository *r,\n \n ## transport.c ##\n @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,\n + \n \tfor (i = 0; i < data->header.references.nr; i++) {\n \t\tstruct ref_list_entry *e = data->header.references.list + i;\n - \t\tstruct ref *ref = alloc_ref(e->name);\n +-\t\tstruct ref *ref = alloc_ref(e->name);\n -\t\toidcpy(&ref->old_oid, &e->oid);\n ++\t\tconst char *name = e->name;\n ++\t\tstruct ref *ref = alloc_ref(name);\n +\t\tstruct object_id *oid = &e->oid;\n +\t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n7: 887313d3b02 ! 3: f1066ee1b9a bundle: remove \"ref_list\" in favor of string-list.c API\n @@ builtin/bundle.c: static int cmd_bundle_list_heads(int argc, const char **argv,\n -\tstruct bundle_header header;\n +\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n - \tint die_no_repo = 0;\n \tint ret;\n + \tstruct option options[] = {\n @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, co\n \t\tret = 1;\n \t\tgoto cleanup;\n @@ builtin/bundle.c: static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n - \t}\n + \t\tdie(_(\"Need a repository to unbundle.\"));\n \tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n +\tbundle_header_release(&header);\n cleanup:\n - \tif (die_no_repo)\n - \t\tdie(_(\"Need a repository to unbundle.\"));\n + \tfree(bundle_file);\n + \treturn ret;\n \n ## bundle.c ##\n @@ bundle.c: static struct {\n @@ bundle.c: static struct {\n \n -static void add_to_ref_list(const struct object_id *oid, const char *name,\n -\t\tstruct ref_list *list)\n --{\n ++void bundle_header_init(struct bundle_header *header)\n + {\n -\tALLOC_GROW(list->list, list->nr + 1, list->alloc);\n -\toidcpy(&list->list[list->nr].oid, oid);\n -\tlist->list[list->nr].name = xstrdup(name);\n -\tlist->nr++;\n --}\n --\n - static int parse_capability(struct bundle_header *header, const char *capability)\n - {\n - \tconst char *arg;\n -@@ bundle.c: static int parse_bundle_header(int fd, struct bundle_header *header,\n - \t/* The bundle header ends with an empty line */\n - \twhile (!strbuf_getwholeline_fd(&buf, fd, '\\n') &&\n - \t buf.len && buf.buf[0] != '\\n') {\n --\t\tstruct object_id oid;\n -+\t\tstruct object_id *oid;\n - \t\tint is_prereq = 0;\n - \t\tconst char *p;\n ++\tmemset(header, 0, sizeof(*header));\n ++\tstring_list_init(&header->prerequisites, 1);\n ++\tstring_list_init(&header->references, 1);\n ++}\n ++\n ++void bundle_header_release(struct bundle_header *header)\n ++{\n ++\tstring_list_clear(&header->prerequisites, 1);\n ++\tstring_list_clear(&header->references, 1);\n + }\n \n + static int parse_capability(struct bundle_header *header, const char *capability)\n @@ bundle.c: static int parse_bundle_header(int fd, struct bundle_header *header,\n - \t\t * Prerequisites have object name that is optionally\n - \t\t * followed by SP and subject line.\n - \t\t */\n --\t\tif (parse_oid_hex_algop(buf.buf, &oid, &p, header->hash_algo) ||\n -+\t\toid = xmalloc(sizeof(struct object_id));\n -+\t\tif (parse_oid_hex_algop(buf.buf, oid, &p, header->hash_algo) ||\n - \t\t (*p && !isspace(*p)) ||\n - \t\t (!is_prereq && !*p)) {\n - \t\t\tif (report_path)\n - \t\t\t\terror(_(\"unrecognized header: %s%s (%d)\"),\n - \t\t\t\t (is_prereq ? \"-\" : \"\"), buf.buf, (int)buf.len);\n \t\t\tstatus = -1;\n -+\t\t\tfree(oid);\n \t\t\tbreak;\n \t\t} else {\n --\t\t\tif (is_prereq)\n ++\t\t\tstruct object_id *dup = oiddup(&oid);\n + \t\t\tif (is_prereq)\n -\t\t\t\tadd_to_ref_list(&oid, \"\", &header->prerequisites);\n --\t\t\telse\n ++\t\t\t\tstring_list_append(&header->prerequisites, \"\")->util = dup;\n + \t\t\telse\n -\t\t\t\tadd_to_ref_list(&oid, p + 1, &header->references);\n -+\t\t\tconst char *string = is_prereq ? \"\" : p + 1;\n -+\t\t\tstruct string_list *list = is_prereq\n -+\t\t\t\t? &header->prerequisites\n -+\t\t\t\t: &header->references;\n -+\t\t\tstring_list_append(list, string)->util = oid;\n ++\t\t\t\tstring_list_append(&header->references, p + 1)->util = dup;\n \t\t}\n \t}\n \n @@ bundle.c: int verify_bundle(struct repository *r,\n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n -\t\tstruct ref_list_entry *e = p->list + i;\n +-\t\tconst char *name = e->name;\n -\t\tstruct object_id *oid = &e->oid;\n +\t\tstruct string_list_item *e = p->items + i;\n ++\t\tconst char *name = e->string;\n +\t\tstruct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n --\t\t\tadd_pending_object(&revs, o, e->name);\n -+\t\t\tadd_pending_object(&revs, o, e->string);\n - \t\t\tcontinue;\n - \t\t}\n - \t\tif (++ret == 1)\n - \t\t\terror(\"%s\", message);\n --\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n -+\t\terror(\"%s %s\", oid_to_hex(oid), e->string);\n - \t}\n - \tif (revs.pending.nr != p->nr)\n - \t\treturn ret;\n @@ bundle.c: int verify_bundle(struct repository *r,\n \t\t\ti--;\n \n \tfor (i = 0; i < p->nr; i++) {\n -\t\tstruct ref_list_entry *e = p->list + i;\n +-\t\tconst char *name = e->name;\n -\t\tstruct object_id *oid = &e->oid;\n +\t\tstruct string_list_item *e = p->items + i;\n ++\t\tconst char *name = e->string;\n +\t\tconst struct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n - \t\t\tcontinue;\n - \t\tif (++ret == 1)\n - \t\t\terror(\"%s\", message);\n --\t\terror(\"%s %s\", oid_to_hex(oid), e->name);\n -+\t\terror(\"%s %s\", oid_to_hex(oid), e->string);\n - \t}\n +@@ bundle.c: int verify_bundle(struct repository *r,\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n @@ bundle.c: int verify_bundle(struct repository *r,\n \n \t\tr = &header->references;\n \t\tprintf_ln(Q_(\"The bundle contains this ref:\",\n -@@ bundle.c: int unbundle(struct repository *r, struct bundle_header *header,\n - \t\treturn error(_(\"index-pack died\"));\n - \treturn 0;\n - }\n -+\n -+void bundle_header_release(struct bundle_header *header)\n -+{\n -+\tstring_list_clear(&header->prerequisites, 1);\n -+\tstring_list_clear(&header->references, 1);\n -+}\n \n ## bundle.h ##\n @@\n @@ bundle.h\n +\t.prerequisites = STRING_LIST_INIT_DUP, \\\n +\t.references = STRING_LIST_INIT_DUP, \\\n +}\n ++void bundle_header_init(struct bundle_header *header);\n ++void bundle_header_release(struct bundle_header *header);\n +\n int is_bundle(const char *path, int quiet);\n int read_bundle_header(const char *path, struct bundle_header *header);\n int create_bundle(struct repository *r, const char *path,\n -@@ bundle.h: int unbundle(struct repository *r, struct bundle_header *header,\n - \t int bundle_fd, int flags);\n - int list_bundle_refs(struct bundle_header *header,\n - \t\tint argc, const char **argv);\n -+void bundle_header_release(struct bundle_header *header);\n - \n - #endif\n \n ## transport.c ##\n @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport,\n @@ transport.c: static struct ref *get_refs_from_bundle(struct transport *transport\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n -\t\tstruct ref_list_entry *e = data->header.references.list + i;\n --\t\tstruct ref *ref = alloc_ref(e->name);\n --\t\tstruct object_id *oid = &e->oid;\n +-\t\tconst char *name = e->name;\n +\t\tstruct string_list_item *e = data->header.references.items + i;\n -+\t\tstruct ref *ref = alloc_ref(e->string);\n -+\t\tconst struct object_id *oid = e->util;\n ++\t\tconst char *name = e->string;\n + \t\tstruct ref *ref = alloc_ref(name);\n +-\t\tstruct object_id *oid = &e->oid;\n ++\t\tstruct object_id *oid = e->util;\n \t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n - \t}\n -+\tstring_list_clear(&data->header.references, 1);\n - \treturn result;\n - }\n - \n @@ transport.c: static int close_bundle(struct transport *transport)\n \tstruct bundle_transport_data *data = transport->data;\n \tif (data->fd > 0)\n @@ transport.c: struct transport *transport_get(struct remote *remote, const char *\n \t\tdie(_(\"git-over-rsync is no longer supported\"));\n \t} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n -+\t\tstring_list_init(&data->header.prerequisites, 1);\n -+\t\tstring_list_init(&data->header.references, 1);\n ++\t\tbundle_header_init(&data->header);\n \t\ttransport_check_allowed(\"file\");\n \t\tret->data = data;\n \t\tret->vtable = &bundle_vtable;\n-- \n2.32.0.613.g8e17abc2eb\n\n"},{"id":"428822","messageId":"patch-1.3-3d0d7a8e8b5-20210630T140339Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com","subject":"[PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T14:06:14Z","receivedAt":"2021-06-30T14:06:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak from the prefix_filename() function introduced with\nits use in 3b754eedd5 (bundle: use prefix_filename with bundle path,\n2017-03-20).\n\nAs noted in that commit the leak was intentional as a part of being\nsloppy about freeing resources just before we exit, I'm changing this\nbecause I'll be fixing other memory leaks in the bundle API (including\nthe library version) in subsequent commits. It's easier to reason\nabout those fixes if valgrind runs cleanly at the end without any\nleaks whatsoever.\n\nAn earlier version of this change went out of its way to not leak\nmemory on the die() codepaths here, but that was deemed too verbose to\nworry about in a built-in that's dying anyway. The only reason we'd\nneed that is to appease a mode like SANITIZE=leak within the scope of\nan entire test file.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 62 ++++++++++++++++++++++++++++++++----------------\n 1 file changed, 41 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex ea6948110b0..15e2bd61965 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,\n \t\tconst char* prefix,\n \t\tconst char * const usagestr[],\n \t\tconst struct option options[],\n-\t\tconst char **bundle_file) {\n+\t\tchar **bundle_file) {\n \tint newargc;\n \tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n@@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \tint progress = isatty(STDERR_FILENO);\n \tstruct strvec pack_opts;\n \tint version = -1;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n \t\t\t N_(\"do not show progress meter\"), 0),\n@@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \t\t\t N_(\"specify bundle format version\")),\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_create_usage, options, &bundle_file);\n@@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to create a bundle.\"));\n-\treturn !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n+\tret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n \tint quiet = 0;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_BOOL('q', \"quiet\", &quiet,\n \t\t\t N_(\"do not show bundle details\")),\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tclose(bundle_fd);\n-\tif (verify_bundle(the_repository, &header, !quiet))\n-\t\treturn 1;\n+\tif (verify_bundle(the_repository, &header, !quiet)) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \tfprintf(stderr, _(\"%s is okay\\n\"), bundle_file);\n-\treturn 0;\n+\tret = 0;\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tclose(bundle_fd);\n-\treturn !!list_bundle_refs(&header, argc, argv);\n+\tret = !!list_bundle_refs(&header, argc, argv);\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n-\treturn !!unbundle(the_repository, &header, bundle_fd, 0) ||\n+\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n int cmd_bundle(int argc, const char **argv, const char *prefix)\n-- \n2.32.0.613.g8e17abc2eb\n\n"},{"id":"428823","messageId":"patch-2.3-e47646d3a98-20210630T140339Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com","subject":"[PATCH v3 2/3] bundle.c: use a temporary variable for OIDs and names","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T14:06:15Z","receivedAt":"2021-06-30T14:06:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for moving away from accessing the OID and name via the\n\"oid\" and \"name\" slots in a subsequent commit, change the code that\naccesses it to use named variables. This makes the subsequent change\nsmaller.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bundle.c | 26 ++++++++++++++++++--------\n transport.c | 6 ++++--\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 693d6195514..7210e5e7105 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \tint i;\n \n \tfor (i = 0; i < r->nr; i++) {\n+\t\tstruct object_id *oid;\n+\t\tconst char *name;\n+\n \t\tif (argc > 1) {\n \t\t\tint j;\n \t\t\tfor (j = 1; j < argc; j++)\n@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \t\t\tif (j == argc)\n \t\t\t\tcontinue;\n \t\t}\n-\t\tprintf(\"%s %s\\n\", oid_to_hex(&r->list[i].oid),\n-\t\t\t\tr->list[i].name);\n+\n+\t\toid = &r->list[i].oid;\n+\t\tname = r->list[i].name;\n+\t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n \t}\n \treturn 0;\n }\n@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,\n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object *o = parse_object(r, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n-\t\t\tadd_pending_object(&revs, o, e->name);\n+\t\t\tadd_pending_object(&revs, o, name);\n \t\t\tcontinue;\n \t\t}\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \tif (revs.pending.nr != p->nr)\n \t\treturn ret;\n@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,\n \n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object *o = parse_object(r, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n \t\t\tcontinue;\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tcommit = lookup_commit_reference_gently(r, &e->oid, 1);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tcommit = lookup_commit_reference_gently(r, oid, 1);\n \t\tif (commit)\n \t\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n \t}\ndiff --git a/transport.c b/transport.c\nindex 50f5830eb6b..95c1138e9ae 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -148,8 +148,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n \t\tstruct ref_list_entry *e = data->header.references.list + i;\n-\t\tstruct ref *ref = alloc_ref(e->name);\n-\t\toidcpy(&ref->old_oid, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct ref *ref = alloc_ref(name);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n \t}\n-- \n2.32.0.613.g8e17abc2eb\n\n"},{"id":"428824","messageId":"patch-3.3-f1066ee1b9a-20210630T140339Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com","subject":"[PATCH v3 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T14:06:16Z","receivedAt":"2021-06-30T14:06:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move away from the \"struct ref_list\" in bundle.c in favor of the\nalmost identical string-list.c API.\n\nThat API fits this use-case perfectly, but did not exist in its\ncurrent form when this code was added in 2e0afafebd (Add git-bundle:\nmove objects and references by archive, 2007-02-22), with hindsight we\ncould have used the path-list API, which later got renamed to\nstring-list. See 8fd2cb4069 (Extract helper bits from\nc-merge-recursive work, 2006-07-25)\n\nWe need to change \"name\" to \"string\" and \"oid\" to \"util\" to make this\nconversion, but other than that the APIs are pretty much identical for\nwhat bundle.c made use of.\n\nLet's also replace the memset(..,0,...) pattern with a more idiomatic\n\"INIT\" macro, and finally add a *_release() function so to free the\nallocated memory.\n\nBefore this the add_to_ref_list() would leak memory, now e.g. \"bundle\nlist-heads\" reports no memory leaks at all under valgrind.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 12 +++++------\n bundle.c | 53 ++++++++++++++++++++++++++----------------------\n bundle.h | 21 ++++++++++---------\n transport.c | 8 +++++---\n 4 files changed, 51 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 15e2bd61965..053a51bea1b 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -100,7 +100,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n }\n \n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint quiet = 0;\n \tint ret;\n@@ -115,7 +115,6 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -130,11 +129,12 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tret = 0;\n cleanup:\n \tfree(bundle_file);\n+\tbundle_header_release(&header);\n \treturn ret;\n }\n \n static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n \tstruct option options[] = {\n@@ -146,7 +146,6 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -155,11 +154,12 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \tret = !!list_bundle_refs(&header, argc, argv);\n cleanup:\n \tfree(bundle_file);\n+\tbundle_header_release(&header);\n \treturn ret;\n }\n \n static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n \tstruct option options[] = {\n@@ -171,7 +171,6 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -180,6 +179,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n \tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n+\tbundle_header_release(&header);\n cleanup:\n \tfree(bundle_file);\n \treturn ret;\ndiff --git a/bundle.c b/bundle.c\nindex 7210e5e7105..cd5356a66f3 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -23,13 +23,17 @@ static struct {\n \t{ 3, v3_bundle_signature },\n };\n \n-static void add_to_ref_list(const struct object_id *oid, const char *name,\n-\t\tstruct ref_list *list)\n+void bundle_header_init(struct bundle_header *header)\n {\n-\tALLOC_GROW(list->list, list->nr + 1, list->alloc);\n-\toidcpy(&list->list[list->nr].oid, oid);\n-\tlist->list[list->nr].name = xstrdup(name);\n-\tlist->nr++;\n+\tmemset(header, 0, sizeof(*header));\n+\tstring_list_init(&header->prerequisites, 1);\n+\tstring_list_init(&header->references, 1);\n+}\n+\n+void bundle_header_release(struct bundle_header *header)\n+{\n+\tstring_list_clear(&header->prerequisites, 1);\n+\tstring_list_clear(&header->references, 1);\n }\n \n static int parse_capability(struct bundle_header *header, const char *capability)\n@@ -112,10 +116,11 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\t\tstatus = -1;\n \t\t\tbreak;\n \t\t} else {\n+\t\t\tstruct object_id *dup = oiddup(&oid);\n \t\t\tif (is_prereq)\n-\t\t\t\tadd_to_ref_list(&oid, \"\", &header->prerequisites);\n+\t\t\t\tstring_list_append(&header->prerequisites, \"\")->util = dup;\n \t\t\telse\n-\t\t\t\tadd_to_ref_list(&oid, p + 1, &header->references);\n+\t\t\t\tstring_list_append(&header->references, p + 1)->util = dup;\n \t\t}\n \t}\n \n@@ -139,19 +144,19 @@ int read_bundle_header(const char *path, struct bundle_header *header)\n \n int is_bundle(const char *path, int quiet)\n {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint fd = open(path, O_RDONLY);\n \n \tif (fd < 0)\n \t\treturn 0;\n-\tmemset(&header, 0, sizeof(header));\n \tfd = parse_bundle_header(fd, &header, quiet ? NULL : path);\n \tif (fd >= 0)\n \t\tclose(fd);\n+\tbundle_header_release(&header);\n \treturn (fd >= 0);\n }\n \n-static int list_refs(struct ref_list *r, int argc, const char **argv)\n+static int list_refs(struct string_list *r, int argc, const char **argv)\n {\n \tint i;\n \n@@ -162,14 +167,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \t\tif (argc > 1) {\n \t\t\tint j;\n \t\t\tfor (j = 1; j < argc; j++)\n-\t\t\t\tif (!strcmp(r->list[i].name, argv[j]))\n+\t\t\t\tif (!strcmp(r->items[i].string, argv[j]))\n \t\t\t\t\tbreak;\n \t\t\tif (j == argc)\n \t\t\t\tcontinue;\n \t\t}\n \n-\t\toid = &r->list[i].oid;\n-\t\tname = r->list[i].name;\n+\t\toid = r->items[i].util;\n+\t\tname = r->items[i].string;\n \t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n \t}\n \treturn 0;\n@@ -186,7 +191,7 @@ int verify_bundle(struct repository *r,\n \t * Do fast check, then if any prereqs are missing then go line by line\n \t * to be verbose about the errors\n \t */\n-\tstruct ref_list *p = &header->prerequisites;\n+\tstruct string_list *p = &header->prerequisites;\n \tstruct rev_info revs;\n \tconst char *argv[] = {NULL, \"--all\", NULL};\n \tstruct commit *commit;\n@@ -198,9 +203,9 @@ int verify_bundle(struct repository *r,\n \n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tconst char *name = e->name;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tconst char *name = e->string;\n+\t\tstruct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n@@ -225,9 +230,9 @@ int verify_bundle(struct repository *r,\n \t\t\ti--;\n \n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tconst char *name = e->name;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tconst char *name = e->string;\n+\t\tconst struct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n@@ -239,15 +244,15 @@ int verify_bundle(struct repository *r,\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tstruct object_id *oid = e->util;\n \t\tcommit = lookup_commit_reference_gently(r, oid, 1);\n \t\tif (commit)\n \t\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n \t}\n \n \tif (verbose) {\n-\t\tstruct ref_list *r;\n+\t\tstruct string_list *r;\n \n \t\tr = &header->references;\n \t\tprintf_ln(Q_(\"The bundle contains this ref:\",\ndiff --git a/bundle.h b/bundle.h\nindex f9e2d1c8ef5..1927d8cd6a4 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -3,22 +3,23 @@\n \n #include \"strvec.h\"\n #include \"cache.h\"\n-\n-struct ref_list {\n-\tunsigned int nr, alloc;\n-\tstruct ref_list_entry {\n-\t\tstruct object_id oid;\n-\t\tchar *name;\n-\t} *list;\n-};\n+#include \"string-list.h\"\n \n struct bundle_header {\n \tunsigned version;\n-\tstruct ref_list prerequisites;\n-\tstruct ref_list references;\n+\tstruct string_list prerequisites;\n+\tstruct string_list references;\n \tconst struct git_hash_algo *hash_algo;\n };\n \n+#define BUNDLE_HEADER_INIT \\\n+{ \\\n+\t.prerequisites = STRING_LIST_INIT_DUP, \\\n+\t.references = STRING_LIST_INIT_DUP, \\\n+}\n+void bundle_header_init(struct bundle_header *header);\n+void bundle_header_release(struct bundle_header *header);\n+\n int is_bundle(const char *path, int quiet);\n int read_bundle_header(const char *path, struct bundle_header *header);\n int create_bundle(struct repository *r, const char *path,\ndiff --git a/transport.c b/transport.c\nindex 95c1138e9ae..745ffa22474 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -147,10 +147,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n-\t\tstruct ref_list_entry *e = data->header.references.list + i;\n-\t\tconst char *name = e->name;\n+\t\tstruct string_list_item *e = data->header.references.items + i;\n+\t\tconst char *name = e->string;\n \t\tstruct ref *ref = alloc_ref(name);\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object_id *oid = e->util;\n \t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n@@ -177,6 +177,7 @@ static int close_bundle(struct transport *transport)\n \tstruct bundle_transport_data *data = transport->data;\n \tif (data->fd > 0)\n \t\tclose(data->fd);\n+\tbundle_header_release(&data->header);\n \tfree(data);\n \treturn 0;\n }\n@@ -1083,6 +1084,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tdie(_(\"git-over-rsync is no longer supported\"));\n \t} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n+\t\tbundle_header_init(&data->header);\n \t\ttransport_check_allowed(\"file\");\n \t\tret->data = data;\n \t\tret->vtable = &bundle_vtable;\n-- \n2.32.0.613.g8e17abc2eb\n\n"},{"id":"428836","messageId":"YNypPeoZTRiOxPPQ@coredump.intra.peff.net","threadId":"55951","inReplyTo":"patch-1.3-3d0d7a8e8b5-20210630T140339Z-avarab@gmail.com","subject":"Re: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T17:26:21Z","receivedAt":"2021-06-30T17:26:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Fix a memory leak from the prefix_filename() function introduced with\n> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,\n> 2017-03-20).\n> \n> As noted in that commit the leak was intentional as a part of being\n> sloppy about freeing resources just before we exit, I'm changing this\n> because I'll be fixing other memory leaks in the bundle API (including\n> the library version) in subsequent commits. It's easier to reason\n> about those fixes if valgrind runs cleanly at the end without any\n> leaks whatsoever.\n\nThanks, this looks good to me.\n\nOne thing, though...\n\n> An earlier version of this change went out of its way to not leak\n> memory on the die() codepaths here, but that was deemed too verbose to\n> worry about in a built-in that's dying anyway. The only reason we'd\n> need that is to appease a mode like SANITIZE=leak within the scope of\n> an entire test file.\n\nObviously you changed this as I asked, but this final sentence makes me\nthink we're not on the same page with respect to die(). I don't think\nany kind of mode matters here. When we call die(), whatever we have on\nthe stack is _not_ a leak, by LSan's or valgrind's standards. Because we\nstill have access to those bytes. And nor can we ever get rid of such\ncases. If we ever do:\n\n void foo(const char *str)\n {\n\tchar *x = xstrdup(str);\n\tbar(x);\n\tfree(x);\n }\n\n void bar(const char *x)\n {\n\tif (!strcmp(x, \"foo\"))\n\t\tdie(\"whatever\");\n }\n\nThen \"x\" will always still be allocated when we die(). We cannot free it\nin bar(), where it is read-only. We cannot free it in foo() before we\ncall bar(), because it is needed there. But control never returns to the\nfree() statement.\n\nSo this code is perfectly fine and unavoidable. In the case you were\ntouching it was foo() that was calling die() directly, so we could work\naround it with some conditionals. But from the leak-checker's\nperspective the two cases are the same.\n\n-Peff\n"},{"id":"428841","messageId":"YNyrDxUO1PlGJvCn@coredump.intra.peff.net","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com","subject":"Re: [PATCH v3 0/3]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T17:34:07Z","receivedAt":"2021-06-30T17:34:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 30, 2021 at 04:06:13PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Refactor the bundle API to use the string_list API instead of its own\n> version of a similar API. See [1] for v2.\n> \n> Addresses comments by Jeff King about us being too overzelous in\n> trying not to leak memory (the 'die_no_repo' is gone), and other\n> flow/style comments of his.\n> \n> I also added a bundle_header_init() function for use in transport.c,\n> and noticed a redundant call to string_list_clear() there.\n\nThanks, all three look good to me.\n\nAs an aside, having to have a separate bundle_header_init() and\nBUNDLE_HEADER_INIT is annoying (because they both must be kept up to\ndate with each other), but quite common in our code base. I wonder if\nwriting:\n\n void bundle_header_init(struct bundle_header *header)\n {\n\tstruct bundle_header blank = BUNDLE_HEADER_INIT;\n\tmemcpy(header, &blank, sizeof(*header));\n }\n\nwould let a smart enough compiler just init \"header\" in place without\nthe extra copy (the performance of a single bundle_header almost\ncertainly doesn't matter, but it might for other types).\n\nJust musing. ;)\n\n-Peff\n"},{"id":"428842","messageId":"YNytp0JAIaQih0Y4@coredump.intra.peff.net","threadId":"55951","inReplyTo":"YNyrDxUO1PlGJvCn@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/3]","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-30T17:45:11Z","receivedAt":"2021-06-30T17:45:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:\n\n> As an aside, having to have a separate bundle_header_init() and\n> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to\n> date with each other), but quite common in our code base. I wonder if\n> writing:\n> \n> void bundle_header_init(struct bundle_header *header)\n> {\n> \tstruct bundle_header blank = BUNDLE_HEADER_INIT;\n> \tmemcpy(header, &blank, sizeof(*header));\n> }\n> \n> would let a smart enough compiler just init \"header\" in place without\n> the extra copy (the performance of a single bundle_header almost\n> certainly doesn't matter, but it might for other types).\n> \n> Just musing. ;)\n\nFor my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9\n\nWith \"gcc -O2\" the memcpy goes away and we init \"header\" directly.\n\nIf we want to start using this technique widely, I don't think it should\nbe part of your series, though. This probably applies to quite a few\ndata structures, so it would make more sense to have a series which\nconverts several.\n\n-Peff\n"},{"id":"428845","messageId":"87y2ardygj.fsf@evledraar.gmail.com","threadId":"55951","inReplyTo":"YNytp0JAIaQih0Y4@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/3]","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T18:00:05Z","receivedAt":"2021-06-30T18:00:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 30 2021, Jeff King wrote:\n\n> On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:\n>\n>> As an aside, having to have a separate bundle_header_init() and\n>> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to\n>> date with each other), but quite common in our code base. I wonder if\n>> writing:\n>> \n>> void bundle_header_init(struct bundle_header *header)\n>> {\n>> \tstruct bundle_header blank = BUNDLE_HEADER_INIT;\n>> \tmemcpy(header, &blank, sizeof(*header));\n>> }\n>> \n>> would let a smart enough compiler just init \"header\" in place without\n>> the extra copy (the performance of a single bundle_header almost\n>> certainly doesn't matter, but it might for other types).\n>> \n>> Just musing. ;)\n>\n> For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9\n>\n> With \"gcc -O2\" the memcpy goes away and we init \"header\" directly.\n>\n> If we want to start using this technique widely, I don't think it should\n> be part of your series, though. This probably applies to quite a few\n> data structures, so it would make more sense to have a series which\n> converts several.\n\nThat's cool, yeah that would make quite a lot of code better. Thanks!\n"},{"id":"428847","messageId":"87v95vdxrc.fsf@evledraar.gmail.com","threadId":"55951","inReplyTo":"YNypPeoZTRiOxPPQ@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-30T18:00:50Z","receivedAt":"2021-06-30T18:15:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 30 2021, Jeff King wrote:\n\n> On Wed, Jun 30, 2021 at 04:06:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Fix a memory leak from the prefix_filename() function introduced with\n>> its use in 3b754eedd5 (bundle: use prefix_filename with bundle path,\n>> 2017-03-20).\n>> \n>> As noted in that commit the leak was intentional as a part of being\n>> sloppy about freeing resources just before we exit, I'm changing this\n>> because I'll be fixing other memory leaks in the bundle API (including\n>> the library version) in subsequent commits. It's easier to reason\n>> about those fixes if valgrind runs cleanly at the end without any\n>> leaks whatsoever.\n>\n> Thanks, this looks good to me.\n>\n> One thing, though...\n>\n>> An earlier version of this change went out of its way to not leak\n>> memory on the die() codepaths here, but that was deemed too verbose to\n>> worry about in a built-in that's dying anyway. The only reason we'd\n>> need that is to appease a mode like SANITIZE=leak within the scope of\n>> an entire test file.\n>\n> Obviously you changed this as I asked, but this final sentence makes me\n> think we're not on the same page with respect to die(). I don't think\n> any kind of mode matters here. When we call die(), whatever we have on\n> the stack is _not_ a leak, by LSan's or valgrind's standards. Because we\n> still have access to those bytes. And nor can we ever get rid of such\n> cases. If we ever do:\n>\n> void foo(const char *str)\n> {\n> \tchar *x = xstrdup(str);\n> \tbar(x);\n> \tfree(x);\n> }\n>\n> void bar(const char *x)\n> {\n> \tif (!strcmp(x, \"foo\"))\n> \t\tdie(\"whatever\");\n> }\n>\n> Then \"x\" will always still be allocated when we die(). We cannot free it\n> in bar(), where it is read-only. We cannot free it in foo() before we\n> call bar(), because it is needed there. But control never returns to the\n> free() statement.\n>\n> So this code is perfectly fine and unavoidable. In the case you were\n> touching it was foo() that was calling die() directly, so we could work\n> around it with some conditionals. But from the leak-checker's\n> perspective the two cases are the same.\n\nI've got you to blame for introducing SANITIZE=*. Now I've got these\nleak checkers all mixed up :)\n\nYes you're right, FWIW I (re-)wrote this commit message just before\nsending and should have said \"a heap leak checker\" instead of\n\"SANITIZE=leak\", I really meant valgrind.\n\nI originally ended with the \"we are about to die\" tracking because I was\ntracing things with valgrind, which does complain about this sort of\nthing. I.e.:\n \n 24 bytes in 1 blocks are still reachable in loss record 8 of 21\n at 0x48356AF: malloc (vg_replace_malloc.c:298)\n by 0x4837DE7: realloc (vg_replace_malloc.c:826)\n by 0x3C06F1: xrealloc (wrapper.c:126)\n by 0x380EC9: strbuf_grow (strbuf.c:98)\n by 0x381A14: strbuf_add (strbuf.c:297)\n by 0x20ADC5: strbuf_addstr (strbuf.h:304)\n by 0x20B66D: prefix_filename (abspath.c:277)\n by 0x13CDC6: parse_options_cmd_bundle (bundle.c:55)\n by 0x13D565: cmd_bundle_unbundle (bundle.c:170)\n by 0x13D829: cmd_bundle (bundle.c:214)\n by 0x1279F4: run_builtin (git.c:461)\n by 0x127DFB: handle_builtin (git.c:714)\n\nRe what I mentioned/asked in\nhttps://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ I was\nexperimenting with doing leak checking in the tests.\n\nIn this case we have 21 in total under --show-leak-kinds=all (and it was\n20 in v2 of this series).\n\nI've found that only including the file tho builtin is in cuts down on\nit to a meaningful set of leaks. I.e. to throw out everything not\nincluding /\\bbundle\\.c:/. We leak in a lot of things we call from\ncommon-main.c, git.c, exec-cmd.c etc.\n\nMaybe if we do end up with a test mode for this we shouldn't care about\ncheckers like valgrind and only cater to the SANITIZE=leak mode.\n\nI'm still partial to the idea that we'll get the most win out of\nsomething that we can run in the tests by default, i.e. we'll only need\nto have a valgrind on the system & have someone run \"make test\" to run a\n(limited set of) tests with this.\n\nWhereas SANITIZE=leak is always a dev-only feature people may not have\non all the time.\n\n"},{"id":"428854","messageId":"xmqqfswzt5jo.fsf@gitster.g","threadId":"55951","inReplyTo":"patch-3.3-f1066ee1b9a-20210630T140339Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-30T21:18:35Z","receivedAt":"2021-06-30T21:18:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Move away from the \"struct ref_list\" in bundle.c in favor of the\n> almost identical string-list.c API.\n\nNice.\n"},{"id":"428885","messageId":"87mtr6e24q.fsf@evledraar.gmail.com","threadId":"55951","inReplyTo":"87y2ardygj.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 0/3]","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-01T10:53:26Z","receivedAt":"2021-07-01T10:53:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 30 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> On Wed, Jun 30 2021, Jeff King wrote:\n>\n>> On Wed, Jun 30, 2021 at 01:34:07PM -0400, Jeff King wrote:\n>>\n>>> As an aside, having to have a separate bundle_header_init() and\n>>> BUNDLE_HEADER_INIT is annoying (because they both must be kept up to\n>>> date with each other), but quite common in our code base. I wonder if\n>>> writing:\n>>> \n>>> void bundle_header_init(struct bundle_header *header)\n>>> {\n>>> \tstruct bundle_header blank = BUNDLE_HEADER_INIT;\n>>> \tmemcpy(header, &blank, sizeof(*header));\n>>> }\n>>> \n>>> would let a smart enough compiler just init \"header\" in place without\n>>> the extra copy (the performance of a single bundle_header almost\n>>> certainly doesn't matter, but it might for other types).\n>>> \n>>> Just musing. ;)\n>>\n>> For my own curiosity, the answer is yes: https://godbolt.org/z/s54dc6ss9\n>>\n>> With \"gcc -O2\" the memcpy goes away and we init \"header\" directly.\n>>\n>> If we want to start using this technique widely, I don't think it should\n>> be part of your series, though. This probably applies to quite a few\n>> data structures, so it would make more sense to have a series which\n>> converts several.\n>\n> That's cool, yeah that would make quite a lot of code better. Thanks!\n\nJust for future reference: I submitted a small series to make use of\nthis suggested idiom:\nhttps://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/\n"},{"id":"428942","messageId":"YN3iIaovvG7XgLQP@coredump.intra.peff.net","threadId":"55951","inReplyTo":"87v95vdxrc.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-01T15:41:21Z","receivedAt":"2021-07-01T15:41:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 30, 2021 at 08:00:50PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > So this code is perfectly fine and unavoidable. In the case you were\n> > touching it was foo() that was calling die() directly, so we could work\n> > around it with some conditionals. But from the leak-checker's\n> > perspective the two cases are the same.\n> \n> I've got you to blame for introducing SANITIZE=*. Now I've got these\n> leak checkers all mixed up :)\n> \n> Yes you're right, FWIW I (re-)wrote this commit message just before\n> sending and should have said \"a heap leak checker\" instead of\n> \"SANITIZE=leak\", I really meant valgrind.\n\nAh, OK, that makes more sense.\n\nI think the distinction here isn't \"heap leak checker\" (since valgrind\ndoes still look at the whole memory space to find references), but\nrather the treatment of \"still reachable\" items. I think LSan/ASan\nignore these entirely.\n\n> I originally ended with the \"we are about to die\" tracking because I was\n> tracing things with valgrind, which does complain about this sort of\n> thing. I.e.:\n> \n> 24 bytes in 1 blocks are still reachable in loss record 8 of 21\n> at 0x48356AF: malloc (vg_replace_malloc.c:298)\n> by 0x4837DE7: realloc (vg_replace_malloc.c:826)\n> by 0x3C06F1: xrealloc (wrapper.c:126)\n> by 0x380EC9: strbuf_grow (strbuf.c:98)\n> by 0x381A14: strbuf_add (strbuf.c:297)\n> by 0x20ADC5: strbuf_addstr (strbuf.h:304)\n> by 0x20B66D: prefix_filename (abspath.c:277)\n> by 0x13CDC6: parse_options_cmd_bundle (bundle.c:55)\n> by 0x13D565: cmd_bundle_unbundle (bundle.c:170)\n> by 0x13D829: cmd_bundle (bundle.c:214)\n> by 0x1279F4: run_builtin (git.c:461)\n> by 0x127DFB: handle_builtin (git.c:714)\n> \n> Re what I mentioned/asked in\n> https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/ I was\n> experimenting with doing leak checking in the tests.\n> \n> In this case we have 21 in total under --show-leak-kinds=all (and it was\n> 20 in v2 of this series).\n\nIMHO these \"still reachable\" leaks are not interesting. They'll fall\ninto one of two categories:\n\n - allocations still on the stack when we exit() without unwinding.\n These are always uninteresting, since by definition we are exiting\n the process.\n\n - globals that hang on to allocations (which will still be present\n even if we exit the program by returning up through main()). Most of\n these will be items we intend to last for the program length (e.g.,\n fields in the_repository).\n\n It's _possible_ that some of these could be interesting. E.g., a\n program might have two phases: in the first, we rely on some\n subsystem which uses global variables to cache some data (say,\n parsed config values in remote.c or userdiff.c), and in the second\n phase we no longer use that subsystem. We could be instructing the\n subsystem to free up the memory after the first phase. But in\n practice this is awkward, because the program-level code doesn't\n know about subsystem allocations (or even which subsystems might\n have been recursively triggered).\n\n And while still-reachable tracking could be used to find\n opportunities like this, I think there's a better approach to\n finding these. If subsystems avoid global-variable caches and\n instead stick their allocations into context structs, then the\n memory is associated with those structs. So for example, if\n userdiff attached its storage to a diff_options, which is attached\n to a rev_info, then any \"leaking\" is all predicated on the rev_info\n (which the main program either cleans up, or annotates with UNLEAK).\n\n> Maybe if we do end up with a test mode for this we shouldn't care about\n> checkers like valgrind and only cater to the SANITIZE=leak mode.\n\nI do find SANITIZE=leak to be a more useful tool in general, but I'm not\nat all against using valgrind if people find it convenient. I just think\n\"reachable\" leaks are not interesting enough to be part of what we're\nsearching for when we run the test suite.\n\n> I'm still partial to the idea that we'll get the most win out of\n> something that we can run in the tests by default, i.e. we'll only need\n> to have a valgrind on the system & have someone run \"make test\" to run a\n> (limited set of) tests with this.\n> \n> Whereas SANITIZE=leak is always a dev-only feature people may not have\n> on all the time.\n\nThat is exactly why I think \"SANITIZE=leak\" is better: it is easier for\npeople to run. valgrind is _extremely_ slow. It's also not available\neverywhere; ASan/LSan aren't either, but they're pretty standard in\nclang and gcc these days.\n\nIt is nice that valgrind can run on an un-instrumented binary, but I\nsort of assume that anybody running \"make test\" will be able to build\nthe binary, since \"make\" is going to want to do that. I.e., I think\n\"make SANITIZE=leak test\" is already doing what we want (we just need to\nfurther annotate known-failures).\n\n-Peff\n"},{"id":"429010","messageId":"patch-1.3-8e1d08113e5-20210702T095450Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210702T095450Z-avarab@gmail.com","subject":"[PATCH v4 1/3] bundle cmd: stop leaking memory from parse_options_cmd_bundle()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-02T09:57:30Z","receivedAt":"2021-07-02T09:57:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak from the prefix_filename() function introduced with\nits use in 3b754eedd5 (bundle: use prefix_filename with bundle path,\n2017-03-20).\n\nAs noted in that commit the leak was intentional as a part of being\nsloppy about freeing resources just before we exit, I'm changing this\nbecause I'll be fixing other memory leaks in the bundle API (including\nthe library version) in subsequent commits. It's easier to reason\nabout those fixes if valgrind runs cleanly at the end without any\nleaks whatsoever.\n\nAn earlier version of this change[1] went out of its way to not leak\nmemory on the die() codepaths here, but doing so will only avoid\nreports of potential leaks under heap-only leak trackers such as\nvalgrind, not the SANITIZE=leak mode.\n\nAvoiding those leaks as well might be useful to enable us to run\ncleanly under the likes of valgrind in the future. But for now the\nrelative verbosity of the resulting code, and the fact that we don't\nhave some valgrind or SANITIZE=leak mode as part of our CI (it's only\nrun ad-hoc, see [2]), means we're not worrying about that for now.\n\n1. https://lore.kernel.org/git/87v95vdxrc.fsf@evledraar.gmail.com/\n2. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 62 ++++++++++++++++++++++++++++++++----------------\n 1 file changed, 41 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex ea6948110b0..15e2bd61965 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -46,7 +46,7 @@ static int parse_options_cmd_bundle(int argc,\n \t\tconst char* prefix,\n \t\tconst char * const usagestr[],\n \t\tconst struct option options[],\n-\t\tconst char **bundle_file) {\n+\t\tchar **bundle_file) {\n \tint newargc;\n \tnewargc = parse_options(argc, argv, NULL, options, usagestr,\n \t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n@@ -61,7 +61,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \tint progress = isatty(STDERR_FILENO);\n \tstruct strvec pack_opts;\n \tint version = -1;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_SET_INT('q', \"quiet\", &progress,\n \t\t\t N_(\"do not show progress meter\"), 0),\n@@ -76,7 +76,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \t\t\t N_(\"specify bundle format version\")),\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_create_usage, options, &bundle_file);\n@@ -94,75 +94,95 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n \n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to create a bundle.\"));\n-\treturn !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n+\tret = !!create_bundle(the_repository, bundle_file, argc, argv, &pack_opts, version);\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n \tint quiet = 0;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_BOOL('q', \"quiet\", &quiet,\n \t\t\t N_(\"do not show bundle details\")),\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tclose(bundle_fd);\n-\tif (verify_bundle(the_repository, &header, !quiet))\n-\t\treturn 1;\n+\tif (verify_bundle(the_repository, &header, !quiet)) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \tfprintf(stderr, _(\"%s is okay\\n\"), bundle_file);\n-\treturn 0;\n+\tret = 0;\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tclose(bundle_fd);\n-\treturn !!list_bundle_refs(&header, argc, argv);\n+\tret = !!list_bundle_refs(&header, argc, argv);\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {\n \tstruct bundle_header header;\n \tint bundle_fd = -1;\n-\n+\tint ret;\n \tstruct option options[] = {\n \t\tOPT_END()\n \t};\n-\tconst char* bundle_file;\n+\tchar *bundle_file;\n \n \targc = parse_options_cmd_bundle(argc, argv, prefix,\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n \tmemset(&header, 0, sizeof(header));\n-\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0)\n-\t\treturn 1;\n+\tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n+\t\tret = 1;\n+\t\tgoto cleanup;\n+\t}\n \tif (!startup_info->have_repository)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n-\treturn !!unbundle(the_repository, &header, bundle_fd, 0) ||\n+\tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n+cleanup:\n+\tfree(bundle_file);\n+\treturn ret;\n }\n \n int cmd_bundle(int argc, const char **argv, const char *prefix)\n-- \n2.32.0.632.g49a94b9226d\n\n"},{"id":"429011","messageId":"cover-0.3-00000000000-20210702T095450Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com","subject":"[PATCH v4 0/3] bundle.c: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-02T09:57:29Z","receivedAt":"2021-07-02T09:57:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This re-roll of v3 changes the discussion in the 1/3 commit message,\nit incorrectly referred to SANITIZE=leak when I meant valgrind.\n\nI also changed the bundle_header_init() pattern to use the same\n\"memcpy() a blank\" as in my parallel series to do that more generally.\n\nv3 at:\nhttps://lore.kernel.org/git/cover-0.3-00000000000-20210630T140339Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (3):\n bundle cmd: stop leaking memory from parse_options_cmd_bundle()\n bundle.c: use a temporary variable for OIDs and names\n bundle: remove \"ref_list\" in favor of string-list.c API\n\n builtin/bundle.c | 74 ++++++++++++++++++++++++++++++------------------\n bundle.c | 64 +++++++++++++++++++++++++----------------\n bundle.h | 21 +++++++-------\n transport.c | 10 +++++--\n 4 files changed, 104 insertions(+), 65 deletions(-)\n\nRange-diff against v3:\n1: 3d0d7a8e8b5 ! 1: 8e1d08113e5 bundle cmd: stop leaking memory from parse_options_cmd_bundle()\n @@ Commit message\n about those fixes if valgrind runs cleanly at the end without any\n leaks whatsoever.\n \n - An earlier version of this change went out of its way to not leak\n - memory on the die() codepaths here, but that was deemed too verbose to\n - worry about in a built-in that's dying anyway. The only reason we'd\n - need that is to appease a mode like SANITIZE=leak within the scope of\n - an entire test file.\n + An earlier version of this change[1] went out of its way to not leak\n + memory on the die() codepaths here, but doing so will only avoid\n + reports of potential leaks under heap-only leak trackers such as\n + valgrind, not the SANITIZE=leak mode.\n +\n + Avoiding those leaks as well might be useful to enable us to run\n + cleanly under the likes of valgrind in the future. But for now the\n + relative verbosity of the resulting code, and the fact that we don't\n + have some valgrind or SANITIZE=leak mode as part of our CI (it's only\n + run ad-hoc, see [2]), means we're not worrying about that for now.\n +\n + 1. https://lore.kernel.org/git/87v95vdxrc.fsf@evledraar.gmail.com/\n + 2. https://lore.kernel.org/git/87czsv2idy.fsf@evledraar.gmail.com/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n2: e47646d3a98 = 2: 5ce376682b3 bundle.c: use a temporary variable for OIDs and names\n3: f1066ee1b9a ! 3: 3e5972e4184 bundle: remove \"ref_list\" in favor of string-list.c API\n @@ Commit message\n Before this the add_to_ref_list() would leak memory, now e.g. \"bundle\n list-heads\" reports no memory leaks at all under valgrind.\n \n + In the bundle_header_init() function we're using a clever trick to\n + memcpy() what we'd get from the corresponding\n + BUNDLE_HEADER_INIT. There is a concurrent series to make use of that\n + pattern more generally, see [1].\n +\n + 1. https://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/bundle.c ##\n @@ bundle.c: static struct {\n -\toidcpy(&list->list[list->nr].oid, oid);\n -\tlist->list[list->nr].name = xstrdup(name);\n -\tlist->nr++;\n -+\tmemset(header, 0, sizeof(*header));\n -+\tstring_list_init(&header->prerequisites, 1);\n -+\tstring_list_init(&header->references, 1);\n ++\tstruct bundle_header blank = BUNDLE_HEADER_INIT;\n ++\tmemcpy(header, &blank, sizeof(*header));\n +}\n +\n +void bundle_header_release(struct bundle_header *header)\n-- \n2.32.0.632.g49a94b9226d\n\n"},{"id":"429012","messageId":"patch-2.3-5ce376682b3-20210702T095450Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210702T095450Z-avarab@gmail.com","subject":"[PATCH v4 2/3] bundle.c: use a temporary variable for OIDs and names","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-02T09:57:31Z","receivedAt":"2021-07-02T09:57:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for moving away from accessing the OID and name via the\n\"oid\" and \"name\" slots in a subsequent commit, change the code that\naccesses it to use named variables. This makes the subsequent change\nsmaller.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bundle.c | 26 ++++++++++++++++++--------\n transport.c | 6 ++++--\n 2 files changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 693d6195514..7210e5e7105 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -156,6 +156,9 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \tint i;\n \n \tfor (i = 0; i < r->nr; i++) {\n+\t\tstruct object_id *oid;\n+\t\tconst char *name;\n+\n \t\tif (argc > 1) {\n \t\t\tint j;\n \t\t\tfor (j = 1; j < argc; j++)\n@@ -164,8 +167,10 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \t\t\tif (j == argc)\n \t\t\t\tcontinue;\n \t\t}\n-\t\tprintf(\"%s %s\\n\", oid_to_hex(&r->list[i].oid),\n-\t\t\t\tr->list[i].name);\n+\n+\t\toid = &r->list[i].oid;\n+\t\tname = r->list[i].name;\n+\t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n \t}\n \treturn 0;\n }\n@@ -194,15 +199,17 @@ int verify_bundle(struct repository *r,\n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object *o = parse_object(r, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n-\t\t\tadd_pending_object(&revs, o, e->name);\n+\t\t\tadd_pending_object(&revs, o, name);\n \t\t\tcontinue;\n \t\t}\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \tif (revs.pending.nr != p->nr)\n \t\treturn ret;\n@@ -219,19 +226,22 @@ int verify_bundle(struct repository *r,\n \n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object *o = parse_object(r, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n \t\t\tcontinue;\n \t\tif (++ret == 1)\n \t\t\terror(\"%s\", message);\n-\t\terror(\"%s %s\", oid_to_hex(&e->oid), e->name);\n+\t\terror(\"%s %s\", oid_to_hex(oid), name);\n \t}\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n \t\tstruct ref_list_entry *e = p->list + i;\n-\t\tcommit = lookup_commit_reference_gently(r, &e->oid, 1);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\tcommit = lookup_commit_reference_gently(r, oid, 1);\n \t\tif (commit)\n \t\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n \t}\ndiff --git a/transport.c b/transport.c\nindex 50f5830eb6b..95c1138e9ae 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -148,8 +148,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n \t\tstruct ref_list_entry *e = data->header.references.list + i;\n-\t\tstruct ref *ref = alloc_ref(e->name);\n-\t\toidcpy(&ref->old_oid, &e->oid);\n+\t\tconst char *name = e->name;\n+\t\tstruct ref *ref = alloc_ref(name);\n+\t\tstruct object_id *oid = &e->oid;\n+\t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n \t}\n-- \n2.32.0.632.g49a94b9226d\n\n"},{"id":"429013","messageId":"patch-3.3-3e5972e4184-20210702T095450Z-avarab@gmail.com","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210702T095450Z-avarab@gmail.com","subject":"[PATCH v4 3/3] bundle: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-02T09:57:32Z","receivedAt":"2021-07-02T09:57:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move away from the \"struct ref_list\" in bundle.c in favor of the\nalmost identical string-list.c API.\n\nThat API fits this use-case perfectly, but did not exist in its\ncurrent form when this code was added in 2e0afafebd (Add git-bundle:\nmove objects and references by archive, 2007-02-22), with hindsight we\ncould have used the path-list API, which later got renamed to\nstring-list. See 8fd2cb4069 (Extract helper bits from\nc-merge-recursive work, 2006-07-25)\n\nWe need to change \"name\" to \"string\" and \"oid\" to \"util\" to make this\nconversion, but other than that the APIs are pretty much identical for\nwhat bundle.c made use of.\n\nLet's also replace the memset(..,0,...) pattern with a more idiomatic\n\"INIT\" macro, and finally add a *_release() function so to free the\nallocated memory.\n\nBefore this the add_to_ref_list() would leak memory, now e.g. \"bundle\nlist-heads\" reports no memory leaks at all under valgrind.\n\nIn the bundle_header_init() function we're using a clever trick to\nmemcpy() what we'd get from the corresponding\nBUNDLE_HEADER_INIT. There is a concurrent series to make use of that\npattern more generally, see [1].\n\n1. https://lore.kernel.org/git/cover-0.5-00000000000-20210701T104855Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/bundle.c | 12 +++++------\n bundle.c | 52 ++++++++++++++++++++++++++----------------------\n bundle.h | 21 +++++++++----------\n transport.c | 8 +++++---\n 4 files changed, 50 insertions(+), 43 deletions(-)\n\ndiff --git a/builtin/bundle.c b/builtin/bundle.c\nindex 15e2bd61965..053a51bea1b 100644\n--- a/builtin/bundle.c\n+++ b/builtin/bundle.c\n@@ -100,7 +100,7 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {\n }\n \n static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint quiet = 0;\n \tint ret;\n@@ -115,7 +115,6 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \t\t\tbuiltin_bundle_verify_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -130,11 +129,12 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {\n \tret = 0;\n cleanup:\n \tfree(bundle_file);\n+\tbundle_header_release(&header);\n \treturn ret;\n }\n \n static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n \tstruct option options[] = {\n@@ -146,7 +146,6 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \t\t\tbuiltin_bundle_list_heads_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -155,11 +154,12 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix\n \tret = !!list_bundle_refs(&header, argc, argv);\n cleanup:\n \tfree(bundle_file);\n+\tbundle_header_release(&header);\n \treturn ret;\n }\n \n static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint bundle_fd = -1;\n \tint ret;\n \tstruct option options[] = {\n@@ -171,7 +171,6 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\t\tbuiltin_bundle_unbundle_usage, options, &bundle_file);\n \t/* bundle internals use argv[1] as further parameters */\n \n-\tmemset(&header, 0, sizeof(header));\n \tif ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {\n \t\tret = 1;\n \t\tgoto cleanup;\n@@ -180,6 +179,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"Need a repository to unbundle.\"));\n \tret = !!unbundle(the_repository, &header, bundle_fd, 0) ||\n \t\tlist_bundle_refs(&header, argc, argv);\n+\tbundle_header_release(&header);\n cleanup:\n \tfree(bundle_file);\n \treturn ret;\ndiff --git a/bundle.c b/bundle.c\nindex 7210e5e7105..ab63f402261 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -23,13 +23,16 @@ static struct {\n \t{ 3, v3_bundle_signature },\n };\n \n-static void add_to_ref_list(const struct object_id *oid, const char *name,\n-\t\tstruct ref_list *list)\n+void bundle_header_init(struct bundle_header *header)\n {\n-\tALLOC_GROW(list->list, list->nr + 1, list->alloc);\n-\toidcpy(&list->list[list->nr].oid, oid);\n-\tlist->list[list->nr].name = xstrdup(name);\n-\tlist->nr++;\n+\tstruct bundle_header blank = BUNDLE_HEADER_INIT;\n+\tmemcpy(header, &blank, sizeof(*header));\n+}\n+\n+void bundle_header_release(struct bundle_header *header)\n+{\n+\tstring_list_clear(&header->prerequisites, 1);\n+\tstring_list_clear(&header->references, 1);\n }\n \n static int parse_capability(struct bundle_header *header, const char *capability)\n@@ -112,10 +115,11 @@ static int parse_bundle_header(int fd, struct bundle_header *header,\n \t\t\tstatus = -1;\n \t\t\tbreak;\n \t\t} else {\n+\t\t\tstruct object_id *dup = oiddup(&oid);\n \t\t\tif (is_prereq)\n-\t\t\t\tadd_to_ref_list(&oid, \"\", &header->prerequisites);\n+\t\t\t\tstring_list_append(&header->prerequisites, \"\")->util = dup;\n \t\t\telse\n-\t\t\t\tadd_to_ref_list(&oid, p + 1, &header->references);\n+\t\t\t\tstring_list_append(&header->references, p + 1)->util = dup;\n \t\t}\n \t}\n \n@@ -139,19 +143,19 @@ int read_bundle_header(const char *path, struct bundle_header *header)\n \n int is_bundle(const char *path, int quiet)\n {\n-\tstruct bundle_header header;\n+\tstruct bundle_header header = BUNDLE_HEADER_INIT;\n \tint fd = open(path, O_RDONLY);\n \n \tif (fd < 0)\n \t\treturn 0;\n-\tmemset(&header, 0, sizeof(header));\n \tfd = parse_bundle_header(fd, &header, quiet ? NULL : path);\n \tif (fd >= 0)\n \t\tclose(fd);\n+\tbundle_header_release(&header);\n \treturn (fd >= 0);\n }\n \n-static int list_refs(struct ref_list *r, int argc, const char **argv)\n+static int list_refs(struct string_list *r, int argc, const char **argv)\n {\n \tint i;\n \n@@ -162,14 +166,14 @@ static int list_refs(struct ref_list *r, int argc, const char **argv)\n \t\tif (argc > 1) {\n \t\t\tint j;\n \t\t\tfor (j = 1; j < argc; j++)\n-\t\t\t\tif (!strcmp(r->list[i].name, argv[j]))\n+\t\t\t\tif (!strcmp(r->items[i].string, argv[j]))\n \t\t\t\t\tbreak;\n \t\t\tif (j == argc)\n \t\t\t\tcontinue;\n \t\t}\n \n-\t\toid = &r->list[i].oid;\n-\t\tname = r->list[i].name;\n+\t\toid = r->items[i].util;\n+\t\tname = r->items[i].string;\n \t\tprintf(\"%s %s\\n\", oid_to_hex(oid), name);\n \t}\n \treturn 0;\n@@ -186,7 +190,7 @@ int verify_bundle(struct repository *r,\n \t * Do fast check, then if any prereqs are missing then go line by line\n \t * to be verbose about the errors\n \t */\n-\tstruct ref_list *p = &header->prerequisites;\n+\tstruct string_list *p = &header->prerequisites;\n \tstruct rev_info revs;\n \tconst char *argv[] = {NULL, \"--all\", NULL};\n \tstruct commit *commit;\n@@ -198,9 +202,9 @@ int verify_bundle(struct repository *r,\n \n \trepo_init_revisions(r, &revs, NULL);\n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tconst char *name = e->name;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tconst char *name = e->string;\n+\t\tstruct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tif (o) {\n \t\t\to->flags |= PREREQ_MARK;\n@@ -225,9 +229,9 @@ int verify_bundle(struct repository *r,\n \t\t\ti--;\n \n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tconst char *name = e->name;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tconst char *name = e->string;\n+\t\tconst struct object_id *oid = e->util;\n \t\tstruct object *o = parse_object(r, oid);\n \t\tassert(o); /* otherwise we'd have returned early */\n \t\tif (o->flags & SHOWN)\n@@ -239,15 +243,15 @@ int verify_bundle(struct repository *r,\n \n \t/* Clean up objects used, as they will be reused. */\n \tfor (i = 0; i < p->nr; i++) {\n-\t\tstruct ref_list_entry *e = p->list + i;\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct string_list_item *e = p->items + i;\n+\t\tstruct object_id *oid = e->util;\n \t\tcommit = lookup_commit_reference_gently(r, oid, 1);\n \t\tif (commit)\n \t\t\tclear_commit_marks(commit, ALL_REV_FLAGS);\n \t}\n \n \tif (verbose) {\n-\t\tstruct ref_list *r;\n+\t\tstruct string_list *r;\n \n \t\tr = &header->references;\n \t\tprintf_ln(Q_(\"The bundle contains this ref:\",\ndiff --git a/bundle.h b/bundle.h\nindex f9e2d1c8ef5..1927d8cd6a4 100644\n--- a/bundle.h\n+++ b/bundle.h\n@@ -3,22 +3,23 @@\n \n #include \"strvec.h\"\n #include \"cache.h\"\n-\n-struct ref_list {\n-\tunsigned int nr, alloc;\n-\tstruct ref_list_entry {\n-\t\tstruct object_id oid;\n-\t\tchar *name;\n-\t} *list;\n-};\n+#include \"string-list.h\"\n \n struct bundle_header {\n \tunsigned version;\n-\tstruct ref_list prerequisites;\n-\tstruct ref_list references;\n+\tstruct string_list prerequisites;\n+\tstruct string_list references;\n \tconst struct git_hash_algo *hash_algo;\n };\n \n+#define BUNDLE_HEADER_INIT \\\n+{ \\\n+\t.prerequisites = STRING_LIST_INIT_DUP, \\\n+\t.references = STRING_LIST_INIT_DUP, \\\n+}\n+void bundle_header_init(struct bundle_header *header);\n+void bundle_header_release(struct bundle_header *header);\n+\n int is_bundle(const char *path, int quiet);\n int read_bundle_header(const char *path, struct bundle_header *header);\n int create_bundle(struct repository *r, const char *path,\ndiff --git a/transport.c b/transport.c\nindex 95c1138e9ae..745ffa22474 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -147,10 +147,10 @@ static struct ref *get_refs_from_bundle(struct transport *transport,\n \ttransport->hash_algo = data->header.hash_algo;\n \n \tfor (i = 0; i < data->header.references.nr; i++) {\n-\t\tstruct ref_list_entry *e = data->header.references.list + i;\n-\t\tconst char *name = e->name;\n+\t\tstruct string_list_item *e = data->header.references.items + i;\n+\t\tconst char *name = e->string;\n \t\tstruct ref *ref = alloc_ref(name);\n-\t\tstruct object_id *oid = &e->oid;\n+\t\tstruct object_id *oid = e->util;\n \t\toidcpy(&ref->old_oid, oid);\n \t\tref->next = result;\n \t\tresult = ref;\n@@ -177,6 +177,7 @@ static int close_bundle(struct transport *transport)\n \tstruct bundle_transport_data *data = transport->data;\n \tif (data->fd > 0)\n \t\tclose(data->fd);\n+\tbundle_header_release(&data->header);\n \tfree(data);\n \treturn 0;\n }\n@@ -1083,6 +1084,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tdie(_(\"git-over-rsync is no longer supported\"));\n \t} else if (url_is_local_not_ssh(url) && is_file(url) && is_bundle(url, 1)) {\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n+\t\tbundle_header_init(&data->header);\n \t\ttransport_check_allowed(\"file\");\n \t\tret->data = data;\n \t\tret->vtable = &bundle_vtable;\n-- \n2.32.0.632.g49a94b9226d\n\n"},{"id":"429110","messageId":"YOBBgfiaz59KdsWS@coredump.intra.peff.net","threadId":"55951","inReplyTo":"cover-0.3-00000000000-20210702T095450Z-avarab@gmail.com","subject":"Re: [PATCH v4 0/3] bundle.c: remove \"ref_list\" in favor of string-list.c API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-03T10:52:49Z","receivedAt":"2021-07-03T10:52:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 02, 2021 at 11:57:29AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> This re-roll of v3 changes the discussion in the 1/3 commit message,\n> it incorrectly referred to SANITIZE=leak when I meant valgrind.\n> \n> I also changed the bundle_header_init() pattern to use the same\n> \"memcpy() a blank\" as in my parallel series to do that more generally.\n\nThanks, this looks good to me.\n\nI'd probably word the discussion about die() a bit differently, but you've\nalready seen my expositions on leak-checking, and it's all tangent here.\nSo let's move forward with this, and we can let leak-checking\nphilosophies iron themselves out as we fix more cases. :)\n\n-Peff\n"},{"id":"429113","messageId":"87y2an8wlv.fsf@evledraar.gmail.com","threadId":"55951","inReplyTo":"YOBBgfiaz59KdsWS@coredump.intra.peff.net","subject":"Re: [PATCH v4 0/3] bundle.c: remove \"ref_list\" in favor of string-list.c API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-03T11:28:32Z","receivedAt":"2021-07-03T11:28:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Jul 03 2021, Jeff King wrote:\n\n> On Fri, Jul 02, 2021 at 11:57:29AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This re-roll of v3 changes the discussion in the 1/3 commit message,\n>> it incorrectly referred to SANITIZE=leak when I meant valgrind.\n>> \n>> I also changed the bundle_header_init() pattern to use the same\n>> \"memcpy() a blank\" as in my parallel series to do that more generally.\n>\n> Thanks, this looks good to me.\n>\n> I'd probably word the discussion about die() a bit differently, but you've\n> already seen my expositions on leak-checking, and it's all tangent here.\n> So let's move forward with this, and we can let leak-checking\n> philosophies iron themselves out as we fix more cases. :)\n\nThanks for the detailed review over multiple rounds.\n"}]}