shallow: handling fetch relative-deepen

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may endup with some shallows, that are behind the reachable ones. 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.

Samo Pogačnik (2):
shallow: free local object_array allocations
shallow: handling fetch relative-deepen

 shallow.c             |   1 +
 t/t5500-fetch-pack.sh |  24 +++++++
 upload-pack.c         | 142 +++++++++++++++++++++++-------------------
 3 files changed, 103 insertions(+), 64 deletions(-)

base-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2121%2Fspog%2Ffix-fetch-deepen-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2121/spog/fix-fetch-deepen-v1
Pull-Request: https://github.com/git/git/pull/2121

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

The local object_array 'stack' in get_shallow_commits() function does not free its dynamic elements before the function returns. As a result elements remain allocated and their reference forgotten.

---
 shallow.c | 1 +
 1 file changed, 1 insertion(+)

index 55b9cd9d3f..497a25836b 100644
@@ -198,6 +198,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		}
 	}
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
+	object_array_clear(&stack);
 
 	return result;
 }

gitgitgadget

I think the elements themselves are actually fine. We have the following loop:

	while (commit || i < heads->nr || stack.nr) {

So while the stack still has entries, we'll keep on iteration. Furthermore, there is no break or early return in the loop, so we can sure that we actually pop every single element from the array.

That being said, what we _don't_ do is to free the array itself. So I'm mostly splitting hairs with how the commit message is phrased, the change looks correct to me.

What I'm wondering though is why we never hit this memory leak in our test suite. I guess the reason is simply that we ain't got enough test coverage around shallow clones. Have you seen this leak in the wild? And if so, can we add a test case that surfaces it?

Thanks!

Patrick

Hi Patrick,
thanks a lot for the reply.

Actually, the test I've added with the patch 2/2 does not pass without this memory fix in linux-leaks and linux-reftable-leaks test runs.

best regards, Samo

In that case it would make sense to point out this detail in the commit message to make it a bit easier for the reviewer. Thanks!

Patrick

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may endup with some shallows, that are behind the reachable ones. 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.

---
 t/t5500-fetch-pack.sh |  24 +++++++
 upload-pack.c         | 142 +++++++++++++++++++++++-------------------
 2 files changed, 102 insertions(+), 64 deletions(-)

index 2677cd5faa..d05c45e32b 100755
@@ -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 &&
+		for commit in $(sed "/^$/d" deepen.git/shallow); do
+			test_grep "$commit" actual || exit 1
+		done
+	)
+'
+
 test_negotiation_algorithm_default () {
 	test_when_finished rm -rf clientv0 clientv2 &&
 	rm -rf server client &&

gitgitgadget

s/endup/end up/
s/shallows, that/shallows that/

Hm, which reachable ones? Sorry, I can't quite follow, it would help the reviewer to add a bit more context.

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 :)

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

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may endup with some shallows, that are behind the reachable ones. 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.

Samo Pogačnik (2):
shallow: free local object_array allocations
shallow: handling fetch relative-deepen

 shallow.c             | 45 +++++++++++++++++--------
 shallow.h             |  1 +
 t/t5500-fetch-pack.sh | 23 +++++++++++++
 upload-pack.c         | 76 +++++--------------------------------------
 4 files changed, 64 insertions(+), 81 deletions(-)

base-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2121%2Fspog%2Ffix-fetch-deepen-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2121/spog/fix-fetch-deepen-v2
Pull-Request: https://github.com/git/git/pull/2121

Range-diff vs v1:

1: 277c8616a9 ! 1: f8a8d077cd shallow: free local object_array allocations
@@ Commit message
does not free its dynamic elements before the function returns. As a result elements remain allocated and their reference forgotten.

+ Also note, that test 'fetching deepen beyond merged branch' added by
+ 'shallow: handling fetch relative-deepen' patch fails without this
+ correction in linux-leaks and linux-reftable-leaks test runs.
+

