From a59cfb32300baab00ee9cec68326309f4b2faca9 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 18 Oct 2019 00:56:13 -0400 Subject: fsck: unify object-name code Commit 90cf590f53 (fsck: optionally show more helpful info for broken links, 2016-07-17) added a system for decorating objects with names. The code is split across builtin/fsck.c (which gives the initial names) and fsck.c (which adds to the names as it traverses the object graph). This leads to some duplication, where both sites have near-identical describe_object() functions (the difference being that the one in builtin/fsck.c uses a circular array of buffers to allow multiple calls in a single printf). Let's provide a unified object_name API for fsck. That lets us drop the duplication, as well as making the interface boundaries more clear (which will let us refactor the implementation more in a future patch). We'll leave describe_object() in builtin/fsck.c as a thin wrapper around the new API, as it relies on a static global to make its many callers a bit shorter. We'll also convert the bare add_decoration() calls in builtin/fsck.c to put_object_name(). This fixes two minor bugs: 1. We leak many small strings. add_decoration() has a last-one-wins approach: it updates the decoration to the new string and returns the old one. But we ignore the return value, leaking the old string. This is quite common to trigger, since we look at reflogs: the tip of any ref will be described both by looking at the actual ref, as well as the latest reflog entry. So we'd always end up leaking one of those strings. 2. The last-one-wins approach gives us lousy names. For instance, we first look at all of the refs, and then all of the reflogs. So rather than seeing "refs/heads/master", we're likely to overwrite it with "HEAD@{12345678}". We're generally better off using the first name we find. And indeed, the test in t1450 expects this ugly HEAD@{} name. After this patch, we've switched to using fsck_put_object_name()'s first-one-wins semantics, and we output the more human-friendly "refs/tags/julius" (and the test is updated accordingly). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fsck.c | 50 +++++++++++++------------------------------------- 1 file changed, 13 insertions(+), 37 deletions(-) (limited to 'builtin') diff --git a/builtin/fsck.c b/builtin/fsck.c index 18403a94fa..237643cc1d 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -52,24 +52,7 @@ static int name_objects; static const char *describe_object(struct object *obj) { - static struct strbuf bufs[] = { - STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT - }; - static int b = 0; - struct strbuf *buf; - char *name = NULL; - - if (name_objects) - name = lookup_decoration(fsck_walk_options.object_names, obj); - - buf = bufs + b; - b = (b + 1) % ARRAY_SIZE(bufs); - strbuf_reset(buf); - strbuf_addstr(buf, oid_to_hex(&obj->oid)); - if (name) - strbuf_addf(buf, " (%s)", name); - - return buf->buf; + return fsck_describe_object(&fsck_walk_options, obj); } static const char *printable_type(struct object *obj) @@ -499,10 +482,10 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid, if (!is_null_oid(oid)) { obj = lookup_object(the_repository, oid); if (obj && (obj->flags & HAS_OBJ)) { - if (timestamp && name_objects) - add_decoration(fsck_walk_options.object_names, - obj, - xstrfmt("%s@{%"PRItime"}", refname, timestamp)); + if (timestamp) + fsck_put_object_name(&fsck_walk_options, obj, + "%s@{%"PRItime"}", + refname, timestamp); obj->flags |= USED; mark_object_reachable(obj); } else if (!is_promisor_object(oid)) { @@ -566,9 +549,8 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid, } default_refs++; obj->flags |= USED; - if (name_objects) - add_decoration(fsck_walk_options.object_names, - obj, xstrdup(refname)); + fsck_put_object_name(&fsck_walk_options, + obj, "%s", refname); mark_object_reachable(obj); return 0; @@ -742,9 +724,7 @@ static int fsck_cache_tree(struct cache_tree *it) return 1; } obj->flags |= USED; - if (name_objects) - add_decoration(fsck_walk_options.object_names, - obj, xstrdup(":")); + fsck_put_object_name(&fsck_walk_options, obj, ":"); mark_object_reachable(obj); if (obj->type != OBJ_TREE) err |= objerror(obj, _("non-tree in cache-tree")); @@ -830,8 +810,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) } if (name_objects) - fsck_walk_options.object_names = - xcalloc(1, sizeof(struct decoration)); + fsck_enable_object_names(&fsck_walk_options); git_config(fsck_config, NULL); @@ -890,9 +869,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) } obj->flags |= USED; - if (name_objects) - add_decoration(fsck_walk_options.object_names, - obj, xstrdup(arg)); + fsck_put_object_name(&fsck_walk_options, obj, + "%s", arg); mark_object_reachable(obj); continue; } @@ -928,10 +906,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) continue; obj = &blob->object; obj->flags |= USED; - if (name_objects) - add_decoration(fsck_walk_options.object_names, - obj, - xstrfmt(":%s", active_cache[i]->name)); + fsck_put_object_name(&fsck_walk_options, obj, + ":%s", active_cache[i]->name); mark_object_reachable(obj); } if (active_cache_tree) -- cgit v1.2.3 From 733902905d4db54612fef9755bb31fd35a89e76c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 18 Oct 2019 00:57:37 -0400 Subject: fsck: use oids rather than objects for object_name API We don't actually care about having object structs; we only need to look up decorations by oid. Let's accept this more limited form, which will give our callers more flexibility. Note that the decoration API we rely on uses object structs itself (even though it only looks at their oids). We can solve this by switching to a kh_oid_map (we could also use the hashmap oidmap, but it's more awkward for the simple case of just storing a void pointer). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fsck.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'builtin') diff --git a/builtin/fsck.c b/builtin/fsck.c index 237643cc1d..66fa727c14 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -52,7 +52,7 @@ static int name_objects; static const char *describe_object(struct object *obj) { - return fsck_describe_object(&fsck_walk_options, obj); + return fsck_describe_object(&fsck_walk_options, &obj->oid); } static const char *printable_type(struct object *obj) @@ -483,7 +483,7 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid, obj = lookup_object(the_repository, oid); if (obj && (obj->flags & HAS_OBJ)) { if (timestamp) - fsck_put_object_name(&fsck_walk_options, obj, + fsck_put_object_name(&fsck_walk_options, oid, "%s@{%"PRItime"}", refname, timestamp); obj->flags |= USED; @@ -550,7 +550,7 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid, default_refs++; obj->flags |= USED; fsck_put_object_name(&fsck_walk_options, - obj, "%s", refname); + oid, "%s", refname); mark_object_reachable(obj); return 0; @@ -724,7 +724,7 @@ static int fsck_cache_tree(struct cache_tree *it) return 1; } obj->flags |= USED; - fsck_put_object_name(&fsck_walk_options, obj, ":"); + fsck_put_object_name(&fsck_walk_options, &it->oid, ":"); mark_object_reachable(obj); if (obj->type != OBJ_TREE) err |= objerror(obj, _("non-tree in cache-tree")); @@ -869,7 +869,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) } obj->flags |= USED; - fsck_put_object_name(&fsck_walk_options, obj, + fsck_put_object_name(&fsck_walk_options, &oid, "%s", arg); mark_object_reachable(obj); continue; @@ -906,7 +906,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) continue; obj = &blob->object; obj->flags |= USED; - fsck_put_object_name(&fsck_walk_options, obj, + fsck_put_object_name(&fsck_walk_options, &obj->oid, ":%s", active_cache[i]->name); mark_object_reachable(obj); } -- cgit v1.2.3 From 82ef89b318a3c88a3e6af21a05b75abf56d715da Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 18 Oct 2019 00:58:07 -0400 Subject: fsck: don't require object structs for display functions Our printable_type() and describe_object() functions take whole object structs, but they really only care about the oid and type. Let's take those individually in order to give our callers more flexibility. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fsck.c | 69 +++++++++++++++++++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 32 deletions(-) (limited to 'builtin') diff --git a/builtin/fsck.c b/builtin/fsck.c index 66fa727c14..59c77c1baa 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -50,23 +50,20 @@ static int name_objects; #define ERROR_REFS 010 #define ERROR_COMMIT_GRAPH 020 -static const char *describe_object(struct object *obj) +static const char *describe_object(const struct object_id *oid) { - return fsck_describe_object(&fsck_walk_options, &obj->oid); + return fsck_describe_object(&fsck_walk_options, oid); } -static const char *printable_type(struct object *obj) +static const char *printable_type(const struct object_id *oid, + enum object_type type) { const char *ret; - if (obj->type == OBJ_NONE) { - enum object_type type = oid_object_info(the_repository, - &obj->oid, NULL); - if (type > 0) - object_as_type(the_repository, obj, type, 0); - } + if (type == OBJ_NONE) + type = oid_object_info(the_repository, oid, NULL); - ret = type_name(obj->type); + ret = type_name(type); if (!ret) ret = _("unknown"); @@ -101,7 +98,8 @@ static int objerror(struct object *obj, const char *err) errors_found |= ERROR_OBJECT; /* TRANSLATORS: e.g. error in tree 01bfda: */ fprintf_ln(stderr, _("error in %s %s: %s"), - printable_type(obj), describe_object(obj), err); + printable_type(&obj->oid, obj->type), + describe_object(&obj->oid), err); return -1; } @@ -112,12 +110,14 @@ static int fsck_error_func(struct fsck_options *o, case FSCK_WARN: /* TRANSLATORS: e.g. warning in tree 01bfda: */ fprintf_ln(stderr, _("warning in %s %s: %s"), - printable_type(obj), describe_object(obj), message); + printable_type(&obj->oid, obj->type), + describe_object(&obj->oid), message); return 0; case FSCK_ERROR: /* TRANSLATORS: e.g. error in tree 01bfda: */ fprintf_ln(stderr, _("error in %s %s: %s"), - printable_type(obj), describe_object(obj), message); + printable_type(&obj->oid, obj->type), + describe_object(&obj->oid), message); return 1; default: BUG("%d (FSCK_IGNORE?) should never trigger this callback", type); @@ -138,7 +138,8 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt if (!obj) { /* ... these references to parent->fld are safe here */ printf_ln(_("broken link from %7s %s"), - printable_type(parent), describe_object(parent)); + printable_type(&parent->oid, parent->type), + describe_object(&parent->oid)); printf_ln(_("broken link from %7s %s"), (type == OBJ_ANY ? _("unknown") : type_name(type)), _("unknown")); @@ -166,10 +167,10 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt if (parent && !has_object_file(&obj->oid)) { printf_ln(_("broken link from %7s %s\n" " to %7s %s"), - printable_type(parent), - describe_object(parent), - printable_type(obj), - describe_object(obj)); + printable_type(&parent->oid, parent->type), + describe_object(&parent->oid), + printable_type(&obj->oid, obj->type), + describe_object(&obj->oid)); errors_found |= ERROR_REACHABLE; } return 1; @@ -275,8 +276,9 @@ static void check_reachable_object(struct object *obj) return; if (has_object_pack(&obj->oid)) return; /* it is in pack - forget about it */ - printf_ln(_("missing %s %s"), printable_type(obj), - describe_object(obj)); + printf_ln(_("missing %s %s"), + printable_type(&obj->oid, obj->type), + describe_object(&obj->oid)); errors_found |= ERROR_REACHABLE; return; } @@ -301,8 +303,9 @@ static void check_unreachable_object(struct object *obj) * since this is something that is prunable. */ if (show_unreachable) { - printf_ln(_("unreachable %s %s"), printable_type(obj), - describe_object(obj)); + printf_ln(_("unreachable %s %s"), + printable_type(&obj->oid, obj->type), + describe_object(&obj->oid)); return; } @@ -320,12 +323,13 @@ static void check_unreachable_object(struct object *obj) */ if (!(obj->flags & USED)) { if (show_dangling) - printf_ln(_("dangling %s %s"), printable_type(obj), - describe_object(obj)); + printf_ln(_("dangling %s %s"), + printable_type(&obj->oid, obj->type), + describe_object(&obj->oid)); if (write_lost_and_found) { char *filename = git_pathdup("lost-found/%s/%s", obj->type == OBJ_COMMIT ? "commit" : "other", - describe_object(obj)); + describe_object(&obj->oid)); FILE *f; if (safe_create_leading_directories_const(filename)) { @@ -338,7 +342,7 @@ static void check_unreachable_object(struct object *obj) if (stream_blob_to_fd(fileno(f), &obj->oid, NULL, 1)) die_errno(_("could not write '%s'"), filename); } else - fprintf(f, "%s\n", describe_object(obj)); + fprintf(f, "%s\n", describe_object(&obj->oid)); if (fclose(f)) die_errno(_("could not finish '%s'"), filename); @@ -357,7 +361,7 @@ static void check_unreachable_object(struct object *obj) static void check_object(struct object *obj) { if (verbose) - fprintf_ln(stderr, _("Checking %s"), describe_object(obj)); + fprintf_ln(stderr, _("Checking %s"), describe_object(&obj->oid)); if (obj->flags & REACHABLE) check_reachable_object(obj); @@ -415,7 +419,8 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size) if (verbose) fprintf_ln(stderr, _("Checking %s %s"), - printable_type(obj), describe_object(obj)); + printable_type(&obj->oid, obj->type), + describe_object(&obj->oid)); if (fsck_walk(obj, NULL, &fsck_obj_options)) objerror(obj, _("broken links")); @@ -428,7 +433,7 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size) if (!commit->parents && show_root) printf_ln(_("root %s"), - describe_object(&commit->object)); + describe_object(&commit->object.oid)); } if (obj->type == OBJ_TAG) { @@ -436,10 +441,10 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size) if (show_tags && tag->tagged) { printf_ln(_("tagged %s %s (%s) in %s"), - printable_type(tag->tagged), - describe_object(tag->tagged), + printable_type(&tag->tagged->oid, tag->tagged->type), + describe_object(&tag->tagged->oid), tag->tag, - describe_object(&tag->object)); + describe_object(&tag->object.oid)); } } -- cgit v1.2.3 From 5afc4b1dc622d574bcd67b5845789a0b5875431a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 18 Oct 2019 00:58:40 -0400 Subject: fsck: only provide oid/type in fsck_error callback None of the callbacks actually care about having a "struct object"; they're happy with just the oid and type information. So let's give ourselves more flexibility to avoid having a "struct object" by just passing the broken-down fields. Note that the callback already takes a "type" field for the fsck message type. We'll rename that to "msg_type" (and use "object_type" for the object type) to make the distinction explicit. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fsck.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) (limited to 'builtin') diff --git a/builtin/fsck.c b/builtin/fsck.c index 59c77c1baa..8d13794b14 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -104,23 +104,26 @@ static int objerror(struct object *obj, const char *err) } static int fsck_error_func(struct fsck_options *o, - struct object *obj, int type, const char *message) + const struct object_id *oid, + enum object_type object_type, + int msg_type, const char *message) { - switch (type) { + switch (msg_type) { case FSCK_WARN: /* TRANSLATORS: e.g. warning in tree 01bfda: */ fprintf_ln(stderr, _("warning in %s %s: %s"), - printable_type(&obj->oid, obj->type), - describe_object(&obj->oid), message); + printable_type(oid, object_type), + describe_object(oid), message); return 0; case FSCK_ERROR: /* TRANSLATORS: e.g. error in tree 01bfda: */ fprintf_ln(stderr, _("error in %s %s: %s"), - printable_type(&obj->oid, obj->type), - describe_object(&obj->oid), message); + printable_type(oid, object_type), + describe_object(oid), message); return 1; default: - BUG("%d (FSCK_IGNORE?) should never trigger this callback", type); + BUG("%d (FSCK_IGNORE?) should never trigger this callback", + msg_type); } } -- cgit v1.2.3