Junio C Hamano <gitster@xxxxxxxxx> writes:
> "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.
Added to commit message for v4, thanks.
> 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.
Having gotten my hands dirty with the pimpl idiom, I think possibly the
best thing about it is that the compiler can tell me exactly where
every Git subsystem outside of trailer.c is accessing members inside the
(newly private) struct. So it's great for checking existing usage of how
things are used already, day-to-day. Not being well-versed in shortlog
or sequencer internals, these warnings from the compiler for these
functions (trying to peer through the opaque pointer) have been very
informative.
I think it would be reasonable to drop the idiom if the additional
performance costs (pointer dereference, allocation/deallocation) are too
much to bear. For the trailer subsystem I don't see an immediate need to
focus on performance though, so I think it's fine for now.
> 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.
That's true. But that would only work for trailer API users inside the
Git codebase. Eventually I envision projects external to Git using
<trailer.h>, and in that scenario we would _really_ not want to let
these external users peek inside structs with members named "private" or
"internal" in name only. Murphy's law suggests those external users will
(naughtily) depend on the internals, and that would place an undue
burden on us if we ever wanted to make large changes to these (public)
structs because we might break them.
Another thing to consider is that if our API users ever want to get more
flexibility out of it, they would immediately come to us if we used the
pimpl idiom (our users are bound by exactly how we define the API). If
the structs were public, then they would be free to implement new
functionality to extend the API on their own, without ever letting us
know. If there are folks making small (unofficial) extensions to the
API, I want that to happen _in_ this project, not outside.
In summary what I'm saying is that this idiom gives very strong guidance
for desining an API for both Git-internal and external-to-Git usage. I
think for at least the trailer subsystem it's worth trying out. I've
pointed out some of the same points in the cover letter also.
>> @@ -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?
Nice catch!
> 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.
I've promoted this change into a bugfix patch as the very first patch
for the next reroll. Thanks.
>> 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.
I am very surprised with your response here, because I was expecting the
complete opposite reaction from reviewers --- something like
Hmph, we have to write accessor functions now when before we could
just reach inside the struct directly? Isn't this just adding
needless verbosity?
(perhaps with more dissatisfaction). Something tells me you meant "bad
change" but accidentally wrote "good change". Am I correct?
