Re: [PATCH] commit-reach: avoid is_dependant_of() shim

[Derrick Stolee <stolee@gmail.com>]

On 6/23/2020 4:17 AM, Carlo Marcelo Arenas Belón wrote:
> d91d6fbf26 (commit-reach: create repo_is_descendant_of(), 2020-06-17)
> adds a repository aware version of is_dependant_of() and a backward
> compatibility shim that is barelly used.> 
> Update all callers to directly use the new repo_is_dependant_of()
> function instead.

In your commit message:

s/dependant/descendant/
s/barelly/barely/

> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/pull.c        |  3 ++-
>  commit-reach.c        | 16 ++++++----------
>  commit-reach.h        |  4 +++-
>  t/helper/test-reach.c |  2 +-
>  4 files changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 8e6572d305..babc6a4e36 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1025,7 +1025,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  			commit_list_insert(head, &list);
>  			merge_head = lookup_commit_reference(the_repository,
>  							     &merge_heads.oid[0]);
> -			if (is_descendant_of(merge_head, list)) {
> +			if (repo_is_descendant_of(the_repository,
> +							merge_head, list)) {

nit: left-align "merge_head" with "the_repository". Should look like this:

+			if (repo_is_descendant_of(the_repository,
+						  merge_head, list)) {

>  				/* we can fast-forward this without invoking rebase */
>  				opt_ff = "--ff-only";
>  				ran_ff = 1;
> diff --git a/commit-reach.c b/commit-reach.c
> index 1761217663..82c73171dd 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -283,9 +283,9 @@ struct commit_list *repo_get_merge_bases(struct repository *r,
>  /*
>   * Is "commit" a descendant of one of the elements on the "with_commit" list?
>   */
> -static int repo_is_descendant_of(struct repository *r,
> -				 struct commit *commit,
> -				 struct commit_list *with_commit)
> +int repo_is_descendant_of(struct repository *r,
> +			  struct commit *commit,
> +			  struct commit_list *with_commit)
>  {
>  	if (!with_commit)
>  		return 1;
> @@ -310,11 +310,6 @@ static int repo_is_descendant_of(struct repository *r,
>  	}
>  }
>  
> -int is_descendant_of(struct commit *commit, struct commit_list *with_commit)
> -{
> -	return repo_is_descendant_of(the_repository, commit, with_commit);
> -}
> -
>  /*
>   * Is "commit" an ancestor of one of the "references"?
>   */
> @@ -433,7 +428,8 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)
>  		return 0;
>  
>  	commit_list_insert(old_commit, &old_commit_list);
> -	ret = is_descendant_of(new_commit, old_commit_list);
> +	ret = repo_is_descendant_of(the_repository,
> +					new_commit, old_commit_list);

nit: align the whitespace so "new_commit" is left-aligned with the start
of "the_repository" in the line above. It should look like this:

+	ret = repo_is_descendant_of(the_repository,
+				    new_commit, old_commit_list);

>  	free_commit_list(old_commit_list);
>  	return ret;
>  }
> @@ -554,7 +550,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,
>  {
>  	if (filter->with_commit_tag_algo)
>  		return contains_tag_algo(commit, list, cache) == CONTAINS_YES;
> -	return is_descendant_of(commit, list);
> +	return repo_is_descendant_of(the_repository, commit, list);
>  }
>  
>  static int compare_commits_by_gen(const void *_a, const void *_b)
> diff --git a/commit-reach.h b/commit-reach.h
> index 99a43e8b64..b49ad71a31 100644
> --- a/commit-reach.h
> +++ b/commit-reach.h
> @@ -27,7 +27,9 @@ struct commit_list *repo_get_merge_bases_many_dirty(struct repository *r,
>  
>  struct commit_list *get_octopus_merge_bases(struct commit_list *in);
>  
> -int is_descendant_of(struct commit *commit, struct commit_list *with_commit);
> +int repo_is_descendant_of(struct repository *r,
> +			  struct commit *commit,
> +			  struct commit_list *with_commit);
>  int repo_in_merge_bases(struct repository *r,
>  			struct commit *commit,
>  			struct commit *reference);
> diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
> index a0272178b7..1d640f4757 100644
> --- a/t/helper/test-reach.c
> +++ b/t/helper/test-reach.c
> @@ -108,7 +108,7 @@ int cmd__reach(int ac, const char **av)
>  	else if (!strcmp(av[1], "in_merge_bases"))
>  		printf("%s(A,B):%d\n", av[1], in_merge_bases(A, B));
>  	else if (!strcmp(av[1], "is_descendant_of"))
> -		printf("%s(A,X):%d\n", av[1], is_descendant_of(A, X));
> +		printf("%s(A,X):%d\n", av[1], repo_is_descendant_of(r, A, X));

This is the most subtle of the changes, but it is done correctly.

This is a worthwhile patch. Some of the callers _look_ more complicated,
but overall this simplifies the codebase. Further: it pushes more
the_repository references higher up the stack.

Thanks,
-Stolee