Re: [RFC PATCH v2 2/4] promised-object, fsck: introduce promised objects

[Jonathan Tan <jonathantanmy@google.com>]

On Thu, 20 Jul 2017 11:07:29 -0700
Stefan Beller <sbeller@google.com> wrote:

> > +       if (fsck_promised_objects()) {
> > +               error("Errors found in promised object list");
> > +               errors_found |= ERROR_PROMISED_OBJECT;
> > +       }
> 
> This got me thinking: It is an error if we do not have an object
> and also do not promise it, but what about the other case:
> having and object and promising it, too?
> That is not harmful to the operation, except that the promise
> machinery may be slower due to its size. (Should that be a soft
> warning then? Do we have warnings in fsck?)

Good question - having an object and also having it promised is not an
error condition (and I don't think it's a good idea to make it so,
because objects can appear quite easily from various sources). In the
future, I expect "git gc" to be extended to remove such redundant lines
from the "promised" list.

> >   * The object type is stored in 3 bits.
> >   */
> 
> We may want to remove this comment while we're here as it
> sounds stale despite being technically correct.
> 1974632c66 (Remove TYPE_* constant macros and use
> object_type enums consistently., 2006-07-11)

I agree that the comment is unnecessary, but in this commit I didn't
modify anything to do with the type, so I left it there.

> >  struct object {
> > +       /*
> > +        * Set if this object is parsed. If set, "type" is populated and this
> > +        * object can be casted to "struct commit" or an equivalent.
> > +        */
> >         unsigned parsed : 1;
> > +       /*
> > +        * Set if this object is not in the repo but is promised. If set,
> > +        * "type" is populated, but this object cannot be casted to "struct
> > +        * commit" or an equivalent.
> > +        */
> > +       unsigned promised : 1;
> 
> Would it make sense to have a bit field instead:
> 
> #define STATE_BITS 2
> #define STATE_PARSED (1<<0)
> #define STATE_PROMISED (1<<1)
> 
>     unsigned state : STATE_BITS
> 
> This would be similar to the types and flags?

Both type and flag have to be bit fields (for different reasons), but
since we don't need such a combined field for "parsed" and "promised", I
prefer separating them each into their own field.

> > +test_expect_success 'fsck fails on missing objects' '
> > +       test_create_repo repo &&
> > +
> > +       test_commit -C repo 1 &&
> > +       test_commit -C repo 2 &&
> > +       test_commit -C repo 3 &&
> > +       git -C repo tag -a annotated_tag -m "annotated tag" &&
> > +       C=$(git -C repo rev-parse 1) &&
> > +       T=$(git -C repo rev-parse 2^{tree}) &&
> > +       B=$(git hash-object repo/3.t) &&
> > +       AT=$(git -C repo rev-parse annotated_tag) &&
> > +
> > +       # missing commit, tree, blob, and tag
> > +       rm repo/.git/objects/$(echo $C | cut -c1-2)/$(echo $C | cut -c3-40) &&
> > +       rm repo/.git/objects/$(echo $T | cut -c1-2)/$(echo $T | cut -c3-40) &&
> > +       rm repo/.git/objects/$(echo $B | cut -c1-2)/$(echo $B | cut -c3-40) &&
> > +       rm repo/.git/objects/$(echo $AT | cut -c1-2)/$(echo $AT | cut -c3-40) &&
> 
> This is a pretty cool test as it promises all sorts of objects
> from different parts of the graph.

Thanks.