Re: [PATCH v4 00/28] Enrich Trailer API

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 12, 2024, 23:37 UTC
Message-ID: <CAP8UFD3qLGua5PTA+i29N+yJH8iKVBPwUn=S606B2E+s959JFQ@mail.gmail.com>
In-Reply-To: <pull.1632.v4.git.1707196348.gitgitgadget@gmail.com>

On Tue, Feb 6, 2024 at 6:12 AM Linus Arver via GitGitGadget <gitgitgadget@gmail.com> wrote:

>
> This patch series is the first 10 patches of a larger cleanup/bugfix series
> (henceforth "larger series") I've been working on.

There are now 28 patches in this series.

I took a look at all of them, and I think that this series should be split into 4 or more series.

For example perhaps one series until the "trailer: move interpret_trailers() to interpret-trailers.c" patch, then another one until "trailer: finish formatting unification". etc.

Also I think it might be possible to avoid some test failures introduced by some patches. If it's not possible, I agree with Junio that it would be nice if the failing tests were changed to use 'test_expect_failure'.

Also it seems to me that many patches towards the end of this series should be split.

Show 5 quoted lines

> The main goal of this
> series is to begin the process of "libifying" the trailer API. By "API" I
> mean the interface exposed in trailer.h. The larger series brings a number
> of additional cleanups (exposing and fixing some bugs along the way), and
> builds on top of this series.

[...]

> With the libification-focused goals out of the way, let's turn to this patch
> series in more detail.

I like the goal of libifying Git the trailer API, and the way you want to do it seems reasonable to me.

[...]

> In summary this series breaks up "process_trailers()" into smaller pieces,
> exposing many of the parts relevant to trailer-related processing in
> trailer.h. This will force us to eventually introduce unit tests for these
> API functions, but that is a good thing for API stability.

I am a bit sad that this series doesn't introduce unit tests using the new test framework in C yet. I understand that this series is mostly a big refactoring and maybe it's better to introduce unit tests only when the refactoring is finished though. Anyway I hope the next series will introduce such tests.

Show 6 quoted lines

> In the future after libification is "complete", users external to Git will
> be able to use the same trailer processing API used by the
> interpret-trailers builtin. For example, a web server may want to parse
> trailers the same way that Git would parse them, without having to call
> interpret-trailers as a subprocess. This use case was the original
> motivation behind my work in this area.

Thanks for telling us about this use case.

Show 9 quoted lines

> Thanks to the aggressive refactoring in this series, I've been able to
> identify and fix several bugs in our existing implementation. Those fixes
> build on top of this series but were not included here, in order to keep
> this series small. Below is a "shortlog" of those fixes I have locally:
>
>  * "trailer: trailer replacement should not change its position" (If we
>    found a trailer we'd like to replace, preserve its position relative to
>    the other trailers found in the trailer block, instead of always moving
>    it to the beginning or end of the entire trailer block.)

I believe there was a reason why it was done this way. I don't remember what it was though.

Show 5 quoted lines

>  * "interpret-trailers: preserve trailers coming from the input" (Sometimes,
>    the parsed trailers from the input will be formatted differently
>    depending on whether we provide --only-trailers or not. Make the trailers
>    that were not modified and which are coming directly from the input get
>    formatted the same way, regardless of this flag.)

It could be a feature to be able to normalize trailers in a certain way.

>  * "interpret-trailers: do not modify the input if NOP" (Refrain from
>    subtracting or adding a newline around the patch divider "---" if we are
>    not adding new trailers.)

It could be a feature to be able to normalize this too.

>  * "trailer formatter: split up format_trailer() monolith" (Fix a bug in
>    git-log where we still printed a blank newline even if we didn't want to
>    format anything.)

I am not sure this is a bug fix either. It could perhaps be a normalization too.

>  * "interpret-trailers: fail if given unrecognized arguments" (E.g., for
>    "--where", only accept recognized WHERE_* enum values. If we get
>    something unrecognized, fail with an error instead of silently doing
>    nothing. Ditto for "--if-exists" and "--if-missing".)

It's possible that there was a reason why it was done this way.

I think you might want to take a look at the discussions on the mailing list when "interpret-trailers" was developed. There were a lot of discussions over a long time, and there were a lot of requests and suggestions about what it should do.