## shallow.c ##
2: b352a33c90 ! 2: ba1f80105f shallow: handling fetch relative-deepen

     @@ Commit message
          shallow: handling fetch relative-deepen
      
          When a shallowed repository gets deepened beyond the beginning of a
     -    merged branch, we may endup with some shallows, that are behind the
     -    reachable ones. Added test 'fetching deepen beyond merged branch'
     -    exposes that behaviour.
     +    merged branch, we may end up with some shallows that are hidden behind
     +    the reachable shallow commits. Added test 'fetching deepen beyond
     +    merged branch' exposes that behaviour.
     +
     +    An example showing the problem based on added test:
     +    0. Whole initial git repo to be cloned from
     +    Graph:
     +    *   033585d (HEAD -> main) Merge branch 'branch'
     +    |\
     +    | * 984f8b1 (branch) five
     +    | * ecb578a four
     +    |/
     +    * 0cb5d20 three
     +    * 2b4e70d two
     +    * 61ba98b one
     +
     +    1. Initial shallow clone --depth=3 (all good)
     +    Shallows:
     +    2b4e70da2a10e1d3231a0ae2df396024735601f1
     +    ecb578a3cf37198d122ae5df7efed9abaca17144
     +    Graph:
     +    *   033585d (HEAD -> main) Merge branch 'branch'
     +    |\
     +    | * 984f8b1 five
     +    | * ecb578a (grafted) four
     +    * 0cb5d20 three
     +    * 2b4e70d (grafted) two
     +
     +    2. Deepen shallow clone with fetch --deepen=1 (NOT OK)
     +    Shallows:
     +    0cb5d204f4ef96ed241feb0f2088c9f4794ba758
     +    61ba98be443fd51c542eb66585a1f6d7e15fcdae
     +    Graph:
     +    *   033585d (HEAD -> main) Merge branch 'branch'
     +    |\
     +    | * 984f8b1 five
     +    | * ecb578a four
     +    |/
     +    * 0cb5d20 (grafted) three
     +    ---
     +    Note that second shallow commit 61ba98be443fd51c542eb66585a1f6d7e15fcdae
     +    is not reachable.
      
          On the other hand, it seems that equivalent absolute depth driven
          fetches result in all the correct shallows. That led to this proposal,
     @@ Commit message
          repo. Thus a new function get_shallows_depth() has been added and the
          function get_reachable_list() became redundant / removed.
      
     +    Same example showing the corrected second step:
     +    2. Deepen shallow clone with fetch --deepen=1 (all good)
     +    Shallow:
     +    61ba98be443fd51c542eb66585a1f6d7e15fcdae
     +    Graph:
     +    *   033585d (HEAD -> main) Merge branch 'branch'
     +    |\
     +    | * 984f8b1 five
     +    | * ecb578a four
     +    |/
     +    * 0cb5d20 three
     +    * 2b4e70d two
     +    * 61ba98b (grafted) one
     +
          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
     @@ Commit message
      
          Signed-off-by: Samo Pogačnik <[email protected]>
      
     + ## shallow.c ##
     +@@ shallow.c: 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;
     ++	int cur_depth = 0, cur_depth_shallow = 0;
     + 	struct commit_list *result = NULL;
     + 	struct object_array stack = OBJECT_ARRAY_INIT;
     + 	struct commit *commit = NULL;
     +@@ shallow.c: 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))
     ++						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;
     ++			}
     ++		} else {
     ++			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;
     ++			}
     ++			commit->object.flags |= not_shallow_flag;
     + 		}
     +-		commit->object.flags |= not_shallow_flag;
     + 		for (p = commit->parents, commit = NULL; p; p = p->next) {
     + 			int **depth_slot = commit_depth_at(&depths, p->item);
     + 			if (!*depth_slot) {
     +@@ shallow.c: struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
     + 	}
     + 	deep_clear_commit_depth(&depths, free_depth_in_slab);
     + 	object_array_clear(&stack);
     +-
     ++	if (shallows && deepen_relative)
     ++		*deepen_relative = cur_depth_shallow;
     + 	return result;
     + }
     + 
     +
     + ## shallow.h ##
     +@@ shallow.h: int commit_shallow_file(struct repository *r, struct shallow_lock *lk);
     + void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);
     + 
     + 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);
     + struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,
     + 						    int shallow_flag, int not_shallow_flag);
     +
       ## t/t5500-fetch-pack.sh ##
      @@ t/t5500-fetch-pack.sh: test_expect_success 'fetching deepen' '
       	)
     @@ t/t5500-fetch-pack.sh: test_expect_success 'fetching deepen' '
      +		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 &&
     -+		for commit in $(sed "/^$/d" deepen.git/shallow); do
     ++		for commit in $(sed "/^$/d" deepen.git/shallow)
     ++		do
      +			test_grep "$commit" actual || exit 1
      +		done
      +	)
     @@ t/t5500-fetch-pack.sh: test_expect_success 'fetching deepen' '
       	rm -rf server client &&
      
       ## upload-pack.c ##
     -@@
     - #include "json-writer.h"
     - #include "strmap.h"
     - #include "promisor-remote.h"
     -+#include "tag.h"
     - 
     - /* Remember to update object flag allocation in object.h */
     - #define THEY_HAVE	(1u << 11)
      @@ upload-pack.c: 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)
     ++static void get_shallows_depth(struct upload_pack_data *data)
       {
      -	struct child_process cmd = CHILD_PROCESS_INIT;
      -	int i;
     @@ upload-pack.c: error:
      -		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)
     -+{
     -+	size_t i = 0, j;
     -+	int cur_depth = 0, cur_depth_shallow = 0;
     -+	struct object_array stack = OBJECT_ARRAY_INIT;
     -+	struct commit *commit = NULL;
     -+	struct commit_graft *graft;
     -+	struct commit_depth depths;
     -+	struct object_array *heads = &data->want_obj;
     -+	struct object_array *shallows = &data->shallows;
     -+
     -+	init_commit_depth(&depths);
     -+	while (commit || i < heads->nr || stack.nr) {
     -+		struct commit_list *p;
     -+		if (!commit) {
     -+			if (i < heads->nr) {
     -+				int **depth_slot;
     -+				commit = (struct commit *)
     -+					deref_tag(the_repository,
     -+						  heads->objects[i++].item,
     -+						  NULL, 0);
     -+				if (!commit || commit->object.type != OBJ_COMMIT) {
     -+					commit = NULL;
     -+					continue;
     -+				}
     -+				depth_slot = commit_depth_at(&depths, commit);
     -+				if (!*depth_slot)
     -+					*depth_slot = xmalloc(sizeof(int));
     -+				**depth_slot = 0;
     -+				cur_depth = 0;
     -+			} else {
     -+				commit = (struct commit *)
     -+					object_array_pop(&stack);
     -+				cur_depth = **commit_depth_at(&depths, commit);
     -+			}
     - 		}
     +-		}
      -	}
      -	for (i = get_max_object_index(the_repository); 0 < i; i--) {
      -		o = get_indexed_object(the_repository, i - 1);
     @@ upload-pack.c: error:
      -		    (o->flags & TMP_MARK)) {
      -			add_object_array(o, NULL, reachable);
      -				o->flags &= ~TMP_MARK;
     -+		parse_commit_or_die(commit);
     -+		cur_depth++;
     -+		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))
     -+					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;
     -+		}
     -+		for (p = commit->parents, commit = NULL; p; p = p->next) {
     -+			int **depth_slot = commit_depth_at(&depths, p->item);
     -+			if (!*depth_slot) {
     -+				*depth_slot = xmalloc(sizeof(int));
     -+				**depth_slot = cur_depth;
     -+			} else {
     -+				if (cur_depth >= **depth_slot)
     -+					continue;
     -+				**depth_slot = cur_depth;
     -+			}
     -+			if (p->next)
     -+				add_object_array(&p->item->object,
     -+						NULL, &stack);
     -+			else {
     -+				commit = p->item;
     -+				cur_depth = **commit_depth_at(&depths, commit);
     -+			}
     - 		}
     - 	}
     +-		}
     +-	}
      -	close(cmd.out);
      -
      -	if (finish_command(&cmd)) {
     @@ upload-pack.c: error:
      -out:
      -	child_process_clear(&cmd);
      -	return ret;
     -+	deep_clear_commit_depth(&depths, free_depth_in_slab);
     -+	object_array_clear(&stack);
     -+	data->deepen_relative = cur_depth_shallow;
     ++	get_shallow_commits(&data->want_obj, &data->shallows,
     ++			    &data->deepen_relative, 0,
     ++			    SHALLOW, NOT_SHALLOW);
       }
       
       static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
     @@ upload-pack.c: static void deepen(struct upload_pack_data *data, int depth)
      +		if (data->deepen_relative)
      +			get_shallows_depth(data);
      +
     -+		result = get_shallow_commits(&data->want_obj,
     ++		result = get_shallow_commits(&data->want_obj, NULL, NULL,
      +					     data->deepen_relative + depth,
       					     SHALLOW, NOT_SHALLOW);
       		send_shallow(data, result);

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

The local object_array 'stack' in get_shallow_commits() function does not free its dynamic elements before the function returns. As a result elements remain allocated and their reference forgotten.

Also note, that test 'fetching deepen beyond merged branch' added by 'shallow: handling fetch relative-deepen' patch fails without this correction in linux-leaks and linux-reftable-leaks test runs.

---
 shallow.c | 1 +
 1 file changed, 1 insertion(+)

index 55b9cd9d3f..497a25836b 100644
@@ -198,6 +198,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		}
 	}
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
+	object_array_clear(&stack);
 
 	return result;
 }

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may end up with some shallows that are hidden behind the reachable shallow commits. Added test 'fetching deepen beyond merged branch' exposes that behaviour.

