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