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

On Thu, 20 Jul 2017 11:07:29 -0700
Stefan Beller [off-list ref] wrote:

quoted

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

quoted

  * 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.

quoted

 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.

quoted

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