git-describe <blob> ?

This will be useful shortly.

Hi Stefan,

Quoted 1 line · Stefan Beller

This will be useful shortly.

Something tells me that I will hate you in the future when I read this commit message and lack the context (e.g. when blaming, where I cannot see the child commits let alone the comment in the Merge commit).

How about:

The functionality to list tree objects in the order they were seen while traversing the commits will be used in the next commit, where we teach git describe to describe not only commits, but trees and blobs, too.

(Not making excuses, but...) I remembered some other senior members having such commit messages, so I felt like I had to step up my game.

https://public-inbox.org/git/70fbcd573f5c8a78a19a08ffc255437c36e7f49d.1495014840.git.mhagger@alum.mit.edu/

Quoted 5 lines · Stefan Beller

(Not making excuses, but...) I remembered some other senior members having such commit messages, so I felt like I had to step up my game.

https://public-inbox.org/git/70fbcd573f5c8a78a19a08ffc255437c36e7f49d.1495014840.git.mhagger@alum.mit.edu/

I forgot to note though, that this never made it into the official tree, though.

Sometimes users are given a hash of an object and they want to identify it further (ex.: Use verify-pack to find the largest blobs, but what are these? or [1])

The best identification of a blob hash is done via a its path at a given commit, which this implements.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob

Hi Stefan,

Nicely done, sir!

I wonder whether it would make sense to extend this to tree objects while we are at it, but maybe that's an easy up-for-grabs.

Thank you very much!
Dscho

Hi Stefan,

Quoted 7 lines · Stefan Beller

Sometimes users are given a hash of an object and they want to identify it further (ex.: Use verify-pack to find the largest blobs, but what are these? or [1])

The best identification of a blob hash is done via a its path at a given commit, which this implements.

Quoted 1 line

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob

I also came up with a script to do that:
https://github.com/msysgit/msysgit/blob/master/bin/what-made-this-repo-so-large.sh

Your method is much more elegant, of course (it describes the commit in the same run as it finds the object, and it does not output tons of stuff only to be filtered).

Quoted 5 lines

builtin/describe.c +0 -0

@@ -282,6 +283,50 @@ static void show_suffix(int depth, const struct object_id *oid)
      printf("-%d-g%s", depth, find_unique_abbrev(oid->hash, abbrev));
 }
 

builtin/describe.c static void show_suffix(int depth, const struct object_id *oid) · line 289

Quoted 4 lines

builtin/describe.c +4 -0

+struct blob_descriptor {
+     struct object_id current_commit;
+     struct object_id looking_for;
+};

Personally, I would call this process_commit_data, but I do not mind too much about the name.

builtin/describe.c static void show_suffix(int depth, const struct object_id *oid) · line 306

Quoted 9 lines

builtin/describe.c +9 -0

+static void process_object(struct object *obj, const char *name, void *data)
+{
+     struct blob_descriptor *bd = data;
+
+     if (!oidcmp(&bd->looking_for, &obj->oid))
+             printf(_("blob %s present at path %s in commit %s\n"),
+                     oid_to_hex(&bd->looking_for), name,
+                     oid_to_hex(&bd->current_commit));
+}

s/name/path/

builtin/describe.c static void describe(const char *arg, int last_one) · line 348

Quoted 13 lines

builtin/describe.c +6 -3

@@ -295,9 +340,14 @@ static void describe(const char *arg, int last_one)
 
      if (get_oid(arg, &oid))
              die(_("Not a valid object name %s"), arg);
