Re: [PATCH] list-objects: mark fewer commits as edges for non-shallow clones

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 11, 2014, 19:26 UTC
Message-ID: <xmqqr3w6dm2w.fsf@gitster.dls.corp.google.com>
In-Reply-To: <xmqqvblidmp1.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> This change affects non-clone/fetch uses of object listing depending
> on the shallowness of the repository, and does not even care if it
> is driven as part of the pack-object codepath, if I am reading it
> correctly.  It smells wrong.
>
> The problematic fbd4a70 already had unintended fallout that needed
> to be corrected with 200abe74 (list-objects: only look at cmdline
> trees with edge_hint, 2014-01-20).  The current code with the fix,
> the decision to use the more expensive marking is tied to
> "edge_hint". I notice that edge_hint is turned on only if the caller
> of rev-list passes the "--objects-edge" option, and currently that
> only happens in the pack-objects codepath when "thin" is given.
> Perhaps that part should decide if it really wants to do edge_hint
> depending on the shallowness of the repository perhaps?
>
> That is, something like this instead?

Eh, perhaps not like that, as that would disable milder use of "thin" when fetching into non-shallow repository.

The right approach would be more like allocating one more bit in struct rev_info (call that edge_hint_aggressive), give a new option "--objects-edge-aggressive", and do something like

	if (thin) {
        	use_internal_rev_list = 1;
		argv_array_push(&rp, is_repository_shallow()
                	? "--objects-edge-aggressive"
                        : "--objects-edge");
	}

in this codepath? I'd actually suggest is_repository_shallow() detection to happen one level even higher (i.e. make decision at the caller of pack-objects) and decide to pass either "--thin" or "--thin-aggressive", so that we can make sure that the damage caused by fbd4a70 to be limited only to fetches into shallow repository with stronger confidence.

Show 17 quoted lines

>
>  builtin/pack-objects.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 3f9f5c7..a9ebf56 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2709,7 +2709,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  		usage_with_options(pack_usage, pack_objects_options);
>  
>  	argv_array_push(&rp, "pack-objects");
> -	if (thin) {
> +	if (thin && is_repository_shallow()) {
>  		use_internal_rev_list = 1;
>  		argv_array_push(&rp, "--objects-edge");
>  	} else