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.
>> */
>
> ... this, simply because there is no obvious sentinel value for an
> unsigned integral type; even if you count MAX_ULONG and its friends,
> they are not as obvious as NULL for pointer types.
I agree that there is no trustworthy sentinel value for an unsigned
integral type.
On the other hand, we never used NULL as a sentinel value before even
when they were const char pointers --- the current comment for these
fields which say ...
If there is no trailer block found, these 2 pointers point to the end of the
input string.
... sounds somewhat arbitrary to me (and I don't think we care about
this property in trailer.c, and AFAICS it's also not something that
consumers should be aware of). Consumers of the trailer_info struct
could also just see if "info->trailer_nr > 0" to check whether any
trailers were found, although if we're merging Patch 1 [1] of this
series the consumers will not have easy access any more to any of the
trailer_info fields, and they should instead be using a public-facing
function that does the "were trailers found" check.
> So, I dunno.
If the "we don't use NULL sentinel values currently anyway" argument is
convincing enough, I'm happy to add this to the commit message on a
reroll. But I'm also OK with dropping this patch. Thoughts?
[1] https://lore.kernel.org/git/pull.1563.git.1691211879.gitgitgadget@xxxxxxxxx/T/#m8f1b5f1eb346331658c8c7b3e057a4ee31223664
