Re: [PATCH v4 4/5] Add reftable library

6 messages, 4 authors, 2020-02-20 · open the first message on its own page

Re: [PATCH v4 4/5] Add reftable library

From: Junio C Hamano <hidden>
Date: 2020-02-11 16:32:04

Han-Wen Nienhuys [off-list ref] writes:
I've uploaded https://git.eclipse.org/r/c/157501/ that proposes a way
to encode the hash size. I  look forward to feedback.
Development and design discussion happens here.  Please do not
require people to go click at an external website---once people
start doing so, we'd have to go around 47 different places to piece
one discussion together, which is just crazy.

Here is what I saw:

    A 24-byte header appears at the beginning of the file:

        'REFT'
        uint8( format_version )
        uint24( block_size )
        uint64( min_update_index )
        uint64( max_update_index )

    The `format_version` is a byte, and it indicates both the version of the on-disk
    format, as well as the size of the hash. The hash size is indicated in the MSB
    of the `format_version`. For the SHA1 hash, `format_version & 0x80 == 0` and all
    hash values are 20 bytes. For SHA256, `format_version & 0x80 == 1`, and all hash
    values are 32 bytes. Future hash functions may be added by using more bits at
    the right.

    The file format version can be extract as `format_version & 0x7f`. Currently,
    only version 1 is defined.

If you cast in stone that "& 0x7f is the way to extract the
version", then you cannot promise that you may steal more bits at
the right of MSB to support more hash functions, as you've reserved
the rightmost 7 bits already for the version number with 0x7f and
there are only 8 bits in your byte.

It seems that you are trying to make the format too dense?  Is it
too much a waste to use a separate word or a byte for hash?  Or
perhaps declare that format version 1 uses SHA-1, format version 2
uses SHA-256, etc. (in other words, do we want to support both SHA-1
and SHA-256 when we are at format version 7)?

Thanks.




Re: [PATCH v4 4/5] Add reftable library

From: Han-Wen Nienhuys <hidden>
Date: 2020-02-11 16:41:12

On Tue, Feb 11, 2020 at 5:32 PM Junio C Hamano [off-list ref] wrote:
Here is what I saw:

    A 24-byte header appears at the beginning of the file:

        'REFT'
        uint8( format_version )
        uint24( block_size )
        uint64( min_update_index )
        uint64( max_update_index )

    The `format_version` is a byte, and it indicates both the version of the on-disk
    format, as well as the size of the hash. The hash size is indicated in the MSB
    of the `format_version`. For the SHA1 hash, `format_version & 0x80 == 0` and all
    hash values are 20 bytes. For SHA256, `format_version & 0x80 == 1`, and all hash
    values are 32 bytes. Future hash functions may be added by using more bits at
    the right.

    The file format version can be extract as `format_version & 0x7f`. Currently,
    only version 1 is defined.

If you cast in stone that "& 0x7f is the way to extract the
version", then you cannot promise that you may steal more bits at
the right of MSB to support more hash functions, as you've reserved
the rightmost 7 bits already for the version number with 0x7f and
there are only 8 bits in your byte.

It seems that you are trying to make the format too dense?  Is it
too much a waste to use a separate word or a byte for hash?  Or
perhaps declare that format version 1 uses SHA-1, format version 2
uses SHA-256, etc. (in other words, do we want to support both SHA-1
and SHA-256 when we are at format version 7)?
I can see a future where we have a different format that allows for
more metadata so we can encode the hash size separately. But maybe
that can be for format v3 and up.

Let's do format v2 = format v1 but with 32-byte hashes.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH v4 4/5] Add reftable library

From: brian m. carlson <hidden>
Date: 2020-02-11 23:40:09

On 2020-02-11 at 16:40:56, Han-Wen Nienhuys wrote:
I can see a future where we have a different format that allows for
more metadata so we can encode the hash size separately. But maybe
that can be for format v3 and up.

Let's do format v2 = format v1 but with 32-byte hashes.
The way we've preferred to do this in Git is to write a four-byte hash
identifier into the file, but we have legacy concerns here, so I think
switching to v2 for SHA-256 is fine.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

Re: [PATCH v4 4/5] Add reftable library

From: Han-Wen Nienhuys <hidden>
Date: 2020-02-18 09:25:38

On Wed, Feb 12, 2020 at 12:40 AM brian m. carlson
[off-list ref] wrote:
On 2020-02-11 at 16:40:56, Han-Wen Nienhuys wrote:
quoted
I can see a future where we have a different format that allows for
more metadata so we can encode the hash size separately. But maybe
that can be for format v3 and up.

Let's do format v2 = format v1 but with 32-byte hashes.
The way we've preferred to do this in Git is to write a four-byte hash
identifier into the file, but we have legacy concerns here, so I think
switching to v2 for SHA-256 is fine.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


I've implemented this in the last version of the series.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--
Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH v4 4/5] Add reftable library

From: Han-Wen Nienhuys <hidden>
Date: 2020-02-11 16:46:21

On Tue, Feb 11, 2020 at 5:32 PM Junio C Hamano [off-list ref] wrote:
Han-Wen Nienhuys [off-list ref] writes:
quoted
I've uploaded https://git.eclipse.org/r/c/157501/ that proposes a way
to encode the hash size. I  look forward to feedback.
Development and design discussion happens here.  Please do not
require people to go click at an external website---once people
start doing so, we'd have to go around 47 different places to piece
one discussion together, which is just crazy.
The ground truth for the format is the spec. The spec is part of the
JGit repository, so it seems reasonable for it to be reviewed there.
Do you want to move it somewhere else?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: [PATCH v4 4/5] Add reftable library

From: Jonathan Nieder <hidden>
Date: 2020-02-20 17:20:07

Han-Wen Nienhuys wrote:
The ground truth for the format is the spec. The spec is part of the
JGit repository, so it seems reasonable for it to be reviewed there.
Do you want to move it somewhere else?
Let's stick a copy in Documentation/technical.  I can send a patch to
Git to do that today, and we can update the doc in JGit to point to
Git's version as the canonical one.

Thanks,
Jonathan
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help