Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> +UNIT_TEST_PROGRAMS += t-trailer
>> UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
>> UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
>> UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
>
> Totally offtopic, but does it bother folks who are interested in
> adding more unit tests that they do not seem to interact very well
> with GIT_SKIP_TESTS environment variable?
FWIW I am not bothered (not that I've actually used GIT_SKIP_TESTS)
mainly because the unit tests finish so quickly.
>
>> diff --git a/t/unit-tests/t-trailer.c b/t/unit-tests/t-trailer.c
>> new file mode 100644
>> index 00000000000..147a51b66b9
>> --- /dev/null
>> +++ b/t/unit-tests/t-trailer.c
>> @@ -0,0 +1,175 @@
>> +#include "test-lib.h"
>> +#include "trailer.h"
>> +
>> +static void t_trailer_iterator(const char *msg, size_t num_expected_trailers)
>> +{
>> + struct trailer_iterator iter;
>> + size_t i = 0;
>> +
>> + trailer_iterator_init(&iter, msg);
>> + while (trailer_iterator_advance(&iter)) {
>> + i++;
>> + }
>
> Unnecessary {braces} around a single-statement block?
Gah, I blame writing too much Go. Will fix.
I also wonder if there's a C linter that could catch this... I am not
very familiar with C tooling. Would be great to run that in CI (GGG).
>> + trailer_iterator_release(&iter);
>> +
>> + check_uint(i, ==, num_expected_trailers);
>> +}
>> +
>> +static void run_t_trailer_iterator(void)
>> +{
>> + static struct test_cases {
>> + const char *name;
>> + const char *msg;
>> + size_t num_expected_trailers;
>
> This is more like number of lines in the trailer block, not
> limiting its count only to true trailers, no?
Yes, but to be even more precise, it would be the number of trailer
objects in the trailer block (a single trailer could be folded over
multiple lines). Will update to "num_expected_objects".
>
>> + } tc[] = {
>> ...
>> + {
>> + "with non-trailer lines in trailer block",
>> + "subject: foo bar\n"
>> + "\n"
>> + /*
>> + * Even though this trailer block has a non-trailer line
>> + * in it, it's still a valid trailer block because it's
>> + * at least 25% trailers and is Git-generated.
>> + */
>> + "not a trailer line\n"
>> + "not a trailer line\n"
>> + "not a trailer line\n"
>> + "Signed-off-by: x\n",
>> + 1
>> + },
>
> It is OK to leave it num_expected_trailers in this step and then
> rename it when you update this "1" (number of real trailer lines)
> to "4" (number of lines in the trailer block).
>
> I wonder if you'd want to make more data available to the test. At
> least it would be more useful if the number of true trailer lines
> and the number of lines in the trialer block are available
> separately.
I purposely did the simplest test possible in order to keep the patch
simple. Totally OK with expanding the data available to the test though,
if you'd prefer that (although that could also be in a separate series
later when we start converting some of the existing shell tests to these
unit tests).
> The interface into the trailers that is being tested by this code is
> "the caller repeatedly calls the iterator, and the caller can
> inspect the iterator's state available as its .raw, .key and .val
> members and use them as it sees fit", so you could check, if you
> wanted to, the following given the above sample data:
>
> * the first iteration finds no key/value pair (optionally, the
> contents found in the .raw member is as expected).
> * the second iteration finds no key/value pair (ditto).
> * the third iteration finds no key/value pair (ditto).
> * the fourth iteration finds key="Signed-off-by" value="x".
> * there is no fifth iteration.
>
> but the current code only checks the last condition and nothing
> else. I dunno.
Yeah, this sounds like the natural thing to do. Basically have an exact
list of "this is the linked list of trailer objects I expect to see
after parsing is complete".
I do plan on making the trailer iterator struct private in a future
series though, so maybe it's best to do the above after that series (to
avoid churn)? IDK.
@Christian thoughts?
