git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>, git <git@vger.kernel.org>, Jacob Keller <jacob.keller@gmail.com>, Kevin Daudt <me@ikke.info>
Subject: Re: [PATCHv2 6/7] builtin/describe.c: describe a blob
Date: Wed, 1 Nov 2017 14:28:27 -0700
Message-ID: <CAGZ79kbVWWo4YAK74hrJHAtwfMxYMixe1ypdcyaeWEMs8SHQpA@mail.gmail.com> (raw)
In-Reply-To: <xmqqbmkmvdrq.fsf@gitster.mtv.corp.google.com>

On Tue, Oct 31, 2017 at 9:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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").

ok.


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

Unfortunately we walk the whole graph, as I have not figured out how to
stop the walking from the callback in 'traverse_commit_list'.

I assume I have to modify the struct rev_info that we operate on,
clearing any pending commits?

>
>> -#define SEEN         (1u << 0)
>
> Interesting.  Now we include revision.h this becomes redundant.
>

Correct. In a way this small part is a revert of 8713ab3079
(Improve git-describe performance by reducing revision listing., 2007-01-13)


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

"normal" git describe doesn't need to walk the whole graph
as we only walk down from the given commit-ish until a land mark
is found.

For --contains, this might be an interesting though, as there we
also have to "walk backwards without pointers to follow".

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

Yeah that is my reasoning as well.

>
>> +             /* NEEDSWORK: --all is incompatible with worktrees for now: */
>
> What's that last colon about?

will replace with a dot, it ought to hint at the line that follows,
the --single-worktree flag.

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

Maybe "unchanged", "still kept around" ?


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

oh!

  parent reply index

