Re: [PATCH v4 2/2] shallow: handling fetch relative-deepen

> 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?