git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: git@vger.kernel.org, jonathantanmy@google.com
Subject: Re: [PATCHv5 7/7] builtin/describe.c: describe a blob
Date: Thu, 16 Nov 2017 12:24:25 +0900	[thread overview]
Message-ID: <xmqqwp2qx5w6.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20171116020039.17810-8-sbeller@google.com> (Stefan Beller's message of "Wed, 15 Nov 2017 18:00:39 -0800")

Stefan Beller <sbeller@google.com> writes:

> When describing commits, we try to anchor them to tags or refs, as these
> are conceptually on a higher level than the commit. And if there is no ref
> or tag that matches exactly, we're out of luck.  So we employ a heuristic
> to make up a name for the commit. These names are ambiguous, there might
> be different tags or refs to anchor to, and there might be different
> path in the DAG to travel to arrive at the commit precisely.

I am not sure if "And if there is ..." is adding much value here (I
do not think it is even technically correct for that matter).  If
there are more than one tag that point at the commit the user is
interested in, we use one of the tags, as tags conceptually sit at a
higher level.  And we use a heuristic to use one or the other tag to
make up a name for the commit, so the same commit can have two valid
names. ---So what?  Neither of these two valid names is "ambigous";
the commit object the user wanted to name _is_ correctly identified
(I would assume that we are not discussing a hash collision).

Lucikly, if we remove "And if...precisely", the logic still flows
nicely, if not more, to the next paragraph.

> When describing a blob, we want to describe the blob from a higher layer
> as well, which is a tuple of (commit, deep/path) as the tree objects
> involved are rather uninteresting.  The same blob can be referenced by
> multiple commits, so how we decide which commit to use?  This patch
> implements a rather naive approach on this: As there are no back pointers
> from blobs to commits in which the blob occurs, we'll start walking from
> any tips available, listing the blobs in-order of the commit and once we

Is "any tips" still the case?  I was wondering why you start your
traversal at HEAD and nothing else in this iteration.  There seems
to be no mention of this design decision in the documentation and no
justification in the log.

> found the blob, we'll take the first commit that listed the blob. For
> example
>
>   git describe --tags v0.99:Makefile
>   conversion-901-g7672db20c2:Makefile
>
> tells us the Makefile as it was in v0.99 was introduced in commit 7672db20.
>
> The walking is performed in reverse order to show the introduction of a
> blob rather than its last occurrence.

The reversing may improve the chance of an older commit to be chosen
rather than the newer one, but it does not even guarantee to show the
"introduction".

What this guarantees is that a long history will be traversed fully
before we start considering which commit has the blob of interest, I
am afraid.  Is this a sensible trade-off?

> +	argv_array_pushl(&args, "internal: The first arg is not parsed",
> +		"--objects", "--in-commit-order", "--reverse", "HEAD",
> +		NULL);


  reply	other threads:[~2017-11-16  3:24 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-16  2:00 [PATCHv5 0/7] describe blob Stefan Beller
2017-11-16  2:00 ` [PATCHv5 1/7] t6120: fix typo in test name Stefan Beller
2017-11-16  2:00 ` [PATCHv5 2/7] list-objects.c: factor out traverse_trees_and_blobs Stefan Beller
2017-11-16  2:00 ` [PATCHv5 3/7] revision.h: introduce blob/tree walking in order of the commits Stefan Beller
2017-11-16  2:00 ` [PATCHv5 4/7] builtin/describe.c: rename `oid` to avoid variable shadowing Stefan Beller
2017-11-16  2:00 ` [PATCHv5 5/7] builtin/describe.c: print debug statements earlier Stefan Beller
2017-11-16  2:00 ` [PATCHv5 6/7] builtin/describe.c: factor out describe_commit Stefan Beller
2017-11-16  2:00 ` [PATCHv5 7/7] builtin/describe.c: describe a blob Stefan Beller
2017-11-16  3:24   ` Junio C Hamano [this message]
2017-11-16 19:34     ` Stefan Beller
2017-11-22  7:55       ` Junio C Hamano
2017-11-22 17:00         ` Stefan Beller
2017-11-24  7:18           ` Junio C Hamano
2017-11-27 19:38             ` Stefan Beller
2017-11-27 23:37               ` Junio C Hamano
2017-11-20 15:10   ` Philip Oakley
2017-12-19 19:22   ` Junio C Hamano
2017-12-19 19:39     ` Stefan Beller
2017-12-22 23:10       ` 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=xmqqwp2qx5w6.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.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).