An example showing the problem based on added test:
0. Whole initial git repo to be cloned from Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

|\
| * 984f8b1 (branch) five
| * ecb578a four
|/

* 0cb5d20 three
* 2b4e70d two
* 61ba98b one

1. Initial shallow clone --depth=3 (all good)
Shallows:
2b4e70da2a10e1d3231a0ae2df396024735601f1
ecb578a3cf37198d122ae5df7efed9abaca17144
Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

|\
| * 984f8b1 five
| * ecb578a (grafted) four

* 0cb5d20 three
* 2b4e70d (grafted) two

2. Deepen shallow clone with fetch --deepen=1 (NOT OK) Shallows:
0cb5d204f4ef96ed241feb0f2088c9f4794ba758 61ba98be443fd51c542eb66585a1f6d7e15fcdae Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

|\
| * 984f8b1 five
| * ecb578a four
|/

* 0cb5d20 (grafted) three
---
Note that second shallow commit 61ba98be443fd51c542eb66585a1f6d7e15fcdae is not reachable.

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.

Same example showing the corrected second step:
2. Deepen shallow clone with fetch --deepen=1 (all good) Shallow:
61ba98be443fd51c542eb66585a1f6d7e15fcdae
Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

|\
| * 984f8b1 five
| * ecb578a four
|/

* 0cb5d20 three
* 2b4e70d two
* 61ba98b (grafted) one

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.

---
 shallow.c             | 44 +++++++++++++++++--------
 shallow.h             |  1 +
 t/t5500-fetch-pack.sh | 23 +++++++++++++
 upload-pack.c         | 76 +++++--------------------------------------
 4 files changed, 63 insertions(+), 81 deletions(-)

index 497a25836b..1a32808865 100644
@@ -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;
+	int cur_depth = 0, cur_depth_shallow = 0;
 	struct commit_list *result = NULL;
 	struct object_array stack = OBJECT_ARRAY_INIT;
 	struct commit *commit = NULL;

@@ -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))
+						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;
+			}
+		} else {
+			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;
+			}
+			commit->object.flags |= not_shallow_flag;
 		}
-		commit->object.flags |= not_shallow_flag;
 		for (p = commit->parents, commit = NULL; p; p = p->next) {
 			int **depth_slot = commit_depth_at(&depths, p->item);
 			if (!*depth_slot) {

@@ -199,7 +214,8 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 	}
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
 	object_array_clear(&stack);
-
+	if (shallows && deepen_relative)
+		*deepen_relative = cur_depth_shallow;
 	return result;
 }
 

index ad591bd139..d1b3878635 100644
@@ -36,6 +36,7 @@ int commit_shallow_file(struct repository *r, struct shallow_lock *lk);
 void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);
 
 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);
 struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,
 						    int shallow_flag, int not_shallow_flag);