-     cmit = lookup_commit_reference(&oid);
-     if (!cmit)
-             die(_("%s is not a valid '%s' object"), arg, commit_type);
+     cmit = lookup_commit_reference_gently(&oid, 1);
+     if (!cmit) {
+             if (lookup_blob(&oid))
+                     describe_blob(oid);
+             else
+                     die(_("%s is not a commit nor blob"), arg);

s/not/neither/

Nicely done, sir!

I wonder whether it would make sense to extend this to tree objects while we are at it, but maybe that's an easy up-for-grabs.

Quoted 4 lines · Johannes Schindelin

[..]
I wonder whether it would make sense to extend this to tree objects while we are at it, but maybe that's an easy up-for-grabs.

I can look into incorporating that, too. What is the use case though? (Is there any error message, common enough that users want to identify trees?)

Thanks for the review,
Stefan

Quoted 2 lines · Johannes Schindelin

I wonder whether it would make sense to extend this to tree objects while we are at it, but maybe that's an easy up-for-grabs.

I can look into incorporating that, too. What is the use case though? (Is there any error message, common enough that users want to identify trees?)

With traverse_trees_and_blobs factored out of the main traverse function, the next patch can introduce an in-order revision walking with ease.

list-objects.c +0 -0

index b3931fa434..0ee0551604 100644

Hi Stefan,

[I was intrigued enough by your work to postpone to later this coming week reading the What's cooking email in favor of reviewing your patch series.]

Quoted 2 lines · Stefan Beller

With traverse_trees_and_blobs factored out of the main traverse function, the next patch can introduce an in-order revision walking with ease.

Makes sense.

Quoted 8 lines

list-objects.c +0 -0

index b3931fa434..0ee0551604 100644
@@ -183,25 +183,13 @@ static void add_pending_tree(struct rev_info *revs, struct tree *tree)
      add_pending_object(revs, &tree->object, "");
 }
 

list-objects.c static void add_pending_tree(struct rev_info *revs, struct tree *tree) · line 187

Quoted 6 lines

list-objects.c +2 -4

-void traverse_commit_list(struct rev_info *revs,
-                       show_commit_fn show_commit,
-                       show_object_fn show_object,
-                       void *data)
+static void traverse_trees_and_blobs(struct rev_info *revs,
+                                  struct strbuf *base,

In the context of one function, it was obvious what base meant. Maybe we can call it base_path now?

Thanks,
Dscho

Occasionally a user is given an object hash from a blob as an error message or other output (e.g. [1]).

It would be useful to get a further description of such a blob, such as the (commit, path) tuple where this blob was introduced.

This implements the answer in builtin/describe, but I am not sure if that is the right place. (One office mate argued it could be a "reverse-blame" that tells you all the trees/commits where the blob is referenced from).

This is RFC for other reasons as well: tests, docs missing.

Any feedback welcome,

With traverse_trees_and_blobs factored out of the main traverse function, the next patch can introduce an in-order revision walking with ease.

The variable holding the base path is only used in the newly factored out function traverse_trees_and_blobs, however we keep its scope to traverse_commit_list to keep the number of invocations for memory allocations and release to one per commit traversal.

Rename the variable to base_path for clarity.

t/t6100-rev-list-in-order.sh new file +9 -0

new file mode 100755
index 0000000000..67ebe815d2
@@ -0,0 +1,44 @@
+#!/bin/sh
+
+test_description='miscellaneous rev-list tests'
+
+. ./test-lib.sh
+
+
+test_expect_success 'setup' '
+	for x in one two three four

t/t6100-rev-list-in-order.sh new file +4 -0

+	test_cmp expect actual
+'
+
+test_done

list-objects.c void traverse_commit_list(struct rev_info *revs, · line 243

Quoted 12 lines · Stefan Beller

list-objects.c +2 -0

index bf46f80dff..5e114c9a8a 100644
@@ -237,6 +237,8 @@ void traverse_commit_list(struct rev_info *revs,
              if (commit->tree)
                      add_pending_tree(revs, commit->tree);
              show_commit(commit, data);
+             if (revs->tree_blobs_in_commit_order)
+                     traverse_trees_and_blobs(revs, &base_path, show_object, data);
      }
      traverse_trees_and_blobs(revs, &base_path, show_object, data);

This one is interesting. While we walk the ancestry chain, we may add the tree for each commit that we discover to the "pending. We used to sweep the pending array at the end after we run out of the commits, but now we do the sweeping as we process each commit.

Would this make the last call to traverse_trees_and_blobs() always a no-op?

I am not suggesting to add a new conditional in front of it;
I am just asking for curiosity's sake.

list-objects.c void traverse_commit_list(struct rev_info *revs,

Quoted 12 lines

list-objects.c +12 -0

+test_expect_success 'setup' '
+     for x in one two three four
+     do
+             echo $x >$x &&
+             git add $x &&
+             git commit -m "add file $x"
+     done &&
+     for x in four three
+     do
+             git rm $x
+             git commit -m "remove $x"
+     done &&

There is break in &&-chain in the second loop, but in any case, for loops and &&-chains do not mix very well unless done carefully. Imagine what happens if "git commit" in the first loop failed when x is two; it won't stop and keep going to create three and four and nobody would noice any error.

Even though we do not have --stdin for rev-parse, you can still do:

        git cat-file --batch-check='%(objectname)' >expect <<-\EOF &&
        HEAD^{commit}
        HEAD^{tree}
        HEAD:one
        HEAD:two
        ...
        EOF

The function describe has already a variable named oid declared at the beginning of the function for an object id. Do now shadow that

variable with a pointer to an object id.

Signed-off-by Stefan Beller <[email protected]>

---
builtin/describe.c | 8 ++++----

1 file changed, 4 insertions(+), 4 deletions(-)

builtin/describe.c +4 -4

index 29075dbd0f..fd61f463cf 100644
@@ -381,9 +381,9 @@ static void describe(const char *arg, int last_one)
 	}
 
 	if (!match_cnt) {
-		struct object_id *oid = &cmit->object.oid;
+		struct object_id *cmit_oid = &cmit->object.oid;
 		if (always) {
-			printf("%s", find_unique_abbrev(oid->hash, abbrev));
+			printf("%s", find_unique_abbrev(cmit_oid->hash, abbrev));
 			if (suffix)
 				printf("%s", suffix);
 			printf("\n");

@@ -392,11 +392,11 @@ static void describe(const char *arg, int
 last_one)
 		if (unannotated_cnt)
 			die(_("No annotated tags can describe '%s'.\n"
 			    "However, there were unannotated tags: try --tags."),
-			    oid_to_hex(oid));
+			    oid_to_hex(cmit_oid));
 		else
 			die(_("No tags can describe '%s'.\n"
 			    "Try --always, or create some tags."),
-			    oid_to_hex(oid));
+			    oid_to_hex(cmit_oid));
 	}
 

	QSORT(all_matches, match_cnt, compare_pt);

For debuggers aid we'd want to print debug statements early, so introduce a new line in the debug output that describes the whole function, and the change the next debug output to describe why we need to search. Conveniently drop the arg from the second line; which will be useful in a follow up commit, that refactors the describe function.

Signed-off-by Stefan Beller <[email protected]>

---

 builtin/describe.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

builtin/describe.c +4 -1

index fd61f463cf..3136efde31 100644
@@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)
 	unsigned long seen_commits = 0;
 	unsigned int unannotated_cnt = 0;
 
+	if (debug)
+		fprintf(stderr, _("describe %s\n"), arg);
+
 	if (get_oid(arg, &oid))
 		die(_("Not a valid object name %s"), arg);
 	cmit = lookup_commit_reference(&oid);

@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)
 	if (!max_candidates)
 		die(_("no tag exactly matches '%s'"), oid_to_hex(&cmit->object.oid));
 	if (debug)
