Linus Arver <linusa@xxxxxxxxxx> writes:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
>> "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>>
>>> From: Linus Arver <linusa@xxxxxxxxxx>
>>>
>>> Previously these fields in the trailer_info struct were of type "const
>>> char *" and pointed to positions in the input string directly (to the
>>> start and end positions of the trailer block).
>>>
>>> Use offsets to make the intended usage less ambiguous. We only need to
>>> reference the input string in format_trailer_info(), so update that
>>> function to take a pointer to the input.
>>
>> Hmm, I am not sure if this is an improvement. If the underlying
>> buffer can be reallocated (to grow), the approach to use the offsets
>> certainly is easier to deal with, as they will stay valid even after
>> such a reallocation. But you lose the obvious sentinel value NULL
>> that can mean something special
>
> True.
>
>> and have to make the readers aware
>> of the local convention you happened to have picked with a comment
>> like ...
>>
>>> Signed-off-by: Linus Arver <linusa@xxxxxxxxxx>
>>> ---
>>> trailer.c | 17 ++++++++---------
>>> trailer.h | 7 +++----
>>> 2 files changed, 11 insertions(+), 13 deletions(-)
>>> ...
>>> /*
>>> - * Pointers to the start and end of the trailer block found. If there
>>> - * is no trailer block found, these 2 pointers point to the end of the
>>> - * input string.
>>> + * Offsets to the trailer block start and end positions in the input
>>> + * string. If no trailer block is found, these are set to 0.
>>> */
I've just realized that the new comment "If no trailer block is found,
these are set to 0" is perhaps not always true in the current version
of this patch. This is because this patch did a mechanical type
conversion of the old fields from "const char *" to "size_t", and we
still run
trailer_end = find_trailer_end(str, trailer_search_boundary);
trailer_start = find_trailer_start(str, trailer_end);
inside trailer_info_get() (not visible in the patch context lines). I
will update this patch to make the comment "If no trailer block is found,
these are set to 0" true.
