On Thu, 20 Jul 2017 11:07:29 -0700
Stefan Beller <sbeller@xxxxxxxxxx> 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.