-		fprintf(stderr, _("searching to describe %s\n"), arg);
+		fprintf(stderr, _("No exact match on refs or tags, searching to describe\n"));
 
 	if (!have_util) {
 		struct hashmap_iter iter;

Quoted 6 lines · Stefan Beller

For debuggers aid we'd want to print debug statements early, so introduce a new line in the debug output that describes the whole function, and the change the next debug output to describe why we need to search. Conveniently drop the arg from the second line; which will be useful in a follow up commit, that refactors the describe function.

Quoted 2 lines

Signed-off-by Stefan Beller <[email protected]>

---

Adding an eary "entry" log may indeed be a good idea. This is not a new issue, but I do not think it is a good trade-off to burden i18n teams to translate debug-only messages, though.

Quoted 27 lines

 builtin/describe.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

builtin/describe.c +4 -1

index fd61f463cf..3136efde31 100644
@@ -293,6 +293,9 @@ static void describe(const char *arg, int last_one)
      unsigned long seen_commits = 0;
      unsigned int unannotated_cnt = 0;
 
+     if (debug)
+             fprintf(stderr, _("describe %s\n"), arg);
+
      if (get_oid(arg, &oid))
              die(_("Not a valid object name %s"), arg);
      cmit = lookup_commit_reference(&oid);

@@ -316,7 +319,7 @@ static void describe(const char *arg, int last_one)
      if (!max_candidates)
              die(_("no tag exactly matches '%s'"), oid_to_hex(&cmit->object.oid));
      if (debug)
-             fprintf(stderr, _("searching to describe %s\n"), arg);
+             fprintf(stderr, _("No exact match on refs or tags, searching to describe\n"));
 
      if (!have_util) {
              struct hashmap_iter iter;

Quoted 6 lines · Stefan Beller

Documentation/git-describe.txt +0 -0

index c924c945ba..3d618b2445 100644
@@ -3,7 +3,7 @@ git-describe(1)
 

Documentation/git-describe.txt git-describe(1) · line 6

Quoted 4 lines

Documentation/git-describe.txt +1 -1

 NAME
 ----
-git-describe - Describe a commit using the most recent tag reachable from it
+git-describe - Describe a commit or blob using the most recent tag reachable from it

If I am not mistaken, this series is about describing a blob as a tuple of a recent commit-ish and a path in the tree in it. Blob never reaches anything, so "desribing blob using X reachable from it" is a mere nonsense.

The original is not great in that it ignores the "--contains" mode and referring to "tagged commit" merely as "tag" for brevity, but at least it made some sense.

Documentation/git-describe.txt By default (without --all or --tags) `git describe` only shows · line 27

Quoted 5 lines

Documentation/git-describe.txt +1 -0

@@ -24,6 +24,16 @@ By default (without --all or --tags) `git describe` only shows
 annotated tags.  For more information about creating annotated tags
 see the -a and -s options to linkgit:git-tag[1].
 
+If the given `<commit-ish>` refers to a blob, it will be described

Perhaps this step should update the SYNOPSIS so that the command takes not just commit-ish but a blob too.

Given the difficulty in
coming up with the single-liner description of what it does we saw above, I suspect that splitting SYNOPSIS out into two very distinct operating mode might make it easier to read.

    SYNOPSIS
    --------
    [verse]
    'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] [<commit-ish>...]

+'git describe' [<options>...] <blob>...

Then this additional paragraph can say "When describin a <blob>", without using a (technically nonsense) phrase "if <commit-ish> refers to a blob", which is never true.

Quoted 11 lines

Given the difficulty in
coming up with the single-liner description of what it does we saw above, I suspect that splitting SYNOPSIS out into two very distinct operating mode might make it easier to read.

    SYNOPSIS
    --------
    [verse]
    'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] [<commit-ish>...]

+'git describe' [<options>...] <blob>...

Quoted 3 lines

Then this additional paragraph can say "When describin a <blob>", without using a (technically nonsense) phrase "if <commit-ish> refers to a blob", which is never true.

ok, do we know about 'blob-ish' as a term?

Quoted 11 lines · Stefan Beller

Quoted 11 lines

Given the difficulty in
coming up with the single-liner description of what it does we saw above, I suspect that splitting SYNOPSIS out into two very distinct operating mode might make it easier to read.

    SYNOPSIS
    --------
    [verse]
    'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] [<commit-ish>...]

+'git describe' [<options>...] <blob>...

Quoted 5 lines

Quoted 3 lines

Then this additional paragraph can say "When describin a <blob>", without using a (technically nonsense) phrase "if <commit-ish> refers to a blob", which is never true.

ok, do we know about 'blob-ish' as a term?

No, and I do not think there is any need to say -ish at all for this use case.

After all, when we accept a <commit> when a <tree-ish> is called for, that is only because there is only one way to use the commit in place of the wanted <tree>; we take the top-level tree contained in it. You cannot say you take <blob-ish> and take a <tree>, as it is unclear which entry in the <tree> can act as the substitute for the wanted <blob>.

But now we have a path as well, the notation of
<commit-ish> COLON <path>
is not a unique description of the blob, because
* there can be multiple <commit-ish>s depending on the tags and walking
* in boilerplate code cases, we might even have the blob at different places (e.g. pristine copies of a license file in subdirectories)

When calling for a tree-ish, we also accept commits and tags plus walking directions.

The reason why we say "-ish" is "Yes we know v2.15.0 is *NOT* a commit object, we very well know it is a tag object, but because we allow it to be used in a context that calls for a commit object, we mark that use context as 'this accepts commit-ish, not just commit'".

Quoted 5 lines · Junio C Hamano

The reason why we say "-ish" is "Yes we know v2.15.0 is *NOT* a commit object, we very well know it is a tag object, but because we allow it to be used in a context that calls for a commit object, we mark that use context as 'this accepts commit-ish, not just commit'".

Having said all that, there is a valid case in which we might want to say "blob-ish".

To review, X-ish is the word we use when the command wants to take an X, but tolerates a lazy user who gives a Y, which is *NOT* X, without bothering to add ^{X} suffix, i.e. Y^{X}. In such a case, the command takes not just X but takes X-ish because it takes a Y and converts it internally to an X to be extra nice.

When the command wants to take a blob, but tolerates something else and does "^{blob}" internally, we can say it takes "blob-ish". Technically that "something else" could be an annotated tag that points at a blob object, without any intervening commit or tree (I did not check if the "describe <blob>" code in this thread handles this, though).

But because it is not usually done to tag a blob directly, it would probably be not worth to say "blob-ish" in the document and cause readers to wonder in what situation something that is not a blob can be treated as if it were a blob. It does feel like we would be pursuing technical correctness too much and sacrificing the readability of the document, at least to me, and a bad trade-off.

Is this not also an alternative case, relative to the user, for the scenario where the user has an oid/sha1 value but does not know what it is, and would like to find its source and type relative to the describe command.

IIUC the existing describe command only accepts <commit-ish> values, and here we are extending that to be even more inclusive, but at the same time the options become more restricted.

Or have I misunderstood how the fast commit search and the slower potentially-a-blob searching are disambiguated?

Quoted 4 lines · Philip Oakley

Is this not also an alternative case, relative to the user, for the scenario where the user has an oid/sha1 value but does not know what it is, and would like to find its source and type relative to the describe command.

I am not sure what you wanted to say with "source and type RELATIVE TO the describe command".

The first thing the combination of the user and the describe command would do when the user has a 40-hex string would be to do the equivalent of "cat-file -t" to learn if it even exists and what its type is. With Stefan's patch, that is what describe command does in order to choose quite a different codeflow from the traditional mode when it learns that it was given a blob.

Quoted 4 lines

IIUC the existing describe command only accepts <commit-ish> values, and here we are extending that to be even more inclusive, but at the same time the options become more restricted.

Do you mean that the command should check if it was given an option that would not be applicable to the "find a commit that has the blob" mode, once it learns that it was given a blob and needs to go in that codepath? I think that would make sense.

Quoted 3 lines

Or have I misunderstood how the fast commit search and the slower potentially-a-blob searching are disambiguated?

I do not think so. We used to barf when we got anything but commit-ish, but Stefan's new code kicks in if the object turns out to be a blob---I think that is what you mean by the disambiguation.

Rereading this discussion, there is currently no urgent thing to address?

Then the state as announced by the last cooking email, to just cook it, seems about right and we'll wait for further feedback.

From: "Stefan Beller" <[email protected]>

Quoted 1 line

Rereading this discussion, there is currently no urgent thing to address?

True.

Quoted 3 lines

Then the state as announced by the last cooking email, to just cook it, seems
about right and we'll wait for further feedback.

Possibly only checking the documenation aspects, so folks don't fall into the same trap as me.. ;-)

Sometimes users are given a hash of an object and they want to identify it further (ex.: Use verify-pack to find the largest blobs, but what are these? or [1])

builtin/describe.c +0 -0

index 9e9a5ed5d4..acfd853a30 100644
...
 static void describe(const char *arg, int last_one)
 {
...

@@ -445,11 +497,18 @@ static void describe(const char *arg, int last_one)
...

Quoted 3 lines · Stefan Beller

Sometimes users are given a hash of an object and they want to identify it further (ex.: Use verify-pack to find the largest blobs, but what are these? or [1])

Thanks for finishing the topic you started.

Documentation/git-describe.txt SYNOPSIS · line 14

Quoted 5 lines

Documentation/git-describe.txt +1 -0

@@ -11,6 +11,7 @@ SYNOPSIS
 [verse]
 'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] [<commit-ish>...]
 'git describe' [--all] [--tags] [--contains] [--abbrev=<n>] --dirty[=<mark>]
+'git describe' <blob>

OK.

Quoted 10 lines

builtin/describe.c +0 -0

index 9e9a5ed5d4..acfd853a30 100644
...
 static void describe(const char *arg, int last_one)
 {
...

@@ -445,11 +497,18 @@ static void describe(const char *arg, int last_one)
...

builtin/describe.c static void describe(const char *arg, int last_one) · line 509

Quoted 10 lines

builtin/describe.c +10 -0

+     cmit = lookup_commit_reference_gently(&oid, 1);
+
+     if (cmit)
+             describe_commit(&oid, &sb);
+     else if (lookup_blob(&oid)) {
+             if (all || tags || longformat || first_parent ||
+                 patterns.nr || exclude_patterns.nr ||
+                 always || dirty || broken)
+                     die(_("options not available for describing blobs"));
+             describe_blob(oid, &sb);

I am not sure if I agree with some of them.

builtin/describe.c static void describe(const char *arg, int last_one) · line 511

Quoted 2 lines

builtin/describe.c +2 -0

+     } else
+             die(_("%s is neither a commit nor blob"), arg);

This side I would agree with, though.

The caller of the describe() function is either

* 'git describe<RETURN>' makes a single call to it with "HEAD" and exits; or
* 'git describe A B C...' makes one call to it for each of these command line arguments.

And 'git describe <blob>' code is most likely trigger from the latter, as it is not likely for HEAD to be pointing at a blob.

    $ blob=$(git rev-parse master:Makefile)
    $ git describe --always master $blob

and trigger the above check. Is the check against "always" useful, or is it better to simply ignore it while describing $blob, but still keeping it in effect while describing 'master'?

The 'dirty' and 'broken' check is redundant because we would have already errored out if either of them is set before calling describe() on user-supplied object names.

If I understand the way "describe <blob>" works correctly, it traverses the history with objects, doing a moral equivalent of "rev-list --objects", stops when it finds the blob object with the given name, and when it stops, it knows the commit object that contains the blob and path in that commit to the blob. Then the commit is described to be a human-readable string, so that the path can be concatenated after it.

Aren't these options that affect how a commit object is described in effect and useful when you do the "internal" describe of the commit you found the blob object in? IOW, wouldn't this

    $ git describe --exclude='*-wip' $blob

make sure that in its output $commit:$path, $commit is not described in terms of any "wip" refs?

Quoted 8 lines · Philip Oakley

From: "Stefan Beller" <[email protected]>

Quoted 1 line

Rereading this discussion, there is currently no urgent thing to address?

True.

Quoted 3 lines

Then the state as announced by the last cooking email, to just cook it, seems
about right and we'll wait for further feedback.

A shiny new toy that is not a fix for a grave bug is rarely urgent, so with that criterion, we'd end up with hundreds of topics not in 'next' but in 'pu' waiting for the original contributor to get out of his or her procrastination, which certainly is not what I want to see, as I'd have to throw them into the Stalled bin and then eventually discard them, while having to worry about possible mismerges with remaining good topics caused by these topics appearing and disappearing from 'pu'.

I'd rather see any topic that consumed reviewers' time to be polished enough to get into 'next' while we all recall the issues raised during previous reviews. I consider the process to further incrementally polish it after that happens a true "cooking".

For this topic, aside from "known issues" that we decided to punt for now, my impression was that the code is in good enough shape, and we need a bit of documentation polishes before I can mark it as "Will merge to 'next'".

Quoted 2 lines

Possibly only checking the documenation aspects, so folks don't fall into the same trap as me.. ;-)

Yup, so let's resolve that documentation thing while we remember that the topic has that issue, and what part of the documentation we find needs improvement.

I am not sure what "trap: you fell into, though. Are you saying that giving

git describe [<option to describe a commit>...] <commit-ish> git describe [<option to describe a blob>...] <blob>

in the synopsis is not helpful, because the user may not know what kind of object s/he has, and cannot decide from which set of options to pick? Then an alternative would be to list

git describe [<option>...] <object>

in the synopsis, say upfront that most options are applicable only when describing a commit-ish, and when describing a blob, we do quite different thing and a separate set of options apply, perhaps?

From: "Junio C Hamano" <[email protected]>
: Friday, November 10, 2017 1:24 AM
[catch up]

Quoted 40 lines

Quoted 8 lines · Philip Oakley

From: "Stefan Beller" <[email protected]>

Quoted 1 line

Rereading this discussion, there is currently no urgent thing to address?

True.

Quoted 3 lines

Then the state as announced by the last cooking email, to just cook it, seems
about right and we'll wait for further feedback.

A shiny new toy that is not a fix for a grave bug is rarely urgent, so with that criterion, we'd end up with hundreds of topics not in 'next' but in 'pu' waiting for the original contributor to get out of his or her procrastination, which certainly is not what I want to see, as I'd have to throw them into the Stalled bin and then eventually discard them, while having to worry about possible mismerges with remaining good topics caused by these topics appearing and disappearing from 'pu'.

I'd rather see any topic that consumed reviewers' time to be polished enough to get into 'next' while we all recall the issues raised during previous reviews. I consider the process to further incrementally polish it after that happens a true "cooking".

For this topic, aside from "known issues" that we decided to punt for now, my impression was that the code is in good enough shape, and we need a bit of documentation polishes before I can mark it as "Will merge to 'next'".

Quoted 2 lines

Possibly only checking the documenation aspects, so folks don't fall into the same trap as me.. ;-)

Yup, so let's resolve that documentation thing while we remember that the topic has that issue, and what part of the documentation we find needs improvement.

I am not sure what "trap: you fell into, though. Are you saying that giving

Quoted 6 lines

git describe [<option to describe a commit>...] <commit-ish> git describe [<option to describe a blob>...] <blob>

in the synopsis is not helpful, because the user may not know what kind of object s/he has, and cannot decide from which set of options to pick? Then an alternative would be to list

(If I remember correctly) My nit pick was roughly along the lines you suggest, and that the two option lists (for commit-ish and blob) were shown in different ways, which could lead to the scenarion that, with knowing the

oid object type (or knowing how to get it), the user could give an invalid option, and think the command failure was because the oid was invalid, not that the option was not appropriate, along with variations on that theme.

The newer synopsis (v5) looks Ok in that it avoids digging the hole by not mentioning the blob options. Personally I'm more for manuals that tend toward instructional, rather than being expert references. I'd sneak in a line saying "The object type can be determined using git cat-file.", but maybe that's my work environment...

Quoted 7 lines

git describe [<option>...] <object>

in the synopsis, say upfront that most options are applicable only when describing a commit-ish, and when describing a blob, we do quite different thing and a separate set of options apply, perhaps?

--
Philip

Maybe

"git-describe - Describe a blob or commit using graph relations"

though that sounds too generic, but it is accurate as all we do is a heuristic for graph walk ways.

Signed-off-by Stefan Beller <[email protected]>

---
 t/t6120-describe.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

t/t6120-describe.sh +1 -1

index 3be01316e8..fd329f173a 100755
@@ -304,7 +304,7 @@ test_expect_success 'describe chokes on severely broken submodules' '
 	mv .git/modules/sub1/ .git/modules/sub_moved &&
 	test_must_fail git describe --dirty
 '
-test_expect_success 'describe ignoring a borken submodule' '
+test_expect_success 'describe ignoring a broken submodule' '
 	git describe --broken >out &&
 	test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" &&
 	grep broken out

Quoted 4 lines · Stefan Beller

Signed-off-by Stefan Beller <[email protected]>

---
 t/t6120-describe.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Good. I am guessing that you are sending this as the last/optional one because this was found _after_ you worked on other parts of the series, but I think it is easier to reason about if this were marked as a preliminary clean-up and moved to the front of the series.

Thanks.

Quoted 4 lines · Stefan Beller

Signed-off-by Stefan Beller <[email protected]>

---
 t/t6120-describe.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Good. I am guessing that you are sending this as the last/optional one because this was found _after_ you worked on other parts of the series, but I think it is easier to reason about if this were marked as a preliminary clean-up and moved to the front of the series.

Thanks.

With traverse_trees_and_blobs factored out of the main traverse function, the next patch can introduce an in-order revision walking with ease.

In the next patch we'll call traverse_trees_and_blobs from within the loop walking the commits, such that we'll have one invocation of that function per commit. That is why we do not want to have memory allocations in that function, such as we'd have if we were to use a strbuf locally. Pass a strbuf from traverse_commit_list into the blob and tree traversing function as a scratch pad that only needs to be allocated once.

t/t6100-rev-list-in-order.sh new file +9 -0

new file mode 100755
index 0000000000..651666979b
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='miscellaneous rev-list tests'
+
+. ./test-lib.sh
+
+
+test_expect_success 'setup' '
+	for x in one two three four

t/t6100-rev-list-in-order.sh new file +12 -0

+	git rev-list --in-commit-order --objects HEAD >actual.raw &&
+	cut -c 1-40 >actual <actual.raw &&
+
+	git cat-file --batch-check="%(objectname)" >expect.raw <<-\EOF &&
+		HEAD^{commit}
+		HEAD^{tree}
+		HEAD^{tree}:one
+		HEAD^{tree}:two
+		HEAD~1^{commit}
+		HEAD~1^{tree}
+		HEAD~1^{tree}:three
+		HEAD~2^{commit}

t/t6100-rev-list-in-order.sh new file +5 -0

+
+	test_cmp expect actual
+'
+
+test_done

Quoted 21 lines · Stefan Beller

t/t6100-rev-list-in-order.sh new file +15 -0

new file mode 100755
index 0000000000..651666979b
@@ -0,0 +1,46 @@
+#!/bin/sh
+
+test_description='miscellaneous rev-list tests'
+
+. ./test-lib.sh
+
+
+test_expect_success 'setup' '
+	for x in one two three four
+	do
+		echo $x >$x &&
+		git add $x &&
+		git commit -m "add file $x"
+	done &&
+	for x in four three

t/t6100-rev-list-in-order.sh line 19

Quoted 4 lines

t/t6100-rev-list-in-order.sh new file +4 -0

+	do
+		git rm $x &&
+		git commit -m "remove $x"
+	done &&

When "git commit -m 'remove four'" fails, this loop would still continue, so the &&-chain in "done &&" would still be rendered ineffetive.

        ...
        (
                for x in four three
                do
                        git rm $x &&
                        git commit -m "remote $x" ||
                        exit
                done
        ) &&
        test 0 -eq $? &&
        ...

Ugly? Yes. Correct? Also yes.

Quoted 13 lines · Johannes Schindelin

        ...
        (
                for x in four three
                do
                        git rm $x &&
                        git commit -m "remote $x" ||
                        exit
                done
        ) &&
        test 0 -eq $? &&
        ...

Quoted 1 line

Ugly? Yes. Correct? Also yes.

I think returning non-zero with "return" is how other tests avoid an extra level of subshell.
Ugly? Yes. Correct? Questionable but it seems to work for those who wrote them ;-)

Quoted 17 lines · Junio C Hamano

Quoted 14 lines · Johannes Schindelin

        ...
        (
                for x in four three
                do
                        git rm $x &&
                        git commit -m "remote $x" ||
                        exit
                done
        ) &&
        test 0 -eq $? &&
        ...

Ugly? Yes. Correct? Also yes.

Quoted 4 lines

I think returning non-zero with "return" is how other tests avoid an extra level of subshell.
Ugly? Yes. Correct? Questionable but it seems to work for those who wrote them ;-)

I think I'll go without the extra subshell and with s/return 1/false/ as the exact value doesn't matter.

Quoted 2 lines

I think I'll go without the extra subshell and with s/return 1/false/ as the exact value doesn't matter.

You mean

        for ...
        do
                xyz ||
                false
        done

? That does not work. Try this:

        for a in 1 2 3; do echo $a && false; done && echo done

While it does not print done, it will print all of 1, 2 and 3.

We do not care about the internal state, aborting early, we rather care only if the whole loop body was executed. Running the test

    test_expect_success 'witty title' '
        for a in 1 2 3; do echo $a && false; done && echo done
    '

not ok 1 - witty title

That is all we want to care about here?

Quoted 3 lines

not ok 1 - witty title

That is all we want to care about here?

We care about the loop body being executed successfully *each time*. A better counter example:

Quoted 13 lines · Johannes Schindelin

        ...
        (
                for x in four three
                do
                        git rm $x &&
                        git commit -m "remote $x" ||
                        exit
                done
        ) &&
        test 0 -eq $? &&
        ...

Quoted 1 line

Ugly? Yes. Correct? Also yes.

I think returning non-zero with "return" is how other tests avoid an extra level of subshell.
Ugly? Yes. Correct? Questionable but it seems to work for those who wrote them ;-)

Hi Junio,

Quoted 17 lines · Junio C Hamano

Quoted 14 lines · Johannes Schindelin

        ...
        (
                for x in four three
                do
                        git rm $x &&
                        git commit -m "remote $x" ||
                        exit
                done
        ) &&
        test 0 -eq $? &&
        ...

Ugly? Yes. Correct? Also yes.

Quoted 4 lines

I think returning non-zero with "return" is how other tests avoid an extra level of subshell.
Ugly? Yes. Correct? Questionable but it seems to work for those who wrote them ;-)

