Re: [PATCH v4 2/2] shallow: handling fetch relative-deepen
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Feb 11, 2026, 13:38 UTC
- 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:
Show 17 quoted lines
> 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.
Show 16 quoted lines
> @@ -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.
Show 8 quoted lines
> + 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`?
Show 11 quoted lines
> 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.
Show 29 quoted lines
> @@ -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?
Show 5 quoted lines
> + 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