Re: [PATCH v3 5/9] pack-objects: perform name-hash traversal for unpacked objects

[Junio C Hamano <gitster@pobox.com>]

Taylor Blau <me@ttaylorr.com> writes:

> 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 affect the
> 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@ttaylorr.com>
> ---


Clever.  And the necessary changes are surprisingly small.  I like
it.


>  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 d60cb042c9..eb2a4099cc 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3644,7 +3644,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)
>  {
> @@ -3671,7 +3671,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"));
> @@ -3790,7 +3790,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);
>  }
> @@ -4068,8 +4068,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) {
> @@ -4090,6 +4091,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;
>  }
>  
> @@ -4098,11 +4103,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)
> @@ -4358,7 +4362,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();