On Mon, Aug 18, 2025 at 09:56:35PM +0200, René Scharfe wrote: > > 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. > > > 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. > > 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. > > 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. > > 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. > > 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
