[PATCH v5 0/6] object-name: make ambiguous object output translatable + show tag date

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@gmail.com

Æ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@gmail.com>
     
      ## 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