Re: [RFC PATCH 1/3] promised-blob, fsck: introduce promised blobs

[Jonathan Tan <jonathantanmy@google.com>]

On Tue, 11 Jul 2017 15:02:09 -0700
Stefan Beller <sbeller@google.com> wrote:

> Here I wondered what this file looks like, in a later patch you
> add documentation:
> 
>   +objects/promisedblob::
>   +       This file records the sha1 object names and sizes of promised
>   +       blobs.
>   +
> 
> but that leaves more fundamental questions:
> * Is that a list of sha1s, separated by LF? (CRLF? How would Windows
>   users interact with it, are they supposed to ever modify this file?)
> * Similar to .git/packed-refs, would we want to have a first line
>   that has some sort of comment?
> * In the first question I assumed a linear list, will that be a sorted list,
>   or do we want to have some fancy data structure here?
>   (critbit tree, bloom filter)
> * is the contents in ASCII or binary? (What are the separators?)
> * In the future I presume we'd want to quickly answer "Is X in the
>   promised blobs list?" so would it be possible (in the future) to
>   improve the searching e.g. binary search?
> * ... I'll read on to see my questions answered, but I'd guess
>   others would prefer to see it in the docs. :)

I'll write more documentation once the file format is finalized. At the
time this patch was written, this was a sorted binary file, where each
entry consists of a 20-byte SHA-1 and an 8-byte size. Now each entry is
a 20-byte SHA-1, a 1-byte type, and an 8-byte size (I will send this
patch out soon).

> Similar to other files, would we want to prefix the file with
> a 4 letter magic number and a version number?

That's a good idea.

> Later down the road, do we want to have a
> (plumbing) command to move an object from
> standard blob to promised blob (as of now I'd think
> it would perform this rm call as well as an insert into
> the promised blobs file) ?
> (Well, we'd also have to think about how to get objects
> out of a pack)
> 
> With such a command you can easily write your own custom
> filter to free up blobs again.

This sounds reasonable, but probably not in the initial set of patches.

> > +       test_must_fail git -C repo fsck
> 
>     test_i18n_grep missing out ?
> 
> maybe, too? (Maybe that is already tested elsewhere,
> so no need for it)

This test is just meant to show that configuration can make fsck pass,
not the existing behavior of fsck when it fails (which is tested
elsewhere - I guess that was your question).