Given that the test_expect_* functions evaluate the code, it makes me wonder whether those return statements really return appropriately, or one call level too low.

Quoted 24 lines · Johannes Schindelin

Quoted 21 lines · Junio C Hamano

Quoted 13 lines · Johannes Schindelin

        ...
        (
                for x in four three
                do
                        git rm $x &&
                        git commit -m "remote $x" ||
                        exit
                done
        ) &&
        test 0 -eq $? &&
        ...

Quoted 1 line

Ugly? Yes. Correct? Also yes.

I think returning non-zero with "return" is how other tests avoid an extra level of subshell.
Ugly? Yes. Correct? Questionable but it seems to work for those who wrote them ;-)

Quoted 3 lines

Given that the test_expect_* functions evaluate the code, it makes me wonder whether those return statements really return appropriately, or one call level too low.

The test_expect functions eval the actual snippets inside a dummy function. This is intentional exactly to allow them to call "return" at will.

Quoted 7 lines

Quoted 3 lines

Given that the test_expect_* functions evaluate the code, it makes me wonder whether those return statements really return appropriately, or one call level too low.

The test_expect functions eval the actual snippets inside a dummy function. This is intentional exactly to allow them to call "return" at will.

Tricksy. And a bit unintuitive for script kings such as myself, but okay.

