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

[Junio C Hamano <gitster@pobox.com>]

"Samo Pogačnik via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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



> 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.
>
> Signed-off-by: Samo Pogačnik <samo_pogacnik@t-2.net>
> ---
>  shallow.c             | 44 +++++++++++++++++--------
>  shallow.h             |  1 +
>  t/t5500-fetch-pack.sh | 23 +++++++++++++
>  upload-pack.c         | 76 +++++--------------------------------------
>  4 files changed, 63 insertions(+), 81 deletions(-)
>
> 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;
> +	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;
>  }
>  
> diff --git a/shallow.h b/shallow.h
> index ad591bd139..d1b3878635 100644
> --- a/shallow.h
> +++ b/shallow.h
> @@ -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);
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 2677cd5faa..5a8b30e1fd 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -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 &&
> 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)
>  {
> -	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;
> -		}
> -	}
> -	for (i = get_max_object_index(the_repository); 0 < i; i--) {
> -		o = get_indexed_object(the_repository, i - 1);
> -		if (o && o->type == OBJ_COMMIT &&
> -		    (o->flags & TMP_MARK)) {
> -			add_object_array(o, NULL, reachable);
> -				o->flags &= ~TMP_MARK;
> -		}
> -	}
> -	close(cmd.out);
> -
> -	if (finish_command(&cmd)) {
> -		ret = -1;
> -		goto out;
> -	}
> -
> -	ret = 0;
> -
> -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)
> @@ -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);
> +
> +		result = get_shallow_commits(&data->want_obj, NULL, NULL,
> +					     data->deepen_relative + depth,
>  					     SHALLOW, NOT_SHALLOW);
>  		send_shallow(data, result);
>  		free_commit_list(result);