From: Patrick Steinhardt <ps@pks.im>
Date: Wed, 11 Feb 2026 13:38:22 GMT
Subject: Re: [PATCH v4 2/2] shallow: handling fetch relative-deepen
Message-ID: <aYyGTmS6fEb2QfBU@pks.im>
In-Reply-To: <e9b20ae06fd2c7f2c6b73c9f093a23c812227b7e.1768602661.git.gitgitgadget@gmail.com>

On Fri, Jan 16, 2026 at 10:31:01PM +0000, Samo Pogačnik via GitGitGadget wrote:
> diff --git a/shallow.c b/shallow.c
> index 497a25836b..1a32808865 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -130,11 +130,12 @@ static void free_depth_in_slab(int **ptr)
>  {
>  	FREE_AND_NULL(*ptr);
>  }
> -struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
> -		int shallow_flag, int not_shallow_flag)
> +struct commit_list *get_shallow_commits(struct object_array *heads,
> +					struct object_array *shallows, int *deepen_relative,
> +					int depth, int shallow_flag, int not_shallow_flag)
>  {
> -	size_t i = 0;
> -	int cur_depth = 0;
> +	size_t i = 0, j;

We can declare `j` in the loop itself, as it's not used anywhere else.

> @@ -168,16 +169,30 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>  		}
>  		parse_commit_or_die(commit);
>  		cur_depth++;
> -		if ((depth != INFINITE_DEPTH && cur_depth >= depth) ||
> -		    (is_repository_shallow(the_repository) && !commit->parents &&
> -		     (graft = lookup_commit_graft(the_repository, &commit->object.oid)) != NULL &&
> -		     graft->nr_parent < 0)) {
> -			commit_list_insert(commit, &result);
> -			commit->object.flags |= shallow_flag;
> -			commit = NULL;
> -			continue;
> +		if (shallows) {
> +			for (j = 0; j < shallows->nr; j++)
> +				if (oideq(&commit->object.oid, &shallows->objects[j].item->oid))
> +					if ((!cur_depth_shallow) || (cur_depth < cur_depth_shallow))

The additional braces around the respective conditions are not needed.

> +						cur_depth_shallow = cur_depth;
> +
> +			if ((is_repository_shallow(the_repository) && !commit->parents &&
> +			     (graft = lookup_commit_graft(the_repository, &commit->object.oid)) != NULL &&
> +			     graft->nr_parent < 0)) {
> +				commit = NULL;
> +				continue;
> +			}

This block here is almost the same as the one below. But there's some
confusing parts:

  - Why don't we update `result` at all?

  - Why don't we set the `shallow_flag`?

  - Why don't we have to check for the passed-in depth?

All of these parts feel somewhat surprising to me, as the function now
behaves so wildly different depending on whether or not `shallows` was
passed.

I guess this is because we really only care about `cur_depth_shallow`?

> diff --git a/upload-pack.c b/upload-pack.c
> index 2d2b70cbf2..4232eef34f 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -704,54 +704,11 @@ error:
>  	return -1;
>  }
>  
> -static int get_reachable_list(struct upload_pack_data *data,
> -			      struct object_array *reachable)
> +static void get_shallows_depth(struct upload_pack_data *data)

I think this function is rather pointless, as there is only a single
caller and we only end up forwarding to `get_shallow_commits()`. Let's
inline it.

> @@ -881,29 +838,14 @@ static void deepen(struct upload_pack_data *data, int depth)
>  			struct object *object = data->shallows.objects[i].item;
>  			object->flags |= NOT_SHALLOW;
>  		}
> -	} else if (data->deepen_relative) {
> -		struct object_array reachable_shallows = OBJECT_ARRAY_INIT;
> -		struct commit_list *result;
> -
> -		/*
> -		 * Checking for reachable shallows requires that our refs be
> -		 * marked with OUR_REF.
> -		 */
> -		refs_head_ref_namespaced(get_main_ref_store(the_repository),
> -					 check_ref, data);
> -		for_each_namespaced_ref_1(check_ref, data);
> -
> -		get_reachable_list(data, &reachable_shallows);
> -		result = get_shallow_commits(&reachable_shallows,
> -					     depth + 1,
> -					     SHALLOW, NOT_SHALLOW);
> -		send_shallow(data, result);
> -		free_commit_list(result);
> -		object_array_clear(&reachable_shallows);
>  	} else {
>  		struct commit_list *result;
>  
> -		result = get_shallow_commits(&data->want_obj, depth,
> +		if (data->deepen_relative)
> +			get_shallows_depth(data);

Okay, so here we now essentially call `get_shallow_commits()` twice. The
first time we compute `data->deepen_relative`, only to then pass it back
to `get_shallow_commits()` a second time. That feels quite strange to
me. Can't we have `get_shallow_commits()` handle this for us directly in
a single call?

> +		result = get_shallow_commits(&data->want_obj, NULL, NULL,
> +					     data->deepen_relative + depth,
>  					     SHALLOW, NOT_SHALLOW);
>  		send_shallow(data, result);
>  		free_commit_list(result);

Thanks!

Patrick