Re: [PATCH v3 02/10] trailer: move interpret_trailers() to interpret-trailers.c

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 31, 2024, 18:54 UTC
Message-ID: <xmqqy1c55o6a.fsf@gitster.g>
In-Reply-To: <cafa39d10489f90897227a244e012457989a7710.1706664145.git.gitgitgadget@gmail.com>

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

> From: Linus Arver <linusa@google.com>
>
> The interpret-trailers.c builtin is the only place we need to call
> interpret_trailers(), so move its definition there.

A few helper functions that are only called by interpret_trailers() are also moved, naturally. I would have less surprised to see the addtion near the beginning of builtin/interpret-trailers.c if this part said:

    ..., so move its definition there, together with a few helper
    functions called only by it, and remove its external declaration
    from <trailer.h>.

> Delete the corresponding declaration from trailer.h, which then forces
> us to expose the working innards of that function.

This was a bit confusing, at least to me. The reason why several other helper functions that are called by interpret_trailers() need to be declared in trailer.h is not because the declaration of interpret_trailers() is deleted from trailer.h but that is how I read the above.

    Several helper functions that are called by interpret_trailers()
    remain in trailer.c because other callers in the same file still
    call them, so add declaration for them to <trailer.h>.

Show 13 quoted lines

> This enriches
> trailer.h with a more granular API, which can then be unit-tested in the
> future (because interpret_trailers() by itself does too many things to
> be able to be easily unit-tested).
>
> Take this opportunity to demote some file-handling functions out of the
> trailer API implementation, as these have nothing to do with trailers.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
>  builtin/interpret-trailers.c |  94 +++++++++++++++++++++++++++
>  trailer.c                    | 120 ++++-------------------------------
>  trailer.h                    |  20 +++++-

Together with the preparatory renaming in the previous step, this made quite a pleasant read. Thanks.