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

From: Junio C Hamano <[email-masked]>
Date: Thu, 01 Feb 2024 10:49:56 -0800

"Linus Arver via GitGitGadget" <[email-masked]> writes:

> 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. <[email-masked]>),
we should be able to catch external accesses without having to hide
the implementation details via an extra pointer dereference.

> @@ -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.

> 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.

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