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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 8, 2018, 21:21 UTC
Message-ID: <xmqq8tc3qitf.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <1518122258-157281-2-git-send-email-dstolee@microsoft.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 5 quoted lines

> 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?

Show 8 quoted lines

> +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").

Show 7 quoted lines

> +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?

Show 36 quoted lines

> +  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?