For debuggers aid we'd want to print debug statements early, so introduce a new line in the debug output that describes the whole function, and the change the next debug output to describe why we need

to search. Conveniently drop the arg from the second line; which will be useful in a follow up commit, that refactors the describe function.

Signed-off-by Stefan Beller <[email protected]>

When describing commits, we try to anchor them to tags or refs, as these are conceptually on a higher level than the commit. And if there is no ref or tag that matches exactly, we're out of luck. So we employ a heuristic to make up a name for the commit. These names are ambivalent, there might

be different tags or refs to anchor to, and there might be different path in the DAG to travel to arrive at the commit precisely.

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob

Signed-off-by Stefan Beller <[email protected]>

---

builtin/describe.c +1 -3

@@ -445,11 +495,16 @@ static void describe(const char *arg, int last_one)
 
        if (get_oid(arg, &oid))
                die(_("Not a valid object name %s"), arg);
-       cmit = lookup_commit_reference(&oid);
-       if (!cmit)
-               die(_("%s is not a valid '%s' object"), arg, commit_type);
+       cmit = lookup_commit_reference_gently(&oid, 1);

Quoted 1 line

to make up a name for the commit. These names are ambivalent, there might

I guess you meant s/ambivalent/ambiguous/ ?

Quoted 2 lines

