Subject: Re: [PATCH 10/38] pack v4: commit object encoding

From: Junio C Hamano <[email-masked]>
Date: Thu, 05 Sep 2013 23:57:53 -0700

Nicolas Pitre <[email-masked]> writes:

> This goes as follows:
>
> - Tree reference: either variable length encoding of the index
>   into the SHA1 table or the literal SHA1 prefixed by 0 (see
>   encode_sha1ref()).
>
> - Parent count: variable length encoding of the number of parents.
>   This is normally going to occupy a single byte but doesn't have to.
>
> - List of parent references: a list of encode_sha1ref() encoded
>   references, or nothing if the parent count was zero.
>
> - Author reference: variable length encoding of an index into the author
>   identifier dictionary table which also covers the time zone.  To make
>   the overall encoding efficient, the author table is sorted by usage
>   frequency so the most used names are first and require the shortest
>   index encoding.
>
> - Author time stamp: variable length encoded.  Year 2038 ready!
>
> - Committer reference: same as author reference.
>
> - Committer time stamp: same as author time stamp.
>
> The remainder of the canonical commit object content is then zlib
> compressed and appended to the above.
>
> Rationale: The most important commit object data is densely encoded while
> requiring no zlib inflate processing on access, and all SHA1 references
> are most likely to be direct indices into the pack index file requiring
> no SHA1 search into the pack index file.

May I suggest a small change to the above, though.

Reorder the entries so that Parent count, list of parents and
committer time stamp come first in this order, and then the rest.

That way, commit.c::parse_commit() could populate its field lazily
with parsing only the very minimum set of fields, and then the
revision walker, revision.c::add_parents_to_list(), can find where
in the priority queue to add the parents to the list of commits to
be processed while still keeping the object partially parsed.  If a
commit is UNINTERESTING, no further parsing needs to be done.

>
> Signed-off-by: Nicolas Pitre <[email-masked]>
> ---
>  packv4-create.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
>
> diff --git a/packv4-create.c b/packv4-create.c
> index 12527c0..d4a79f4 100644
> --- a/packv4-create.c
> +++ b/packv4-create.c
> @@ -14,6 +14,9 @@
>  #include "pack.h"
>  #include "varint.h"
>  
> +
> +static int pack_compression_level = Z_DEFAULT_COMPRESSION;
> +
>  struct data_entry {
>  	unsigned offset;
>  	unsigned size;
> @@ -274,6 +277,122 @@ static int encode_sha1ref(const unsigned char *sha1, unsigned char *buf)
>  	return 1 + 20;
>  }
>  
> +/*
> + * This converts a canonical commit object buffer into its
> + * tightly packed representation using the already populated
> + * and sorted commit_name_table dictionary.  The parsing is
> + * strict so to ensure the canonical version may always be
> + * regenerated and produce the same hash.
> + */
> +void *pv4_encode_commit(void *buffer, unsigned long *sizep)
> +{
> +	unsigned long size = *sizep;
> +	char *in, *tail, *end;
> +	unsigned char *out;
> +	unsigned char sha1[20];
> +	int nb_parents, index, tz_val;
> +	unsigned long time;
> +	z_stream stream;
> +	int status;
> +
> +	/*
> +	 * It is guaranteed that the output is always going to be smaller
> +	 * than the input.  We could even do this conversion in place.
> +	 */
> +	in = buffer;
> +	tail = in + size;
> +	buffer = xmalloc(size);
> +	out = buffer;
> +
> +	/* parse the "tree" line */
> +	if (in + 46 >= tail || memcmp(in, "tree ", 5) || in[45] != '\n')
> +		goto bad_data;
> +	if (get_sha1_lowhex(in + 5, sha1) < 0)
> +		goto bad_data;
> +	in += 46;
> +	out += encode_sha1ref(sha1, out);
> +
> +	/* count how many "parent" lines */
> +	nb_parents = 0;
> +	while (in + 48 < tail && !memcmp(in, "parent ", 7) && in[47] == '\n') {
> +		nb_parents++;
> +		in += 48;
> +	}
> +	out += encode_varint(nb_parents, out);
> +
> +	/* rewind and parse the "parent" lines */
> +	in -= 48 * nb_parents;
> +	while (nb_parents--) {
> +		if (get_sha1_lowhex(in + 7, sha1))
> +			goto bad_data;
> +		out += encode_sha1ref(sha1, out);
> +		in += 48;
> +	}
> +
> +	/* parse the "author" line */
> +	/* it must be at least "author x <x> 0 +0000\n" i.e. 21 chars */
> +	if (in + 21 >= tail || memcmp(in, "author ", 7))
> +		goto bad_data;
> +	in += 7;
> +	end = get_nameend_and_tz(in, &tz_val);
> +	if (!end)
> +		goto bad_data;
> +	index = dict_add_entry(commit_name_table, tz_val, in, end - in);
> +	if (index < 0)
> +		goto bad_dict;
> +	out += encode_varint(index, out);
> +	time = strtoul(end, &end, 10);
> +	if (!end || end[0] != ' ' || end[6] != '\n')
> +		goto bad_data;
> +	out += encode_varint(time, out);
> +	in = end + 7;
> +
> +	/* parse the "committer" line */
> +	/* it must be at least "committer x <x> 0 +0000\n" i.e. 24 chars */
> +	if (in + 24 >= tail || memcmp(in, "committer ", 7))
> +		goto bad_data;
> +	in += 10;
> +	end = get_nameend_and_tz(in, &tz_val);
> +	if (!end)
> +		goto bad_data;
> +	index = dict_add_entry(commit_name_table, tz_val, in, end - in);
> +	if (index < 0)
> +		goto bad_dict;
> +	out += encode_varint(index, out);
> +	time = strtoul(end, &end, 10);
> +	if (!end || end[0] != ' ' || end[6] != '\n')
> +		goto bad_data;
> +	out += encode_varint(time, out);
> +	in = end + 7;
> +
> +	/* finally, deflate the remaining data */
> +	memset(&stream, 0, sizeof(stream));
> +	deflateInit(&stream, pack_compression_level);
> +	stream.next_in = (unsigned char *)in;
> +	stream.avail_in = tail - in;
> +	stream.next_out = (unsigned char *)out;
> +	stream.avail_out = size - (out - (unsigned char *)buffer);
> +	status = deflate(&stream, Z_FINISH);
> +	end = (char *)stream.next_out;
> +	deflateEnd(&stream);
> +	if (status != Z_STREAM_END) {
> +		error("deflate error status %d", status);
> +		goto bad;
> +	}
> +
> +	*sizep = end - (char *)buffer;
> +	return buffer;
> +
> +bad_data:
> +	error("bad commit data");
> +	goto bad;
> +bad_dict:
> +	error("bad dict entry");
> +bad:
> +	free(buffer);
> +	return NULL;
> +}
> +
>  static struct pack_idx_entry *get_packed_object_list(struct packed_git *p)
>  {
>  	unsigned i, nr_objects = p->num_objects;