index 2677cd5faa..5a8b30e1fd 100755
@@ -955,6 +955,29 @@ 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 &&
+		git -C deepen.git rev-list --all >actual &&
+		for commit in $(sed "/^$/d" deepen.git/shallow)
+		do
+			test_grep "$commit" actual || exit 1
+		done
+	)
+'
+
 test_negotiation_algorithm_default () {
 	test_when_finished rm -rf clientv0 clientv2 &&
 	rm -rf server client &&

gitgitgadget

This three-dash line will act as a marker to tell "git am" that your log message ends here.

To avoid such an accident, make it a habit to indent any and all displayed material used as examples, e.g.,

2. Deepen shallow clone with fetch --deepen=1 (NOT OK)

Shallows:
0cb5d204f4ef96ed241feb0f2088c9f4794ba758
61ba98be443fd51c542eb66585a1f6d7e15fcdae
Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a four
   |/

* 0cb5d20 (grafted) three
---

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may endup with some shallows, that are behind the reachable ones. 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.

Samo Pogačnik (2):
shallow: free local object_array allocations
shallow: handling fetch relative-deepen

 shallow.c             | 45 +++++++++++++++++--------
 shallow.h             |  1 +
 t/t5500-fetch-pack.sh | 23 +++++++++++++
 upload-pack.c         | 76 +++++--------------------------------------
 4 files changed, 64 insertions(+), 81 deletions(-)

base-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2121%2Fspog%2Ffix-fetch-deepen-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2121/spog/fix-fetch-deepen-v3
Pull-Request: https://github.com/git/git/pull/2121

Range-diff vs v2:

 1:  f8a8d077cd = 1:  f8a8d077cd shallow: free local object_array allocations
 2:  ba1f80105f ! 2:  e79ab6b740 shallow: handling fetch relative-deepen

@@ Commit message

An example showing the problem based on added test:
0. Whole initial git repo to be cloned from
- Graph:
- * 033585d (HEAD -> main) Merge branch 'branch'
- |\
- | * 984f8b1 (branch) five
- | * ecb578a four
- |/
- * 0cb5d20 three
- * 2b4e70d two
- * 61ba98b one
+ Graph:
+ * 033585d (HEAD -> main) Merge branch 'branch'
+ |\
+ | * 984f8b1 (branch) five
+ | * ecb578a four
+ |/
+ * 0cb5d20 three
+ * 2b4e70d two
+ * 61ba98b one

1. Initial shallow clone --depth=3 (all good)
- Shallows:
- 2b4e70da2a10e1d3231a0ae2df396024735601f1
- ecb578a3cf37198d122ae5df7efed9abaca17144
- Graph:
- * 033585d (HEAD -> main) Merge branch 'branch'
- |\
- | * 984f8b1 five
- | * ecb578a (grafted) four
- * 0cb5d20 three
- * 2b4e70d (grafted) two
+ Shallows:
+ 2b4e70da2a10e1d3231a0ae2df396024735601f1
+ ecb578a3cf37198d122ae5df7efed9abaca17144
+ Graph:
+ * 033585d (HEAD -> main) Merge branch 'branch'
+ |\
+ | * 984f8b1 five
+ | * ecb578a (grafted) four
+ * 0cb5d20 three
+ * 2b4e70d (grafted) two

2. Deepen shallow clone with fetch --deepen=1 (NOT OK)
- Shallows:
- 0cb5d204f4ef96ed241feb0f2088c9f4794ba758
- 61ba98be443fd51c542eb66585a1f6d7e15fcdae
- Graph:
- * 033585d (HEAD -> main) Merge branch 'branch'
- |\
- | * 984f8b1 five
- | * ecb578a four
- |/
- * 0cb5d20 (grafted) three
- ---
- Note that second shallow commit 61ba98be443fd51c542eb66585a1f6d7e15fcdae
- is not reachable.
+ Shallows:
+ 0cb5d204f4ef96ed241feb0f2088c9f4794ba758
+ 61ba98be443fd51c542eb66585a1f6d7e15fcdae
+ Graph:
+ * 033585d (HEAD -> main) Merge branch 'branch'
+ |\
+ | * 984f8b1 five
+ | * ecb578a four
+ |/
+ * 0cb5d20 (grafted) three
+ ---
+ Note that second shallow commit 61ba98be443fd51c542eb66585a1f6d7e15fcdae
+ is not reachable.

On the other hand, it seems that equivalent absolute depth driven fetches result in all the correct shallows. That led to this proposal,
@@ Commit message

Same example showing the corrected second step:
2. Deepen shallow clone with fetch --deepen=1 (all good)
- Shallow:
- 61ba98be443fd51c542eb66585a1f6d7e15fcdae
- Graph:
- * 033585d (HEAD -> main) Merge branch 'branch'
- |\
- | * 984f8b1 five
- | * ecb578a four
- |/
- * 0cb5d20 three
- * 2b4e70d two
- * 61ba98b (grafted) one
+ Shallow:
+ 61ba98be443fd51c542eb66585a1f6d7e15fcdae
+ Graph:
+ * 033585d (HEAD -> main) Merge branch 'branch'
+ |\
+ | * 984f8b1 five
+ | * ecb578a four
+ |/
+ * 0cb5d20 three
+ * 2b4e70d two
+ * 61ba98b (grafted) one

The get_shallows_depth() function also shares the logic of the get_shallow_commits() function, but it focuses on counting depth of

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

The local object_array 'stack' in get_shallow_commits() function does not free its dynamic elements before the function returns. As a result elements remain allocated and their reference forgotten.

Also note, that test 'fetching deepen beyond merged branch' added by 'shallow: handling fetch relative-deepen' patch fails without this correction in linux-leaks and linux-reftable-leaks test runs.

---
 shallow.c | 1 +
 1 file changed, 1 insertion(+)

index 55b9cd9d3f..497a25836b 100644
@@ -198,6 +198,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		}
 	}
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
+	object_array_clear(&stack);
 
 	return result;
 }

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may end up with some shallows that are hidden behind the reachable shallow commits. Added test 'fetching deepen beyond merged branch' exposes that behaviour.

