Re: [PATCH v2 6/6] trailer: use offsets for trailer_start/trailer_end

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 11, 2023, 19:01 UTC
Message-ID: <xmqqmsxsv8ik.fsf@gitster.g>
In-Reply-To: <0463066ebe0889b72b6a1f6c344f2de127458391.1694240177.git.gitgitgadget@gmail.com>

"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 9 quoted lines

> From: Linus Arver <linusa@google.com>
>
> 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, and have to make the readers aware of the local convention you happened to have picked with a comment like ...

Show 13 quoted lines

> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  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.

So, I dunno.