@matheusbsbs / matheus-bagliano / commits / fd9b1baef8

pack-objects: turn type and in_pack_type to bitfields

An extra field type_valid is added to carry the equivalent of OBJ_BAD in the original "type" field. in_pack_type always contains a valid type so we only need 3 bits for it. A note about accepting OBJ_NONE as "valid" type. The function read_object_list_from_stdin() can pass this value [1] and it eventually calls create_object_entry() where current code skip setting "type" field if the incoming type is zero. This does not have any bad side effects because "type" field should be memset()'d anyway. But since we also need to set type_valid now, skipping oe_set_type() leaves type_valid zero/false, which will make oe_type() return OBJ_BAD, not OBJ_NONE anymore. Apparently we do care about OBJ_NONE in prepare_pack(). This switch from OBJ_NONE to OBJ_BAD may trigger fatal: unable to get type of object ... Accepting OBJ_NONE [2] does sound wrong, but this is how it is has been for a very long time and I haven't time to dig in further. [1] See 5c49c11686 (pack-objects: better check_object() performances - 2007-04-16) [2] 21666f1aae (convert object type handling from a string to a number - 2007-02-26) Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Nguyễn Thái Ngọc Duy committed Apr 14, 2018 at 17:35 UTC fd9b1baef8a940c9c251995b006a3d96f210e639

5 files changed +58 -30

builtin/pack-objects.c

+35 -24

