This topic improves the output we emit on ambiguous objects as noted in 4/6, and makes it translatable, see 3/6. See [1] for v4. This addresses the feedback Jeff King had on v4. There weren't any tests for cases where we'd return -1 when parsing objects, and I was focused on different object types in earlier iterations, and missed that case. So this v5 leads with some exhaustive testing of the existing functionality to address that and other blind spots, I then resurrected the patch from an earlier iteration to buffer the output for a single advice() call at the end. As the exhaustive tests that we have now show if we call error() (which can and will happen several times on invalid objects) while parsing our N objects, we'll split up the header and body for the advice(), by buffering it up we're guaranteed to print errors and the payload separately. 1. https://lore.kernel.org/git/cover-v4-0.3-00000000000-20211122T175219Z-avarab@xxxxxxxxx Ævar Arnfjörð Bjarmason (6): object-name tests: add tests for ambiguous object blind spots object-name: explicitly handle OBJ_BAD in show_ambiguous_object() object-name: make ambiguous object output translatable object-name: show date for ambiguous tag objects object-name: iterate ambiguous objects before showing header object-name: re-use "struct strbuf" in show_ambiguous_object() object-name.c | 111 +++++++++++++++++++++++++--- t/t1512-rev-parse-disambiguation.sh | 83 +++++++++++++++++++++ 2 files changed, 182 insertions(+), 12 deletions(-) Range-diff against v4: -: ----------- > 1: 767165d096d object-name tests: add tests for ambiguous object blind spots 1: 2e7090c09f9 ! 2: ee86912f1c1 object-name: remove unreachable "unknown type" handling @@ Metadata Author: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## Commit message ## - object-name: remove unreachable "unknown type" handling + object-name: explicitly handle OBJ_BAD in show_ambiguous_object() - Remove unreachable "unknown type" handling in the code that displays - the ambiguous object list. See [1] for the current output, and [1] for - the commit that added the "unknown type" handling. + Amend the "unknown type" handling in the code that displays the + ambiguous object list to assert() that we're either going to get the + "real" object types we can pass to type_name(), or a -1 (OBJ_BAD) + return value from oid_object_info(). - The reason this code wasn't reachable is because we're not passing in - OBJECT_INFO_ALLOW_UNKNOWN_TYPE, so we'll die in sort_ambiguous() - before we get to show_ambiguous_object(): + See [1] for the current output, and [1] for the commit that added the + "unknown type" handling. - $ git rev-parse 8315 - error: short object ID 8315 is ambiguous - hint: The candidates are: - fatal: invalid object type + We are never going to get an "unknown type" in the sense of custom + types crafted with "hash-object --literally", since we're not using + the OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag. - We should do better here, but let's leave that for some future - improvement. In a subsequent commit I'll improve the output we do - show, and not having to handle the "unknown type" case simplifies that - change. + If we manage to otherwise unpack such an object without errors we'll + die() in parse_loose_header_extended() called by sort_ambiguous() + before we get to show_ambiguous_object(), as is asserted by the test + added in the preceding commit. - Even though we know that this isn't reachable let's back that up with - an assert() both for self-documentation and sanity checking. + So saying "unknown type" here was always misleading, we really meant + to say that we had a failure parsing the object at all, if the problem + is only that it's type is unknown we won't reach this code. + + So let's emit a generic "[bad object]" instead. As our tests added in + the preceding commit show, we'll have emitted various "error" output + already in those cases. + + We should do better in the truly "unknown type" cases, which we'd need + to handle if we were passing down the OBJECT_INFO_ALLOW_UNKNOWN_TYPE + flag. But let's leave that for some future improvement. In a + subsequent commit I'll improve the output we do show, and not having + to handle the "unknown type" (as in OBJECT_INFO_ALLOW_UNKNOWN_TYPE) + simplifies that change. 1. 5cc044e0257 (get_short_oid: sort ambiguous objects by type, then SHA-1, 2018-05-10) @@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi return 0; type = oid_object_info(ds->repo, oid, NULL); ++ ++ if (type < 0) { ++ strbuf_addstr(&desc, "[bad object]"); ++ goto out; ++ } ++ + assert(type == OBJ_TREE || type == OBJ_COMMIT || + type == OBJ_BLOB || type == OBJ_TAG); ++ strbuf_addstr(&desc, type_name(type)); ++ if (type == OBJ_COMMIT) { struct commit *commit = lookup_commit(ds->repo, oid); if (commit) { @@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data) + strbuf_addf(&desc, " %s", tag->tag); + } - advise(" %s %s%s", +- advise(" %s %s%s", ++out: ++ advise(" %s %s", repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV), - type_name(type) ? type_name(type) : "unknown type", -- desc.buf); -+ type_name(type), desc.buf); + desc.buf); strbuf_release(&desc); - return 0; + + ## t/t1512-rev-parse-disambiguation.sh ## +@@ t/t1512-rev-parse-disambiguation.sh: test_expect_success POSIXPERM 'ambigous zlib corrupt loose blob' ' + error: unable to unpack cafe... header + error: inflate: data stream error (incorrect header check) + error: unable to unpack cafe... header +- hint: cafe... unknown type ++ hint: cafe... [bad object] + hint: cafe... blob + fatal: ambiguous argument '\''cafe...'\'': unknown revision or path not in the working tree. + Use '\''--'\'' to separate paths from revisions, like this: 2: 00d84faeb1d ! 3: b79964483e8 object-name: make ambiguous object output translatable @@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi if (ds->fn && !ds->fn(ds->repo, oid, ds->cb_data)) return 0; -@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data) + ++ hash = repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV); type = oid_object_info(ds->repo, oid, NULL); + + if (type < 0) { +- strbuf_addstr(&desc, "[bad object]"); ++ /* ++ * TRANSLATORS: This is a line of ambiguous object ++ * output shown when we cannot look up or parse the ++ * object in question. E.g. "deadbeef [bad object]". ++ */ ++ strbuf_addf(&desc, _("%s [bad object]"), hash); + goto out; + } + assert(type == OBJ_TREE || type == OBJ_COMMIT || type == OBJ_BLOB || type == OBJ_TAG); -+ hash = repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV); -+ +- strbuf_addstr(&desc, type_name(type)); + if (type == OBJ_COMMIT) { + struct strbuf ad = STRBUF_INIT; + struct strbuf s = STRBUF_INIT; @@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi + * object output. E.g. "deadbeef blob". + */ + strbuf_addf(&desc, _("%s blob"), hash); -+ } else { -+ BUG("unreachable"); } -- advise(" %s %s%s", ++ + out: +- advise(" %s %s", - repo_find_unique_abbrev(ds->repo, oid, DEFAULT_ABBREV), -- type_name(type), desc.buf); +- desc.buf); + /* -+ * TRANSLATORS: This is line item of ambiguous object output, -+ * translated above. ++ * TRANSLATORS: This is line item of ambiguous object output ++ * from describe_ambiguous_object() above. + */ + advise(_(" %s"), desc.buf); 3: 9d24bab635d ! 4: 36b6b440c37 object-name: show date for ambiguous tag objects @@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, voi /* * TRANSLATORS: This is a line of -@@ object-name.c: static int show_ambiguous_object(const struct object_id *oid, void *data) + * ambiguous tag object output. E.g.: + * +- * "deadbeef tag Some Tag Message" ++ * "deadbeef tag 2021-01-01 - Some Tag Message" + * + * The second argument is the "tag" string from * object.c, it should (hopefully) already be * translated. */ -: ----------- > 5: 8880c283559 object-name: iterate ambiguous objects before showing header -: ----------- > 6: 78bb0995f08 object-name: re-use "struct strbuf" in show_ambiguous_object() -- 2.34.1.838.g779e9098efb