An example showing the problem based on added test:
0. Whole initial git repo to be cloned from Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 (branch) five
   | * ecb578a four
   |/

* 0cb5d20 three
* 2b4e70d two
* 61ba98b one

1. Initial shallow clone --depth=3 (all good)
Shallows:
2b4e70da2a10e1d3231a0ae2df396024735601f1
ecb578a3cf37198d122ae5df7efed9abaca17144
Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a (grafted) four

* 0cb5d20 three
* 2b4e70d (grafted) two

2. Deepen shallow clone with fetch --deepen=1 (NOT OK) Shallows:
0cb5d204f4ef96ed241feb0f2088c9f4794ba758 61ba98be443fd51c542eb66585a1f6d7e15fcdae Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a four
   |/

* 0cb5d20 (grafted) three
---
Note that second shallow commit 61ba98be443fd51c542eb66585a1f6d7e15fcdae is not reachable.

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.

Same example showing the corrected second step:
2. Deepen shallow clone with fetch --deepen=1 (all good) Shallow:
61ba98be443fd51c542eb66585a1f6d7e15fcdae Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a four
   |/

* 0cb5d20 three
* 2b4e70d two
* 61ba98b (grafted) one

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.

---
 shallow.c             | 44 +++++++++++++++++--------
 shallow.h             |  1 +
 t/t5500-fetch-pack.sh | 23 +++++++++++++
 upload-pack.c         | 76 +++++--------------------------------------
 4 files changed, 63 insertions(+), 81 deletions(-)

index 497a25836b..1a32808865 100644
@@ -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;
+	int cur_depth = 0, cur_depth_shallow = 0;
 	struct commit_list *result = NULL;
 	struct object_array stack = OBJECT_ARRAY_INIT;
 	struct commit *commit = NULL;

@@ -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))
+						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;
+			}
+		} else {
+			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;
+			}
+			commit->object.flags |= not_shallow_flag;
 		}
-		commit->object.flags |= not_shallow_flag;
 		for (p = commit->parents, commit = NULL; p; p = p->next) {
 			int **depth_slot = commit_depth_at(&depths, p->item);
 			if (!*depth_slot) {

@@ -199,7 +214,8 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 	}
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
 	object_array_clear(&stack);
-
+	if (shallows && deepen_relative)
+		*deepen_relative = cur_depth_shallow;
 	return result;
 }
 

index ad591bd139..d1b3878635 100644
@@ -36,6 +36,7 @@ int commit_shallow_file(struct repository *r, struct shallow_lock *lk);
 void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);
 
 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);
 struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,
 						    int shallow_flag, int not_shallow_flag);

index 2677cd5faa..5a8b30e1fd 100755
@@ -955,6 +955,29 @@ 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 &&
+		git -C deepen.git rev-list --all >actual &&
+		for commit in $(sed "/^$/d" deepen.git/shallow)
+		do
+			test_grep "$commit" actual || exit 1
+		done
+	)
+'
+
 test_negotiation_algorithm_default () {
 	test_when_finished rm -rf clientv0 clientv2 &&
 	rm -rf server client &&

gitgitgadget

s/allways summ/always sum/ ?

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may endup with some shallows, that are behind the reachable ones. 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.

Samo Pogačnik (2):
shallow: free local object_array allocations
shallow: handling fetch relative-deepen

 shallow.c             | 45 +++++++++++++++++--------
 shallow.h             |  1 +
 t/t5500-fetch-pack.sh | 23 +++++++++++++
 upload-pack.c         | 76 +++++--------------------------------------
 4 files changed, 64 insertions(+), 81 deletions(-)

base-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2121%2Fspog%2Ffix-fetch-deepen-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2121/spog/fix-fetch-deepen-v4
Pull-Request: https://github.com/git/git/pull/2121

Range-diff vs v3:

 1:  f8a8d077cd = 1:  f8a8d077cd shallow: free local object_array allocations
 2:  e79ab6b740 ! 2:  e9b20ae06f shallow: handling fetch relative-deepen

@@ Commit message
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'
+ deepening anyway. That way we can always sum 'data->deepen_relative'
and 'depth' values, because 'data->deepen_relative' is always 0 in absolute deepening.
+ To avoid duplicating logic between get_shallows_depth() and
+ get_shallow_commits(), get_shallow_commits() was modified so that
+ it is used by get_shallows_depth().

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

The local object_array 'stack' in get_shallow_commits() function does not free its dynamic elements before the function returns. As a result elements remain allocated and their reference forgotten.

Also note, that test 'fetching deepen beyond merged branch' added by 'shallow: handling fetch relative-deepen' patch fails without this correction in linux-leaks and linux-reftable-leaks test runs.

---
 shallow.c | 1 +
 1 file changed, 1 insertion(+)

index 55b9cd9d3f..497a25836b 100644
@@ -198,6 +198,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		}
 	}
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
+	object_array_clear(&stack);
 
 	return result;
 }

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may end up with some shallows that are hidden behind the reachable shallow commits. Added test 'fetching deepen beyond merged branch' exposes that behaviour.

An example showing the problem based on added test:
0. Whole initial git repo to be cloned from Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 (branch) five
   | * ecb578a four
   |/

* 0cb5d20 three
* 2b4e70d two
* 61ba98b one

1. Initial shallow clone --depth=3 (all good)
Shallows:
2b4e70da2a10e1d3231a0ae2df396024735601f1
ecb578a3cf37198d122ae5df7efed9abaca17144
Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a (grafted) four

* 0cb5d20 three
* 2b4e70d (grafted) two

