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

> On Tue, Dec 09, 2025 at 06:11:20PM +0000, Samo Pogačnik via GitGitGadget
> wrote:
> > From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <samo_pogacnik@t-2.net>
> > 
> > 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?