[1] https://stackoverflow.com/questions/223678/which-commit-has-this-blob

Signed-off-by Stefan Beller <[email protected]>

Blank line before sign-off.

Not at all worth a re-roll, but less nesting and a bit less noisy:

-#define SEEN (1u << 0)

builtin/describe.c +6 -0

+		"--single-worktree",
+		"--objects",
+		"--in-commit-order",
+		NULL);
+
+	init_revisions(&revs, NULL);

t/t6120-describe.sh +5 -0

index 1c0e8659d9..3be01316e8 100755
@@ -310,6 +310,21 @@ test_expect_success 'describe ignoring a borken submodule' '
 	grep broken out
 '
 
+test_expect_success 'describe a blob at a tag' '
+	echo "make it a unique blob" >file &&
+	git add file && git commit -m "content in file" &&
+	git tag -a -m "latest annotated tag" unique-file &&
+	git describe HEAD:file >actual &&

Documentation/git-describe.txt By default (without --all or --tags) `git describe` only shows · line 30

Quoted 3 lines · Stefan Beller

Documentation/git-describe.txt +3 -0

+If the given object refers to a blob, it will be described
+as `<commit-ish>:<path>`, such that the blob can be found
+at `<path>` in the `<commit-ish>`. Note, that the commit is likely

Does the code describe a9dbc3f12c as v2.15.0:GIT-VERSION-GEN, or would it always be <commit>:<path>?

Documentation/git-describe.txt By default (without --all or --tags) `git describe` only shows · line 32

Quoted 2 lines

Documentation/git-describe.txt +2 -0

+not the commit that introduced the blob, but the one that was found
+first; to find the commit that introduced the blob, you need to find

Because we do not want to descend into the same tree object we saw earlier, this "we show the name we happened to find first without attempting to refine it for a better name" is a rather fundamental limitation of this approach. Hopefully we can later improve it with more thought, but for now it is better than nothing (and much better than "git log --raw | grep").

Quoted 2 lines

Does the code describe a9dbc3f12c as v2.15.0:GIT-VERSION-GEN, or would it always be <commit>:<path>?

As the blob is described using this function:

static void process_object(struct object *obj, const char *path, void *data)