Thread overview: 110+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-10-28  0:44 [RFC PATCH 0/3] git-describe <blob> ? Stefan Beller
2017-10-28  0:45 ` [PATCH 1/3] list-objects.c: factor out traverse_trees_and_blobs Stefan Beller
2017-10-28  0:45   ` [PATCH 2/3] revision.h: introduce blob/tree walking in order of the commits Stefan Beller
2017-10-28 17:20     ` Johannes Schindelin
2017-10-29  3:22       ` Stefan Beller
2017-10-29  3:23         ` Stefan Beller
2017-10-29  3:43           ` Junio C Hamano
2017-10-28  0:45   ` [PATCH 3/3] builtin/describe: describe blobs Stefan Beller
2017-10-28 17:32     ` Johannes Schindelin
2017-10-28 22:47       ` Jacob Keller
2017-10-29  3:28       ` Stefan Beller
2017-10-29 12:02         ` Kevin Daudt
2017-10-29 12:07         ` Johannes Schindelin
2017-10-28 17:15   ` [PATCH 1/3] list-objects.c: factor out traverse_trees_and_blobs Johannes Schindelin
2017-10-29  3:13     ` Stefan Beller
2017-10-28 16:04 ` [RFC PATCH 0/3] git-describe <blob> ? Johannes Schindelin
2017-10-31  0:33 ` [PATCH 0/7] git-describe <blob> Stefan Beller
2017-10-31  0:33   ` [PATCH 1/7] list-objects.c: factor out traverse_trees_and_blobs Stefan Beller
2017-10-31  6:07     ` Junio C Hamano
2017-10-31  0:33   ` [PATCH 2/7] revision.h: introduce blob/tree walking in order of the commits Stefan Beller
2017-10-31  6:57     ` Junio C Hamano
2017-10-31 18:12       ` Stefan Beller
2017-10-31  0:33   ` [PATCH 3/7] builtin/describe.c: rename `oid` to avoid variable shadowing Stefan Beller
2017-10-31  8:15     ` Jacob Keller
2017-10-31  0:33   ` [PATCH 4/7] builtin/describe.c: print debug statements earlier Stefan Beller
2017-10-31  7:03     ` Junio C Hamano
2017-10-31 19:05       ` Stefan Beller
2017-10-31  0:33   ` [PATCH 5/7] builtin/describe.c: factor out describe_commit Stefan Beller
2017-10-31  0:33   ` [PATCH 6/7] builtin/describe.c: describe a blob Stefan Beller
2017-10-31  6:25     ` Junio C Hamano
2017-10-31 19:16       ` Stefan Beller
2017-11-01  3:34         ` Junio C Hamano
2017-11-01 20:58           ` Stefan Beller
2017-11-02  1:53             ` Junio C Hamano
2017-11-02  4:23               ` Junio C Hamano
2017-11-04 21:15                 ` Philip Oakley
2017-11-05  6:28                   ` Junio C Hamano
2017-11-06 23:50                     ` Philip Oakley
2017-11-09 20:30                       ` Stefan Beller
2017-11-10  0:25                         ` Philip Oakley
2017-11-10  1:24                           ` Junio C Hamano
2017-11-10 22:44                             ` [PATCH 0/1] describe a blob: with better docs Stefan Beller
2017-11-10 22:44                               ` [PATCH] builtin/describe.c: describe a blob Stefan Beller
2017-11-13  1:33                                 ` Junio C Hamano
2017-11-14 23:37                                   ` Stefan Beller
2017-11-20 15:22                             ` [PATCH 6/7] " Philip Oakley
2017-11-20 18:18                               ` Philip Oakley
2017-11-01  3:44         ` Junio C Hamano
2017-10-31  0:33   ` [PATCH 7/7] t6120: fix typo in test name Stefan Beller
2017-11-01  1:21     ` Junio C Hamano
2017-11-01 18:13       ` Stefan Beller
2017-11-02  1:36       ` Junio C Hamano
2017-10-31 21:18   ` [PATCHv2 0/7] git describe blob Stefan Beller
2017-10-31 21:18     ` [PATCHv2 1/7] list-objects.c: factor out traverse_trees_and_blobs Stefan Beller
2017-11-01  3:46       ` Junio C Hamano
2017-10-31 21:18     ` [PATCHv2 2/7] revision.h: introduce blob/tree walking in order of the commits Stefan Beller
2017-11-01  3:50       ` Junio C Hamano
2017-11-01 12:26         ` Johannes Schindelin
2017-11-01 12:37           ` Junio C Hamano
2017-11-01 19:37             ` Stefan Beller
2017-11-01 22:08               ` Johannes Schindelin
2017-11-01 22:19                 ` Stefan Beller
2017-11-01 22:39                   ` Johannes Schindelin
2017-11-01 22:46                     ` Stefan Beller
2017-11-01 21:36             ` Johannes Schindelin
2017-11-01 21:39               ` Jeff King
2017-11-01 22:33                 ` Johannes Schindelin
2017-11-02  1:20                   ` Junio C Hamano
2017-10-31 21:18     ` [PATCHv2 3/7] builtin/describe.c: rename `oid` to avoid variable shadowing Stefan Beller
2017-10-31 21:18     ` [PATCHv2 4/7] builtin/describe.c: print debug statements earlier Stefan Beller
2017-10-31 21:31       ` Eric Sunshine
2017-10-31 21:18     ` [PATCHv2 5/7] builtin/describe.c: factor out describe_commit Stefan Beller
2017-10-31 21:18     ` [PATCHv2 6/7] builtin/describe.c: describe a blob Stefan Beller
2017-10-31 21:49       ` Eric Sunshine
2017-11-01 19:51         ` Stefan Beller
2017-11-01  4:11       ` Junio C Hamano
2017-11-01 12:32         ` Johannes Schindelin
2017-11-01 17:59           ` Stefan Beller
2017-11-01 21:05             ` Jacob Keller
2017-11-01 22:12               ` Johannes Schindelin
2017-11-01 22:21                 ` Stefan Beller
2017-11-01 22:41                   ` Johannes Schindelin
2017-11-01 22:53                     ` Stefan Beller
2017-11-02  6:05                     ` Jacob Keller
2017-11-03  5:18                       ` Junio C Hamano
2017-11-03  6:55                         ` Jacob Keller
2017-11-03 15:02                           ` Junio C Hamano
2017-11-02  7:23                     ` Andreas Schwab
2017-11-02 18:18                       ` Stefan Beller
2017-11-03 12:05                         ` Johannes Schindelin
2017-11-01 21:28         ` Stefan Beller [this message]
2017-10-31 21:18     ` [PATCHv2 7/7] t6120: fix typo in test name Stefan Beller
2017-11-01  5:14     ` [PATCHv2 0/7] git describe blob Junio C Hamano
2017-11-02 19:41     ` [PATCHv3 " Stefan Beller
2017-11-02 19:41       ` [PATCHv3 1/7] t6120: fix typo in test name Stefan Beller
2017-11-02 19:41       ` [PATCHv3 2/7] list-objects.c: factor out traverse_trees_and_blobs Stefan Beller
2017-11-02 19:41       ` [PATCHv3 3/7] revision.h: introduce blob/tree walking in order of the commits Stefan Beller
2017-11-14 19:52         ` Jonathan Tan
2017-11-02 19:41       ` [PATCHv3 4/7] builtin/describe.c: rename `oid` to avoid variable shadowing Stefan Beller
2017-11-02 19:41       ` [PATCHv3 5/7] builtin/describe.c: print debug statements earlier Stefan Beller
2017-11-14 19:55         ` Jonathan Tan
2017-11-14 20:00           ` Stefan Beller
2017-11-02 19:41       ` [PATCHv3 6/7] builtin/describe.c: factor out describe_commit Stefan Beller
2017-11-02 19:41       ` [PATCHv3 7/7] builtin/describe.c: describe a blob Stefan Beller
2017-11-14 20:02         ` Jonathan Tan
2017-11-14 20:40           ` Stefan Beller
2017-11-14 21:17             ` Jonathan Tan
2017-11-03  0:23       ` [PATCHv3 0/7] git describe blob Jacob Keller
2017-11-03  1:46         ` Junio C Hamano
2017-11-03  2:29           ` Stefan Beller

Reply instructions:

You may reply publically 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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kbVWWo4YAK74hrJHAtwfMxYMixe1ypdcyaeWEMs8SHQpA@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jacob.keller@gmail.com \
    --cc=me@ikke.info \
    /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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox