Re: [PATCH v3 01/14] commit-graph: add format document

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

> Add document specifying the binary format for commit graphs. This
> format allows for:
>
> * New versions.
> * New hash functions and hash lengths.

It still is unclear, at least to me, why OID and OID length are
stored as if they can be independent.  If a reader does not
understand a new Object Id hash, is there anything the reader can
still do by knowing how long the hash (which it cannot recompute to
validate) is?  And if a reader does know what OID hashing scheme is
used to refer to the objects, it certainly would know how long the
OIDs are.

Giving length may make sense only when a reader can treat these OIDs
as completely opaque identifiers, without having to (re)hash from
the contents, but if that is the case, then there is no point saying
what exact hash function is used to compute OID.

So I'd understand storing only either one or the other, but not
both.  Am I missing something?

> +The Git commit graph stores a list of commit OIDs and some associated
> +metadata, including:
> +
> +- The generation number of the commit. Commits with no parents have
> +  generation number 1; commits with parents have generation number
> +  one more than the maximum generation number of its parents. We
> +  reserve zero as special, and can be used to mark a generation
> +  number invalid or as "not computed".

This "most natural" definition of generation number is stricter than
absolutely necessary (a looser definition that is sufficient is
"gennum of a child is larger than all of its parents'").  While I
personally think that is OK, some people who floated different ideas
in previous discussions on generation numbers may want to articulate
their ideas again.  One idea that I found clever was to use the
total number of commits that are ancestors of a commit instead (it
is far more expensive to compute than the most natural gennum, but
doing so may help other topology math, like "describe").

> +CHUNK LOOKUP:
> +
> +  (C + 1) * 12 bytes listing the table of contents for the chunks:
> +      First 4 bytes describe chunk id. Value 0 is a terminating label.
> +      Other 8 bytes provide offset in current file for chunk to start.
> +      (Chunks are ordered contiguously in the file, so you can infer
> +      the length using the next chunk position if necessary.)

Aren't chunks numbered contiguously, starting from #1, thereby
making it unnecessary to store the 4-byte?

How does a reader obtain the length of the last chunk?  Ahh, that is
why there are C+1 entries in this table, not just C, so that the
reader knows where to stop while reading the last one.  Does that
mean that this table looks like this?
   
    { 1, offset_1 },
    { 2, offset_2 },
    ...
    { C, offset_C },
    { 0, offset_C+1 },

where where (offset_N+1 - offset_N) gives the length of chunk #N?

> +  The remaining data in the body is described one chunk at a time, and
> +  these chunks may be given in any order. Chunks are required unless
> +  otherwise specified.
> +
> +CHUNK DATA:
> +
> +  OID Fanout (ID: {'O', 'I', 'D', 'F'}) (256 * 4 bytes)
> +      The ith entry, F[i], stores the number of OIDs with first
> +      byte at most i. Thus F[255] stores the total
> +      number of commits (N).
> +
> +  OID Lookup (ID: {'O', 'I', 'D', 'L'}) (N * H bytes)
> +      The OIDs for all commits in the graph, sorted in ascending order.
> +
> +  Commit Data (ID: {'C', 'G', 'E', 'T' }) (N * (H + 16) bytes)
> +    * The first H bytes are for the OID of the root tree.
> +    * The next 8 bytes are for the int-ids of the first two parents
> +      of the ith commit. Stores value 0xffffffff if no parent in that
> +      position. If there are more than two parents, the second value
> +      has its most-significant bit on and the other bits store an array
> +      position into the Large Edge List chunk.
> +    * The next 8 bytes store the generation number of the commit and
> +      the commit time in seconds since EPOCH. The generation number
> +      uses the higher 30 bits of the first 4 bytes, while the commit
> +      time uses the 32 bits of the second 4 bytes, along with the lowest
> +      2 bits of the lowest byte, storing the 33rd and 34th bit of the
> +      commit time.
> +
> +  Large Edge List (ID: {'E', 'D', 'G', 'E'})
> +      This list of 4-byte values store the second through nth parents for
> +      all octopus merges. The second parent value in the commit data stores
> +      an array position within this list along with the most-significant bit
> +      on. Starting at that array position, iterate through this list of int-ids
> +      for the parents until reaching a value with the most-significant bit on.
> +      The other bits correspond to the int-id of the last parent. This chunk
> +      should always be present, but may be empty.

I am not convinced about the value of these 4-byte section IDs.  

They are useless unless you define what should happen when a reader
sees a block of data with an unknown ID; presence of these IDs given
an impression that you allow a reimplementation to reorder the
sections unnecessarily, especially when all of these are required
anyway, making the canonical reader implementation unnecessarily
complex, no?