{
       struct process_commit_data *pcd = data;

       if (!oidcmp(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {
               reset_revision_walk();
               describe_commit(&pcd->current_commit, pcd->dst);
               strbuf_addf(pcd->dst, ":%s", path);
       }
}

i.e. as describe_commit() is used on the commit part, the answer to your question is: the former. I guess that is why Stefan wrote commit-ish instead of commit ;-)

Quoted 6 lines

Quoted 2 lines

Does the code describe a9dbc3f12c as v2.15.0:GIT-VERSION-GEN, or would it always be <commit>:<path>?

As the blob is described using this function:

static void process_object(struct object *obj, const char *path, void

*data)

Quoted 11 lines

{
       struct process_commit_data *pcd = data;

       if (!oidcmp(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {
               reset_revision_walk();
               describe_commit(&pcd->current_commit, pcd->dst);
               strbuf_addf(pcd->dst, ":%s", path);
       }
}

i.e. as describe_commit() is used on the commit part, the answer to

your

Quoted 1 line

question is: the former. I guess that is why Stefan wrote

commit-ish

Quoted 1 line

instead of commit ;-)

$ ./git describe a9dbc3f12c
warning: reflog of 'HEAD' references pruned commits
v2.15.0-7-g980e40477f:GIT-VERSION-GEN

So as noted below, this output is less than ideal, but technically correct as
v2.15.0-7-g980e40477f contains that blob as well (you don't have these; it is this very series consisting of 7 patches on top of 2.15, none of them
touching GIT-VERSION-GEN, hence that blob stays intact.)

The way Junio asked, we actually may prefer the commit-ish to give that commit that introduced the blob for the first time, i.e. add the --reverse to the graph walking.

Quoted 34 lines · Stefan Beller

Quoted 6 lines

Quoted 2 lines

Does the code describe a9dbc3f12c as v2.15.0:GIT-VERSION-GEN, or would it always be <commit>:<path>?

As the blob is described using this function:

static void process_object(struct object *obj, const char *path, void

*data)

Quoted 11 lines

{
       struct process_commit_data *pcd = data;

       if (!oidcmp(&pcd->looking_for, &obj->oid) && !pcd->dst->len) {
               reset_revision_walk();
               describe_commit(&pcd->current_commit, pcd->dst);
               strbuf_addf(pcd->dst, ":%s", path);
       }
}

i.e. as describe_commit() is used on the commit part, the answer to

your

Quoted 1 line

question is: the former. I guess that is why Stefan wrote

commit-ish

Quoted 1 line

instead of commit ;-)

$ ./git describe a9dbc3f12c
warning: reflog of 'HEAD' references pruned commits
v2.15.0-7-g980e40477f:GIT-VERSION-GEN

So as noted below, this output is less than ideal, but technically correct as
v2.15.0-7-g980e40477f contains that blob as well (you don't have these; it is this very series consisting of 7 patches on top of 2.15, none of them
touching GIT-VERSION-GEN, hence that blob stays intact.)

Quoted 3 lines

The way Junio asked, we actually may prefer the commit-ish to give that commit that introduced the blob for the first time, i.e. add the --reverse to the graph walking.

I know id prefer the first commit that introduced the blob. That's what describing a commit does, it finds the oldest tag prior to the commit, while --contains finds the first tag that contains the commit as an ancestor.

Quoted 4 lines

I know id prefer the first commit that introduced the blob. That's what describing a commit does, it finds the oldest tag prior to the commit, while --contains finds the first tag that contains the commit as an ancestor.

It is very easy to wish for "the oldest commit introducing a blob". But since we're in a DAG, this question is not necessarily easy to answer:

- A - B - C

    \   /
      D

Let's assume that all commits except A have the blob, and that B and D (for some reason) have the same author/committer dates.

Do you identify B or D as the commit introducing the blob?

Quoted 15 lines

Quoted 4 lines

I know id prefer the first commit that introduced the blob. That's what describing a commit does, it finds the oldest tag prior to the commit, while --contains finds the first tag that contains the commit as an ancestor.

It is very easy to wish for "the oldest commit introducing a blob". But since we're in a DAG, this question is not necessarily easy to answer:

- A - B - C

    \   /
      D

Let's assume that all commits except A have the blob, and that B and D (for some reason) have the same author/committer dates.

Quoted 1 line

Do you identify B or D as the commit introducing the blob?

The current implementation gives C, though.
(assuming C is HEAD, and A is ancient)

With the --reverse flag one of B or D is given (not sure which, depends on the exact order).

Quoted 5 lines

The current implementation gives C, though.
(assuming C is HEAD, and A is ancient)

With the --reverse flag one of B or D is given (not sure which, depends on the exact order).

Sure, but it is still a tricky thing. Imagine

- A1 - B1 - A2 - B2 - B3

where all the B* commits have the blob. Do you really want to report B1 rather than B2 as the commit introducing the blob? (I would prefer B2...)

Hi Stefan,

Quoted 15 lines · Stefan Beller

Quoted 15 lines

Quoted 4 lines

I know id prefer the first commit that introduced the blob. That's what describing a commit does, it finds the oldest tag prior to the commit, while --contains finds the first tag that contains the commit as an ancestor.

It is very easy to wish for "the oldest commit introducing a blob". But since we're in a DAG, this question is not necessarily easy to answer:

- A - B - C

    \   /
      D

Let's assume that all commits except A have the blob, and that B and D (for some reason) have the same author/committer dates.

Quoted 7 lines

Quoted 1 line

Do you identify B or D as the commit introducing the blob?

The current implementation gives C, though.
(assuming C is HEAD, and A is ancient)

With the --reverse flag one of B or D is given (not sure which, depends on the exact order).

Sure, but it is still a tricky thing. Imagine

- A1 - B1 - A2 - B2 - B3

where all the B* commits have the blob. Do you really want to report B1 rather than B2 as the commit introducing the blob? (I would prefer B2...)

Ciao,
Dscho

I think both questions are valuable, the first is "which commit last had this blob", the second is "which commit first introduced this blob", neither of which can always give a definitive answer. It really depends on what question you're asking up front.

Quoted 4 lines · Jacob Keller

I think both questions are valuable, the first is "which commit last had this blob", the second is "which commit first introduced this blob", neither of which can always give a definitive answer. It really depends on what question you're asking up front.

Given that "describe" is about giving an object _a_ name that is plausibly useful to refer to it, it is not a good match for the above query that wants to know where it came from, how long it remains in the tree and where it ceases to be relevant. In order to support that use case, a totally different and possibly more useful avenue would be to think how this can be hooked into "git log" machinery.

A refresher for how "git log [--options] <pathspec>" works may be beneficial. We walk history and compare the tree of the commit we are looking at with the tree of its parent commits. If everything within <pathspec> is the same, we mark the transition between the parent and our commit TREESAME (other commits, i.e. the ones that have meaningful change within <pathspec>, are !TREESAME). Then the output routine presents the set of commits that includes commits that are !TREESAME, within the constraints of the --options given (e.g. with --full-history, sides of a merge that is TREESAME may still be shown to preserve connectedness of the resulting graph).

It is easy to imagine that we can restrict "git log" traversal with a "--blobchange=<blob>" option instead of (or in addition to) the limitation <pathspec> gives us. Instead of treating a commit whose diff against its parent commit has any filepair that is different within <pathspec> as "!TREESAME", we can treat a commit whose diff against its parent commit has a filepair that has the <blob> on either side of the filepair as "!TREESAME" (in other words, we ignore a transition that is not about introducing or forgetting the <blob> we are looking for as an "interesting change"). That would give you a commit history graph in which only (and all) such commits that either adds or removes the <blob> in it.

Hmm?

Quoted 13 lines · Junio C Hamano

...
It is easy to imagine that we can restrict "git log" traversal with a "--blobchange=<blob>" option instead of (or in addition to) the limitation <pathspec> gives us. Instead of treating a commit whose diff against its parent commit has any filepair that is different within <pathspec> as "!TREESAME", we can treat a commit whose diff against its parent commit has a filepair that has the <blob> on either side of the filepair as "!TREESAME" (in other words, we ignore a transition that is not about introducing or forgetting the <blob> we are looking for as an "interesting change"). That would give you a commit history graph in which only (and all) such commits that either adds or removes the <blob> in it.

Quoted 1 line

Hmm?

This seems quite useful in the context of figuring out how a file got to such a state. This is useful to me, since if I know the state of a file (ie: it's exact contents) I can determine the blob name, and then use that to lookup where it was introduced.

Sure, but it is still a tricky thing. Imagine

- A1 - B1 - A2 - B2 - B3

where all the B* commits have the blob. Do you really want to report B1 rather than B2 as the commit introducing the blob? (I would prefer B2...)

Quoted 2 lines · Johannes Schindelin

Sure, but it is still a tricky thing. Imagine

Quoted 4 lines

- A1 - B1 - A2 - B2 - B3

where all the B* commits have the blob. Do you really want to report B1 rather than B2 as the commit introducing the blob? (I would prefer B2...)

What if B3 renames or copies the blob?

Andreas.

Quoted 4 lines · Andreas Schwab

Quoted 2 lines · Johannes Schindelin

Sure, but it is still a tricky thing. Imagine

Quoted 8 lines

Quoted 4 lines

- A1 - B1 - A2 - B2 - B3

where all the B* commits have the blob. Do you really want to report B1 rather than B2 as the commit introducing the blob? (I would prefer B2...)

What if B3 renames or copies the blob?

Andreas.

With the current proposed patch you'd find B3, and then use the diff machinery to digg deeper from there (renames/copies ought to be easy to detect already?)

So with a copy B3 might be a better start than B1, as starting from B1 you would not find B3 easily.

For a rename, I would think a reverse log/blame on B1:path may help.

With that said, I think I'll just reroll the series with the current logic fixing the other minor issues that were brought up as B3 seems to be the most versatile (though not optimal) answer for many use cases.

Documentation/git-describe.txt By default (without --all or --tags) `git describe` only shows · line 30

Quoted 3 lines · Stefan Beller

Documentation/git-describe.txt +3 -0

+If the given object refers to a blob, it will be described
+as `<commit-ish>:<path>`, such that the blob can be found
+at `<path>` in the `<commit-ish>`. Note, that the commit is likely

Does the code describe a9dbc3f12c as v2.15.0:GIT-VERSION-GEN, or would it always be <commit>:<path>?

Documentation/git-describe.txt By default (without --all or --tags) `git describe` only shows · line 32

Quoted 2 lines

Documentation/git-describe.txt +2 -0

+not the commit that introduced the blob, but the one that was found
+first; to find the commit that introduced the blob, you need to find

Because we do not want to descend into the same tree object we saw earlier, this "we show the name we happened to find first without attempting to refine it for a better name" is a rather fundamental limitation of this approach. Hopefully we can later improve it with more thought, but for now it is better than nothing (and much better than "git log --raw | grep").

Documentation/git-describe.txt By default (without --all or --tags) `git describe` only shows · line 36

Quoted 5 lines

Documentation/git-describe.txt +4 -0

+the commit that last touched the path, e.g.
+`git log <commit-description> -- <path>`.
+As blobs do not point at the commits they are contained in,
+describing blobs is slow as we have to walk the whole graph.

Is it true that we walk the "whole graph", or do we stop immediately we find a path to the blob? The former makes it sound like completely useless.

Quoted 2 lines

-#define SEEN (1u << 0)

Interesting. Now we include revision.h this becomes redundant.

builtin/describe.c static void describe_commit(struct object_id *oid, struct strbuf *dst) · line 469

Quoted 2 lines

builtin/describe.c +2 -0

+     argv_array_pushl(&args, "internal: The first arg is not parsed",
+             "--all", "--reflog", /* as many starting points as possible */

Interesting.

Do we also search in the reflog in the normal "describe" operation? If not, perhaps we should? I wonder what's the performance implications would be.

That tangent aside, as "describe blob" is most likely a "what reaches and is holding onto this blob?" query, finding something that can only be reached from a reflog entry would make it more helpful than without the option.

Quoted 2 lines

+ /* NEEDSWORK: --all is incompatible with worktrees for now: */

What's that last colon about?

Quoted 7 lines

builtin/describe.c +6 -0

+             "--single-worktree",
+             "--objects",
+             "--in-commit-order",
+             NULL);
+
+     init_revisions(&revs, NULL);

builtin/describe.c static void describe_commit(struct object_id *oid, struct strbuf *dst) · line 486

Quoted 10 lines

builtin/describe.c +10 -0

+     if (setup_revisions(args.argc, args.argv, &revs, NULL) > 1)
+             BUG("setup_revisions could not handle all args?");
+
+     if (prepare_revision_walk(&revs))
+             die("revision walk setup failed");
+
+     traverse_commit_list(&revs, process_commit, process_object, &pcd);
+     reset_revision_walk();
+}
+

OK.

Quoted 17 lines

t/t6120-describe.sh +9 -0

index 1c0e8659d9..3be01316e8 100755
@@ -310,6 +310,21 @@ test_expect_success 'describe ignoring a borken submodule' '
      grep broken out
 '
 
+test_expect_success 'describe a blob at a tag' '
+     echo "make it a unique blob" >file &&
+     git add file && git commit -m "content in file" &&
+     git tag -a -m "latest annotated tag" unique-file &&
+     git describe HEAD:file >actual &&
+     echo "unique-file:file" >expect &&
+     test_cmp expect actual
+'
+

t/t6120-describe.sh test_expect_success 'describe ignoring a borken submodule' ' · line 327

Quoted 6 lines

t/t6120-describe.sh +6 -0

+test_expect_success 'describe a surviving blob' '
+     git commit --allow-empty -m "empty commit" &&
+     git describe HEAD:file >actual &&
+     grep unique-file-1-g actual
+'
+

I am not sure what "surviving" means in this context.

The word
hinted that the test would be finding a blob that only appears in a commit that only appears as a reflog entry, but that wasn't the case, which was a bit disappointing.

Occasionally a user is given an object hash from a blob as an error message or other output (e.g. [1]).

It would be useful to get a further description of such a blob, such as the (commit, path) tuple where this blob was introduced.

This implements the answer in builtin/describe, however the heuristics are weak. See patch 6 for details.