2. Deepen shallow clone with fetch --deepen=1 (NOT OK) Shallows:
0cb5d204f4ef96ed241feb0f2088c9f4794ba758 61ba98be443fd51c542eb66585a1f6d7e15fcdae Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a four
   |/

* 0cb5d20 (grafted) three
---
Note that second shallow commit 61ba98be443fd51c542eb66585a1f6d7e15fcdae is not reachable.

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.

Same example showing the corrected second step:
2. Deepen shallow clone with fetch --deepen=1 (all good) Shallow:
61ba98be443fd51c542eb66585a1f6d7e15fcdae Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a four
   |/

* 0cb5d20 three
* 2b4e70d two
* 61ba98b (grafted) one

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 always sum 'data->deepen_relative' and 'depth' values, because 'data->deepen_relative' is always 0 in absolute deepening.
To avoid duplicating logic between get_shallows_depth() and get_shallow_commits(), get_shallow_commits() was modified so that it is used by get_shallows_depth().

---
 shallow.c             | 44 +++++++++++++++++--------
 shallow.h             |  1 +
 t/t5500-fetch-pack.sh | 23 +++++++++++++
 upload-pack.c         | 76 +++++--------------------------------------
 4 files changed, 63 insertions(+), 81 deletions(-)

index 497a25836b..1a32808865 100644
@@ -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;
+	int cur_depth = 0, cur_depth_shallow = 0;
 	struct commit_list *result = NULL;
 	struct object_array stack = OBJECT_ARRAY_INIT;
 	struct commit *commit = NULL;

@@ -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))
+						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;
+			}
+		} else {
+			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;
+			}
+			commit->object.flags |= not_shallow_flag;
 		}
-		commit->object.flags |= not_shallow_flag;
 		for (p = commit->parents, commit = NULL; p; p = p->next) {
 			int **depth_slot = commit_depth_at(&depths, p->item);
 			if (!*depth_slot) {

@@ -199,7 +214,8 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 	}
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
 	object_array_clear(&stack);
-
+	if (shallows && deepen_relative)
+		*deepen_relative = cur_depth_shallow;
 	return result;
 }
 

index ad591bd139..d1b3878635 100644
@@ -36,6 +36,7 @@ int commit_shallow_file(struct repository *r, struct shallow_lock *lk);
 void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);
 
 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);
 struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,
 						    int shallow_flag, int not_shallow_flag);