@@ -266,7 +266,7 @@ static unsigned long write_no_reuse_object(struct hashfile *f, struct object_ent

266 struct git_istream *st = NULL;

267

268 if (!usable_delta) {

269 - if (entry->type == OBJ_BLOB &&

269 + if (oe_type(entry) == OBJ_BLOB &&

270 entry->size > big_file_threshold &&

271 (st = open_istream(&entry->idx.oid, &type, &size, NULL)) != NULL)

272 buf = NULL;

@@ -371,7 +371,7 @@ static off_t write_reuse_object(struct hashfile *f, struct object_entry *entry,

371 struct pack_window *w_curs = NULL;

372 struct revindex_entry *revidx;

373 off_t offset;

374 - enum object_type type = entry->type;

374 + enum object_type type = oe_type(entry);

375 off_t datalen;

376 unsigned char header[MAX_PACK_OBJECT_HEADER],

377 dheader[MAX_PACK_OBJECT_HEADER];

@@ -480,11 +480,12 @@ static off_t write_object(struct hashfile *f,

480 to_reuse = 0; /* explicit */

481 else if (!entry->in_pack)

482 to_reuse = 0; /* can't reuse what we don't have */

483 - else if (entry->type == OBJ_REF_DELTA || entry->type == OBJ_OFS_DELTA)

483 + else if (oe_type(entry) == OBJ_REF_DELTA ||

484 + oe_type(entry) == OBJ_OFS_DELTA)

485 /* check_object() decided it for us ... */

486 to_reuse = usable_delta;

487 /* ... but pack split may override that */

487 - else if (entry->type != entry->in_pack_type)

488 + else if (oe_type(entry) != entry->in_pack_type)

489 to_reuse = 0; /* pack has delta which is unusable */

490 else if (entry->delta)

491 to_reuse = 0; /* we want to pack afresh */

@@ -705,8 +706,8 @@ static struct object_entry **compute_write_order(void)

706 * And then all remaining commits and tags.

707 */

708 for (i = last_untagged; i < to_pack.nr_objects; i++) {

708 - if (objects[i].type != OBJ_COMMIT &&

709 - objects[i].type != OBJ_TAG)

709 + if (oe_type(&objects[i]) != OBJ_COMMIT &&

710 + oe_type(&objects[i]) != OBJ_TAG)

711 continue;

712 add_to_write_order(wo, &wo_end, &objects[i]);

713 }

@@ -715,7 +716,7 @@ static struct object_entry **compute_write_order(void)

716 * And then all the trees.

717 */

718 for (i = last_untagged; i < to_pack.nr_objects; i++) {

718 - if (objects[i].type != OBJ_TREE)

719 + if (oe_type(&objects[i]) != OBJ_TREE)

720 continue;

721 add_to_write_order(wo, &wo_end, &objects[i]);

722 }

@@ -1066,8 +1067,7 @@ static void create_object_entry(const struct object_id *oid,

1067

1068 entry = packlist_alloc(&to_pack, oid->hash, index_pos);

1069 entry->hash = hash;

1069 - if (type)

1070 - entry->type = type;

1070 + oe_set_type(entry, type);

1071 if (exclude)

1072 entry->preferred_base = 1;

1073 else

@@ -1407,6 +1407,7 @@ static void check_object(struct object_entry *entry)

1407 unsigned long avail;

1408 off_t ofs;

1409 unsigned char *buf, c;

1410 + enum object_type type;

1411

1412 buf = use_pack(p, &w_curs, entry->in_pack_offset, &avail);

1413

@@ -1415,11 +1416,15 @@ static void check_object(struct object_entry *entry)

1416 * since non-delta representations could still be reused.

1417 */

1418 used = unpack_object_header_buffer(buf, avail,

1418 - &entry->in_pack_type,

1419 + &type,

1420 &entry->size);

1421 if (used == 0)

1422 goto give_up;

1423

1424 + if (type < 0)

1425 + BUG("invalid type %d", type);

1426 + entry->in_pack_type = type;

1427 +

1428 /*

1429 * Determine if this is a delta and if so whether we can

1430 * reuse it or not. Otherwise let's find out as cheaply as

@@ -1428,9 +1433,9 @@ static void check_object(struct object_entry *entry)

1433 switch (entry->in_pack_type) {

1434 default:

1435 /* Not a delta hence we've already got all we need. */

1431 - entry->type = entry->in_pack_type;

1436 + oe_set_type(entry, entry->in_pack_type);

1437 entry->in_pack_header_size = used;

1433 - if (entry->type < OBJ_COMMIT || entry->type > OBJ_BLOB)

1438 + if (oe_type(entry) < OBJ_COMMIT || oe_type(entry) > OBJ_BLOB)

1439 goto give_up;

1440 unuse_pack(&w_curs);

1441 return;

@@ -1484,7 +1489,7 @@ static void check_object(struct object_entry *entry)

1489 * deltify other objects against, in order to avoid

1490 * circular deltas.

1491 */

1487 - entry->type = entry->in_pack_type;

1492 + oe_set_type(entry, entry->in_pack_type);

1493 entry->delta = base_entry;

1494 entry->delta_size = entry->size;

1495 entry->delta_sibling = base_entry->delta_child;

@@ -1493,7 +1498,7 @@ static void check_object(struct object_entry *entry)

1498 return;

1499 }

1500

1496 - if (entry->type) {

1501 + if (oe_type(entry)) {

1502 /*

1503 * This must be a delta and we already know what the

1504 * final object type is. Let's extract the actual

@@ -1516,7 +1521,7 @@ static void check_object(struct object_entry *entry)

1521 unuse_pack(&w_curs);

1522 }

1523

1519 - entry->type = oid_object_info(&entry->idx.oid, &entry->size);

1524 + oe_set_type(entry, oid_object_info(&entry->idx.oid, &entry->size));

1525 /*

1526 * The error condition is checked in prepare_pack(). This is

1527 * to permit a missing preferred base object to be ignored

@@ -1559,6 +1564,7 @@ static void drop_reused_delta(struct object_entry *entry)

1564 {

1565 struct object_entry **p = &entry->delta->delta_child;

1566 struct object_info oi = OBJECT_INFO_INIT;

1567 + enum object_type type;

1568

1569 while (*p) {

1570 if (*p == entry)

@@ -1570,15 +1576,18 @@ static void drop_reused_delta(struct object_entry *entry)

1576 entry->depth = 0;

1577

1578 oi.sizep = &entry->size;

1573 - oi.typep = &entry->type;

1579 + oi.typep = &type;

1580 if (packed_object_info(entry->in_pack, entry->in_pack_offset, &oi) < 0) {

1581 /*

1582 * We failed to get the info from this pack for some reason;

1583 * fall back to sha1_object_info, which may find another copy.

1578 - * And if that fails, the error will be recorded in entry->type

1584 + * And if that fails, the error will be recorded in oe_type(entry)

1585 * and dealt with in prepare_pack().

1586 */

1581 - entry->type = oid_object_info(&entry->idx.oid, &entry->size);

1587 + oe_set_type(entry, oid_object_info(&entry->idx.oid,

1588 + &entry->size));

1589 + } else {

1590 + oe_set_type(entry, type);

1591 }

1592 }

1593

@@ -1746,10 +1755,12 @@ static int type_size_sort(const void *_a, const void *_b)

1755 {

1756 const struct object_entry *a = *(struct object_entry **)_a;

1757 const struct object_entry *b = *(struct object_entry **)_b;

1758 + enum object_type a_type = oe_type(a);

1759 + enum object_type b_type = oe_type(b);

1760

1750 - if (a->type > b->type)

1761 + if (a_type > b_type)

1762 return -1;

1752 - if (a->type < b->type)

1763 + if (a_type < b_type)

1764 return 1;

1765 if (a->hash > b->hash)

1766 return -1;

@@ -1825,7 +1836,7 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,

1836 void *delta_buf;

1837

1838 /* Don't bother doing diffs between different types */

1828 - if (trg_entry->type != src_entry->type)

1839 + if (oe_type(trg_entry) != oe_type(src_entry))

1840 return -1;

1841

1842 /*

@@ -2429,11 +2440,11 @@ static void prepare_pack(int window, int depth)

2440

2441 if (!entry->preferred_base) {

2442 nr_deltas++;

2432 - if (entry->type < 0)

2443 + if (oe_type(entry) < 0)

2444 die("unable to get type of object %s",

2445 oid_to_hex(&entry->idx.oid));

2446 } else {

2436 - if (entry->type < 0) {

2447 + if (oe_type(entry) < 0) {

2448 /*

2449 * This object is not found, but we

2450 * don't have to include it anyway.

@@ -2542,7 +2553,7 @@ static void read_object_list_from_stdin(void)

2553 die("expected object ID, got garbage:\n %s", line);

2554

2555 add_preferred_base_object(p + 1);

2545 - add_object_entry(&oid, 0, p + 1, 0);

2556 + add_object_entry(&oid, OBJ_NONE, p + 1, 0);

2557 }

2558 }

2559

cache.h

@@ -373,6 +373,8 @@ extern void free_name_hash(struct index_state *istate);

373 #define read_blob_data_from_cache(path, sz) read_blob_data_from_index(&the_index, (path), (sz))

374 #endif

375

376 +#define TYPE_BITS 3

377 +

378 enum object_type {

379 OBJ_BAD = -1,

380 OBJ_NONE = 0,

object.h

-1

@@ -25,7 +25,6 @@ struct object_array {

26 #define OBJECT_ARRAY_INIT { 0, 0, NULL }

28 -#define TYPE_BITS 3

28 /*

29 * object flag allocation:

30 * revision.h: 0---------10 26

pack-bitmap-write.c

+3 -3

@@ -64,12 +64,12 @@ void bitmap_writer_build_type_index(struct pack_idx_entry **index,

65 entry->in_pack_pos = i;

67 - switch (entry->type) {

67 + switch (oe_type(entry)) {

68 case OBJ_COMMIT:

69 case OBJ_TREE:

70 case OBJ_BLOB:

71 case OBJ_TAG:

72 - real_type = entry->type;

72 + real_type = oe_type(entry);

73 break;

75 default:

@@ -97,7 +97,7 @@ void bitmap_writer_build_type_index(struct pack_idx_entry **index,

97 default:

98 die("Missing type information for %s (%d/%d)",

99 oid_to_hex(&entry->idx.oid), real_type,

100 - entry->type);

100 + oe_type(entry));

101 }

102 }

103 }

pack-objects.h

+18 -2

@@ -59,8 +59,9 @@ struct object_entry {

59 void *delta_data; /* cached delta (uncompressed) */

60 unsigned long delta_size; /* delta data size (uncompressed) */

61 unsigned long z_delta_size; /* delta data size (compressed) */

62 - enum object_type type;

63 - enum object_type in_pack_type; /* could be delta */

62 + unsigned type_:TYPE_BITS;

63 + unsigned in_pack_type:TYPE_BITS; /* could be delta */

64 + unsigned type_valid:1;

65 uint32_t hash; /* name hint hash */

66 unsigned int in_pack_pos;

67 unsigned char in_pack_header_size;

@@ -123,4 +124,19 @@ static inline uint32_t pack_name_hash(const char *name)

124 return hash;

125 }

126

127 +static inline enum object_type oe_type(const struct object_entry *e)

128 +{

129 + return e->type_valid ? e->type_ : OBJ_BAD;

130 +}

131 +

132 +static inline void oe_set_type(struct object_entry *e,

133 + enum object_type type)

134 +{

135 + if (type >= OBJ_ANY)

136 + BUG("OBJ_ANY cannot be set in pack-objects code");

137 +

138 + e->type_valid = type >= OBJ_NONE;

139 + e->type_ = (unsigned)type;

140 +}

141 +

142 #endif