git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, gitster@pobox.com
Subject: Re: [PATCHv4 7/7] builtin/describe.c: describe a blob
Date: Tue, 14 Nov 2017 17:52:07 -0800	[thread overview]
Message-ID: <20171114175207.f23d492045d52b8aa16c00be@google.com> (raw)
In-Reply-To: <20171115003043.24080-8-sbeller@google.com>

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.

>  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".

> +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?

> +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.

> +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". (Should we also test the case where a blob is directly
tagged?)

> +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.

  reply	other threads:[~2017-11-15  1:52 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-15  0:30 [PATCHv4 0/7] describe a blob Stefan Beller
2017-11-15  0:30 ` [PATCHv4 1/7] t6120: fix typo in test name Stefan Beller
2017-11-15  0:30 ` [PATCHv4 2/7] list-objects.c: factor out traverse_trees_and_blobs Stefan Beller
2017-11-15  0:30 ` [PATCHv4 3/7] revision.h: introduce blob/tree walking in order of the commits Stefan Beller
2017-11-15  1:38   ` Jonathan Tan
2017-11-15  0:30 ` [PATCHv4 4/7] builtin/describe.c: rename `oid` to avoid variable shadowing Stefan Beller
2017-11-15  0:30 ` [PATCHv4 5/7] builtin/describe.c: print debug statements earlier Stefan Beller
2017-11-15  1:41   ` Jonathan Tan
2017-11-15  0:30 ` [PATCHv4 6/7] builtin/describe.c: factor out describe_commit Stefan Beller
2017-11-15  1:44   ` Jonathan Tan
2017-11-15  0:30 ` [PATCHv4 7/7] builtin/describe.c: describe a blob Stefan Beller
2017-11-15  1:52   ` Jonathan Tan [this message]
2017-11-16  1:22     ` Stefan Beller
2017-11-16  1:43       ` Junio C Hamano
2017-11-16  1:49         ` Stefan Beller
2017-11-16  2:04           ` Junio C Hamano
2017-11-17  1:06             ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20171114175207.f23d492045d52b8aa16c00be@google.com \
    --to=jonathantanmy@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).