Re: [GSoC][PATCH v4 0/7] t: port reftable/pq_test.c to the unit testing framework

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 14, 2024, 17:40 UTC
Message-ID: <xmqqo783fmo9.fsf@gitster.g>
In-Reply-To: <20240614095136.12052-1-chandrapratap3519@gmail.com>

Chandra Pratap <chandrapratap3519@gmail.com> writes:

Show 21 quoted lines

> In the recent codebase update (commit 8bf6fbd, 2023-12-09), a new unit
> testing framework written entirely in C was introduced to the Git project
> aimed at simplifying testing and reducing test run times.
> Currently, tests for the reftable refs-backend are performed by a custom
> testing framework defined by reftable/test_framework.{c, h}. Port
> reftable/pq_test.c to the unit testing framework and improve upon
> the ported test.
>
> The first two patches in the series are preparatory cleanup, the third patch
> moves the test to the unit testing framework, and the rest of the patches
> improve upon the ported test.
>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> ---
> Changes in v4:
> - Add a commit message for the second patch
>
> CI/PR for v4: https://github.com/gitgitgadget/git/pull/1745

Hmph.

A larger question is how pristine we want to keep the "imported source" that is reftable/* stuff.

Obviously we are getting rid of some parts of it (like pq_test.c here) so our "fork" will no longer be usable, without pulling some of the stuff our project offers from outside that directory, by outside people. This concern is shared with other topics we saw recently to move the unit tests bundled with the reftable library to t/unit-tests/.

But this series brings in another twist that makes it a larger question. If we are willing to butcher our copy of reftable library to a shape that cannot be reused by outside folks, do we still want to use reftable/pq.c instead of getting rid of it and use the other prio queue implementation we use elsewhere in our system? If that will be the longer term direction we want to go, then porting the unit tests for reftable/pq.c may end up being a wasted effort.

So, I dunno.

Show 37 quoted lines

> Chandra Pratap(7):
> reftable: remove unncessary curly braces in reftable/pq.c
> reftable: change the type of array indices to 'size_t' in
> t: move reftable/pq_test.c to the unit testing framework
> t-reftable-pq: make merged_iter_pqueue_check() static
> t-reftable-pq: make merged_iter_pqueue_check() callable
> t-reftable-pq: add test for index based comparison
> t-reftable-pq: add tests for merged_iter_pqueue_top()
>
> Makefile                     |   2 +-
> reftable/pq.c                |  29 +++--------
> reftable/pq.h                |   1 -
> reftable/pq_test.c           |  74 ----------------------------
> t/helper/test-reftable.c     |   1 -
> t/unit-tests/t-reftable-pq.c | 155 +++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 166 insertions(+), 96 deletions(-)
>
> Range-diff against v3:
> 1:  3c333e7770 ! 1:  1873fb02ce reftable: change the type of array indices to 'size_t' in reftable/pq.c
>     @@ Metadata
>       ## Commit message ##
>          reftable: change the type of array indices to 'size_t' in reftable/pq.c
>      
>     +    The variables 'i', 'j', 'k' and 'min' are used as indices for
>     +    'pq->heap', which is an array. Additionally, 'pq->len' is of
>     +    type 'size_t' and is often used to assign values to these
>     +    variables. Hence, change the type of these variables from 'int'
>     +    to 'size_t'.
>     +
>          Mentored-by: Patrick Steinhardt <ps@pks.im>
>          Mentored-by: Christian Couder <chriscool@tuxfamily.org>
>          Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> 2:  bf547f705a = 2:  3cccf8266a t: move reftable/pq_test.c to the unit testing framework
> 3:  7dd3a2b27f = 3:  4b63849694 t-reftable-pq: make merged_iter_pqueue_check() static
> 4:  c803e7adfc = 4:  3698a7189f t-reftable-pq: make merged_iter_pqueue_check() callable by reference
> 5:  0b03f3567d = 5:  d58c8f709e t-reftable-pq: add test for index based comparison
> 6:  0cdfa6221e = 6:  69521f0ff7 t-reftable-pq: add tests for merged_iter_pqueue_top()