Re: [PATCHv4 7/7] builtin/describe.c: describe a blob
- From
Stefan Beller <sbeller@google.com>
- Date
- Nov 16, 2017, 01:22 UTC
- Message-ID
- <CAGZ79kaum5py=14kdFy1a+K_0MzfaD5boYStixh=1aY2tUCV-Q@mail.gmail.com>
- In-Reply-To
- <20171114175207.f23d492045d52b8aa16c00be@google.com>
On Tue, Nov 14, 2017 at 5:52 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
Show 8 quoted lines
> On Tue, 14 Nov 2017 16:30:43 -0800 > Stefan Beller <sbeller@google.com> 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.
Show 7 quoted lines
> >> 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.
Show 12 quoted lines
> >> +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.
Show 19 quoted lines
>
>> +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.
Show 12 quoted lines
> >> +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?
Show 15 quoted lines
> >> +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.