"Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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. <xmqq1q9ybsnl.fsf@gitster.g>),
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;
> };
