{"thread":{"id":"52064","subject":"[PATCH 08/23] fsck: require an actual buffer for non-blobs","startedAt":"2019-10-18T04:54:18Z","lastAt":"2019-10-25T21:20:23Z","messageCount":39,"participants":["Jeff King","Junio C Hamano","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":23},"messages":[{"id":"384337","messageId":"20191018045412.GH17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 08/23] fsck: require an actual buffer for non-blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:54:12Z","receivedAt":"2019-10-18T04:54:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The fsck_object() function takes in a buffer, but also a \"struct\nobject\". The rules for using these vary between types:\n\n - for a commit, we'll use the provided buffer; if it's NULL, we'll\n fall back to get_commit_buffer(), which loads from either an\n in-memory cache or from disk. If the latter fails, we'd die(), which\n is non-ideal for fsck.\n\n - for a tag, a NULL buffer will fall back to loading the object from\n disk (and failure would lead to an fsck error)\n\n - for a tree, we _never_ look at the provided buffer, and always use\n tree->buffer\n\n - for a blob, we usually don't look at the buffer at all, unless it\n has been marked as a .gitmodule file. In that case we check the\n buffer given to us, or assume a NULL buffer is a very large blob\n (and complain about it)\n\nThis is much more complex than it needs to be. It turns out that nobody\never feeds a NULL buffer that isn't a blob:\n\n - git-fsck calls fsck_object() only from fsck_obj(). That in turn is\n called by one of:\n\n - fsck_obj_buffer(), which is a callback to verify_pack(), which\n\tunpacks everything except large blobs into a buffer (see\n\tpack-check.c, lines 131-141).\n\n - fsck_loose(), which hits a BUG() on non-blobs with a NULL buffer\n\t(builtin/fsck.c, lines 639-640)\n\n And in either case, we'll have just called parse_object_buffer()\n anyway, which would segfault on a NULL buffer for commits or tags\n (not for trees, but it would install a NULL tree->buffer which would\n later cause a segfault)\n\n - git-index-pack asserts that the buffer is non-NULL unless the object\n is a blob (see builtin/index-pack.c, line 832)\n\n - git-unpack-objects always writes a non-NULL buffer into its\n obj_buffer hash, which is then fed to fsck_object(). (There is\n actually a funny thing here where it does not store blob buffers at\n all, nor does it call fsck on them; it does check any needed blobs\n via fsck_finish() though).\n\nLet's make the rules simpler, which reduces the amount of code and gives\nus more flexibility in refactoring the fsck code. The new rules are:\n\n - only blobs are allowed to pass a NULL buffer\n\n - we always use the provided buffer, never pulling information from\n the object struct\n\nWe don't have to adjust any callers, because they were already adhering\nto these. Note that we do drop a few fsck identifiers for missing tags,\nbut that was all dead code (because nobody passed a NULL tag buffer).\n\nSigned-off-by: Jeff King \n---\n fsck.c | 51 +++++++++------------------------------------------\n fsck.h | 6 +++++-\n 2 files changed, 14 insertions(+), 43 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 79ce3a97c8..347a0ef5c9 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -49,13 +49,11 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \\\n \tFUNC(MISSING_TAG, ERROR) \\\n \tFUNC(MISSING_TAG_ENTRY, ERROR) \\\n-\tFUNC(MISSING_TAG_OBJECT, ERROR) \\\n \tFUNC(MISSING_TREE, ERROR) \\\n \tFUNC(MISSING_TREE_OBJECT, ERROR) \\\n \tFUNC(MISSING_TYPE, ERROR) \\\n \tFUNC(MISSING_TYPE_ENTRY, ERROR) \\\n \tFUNC(MULTIPLE_AUTHORS, ERROR) \\\n-\tFUNC(TAG_OBJECT_NOT_TAG, ERROR) \\\n \tFUNC(TREE_NOT_SORTED, ERROR) \\\n \tFUNC(UNKNOWN_TYPE, ERROR) \\\n \tFUNC(ZERO_PADDED_DATE, ERROR) \\\n@@ -541,7 +539,9 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n \n-static int fsck_tree(struct tree *item, struct fsck_options *options)\n+static int fsck_tree(struct tree *item,\n+\t\t const char *buffer, unsigned long size,\n+\t\t struct fsck_options *options)\n {\n \tint retval = 0;\n \tint has_null_sha1 = 0;\n@@ -558,7 +558,7 @@ static int fsck_tree(struct tree *item, struct fsck_options *options)\n \tunsigned o_mode;\n \tconst char *o_name;\n \n-\tif (init_tree_desc_gently(&desc, item->buffer, item->size)) {\n+\tif (init_tree_desc_gently(&desc, buffer, size)) {\n \t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n@@ -733,8 +733,8 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option\n \treturn 0;\n }\n \n-static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n-\tunsigned long size, struct fsck_options *options)\n+static int fsck_commit(struct commit *commit, const char *buffer,\n+\t\t unsigned long size, struct fsck_options *options)\n {\n \tstruct object_id tree_oid, oid;\n \tunsigned author_count;\n@@ -788,47 +788,15 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \treturn 0;\n }\n \n-static int fsck_commit(struct commit *commit, const char *data,\n-\tunsigned long size, struct fsck_options *options)\n-{\n-\tconst char *buffer = data ? data : get_commit_buffer(commit, &size);\n-\tint ret = fsck_commit_buffer(commit, buffer, size, options);\n-\tif (!data)\n-\t\tunuse_commit_buffer(commit, buffer);\n-\treturn ret;\n-}\n-\n-static int fsck_tag(struct tag *tag, const char *data,\n+static int fsck_tag(struct tag *tag, const char *buffer,\n \t\t unsigned long size, struct fsck_options *options)\n {\n \tstruct object_id oid;\n \tint ret = 0;\n-\tconst char *buffer;\n-\tchar *to_free = NULL, *eol;\n+\tchar *eol;\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *p;\n \n-\tif (data)\n-\t\tbuffer = data;\n-\telse {\n-\t\tenum object_type type;\n-\n-\t\tbuffer = to_free =\n-\t\t\tread_object_file(&tag->object.oid, &type, &size);\n-\t\tif (!buffer)\n-\t\t\treturn report(options, &tag->object,\n-\t\t\t\tFSCK_MSG_MISSING_TAG_OBJECT,\n-\t\t\t\t\"cannot read tag object\");\n-\n-\t\tif (type != OBJ_TAG) {\n-\t\t\tret = report(options, &tag->object,\n-\t\t\t\tFSCK_MSG_TAG_OBJECT_NOT_TAG,\n-\t\t\t\t\"expected tag got %s\",\n-\t\t\t type_name(type));\n-\t\t\tgoto done;\n-\t\t}\n-\t}\n-\n \tret = verify_headers(buffer, size, &tag->object, options);\n \tif (ret)\n \t\tgoto done;\n@@ -889,7 +857,6 @@ static int fsck_tag(struct tag *tag, const char *data,\n \n done:\n \tstrbuf_release(&sb);\n-\tfree(to_free);\n \treturn ret;\n }\n \n@@ -979,7 +946,7 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \tif (obj->type == OBJ_BLOB)\n \t\treturn fsck_blob((struct blob *)obj, data, size, options);\n \tif (obj->type == OBJ_TREE)\n-\t\treturn fsck_tree((struct tree *) obj, options);\n+\t\treturn fsck_tree((struct tree *) obj, data, size, options);\n \tif (obj->type == OBJ_COMMIT)\n \t\treturn fsck_commit((struct commit *) obj, (const char *) data,\n \t\t\tsize, options);\ndiff --git a/fsck.h b/fsck.h\nindex b95595ae5f..e479461075 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -52,7 +52,11 @@ struct fsck_options {\n * 0\t\teverything OK\n */\n int fsck_walk(struct object *obj, void *data, struct fsck_options *options);\n-/* If NULL is passed for data, we assume the object is local and read it. */\n+\n+/*\n+ * Blob objects my pass a NULL data pointer, which indicates they are too large\n+ * to fit in memory. All other types must pass a real buffer.\n+ */\n int fsck_object(struct object *obj, void *data, unsigned long size,\n \tstruct fsck_options *options);\n \n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384338","messageId":"20191018044721.GD17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 04/23] remember commit/tag parse failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:47:21Z","receivedAt":"2019-10-18T04:54:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we can't parse a commit, then parse_commit() will return an error\ncode. But it _also_ sets the \"parsed\" flag, which tells us not to bother\ntrying to re-parse the object. That means that subsequent parses have no\nidea that the information in the struct may be bogus. I.e., doing this:\n\n parse_commit(commit);\n ...\n if (parse_commit(commit) < 0)\n die(\"commit is broken\");\n\nwill never trigger the die(). The second parse_commit() will see the\n\"parsed\" flag and quietly return success.\n\nThere are two obvious ways to fix this:\n\n 1. Stop setting \"parsed\" until we've successfully parsed.\n\n 2. Keep a second \"corrupt\" flag to indicate that we saw an error (and\n when the parsed flag is set, return 0/-1 depending on the corrupt\n flag).\n\nThis patch does option 1. The obvious downside versus option 2 is that\nwe might continually re-parse a broken object. But in practice,\ncorruption like this is rare, and we typically die() or return an error\nin the caller. So it's OK not to worry about optimizing for corruption.\nAnd it's much simpler: we don't need to use an extra bit in the object\nstruct, and callers which check the \"parsed\" flag don't need to learn\nabout the corrupt bit, too.\n\nThere's no new test here, because this case is already covered in t5318.\nNote that we do need to update the expected message there, because we\nnow detect the problem in the return from \"parse_commit()\", and not with\na separate check for a NULL tree. In fact, we can now ditch that\nexplicit tree check entirely, as we're covered robustly by this change\n(and the previous recent change to treat a NULL tree as a parse error).\n\nWe'll also give tags the same treatment. I don't know offhand of any\ncases where the problem can be triggered (it implies somebody ignoring a\nparse error earlier in the process), but consistently returning an error\nshould cause the least surprise.\n\nSigned-off-by: Jeff King \n---\n commit-graph.c | 3 ---\n commit.c | 14 +++++++++++++-\n t/t5318-commit-graph.sh | 2 +-\n tag.c | 12 +++++++++++-\n 4 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex fc4a43b8d6..852b9c39e6 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -855,9 +855,6 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \t\t\tdie(_(\"unable to parse commit %s\"),\n \t\t\t\toid_to_hex(&(*list)->object.oid));\n \t\ttree = get_commit_tree_oid(*list);\n-\t\tif (!tree)\n-\t\t\tdie(_(\"unable to get tree for %s\"),\n-\t\t\t\toid_to_hex(&(*list)->object.oid));\n \t\thashwrite(f, tree->hash, hash_len);\n \n \t\tparent = (*list)->parents;\ndiff --git a/commit.c b/commit.c\nindex 810419a168..e12e7998ad 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -405,7 +405,18 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\titem->object.parsed = 1;\n+\n+\tif (item->parents) {\n+\t\t/*\n+\t\t * Presumably this is leftover from an earlier failed parse;\n+\t\t * clear it out in preparation for us re-parsing (we'll hit the\n+\t\t * same error, but that's good, since it lets our caller know\n+\t\t * the result cannot be trusted.\n+\t\t */\n+\t\tfree_commit_list(item->parents);\n+\t\titem->parents = NULL;\n+\t}\n+\n \ttail += size;\n \tif (tail <= bufptr + tree_entry_len + 1 || memcmp(bufptr, \"tree \", 5) ||\n \t\t\tbufptr[tree_entry_len] != '\\n')\n@@ -462,6 +473,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tif (check_graph)\n \t\tload_commit_graph_info(r, item);\n \n+\titem->object.parsed = 1;\n \treturn 0;\n }\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d42b3efe39..127b404856 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -660,7 +660,7 @@ test_expect_success 'corrupt commit-graph write (missing tree)' '\n \t\tgit commit-tree -p \"$broken\" -m \"good\" \"$tree\" >good &&\n \t\ttest_must_fail git commit-graph write --stdin-commits \\\n \t\t\ttest_err &&\n-\t\ttest_i18ngrep \"unable to get tree for\" test_err\n+\t\ttest_i18ngrep \"unable to parse commit\" test_err\n \t)\n '\n \ndiff --git a/tag.c b/tag.c\nindex 6a51efda8d..71b544467e 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -141,7 +141,16 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\titem->object.parsed = 1;\n+\n+\tif (item->tag) {\n+\t\t/*\n+\t\t * Presumably left over from a previous failed parse;\n+\t\t * clear it out in preparation for re-parsing (we'll probably\n+\t\t * hit the same error, which lets us tell our current caller\n+\t\t * about the problem).\n+\t\t */\n+\t\tFREE_AND_NULL(item->tag);\n+\t}\n \n \tif (size < the_hash_algo->hexsz + 24)\n \t\treturn -1;\n@@ -192,6 +201,7 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \telse\n \t\titem->date = 0;\n \n+\titem->object.parsed = 1;\n \treturn 0;\n }\n \n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384339","messageId":"20191018044820.GE17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 05/23] fsck: stop checking commit->tree value","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:48:20Z","receivedAt":"2019-10-18T04:55:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We check in fsck_commit_buffer() that commit->tree isn't NULL, which in\nturn generally comes from a previous parse by parse_commit(). But this\nisn't really accomplishing anything. The two things we might care about\nare:\n\n - was there a syntactically valid \"tree \" line in the object? But\n we've just done our own parse in fsck_commit_buffer() to check this.\n\n - does it point to a valid tree object? But checking the \"tree\"\n pointer here doesn't actually accomplish that; it just shows that\n lookup_tree() didn't return NULL, which only means that we haven't\n yet seen that oid as a non-tree in this process.\n\n A real connectivity check would exhaustively walk all graph links,\n and we do that already in a separate function.\n\nSo this code isn't helping anything. And it makes the fsck code slightly\nmore confusing and rigid (e.g., it requires that any commit structs have\nalready been parsed). Let's drop it.\n\nAs a bit of history, the presence of this code looks like a leftover\nfrom early fsck code (which did rely on parse_commit() to do most of the\nparsing). The check comes from ff5ebe39b0 (Port fsck-cache to use\nparsing functions, 2005-04-18), but we later added an explicit walk in\n355885d531 (add generic, type aware object chain walker, 2008-02-25).\n\nSigned-off-by: Jeff King \n---\n fsck.c | 5 -----\n 1 file changed, 5 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex cdb7d8db03..6dfc533fb0 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -800,11 +800,6 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \terr = fsck_ident(&buffer, &commit->object, options);\n \tif (err)\n \t\treturn err;\n-\tif (!get_commit_tree(commit)) {\n-\t\terr = report(options, &commit->object, FSCK_MSG_BAD_TREE, \"could not load commit's tree %s\", oid_to_hex(&tree_oid));\n-\t\tif (err)\n-\t\t\treturn err;\n-\t}\n \tif (memchr(buffer_begin, '\\0', size)) {\n \t\terr = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,\n \t\t\t \"NUL byte in the commit object body\");\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384340","messageId":"20191018044910.GF17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 06/23] fsck: stop checking commit->parent counts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:49:10Z","receivedAt":"2019-10-18T04:55:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In 4516338243 (builtin-fsck: reports missing parent commits,\n2008-02-25), we added code to check that fsck found the same number of\nparents from parsing the commit itself as we see in the commit struct we\ngot from parse_commit_buffer(). Back then the rationale was that the\nnormal commit parser might skip some bad parents.\n\nBut earlier in this series, we started treating that reliably as a\nparsing error, meaning that we'd complain about it before we even hit\nthe code in fsck.c.\n\nLet's drop this code, which now makes fsck_commit_buffer() completely\nindependent of any parsed values in the commit struct (that's\nconceptually cleaner, and also opens up more refactoring options).\n\nNote that we can also drop the MISSING_PARENT and MISSING_GRAFT fsck\nidentifiers. This is no loss, as these would not trigger reliably\nanyway. We'd hit them only when lookup_commit() failed, which occurs\nonly if we happen to have seen the object with another type already in\nthe same process. In most cases, we'd actually run into the problem\nduring the connectivity walk, not here.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 23 +----------------------\n 1 file changed, 1 insertion(+), 22 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 6dfc533fb0..a0f8ae7650 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -43,10 +43,8 @@ static struct oidset gitmodules_done = OIDSET_INIT;\n \tFUNC(MISSING_AUTHOR, ERROR) \\\n \tFUNC(MISSING_COMMITTER, ERROR) \\\n \tFUNC(MISSING_EMAIL, ERROR) \\\n-\tFUNC(MISSING_GRAFT, ERROR) \\\n \tFUNC(MISSING_NAME_BEFORE_EMAIL, ERROR) \\\n \tFUNC(MISSING_OBJECT, ERROR) \\\n-\tFUNC(MISSING_PARENT, ERROR) \\\n \tFUNC(MISSING_SPACE_BEFORE_DATE, ERROR) \\\n \tFUNC(MISSING_SPACE_BEFORE_EMAIL, ERROR) \\\n \tFUNC(MISSING_TAG, ERROR) \\\n@@ -739,8 +737,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \tunsigned long size, struct fsck_options *options)\n {\n \tstruct object_id tree_oid, oid;\n-\tstruct commit_graft *graft;\n-\tunsigned parent_count, parent_line_count = 0, author_count;\n+\tunsigned author_count;\n \tint err;\n \tconst char *buffer_begin = buffer;\n \tconst char *p;\n@@ -763,24 +760,6 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \t\t\t\treturn err;\n \t\t}\n \t\tbuffer = p + 1;\n-\t\tparent_line_count++;\n-\t}\n-\tgraft = lookup_commit_graft(the_repository, &commit->object.oid);\n-\tparent_count = commit_list_count(commit->parents);\n-\tif (graft) {\n-\t\tif (graft->nr_parent == -1 && !parent_count)\n-\t\t\t; /* shallow commit */\n-\t\telse if (graft->nr_parent != parent_count) {\n-\t\t\terr = report(options, &commit->object, FSCK_MSG_MISSING_GRAFT, \"graft objects missing\");\n-\t\t\tif (err)\n-\t\t\t\treturn err;\n-\t\t}\n-\t} else {\n-\t\tif (parent_count != parent_line_count) {\n-\t\t\terr = report(options, &commit->object, FSCK_MSG_MISSING_PARENT, \"parent objects missing\");\n-\t\t\tif (err)\n-\t\t\t\treturn err;\n-\t\t}\n \t}\n \tauthor_count = 0;\n \twhile (skip_prefix(buffer, \"author \", &buffer)) {\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384341","messageId":"20191018045613.GI17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 09/23] fsck: unify object-name code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:56:13Z","receivedAt":"2019-10-18T04:56:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 90cf590f53 (fsck: optionally show more helpful info for broken\nlinks, 2016-07-17) added a system for decorating objects with names. The\ncode is split across builtin/fsck.c (which gives the initial names) and\nfsck.c (which adds to the names as it traverses the object graph). This\nleads to some duplication, where both sites have near-identical\ndescribe_object() functions (the difference being that the one in\nbuiltin/fsck.c uses a circular array of buffers to allow multiple calls\nin a single printf).\n\nLet's provide a unified object_name API for fsck. That lets us drop the\nduplication, as well as making the interface boundaries more clear\n(which will let us refactor the implementation more in a future patch).\n\nWe'll leave describe_object() in builtin/fsck.c as a thin wrapper around\nthe new API, as it relies on a static global to make its many callers a\nbit shorter.\n\nWe'll also convert the bare add_decoration() calls in builtin/fsck.c to\nput_object_name(). This fixes two minor bugs:\n\n 1. We leak many small strings. add_decoration() has a last-one-wins\n approach: it updates the decoration to the new string and returns\n the old one. But we ignore the return value, leaking the old\n string. This is quite common to trigger, since we look at reflogs:\n the tip of any ref will be described both by looking at the actual\n ref, as well as the latest reflog entry. So we'd always end up\n leaking one of those strings.\n\n 2. The last-one-wins approach gives us lousy names. For instance, we\n first look at all of the refs, and then all of the reflogs. So\n rather than seeing \"refs/heads/master\", we're likely to overwrite\n it with \"HEAD@{12345678}\". We're generally better off using the\n first name we find.\n\n And indeed, the test in t1450 expects this ugly HEAD@{} name. After\n this patch, we've switched to using fsck_put_object_name()'s\n first-one-wins semantics, and we output the more human-friendly\n \"refs/tags/julius\" (and the test is updated accordingly).\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 50 ++++++++----------------------\n fsck.c | 82 ++++++++++++++++++++++++++++++-------------------\n fsck.h | 24 +++++++++++++++\n t/t1450-fsck.sh | 2 +-\n 4 files changed, 89 insertions(+), 69 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 18403a94fa..237643cc1d 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -52,24 +52,7 @@ static int name_objects;\n \n static const char *describe_object(struct object *obj)\n {\n-\tstatic struct strbuf bufs[] = {\n-\t\tSTRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n-\t};\n-\tstatic int b = 0;\n-\tstruct strbuf *buf;\n-\tchar *name = NULL;\n-\n-\tif (name_objects)\n-\t\tname = lookup_decoration(fsck_walk_options.object_names, obj);\n-\n-\tbuf = bufs + b;\n-\tb = (b + 1) % ARRAY_SIZE(bufs);\n-\tstrbuf_reset(buf);\n-\tstrbuf_addstr(buf, oid_to_hex(&obj->oid));\n-\tif (name)\n-\t\tstrbuf_addf(buf, \" (%s)\", name);\n-\n-\treturn buf->buf;\n+\treturn fsck_describe_object(&fsck_walk_options, obj);\n }\n \n static const char *printable_type(struct object *obj)\n@@ -499,10 +482,10 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,\n \tif (!is_null_oid(oid)) {\n \t\tobj = lookup_object(the_repository, oid);\n \t\tif (obj && (obj->flags & HAS_OBJ)) {\n-\t\t\tif (timestamp && name_objects)\n-\t\t\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\t\t\tobj,\n-\t\t\t\t\txstrfmt(\"%s@{%\"PRItime\"}\", refname, timestamp));\n+\t\t\tif (timestamp)\n+\t\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\t\t\t\t \"%s@{%\"PRItime\"}\",\n+\t\t\t\t\t\t refname, timestamp);\n \t\t\tobj->flags |= USED;\n \t\t\tmark_object_reachable(obj);\n \t\t} else if (!is_promisor_object(oid)) {\n@@ -566,9 +549,8 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid,\n \t}\n \tdefault_refs++;\n \tobj->flags |= USED;\n-\tif (name_objects)\n-\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\tobj, xstrdup(refname));\n+\tfsck_put_object_name(&fsck_walk_options,\n+\t\t\t obj, \"%s\", refname);\n \tmark_object_reachable(obj);\n \n \treturn 0;\n@@ -742,9 +724,7 @@ static int fsck_cache_tree(struct cache_tree *it)\n \t\t\treturn 1;\n \t\t}\n \t\tobj->flags |= USED;\n-\t\tif (name_objects)\n-\t\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\t\tobj, xstrdup(\":\"));\n+\t\tfsck_put_object_name(&fsck_walk_options, obj, \":\");\n \t\tmark_object_reachable(obj);\n \t\tif (obj->type != OBJ_TREE)\n \t\t\terr |= objerror(obj, _(\"non-tree in cache-tree\"));\n@@ -830,8 +810,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (name_objects)\n-\t\tfsck_walk_options.object_names =\n-\t\t\txcalloc(1, sizeof(struct decoration));\n+\t\tfsck_enable_object_names(&fsck_walk_options);\n \n \tgit_config(fsck_config, NULL);\n \n@@ -890,9 +869,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \n \t\t\tobj->flags |= USED;\n-\t\t\tif (name_objects)\n-\t\t\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\t\t\tobj, xstrdup(arg));\n+\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\t\t\t \"%s\", arg);\n \t\t\tmark_object_reachable(obj);\n \t\t\tcontinue;\n \t\t}\n@@ -928,10 +906,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\tobj = &blob->object;\n \t\t\tobj->flags |= USED;\n-\t\t\tif (name_objects)\n-\t\t\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\t\t\tobj,\n-\t\t\t\t\txstrfmt(\":%s\", active_cache[i]->name));\n+\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\t\t\t \":%s\", active_cache[i]->name);\n \t\t\tmark_object_reachable(obj);\n \t\t}\n \t\tif (active_cache_tree)\ndiff --git a/fsck.c b/fsck.c\nindex 347a0ef5c9..ecd5957362 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -312,15 +312,21 @@ static int report(struct fsck_options *options, struct object *object,\n \treturn result;\n }\n \n-static char *get_object_name(struct fsck_options *options, struct object *obj)\n+void fsck_enable_object_names(struct fsck_options *options)\n+{\n+\tif (!options->object_names)\n+\t\toptions->object_names = xcalloc(1, sizeof(struct decoration));\n+}\n+\n+const char *fsck_get_object_name(struct fsck_options *options, struct object *obj)\n {\n \tif (!options->object_names)\n \t\treturn NULL;\n \treturn lookup_decoration(options->object_names, obj);\n }\n \n-static void put_object_name(struct fsck_options *options, struct object *obj,\n-\tconst char *fmt, ...)\n+void fsck_put_object_name(struct fsck_options *options, struct object *obj,\n+\t\t\t const char *fmt, ...)\n {\n \tva_list ap;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -337,17 +343,27 @@ static void put_object_name(struct fsck_options *options, struct object *obj,\n \tva_end(ap);\n }\n \n-static const char *describe_object(struct fsck_options *o, struct object *obj)\n+const char *fsck_describe_object(struct fsck_options *options,\n+\t\t\t\t struct object *obj)\n {\n-\tstatic struct strbuf buf = STRBUF_INIT;\n-\tchar *name;\n-\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addstr(&buf, oid_to_hex(&obj->oid));\n-\tif (o->object_names && (name = lookup_decoration(o->object_names, obj)))\n-\t\tstrbuf_addf(&buf, \" (%s)\", name);\n+\tstatic struct strbuf bufs[] = {\n+\t\tSTRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n+\t};\n+\tstatic int b = 0;\n+\tstruct strbuf *buf;\n+\tchar *name = NULL;\n+\n+\tif (options->object_names)\n+\t\tname = lookup_decoration(options->object_names, obj);\n+\n+\tbuf = bufs + b;\n+\tb = (b + 1) % ARRAY_SIZE(bufs);\n+\tstrbuf_reset(buf);\n+\tstrbuf_addstr(buf, oid_to_hex(&obj->oid));\n+\tif (name)\n+\t\tstrbuf_addf(buf, \" (%s)\", name);\n \n-\treturn buf.buf;\n+\treturn buf->buf;\n }\n \n static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *options)\n@@ -360,7 +376,7 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \tif (parse_tree(tree))\n \t\treturn -1;\n \n-\tname = get_object_name(options, &tree->object);\n+\tname = fsck_get_object_name(options, &tree->object);\n \tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n \t\treturn -1;\n \twhile (tree_entry_gently(&desc, &entry)) {\n@@ -373,20 +389,21 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\tif (S_ISDIR(entry.mode)) {\n \t\t\tobj = (struct object *)lookup_tree(the_repository, &entry.oid);\n \t\t\tif (name && obj)\n-\t\t\t\tput_object_name(options, obj, \"%s%s/\", name,\n-\t\t\t\t\tentry.path);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%s%s/\",\n+\t\t\t\t\t\t name, entry.path);\n \t\t\tresult = options->walk(obj, OBJ_TREE, data, options);\n \t\t}\n \t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {\n \t\t\tobj = (struct object *)lookup_blob(the_repository, &entry.oid);\n \t\t\tif (name && obj)\n-\t\t\t\tput_object_name(options, obj, \"%s%s\", name,\n-\t\t\t\t\tentry.path);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%s%s\",\n+\t\t\t\t\t\t name, entry.path);\n \t\t\tresult = options->walk(obj, OBJ_BLOB, data, options);\n \t\t}\n \t\telse {\n \t\t\tresult = error(\"in tree %s: entry %s has bad mode %.6o\",\n-\t\t\t\t\tdescribe_object(options, &tree->object), entry.path, entry.mode);\n+\t\t\t\t fsck_describe_object(options, &tree->object),\n+\t\t\t\t entry.path, entry.mode);\n \t\t}\n \t\tif (result < 0)\n \t\t\treturn result;\n@@ -407,10 +424,10 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \tif (parse_commit(commit))\n \t\treturn -1;\n \n-\tname = get_object_name(options, &commit->object);\n+\tname = fsck_get_object_name(options, &commit->object);\n \tif (name)\n-\t\tput_object_name(options, &get_commit_tree(commit)->object,\n-\t\t\t\t\"%s:\", name);\n+\t\tfsck_put_object_name(options, &get_commit_tree(commit)->object,\n+\t\t\t\t \"%s:\", name);\n \n \tresult = options->walk((struct object *)get_commit_tree(commit),\n \t\t\t OBJ_TREE, data, options);\n@@ -441,13 +458,15 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \t\t\tstruct object *obj = &parents->item->object;\n \n \t\t\tif (counter++)\n-\t\t\t\tput_object_name(options, obj, \"%s^%d\",\n-\t\t\t\t\tname, counter);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%s^%d\",\n+\t\t\t\t\t\t name, counter);\n \t\t\telse if (generation > 0)\n-\t\t\t\tput_object_name(options, obj, \"%.*s~%d\",\n-\t\t\t\t\tname_prefix_len, name, generation + 1);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%.*s~%d\",\n+\t\t\t\t\t\t name_prefix_len, name,\n+\t\t\t\t\t\t generation + 1);\n \t\t\telse\n-\t\t\t\tput_object_name(options, obj, \"%s^\", name);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%s^\",\n+\t\t\t\t\t\t name);\n \t\t}\n \t\tresult = options->walk((struct object *)parents->item, OBJ_COMMIT, data, options);\n \t\tif (result < 0)\n@@ -461,12 +480,12 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \n static int fsck_walk_tag(struct tag *tag, void *data, struct fsck_options *options)\n {\n-\tchar *name = get_object_name(options, &tag->object);\n+\tconst char *name = fsck_get_object_name(options, &tag->object);\n \n \tif (parse_tag(tag))\n \t\treturn -1;\n \tif (name)\n-\t\tput_object_name(options, tag->tagged, \"%s\", name);\n+\t\tfsck_put_object_name(options, tag->tagged, \"%s\", name);\n \treturn options->walk(tag->tagged, OBJ_ANY, data, options);\n }\n \n@@ -488,7 +507,8 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)\n \tcase OBJ_TAG:\n \t\treturn fsck_walk_tag((struct tag *)obj, data, options);\n \tdefault:\n-\t\terror(\"Unknown object type for %s\", describe_object(options, obj));\n+\t\terror(\"Unknown object type for %s\",\n+\t\t fsck_describe_object(options, obj));\n \t\treturn -1;\n \t}\n }\n@@ -962,10 +982,10 @@ int fsck_error_function(struct fsck_options *o,\n \tstruct object *obj, int msg_type, const char *message)\n {\n \tif (msg_type == FSCK_WARN) {\n-\t\twarning(\"object %s: %s\", describe_object(o, obj), message);\n+\t\twarning(\"object %s: %s\", fsck_describe_object(o, obj), message);\n \t\treturn 0;\n \t}\n-\terror(\"object %s: %s\", describe_object(o, obj), message);\n+\terror(\"object %s: %s\", fsck_describe_object(o, obj), message);\n \treturn 1;\n }\n \ndiff --git a/fsck.h b/fsck.h\nindex e479461075..f6f0c40060 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -67,4 +67,28 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n */\n int fsck_finish(struct fsck_options *options);\n \n+/*\n+ * Subsystem for storing human-readable names for each object.\n+ *\n+ * If fsck_enable_object_names() has not been called, all other functions are\n+ * noops.\n+ *\n+ * Use put_object_name() to seed initial names (e.g. from refnames); the fsck\n+ * code will extend that while walking trees, etc.\n+ *\n+ * Use get_object_name() to get a single name (or NULL if none). Or the more\n+ * convenient describe_object(), which always produces an output string with\n+ * the oid combined with the name (if any). Note that the return value points\n+ * to a rotating array of static buffers, and may be invalidated by a\n+ * subsequent call.\n+ */\n+void fsck_enable_object_names(struct fsck_options *options);\n+const char *fsck_get_object_name(struct fsck_options *options,\n+\t\t\t\t struct object *obj);\n+__attribute__((format (printf,3,4)))\n+void fsck_put_object_name(struct fsck_options *options, struct object *obj,\n+\t\t\t const char *fmt, ...);\n+const char *fsck_describe_object(struct fsck_options *options,\n+\t\t\t\t struct object *obj);\n+\n #endif\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 50d28e6fdb..7c7ff7e961 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -616,7 +616,7 @@ test_expect_success 'fsck --name-objects' '\n \t\tremove_object $(git rev-parse julius:caesar.t) &&\n \t\ttest_must_fail git fsck --name-objects >out &&\n \t\ttree=$(git rev-parse --verify julius:) &&\n-\t\ttest_i18ngrep -E \"$tree \\((refs/heads/master|HEAD)@\\{[0-9]*\\}:\" out\n+\t\ttest_i18ngrep \"$tree (refs/tags/julius:\" out\n \t)\n '\n \n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384342","messageId":"20191018045638.GJ17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 10/23] fsck_describe_object(): build on our get_object_name() primitive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:56:38Z","receivedAt":"2019-10-18T04:56:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This isolates the implementation detail of using the decoration code to\nour put/get functions.\n\nSigned-off-by: Jeff King \n---\nArguably this could be squashed into the previous commit. By not doing\nso, it made describe_object() more of a pure code movement.\n\n fsck.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex ecd5957362..b0c4de67c9 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -351,10 +351,7 @@ const char *fsck_describe_object(struct fsck_options *options,\n \t};\n \tstatic int b = 0;\n \tstruct strbuf *buf;\n-\tchar *name = NULL;\n-\n-\tif (options->object_names)\n-\t\tname = lookup_decoration(options->object_names, obj);\n+\tconst char *name = fsck_get_object_name(options, obj);\n \n \tbuf = bufs + b;\n \tb = (b + 1) % ARRAY_SIZE(bufs);\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384344","messageId":"20191018045737.GK17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 11/23] fsck: use oids rather than objects for object_name API","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:57:37Z","receivedAt":"2019-10-18T04:57:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't actually care about having object structs; we only need to look\nup decorations by oid. Let's accept this more limited form, which will\ngive our callers more flexibility.\n\nNote that the decoration API we rely on uses object structs itself (even\nthough it only looks at their oids). We can solve this by switching to\na kh_oid_map (we could also use the hashmap oidmap, but it's more\nawkward for the simple case of just storing a void pointer).\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 12 +++++-----\n fsck.c | 61 ++++++++++++++++++++++++++++----------------------\n fsck.h | 9 ++++----\n 3 files changed, 45 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 237643cc1d..66fa727c14 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -52,7 +52,7 @@ static int name_objects;\n \n static const char *describe_object(struct object *obj)\n {\n-\treturn fsck_describe_object(&fsck_walk_options, obj);\n+\treturn fsck_describe_object(&fsck_walk_options, &obj->oid);\n }\n \n static const char *printable_type(struct object *obj)\n@@ -483,7 +483,7 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,\n \t\tobj = lookup_object(the_repository, oid);\n \t\tif (obj && (obj->flags & HAS_OBJ)) {\n \t\t\tif (timestamp)\n-\t\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\t\tfsck_put_object_name(&fsck_walk_options, oid,\n \t\t\t\t\t\t \"%s@{%\"PRItime\"}\",\n \t\t\t\t\t\t refname, timestamp);\n \t\t\tobj->flags |= USED;\n@@ -550,7 +550,7 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid,\n \tdefault_refs++;\n \tobj->flags |= USED;\n \tfsck_put_object_name(&fsck_walk_options,\n-\t\t\t obj, \"%s\", refname);\n+\t\t\t oid, \"%s\", refname);\n \tmark_object_reachable(obj);\n \n \treturn 0;\n@@ -724,7 +724,7 @@ static int fsck_cache_tree(struct cache_tree *it)\n \t\t\treturn 1;\n \t\t}\n \t\tobj->flags |= USED;\n-\t\tfsck_put_object_name(&fsck_walk_options, obj, \":\");\n+\t\tfsck_put_object_name(&fsck_walk_options, &it->oid, \":\");\n \t\tmark_object_reachable(obj);\n \t\tif (obj->type != OBJ_TREE)\n \t\t\terr |= objerror(obj, _(\"non-tree in cache-tree\"));\n@@ -869,7 +869,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \n \t\t\tobj->flags |= USED;\n-\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\tfsck_put_object_name(&fsck_walk_options, &oid,\n \t\t\t\t\t \"%s\", arg);\n \t\t\tmark_object_reachable(obj);\n \t\t\tcontinue;\n@@ -906,7 +906,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\tobj = &blob->object;\n \t\t\tobj->flags |= USED;\n-\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\tfsck_put_object_name(&fsck_walk_options, &obj->oid,\n \t\t\t\t\t \":%s\", active_cache[i]->name);\n \t\t\tmark_object_reachable(obj);\n \t\t}\ndiff --git a/fsck.c b/fsck.c\nindex b0c4de67c9..124c0184d4 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -315,48 +315,56 @@ static int report(struct fsck_options *options, struct object *object,\n void fsck_enable_object_names(struct fsck_options *options)\n {\n \tif (!options->object_names)\n-\t\toptions->object_names = xcalloc(1, sizeof(struct decoration));\n+\t\toptions->object_names = kh_init_oid_map();\n }\n \n-const char *fsck_get_object_name(struct fsck_options *options, struct object *obj)\n+const char *fsck_get_object_name(struct fsck_options *options,\n+\t\t\t\t const struct object_id *oid)\n {\n+\tkhiter_t pos;\n \tif (!options->object_names)\n \t\treturn NULL;\n-\treturn lookup_decoration(options->object_names, obj);\n+\tpos = kh_get_oid_map(options->object_names, *oid);\n+\tif (pos >= kh_end(options->object_names))\n+\t\treturn NULL;\n+\treturn kh_value(options->object_names, pos);\n }\n \n-void fsck_put_object_name(struct fsck_options *options, struct object *obj,\n+void fsck_put_object_name(struct fsck_options *options,\n+\t\t\t const struct object_id *oid,\n \t\t\t const char *fmt, ...)\n {\n \tva_list ap;\n \tstruct strbuf buf = STRBUF_INIT;\n-\tchar *existing;\n+\tkhiter_t pos;\n+\tint hashret;\n \n \tif (!options->object_names)\n \t\treturn;\n-\texisting = lookup_decoration(options->object_names, obj);\n-\tif (existing)\n+\n+\tpos = kh_put_oid_map(options->object_names, *oid, &hashret);\n+\tif (!hashret)\n \t\treturn;\n \tva_start(ap, fmt);\n \tstrbuf_vaddf(&buf, fmt, ap);\n-\tadd_decoration(options->object_names, obj, strbuf_detach(&buf, NULL));\n+\tkh_value(options->object_names, pos) = strbuf_detach(&buf, NULL);\n \tva_end(ap);\n }\n \n const char *fsck_describe_object(struct fsck_options *options,\n-\t\t\t\t struct object *obj)\n+\t\t\t\t const struct object_id *oid)\n {\n \tstatic struct strbuf bufs[] = {\n \t\tSTRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n \t};\n \tstatic int b = 0;\n \tstruct strbuf *buf;\n-\tconst char *name = fsck_get_object_name(options, obj);\n+\tconst char *name = fsck_get_object_name(options, oid);\n \n \tbuf = bufs + b;\n \tb = (b + 1) % ARRAY_SIZE(bufs);\n \tstrbuf_reset(buf);\n-\tstrbuf_addstr(buf, oid_to_hex(&obj->oid));\n+\tstrbuf_addstr(buf, oid_to_hex(oid));\n \tif (name)\n \t\tstrbuf_addf(buf, \" (%s)\", name);\n \n@@ -373,7 +381,7 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \tif (parse_tree(tree))\n \t\treturn -1;\n \n-\tname = fsck_get_object_name(options, &tree->object);\n+\tname = fsck_get_object_name(options, &tree->object.oid);\n \tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n \t\treturn -1;\n \twhile (tree_entry_gently(&desc, &entry)) {\n@@ -386,20 +394,20 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\tif (S_ISDIR(entry.mode)) {\n \t\t\tobj = (struct object *)lookup_tree(the_repository, &entry.oid);\n \t\t\tif (name && obj)\n-\t\t\t\tfsck_put_object_name(options, obj, \"%s%s/\",\n+\t\t\t\tfsck_put_object_name(options, &entry.oid, \"%s%s/\",\n \t\t\t\t\t\t name, entry.path);\n \t\t\tresult = options->walk(obj, OBJ_TREE, data, options);\n \t\t}\n \t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {\n \t\t\tobj = (struct object *)lookup_blob(the_repository, &entry.oid);\n \t\t\tif (name && obj)\n-\t\t\t\tfsck_put_object_name(options, obj, \"%s%s\",\n+\t\t\t\tfsck_put_object_name(options, &entry.oid, \"%s%s\",\n \t\t\t\t\t\t name, entry.path);\n \t\t\tresult = options->walk(obj, OBJ_BLOB, data, options);\n \t\t}\n \t\telse {\n \t\t\tresult = error(\"in tree %s: entry %s has bad mode %.6o\",\n-\t\t\t\t fsck_describe_object(options, &tree->object),\n+\t\t\t\t fsck_describe_object(options, &tree->object.oid),\n \t\t\t\t entry.path, entry.mode);\n \t\t}\n \t\tif (result < 0)\n@@ -421,9 +429,9 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \tif (parse_commit(commit))\n \t\treturn -1;\n \n-\tname = fsck_get_object_name(options, &commit->object);\n+\tname = fsck_get_object_name(options, &commit->object.oid);\n \tif (name)\n-\t\tfsck_put_object_name(options, &get_commit_tree(commit)->object,\n+\t\tfsck_put_object_name(options, get_commit_tree_oid(commit),\n \t\t\t\t \"%s:\", name);\n \n \tresult = options->walk((struct object *)get_commit_tree(commit),\n@@ -452,18 +460,17 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \n \twhile (parents) {\n \t\tif (name) {\n-\t\t\tstruct object *obj = &parents->item->object;\n+\t\t\tstruct object_id *oid = &parents->item->object.oid;\n \n \t\t\tif (counter++)\n-\t\t\t\tfsck_put_object_name(options, obj, \"%s^%d\",\n+\t\t\t\tfsck_put_object_name(options, oid, \"%s^%d\",\n \t\t\t\t\t\t name, counter);\n \t\t\telse if (generation > 0)\n-\t\t\t\tfsck_put_object_name(options, obj, \"%.*s~%d\",\n+\t\t\t\tfsck_put_object_name(options, oid, \"%.*s~%d\",\n \t\t\t\t\t\t name_prefix_len, name,\n \t\t\t\t\t\t generation + 1);\n \t\t\telse\n-\t\t\t\tfsck_put_object_name(options, obj, \"%s^\",\n-\t\t\t\t\t\t name);\n+\t\t\t\tfsck_put_object_name(options, oid, \"%s^\", name);\n \t\t}\n \t\tresult = options->walk((struct object *)parents->item, OBJ_COMMIT, data, options);\n \t\tif (result < 0)\n@@ -477,12 +484,12 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \n static int fsck_walk_tag(struct tag *tag, void *data, struct fsck_options *options)\n {\n-\tconst char *name = fsck_get_object_name(options, &tag->object);\n+\tconst char *name = fsck_get_object_name(options, &tag->object.oid);\n \n \tif (parse_tag(tag))\n \t\treturn -1;\n \tif (name)\n-\t\tfsck_put_object_name(options, tag->tagged, \"%s\", name);\n+\t\tfsck_put_object_name(options, &tag->tagged->oid, \"%s\", name);\n \treturn options->walk(tag->tagged, OBJ_ANY, data, options);\n }\n \n@@ -505,7 +512,7 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)\n \t\treturn fsck_walk_tag((struct tag *)obj, data, options);\n \tdefault:\n \t\terror(\"Unknown object type for %s\",\n-\t\t fsck_describe_object(options, obj));\n+\t\t fsck_describe_object(options, &obj->oid));\n \t\treturn -1;\n \t}\n }\n@@ -979,10 +986,10 @@ int fsck_error_function(struct fsck_options *o,\n \tstruct object *obj, int msg_type, const char *message)\n {\n \tif (msg_type == FSCK_WARN) {\n-\t\twarning(\"object %s: %s\", fsck_describe_object(o, obj), message);\n+\t\twarning(\"object %s: %s\", fsck_describe_object(o, &obj->oid), message);\n \t\treturn 0;\n \t}\n-\terror(\"object %s: %s\", fsck_describe_object(o, obj), message);\n+\terror(\"object %s: %s\", fsck_describe_object(o, &obj->oid), message);\n \treturn 1;\n }\n \ndiff --git a/fsck.h b/fsck.h\nindex f6f0c40060..4397cab20f 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -38,7 +38,7 @@ struct fsck_options {\n \tunsigned strict:1;\n \tint *msg_type;\n \tstruct oidset skiplist;\n-\tstruct decoration *object_names;\n+\tkh_oid_map_t *object_names;\n };\n \n #define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL, OIDSET_INIT }\n@@ -84,11 +84,12 @@ int fsck_finish(struct fsck_options *options);\n */\n void fsck_enable_object_names(struct fsck_options *options);\n const char *fsck_get_object_name(struct fsck_options *options,\n-\t\t\t\t struct object *obj);\n+\t\t\t\t const struct object_id *oid);\n __attribute__((format (printf,3,4)))\n-void fsck_put_object_name(struct fsck_options *options, struct object *obj,\n+void fsck_put_object_name(struct fsck_options *options,\n+\t\t\t const struct object_id *oid,\n \t\t\t const char *fmt, ...);\n const char *fsck_describe_object(struct fsck_options *options,\n-\t\t\t\t struct object *obj);\n+\t\t\t\t const struct object_id *oid);\n \n #endif\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384345","messageId":"20191018045118.GG17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 07/23] fsck: stop checking tag->tagged","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:51:19Z","receivedAt":"2019-10-18T04:58:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Way back in 92d4c85d24 (fsck-cache: fix SIGSEGV on bad tag object,\n2005-05-03), we added an fsck check that the \"tagged\" field of a tag\nstruct isn't NULL. But that was mainly protecting the printing code for\n\"--tags\", and that code wasn't moved along with the check as part of\nba002f3b28 (builtin-fsck: move common object checking code to fsck.c,\n2008-02-25).\n\nIt could also serve to detect type mismatch problems (where a tag points\nto object X as a commit, but really X is a blob), but it couldn't do so\nreliably (we'd call lookup_commit(X), but it will only notice the\nproblem if we happen to have previously called lookup_blob(X) in the\nsame process). And as of a commit earlier in this series, we'd consider\nthat a parse error and complain about the object even before getting to\nthis point anyway.\n\nSo let's drop this \"tag->tagged\" check. It's not helping anything, and\ngetting rid of it makes the function conceptually cleaner, as it really\nis just checking the buffer we feed it. In fact, we can get rid of our\none-line wrapper and just unify fsck_tag() and fsck_tag_buffer().\n\nSigned-off-by: Jeff King \n---\n fsck.c | 15 ++-------------\n 1 file changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex a0f8ae7650..79ce3a97c8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -798,8 +798,8 @@ static int fsck_commit(struct commit *commit, const char *data,\n \treturn ret;\n }\n \n-static int fsck_tag_buffer(struct tag *tag, const char *data,\n-\tunsigned long size, struct fsck_options *options)\n+static int fsck_tag(struct tag *tag, const char *data,\n+\t\t unsigned long size, struct fsck_options *options)\n {\n \tstruct object_id oid;\n \tint ret = 0;\n@@ -893,17 +893,6 @@ static int fsck_tag_buffer(struct tag *tag, const char *data,\n \treturn ret;\n }\n \n-static int fsck_tag(struct tag *tag, const char *data,\n-\tunsigned long size, struct fsck_options *options)\n-{\n-\tstruct object *tagged = tag->tagged;\n-\n-\tif (!tagged)\n-\t\treturn report(options, &tag->object, FSCK_MSG_BAD_TAG_OBJECT, \"could not load tagged object\");\n-\n-\treturn fsck_tag_buffer(tag, data, size, options);\n-}\n-\n struct fsck_gitmodules_data {\n \tstruct object *obj;\n \tstruct fsck_options *options;\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384346","messageId":"20191018045807.GL17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 12/23] fsck: don't require object structs for display functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:58:07Z","receivedAt":"2019-10-18T04:58:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our printable_type() and describe_object() functions take whole object\nstructs, but they really only care about the oid and type. Let's take\nthose individually in order to give our callers more flexibility.\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 69 +++++++++++++++++++++++++++-----------------------\n 1 file changed, 37 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 66fa727c14..59c77c1baa 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -50,23 +50,20 @@ static int name_objects;\n #define ERROR_REFS 010\n #define ERROR_COMMIT_GRAPH 020\n \n-static const char *describe_object(struct object *obj)\n+static const char *describe_object(const struct object_id *oid)\n {\n-\treturn fsck_describe_object(&fsck_walk_options, &obj->oid);\n+\treturn fsck_describe_object(&fsck_walk_options, oid);\n }\n \n-static const char *printable_type(struct object *obj)\n+static const char *printable_type(const struct object_id *oid,\n+\t\t\t\t enum object_type type)\n {\n \tconst char *ret;\n \n-\tif (obj->type == OBJ_NONE) {\n-\t\tenum object_type type = oid_object_info(the_repository,\n-\t\t\t\t\t\t\t&obj->oid, NULL);\n-\t\tif (type > 0)\n-\t\t\tobject_as_type(the_repository, obj, type, 0);\n-\t}\n+\tif (type == OBJ_NONE)\n+\t\ttype = oid_object_info(the_repository, oid, NULL);\n \n-\tret = type_name(obj->type);\n+\tret = type_name(type);\n \tif (!ret)\n \t\tret = _(\"unknown\");\n \n@@ -101,7 +98,8 @@ static int objerror(struct object *obj, const char *err)\n \terrors_found |= ERROR_OBJECT;\n \t/* TRANSLATORS: e.g. error in tree 01bfda: */\n \tfprintf_ln(stderr, _(\"error in %s %s: %s\"),\n-\t\t printable_type(obj), describe_object(obj), err);\n+\t\t printable_type(&obj->oid, obj->type),\n+\t\t describe_object(&obj->oid), err);\n \treturn -1;\n }\n \n@@ -112,12 +110,14 @@ static int fsck_error_func(struct fsck_options *o,\n \tcase FSCK_WARN:\n \t\t/* TRANSLATORS: e.g. warning in tree 01bfda: */\n \t\tfprintf_ln(stderr, _(\"warning in %s %s: %s\"),\n-\t\t\t printable_type(obj), describe_object(obj), message);\n+\t\t\t printable_type(&obj->oid, obj->type),\n+\t\t\t describe_object(&obj->oid), message);\n \t\treturn 0;\n \tcase FSCK_ERROR:\n \t\t/* TRANSLATORS: e.g. error in tree 01bfda: */\n \t\tfprintf_ln(stderr, _(\"error in %s %s: %s\"),\n-\t\t\t printable_type(obj), describe_object(obj), message);\n+\t\t\t printable_type(&obj->oid, obj->type),\n+\t\t\t describe_object(&obj->oid), message);\n \t\treturn 1;\n \tdefault:\n \t\tBUG(\"%d (FSCK_IGNORE?) should never trigger this callback\", type);\n@@ -138,7 +138,8 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt\n \tif (!obj) {\n \t\t/* ... these references to parent->fld are safe here */\n \t\tprintf_ln(_(\"broken link from %7s %s\"),\n-\t\t\t printable_type(parent), describe_object(parent));\n+\t\t\t printable_type(&parent->oid, parent->type),\n+\t\t\t describe_object(&parent->oid));\n \t\tprintf_ln(_(\"broken link from %7s %s\"),\n \t\t\t (type == OBJ_ANY ? _(\"unknown\") : type_name(type)),\n \t\t\t _(\"unknown\"));\n@@ -166,10 +167,10 @@ static int mark_object(struct object *obj, int type, void *data, struct fsck_opt\n \t\tif (parent && !has_object_file(&obj->oid)) {\n \t\t\tprintf_ln(_(\"broken link from %7s %s\\n\"\n \t\t\t\t \" to %7s %s\"),\n-\t\t\t\t printable_type(parent),\n-\t\t\t\t describe_object(parent),\n-\t\t\t\t printable_type(obj),\n-\t\t\t\t describe_object(obj));\n+\t\t\t\t printable_type(&parent->oid, parent->type),\n+\t\t\t\t describe_object(&parent->oid),\n+\t\t\t\t printable_type(&obj->oid, obj->type),\n+\t\t\t\t describe_object(&obj->oid));\n \t\t\terrors_found |= ERROR_REACHABLE;\n \t\t}\n \t\treturn 1;\n@@ -275,8 +276,9 @@ static void check_reachable_object(struct object *obj)\n \t\t\treturn;\n \t\tif (has_object_pack(&obj->oid))\n \t\t\treturn; /* it is in pack - forget about it */\n-\t\tprintf_ln(_(\"missing %s %s\"), printable_type(obj),\n-\t\t\t describe_object(obj));\n+\t\tprintf_ln(_(\"missing %s %s\"),\n+\t\t\t printable_type(&obj->oid, obj->type),\n+\t\t\t describe_object(&obj->oid));\n \t\terrors_found |= ERROR_REACHABLE;\n \t\treturn;\n \t}\n@@ -301,8 +303,9 @@ static void check_unreachable_object(struct object *obj)\n \t * since this is something that is prunable.\n \t */\n \tif (show_unreachable) {\n-\t\tprintf_ln(_(\"unreachable %s %s\"), printable_type(obj),\n-\t\t\t describe_object(obj));\n+\t\tprintf_ln(_(\"unreachable %s %s\"),\n+\t\t\t printable_type(&obj->oid, obj->type),\n+\t\t\t describe_object(&obj->oid));\n \t\treturn;\n \t}\n \n@@ -320,12 +323,13 @@ static void check_unreachable_object(struct object *obj)\n \t */\n \tif (!(obj->flags & USED)) {\n \t\tif (show_dangling)\n-\t\t\tprintf_ln(_(\"dangling %s %s\"), printable_type(obj),\n-\t\t\t\t describe_object(obj));\n+\t\t\tprintf_ln(_(\"dangling %s %s\"),\n+\t\t\t\t printable_type(&obj->oid, obj->type),\n+\t\t\t\t describe_object(&obj->oid));\n \t\tif (write_lost_and_found) {\n \t\t\tchar *filename = git_pathdup(\"lost-found/%s/%s\",\n \t\t\t\tobj->type == OBJ_COMMIT ? \"commit\" : \"other\",\n-\t\t\t\tdescribe_object(obj));\n+\t\t\t\tdescribe_object(&obj->oid));\n \t\t\tFILE *f;\n \n \t\t\tif (safe_create_leading_directories_const(filename)) {\n@@ -338,7 +342,7 @@ static void check_unreachable_object(struct object *obj)\n \t\t\t\tif (stream_blob_to_fd(fileno(f), &obj->oid, NULL, 1))\n \t\t\t\t\tdie_errno(_(\"could not write '%s'\"), filename);\n \t\t\t} else\n-\t\t\t\tfprintf(f, \"%s\\n\", describe_object(obj));\n+\t\t\t\tfprintf(f, \"%s\\n\", describe_object(&obj->oid));\n \t\t\tif (fclose(f))\n \t\t\t\tdie_errno(_(\"could not finish '%s'\"),\n \t\t\t\t\t filename);\n@@ -357,7 +361,7 @@ static void check_unreachable_object(struct object *obj)\n static void check_object(struct object *obj)\n {\n \tif (verbose)\n-\t\tfprintf_ln(stderr, _(\"Checking %s\"), describe_object(obj));\n+\t\tfprintf_ln(stderr, _(\"Checking %s\"), describe_object(&obj->oid));\n \n \tif (obj->flags & REACHABLE)\n \t\tcheck_reachable_object(obj);\n@@ -415,7 +419,8 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size)\n \n \tif (verbose)\n \t\tfprintf_ln(stderr, _(\"Checking %s %s\"),\n-\t\t\t printable_type(obj), describe_object(obj));\n+\t\t\t printable_type(&obj->oid, obj->type),\n+\t\t\t describe_object(&obj->oid));\n \n \tif (fsck_walk(obj, NULL, &fsck_obj_options))\n \t\tobjerror(obj, _(\"broken links\"));\n@@ -428,7 +433,7 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size)\n \n \t\tif (!commit->parents && show_root)\n \t\t\tprintf_ln(_(\"root %s\"),\n-\t\t\t\t describe_object(&commit->object));\n+\t\t\t\t describe_object(&commit->object.oid));\n \t}\n \n \tif (obj->type == OBJ_TAG) {\n@@ -436,10 +441,10 @@ static int fsck_obj(struct object *obj, void *buffer, unsigned long size)\n \n \t\tif (show_tags && tag->tagged) {\n \t\t\tprintf_ln(_(\"tagged %s %s (%s) in %s\"),\n-\t\t\t\t printable_type(tag->tagged),\n-\t\t\t\t describe_object(tag->tagged),\n+\t\t\t\t printable_type(&tag->tagged->oid, tag->tagged->type),\n+\t\t\t\t describe_object(&tag->tagged->oid),\n \t\t\t\t tag->tag,\n-\t\t\t\t describe_object(&tag->object));\n+\t\t\t\t describe_object(&tag->object.oid));\n \t\t}\n \t}\n \n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384347","messageId":"20191018045840.GM17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 13/23] fsck: only provide oid/type in fsck_error callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:58:40Z","receivedAt":"2019-10-18T04:58:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"None of the callbacks actually care about having a \"struct object\";\nthey're happy with just the oid and type information. So let's give\nourselves more flexibility to avoid having a \"struct object\" by just\npassing the broken-down fields.\n\nNote that the callback already takes a \"type\" field for the fsck message\ntype. We'll rename that to \"msg_type\" (and use \"object_type\" for the\nobject type) to make the distinction explicit.\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 17 ++++++++++-------\n fsck.c | 11 +++++++----\n fsck.h | 6 ++++--\n 3 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 59c77c1baa..8d13794b14 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -104,23 +104,26 @@ static int objerror(struct object *obj, const char *err)\n }\n \n static int fsck_error_func(struct fsck_options *o,\n-\tstruct object *obj, int type, const char *message)\n+\t\t\t const struct object_id *oid,\n+\t\t\t enum object_type object_type,\n+\t\t\t int msg_type, const char *message)\n {\n-\tswitch (type) {\n+\tswitch (msg_type) {\n \tcase FSCK_WARN:\n \t\t/* TRANSLATORS: e.g. warning in tree 01bfda: */\n \t\tfprintf_ln(stderr, _(\"warning in %s %s: %s\"),\n-\t\t\t printable_type(&obj->oid, obj->type),\n-\t\t\t describe_object(&obj->oid), message);\n+\t\t\t printable_type(oid, object_type),\n+\t\t\t describe_object(oid), message);\n \t\treturn 0;\n \tcase FSCK_ERROR:\n \t\t/* TRANSLATORS: e.g. error in tree 01bfda: */\n \t\tfprintf_ln(stderr, _(\"error in %s %s: %s\"),\n-\t\t\t printable_type(&obj->oid, obj->type),\n-\t\t\t describe_object(&obj->oid), message);\n+\t\t\t printable_type(oid, object_type),\n+\t\t\t describe_object(oid), message);\n \t\treturn 1;\n \tdefault:\n-\t\tBUG(\"%d (FSCK_IGNORE?) should never trigger this callback\", type);\n+\t\tBUG(\"%d (FSCK_IGNORE?) should never trigger this callback\",\n+\t\t msg_type);\n \t}\n }\n \ndiff --git a/fsck.c b/fsck.c\nindex 124c0184d4..c036ba09ab 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -305,7 +305,8 @@ static int report(struct fsck_options *options, struct object *object,\n \n \tva_start(ap, fmt);\n \tstrbuf_vaddf(&sb, fmt, ap);\n-\tresult = options->error_func(options, object, msg_type, sb.buf);\n+\tresult = options->error_func(options, &object->oid, object->type,\n+\t\t\t\t msg_type, sb.buf);\n \tstrbuf_release(&sb);\n \tva_end(ap);\n \n@@ -983,13 +984,15 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n }\n \n int fsck_error_function(struct fsck_options *o,\n-\tstruct object *obj, int msg_type, const char *message)\n+\t\t\tconst struct object_id *oid,\n+\t\t\tenum object_type object_type,\n+\t\t\tint msg_type, const char *message)\n {\n \tif (msg_type == FSCK_WARN) {\n-\t\twarning(\"object %s: %s\", fsck_describe_object(o, &obj->oid), message);\n+\t\twarning(\"object %s: %s\", fsck_describe_object(o, oid), message);\n \t\treturn 0;\n \t}\n-\terror(\"object %s: %s\", fsck_describe_object(o, &obj->oid), message);\n+\terror(\"object %s: %s\", fsck_describe_object(o, oid), message);\n \treturn 1;\n }\n \ndiff --git a/fsck.h b/fsck.h\nindex 4397cab20f..f81aedf12f 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -27,10 +27,12 @@ typedef int (*fsck_walk_func)(struct object *obj, int type, void *data, struct f\n \n /* callback for fsck_object, type is FSCK_ERROR or FSCK_WARN */\n typedef int (*fsck_error)(struct fsck_options *o,\n-\tstruct object *obj, int type, const char *message);\n+\t\t\t const struct object_id *oid, enum object_type object_type,\n+\t\t\t int msg_type, const char *message);\n \n int fsck_error_function(struct fsck_options *o,\n-\tstruct object *obj, int type, const char *message);\n+\t\t\tconst struct object_id *oid, enum object_type object_type,\n+\t\t\tint msg_type, const char *message);\n \n struct fsck_options {\n \tfsck_walk_func walk;\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384348","messageId":"20191018045851.GN17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 14/23] fsck: only require an oid for skiplist functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:58:51Z","receivedAt":"2019-10-18T04:58:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The skiplist is inherently an oidset, so we don't need a full object\nstruct. Let's take just the oid to give our callers more flexibility.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex c036ba09ab..2309c40a11 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -277,9 +277,10 @@ static void append_msg_id(struct strbuf *sb, const char *msg_id)\n \tstrbuf_addstr(sb, \": \");\n }\n \n-static int object_on_skiplist(struct fsck_options *opts, struct object *obj)\n+static int object_on_skiplist(struct fsck_options *opts,\n+\t\t\t const struct object_id *oid)\n {\n-\treturn opts && obj && oidset_contains(&opts->skiplist, &obj->oid);\n+\treturn opts && oid && oidset_contains(&opts->skiplist, oid);\n }\n \n __attribute__((format (printf, 4, 5)))\n@@ -293,7 +294,7 @@ static int report(struct fsck_options *options, struct object *object,\n \tif (msg_type == FSCK_IGNORE)\n \t\treturn 0;\n \n-\tif (object_on_skiplist(options, object))\n+\tif (object_on_skiplist(options, &object->oid))\n \t\treturn 0;\n \n \tif (msg_type == FSCK_FATAL)\n@@ -935,7 +936,7 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \t\treturn 0;\n \toidset_insert(&gitmodules_done, &blob->object.oid);\n \n-\tif (object_on_skiplist(options, &blob->object))\n+\tif (object_on_skiplist(options, &blob->object.oid))\n \t\treturn 0;\n \n \tif (!buf) {\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384349","messageId":"20191018045915.GO17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 15/23] fsck: don't require an object struct for report()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:59:15Z","receivedAt":"2019-10-18T04:59:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The report() function really only cares about the oid and type of the\nobject, not the full object struct. Let's convert it to take those two\nitems separately, which gives our callers more flexibility.\n\nThis makes some already-long lines even longer. I've mostly left them,\nas our eventual goal is to shrink these down as we continue refactoring\n(e.g., \"&item->object\" becomes \"&item->object.oid, item->object.type\",\nbut will eventually shrink down to \"oid, type\").\n\nSigned-off-by: Jeff King \n---\n fsck.c | 128 +++++++++++++++++++++++++++++++--------------------------\n 1 file changed, 69 insertions(+), 59 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 2309c40a11..465247be71 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -283,9 +283,10 @@ static int object_on_skiplist(struct fsck_options *opts,\n \treturn opts && oid && oidset_contains(&opts->skiplist, oid);\n }\n \n-__attribute__((format (printf, 4, 5)))\n-static int report(struct fsck_options *options, struct object *object,\n-\tenum fsck_msg_id id, const char *fmt, ...)\n+__attribute__((format (printf, 5, 6)))\n+static int report(struct fsck_options *options,\n+\t\t const struct object_id *oid, enum object_type object_type,\n+\t\t enum fsck_msg_id id, const char *fmt, ...)\n {\n \tva_list ap;\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -294,7 +295,7 @@ static int report(struct fsck_options *options, struct object *object,\n \tif (msg_type == FSCK_IGNORE)\n \t\treturn 0;\n \n-\tif (object_on_skiplist(options, &object->oid))\n+\tif (object_on_skiplist(options, oid))\n \t\treturn 0;\n \n \tif (msg_type == FSCK_FATAL)\n@@ -306,7 +307,7 @@ static int report(struct fsck_options *options, struct object *object,\n \n \tva_start(ap, fmt);\n \tstrbuf_vaddf(&sb, fmt, ap);\n-\tresult = options->error_func(options, &object->oid, object->type,\n+\tresult = options->error_func(options, oid, object_type,\n \t\t\t\t msg_type, sb.buf);\n \tstrbuf_release(&sb);\n \tva_end(ap);\n@@ -585,7 +586,7 @@ static int fsck_tree(struct tree *item,\n \tconst char *o_name;\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -611,13 +612,14 @@ static int fsck_tree(struct tree *item,\n \t\t\tif (!S_ISLNK(mode))\n \t\t\t\toidset_insert(&gitmodules_found, oid);\n \t\t\telse\n-\t\t\t\tretval += report(options, &item->object,\n+\t\t\t\tretval += report(options,\n+\t\t\t\t\t\t &item->object.oid, item->object.type,\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, &item->object, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -662,25 +664,25 @@ static int fsck_tree(struct tree *item,\n \t}\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, &item->object, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, &item->object, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, &item->object, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, &item->object, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_HAS_DOT, \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, &item->object, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, &item->object, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, &item->object, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, &item->object, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, &item->object, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, &item->object, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n \treturn retval;\n }\n \n@@ -693,7 +695,7 @@ static int verify_headers(const void *data, unsigned long size,\n \tfor (i = 0; i < size; i++) {\n \t\tswitch (buffer[i]) {\n \t\tcase '\\0':\n-\t\t\treturn report(options, obj,\n+\t\t\treturn report(options, &obj->oid, obj->type,\n \t\t\t\tFSCK_MSG_NUL_IN_HEADER,\n \t\t\t\t\"unterminated header: NUL at offset %ld\", i);\n \t\tcase '\\n':\n@@ -711,7 +713,7 @@ static int verify_headers(const void *data, unsigned long size,\n \tif (size && buffer[size - 1] == '\\n')\n \t\treturn 0;\n \n-\treturn report(options, obj,\n+\treturn report(options, &obj->oid, obj->type,\n \t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n@@ -725,28 +727,28 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option\n \t\t(*ident)++;\n \n \tif (*p == '<')\n-\t\treturn report(options, obj, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n \tp += strcspn(p, \"<>\\n\");\n \tif (*p == '>')\n-\t\treturn report(options, obj, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n \tif (*p != '<')\n-\t\treturn report(options, obj, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n \tif (p[-1] != ' ')\n-\t\treturn report(options, obj, FSCK_MSG_MISSING_SPACE_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_MISSING_SPACE_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n \tp++;\n \tp += strcspn(p, \"<>\\n\");\n \tif (*p != '>')\n-\t\treturn report(options, obj, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n \tp++;\n \tif (*p != ' ')\n-\t\treturn report(options, obj, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n \tp++;\n \tif (*p == '0' && p[1] != ' ')\n-\t\treturn report(options, obj, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n \tif (date_overflows(parse_timestamp(p, &end, 10)))\n-\t\treturn report(options, obj, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n \tif ((end == p || *end != ' '))\n-\t\treturn report(options, obj, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n \tp = end + 1;\n \tif ((*p != '+' && *p != '-') ||\n \t !isdigit(p[1]) ||\n@@ -754,7 +756,7 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option\n \t !isdigit(p[3]) ||\n \t !isdigit(p[4]) ||\n \t (p[5] != '\\n'))\n-\t\treturn report(options, obj, FSCK_MSG_BAD_TIMEZONE, \"invalid author/committer line - bad time zone\");\n+\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_TIMEZONE, \"invalid author/committer line - bad time zone\");\n \tp += 6;\n \treturn 0;\n }\n@@ -772,16 +774,16 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \t\treturn -1;\n \n \tif (!skip_prefix(buffer, \"tree \", &buffer))\n-\t\treturn report(options, &commit->object, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n+\t\treturn report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n \tif (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n') {\n-\t\terr = report(options, &commit->object, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n+\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n \t\tif (err)\n \t\t\treturn err;\n \t}\n \tbuffer = p + 1;\n \twhile (skip_prefix(buffer, \"parent \", &buffer)) {\n \t\tif (parse_oid_hex(buffer, &oid, &p) || *p != '\\n') {\n-\t\t\terr = report(options, &commit->object, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n+\t\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n \t\t\tif (err)\n \t\t\t\treturn err;\n \t\t}\n@@ -795,18 +797,18 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \t\t\treturn err;\n \t}\n \tif (author_count < 1)\n-\t\terr = report(options, &commit->object, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n+\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n \telse if (author_count > 1)\n-\t\terr = report(options, &commit->object, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n+\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n \tif (err)\n \t\treturn err;\n \tif (!skip_prefix(buffer, \"committer \", &buffer))\n-\t\treturn report(options, &commit->object, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n+\t\treturn report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n \terr = fsck_ident(&buffer, &commit->object, options);\n \tif (err)\n \t\treturn err;\n \tif (memchr(buffer_begin, '\\0', size)) {\n-\t\terr = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,\n+\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_NUL_IN_COMMIT,\n \t\t\t \"NUL byte in the commit object body\");\n \t\tif (err)\n \t\t\treturn err;\n@@ -828,45 +830,46 @@ static int fsck_tag(struct tag *tag, const char *buffer,\n \t\tgoto done;\n \n \tif (!skip_prefix(buffer, \"object \", &buffer)) {\n-\t\tret = report(options, &tag->object, FSCK_MSG_MISSING_OBJECT, \"invalid format - expected 'object' line\");\n+\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_OBJECT, \"invalid format - expected 'object' line\");\n \t\tgoto done;\n \t}\n \tif (parse_oid_hex(buffer, &oid, &p) || *p != '\\n') {\n-\t\tret = report(options, &tag->object, FSCK_MSG_BAD_OBJECT_SHA1, \"invalid 'object' line format - bad sha1\");\n+\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_BAD_OBJECT_SHA1, \"invalid 'object' line format - bad sha1\");\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n \tbuffer = p + 1;\n \n \tif (!skip_prefix(buffer, \"type \", &buffer)) {\n-\t\tret = report(options, &tag->object, FSCK_MSG_MISSING_TYPE_ENTRY, \"invalid format - expected 'type' line\");\n+\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TYPE_ENTRY, \"invalid format - expected 'type' line\");\n \t\tgoto done;\n \t}\n \teol = strchr(buffer, '\\n');\n \tif (!eol) {\n-\t\tret = report(options, &tag->object, FSCK_MSG_MISSING_TYPE, \"invalid format - unexpected end after 'type' line\");\n+\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TYPE, \"invalid format - unexpected end after 'type' line\");\n \t\tgoto done;\n \t}\n \tif (type_from_string_gently(buffer, eol - buffer, 1) < 0)\n-\t\tret = report(options, &tag->object, FSCK_MSG_BAD_TYPE, \"invalid 'type' value\");\n+\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_BAD_TYPE, \"invalid 'type' value\");\n \tif (ret)\n \t\tgoto done;\n \tbuffer = eol + 1;\n \n \tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n-\t\tret = report(options, &tag->object, FSCK_MSG_MISSING_TAG_ENTRY, \"invalid format - expected 'tag' line\");\n+\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TAG_ENTRY, \"invalid format - expected 'tag' line\");\n \t\tgoto done;\n \t}\n \teol = strchr(buffer, '\\n');\n \tif (!eol) {\n-\t\tret = report(options, &tag->object, FSCK_MSG_MISSING_TAG, \"invalid format - unexpected end after 'type' line\");\n+\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TAG, \"invalid format - unexpected end after 'type' line\");\n \t\tgoto done;\n \t}\n \tstrbuf_addf(&sb, \"refs/tags/%.*s\", (int)(eol - buffer), buffer);\n \tif (check_refname_format(sb.buf, 0)) {\n-\t\tret = report(options, &tag->object, FSCK_MSG_BAD_TAG_NAME,\n-\t\t\t \"invalid 'tag' name: %.*s\",\n-\t\t\t (int)(eol - buffer), buffer);\n+\t\tret = report(options, &tag->object.oid, tag->object.type,\n+\t\t\t FSCK_MSG_BAD_TAG_NAME,\n+\t\t\t \"invalid 'tag' name: %.*s\",\n+\t\t\t (int)(eol - buffer), buffer);\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n@@ -874,7 +877,7 @@ static int fsck_tag(struct tag *tag, const char *buffer,\n \n \tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n \t\t/* early tags do not contain 'tagger' lines; warn only */\n-\t\tret = report(options, &tag->object, FSCK_MSG_MISSING_TAGGER_ENTRY, \"invalid format - expected 'tagger' line\");\n+\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TAGGER_ENTRY, \"invalid format - expected 'tagger' line\");\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n@@ -905,19 +908,22 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)\n \n \tname = xmemdupz(subsection, subsection_len);\n \tif (check_submodule_name(name) < 0)\n-\t\tdata->ret |= report(data->options, data->obj,\n+\t\tdata->ret |= report(data->options,\n+\t\t\t\t &data->obj->oid, data->obj->type,\n \t\t\t\t FSCK_MSG_GITMODULES_NAME,\n \t\t\t\t \"disallowed submodule name: %s\",\n \t\t\t\t name);\n \tif (!strcmp(key, \"url\") && value &&\n \t looks_like_command_line_option(value))\n-\t\tdata->ret |= report(data->options, data->obj,\n+\t\tdata->ret |= report(data->options,\n+\t\t\t\t &data->obj->oid, data->obj->type,\n \t\t\t\t FSCK_MSG_GITMODULES_URL,\n \t\t\t\t \"disallowed submodule url: %s\",\n \t\t\t\t value);\n \tif (!strcmp(key, \"path\") && value &&\n \t looks_like_command_line_option(value))\n-\t\tdata->ret |= report(data->options, data->obj,\n+\t\tdata->ret |= report(data->options,\n+\t\t\t\t &data->obj->oid, data->obj->type,\n \t\t\t\t FSCK_MSG_GITMODULES_PATH,\n \t\t\t\t \"disallowed submodule path: %s\",\n \t\t\t\t value);\n@@ -945,7 +951,7 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \t\t * blob too gigantic to load into memory. Let's just consider\n \t\t * that an error.\n \t\t */\n-\t\treturn report(options, &blob->object,\n+\t\treturn report(options, &blob->object.oid, blob->object.type,\n \t\t\t FSCK_MSG_GITMODULES_LARGE,\n \t\t\t \".gitmodules too large to parse\");\n \t}\n@@ -956,7 +962,7 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \tconfig_opts.error_action = CONFIG_ERROR_SILENT;\n \tif (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n \t\t\t\t\".gitmodules\", buf, size, &data, &config_opts))\n-\t\tdata.ret |= report(options, &blob->object,\n+\t\tdata.ret |= report(options, &blob->object.oid, blob->object.type,\n \t\t\t\t FSCK_MSG_GITMODULES_PARSE,\n \t\t\t\t \"could not parse gitmodules blob\");\n \n@@ -967,7 +973,7 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \tstruct fsck_options *options)\n {\n \tif (!obj)\n-\t\treturn report(options, obj, FSCK_MSG_BAD_OBJECT_SHA1, \"no valid object to fsck\");\n+\t\treturn report(options, NULL, OBJ_NONE, FSCK_MSG_BAD_OBJECT_SHA1, \"no valid object to fsck\");\n \n \tif (obj->type == OBJ_BLOB)\n \t\treturn fsck_blob((struct blob *)obj, data, size, options);\n@@ -980,8 +986,10 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \t\treturn fsck_tag((struct tag *) obj, (const char *) data,\n \t\t\tsize, options);\n \n-\treturn report(options, obj, FSCK_MSG_UNKNOWN_TYPE, \"unknown type '%d' (internal fsck error)\",\n-\t\t\t obj->type);\n+\treturn report(options, &obj->oid, obj->type,\n+\t\t FSCK_MSG_UNKNOWN_TYPE,\n+\t\t \"unknown type '%d' (internal fsck error)\",\n+\t\t obj->type);\n }\n \n int fsck_error_function(struct fsck_options *o,\n@@ -1016,7 +1024,7 @@ int fsck_finish(struct fsck_options *options)\n \t\tblob = lookup_blob(the_repository, oid);\n \t\tif (!blob) {\n \t\t\tstruct object *obj = lookup_unknown_object(oid);\n-\t\t\tret |= report(options, obj,\n+\t\t\tret |= report(options, &obj->oid, obj->type,\n \t\t\t\t FSCK_MSG_GITMODULES_BLOB,\n \t\t\t\t \"non-blob found at .gitmodules\");\n \t\t\tcontinue;\n@@ -1026,7 +1034,8 @@ int fsck_finish(struct fsck_options *options)\n \t\tif (!buf) {\n \t\t\tif (is_promisor_object(&blob->object.oid))\n \t\t\t\tcontinue;\n-\t\t\tret |= report(options, &blob->object,\n+\t\t\tret |= report(options,\n+\t\t\t\t &blob->object.oid, blob->object.type,\n \t\t\t\t FSCK_MSG_GITMODULES_MISSING,\n \t\t\t\t \"unable to read .gitmodules blob\");\n \t\t\tcontinue;\n@@ -1035,7 +1044,8 @@ int fsck_finish(struct fsck_options *options)\n \t\tif (type == OBJ_BLOB)\n \t\t\tret |= fsck_blob(blob, buf, size, options);\n \t\telse\n-\t\t\tret |= report(options, &blob->object,\n+\t\t\tret |= report(options,\n+\t\t\t\t &blob->object.oid, blob->object.type,\n \t\t\t\t FSCK_MSG_GITMODULES_BLOB,\n \t\t\t\t \"non-blob found at .gitmodules\");\n \t\tfree(buf);\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384350","messageId":"20191018045928.GP17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 16/23] fsck: accept an oid instead of a \"struct blob\" for fsck_blob()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:59:29Z","receivedAt":"2019-10-18T04:59:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't actually need any information from the object struct except its\noid (and the type, of course, but that's implicitly OBJ_BLOB). This\ngives our callers more flexibility to drop the object structs, too.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 26 +++++++++++++-------------\n 1 file changed, 13 insertions(+), 13 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 465247be71..6e9640a1a6 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -890,7 +890,7 @@ static int fsck_tag(struct tag *tag, const char *buffer,\n }\n \n struct fsck_gitmodules_data {\n-\tstruct object *obj;\n+\tconst struct object_id *oid;\n \tstruct fsck_options *options;\n \tint ret;\n };\n@@ -909,21 +909,21 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)\n \tname = xmemdupz(subsection, subsection_len);\n \tif (check_submodule_name(name) < 0)\n \t\tdata->ret |= report(data->options,\n-\t\t\t\t &data->obj->oid, data->obj->type,\n+\t\t\t\t data->oid, OBJ_BLOB,\n \t\t\t\t FSCK_MSG_GITMODULES_NAME,\n \t\t\t\t \"disallowed submodule name: %s\",\n \t\t\t\t name);\n \tif (!strcmp(key, \"url\") && value &&\n \t looks_like_command_line_option(value))\n \t\tdata->ret |= report(data->options,\n-\t\t\t\t &data->obj->oid, data->obj->type,\n+\t\t\t\t data->oid, OBJ_BLOB,\n \t\t\t\t FSCK_MSG_GITMODULES_URL,\n \t\t\t\t \"disallowed submodule url: %s\",\n \t\t\t\t value);\n \tif (!strcmp(key, \"path\") && value &&\n \t looks_like_command_line_option(value))\n \t\tdata->ret |= report(data->options,\n-\t\t\t\t &data->obj->oid, data->obj->type,\n+\t\t\t\t data->oid, OBJ_BLOB,\n \t\t\t\t FSCK_MSG_GITMODULES_PATH,\n \t\t\t\t \"disallowed submodule path: %s\",\n \t\t\t\t value);\n@@ -932,17 +932,17 @@ static int fsck_gitmodules_fn(const char *var, const char *value, void *vdata)\n \treturn 0;\n }\n \n-static int fsck_blob(struct blob *blob, const char *buf,\n+static int fsck_blob(const struct object_id *oid, const char *buf,\n \t\t unsigned long size, struct fsck_options *options)\n {\n \tstruct fsck_gitmodules_data data;\n \tstruct config_options config_opts = { 0 };\n \n-\tif (!oidset_contains(&gitmodules_found, &blob->object.oid))\n+\tif (!oidset_contains(&gitmodules_found, oid))\n \t\treturn 0;\n-\toidset_insert(&gitmodules_done, &blob->object.oid);\n+\toidset_insert(&gitmodules_done, oid);\n \n-\tif (object_on_skiplist(options, &blob->object.oid))\n+\tif (object_on_skiplist(options, oid))\n \t\treturn 0;\n \n \tif (!buf) {\n@@ -951,18 +951,18 @@ static int fsck_blob(struct blob *blob, const char *buf,\n \t\t * blob too gigantic to load into memory. Let's just consider\n \t\t * that an error.\n \t\t */\n-\t\treturn report(options, &blob->object.oid, blob->object.type,\n+\t\treturn report(options, oid, OBJ_BLOB,\n \t\t\t FSCK_MSG_GITMODULES_LARGE,\n \t\t\t \".gitmodules too large to parse\");\n \t}\n \n-\tdata.obj = &blob->object;\n+\tdata.oid = oid;\n \tdata.options = options;\n \tdata.ret = 0;\n \tconfig_opts.error_action = CONFIG_ERROR_SILENT;\n \tif (git_config_from_mem(fsck_gitmodules_fn, CONFIG_ORIGIN_BLOB,\n \t\t\t\t\".gitmodules\", buf, size, &data, &config_opts))\n-\t\tdata.ret |= report(options, &blob->object.oid, blob->object.type,\n+\t\tdata.ret |= report(options, oid, OBJ_BLOB,\n \t\t\t\t FSCK_MSG_GITMODULES_PARSE,\n \t\t\t\t \"could not parse gitmodules blob\");\n \n@@ -976,7 +976,7 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \t\treturn report(options, NULL, OBJ_NONE, FSCK_MSG_BAD_OBJECT_SHA1, \"no valid object to fsck\");\n \n \tif (obj->type == OBJ_BLOB)\n-\t\treturn fsck_blob((struct blob *)obj, data, size, options);\n+\t\treturn fsck_blob(&obj->oid, data, size, options);\n \tif (obj->type == OBJ_TREE)\n \t\treturn fsck_tree((struct tree *) obj, data, size, options);\n \tif (obj->type == OBJ_COMMIT)\n@@ -1042,7 +1042,7 @@ int fsck_finish(struct fsck_options *options)\n \t\t}\n \n \t\tif (type == OBJ_BLOB)\n-\t\t\tret |= fsck_blob(blob, buf, size, options);\n+\t\t\tret |= fsck_blob(&blob->object.oid, buf, size, options);\n \t\telse\n \t\t\tret |= report(options,\n \t\t\t\t &blob->object.oid, blob->object.type,\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384351","messageId":"20191018045953.GQ17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 17/23] fsck: drop blob struct from fsck_finish()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:59:54Z","receivedAt":"2019-10-18T04:59:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since fsck_blob() no longer requires us to have a \"struct blob\", we\ndon't need to create one. Which also means we don't need to worry about\nhandling the case that lookup_blob() returns NULL (we'll still catch\nwrongly-identified blobs when we read the actual object contents and\ntype from disk).\n\nSigned-off-by: Jeff King \n---\n fsck.c | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 6e9640a1a6..4ff0ceb4ac 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -1013,7 +1013,6 @@ int fsck_finish(struct fsck_options *options)\n \n \toidset_iter_init(&gitmodules_found, &iter);\n \twhile ((oid = oidset_iter_next(&iter))) {\n-\t\tstruct blob *blob;\n \t\tenum object_type type;\n \t\tunsigned long size;\n \t\tchar *buf;\n@@ -1021,31 +1020,22 @@ int fsck_finish(struct fsck_options *options)\n \t\tif (oidset_contains(&gitmodules_done, oid))\n \t\t\tcontinue;\n \n-\t\tblob = lookup_blob(the_repository, oid);\n-\t\tif (!blob) {\n-\t\t\tstruct object *obj = lookup_unknown_object(oid);\n-\t\t\tret |= report(options, &obj->oid, obj->type,\n-\t\t\t\t FSCK_MSG_GITMODULES_BLOB,\n-\t\t\t\t \"non-blob found at .gitmodules\");\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tbuf = read_object_file(oid, &type, &size);\n \t\tif (!buf) {\n-\t\t\tif (is_promisor_object(&blob->object.oid))\n+\t\t\tif (is_promisor_object(oid))\n \t\t\t\tcontinue;\n \t\t\tret |= report(options,\n-\t\t\t\t &blob->object.oid, blob->object.type,\n+\t\t\t\t oid, OBJ_BLOB,\n \t\t\t\t FSCK_MSG_GITMODULES_MISSING,\n \t\t\t\t \"unable to read .gitmodules blob\");\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (type == OBJ_BLOB)\n-\t\t\tret |= fsck_blob(&blob->object.oid, buf, size, options);\n+\t\t\tret |= fsck_blob(oid, buf, size, options);\n \t\telse\n \t\t\tret |= report(options,\n-\t\t\t\t &blob->object.oid, blob->object.type,\n+\t\t\t\t oid, type,\n \t\t\t\t FSCK_MSG_GITMODULES_BLOB,\n \t\t\t\t \"non-blob found at .gitmodules\");\n \t\tfree(buf);\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384352","messageId":"20191018050004.GR17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 18/23] fsck: don't require an object struct for fsck_ident()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T05:00:04Z","receivedAt":"2019-10-18T05:00:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The only thing we do with the struct is pass its oid and type to\nreport(). We can just take those explicitly, which gives our callers\nmore flexibility.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 30 ++++++++++++++++--------------\n 1 file changed, 16 insertions(+), 14 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 4ff0ceb4ac..e1d06fb210 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -717,7 +717,9 @@ static int verify_headers(const void *data, unsigned long size,\n \t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n-static int fsck_ident(const char **ident, struct object *obj, struct fsck_options *options)\n+static int fsck_ident(const char **ident,\n+\t\t const struct object_id *oid, enum object_type type,\n+\t\t struct fsck_options *options)\n {\n \tconst char *p = *ident;\n \tchar *end;\n@@ -727,28 +729,28 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option\n \t\t(*ident)++;\n \n \tif (*p == '<')\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n+\t\treturn report(options, oid, type, FSCK_MSG_MISSING_NAME_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n \tp += strcspn(p, \"<>\\n\");\n \tif (*p == '>')\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n+\t\treturn report(options, oid, type, FSCK_MSG_BAD_NAME, \"invalid author/committer line - bad name\");\n \tif (*p != '<')\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n+\t\treturn report(options, oid, type, FSCK_MSG_MISSING_EMAIL, \"invalid author/committer line - missing email\");\n \tif (p[-1] != ' ')\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_MISSING_SPACE_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n+\t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_EMAIL, \"invalid author/committer line - missing space before email\");\n \tp++;\n \tp += strcspn(p, \"<>\\n\");\n \tif (*p != '>')\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n+\t\treturn report(options, oid, type, FSCK_MSG_BAD_EMAIL, \"invalid author/committer line - bad email\");\n \tp++;\n \tif (*p != ' ')\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n+\t\treturn report(options, oid, type, FSCK_MSG_MISSING_SPACE_BEFORE_DATE, \"invalid author/committer line - missing space before date\");\n \tp++;\n \tif (*p == '0' && p[1] != ' ')\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n+\t\treturn report(options, oid, type, FSCK_MSG_ZERO_PADDED_DATE, \"invalid author/committer line - zero-padded date\");\n \tif (date_overflows(parse_timestamp(p, &end, 10)))\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n+\t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE_OVERFLOW, \"invalid author/committer line - date causes integer overflow\");\n \tif ((end == p || *end != ' '))\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n+\t\treturn report(options, oid, type, FSCK_MSG_BAD_DATE, \"invalid author/committer line - bad date\");\n \tp = end + 1;\n \tif ((*p != '+' && *p != '-') ||\n \t !isdigit(p[1]) ||\n@@ -756,7 +758,7 @@ static int fsck_ident(const char **ident, struct object *obj, struct fsck_option\n \t !isdigit(p[3]) ||\n \t !isdigit(p[4]) ||\n \t (p[5] != '\\n'))\n-\t\treturn report(options, &obj->oid, obj->type, FSCK_MSG_BAD_TIMEZONE, \"invalid author/committer line - bad time zone\");\n+\t\treturn report(options, oid, type, FSCK_MSG_BAD_TIMEZONE, \"invalid author/committer line - bad time zone\");\n \tp += 6;\n \treturn 0;\n }\n@@ -792,7 +794,7 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \tauthor_count = 0;\n \twhile (skip_prefix(buffer, \"author \", &buffer)) {\n \t\tauthor_count++;\n-\t\terr = fsck_ident(&buffer, &commit->object, options);\n+\t\terr = fsck_ident(&buffer, &commit->object.oid, commit->object.type, options);\n \t\tif (err)\n \t\t\treturn err;\n \t}\n@@ -804,7 +806,7 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \t\treturn err;\n \tif (!skip_prefix(buffer, \"committer \", &buffer))\n \t\treturn report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n-\terr = fsck_ident(&buffer, &commit->object, options);\n+\terr = fsck_ident(&buffer, &commit->object.oid, commit->object.type, options);\n \tif (err)\n \t\treturn err;\n \tif (memchr(buffer_begin, '\\0', size)) {\n@@ -882,7 +884,7 @@ static int fsck_tag(struct tag *tag, const char *buffer,\n \t\t\tgoto done;\n \t}\n \telse\n-\t\tret = fsck_ident(&buffer, &tag->object, options);\n+\t\tret = fsck_ident(&buffer, &tag->object.oid, tag->object.type, options);\n \n done:\n \tstrbuf_release(&sb);\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384353","messageId":"20191018050050.GS17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 19/23] fsck: don't require an object struct in verify_headers()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T05:00:50Z","receivedAt":"2019-10-18T05:00:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We only need the oid and type to pass on to report(). Let's accept the\nbroken-out parameters to give our callers more flexibility.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex e1d06fb210..50c93200ed 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -687,7 +687,8 @@ static int fsck_tree(struct tree *item,\n }\n \n static int verify_headers(const void *data, unsigned long size,\n-\t\t\t struct object *obj, struct fsck_options *options)\n+\t\t\t const struct object_id *oid, enum object_type type,\n+\t\t\t struct fsck_options *options)\n {\n \tconst char *buffer = (const char *)data;\n \tunsigned long i;\n@@ -695,7 +696,7 @@ static int verify_headers(const void *data, unsigned long size,\n \tfor (i = 0; i < size; i++) {\n \t\tswitch (buffer[i]) {\n \t\tcase '\\0':\n-\t\t\treturn report(options, &obj->oid, obj->type,\n+\t\t\treturn report(options, oid, type,\n \t\t\t\tFSCK_MSG_NUL_IN_HEADER,\n \t\t\t\t\"unterminated header: NUL at offset %ld\", i);\n \t\tcase '\\n':\n@@ -713,7 +714,7 @@ static int verify_headers(const void *data, unsigned long size,\n \tif (size && buffer[size - 1] == '\\n')\n \t\treturn 0;\n \n-\treturn report(options, &obj->oid, obj->type,\n+\treturn report(options, oid, type,\n \t\tFSCK_MSG_UNTERMINATED_HEADER, \"unterminated header\");\n }\n \n@@ -772,7 +773,8 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \tconst char *buffer_begin = buffer;\n \tconst char *p;\n \n-\tif (verify_headers(buffer, size, &commit->object, options))\n+\tif (verify_headers(buffer, size, &commit->object.oid,\n+\t\t\t commit->object.type, options))\n \t\treturn -1;\n \n \tif (!skip_prefix(buffer, \"tree \", &buffer))\n@@ -827,7 +829,7 @@ static int fsck_tag(struct tag *tag, const char *buffer,\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *p;\n \n-\tret = verify_headers(buffer, size, &tag->object, options);\n+\tret = verify_headers(buffer, size, &tag->object.oid, tag->object.type, options);\n \tif (ret)\n \t\tgoto done;\n \n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384354","messageId":"20191018050059.GT17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 20/23] fsck: rename vague \"oid\" local variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T05:00:59Z","receivedAt":"2019-10-18T05:01:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In fsck_commit() and fsck_tag(), we have local \"oid\" variables used for\nparsing parent and tagged-object oids. Let's give these more specific\nnames in preparation for the functions taking an \"oid\" parameter for the\nobject we're checking.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 50c93200ed..42e7d1f71f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -767,7 +767,7 @@ static int fsck_ident(const char **ident,\n static int fsck_commit(struct commit *commit, const char *buffer,\n \t\t unsigned long size, struct fsck_options *options)\n {\n-\tstruct object_id tree_oid, oid;\n+\tstruct object_id tree_oid, parent_oid;\n \tunsigned author_count;\n \tint err;\n \tconst char *buffer_begin = buffer;\n@@ -786,7 +786,7 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \t}\n \tbuffer = p + 1;\n \twhile (skip_prefix(buffer, \"parent \", &buffer)) {\n-\t\tif (parse_oid_hex(buffer, &oid, &p) || *p != '\\n') {\n+\t\tif (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n \t\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n \t\t\tif (err)\n \t\t\t\treturn err;\n@@ -823,7 +823,7 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n static int fsck_tag(struct tag *tag, const char *buffer,\n \t\t unsigned long size, struct fsck_options *options)\n {\n-\tstruct object_id oid;\n+\tstruct object_id tagged_oid;\n \tint ret = 0;\n \tchar *eol;\n \tstruct strbuf sb = STRBUF_INIT;\n@@ -837,7 +837,7 @@ static int fsck_tag(struct tag *tag, const char *buffer,\n \t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_OBJECT, \"invalid format - expected 'object' line\");\n \t\tgoto done;\n \t}\n-\tif (parse_oid_hex(buffer, &oid, &p) || *p != '\\n') {\n+\tif (parse_oid_hex(buffer, &tagged_oid, &p) || *p != '\\n') {\n \t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_BAD_OBJECT_SHA1, \"invalid 'object' line format - bad sha1\");\n \t\tif (ret)\n \t\t\tgoto done;\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384355","messageId":"20191018050125.GU17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 21/23] fsck: accept an oid instead of a \"struct tag\" for fsck_tag()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T05:01:26Z","receivedAt":"2019-10-18T05:01:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't actually look at the tag struct in fsck_tag() beyond its oid\nand type (which is obviously OBJ_TAG). Just taking an oid gives our\ncallers more flexibility to avoid creating or parsing a struct, and\nmakes it clear that we are fscking just what is in the buffer, not any\npre-parsed bits from the struct.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 42e7d1f71f..38be501278 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -820,7 +820,7 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \treturn 0;\n }\n \n-static int fsck_tag(struct tag *tag, const char *buffer,\n+static int fsck_tag(const struct object_id *oid, const char *buffer,\n \t\t unsigned long size, struct fsck_options *options)\n {\n \tstruct object_id tagged_oid;\n@@ -829,48 +829,48 @@ static int fsck_tag(struct tag *tag, const char *buffer,\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *p;\n \n-\tret = verify_headers(buffer, size, &tag->object.oid, tag->object.type, options);\n+\tret = verify_headers(buffer, size, oid, OBJ_TAG, options);\n \tif (ret)\n \t\tgoto done;\n \n \tif (!skip_prefix(buffer, \"object \", &buffer)) {\n-\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_OBJECT, \"invalid format - expected 'object' line\");\n+\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_OBJECT, \"invalid format - expected 'object' line\");\n \t\tgoto done;\n \t}\n \tif (parse_oid_hex(buffer, &tagged_oid, &p) || *p != '\\n') {\n-\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_BAD_OBJECT_SHA1, \"invalid 'object' line format - bad sha1\");\n+\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_BAD_OBJECT_SHA1, \"invalid 'object' line format - bad sha1\");\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n \tbuffer = p + 1;\n \n \tif (!skip_prefix(buffer, \"type \", &buffer)) {\n-\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TYPE_ENTRY, \"invalid format - expected 'type' line\");\n+\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TYPE_ENTRY, \"invalid format - expected 'type' line\");\n \t\tgoto done;\n \t}\n \teol = strchr(buffer, '\\n');\n \tif (!eol) {\n-\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TYPE, \"invalid format - unexpected end after 'type' line\");\n+\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TYPE, \"invalid format - unexpected end after 'type' line\");\n \t\tgoto done;\n \t}\n \tif (type_from_string_gently(buffer, eol - buffer, 1) < 0)\n-\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_BAD_TYPE, \"invalid 'type' value\");\n+\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_BAD_TYPE, \"invalid 'type' value\");\n \tif (ret)\n \t\tgoto done;\n \tbuffer = eol + 1;\n \n \tif (!skip_prefix(buffer, \"tag \", &buffer)) {\n-\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TAG_ENTRY, \"invalid format - expected 'tag' line\");\n+\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAG_ENTRY, \"invalid format - expected 'tag' line\");\n \t\tgoto done;\n \t}\n \teol = strchr(buffer, '\\n');\n \tif (!eol) {\n-\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TAG, \"invalid format - unexpected end after 'type' line\");\n+\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAG, \"invalid format - unexpected end after 'type' line\");\n \t\tgoto done;\n \t}\n \tstrbuf_addf(&sb, \"refs/tags/%.*s\", (int)(eol - buffer), buffer);\n \tif (check_refname_format(sb.buf, 0)) {\n-\t\tret = report(options, &tag->object.oid, tag->object.type,\n+\t\tret = report(options, oid, OBJ_TAG,\n \t\t\t FSCK_MSG_BAD_TAG_NAME,\n \t\t\t \"invalid 'tag' name: %.*s\",\n \t\t\t (int)(eol - buffer), buffer);\n@@ -881,12 +881,12 @@ static int fsck_tag(struct tag *tag, const char *buffer,\n \n \tif (!skip_prefix(buffer, \"tagger \", &buffer)) {\n \t\t/* early tags do not contain 'tagger' lines; warn only */\n-\t\tret = report(options, &tag->object.oid, tag->object.type, FSCK_MSG_MISSING_TAGGER_ENTRY, \"invalid format - expected 'tagger' line\");\n+\t\tret = report(options, oid, OBJ_TAG, FSCK_MSG_MISSING_TAGGER_ENTRY, \"invalid format - expected 'tagger' line\");\n \t\tif (ret)\n \t\t\tgoto done;\n \t}\n \telse\n-\t\tret = fsck_ident(&buffer, &tag->object.oid, tag->object.type, options);\n+\t\tret = fsck_ident(&buffer, oid, OBJ_TAG, options);\n \n done:\n \tstrbuf_release(&sb);\n@@ -987,8 +987,7 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \t\treturn fsck_commit((struct commit *) obj, (const char *) data,\n \t\t\tsize, options);\n \tif (obj->type == OBJ_TAG)\n-\t\treturn fsck_tag((struct tag *) obj, (const char *) data,\n-\t\t\tsize, options);\n+\t\treturn fsck_tag(&obj->oid, data, size, options);\n \n \treturn report(options, &obj->oid, obj->type,\n \t\t FSCK_MSG_UNKNOWN_TYPE,\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384356","messageId":"20191018050148.GV17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 22/23] fsck: accept an oid instead of a \"struct commit\" for fsck_commit()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T05:01:48Z","receivedAt":"2019-10-18T05:01:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't actually look at the commit struct in fsck_commit() beyond its\noid and type (which is obviously OBJ_COMMIT). Just taking an oid gives\nour callers more flexibility to avoid creating or parsing a struct, and\nmakes it clear that we are fscking just what is in the buffer, not any\npre-parsed bits from the struct.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 38be501278..f8c5bbe891 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -764,8 +764,9 @@ static int fsck_ident(const char **ident,\n \treturn 0;\n }\n \n-static int fsck_commit(struct commit *commit, const char *buffer,\n-\t\t unsigned long size, struct fsck_options *options)\n+static int fsck_commit(const struct object_id *oid,\n+\t\t const char *buffer, unsigned long size,\n+\t\t struct fsck_options *options)\n {\n \tstruct object_id tree_oid, parent_oid;\n \tunsigned author_count;\n@@ -773,21 +774,20 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \tconst char *buffer_begin = buffer;\n \tconst char *p;\n \n-\tif (verify_headers(buffer, size, &commit->object.oid,\n-\t\t\t commit->object.type, options))\n+\tif (verify_headers(buffer, size, oid, OBJ_COMMIT, options))\n \t\treturn -1;\n \n \tif (!skip_prefix(buffer, \"tree \", &buffer))\n-\t\treturn report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n+\t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_TREE, \"invalid format - expected 'tree' line\");\n \tif (parse_oid_hex(buffer, &tree_oid, &p) || *p != '\\n') {\n-\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n+\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_TREE_SHA1, \"invalid 'tree' line format - bad sha1\");\n \t\tif (err)\n \t\t\treturn err;\n \t}\n \tbuffer = p + 1;\n \twhile (skip_prefix(buffer, \"parent \", &buffer)) {\n \t\tif (parse_oid_hex(buffer, &parent_oid, &p) || *p != '\\n') {\n-\t\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n+\t\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_BAD_PARENT_SHA1, \"invalid 'parent' line format - bad sha1\");\n \t\t\tif (err)\n \t\t\t\treturn err;\n \t\t}\n@@ -796,23 +796,23 @@ static int fsck_commit(struct commit *commit, const char *buffer,\n \tauthor_count = 0;\n \twhile (skip_prefix(buffer, \"author \", &buffer)) {\n \t\tauthor_count++;\n-\t\terr = fsck_ident(&buffer, &commit->object.oid, commit->object.type, options);\n+\t\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n \t\tif (err)\n \t\t\treturn err;\n \t}\n \tif (author_count < 1)\n-\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n+\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_AUTHOR, \"invalid format - expected 'author' line\");\n \telse if (author_count > 1)\n-\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n+\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_MULTIPLE_AUTHORS, \"invalid format - multiple 'author' lines\");\n \tif (err)\n \t\treturn err;\n \tif (!skip_prefix(buffer, \"committer \", &buffer))\n-\t\treturn report(options, &commit->object.oid, commit->object.type, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n-\terr = fsck_ident(&buffer, &commit->object.oid, commit->object.type, options);\n+\t\treturn report(options, oid, OBJ_COMMIT, FSCK_MSG_MISSING_COMMITTER, \"invalid format - expected 'committer' line\");\n+\terr = fsck_ident(&buffer, oid, OBJ_COMMIT, options);\n \tif (err)\n \t\treturn err;\n \tif (memchr(buffer_begin, '\\0', size)) {\n-\t\terr = report(options, &commit->object.oid, commit->object.type, FSCK_MSG_NUL_IN_COMMIT,\n+\t\terr = report(options, oid, OBJ_COMMIT, FSCK_MSG_NUL_IN_COMMIT,\n \t\t\t \"NUL byte in the commit object body\");\n \t\tif (err)\n \t\t\treturn err;\n@@ -984,8 +984,7 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \tif (obj->type == OBJ_TREE)\n \t\treturn fsck_tree((struct tree *) obj, data, size, options);\n \tif (obj->type == OBJ_COMMIT)\n-\t\treturn fsck_commit((struct commit *) obj, (const char *) data,\n-\t\t\tsize, options);\n+\t\treturn fsck_commit(&obj->oid, data, size, options);\n \tif (obj->type == OBJ_TAG)\n \t\treturn fsck_tag(&obj->oid, data, size, options);\n \n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384357","messageId":"20191018050208.GW17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 23/23] fsck: accept an oid instead of a \"struct tree\" for fsck_tree()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T05:02:08Z","receivedAt":"2019-10-18T05:02:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't actually look at the tree struct in fsck_tree() beyond its oid\nand type (which is obviously OBJ_TREE). Just taking an oid gives our\ncallers more flexibility to avoid creating a struct, and makes it clear\nthat we are fscking just what is in the buffer, not any pre-parsed bits\nfrom the struct.\n\nSigned-off-by: Jeff King \n---\n fsck.c | 30 +++++++++++++++---------------\n 1 file changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex f8c5bbe891..ac4ba4c8e8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -566,7 +566,7 @@ static int verify_ordered(unsigned mode1, const char *name1, unsigned mode2, con\n \treturn c1 < c2 ? 0 : TREE_UNORDERED;\n }\n \n-static int fsck_tree(struct tree *item,\n+static int fsck_tree(const struct object_id *oid,\n \t\t const char *buffer, unsigned long size,\n \t\t struct fsck_options *options)\n {\n@@ -586,7 +586,7 @@ static int fsck_tree(struct tree *item,\n \tconst char *o_name;\n \n \tif (init_tree_desc_gently(&desc, buffer, size)) {\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\treturn retval;\n \t}\n \n@@ -613,13 +613,13 @@ static int fsck_tree(struct tree *item,\n \t\t\t\toidset_insert(&gitmodules_found, oid);\n \t\t\telse\n \t\t\t\tretval += report(options,\n-\t\t\t\t\t\t &item->object.oid, item->object.type,\n+\t\t\t\t\t\t oid, OBJ_TREE,\n \t\t\t\t\t\t FSCK_MSG_GITMODULES_SYMLINK,\n \t\t\t\t\t\t \".gitmodules is a symbolic link\");\n \t\t}\n \n \t\tif (update_tree_entry_gently(&desc)) {\n-\t\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n+\t\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_TREE, \"cannot be parsed as a tree\");\n \t\t\tbreak;\n \t\t}\n \n@@ -664,25 +664,25 @@ static int fsck_tree(struct tree *item,\n \t}\n \n \tif (has_null_sha1)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_NULL_SHA1, \"contains entries pointing to null sha1\");\n \tif (has_full_path)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_FULL_PATHNAME, \"contains full pathnames\");\n \tif (has_empty_name)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_EMPTY_NAME, \"contains empty pathname\");\n \tif (has_dot)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_HAS_DOT, \"contains '.'\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOT, \"contains '.'\");\n \tif (has_dotdot)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTDOT, \"contains '..'\");\n \tif (has_dotgit)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_HAS_DOTGIT, \"contains '.git'\");\n \tif (has_zero_pad)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_ZERO_PADDED_FILEMODE, \"contains zero-padded file modes\");\n \tif (has_bad_modes)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_BAD_FILEMODE, \"contains bad file modes\");\n \tif (has_dup_entries)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_DUPLICATE_ENTRIES, \"contains duplicate file entries\");\n \tif (not_properly_sorted)\n-\t\tretval += report(options, &item->object.oid, item->object.type, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n+\t\tretval += report(options, oid, OBJ_TREE, FSCK_MSG_TREE_NOT_SORTED, \"not properly sorted\");\n \treturn retval;\n }\n \n@@ -982,7 +982,7 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n \tif (obj->type == OBJ_BLOB)\n \t\treturn fsck_blob(&obj->oid, data, size, options);\n \tif (obj->type == OBJ_TREE)\n-\t\treturn fsck_tree((struct tree *) obj, data, size, options);\n+\t\treturn fsck_tree(&obj->oid, data, size, options);\n \tif (obj->type == OBJ_COMMIT)\n \t\treturn fsck_commit(&obj->oid, data, size, options);\n \tif (obj->type == OBJ_TAG)\n-- \n2.23.0.1228.gee29b05929\n"},{"id":"384358","messageId":"20191018044103.GA17625@sigill.intra.peff.net","threadId":"52064","inReplyTo":null,"subject":"[PATCH 0/23] parsing and fsck cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:41:03Z","receivedAt":"2019-10-18T05:07:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The thread starting at:\n\n https://public-inbox.org/git/xmqqo8zxnz0m.fsf@gitster-ct.c.googlers.com/\n\ndiscusses some issues with our handling of corrupt objects, as well as\nsome weirdness in fsck. This series is my attempt to clean it up. The\nnumber of patches is a little daunting, but the early ones are the most\ninteresting. The latter half is part of a big refactor/cleanup that's\nmostly mechanical (and isn't strictly necessary; see below for\ndiscussion).\n\n [01/23]: parse_commit_buffer(): treat lookup_commit() failure as parse error\n [02/23]: parse_commit_buffer(): treat lookup_tree() failure as parse error\n [03/23]: parse_tag_buffer(): treat NULL tag pointer as parse error\n [04/23]: remember commit/tag parse failures\n\n These ones are tightening up our parser to report failures more\n consistently. The first one definitely fixes a demonstrable bug, and\n I suspect the rest of them are fixing hard-to-trigger but lurking\n segfaults.\n\n [05/23]: fsck: stop checking commit->tree value\n [06/23]: fsck: stop checking commit->parent counts\n [07/23]: fsck: stop checking tag->tagged\n [08/23]: fsck: require an actual buffer for non-blobs\n\n These ones clean up weirdness where fsck is dependent on the results\n of parse_commit(), etc, rather than just looking at the buffer we\n gave it. I don't think they're _hurting_ anything, but it certainly\n makes following the fsck logic more confusing.\n\n [09/23]: fsck: unify object-name code\n\n Cleanup that fixes a few minor bugs.\n\n [10/23]: fsck_describe_object(): build on our get_object_name() primitive\n [11/23]: fsck: use oids rather than objects for object_name API\n [12/23]: fsck: don't require full object structs for display functions\n [13/23]: fsck: only provide oid/type in fsck_error callback\n [14/23]: fsck: only require an oid for skiplist functions\n [15/23]: fsck: don't require an object struct for report()\n [16/23]: fsck: accept an oid instead of a \"struct blob\" for fsck_blob()\n [17/23]: fsck: drop blob struct from fsck_finish()\n [18/23]: fsck: don't require an object struct for fsck_ident()\n [19/23]: fsck: don't require an object struct in verify_headers()\n [20/23]: fsck: rename vague \"oid\" local variables\n [21/23]: fsck: accept an oid instead of a \"struct tag\" for fsck_tag()\n [22/23]: fsck: accept an oid instead of a \"struct commit\" for fsck_commit()\n [23/23]: fsck: accept an oid instead of a \"struct tree\" for fsck_tree()\n\n This a string of refactors that ends up with all of the\n type-specific fsck functions not getting an object struct at all.\n My goal there was two-fold:\n\n - it makes it harder to introduce weirdness like we saw in\n\t patches 5-8.\n\n - it _could_ make things less awkward for callers like index-pack\n\t which don't necessarily have object structs. And at the end, we\n\t basically have an fsck_object() that doesn't need an object\n\t struct. But index-pack still calls fsck_walk(), which does (and\n\t which relies on the parsed values to traverse). It's not\n\t entirely clear to me whether index-pack needs to be doing\n\t fsck_walk() in the first place, or if it should be relying on\n\t the usual connectivity check.\n\n\t So I'm undecided whether this is worth taking on its own, or if\n\t trying to avoid object structs in the fsck code is just a\n\t fool's errand. I do think the result isn't too bad to look at,\n\t though and there are some minor improvements along the way\n\t (e.g., patch 17 is able to drop some awkwardness).\n\n Most of the patches are pretty mechanical. There are so many because\n I split it by call stack layer. If A calls B calls C, then I\n converted \"C\" away from \"struct object\" first, which enables\n converting \"B\", and so on.\n\n builtin/fsck.c | 126 ++++----\n commit-graph.c | 3 -\n commit.c | 33 ++-\n fsck.c | 386 +++++++++++--------------\n fsck.h | 39 ++-\n t/t1450-fsck.sh | 2 +-\n t/t5318-commit-graph.sh | 2 +-\n t/t6102-rev-list-unexpected-objects.sh | 2 +-\n tag.c | 21 +-\n 9 files changed, 312 insertions(+), 302 deletions(-)\n\n"},{"id":"384359","messageId":"20191018044212.GA17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 01/23] parse_commit_buffer(): treat lookup_commit() failure as parse error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:42:13Z","receivedAt":"2019-10-18T05:08:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"While parsing the parents of a commit, if we are able to parse an actual\noid but lookup_commit() fails on it (because we previously saw it in\nthis process as a different object type), we silently omit the parent\nand do not report any error to the caller.\n\nThe caller has no way of knowing this happened, because even an empty\nparent list is a valid parse result. As a result, it's possible to fool\nour \"rev-list\" connectivity check into accepting a corrupted set of\nobjects.\n\nThere's a test for this case already in t6102, but unfortunately it has\na slight error. It creates a broken commit with a parent line pointing\nto a blob, and then checks that rev-list notices the problem in two\ncases:\n\n 1. the \"lone\" case: we traverse the broken commit by itself (here we\n try to actually load the blob from disk and find out that it's not\n a commit)\n\n 2. the \"seen\" case: we parse the blob earlier in the process, and then\n when calling lookup_commit() we realize immediately that it's not a\n commit\n\nThe \"seen\" variant for this test mistakenly parsed another commit\ninstead of the blob, meaning that we were actually just testing the\n\"lone\" case again. Changing that reveals the breakage (and shows that\nthis fixes it).\n\nSigned-off-by: Jeff King \n---\n commit.c | 11 ++++++++---\n t/t6102-rev-list-unexpected-objects.sh | 2 +-\n 2 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/commit.c b/commit.c\nindex 40890ae7ce..6467c9e175 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -432,8 +432,11 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n \t\t\tcontinue;\n \t\tnew_parent = lookup_commit(r, &parent);\n-\t\tif (new_parent)\n-\t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n+\t\tif (!new_parent)\n+\t\t\treturn error(\"bad parent %s in commit %s\",\n+\t\t\t\t oid_to_hex(&parent),\n+\t\t\t\t oid_to_hex(&item->object.oid));\n+\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n \t}\n \tif (graft) {\n \t\tint i;\n@@ -442,7 +445,9 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \t\t\tnew_parent = lookup_commit(r,\n \t\t\t\t\t\t &graft->parent[i]);\n \t\t\tif (!new_parent)\n-\t\t\t\tcontinue;\n+\t\t\t\treturn error(\"bad graft parent %s in commit %s\",\n+\t\t\t\t\t oid_to_hex(&graft->parent[i]),\n+\t\t\t\t\t oid_to_hex(&item->object.oid));\n \t\t\tpptr = &commit_list_insert(new_parent, pptr)->next;\n \t\t}\n \t}\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 28611c978e..52cde097dd 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -52,7 +52,7 @@ test_expect_success 'traverse unexpected non-commit parent (lone)' '\n '\n \n test_expect_success 'traverse unexpected non-commit parent (seen)' '\n-\ttest_must_fail git rev-list --objects $commit $broken_commit \\\n+\ttest_must_fail git rev-list --objects $blob $broken_commit \\\n \t\t>output 2>&1 &&\n \ttest_i18ngrep \"not a commit\" output\n '\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384360","messageId":"20191018044328.GB17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 02/23] parse_commit_buffer(): treat lookup_tree() failure as parse error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:43:29Z","receivedAt":"2019-10-18T05:10:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If parsing a commit yields a valid tree oid, but we've seen that same\noid as a non-tree in the same process, the resulting commit struct will\nend up with a NULL tree pointer, but not otherwise report a parsing\nfailure.\n\nThat's perhaps convenient for callers which want to operate on even\npartially corrupt commits (e.g., by still looking at the parents). But\nit leaves a potential trap for most callers, who now have to manually\ncheck for a NULL tree. Most do not, and it's likely that there are\npossible segfaults lurking. I say \"possible\" because there are many\ncandidates, and I don't think it's worth following through on\nreproducing them when we can just fix them all in one spot. And\ncertainly we _have_ seen real-world cases, such as the one fixed by\n806278dead (commit-graph.c: handle corrupt/missing trees, 2019-09-05).\n\nNote that we can't quite drop the check in the caller added by that\ncommit yet, as there's some subtlety with repeated parsings (which will\nbe addressed in a future commit).\n\nSigned-off-by: Jeff King \n---\n commit.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/commit.c b/commit.c\nindex 6467c9e175..810419a168 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -401,6 +401,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tstruct commit_graft *graft;\n \tconst int tree_entry_len = the_hash_algo->hexsz + 5;\n \tconst int parent_entry_len = the_hash_algo->hexsz + 7;\n+\tstruct tree *tree;\n \n \tif (item->object.parsed)\n \t\treturn 0;\n@@ -412,7 +413,12 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tif (get_oid_hex(bufptr + 5, &parent) < 0)\n \t\treturn error(\"bad tree pointer in commit %s\",\n \t\t\t oid_to_hex(&item->object.oid));\n-\tset_commit_tree(item, lookup_tree(r, &parent));\n+\ttree = lookup_tree(r, &parent);\n+\tif (!tree)\n+\t\treturn error(\"bad tree pointer %s in commit %s\",\n+\t\t\t oid_to_hex(&parent),\n+\t\t\t oid_to_hex(&item->object.oid));\n+\tset_commit_tree(item, tree);\n \tbufptr += tree_entry_len + 1; /* \"tree \" + \"hex sha1\" + \"\\n\" */\n \tpptr = &item->parents;\n \n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384361","messageId":"20191018044534.GC17879@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"[PATCH 03/23] parse_tag_buffer(): treat NULL tag pointer as parse error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-18T04:45:35Z","receivedAt":"2019-10-18T05:12:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing a tag, we may end up with a NULL \"tagged\" field when\nthere's a type mismatch (e.g., the tag claims to point to object X as a\ncommit, but we previously saw X as a blob in the same process), but we\ndo not otherwise indicate a parse failure to the caller.\n\nThis is similar to the case discussed in the previous commit, where a\ncommit could end up with a NULL tree field: while slightly convenient\nfor callers who want to overlook a corrupt object, it means that normal\ncallers have to explicitly deal with this case (rather than just relying\non the return code from parsing). And most don't, leading to segfault\nfixes like the one in c77722b3ea (use get_tagged_oid(), 2019-09-05).\n\nLet's address this more centrally, by returning an error code from the\nparse itself, which most callers would already notice (adventurous\ncallers are free to ignore the error and continue looking at the\nstruct).\n\nThis also covers the case where the tag contains a nonsensical \"type\"\nfield (there we produced a user-visible error but still returned success\nto the caller; now we'll produce a slightly better message and return an\nerror).\n\nSigned-off-by: Jeff King \n---\nIt's possible that c77722b3ea fixed all of the segfaults here, though of\ncourse new callers must remember to use it instead of accessing\ntag->tagged directly. We could _possibly_ get rid of that check now, but\nthat assumes that all of the caller notice parse_tag() or parse_object()\nfailing. Having both gives us belt-and-suspenders protection against a\nsegfault.\n\n tag.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/tag.c b/tag.c\nindex bfa0e31435..6a51efda8d 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -167,10 +167,15 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \t} else if (!strcmp(type, tag_type)) {\n \t\titem->tagged = (struct object *)lookup_tag(r, &oid);\n \t} else {\n-\t\terror(\"Unknown type %s\", type);\n-\t\titem->tagged = NULL;\n+\t\treturn error(\"unknown tag type '%s' in %s\",\n+\t\t\t type, oid_to_hex(&item->object.oid));\n \t}\n \n+\tif (!item->tagged)\n+\t\treturn error(\"bad tag pointer to %s in %s\",\n+\t\t\t oid_to_hex(&oid),\n+\t\t\t oid_to_hex(&item->object.oid));\n+\n \tif (bufptr + 4 < tail && starts_with(bufptr, \"tag \"))\n \t\t; \t\t/* good */\n \telse\n-- \n2.23.0.1228.gee29b05929\n\n"},{"id":"384743","messageId":"xmqq1rv2hkzn.fsf@gitster-ct.c.googlers.com","threadId":"52064","inReplyTo":"20191018044212.GA17879@sigill.intra.peff.net","subject":"Re: [PATCH 01/23] parse_commit_buffer(): treat lookup_commit() failure as parse error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-24T03:37:32Z","receivedAt":"2019-10-24T03:37:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...\n> The \"seen\" variant for this test mistakenly parsed another commit\n> instead of the blob, meaning that we were actually just testing the\n> \"lone\" case again. Changing that reveals the breakage (and shows that\n> this fixes it).\n> ...\n> @@ -52,7 +52,7 @@ test_expect_success 'traverse unexpected non-commit parent (lone)' '\n> '\n> \n> test_expect_success 'traverse unexpected non-commit parent (seen)' '\n> -\ttest_must_fail git rev-list --objects $commit $broken_commit \\\n> +\ttest_must_fail git rev-list --objects $blob $broken_commit \\\n> \t\t>output 2>&1 &&\n> \ttest_i18ngrep \"not a commit\" output\n> '\n\nYikes. Thanks for spotting.\n\n"},{"id":"384744","messageId":"xmqqwocug5r5.fsf@gitster-ct.c.googlers.com","threadId":"52064","inReplyTo":"20191018044721.GD17879@sigill.intra.peff.net","subject":"Re: [PATCH 04/23] remember commit/tag parse failures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-24T03:51:58Z","receivedAt":"2019-10-24T03:52:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...\n> a separate check for a NULL tree. In fact, we can now ditch that\n> explicit tree check entirely, as we're covered robustly by this change\n> (and the previous recent change to treat a NULL tree as a parse error).\n> ...\n> @@ -855,9 +855,6 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n> \t\t\tdie(_(\"unable to parse commit %s\"),\n> \t\t\t\toid_to_hex(&(*list)->object.oid));\n> \t\ttree = get_commit_tree_oid(*list);\n> -\t\tif (!tree)\n> -\t\t\tdie(_(\"unable to get tree for %s\"),\n> -\t\t\t\toid_to_hex(&(*list)->object.oid));\n\nThe context before this hunk is to die when parse_commit(*list)\nfails, and because a successful parse_commit() will always set a\nnon-NULL tree now, the null-ness check on its tree becomes unneeded,\nwhich makes sense.\n\n"},{"id":"384745","messageId":"xmqqsgnig5il.fsf@gitster-ct.c.googlers.com","threadId":"52064","inReplyTo":"20191018044820.GE17879@sigill.intra.peff.net","subject":"Re: [PATCH 05/23] fsck: stop checking commit->tree value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-24T03:57:06Z","receivedAt":"2019-10-24T03:57:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> We check in fsck_commit_buffer() that commit->tree isn't NULL, which in\n> ...\n> So this code isn't helping anything. And it makes the fsck code slightly\n> more confusing and rigid (e.g., it requires that any commit structs have\n> already been parsed). Let's drop it.\n\nThanks; I recall we discussed this during a review of a topic.\n"},{"id":"384749","messageId":"xmqq8spafzjt.fsf@gitster-ct.c.googlers.com","threadId":"52064","inReplyTo":"20191018045613.GI17879@sigill.intra.peff.net","subject":"Re: [PATCH 09/23] fsck: unify object-name code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-24T06:05:58Z","receivedAt":"2019-10-24T06:06:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +/*\n> + * Subsystem for storing human-readable names for each object.\n> + *\n> + * If fsck_enable_object_names() has not been called, all other functions are\n> + * noops.\n\nI think it is clear from the context that put_ and get_ we see below\nrefer to functions whose true names have fsck_ prefix, but the fact\nthat this one has fsck_ spelled out disrupts that context.\n\nIOW, s/fsck_// for consistency, perhaps.\n\n> + * Use put_object_name() to seed initial names (e.g. from refnames); the fsck\n> + * code will extend that while walking trees, etc.\n> + *\n> + * Use get_object_name() to get a single name (or NULL if none). Or the more\n> + * convenient describe_object(), which always produces an output string with\n> + * the oid combined with the name (if any). Note that the return value points\n> + * to a rotating array of static buffers, and may be invalidated by a\n> + * subsequent call.\n> + */\n> +void fsck_enable_object_names(struct fsck_options *options);\n> +const char *fsck_get_object_name(struct fsck_options *options,\n> +\t\t\t\t struct object *obj);\n> +__attribute__((format (printf,3,4)))\n> +void fsck_put_object_name(struct fsck_options *options, struct object *obj,\n> +\t\t\t const char *fmt, ...);\n> +const char *fsck_describe_object(struct fsck_options *options,\n> +\t\t\t\t struct object *obj);\n> +\n> #endif\n> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\n> index 50d28e6fdb..7c7ff7e961 100755\n> --- a/t/t1450-fsck.sh\n> +++ b/t/t1450-fsck.sh\n> @@ -616,7 +616,7 @@ test_expect_success 'fsck --name-objects' '\n> \t\tremove_object $(git rev-parse julius:caesar.t) &&\n> \t\ttest_must_fail git fsck --name-objects >out &&\n> \t\ttree=$(git rev-parse --verify julius:) &&\n> -\t\ttest_i18ngrep -E \"$tree \\((refs/heads/master|HEAD)@\\{[0-9]*\\}:\" out\n> +\t\ttest_i18ngrep \"$tree (refs/tags/julius:\" out\n> \t)\n> '\n"},{"id":"384750","messageId":"xmqq4kzyfziq.fsf@gitster-ct.c.googlers.com","threadId":"52064","inReplyTo":"20191018045638.GJ17879@sigill.intra.peff.net","subject":"Re: [PATCH 10/23] fsck_describe_object(): build on our get_object_name() primitive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-24T06:06:37Z","receivedAt":"2019-10-24T06:06:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This isolates the implementation detail of using the decoration code to\n> our put/get functions.\n>\n> Signed-off-by: Jeff King \n> ---\n> Arguably this could be squashed into the previous commit. By not doing\n> so, it made describe_object() more of a pure code movement.\n\nIndeed it did.\n"},{"id":"384780","messageId":"20191024180110.GB12892@sigill.intra.peff.net","threadId":"52064","inReplyTo":"xmqq1rv2hkzn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 01/23] parse_commit_buffer(): treat lookup_commit() failure as parse error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-24T18:01:11Z","receivedAt":"2019-10-24T18:01:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 24, 2019 at 12:37:32PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ...\n> > The \"seen\" variant for this test mistakenly parsed another commit\n> > instead of the blob, meaning that we were actually just testing the\n> > \"lone\" case again. Changing that reveals the breakage (and shows that\n> > this fixes it).\n> > ...\n> > @@ -52,7 +52,7 @@ test_expect_success 'traverse unexpected non-commit parent (lone)' '\n> > '\n> > \n> > test_expect_success 'traverse unexpected non-commit parent (seen)' '\n> > -\ttest_must_fail git rev-list --objects $commit $broken_commit \\\n> > +\ttest_must_fail git rev-list --objects $blob $broken_commit \\\n> > \t\t>output 2>&1 &&\n> > \ttest_i18ngrep \"not a commit\" output\n> > '\n> \n> Yikes. Thanks for spotting.\n\nBy the way, I shuffled this one to the front so that it could be taken\nseparately for \"maint\" if you want (but it has been this way for at\nleast a decade, so I don't think it's urgent for v2.24).\n\n-Peff\n"},{"id":"384781","messageId":"20191024180710.GC12892@sigill.intra.peff.net","threadId":"52064","inReplyTo":"xmqq8spafzjt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 09/23] fsck: unify object-name code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-24T18:07:10Z","receivedAt":"2019-10-24T18:07:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 24, 2019 at 03:05:58PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > +/*\n> > + * Subsystem for storing human-readable names for each object.\n> > + *\n> > + * If fsck_enable_object_names() has not been called, all other functions are\n> > + * noops.\n> \n> I think it is clear from the context that put_ and get_ we see below\n> refer to functions whose true names have fsck_ prefix, but the fact\n> that this one has fsck_ spelled out disrupts that context.\n> \n> IOW, s/fsck_// for consistency, perhaps.\n\nHeh. I originally wrote just \"put\" and \"get\" for brevity, but then\nworried that wasn't clear enough. Let's just use the full names. We can\nafford the extra few bytes, and it makes it easier if we later need to\ngrep for them or whatever. Here's a replacement patch (I won't send the\nwhole series just yet, as it seems it hasn't gotten a whole lot of\nreview so far).\n\n-- >8 --\nSubject: [PATCH] fsck: unify object-name code\n\nCommit 90cf590f53 (fsck: optionally show more helpful info for broken\nlinks, 2016-07-17) added a system for decorating objects with names. The\ncode is split across builtin/fsck.c (which gives the initial names) and\nfsck.c (which adds to the names as it traverses the object graph). This\nleads to some duplication, where both sites have near-identical\ndescribe_object() functions (the difference being that the one in\nbuiltin/fsck.c uses a circular array of buffers to allow multiple calls\nin a single printf).\n\nLet's provide a unified object_name API for fsck. That lets us drop the\nduplication, as well as making the interface boundaries more clear\n(which will let us refactor the implementation more in a future patch).\n\nWe'll leave describe_object() in builtin/fsck.c as a thin wrapper around\nthe new API, as it relies on a static global to make its many callers a\nbit shorter.\n\nWe'll also convert the bare add_decoration() calls in builtin/fsck.c to\nput_object_name(). This fixes two minor bugs:\n\n 1. We leak many small strings. add_decoration() has a last-one-wins\n approach: it updates the decoration to the new string and returns\n the old one. But we ignore the return value, leaking the old\n string. This is quite common to trigger, since we look at reflogs:\n the tip of any ref will be described both by looking at the actual\n ref, as well as the latest reflog entry. So we'd always end up\n leaking one of those strings.\n\n 2. The last-one-wins approach gives us lousy names. For instance, we\n first look at all of the refs, and then all of the reflogs. So\n rather than seeing \"refs/heads/master\", we're likely to overwrite\n it with \"HEAD@{12345678}\". We're generally better off using the\n first name we find.\n\n And indeed, the test in t1450 expects this ugly HEAD@{} name. After\n this patch, we've switched to using fsck_put_object_name()'s\n first-one-wins semantics, and we output the more human-friendly\n \"refs/tags/julius\" (and the test is updated accordingly).\n\nSigned-off-by: Jeff King \n---\n builtin/fsck.c | 50 ++++++++----------------------\n fsck.c | 82 ++++++++++++++++++++++++++++++-------------------\n fsck.h | 24 +++++++++++++++\n t/t1450-fsck.sh | 2 +-\n 4 files changed, 89 insertions(+), 69 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 18403a94fa..237643cc1d 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -52,24 +52,7 @@ static int name_objects;\n \n static const char *describe_object(struct object *obj)\n {\n-\tstatic struct strbuf bufs[] = {\n-\t\tSTRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n-\t};\n-\tstatic int b = 0;\n-\tstruct strbuf *buf;\n-\tchar *name = NULL;\n-\n-\tif (name_objects)\n-\t\tname = lookup_decoration(fsck_walk_options.object_names, obj);\n-\n-\tbuf = bufs + b;\n-\tb = (b + 1) % ARRAY_SIZE(bufs);\n-\tstrbuf_reset(buf);\n-\tstrbuf_addstr(buf, oid_to_hex(&obj->oid));\n-\tif (name)\n-\t\tstrbuf_addf(buf, \" (%s)\", name);\n-\n-\treturn buf->buf;\n+\treturn fsck_describe_object(&fsck_walk_options, obj);\n }\n \n static const char *printable_type(struct object *obj)\n@@ -499,10 +482,10 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,\n \tif (!is_null_oid(oid)) {\n \t\tobj = lookup_object(the_repository, oid);\n \t\tif (obj && (obj->flags & HAS_OBJ)) {\n-\t\t\tif (timestamp && name_objects)\n-\t\t\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\t\t\tobj,\n-\t\t\t\t\txstrfmt(\"%s@{%\"PRItime\"}\", refname, timestamp));\n+\t\t\tif (timestamp)\n+\t\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\t\t\t\t \"%s@{%\"PRItime\"}\",\n+\t\t\t\t\t\t refname, timestamp);\n \t\t\tobj->flags |= USED;\n \t\t\tmark_object_reachable(obj);\n \t\t} else if (!is_promisor_object(oid)) {\n@@ -566,9 +549,8 @@ static int fsck_handle_ref(const char *refname, const struct object_id *oid,\n \t}\n \tdefault_refs++;\n \tobj->flags |= USED;\n-\tif (name_objects)\n-\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\tobj, xstrdup(refname));\n+\tfsck_put_object_name(&fsck_walk_options,\n+\t\t\t obj, \"%s\", refname);\n \tmark_object_reachable(obj);\n \n \treturn 0;\n@@ -742,9 +724,7 @@ static int fsck_cache_tree(struct cache_tree *it)\n \t\t\treturn 1;\n \t\t}\n \t\tobj->flags |= USED;\n-\t\tif (name_objects)\n-\t\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\t\tobj, xstrdup(\":\"));\n+\t\tfsck_put_object_name(&fsck_walk_options, obj, \":\");\n \t\tmark_object_reachable(obj);\n \t\tif (obj->type != OBJ_TREE)\n \t\t\terr |= objerror(obj, _(\"non-tree in cache-tree\"));\n@@ -830,8 +810,7 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (name_objects)\n-\t\tfsck_walk_options.object_names =\n-\t\t\txcalloc(1, sizeof(struct decoration));\n+\t\tfsck_enable_object_names(&fsck_walk_options);\n \n \tgit_config(fsck_config, NULL);\n \n@@ -890,9 +869,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t}\n \n \t\t\tobj->flags |= USED;\n-\t\t\tif (name_objects)\n-\t\t\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\t\t\tobj, xstrdup(arg));\n+\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\t\t\t \"%s\", arg);\n \t\t\tmark_object_reachable(obj);\n \t\t\tcontinue;\n \t\t}\n@@ -928,10 +906,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\tobj = &blob->object;\n \t\t\tobj->flags |= USED;\n-\t\t\tif (name_objects)\n-\t\t\t\tadd_decoration(fsck_walk_options.object_names,\n-\t\t\t\t\tobj,\n-\t\t\t\t\txstrfmt(\":%s\", active_cache[i]->name));\n+\t\t\tfsck_put_object_name(&fsck_walk_options, obj,\n+\t\t\t\t\t \":%s\", active_cache[i]->name);\n \t\t\tmark_object_reachable(obj);\n \t\t}\n \t\tif (active_cache_tree)\ndiff --git a/fsck.c b/fsck.c\nindex 347a0ef5c9..ecd5957362 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -312,15 +312,21 @@ static int report(struct fsck_options *options, struct object *object,\n \treturn result;\n }\n \n-static char *get_object_name(struct fsck_options *options, struct object *obj)\n+void fsck_enable_object_names(struct fsck_options *options)\n+{\n+\tif (!options->object_names)\n+\t\toptions->object_names = xcalloc(1, sizeof(struct decoration));\n+}\n+\n+const char *fsck_get_object_name(struct fsck_options *options, struct object *obj)\n {\n \tif (!options->object_names)\n \t\treturn NULL;\n \treturn lookup_decoration(options->object_names, obj);\n }\n \n-static void put_object_name(struct fsck_options *options, struct object *obj,\n-\tconst char *fmt, ...)\n+void fsck_put_object_name(struct fsck_options *options, struct object *obj,\n+\t\t\t const char *fmt, ...)\n {\n \tva_list ap;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -337,17 +343,27 @@ static void put_object_name(struct fsck_options *options, struct object *obj,\n \tva_end(ap);\n }\n \n-static const char *describe_object(struct fsck_options *o, struct object *obj)\n+const char *fsck_describe_object(struct fsck_options *options,\n+\t\t\t\t struct object *obj)\n {\n-\tstatic struct strbuf buf = STRBUF_INIT;\n-\tchar *name;\n-\n-\tstrbuf_reset(&buf);\n-\tstrbuf_addstr(&buf, oid_to_hex(&obj->oid));\n-\tif (o->object_names && (name = lookup_decoration(o->object_names, obj)))\n-\t\tstrbuf_addf(&buf, \" (%s)\", name);\n+\tstatic struct strbuf bufs[] = {\n+\t\tSTRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT\n+\t};\n+\tstatic int b = 0;\n+\tstruct strbuf *buf;\n+\tchar *name = NULL;\n+\n+\tif (options->object_names)\n+\t\tname = lookup_decoration(options->object_names, obj);\n+\n+\tbuf = bufs + b;\n+\tb = (b + 1) % ARRAY_SIZE(bufs);\n+\tstrbuf_reset(buf);\n+\tstrbuf_addstr(buf, oid_to_hex(&obj->oid));\n+\tif (name)\n+\t\tstrbuf_addf(buf, \" (%s)\", name);\n \n-\treturn buf.buf;\n+\treturn buf->buf;\n }\n \n static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *options)\n@@ -360,7 +376,7 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \tif (parse_tree(tree))\n \t\treturn -1;\n \n-\tname = get_object_name(options, &tree->object);\n+\tname = fsck_get_object_name(options, &tree->object);\n \tif (init_tree_desc_gently(&desc, tree->buffer, tree->size))\n \t\treturn -1;\n \twhile (tree_entry_gently(&desc, &entry)) {\n@@ -373,20 +389,21 @@ static int fsck_walk_tree(struct tree *tree, void *data, struct fsck_options *op\n \t\tif (S_ISDIR(entry.mode)) {\n \t\t\tobj = (struct object *)lookup_tree(the_repository, &entry.oid);\n \t\t\tif (name && obj)\n-\t\t\t\tput_object_name(options, obj, \"%s%s/\", name,\n-\t\t\t\t\tentry.path);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%s%s/\",\n+\t\t\t\t\t\t name, entry.path);\n \t\t\tresult = options->walk(obj, OBJ_TREE, data, options);\n \t\t}\n \t\telse if (S_ISREG(entry.mode) || S_ISLNK(entry.mode)) {\n \t\t\tobj = (struct object *)lookup_blob(the_repository, &entry.oid);\n \t\t\tif (name && obj)\n-\t\t\t\tput_object_name(options, obj, \"%s%s\", name,\n-\t\t\t\t\tentry.path);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%s%s\",\n+\t\t\t\t\t\t name, entry.path);\n \t\t\tresult = options->walk(obj, OBJ_BLOB, data, options);\n \t\t}\n \t\telse {\n \t\t\tresult = error(\"in tree %s: entry %s has bad mode %.6o\",\n-\t\t\t\t\tdescribe_object(options, &tree->object), entry.path, entry.mode);\n+\t\t\t\t fsck_describe_object(options, &tree->object),\n+\t\t\t\t entry.path, entry.mode);\n \t\t}\n \t\tif (result < 0)\n \t\t\treturn result;\n@@ -407,10 +424,10 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \tif (parse_commit(commit))\n \t\treturn -1;\n \n-\tname = get_object_name(options, &commit->object);\n+\tname = fsck_get_object_name(options, &commit->object);\n \tif (name)\n-\t\tput_object_name(options, &get_commit_tree(commit)->object,\n-\t\t\t\t\"%s:\", name);\n+\t\tfsck_put_object_name(options, &get_commit_tree(commit)->object,\n+\t\t\t\t \"%s:\", name);\n \n \tresult = options->walk((struct object *)get_commit_tree(commit),\n \t\t\t OBJ_TREE, data, options);\n@@ -441,13 +458,15 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \t\t\tstruct object *obj = &parents->item->object;\n \n \t\t\tif (counter++)\n-\t\t\t\tput_object_name(options, obj, \"%s^%d\",\n-\t\t\t\t\tname, counter);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%s^%d\",\n+\t\t\t\t\t\t name, counter);\n \t\t\telse if (generation > 0)\n-\t\t\t\tput_object_name(options, obj, \"%.*s~%d\",\n-\t\t\t\t\tname_prefix_len, name, generation + 1);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%.*s~%d\",\n+\t\t\t\t\t\t name_prefix_len, name,\n+\t\t\t\t\t\t generation + 1);\n \t\t\telse\n-\t\t\t\tput_object_name(options, obj, \"%s^\", name);\n+\t\t\t\tfsck_put_object_name(options, obj, \"%s^\",\n+\t\t\t\t\t\t name);\n \t\t}\n \t\tresult = options->walk((struct object *)parents->item, OBJ_COMMIT, data, options);\n \t\tif (result < 0)\n@@ -461,12 +480,12 @@ static int fsck_walk_commit(struct commit *commit, void *data, struct fsck_optio\n \n static int fsck_walk_tag(struct tag *tag, void *data, struct fsck_options *options)\n {\n-\tchar *name = get_object_name(options, &tag->object);\n+\tconst char *name = fsck_get_object_name(options, &tag->object);\n \n \tif (parse_tag(tag))\n \t\treturn -1;\n \tif (name)\n-\t\tput_object_name(options, tag->tagged, \"%s\", name);\n+\t\tfsck_put_object_name(options, tag->tagged, \"%s\", name);\n \treturn options->walk(tag->tagged, OBJ_ANY, data, options);\n }\n \n@@ -488,7 +507,8 @@ int fsck_walk(struct object *obj, void *data, struct fsck_options *options)\n \tcase OBJ_TAG:\n \t\treturn fsck_walk_tag((struct tag *)obj, data, options);\n \tdefault:\n-\t\terror(\"Unknown object type for %s\", describe_object(options, obj));\n+\t\terror(\"Unknown object type for %s\",\n+\t\t fsck_describe_object(options, obj));\n \t\treturn -1;\n \t}\n }\n@@ -962,10 +982,10 @@ int fsck_error_function(struct fsck_options *o,\n \tstruct object *obj, int msg_type, const char *message)\n {\n \tif (msg_type == FSCK_WARN) {\n-\t\twarning(\"object %s: %s\", describe_object(o, obj), message);\n+\t\twarning(\"object %s: %s\", fsck_describe_object(o, obj), message);\n \t\treturn 0;\n \t}\n-\terror(\"object %s: %s\", describe_object(o, obj), message);\n+\terror(\"object %s: %s\", fsck_describe_object(o, obj), message);\n \treturn 1;\n }\n \ndiff --git a/fsck.h b/fsck.h\nindex e479461075..f6f0c40060 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -67,4 +67,28 @@ int fsck_object(struct object *obj, void *data, unsigned long size,\n */\n int fsck_finish(struct fsck_options *options);\n \n+/*\n+ * Subsystem for storing human-readable names for each object.\n+ *\n+ * If fsck_enable_object_names() has not been called, all other functions are\n+ * noops.\n+ *\n+ * Use put_object_name() to seed initial names (e.g. from refnames); the fsck\n+ * code will extend that while walking trees, etc.\n+ *\n+ * Use get_object_name() to get a single name (or NULL if none). Or the more\n+ * convenient describe_object(), which always produces an output string with\n+ * the oid combined with the name (if any). Note that the return value points\n+ * to a rotating array of static buffers, and may be invalidated by a\n+ * subsequent call.\n+ */\n+void fsck_enable_object_names(struct fsck_options *options);\n+const char *fsck_get_object_name(struct fsck_options *options,\n+\t\t\t\t struct object *obj);\n+__attribute__((format (printf,3,4)))\n+void fsck_put_object_name(struct fsck_options *options, struct object *obj,\n+\t\t\t const char *fmt, ...);\n+const char *fsck_describe_object(struct fsck_options *options,\n+\t\t\t\t struct object *obj);\n+\n #endif\ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex 50d28e6fdb..7c7ff7e961 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -616,7 +616,7 @@ test_expect_success 'fsck --name-objects' '\n \t\tremove_object $(git rev-parse julius:caesar.t) &&\n \t\ttest_must_fail git fsck --name-objects >out &&\n \t\ttree=$(git rev-parse --verify julius:) &&\n-\t\ttest_i18ngrep -E \"$tree \\((refs/heads/master|HEAD)@\\{[0-9]*\\}:\" out\n+\t\ttest_i18ngrep \"$tree (refs/tags/julius:\" out\n \t)\n '\n \n-- \n2.24.0.rc1.670.g28e7e9fef7\n\n"},{"id":"384806","messageId":"20191024231220.96547-1-jonathantanmy@google.com","threadId":"52064","inReplyTo":"20191018044328.GB17879@sigill.intra.peff.net","subject":"Re: [PATCH 02/23] parse_commit_buffer(): treat lookup_tree() failure as parse error","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-24T23:12:20Z","receivedAt":"2019-10-24T23:12:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> And\n> certainly we _have_ seen real-world cases, such as the one fixed by\n> 806278dead (commit-graph.c: handle corrupt/missing trees, 2019-09-05).\n> \n> Note that we can't quite drop the check in the caller added by that\n> commit yet, as there's some subtlety with repeated parsings (which will\n> be addressed in a future commit).\n\nI tried figuring out what the subtlety is by undoing the check you\ndescribe, and did get a segfault in one of the tests, but couldn't\nfigure out exactly what is going on. Looking at the code, is it because\nload_tree_for_commit() in commit-graph.c uses the return value of\nlookup_tree() indiscriminately (which is the issue that this patch\nfixes)?\n\nThis patch itself and patch 1 looks good (with the reasoning in the\ncommit message), of course.\n"},{"id":"384808","messageId":"20191024232207.GA32602@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191024231220.96547-1-jonathantanmy@google.com","subject":"Re: [PATCH 02/23] parse_commit_buffer(): treat lookup_tree() failure as parse error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-24T23:22:07Z","receivedAt":"2019-10-24T23:22:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 24, 2019 at 04:12:20PM -0700, Jonathan Tan wrote:\n\n> > And\n> > certainly we _have_ seen real-world cases, such as the one fixed by\n> > 806278dead (commit-graph.c: handle corrupt/missing trees, 2019-09-05).\n> > \n> > Note that we can't quite drop the check in the caller added by that\n> > commit yet, as there's some subtlety with repeated parsings (which will\n> > be addressed in a future commit).\n> \n> I tried figuring out what the subtlety is by undoing the check you\n> describe, and did get a segfault in one of the tests, but couldn't\n> figure out exactly what is going on. Looking at the code, is it because\n> load_tree_for_commit() in commit-graph.c uses the return value of\n> lookup_tree() indiscriminately (which is the issue that this patch\n> fixes)?\n> \n> This patch itself and patch 1 looks good (with the reasoning in the\n> commit message), of course.\n\nIt's the issue addressed in patch 4. The issue is that some _other_ code\nalready called parse_commit(), got an error, but then for whatever\nreason ignored it. Now when we call parse_commit(), the \"parsed\" flag is\nset, so it returns success even though the tree is still NULL.\n\nThat commit fixes the problem and does drop the extra tree check.\n\n-Peff\n"},{"id":"384810","messageId":"20191024232546.70565-1-jonathantanmy@google.com","threadId":"52064","inReplyTo":"20191018044721.GD17879@sigill.intra.peff.net","subject":"Re: [PATCH 04/23] remember commit/tag parse failures","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-24T23:25:46Z","receivedAt":"2019-10-24T23:25:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Firstly, this patch is not about remembering, but about not setting\nanything, so I think that the title should be something like:\n\n commit, tag: set parsed only if no parsing error\n\nIncidentally, the check that you mentioned in PATCH 02 is probably no\nlonger necessary. The tests all pass even with the following diff:\n\ndiff --git a/commit.c b/commit.c\nindex e12e7998ad..086011d944 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -359,7 +359,7 @@ struct tree *repo_get_commit_tree(struct repository *r,\n struct object_id *get_commit_tree_oid(const struct commit *commit)\n {\n struct tree *tree = get_commit_tree(commit);\n- return tree ? &tree->object.oid : NULL;\n+ return &tree->object.oid;\n }\n"},{"id":"384816","messageId":"20191024234111.GC32602@sigill.intra.peff.net","threadId":"52064","inReplyTo":"20191024232546.70565-1-jonathantanmy@google.com","subject":"Re: [PATCH 04/23] remember commit/tag parse failures","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-24T23:41:11Z","receivedAt":"2019-10-24T23:41:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 24, 2019 at 04:25:46PM -0700, Jonathan Tan wrote:\n\n> Firstly, this patch is not about remembering, but about not setting\n> anything, so I think that the title should be something like:\n> \n> commit, tag: set parsed only if no parsing error\n\nTrue. I had also played with actually remembering via a bit, which I\nthink is how I ended up thinking about it that way. You could argue that\nit is \"not forgetting\", which is remembering. :) But I think your\nsuggested title is better.\n\n> Incidentally, the check that you mentioned in PATCH 02 is probably no\n> longer necessary. The tests all pass even with the following diff:\n> \n> diff --git a/commit.c b/commit.c\n> index e12e7998ad..086011d944 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -359,7 +359,7 @@ struct tree *repo_get_commit_tree(struct repository *r,\n> struct object_id *get_commit_tree_oid(const struct commit *commit)\n> {\n> struct tree *tree = get_commit_tree(commit);\n> - return tree ? &tree->object.oid : NULL;\n> + return &tree->object.oid;\n> }\n\nAh, I see the confusion you had earlier. The check I meant in patch 2\n(and here) was the one in write_graph_chunk_data(), which checks for a\nnon-NULL tree even after we just saw a successful parse.\n\nI agree that getting rid of the check in get_commit_tree_oid() is\nunlikely to cause any bugs, but there are still cases where it could\nhelp. Namely if I choose to ignore the parse failure (because I want to\nsee the parts of the commit struct that we did manage to get), then I'd\nlike to be able to ask whether we have a valid tree, like:\n\n oid = get_commit_tree_oid(commit);\n if (!oid)\n\tdo something...\n\nWith the revert you showed above, that's dangerous, because we'd get a\nbogus value like \"8\" (because the oid is offset 8 bytes in the struct\nwhich we've dereferenced as NULL).\n\nYou could obviously use \"get_commit_tree()\" instead, which would let you\ncompare to NULL. But it seemed simpler to leave the extra safety in\nplace.\n\n-Peff\n"},{"id":"384817","messageId":"20191024234931.44192-1-jonathantanmy@google.com","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"Re: [PATCH 0/23] parsing and fsck cleanups","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-24T23:49:31Z","receivedAt":"2019-10-24T23:49:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"I've looked at the rest of the patch set and I think that this set is\nworth taking.\n\n> This a string of refactors that ends up with all of the\n> type-specific fsck functions not getting an object struct at all.\n> My goal there was two-fold:\n> \n> - it makes it harder to introduce weirdness like we saw in\n> \t patches 5-8.\n> \n> - it _could_ make things less awkward for callers like index-pack\n> \t which don't necessarily have object structs. And at the end, we\n> \t basically have an fsck_object() that doesn't need an object\n> \t struct. But index-pack still calls fsck_walk(), which does (and\n> \t which relies on the parsed values to traverse). It's not\n> \t entirely clear to me whether index-pack needs to be doing\n> \t fsck_walk() in the first place, or if it should be relying on\n> \t the usual connectivity check.\n> \n> \t So I'm undecided whether this is worth taking on its own, or if\n> \t trying to avoid object structs in the fsck code is just a\n> \t fool's errand. I do think the result isn't too bad to look at,\n> \t though and there are some minor improvements along the way\n> \t (e.g., patch 17 is able to drop some awkwardness).\n\nIf we can partially avoid object structs in the fsck code, I think\nthat's an improvement too.\n"},{"id":"384838","messageId":"xmqqeez1ecz8.fsf@gitster-ct.c.googlers.com","threadId":"52064","inReplyTo":"20191018044103.GA17625@sigill.intra.peff.net","subject":"Re: [PATCH 0/23] parsing and fsck cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-25T03:11:07Z","receivedAt":"2019-10-25T03:11:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This a string of refactors that ends up with all of the\n> type-specific fsck functions not getting an object struct at all.\n> My goal there was two-fold:\n>\n> - it makes it harder to introduce weirdness like we saw in\n> \t patches 5-8.\n\nYup. I'd see that alone as a reason that makes these worth doing.\n\nAnd it was a pleasant read. Thanks.\n"},{"id":"384839","messageId":"xmqqa79pecec.fsf@gitster-ct.c.googlers.com","threadId":"52064","inReplyTo":"20191024180710.GC12892@sigill.intra.peff.net","subject":"Re: [PATCH 09/23] fsck: unify object-name code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-25T03:23:39Z","receivedAt":"2019-10-25T03:23:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Heh. I originally wrote just \"put\" and \"get\" for brevity, but then\n> worried that wasn't clear enough. Let's just use the full names. We can\n> afford the extra few bytes, and it makes it easier if we later need to\n> grep for them or whatever. Here's a replacement patch (I won't send the\n> whole series just yet, as it seems it hasn't gotten a whole lot of\n> review so far).\n\nIt seems that you sent the original instead X-<.\n\nI'll just replace the original 09/23 with the following squashed in\nfor now.\n\nThanks.\n\n fsck.h | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/fsck.h b/fsck.h\nindex f6f0c40060..6228f0b2d4 100644\n--- a/fsck.h\n+++ b/fsck.h\n@@ -73,13 +73,13 @@ int fsck_finish(struct fsck_options *options);\n * If fsck_enable_object_names() has not been called, all other functions are\n * noops.\n *\n- * Use put_object_name() to seed initial names (e.g. from refnames); the fsck\n- * code will extend that while walking trees, etc.\n+ * Use fsck_put_object_name() to seed initial names (e.g. from refnames); the\n+ * fsck code will extend that while walking trees, etc.\n *\n- * Use get_object_name() to get a single name (or NULL if none). Or the more\n- * convenient describe_object(), which always produces an output string with\n- * the oid combined with the name (if any). Note that the return value points\n- * to a rotating array of static buffers, and may be invalidated by a\n+ * Use fsck_get_object_name() to get a single name (or NULL if none). Or the\n+ * more convenient describe_object(), which always produces an output string\n+ * with the oid combined with the name (if any). Note that the return value\n+ * points to a rotating array of static buffers, and may be invalidated by a\n * subsequent call.\n */\n void fsck_enable_object_names(struct fsck_options *options);\n"},{"id":"384885","messageId":"20191025212020.GA25625@sigill.intra.peff.net","threadId":"52064","inReplyTo":"xmqqa79pecec.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 09/23] fsck: unify object-name code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-25T21:20:20Z","receivedAt":"2019-10-25T21:20:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 25, 2019 at 12:23:39PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Heh. I originally wrote just \"put\" and \"get\" for brevity, but then\n> > worried that wasn't clear enough. Let's just use the full names. We can\n> > afford the extra few bytes, and it makes it easier if we later need to\n> > grep for them or whatever. Here's a replacement patch (I won't send the\n> > whole series just yet, as it seems it hasn't gotten a whole lot of\n> > review so far).\n> \n> It seems that you sent the original instead X-<.\n> \n> I'll just replace the original 09/23 with the following squashed in\n> for now.\n\nDoh. Perhaps you have a brown paper bag I could borrow, as well. :)\n\nWhat you showed matches what I meant to send. If I can trouble you once\nmore on this series, can you swap out the subject for patch 4 (\"remember\ncommit/tag parse failures\", as suggested by Jonathan Tan? Here's a\nreplacement patch (really this time!), but the subject is the only\ndifferent thing:\n\n-- >8 --\nSubject: [PATCH] commit, tag: don't set parsed bit for parse failures\n\nIf we can't parse a commit, then parse_commit() will return an error\ncode. But it _also_ sets the \"parsed\" flag, which tells us not to bother\ntrying to re-parse the object. That means that subsequent parses have no\nidea that the information in the struct may be bogus. I.e., doing this:\n\n parse_commit(commit);\n ...\n if (parse_commit(commit) < 0)\n die(\"commit is broken\");\n\nwill never trigger the die(). The second parse_commit() will see the\n\"parsed\" flag and quietly return success.\n\nThere are two obvious ways to fix this:\n\n 1. Stop setting \"parsed\" until we've successfully parsed.\n\n 2. Keep a second \"corrupt\" flag to indicate that we saw an error (and\n when the parsed flag is set, return 0/-1 depending on the corrupt\n flag).\n\nThis patch does option 1. The obvious downside versus option 2 is that\nwe might continually re-parse a broken object. But in practice,\ncorruption like this is rare, and we typically die() or return an error\nin the caller. So it's OK not to worry about optimizing for corruption.\nAnd it's much simpler: we don't need to use an extra bit in the object\nstruct, and callers which check the \"parsed\" flag don't need to learn\nabout the corrupt bit, too.\n\nThere's no new test here, because this case is already covered in t5318.\nNote that we do need to update the expected message there, because we\nnow detect the problem in the return from \"parse_commit()\", and not with\na separate check for a NULL tree. In fact, we can now ditch that\nexplicit tree check entirely, as we're covered robustly by this change\n(and the previous recent change to treat a NULL tree as a parse error).\n\nWe'll also give tags the same treatment. I don't know offhand of any\ncases where the problem can be triggered (it implies somebody ignoring a\nparse error earlier in the process), but consistently returning an error\nshould cause the least surprise.\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n commit-graph.c | 3 ---\n commit.c | 14 +++++++++++++-\n t/t5318-commit-graph.sh | 2 +-\n tag.c | 12 +++++++++++-\n 4 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex fc4a43b8d6..852b9c39e6 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -855,9 +855,6 @@ static void write_graph_chunk_data(struct hashfile *f, int hash_len,\n \t\t\tdie(_(\"unable to parse commit %s\"),\n \t\t\t\toid_to_hex(&(*list)->object.oid));\n \t\ttree = get_commit_tree_oid(*list);\n-\t\tif (!tree)\n-\t\t\tdie(_(\"unable to get tree for %s\"),\n-\t\t\t\toid_to_hex(&(*list)->object.oid));\n \t\thashwrite(f, tree->hash, hash_len);\n \n \t\tparent = (*list)->parents;\ndiff --git a/commit.c b/commit.c\nindex 810419a168..e12e7998ad 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -405,7 +405,18 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\titem->object.parsed = 1;\n+\n+\tif (item->parents) {\n+\t\t/*\n+\t\t * Presumably this is leftover from an earlier failed parse;\n+\t\t * clear it out in preparation for us re-parsing (we'll hit the\n+\t\t * same error, but that's good, since it lets our caller know\n+\t\t * the result cannot be trusted.\n+\t\t */\n+\t\tfree_commit_list(item->parents);\n+\t\titem->parents = NULL;\n+\t}\n+\n \ttail += size;\n \tif (tail <= bufptr + tree_entry_len + 1 || memcmp(bufptr, \"tree \", 5) ||\n \t\t\tbufptr[tree_entry_len] != '\\n')\n@@ -462,6 +473,7 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b\n \tif (check_graph)\n \t\tload_commit_graph_info(r, item);\n \n+\titem->object.parsed = 1;\n \treturn 0;\n }\n \ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d42b3efe39..127b404856 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -660,7 +660,7 @@ test_expect_success 'corrupt commit-graph write (missing tree)' '\n \t\tgit commit-tree -p \"$broken\" -m \"good\" \"$tree\" >good &&\n \t\ttest_must_fail git commit-graph write --stdin-commits \\\n \t\t\ttest_err &&\n-\t\ttest_i18ngrep \"unable to get tree for\" test_err\n+\t\ttest_i18ngrep \"unable to parse commit\" test_err\n \t)\n '\n \ndiff --git a/tag.c b/tag.c\nindex 6a51efda8d..71b544467e 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -141,7 +141,16 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \n \tif (item->object.parsed)\n \t\treturn 0;\n-\titem->object.parsed = 1;\n+\n+\tif (item->tag) {\n+\t\t/*\n+\t\t * Presumably left over from a previous failed parse;\n+\t\t * clear it out in preparation for re-parsing (we'll probably\n+\t\t * hit the same error, which lets us tell our current caller\n+\t\t * about the problem).\n+\t\t */\n+\t\tFREE_AND_NULL(item->tag);\n+\t}\n \n \tif (size < the_hash_algo->hexsz + 24)\n \t\treturn -1;\n@@ -192,6 +201,7 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \telse\n \t\titem->date = 0;\n \n+\titem->object.parsed = 1;\n \treturn 0;\n }\n \n-- \n2.24.0.rc1.670.g28e7e9fef7\n\n"}]}