On Mon, Apr 14, 2025 at 1:06 PM Taylor Blau <me@xxxxxxxxxxxx> wrote:
>
> With '--unpacked', pack-objects adds loose objects (which don't appear
> in any of the excluded packs from '--stdin-packs') to the output pack
> without considering them as reachability tips for the name-hash
> traversal.
>
> This was an oversight in the original implementation of '--stdin-packs',
> since the code which enumerates and adds loose objects to the output
> pack (`add_unreachable_loose_objects()`) did not have access to the
> 'rev_info' struct found in `read_packs_list_from_stdin()`.
>
> Excluding unpacked objects from that traversal doesn't effect the
s/effect/affect/ ?
> correctness of the resulting pack, but it does make it harder to
> discover good deltas for loose objects.
>
> Now that the 'rev_info' struct is declared outside of
> `read_packs_list_from_stdin()`, we can pass it to
> `add_objects_in_unpacked_packs()` and add any loose objects as tips to
> the above-mentioned traversal, in theory producing slightly tighter
> packs as a result.
>
> Signed-off-by: Taylor Blau <me@xxxxxxxxxxxx>
> ---
> builtin/pack-objects.c | 20 ++++++++++++--------
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 1689cddd3a..2aa12da4af 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3642,7 +3642,7 @@ static void read_packs_list_from_stdin(struct rev_info *revs)
> string_list_clear(&exclude_packs, 0);
> }
>
> -static void add_unreachable_loose_objects(void);
> +static void add_unreachable_loose_objects(struct rev_info *revs);
>
> static void read_stdin_packs(int rev_list_unpacked)
> {
> @@ -3669,7 +3669,7 @@ static void read_stdin_packs(int rev_list_unpacked)
> ignore_packed_keep_in_core = 1;
> read_packs_list_from_stdin(&revs);
> if (rev_list_unpacked)
> - add_unreachable_loose_objects();
> + add_unreachable_loose_objects(&revs);
>
> if (prepare_revision_walk(&revs))
> die(_("revision walk setup failed"));
> @@ -3788,7 +3788,7 @@ static void enumerate_cruft_objects(void)
> _("Enumerating cruft objects"), 0);
>
> add_objects_in_unpacked_packs();
> - add_unreachable_loose_objects();
> + add_unreachable_loose_objects(NULL);
>
> stop_progress(&progress_state);
> }
> @@ -4066,8 +4066,9 @@ static void add_objects_in_unpacked_packs(void)
> }
>
> static int add_loose_object(const struct object_id *oid, const char *path,
> - void *data UNUSED)
> + void *data)
> {
> + struct rev_info *revs = data;
> enum object_type type = oid_object_info(the_repository, oid, NULL);
>
> if (type < 0) {
> @@ -4088,6 +4089,10 @@ static int add_loose_object(const struct object_id *oid, const char *path,
> } else {
> add_object_entry(oid, type, "", 0);
> }
> +
> + if (revs && type == OBJ_COMMIT)
> + add_pending_oid(revs, NULL, oid, 0);
> +
> return 0;
> }
>
> @@ -4096,11 +4101,10 @@ static int add_loose_object(const struct object_id *oid, const char *path,
> * add_object_entry will weed out duplicates, so we just add every
> * loose object we find.
> */
> -static void add_unreachable_loose_objects(void)
> +static void add_unreachable_loose_objects(struct rev_info *revs)
> {
> for_each_loose_file_in_objdir(repo_get_object_directory(the_repository),
> - add_loose_object,
> - NULL, NULL, NULL);
> + add_loose_object, NULL, NULL, revs);
> }
>
> static int has_sha1_pack_kept_or_nonlocal(const struct object_id *oid)
> @@ -4356,7 +4360,7 @@ static void get_object_list(struct rev_info *revs, int ac, const char **av)
> if (keep_unreachable)
> add_objects_in_unpacked_packs();
> if (pack_loose_unreachable)
> - add_unreachable_loose_objects();
> + add_unreachable_loose_objects(NULL);
> if (unpack_unreachable)
> loosen_unused_packed_objects();
>
> --
> 2.49.0.229.gc267761125.dirty
Should this patch have some tests demonstrating the difference in
which objects are included?
