On Tue, Nov 14, 2017 at 5:52 PM, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
> On Tue, 14 Nov 2017 16:30:43 -0800
> Stefan Beller <sbeller@xxxxxxxxxx> wrote:
>
>> The walking is performed in reverse order to show the introduction of a
>> blob rather than its last occurrence.
>
> The code as implemented here does not do this - it instead shows the last
> occurrence.
fixed to show the first occurrence.
>
>> NAME
>> ----
>> -git-describe - Describe a commit using the most recent tag reachable from it
>> +git-describe - Describe a commit or blob using the graph relations
>
> I would write "Describe a commit or blob using a tag reachable from it".
using a ref, as we also can use refs.
I think 'the graph' is technically correct here, but may be too confusing.
>
>> +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
>> +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
>> +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.
>
> I think some of this needs to be updated?
fixed.
>
>> +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);
>> + pcd->revs->max_count = 0;
>> + }
>> +}
>
> Setting max_count to 0 does not work when reverse is used, because the
> commits are first buffered into revs->commits (see get_revision() in
> revision.c). There doesn't seem to be a convenient way to terminate the
> traversal immediately - I think setting revs->commits to NULL should
> work (but I didn't check). Remember to free revs->commits (using
> free_commit_list) first.
This does work indeed.
>
>> +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
>> +'
>
> This is probably better named "describe a blob at a directly tagged
> commit".
ok
> (Should we also test the case where a blob is directly
> tagged?)
We do a bad job at describing tags that point at a blob currently:
git tag test-blob HEAD:Makefile
git describe test-blob
error: object cd75985991f4535c45e2589222a9e6a38fb1d613 is a blob, not a commit
fatal: test-blob is not a valid 'commit' object
This series changes this to
git describe test-blob
v2.15.0-rc0-43-g54bd705a95:Makefile
which might not be expected (you'd expect "test-blob"),
so I think I can write a test telling that this is suboptimal
behavior?
>
>> +test_expect_success 'describe a blob with its last introduction' '
>> + git commit --allow-empty -m "empty commit" &&
>> + git rm file &&
>> + git commit -m "delete blob" &&
>> + git revert HEAD &&
>> + git commit --allow-empty -m "empty commit" &&
>> + git describe HEAD:file >actual &&
>> + grep unique-file-3-g actual
>> +'
>
> The description is not true: firstly, this shows the last occurrence,
> not the last introduction (you can verify this by adding another commit
> and noticing that the contents of "actual" changes), and what we want is
> not the last introduction anyway, but the first one.
fixed.
