Re: [PATCH v2 2/4] t-reftable-readwrite: use free_names() instead of a for loop

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 9, 2024, 18:57 UTC
Message-ID: <xmqqjzgpcymq.fsf@gitster.g>
In-Reply-To: <20240809111312.4401-3-chandrapratap3519@gmail.com>

Chandra Pratap <chandrapratap3519@gmail.com> writes:

> free_names() as defined by reftable/basics.{c,h} frees a NULL
> terminated array of malloced strings along with the array itself.
> Use this function instead of a for loop to free such an array.

Going back to [1/4], the headers included in this test looked like this:

    -#include "system.h"
    -
    -#include "basics.h"
    -#include "block.h"
    -#include "blocksource.h"
    -#include "reader.h"
    -#include "record.h"
    -#include "test_framework.h"
    -#include "reftable-tests.h"
    -#include "reftable-writer.h"
    +#include "test-lib.h"
    +#include "reftable/reader.h"
    +#include "reftable/blocksource.h"
    +#include "reftable/reftable-error.h"
    +#include "reftable/reftable-writer.h"

I found this part a bit curious, perhaps because I was not involved in either reftable/ or unit-tests/ development. So I may be asking a stupid question, but is it intended that some headers like "block.h" and "record.h" are no longer included?

It is understandable that inclusion of "test-lib.h" is new (and needs to be there to work as part of t/unit-tests/), and the leading directory name "reftable/" added to header files are also justified, of course. But if you depend on "basics.h" and do not include it, that does not sound like the most hygenic thing to do, at least to me.

The code changes themselves look good; I can see that the implementation of free_names() in reftable/basics.c safely replaces these loops. There is a slight behaviour difference that names[] that was fed to reftable_iterator_seek_ref() earlier goes away before the iterator is destroyed, but _seek_ref() does not retain the names[0] argument in the iterator object, so that is OK.

Thanks.