Re: [PATCH v3 05/10] trailer: make trailer_info struct private

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 1, 2024, 18:49 UTC
Message-ID: <xmqqplxgyq7f.fsf@gitster.g>
In-Reply-To: <c19c1dcc592186d8da2857692f4ebbfe35adfac0.1706664145.git.gitgitgadget@gmail.com>

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

Show 8 quoted lines

> Make this struct private by putting its definition inside trailer.c.
> This has two benefits:
>
>   (1) it makes the surface area of the public facing
>       interface (trailer.h) smaller, and
>
>   (2) external API users are unable to peer inside this struct (because
>       it is only ever exposed as an opaque pointer).

At the cost of an extra pointer dereference every time the member of the struct is accessed, plus the cost of allocation/deallocation.

Which may not be a huge deal, but I wonder if the approach to name the member of the outer struct with "private" that seems to be used in other parts of the code (e.g. the .private_size member in the hashmap structure, the .refs_private member in the repository structure) or even a documented convention (e.g. raw_object_store), might be more appropriate here. If Coccinelle works well (which we may be having some trouble with --- cf. <xmqq1q9ybsnl.fsf@gitster.g>), we should be able to catch external accesses without having to hide the implementation details via an extra pointer dereference.

Show 12 quoted lines

> @@ -176,11 +176,12 @@ static void interpret_trailers(const struct process_trailer_options *opts,
>  	strbuf_release(&trailer_block);
>  
>  	free_trailers(&head);
> -	trailer_info_release(&info);
>  
>  	/* Print the lines after the trailers as is */
>  	if (!opts->only_trailers)
> -		fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
> +		fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
> +
> +	trailer_info_release(info);

Interesting. Is this an indenendent bugfix even if we decided not to take this patch? No, I have not fully decided to be negative on the move this entire patch makes (even though I am leaning towards saying so). Just hypothetically, even if we wanted to keep "info" here as a structure and not a pointer to an opaque structure, doesn't this hunk fix a real bug?

Well, technically, not quite, because the members referenced in that if (.only_trailers) block are still live in the info struct. But it still smells wrong to access info.* after calling _release() on it, and this fix should come before "info" is turned from an instance to a pointer, I would say.

Show 20 quoted lines

> diff --git a/trailer.h b/trailer.h
> index a7599067acc..e19ddf84e64 100644
> --- a/trailer.h
> +++ b/trailer.h
> @@ -4,6 +4,8 @@
>  #include "list.h"
>  #include "strbuf.h"
>  
> +struct trailer_info;
> +
>  enum trailer_where {
>  	WHERE_DEFAULT,
>  	WHERE_END,
> @@ -29,27 +31,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
>  int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
>  int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
>  
> +size_t trailer_block_start(struct trailer_info *info);
> +size_t trailer_block_end(struct trailer_info *info);
> +int blank_line_before_trailer_block(struct trailer_info *info);

And we need new accessors, which is a good change regardless of the answer to the "do we really want an extra pointer dereference? Shouldn't the existing 'private' and 'internal' label we see below sufficient?" question.

Show 9 quoted lines

> @@ -142,7 +123,7 @@ struct trailer_iterator {
>  
>  	/* private */
>  	struct {
> -		struct trailer_info info;
> +		struct trailer_info *info;
>  		size_t cur;
>  	} internal;
>  };