Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
From: Jeff King <peff@peff.net>
To: Eric Sunshine <ericsunshine@gmail.com>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Jakub Narebski" <jnareb@gmail.com>, "Ted Ts\'o" <tytso@mit.edu>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Clemens Buchacher" <drizzd@aon.at>,
	"Shawn O. Pearce" <spearce@spearce.org>
Subject: Re: [RFC/PATCHv2 5/6] check commit generation cache validity against grafts
Date: Wed, 13 Jul 2011 15:35:09 -0400
Message-ID: <20110713193509.GC31965@sigill.intra.peff.net> (raw)
In-Reply-To: <4E1DAB14.5040001@gmail.com>

On Wed, Jul 13, 2011 at 10:26:28AM -0400, Eric Sunshine wrote:

> On 7/13/2011 3:06 AM, Jeff King wrote:
> >+void metadata_graph_validity(unsigned char out[20])
> >+{
> >+	git_SHA_CTX ctx;
> >+
> >+	git_SHA1_Init(&ctx);
> >+
> >+	git_SHA1_Update(&ctx, "grafts", 6);
> >+	commit_graft_validity(&ctx);
> >+
> >+	git_SHA1_Update(&ctx, "replace", 7);
> >+	replace_object_validity(&ctx);
> 
> The implementation of metadata_graph_validity() makes it clear that
> commit_graft_validity() and replace_object_validity() are computing
> checksums in aid of validity-checking of the generations cache.
> However, the naive reader seeing the names commit_graft_validity()
> and replace_object_validity() in the API is likely to assume that
> these functions are somehow checking validity of the grafts and
> replace-refs themselves, which is not the case. Perhaps better names
> would be commit_graft_checksum() and replace_object_checksum()?

Agreed. The term "validity" is a bit funny, and I think checksum is
better.

> The name metadata_graph_validity() also suffers from this
> shortcoming. The actual validity check is performed by
> check_cache_header(), whereas metadata_graph_validity() is merely
> computing a checksum.

Yeah. I had originally called it metadata_validity_graph() with the
assumption that the metadata-cache would provide a collection of
commonly-used validity functions. That didn't seem quite right, so I
switched the two words around to emphasize that it was about the graph.
But this function really has nothing to do with the metadata-cache at
all (except that validity tokens are a good place to use the result). It
really belongs to the commit subsystem, because it is about the shape of
the history graph.

So here's what I've done for the next iteration:

  1. metadata_graph_validity is now:

      void commit_graph_checksum(unsigned char out[20]);

     and lives in commit.[ch].

  2. commit_graft_validity is now commit_graft_checksum, and is static
     inside commit.c.

  3. replace_object_validity is now replace_objects_checksum. It must
     remain non-static because it lives in replace-object.c. I
     pluralized the "objects" to make it more clear it was not about
     checksumming a single replace_object, but rather all of them.

So now the only two warts I see are:

  1. Some of the *_checksum functions return a 20-byte sha1, and some
     are just meant to stir their contents into a SHA_CTX. I can live
     with that. You can tell which is which by looking at their
     signatures.

  2. The name "replace_object_checksum" can be read as "checksum the
     replace-objects" or as "replace this object's checksum". But the
     ambiguous verb in the name of the subsystem is not a problem I am
     introducing. The "replace-object" subsystem should perhaps have
     been called "replacement-object" from the beginning to make this
     more clear.

Thanks for the suggestions (I also took the suggestions from your other
email for improving the commit message).

-Peff

  reply index

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-07-13  6:47 [RFC/PATCHv2 0/6] generation numbers for faster traversals Jeff King
2011-07-13  6:57 ` [RFC/PATCHv2 1/6] decorate: allow storing values instead of pointers Jeff King
2011-07-13 17:52   ` Jonathan Nieder
2011-07-13 20:08     ` Jeff King
2011-07-14 17:34       ` Jeff King
2011-07-14 17:51         ` [PATCH 1/3] implement generic key/value map Jeff King
2011-07-14 18:52           ` Bert Wesarg
2011-07-14 18:54             ` Bert Wesarg
2011-07-14 18:55               ` Jeff King
2011-07-14 19:07                 ` Bert Wesarg
2011-07-14 19:14                   ` Jeff King
2011-07-14 19:18                     ` Bert Wesarg
2011-07-14 17:52         ` [PATCH 2/3] fast-export: use object to uint32 map instead of "decorate" Jeff King
2011-07-15  9:40           ` Sverre Rabbelier
2011-07-15 20:00             ` Jeff King
2011-07-14 17:53         ` [PATCH 3/3] decorate: use "map" for the underlying implementation Jeff King
2011-07-14 21:06         ` [RFC/PATCHv2 1/6] decorate: allow storing values instead of pointers Junio C Hamano
2011-08-04 22:43           ` [RFC/PATCH 0/5] macro-based key/value maps Jeff King
2011-08-04 22:45             ` [PATCH 1/5] implement generic key/value map Jeff King
2011-08-04 22:46             ` [PATCH 2/5] fast-export: use object to uint32 map instead of "decorate" Jeff King
2011-08-04 22:46             ` [PATCH 3/5] decorate: use "map" for the underlying implementation Jeff King
2011-08-04 22:46             ` [PATCH 4/5] map: implement persistent maps Jeff King
2011-08-04 22:46             ` [PATCH 5/5] implement metadata cache subsystem Jeff King
2011-08-05 11:03             ` [RFC/PATCH 0/5] macro-based key/value maps Jeff King
2011-08-05 15:31               ` René Scharfe
2011-08-06  6:30                 ` Jeff King
2011-07-13  7:04 ` [RFC/PATCHv2 2/6] add metadata-cache infrastructure Jeff King
2011-07-13  8:18   ` Bert Wesarg
2011-07-13  8:31     ` Jeff King
2011-07-13  8:45       ` Bert Wesarg
2011-07-13 19:18         ` Jeff King
2011-07-13 19:40       ` Junio C Hamano
2011-07-13 19:33   ` Junio C Hamano
2011-07-13 20:25     ` Jeff King
2011-07-13  7:05 ` [RFC/PATCHv2 3/6] commit: add commit_generation function Jeff King
2011-07-13 14:26   ` Eric Sunshine
2011-07-13  7:05 ` [RFC/PATCHv2 4/6] pretty: support %G to show the generation number of a commit Jeff King
2011-07-13  7:06 ` [RFC/PATCHv2 5/6] check commit generation cache validity against grafts Jeff King
2011-07-13 14:26   ` Eric Sunshine
2011-07-13 19:35     ` Jeff King [this message]
2011-07-13  7:06 ` [RFC/PATCHv2 6/6] limit "contains" traversals based on commit generation Jeff King
2011-07-13  7:23   ` Jeff King
2011-07-13 20:33     ` Junio C Hamano
2011-07-13 20:58       ` Jeff King
2011-07-13 21:12         ` Junio C Hamano
2011-07-13 21:18           ` Jeff King
2011-07-15 18:22   ` Junio C Hamano
2011-07-15 20:40     ` Jeff King
2011-07-15 21:04       ` Junio C Hamano
2011-07-15 21:14         ` Jeff King
2011-07-15 21:01 ` Generation numbers and replacement objects Jakub Narebski
2011-07-15 21:10   ` Jeff King
2011-07-16 21:10     ` Jakub Narebski
2011-08-04 22:48 [RFC/PATCH 0/2] patch-id caching Jeff King
2011-08-04 22:49 ` [PATCH 1/2] cherry: read default config Jeff King
2011-08-04 22:49 ` [PATCH 2/2] cache patch ids on disk Jeff King
2011-08-04 22:52   ` Jeff King

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

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

  git send-email \
    --in-reply-to=20110713193509.GC31965@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=avarab@gmail.com \
    --cc=drizzd@aon.at \
    --cc=ericsunshine@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jnareb@gmail.com \
    --cc=jrnieder@gmail.com \
    --cc=spearce@spearce.org \
    --cc=tytso@mit.edu \
    /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 Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


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