On Tue, May 24 2022, Taylor Blau wrote: > - struct rev_info *revs = _data; > - struct object_info oi = OBJECT_INFO_INIT; > off_t ofs; > - enum object_type type; > + enum object_type type = OBJ_NONE; > > display_progress(progress_state, ++nr_seen); > > @@ -3215,20 +3213,25 @@ static int add_object_entry_from_pack(const struct object_id *oid, > if (!want_object_in_pack(oid, 0, &p, &ofs)) > return 0; > > - oi.typep = &type; > - if (packed_object_info(the_repository, p, ofs, &oi) < 0) > - die(_("could not get type of object %s in pack %s"), > - oid_to_hex(oid), p->pack_name); > - else if (type == OBJ_COMMIT) { > - /* > - * commits in included packs are used as starting points for the > - * subsequent revision walk > - */ > - add_pending_oid(revs, NULL, oid, 0); > + if (p) { > + struct rev_info *revs = _data; > + struct object_info oi = OBJECT_INFO_INIT; > + > + oi.typep = &type; > + if (packed_object_info(the_repository, p, ofs, &oi) < 0) { > + die(_("could not get type of object %s in pack %s"), > + oid_to_hex(oid), p->pack_name); > + } else if (type == OBJ_COMMIT) { > + /* > + * commits in included packs are used as starting points for the > + * subsequent revision walk > + */ > + add_pending_oid(revs, NULL, oid, 0); > + } > + > + stdin_packs_found_nr++; > } > > - stdin_packs_found_nr++; > - > create_object_entry(oid, type, 0, 0, 0, p, ofs); Not rhetorical, since I have no idea: Is the behavior change here to make create_object_entry with type=OBJ_NONE desired? I.e. do we actually want to create object entries for OBJ_NONE? If that is the case I for one would find this a bit easier to follow like this, even if it has some minor duplication, i.e. the intent is clearer: diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index ffeaecd1d84..a447f6d5164 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -3202,7 +3202,6 @@ static int add_object_entry_from_pack(const struct object_id *oid, void *_data) { off_t ofs; - enum object_type type = OBJ_NONE; display_progress(progress_state, ++nr_seen); @@ -3216,6 +3215,7 @@ static int add_object_entry_from_pack(const struct object_id *oid, if (p) { struct rev_info *revs = _data; struct object_info oi = OBJECT_INFO_INIT; + enum object_type type; oi.typep = &type; if (packed_object_info(the_repository, p, ofs, &oi) < 0) { @@ -3230,9 +3230,11 @@ static int add_object_entry_from_pack(const struct object_id *oid, } stdin_packs_found_nr++; - } - create_object_entry(oid, type, 0, 0, 0, p, ofs); + create_object_entry(oid, type, 0, 0, 0, p, ofs); + } else { + create_object_entry(oid, OBJ_NONE, 0, 0, 0, p, ofs); + } return 0; } Or the same with adding "type = OBJ_NONE" to the "else" branch, leaving the initial "type" uninitialized"? Or perhaps this is a bug? I see some OBJ_NONE mentions in the code, but do packfiles really have "none" objects in some fashion as far as add_object_entry_from_pack() is concerned? (I'm not familiar enough with this part of the codebase to know).