From: Patrick Steinhardt Date: Tue, 06 Jan 2026 07:44:43 GMT Subject: Re: [PATCH 2/2] shallow: handling fetch relative-deepen Message-ID: In-Reply-To: On Tue, Dec 09, 2025 at 06:11:20PM +0000, Samo Pogačnik via GitGitGadget wrote: > From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= > > When a shallowed repository gets deepened beyond the beginning of a > merged branch, we may endup with some shallows, that are behind the s/endup/end up/ s/shallows, that/shallows that/ > reachable ones. Hm, which reachable ones? Sorry, I can't quite follow, it would help the reviewer to add a bit more context. > Added test 'fetching deepen beyond merged branch' exposes that > behaviour. > > On the other hand, it seems that equivalent absolute depth driven > fetches result in all the correct shallows. That led to this proposal, > which unifies absolute and relative deepening in a way that the same > get_shallow_commits() call is used in both cases. The difference is > only that depth is adapted for relative deepening by measuring > equivalent depth of current local shallow commits in the current remote > repo. Thus a new function get_shallows_depth() has been added and the > function get_reachable_list() became redundant / removed. > > The get_shallows_depth() function also shares the logic of the > get_shallow_commits() function, but it focuses on counting depth of > each existing shallow commit. The minimum result is stored as > 'data->deepen_relative', which is set not to be zero for relative > deepening anyway. That way we can allways summ 'data->deepen_relative' > and 'depth' values, because 'data->deepen_relative' is always 0 in > absolute deepening. I think the commit message needs some polishing. I myself am not that familiar with our shallow logic, so I'm a bit lost here to be honest. Typically, a commit message should be self-explanatory and guide the reader through the problem space as well as the solution. It should, in the following order: - Explain what the actual issue is as observed by the user. I'm not really sure about this part, only that it's something related to shallow clones, deepening and merge commits. - Explain what the root cause of the issue is. - Explain how the root cause is being fixed. Ideally, it should also explain why that is the correct fix, potentially referencing other code like you do. Your commit message on the other hand explains more of the "what" and less of the "why", which makes it hard to follow. Also, an ASCII commit graph would probably go a long way in explaining the issue :) > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index 2677cd5faa..d05c45e32b 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -955,6 +955,30 @@ test_expect_success 'fetching deepen' ' > ) > ' > > +test_expect_success 'fetching deepen beyond merged branch' ' > + test_create_repo shallow-deepen-merged && > + ( > + cd shallow-deepen-merged && > + git commit --allow-empty -m one && > + git commit --allow-empty -m two && > + git commit --allow-empty -m three && > + git switch -c branch && > + git commit --allow-empty -m four && > + git commit --allow-empty -m five && > + git switch main && > + git merge --no-ff branch && > + cd - && > + git clone --bare --depth 3 "file://$(pwd)/shallow-deepen-merged" deepen.git && > + git -C deepen.git fetch origin --deepen=1 && > + echo "Shallow:" && cat deepen.git/shallow && > + git -C deepen.git rev-list --all >actual && > + echo "All rev-lis:" && cat actual && This statement and the one two lines further up look like debug code to me. > + for commit in $(sed "/^$/d" deepen.git/shallow); do Nit: loops should be formatted like this: for commit in ... do ... done > diff --git a/upload-pack.c b/upload-pack.c > index 2d2b70cbf2..ecd3e7f5ef 100644 > --- a/upload-pack.c > +++ b/upload-pack.c > @@ -704,54 +705,82 @@ error: > return -1; > } > > -static int get_reachable_list(struct upload_pack_data *data, > - struct object_array *reachable) > +define_commit_slab(commit_depth, int *); > +static void free_depth_in_slab(int **ptr) > { > - struct child_process cmd = CHILD_PROCESS_INIT; > - int i; > - struct object *o; > - char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */ > - const unsigned hexsz = the_hash_algo->hexsz; > - int ret; > - > - if (do_reachable_revlist(&cmd, &data->shallows, reachable, > - data->allow_uor) < 0) { > - ret = -1; > - goto out; > - } > - > - while ((i = read_in_full(cmd.out, namebuf, hexsz + 1)) == hexsz + 1) { > - struct object_id oid; > - const char *p; > - > - if (parse_oid_hex(namebuf, &oid, &p) || *p != '\n') > - break; > - > - o = lookup_object(the_repository, &oid); > - if (o && o->type == OBJ_COMMIT) { > - o->flags &= ~TMP_MARK; > + FREE_AND_NULL(*ptr); > +} > +static void get_shallows_depth(struct upload_pack_data *data) This function looks very similar to `get_shallow_commits()`. Is it possible to deduplicate the logic? Thanks! Patrick