On Mon, Aug 18, 2025 at 09:56:35PM +0200, René Scharfe wrote:

quoted

  1. What should happen when traversing from HEAD does not find the blob
     in question? Right now we print a blank line, which is...weird.

Weird indeed.

quoted

     Probably we should either print nothing, or return an error.

The latter, consistent with "git describe <commit-ish>".

Certainly if we were starting from scratch, that's my thought. I just
wondered if we were stuck with the behavior for historical reasons.

quoted

If we return an error, should we respect --always?

The documentation says "git describe <blob>" takes no options.  It could
learn some, of course.  But does it have to?  Perhaps better keep that
thing contained.

OK. It is easy to just treat --always like we do in the commit case, but
I'm not sure it's actually useful. Just bailing early to say the option
is incompatible is reasonable.

quoted

  2. When we are on an unborn branch, we print a confusing message:

       $ git init
       $ git commit --allow-empty -m foo
       $ git tag foo
       $ git symbolic-ref HEAD refs/heads/unborn
       $ git describe $(echo blob | git hash-object -w --stdin)
       fatal: ambiguous argument 'HEAD': unknown revision or path not in the working tree.
       Use '--' to separate paths from revisions, like this:
       'git <command> [<revision>...] -- [<file>...]'

     We should probably resolve HEAD ourselves and either bail with an
     empty output or an error (depending on what we do for (1) above).

It already is an error, just needs a better message.  It should still
report an error even if we were to stick with showing blank lines for
unrelated blobs.

I think an error here is OK. But if we quietly return no output for (1),
then I think this should do the same. If we return an error for (1),
then yeah, it should remain one here and just get a better message.

quoted

  3. When we do traverse, if process_object() sees that we didn't find a
     commit, we should detect that and either return an empty result or
     an error (again, depending on the behavior of (2) above). This is
     done by checking is_null_oid(&pcd->current_commit) there.

OK, ending the search right there might be the best option.  Traversing
deeper into the forest that we then know to be cursed would be the
unappealing alternative.

Right. There is no real "deeper" because we know we will never find a
commit.

  5. When process_object() has a commit, but it is indescribable, it
     shows an error:

     $ git describe 5afbe6da1d6ab0b8939343166636b828e561bf35
     fatal: No tags can describe '3b681e255cd8577a77983958ef7f566b05806cd0'.
     Try --always, or create some tags.

     It's not immediately clear that the reported hash belongs to the
     found commit.  And that suggestion to try --always is misleading,
     as "git describe <blob>" takes no options according to the
     documentation.  I'm not sure I like it in general -- can't tell
     if the command is being snarky with me.

Yeah, describe_commit()'s messages are not really set up to handle
describing an arbitrary commit that the user did not specify.

quoted

So it all depends on what to do with (1), and for a feature that IMHO
should not even exist in the first place, I had trouble summoning the
will-power to make this 4-patch series.

644eb60bd0 (builtin/describe.c: describe a blob, 2017-11-15) and
15af58c1ad (diffcore: add a pickaxe option to find a specific blob,
2018-01-04) confuse me; the latter's commit message sounds like the
former wasn't (supposed to be?) merged.

I think the issues you listed are independent, though.  Or what's wrong
with this demo that addresses point 3 in process_object() and 1 in
describe_blob().  If we want a blank line for 1 then we apply only
the first hunk.  Or am I missing something?

Yeah, I came up with a similar patch. The question was more of what the
behavior _should_ be, and whether we wanted to align that with an unborn
HEAD. And of course tests for all of these cases.

I'll send out a few patches in a moment (I guess I summoned some
willpower in the interim).

-Peff