Re: [PATCH] commit-reach: remove get_reachable_subset()

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

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> get_reachable_subset() was introduced in fcb2c0769d (2018-11-02)
> for add_missing_tags() in remote.c. tips_reachable_from_bases()
> was added in cbfe360b14 (2023-03-20) as part of the ahead-behind
> series. The two were never consolidated.

Good finding.  It is curious to see that these were from the same
author.

> ... Without generation numbers, some edge cases
> may be slower with DFS instead of BFS since the date-ordered
> prio_queue naturally stays near the top of the graph, but this
> should not matter in practice

"should not matter in practice" because...?

> -- worst case both visit the full
> graph down from the bases.

And of course the worst case scenario is by definition not a typical
case that appear in practice, so it does not make a good explanation
for "should not matter in practice".

> The flag in remote.c changes from 1 (bit 0) to TMP_MARK (bit 4)
> because tips_reachable_from_bases() uses SEEN (bit 0) internally.
> TMP_MARK is already used for deduplication earlier in the same
> function and is cleared before the reachability check.

And tips_reachable_from_bases() clears SEEN at the end as expected.

>  commit-reach.c        | 73 -------------------------------------------
>  commit-reach.h        | 13 --------
>  remote.c              | 19 ++++++-----
>  t/helper/test-reach.c | 39 +++++++++++------------
>  t/t6600-test-reach.sh | 18 +++++------
>  5 files changed, 36 insertions(+), 126 deletions(-)

Yay, a lot of deletions ;-)

> diff --git a/t/t6600-test-reach.sh b/t/t6600-test-reach.sh
> index b5b314e570..51b140a539 100755
> --- a/t/t6600-test-reach.sh
> +++ b/t/t6600-test-reach.sh
> @@ -391,7 +391,7 @@ test_expect_success 'rev-list: symmetric difference topo-order' '
>  	run_all_modes git rev-list --topo-order commit-3-8...commit-6-6
>  '
>  
> -test_expect_success 'get_reachable_subset:all' '
> +test_expect_success 'tips_reachable_from_bases:all' '
>  	cat >input <<-\EOF &&
>  	X:commit-9-1
>  	X:commit-8-3
> @@ -403,15 +403,15 @@ test_expect_success 'get_reachable_subset:all' '
>  	Y:commit-5-6
>  	EOF
>  	(
> -		echo "get_reachable_subset(X,Y)" &&
> +		echo "tips_reachable_from_bases(X,Y)" &&
>  		git rev-parse commit-3-3 \
>  			      commit-1-7 \
>  			      commit-5-6 | sort
>  	) >expect &&
> -	test_all_modes get_reachable_subset
> +	test_all_modes tips_reachable_from_bases
>  '
>  
> -test_expect_success 'get_reachable_subset:some' '
> +test_expect_success 'tips_reachable_from_bases:some' '
>  	cat >input <<-\EOF &&
>  	X:commit-9-1
>  	X:commit-8-3
> @@ -422,14 +422,14 @@ test_expect_success 'get_reachable_subset:some' '
>  	Y:commit-5-6
>  	EOF
>  	(
> -		echo "get_reachable_subset(X,Y)" &&
> +		echo "tips_reachable_from_bases(X,Y)" &&
>  		git rev-parse commit-3-3 \
>  			      commit-1-7 | sort
>  	) >expect &&
> -	test_all_modes get_reachable_subset
> +	test_all_modes tips_reachable_from_bases
>  '
>  
> -test_expect_success 'get_reachable_subset:none' '
> +test_expect_success 'tips_reachable_from_bases:none' '
>  	cat >input <<-\EOF &&
>  	X:commit-9-1
>  	X:commit-8-3
> @@ -439,8 +439,8 @@ test_expect_success 'get_reachable_subset:none' '
>  	Y:commit-7-6
>  	Y:commit-2-8
>  	EOF
> -	echo "get_reachable_subset(X,Y)" >expect &&
> -	test_all_modes get_reachable_subset
> +	echo "tips_reachable_from_bases(X,Y)" >expect &&
> +	test_all_modes tips_reachable_from_bases
>  '
>  
>  test_expect_success 'for-each-ref ahead-behind:linear' '
>
> base-commit: 600fe743028cbfb640855f659e9851522214bc0b

Initially I feared that changes to the test script were a sign of
need to adjuist to behaviour changes, but as the proposed log
message explained, all of the above changes are about the name of
the function being used and tested, which makes sense.

Thanks.