Re: [RFC/PATCHv2 5/6] check commit generation cache validity against grafts

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

On 7/13/2011 3:06 AM, Jeff King wrote:

quoted

+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