From: Samo Pogačnik Date: Fri, 09 Jan 2026 16:48:19 GMT Subject: Re: [PATCH 2/2] shallow: handling fetch relative-deepen Message-ID: In-Reply-To: On Tue, 2026-01-06 at 08:44 +0100, Patrick Steinhardt wrote: > 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? Thank you for the valuable reply. I'll try to address all the raised points including mentioning in commit 1/2 that added test from 2/2 fail without 1/2 as quickly as time allows me to. Thanks! Samo