index 2677cd5faa..5a8b30e1fd 100755
@@ -955,6 +955,29 @@ 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 &&
+		git -C deepen.git rev-list --all >actual &&
+		for commit in $(sed "/^$/d" deepen.git/shallow)
+		do
+			test_grep "$commit" actual || exit 1
+		done
+	)
+'
+
 test_negotiation_algorithm_default () {
 	test_when_finished rm -rf clientv0 clientv2 &&
 	rm -rf server client &&

gitgitgadget

Yes, sure.

Of course.

Exactly, i merged the two functions with almost the same algorithm producing different results depending on additional input parameter shallows. When shallows passed only current maximum absolute depth is returned in the extra output parameter deepen_relative (if provided) and nothing else is done. The passed-in depth is not checked as it is irrelevant for this depth-measuring scenario.

True, i suppose you ment inline the function code instead of function call and not making the function inline?

Nicely put, i just wasn't (and still am not) confident enough to change code in a way that would potentially affect any other scenarios than fetching relative- deepen.

Thank you very much for the review and i'll try to address all your review points in the next patch version.

Best regards, Samo

I have additional dilemma regarding handling this in a single call. Wouldn't it be generally good/useful to have a separate function in shallow.c just for measuring current absolute depth instead of blending the measurement into get_shallow_commits()?

Thanks, Samo

I prepared another version which i am going to post shortly. It does make a single call to 'get_shallow_commits()' in upload_pack.c, however in shallow.c there is again a common internal function, which is called once for measuring current depth and then again to get a list shallow commits. I do not know how to perform current depth measurement and shallows list retrieval with new depth extended by the same measurement simultaneously. However i find it important to keep common algorithm in a single function for easier maintenance. I hope this is ok.

Best regards, Samo

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may endup with some shallows, that are behind the reachable ones. 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.

Samo Pogačnik (2):
shallow: free local object_array allocations
shallow: handling fetch relative-deepen

 shallow.c             | 73 ++++++++++++++++++++++++++++++++++++-------
 shallow.h             |  2 ++
 t/t5500-fetch-pack.sh | 23 ++++++++++++++
 upload-pack.c         | 72 ++----------------------------------------
 4 files changed, 88 insertions(+), 82 deletions(-)

base-commit: f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2121%2Fspog%2Ffix-fetch-deepen-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2121/spog/fix-fetch-deepen-v5
Pull-Request: https://github.com/git/git/pull/2121

Range-diff vs v4:

 1:  f8a8d077cd = 1:  f8a8d077cd shallow: free local object_array allocations
 2:  e9b20ae06f ! 2:  8d48ba9cd1 shallow: handling fetch relative-deepen
     @@ Commit message
      
          Signed-off-by: Samo Pogačnik <[email protected]>
      
     +    Fixing v4
     +
     +    Fixing v4 again
     +
       ## shallow.c ##
      @@ shallow.c: static void free_depth_in_slab(int **ptr)
       {
     @@ shallow.c: static void free_depth_in_slab(int **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)
     ++/*
     ++ * This is a common internal function that can either return a list of
     ++ * shallow commits or calculate the current maximum depth of a shallow
     ++ * repository, depending on the input parameters.
     ++ *
     ++ * Depth calculation is triggered by passing the `shallows` parameter.
     ++ * In this case, the computed depth is stored in `max_cur_depth` (if it is
     ++ * provided), and the function returns NULL.
     ++ *
     ++ * Otherwise, `max_cur_depth` remains unchanged and the function returns
     ++ * a list of shallow commits.
     ++ */
     ++static struct commit_list *get_shallows_or_depth(struct object_array *heads,
     ++				struct object_array *shallows, int *max_cur_depth,
     ++				int depth, int shallow_flag, int not_shallow_flag)
       {
     --	size_t i = 0;
     + 	size_t i = 0;
      -	int cur_depth = 0;
     -+	size_t i = 0, j;
      +	int cur_depth = 0, cur_depth_shallow = 0;
       	struct commit_list *result = NULL;
       	struct object_array stack = OBJECT_ARRAY_INIT;
     @@ shallow.c: struct commit_list *get_shallow_commits(struct object_array *heads, i
      -			commit = NULL;
      -			continue;
      +		if (shallows) {
     -+			for (j = 0; j < shallows->nr; j++)
     ++			for (size_t 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))
     ++					if (!cur_depth_shallow || cur_depth < cur_depth_shallow)
      +						cur_depth_shallow = cur_depth;
      +
      +			if ((is_repository_shallow(the_repository) && !commit->parents &&
     @@ shallow.c: struct commit_list *get_shallow_commits(struct object_array *heads, i
       			int **depth_slot = commit_depth_at(&depths, p->item);
       			if (!*depth_slot) {
      @@ shallow.c: struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
     - 	}
       	deep_clear_commit_depth(&depths, free_depth_in_slab);
       	object_array_clear(&stack);
     --
     -+	if (shallows && deepen_relative)
     -+		*deepen_relative = cur_depth_shallow;
     + 
     ++	if (shallows && max_cur_depth)
     ++		*max_cur_depth = cur_depth_shallow;
       	return result;
       }
       
     ++int get_shallows_depth(struct object_array *heads, struct object_array *shallows)
     ++{
     ++	int max_cur_depth = 0;
     ++	get_shallows_or_depth(heads, shallows, &max_cur_depth, 0, 0, 0);
     ++	return max_cur_depth;
     ++
     ++}
     ++
     ++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)
     ++{
     ++	if (shallows && deepen_relative) {
     ++		depth += get_shallows_depth(heads, shallows);
     ++	}
     ++	return get_shallows_or_depth(heads, NULL, NULL,
     ++				     depth, shallow_flag, not_shallow_flag);
     ++}
     ++
     + static void show_commit(struct commit *commit, void *data)
     + {
     + 	commit_list_insert(commit, data);
      
       ## shallow.h ##
      @@ shallow.h: int commit_shallow_file(struct repository *r, struct shallow_lock *lk);
     + /* rollback $GIT_DIR/shallow and reset stat-validity checks */
       void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);
       
     ++int get_shallows_depth(struct object_array *heads, struct object_array *shallows);
       struct commit_list *get_shallow_commits(struct object_array *heads,
     -+					struct object_array *shallows, int *deepen_relative,
     ++					struct object_array *shallows, int deepen_relative,
       					int depth, int shallow_flag, int not_shallow_flag);
       struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,
       						    int shallow_flag, int not_shallow_flag);
     @@ upload-pack.c: error:
       
      -static int get_reachable_list(struct upload_pack_data *data,
      -			      struct object_array *reachable)
     -+static void get_shallows_depth(struct upload_pack_data *data)
     - {
     +-{
      -	struct child_process cmd = CHILD_PROCESS_INIT;
      -	int i;
      -	struct object *o;
     @@ upload-pack.c: error:
      -out:
      -	child_process_clear(&cmd);
      -	return ret;
     -+	get_shallow_commits(&data->want_obj, &data->shallows,
     -+			    &data->deepen_relative, 0,
     -+			    SHALLOW, NOT_SHALLOW);
     - }
     - 
     +-}
     +-
       static int has_unreachable(struct object_array *src, enum allow_uor allow_uor)
     + {
     + 	struct child_process cmd = CHILD_PROCESS_INIT;
      @@ upload-pack.c: static void deepen(struct upload_pack_data *data, int depth)
       			struct object *object = data->shallows.objects[i].item;
       			object->flags |= NOT_SHALLOW;
     @@ upload-pack.c: static void deepen(struct upload_pack_data *data, int depth)
       		struct commit_list *result;
       
      -		result = get_shallow_commits(&data->want_obj, depth,
     -+		if (data->deepen_relative)
     -+			get_shallows_depth(data);
     -+
     -+		result = get_shallow_commits(&data->want_obj, NULL, NULL,
     -+					     data->deepen_relative + depth,
     ++		result = get_shallow_commits(&data->want_obj, &data->shallows,
     ++					     data->deepen_relative, depth,
       					     SHALLOW, NOT_SHALLOW);
       		send_shallow(data, result);
       		free_commit_list(result);

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

The local object_array 'stack' in get_shallow_commits() function does not free its dynamic elements before the function returns. As a result elements remain allocated and their reference forgotten.

Also note, that test 'fetching deepen beyond merged branch' added by 'shallow: handling fetch relative-deepen' patch fails without this correction in linux-leaks and linux-reftable-leaks test runs.

---
 shallow.c | 1 +
 1 file changed, 1 insertion(+)

index 55b9cd9d3f..497a25836b 100644
@@ -198,6 +198,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		}
 	}
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
+	object_array_clear(&stack);
 
 	return result;
 }

gitgitgadget

From: =?UTF-8?q?Samo=20Poga=C4=8Dnik?= <[email protected]>

When a shallowed repository gets deepened beyond the beginning of a merged branch, we may end up with some shallows that are hidden behind the reachable shallow commits. Added test 'fetching deepen beyond merged branch' exposes that behaviour.

An example showing the problem based on added test:
0. Whole initial git repo to be cloned from Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 (branch) five
   | * ecb578a four
   |/

* 0cb5d20 three
* 2b4e70d two
* 61ba98b one

1. Initial shallow clone --depth=3 (all good)
Shallows:
2b4e70da2a10e1d3231a0ae2df396024735601f1
ecb578a3cf37198d122ae5df7efed9abaca17144
Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a (grafted) four

* 0cb5d20 three
* 2b4e70d (grafted) two

2. Deepen shallow clone with fetch --deepen=1 (NOT OK) Shallows:
0cb5d204f4ef96ed241feb0f2088c9f4794ba758 61ba98be443fd51c542eb66585a1f6d7e15fcdae Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a four
   |/

* 0cb5d20 (grafted) three
---
Note that second shallow commit 61ba98be443fd51c542eb66585a1f6d7e15fcdae is not reachable.

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.

Same example showing the corrected second step:
2. Deepen shallow clone with fetch --deepen=1 (all good) Shallow:
61ba98be443fd51c542eb66585a1f6d7e15fcdae Graph:
* 033585d (HEAD -> main) Merge branch 'branch'

   |\
   | * 984f8b1 five
   | * ecb578a four
   |/

* 0cb5d20 three
* 2b4e70d two
* 61ba98b (grafted) one

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 always sum 'data->deepen_relative' and 'depth' values, because 'data->deepen_relative' is always 0 in absolute deepening.
To avoid duplicating logic between get_shallows_depth() and get_shallow_commits(), get_shallow_commits() was modified so that it is used by get_shallows_depth().

Fixing v4

Fixing v4 again

---
 shallow.c             | 72 +++++++++++++++++++++++++++++++++++--------
 shallow.h             |  2 ++
 t/t5500-fetch-pack.sh | 23 ++++++++++++++
 upload-pack.c         | 72 ++-----------------------------------------
 4 files changed, 87 insertions(+), 82 deletions(-)

index 497a25836b..a156006d88 100644
@@ -130,11 +130,24 @@ 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)
+/*
+ * This is a common internal function that can either return a list of
+ * shallow commits or calculate the current maximum depth of a shallow
+ * repository, depending on the input parameters.
+ *
+ * Depth calculation is triggered by passing the `shallows` parameter.
+ * In this case, the computed depth is stored in `max_cur_depth` (if it is
+ * provided), and the function returns NULL.
+ *
+ * Otherwise, `max_cur_depth` remains unchanged and the function returns
+ * a list of shallow commits.
+ */
+static struct commit_list *get_shallows_or_depth(struct object_array *heads,
+				struct object_array *shallows, int *max_cur_depth,
+				int depth, int shallow_flag, int not_shallow_flag)
 {
 	size_t i = 0;
-	int cur_depth = 0;
+	int cur_depth = 0, cur_depth_shallow = 0;
 	struct commit_list *result = NULL;
 	struct object_array stack = OBJECT_ARRAY_INIT;
 	struct commit *commit = NULL;

@@ -168,16 +181,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 (size_t 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)
+						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;
+			}
+		} else {
+			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;
+			}
+			commit->object.flags |= not_shallow_flag;
 		}
-		commit->object.flags |= not_shallow_flag;
 		for (p = commit->parents, commit = NULL; p; p = p->next) {
 			int **depth_slot = commit_depth_at(&depths, p->item);
 			if (!*depth_slot) {

@@ -200,9 +227,30 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 	deep_clear_commit_depth(&depths, free_depth_in_slab);
 	object_array_clear(&stack);
 
+	if (shallows && max_cur_depth)
+		*max_cur_depth = cur_depth_shallow;
 	return result;
 }
 
+int get_shallows_depth(struct object_array *heads, struct object_array *shallows)
+{
+	int max_cur_depth = 0;
+	get_shallows_or_depth(heads, shallows, &max_cur_depth, 0, 0, 0);
+	return max_cur_depth;
+
+}
+
+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)
+{
+	if (shallows && deepen_relative) {
+		depth += get_shallows_depth(heads, shallows);
+	}
+	return get_shallows_or_depth(heads, NULL, NULL,
+				     depth, shallow_flag, not_shallow_flag);
+}
+
 static void show_commit(struct commit *commit, void *data)
 {
 	commit_list_insert(commit, data);

index ad591bd139..e3f0df57ad 100644
@@ -35,7 +35,9 @@ int commit_shallow_file(struct repository *r, struct shallow_lock *lk);
 /* rollback $GIT_DIR/shallow and reset stat-validity checks */
 void rollback_shallow_file(struct repository *r, struct shallow_lock *lk);
 
+int get_shallows_depth(struct object_array *heads, struct object_array *shallows);
 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);
 struct commit_list *get_shallow_commits_by_rev_list(struct strvec *argv,
 						    int shallow_flag, int not_shallow_flag);

index 2677cd5faa..5a8b30e1fd 100755
@@ -955,6 +955,29 @@ 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 &&
+		git -C deepen.git rev-list --all >actual &&
+		for commit in $(sed "/^$/d" deepen.git/shallow)
+		do
+			test_grep "$commit" actual || exit 1
+		done
+	)
+'
+
 test_negotiation_algorithm_default () {
 	test_when_finished rm -rf clientv0 clientv2 &&
 	rm -rf server client &&

gitgitgadget

We didn't see any response to the latest round, and the comments on previous rounds seem to have been addressed. Is this ready to be merged down to 'next' now?

Thanks.