Re: [PATCHv2 6/7] builtin/describe.c: describe a blob
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 1, 2017, 04:11 UTC
- Message-ID
- <xmqqbmkmvdrq.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20171031211852.13001-7-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> +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>?
> +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").
> +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.
> -#define SEEN (1u << 0)
Interesting. Now we include revision.h this becomes redundant.
Show 8 quoted lines
> +static void describe_blob(struct object_id oid, struct strbuf *dst)
> +{
> + struct rev_info revs;
> + struct argv_array args = ARGV_ARRAY_INIT;
> + struct process_commit_data pcd = { null_oid, oid, dst};
> +
> + 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.
> + /* NEEDSWORK: --all is incompatible with worktrees for now: */
What's that last colon about?
Show 16 quoted lines
> + "--single-worktree",
> + "--objects",
> + "--in-commit-order",
> + NULL);
> +
> + init_revisions(&revs, NULL);
> + 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.
Show 23 quoted lines
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > index 1c0e8659d9..3be01316e8 100755 > --- a/t/t6120-describe.sh > +++ b/t/t6120-describe.sh > @@ -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 > +' > + > +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.