Junio C Hamano <gitster@xxxxxxxxx> writes:
> "Linus Arver via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> This patch series is the first 10 patches of a much larger series I've been
>> working. The main goal of this series is to enrich the API in trailer.h. The
>> larger series brings a number of additional code simplifications and
>> cleanups (exposing and fixing some bugs along the way), and builds on top of
>> this series. The goal of the larger series is to make the trailer interface
>> ready for unit testing. By "trailer API" I mean those functions exposed in
>> trailer.h.
>
> Are there places in the current code that deals with trailers but
> does not use the trailer API (e.g., manually parse and/or insert the
> trailer in an in-core buffer)?
While working on this series I unfortunately did not search for such use
cases (I limited the scope of my work to mainly the interpret-trailers
builtin). But, a quick search just now on master branch turned up
append_signoff() in sequencer.c which constructs a Signed-off-by trailer
manually with a raw strbuf [1].
This is somewhat understandable though, because AFAICS the current
trailer API does not support creating and formatting single trailer
objects.
> Is it part of the larger goal to
> update these places so that we will always use the trailer API to
> touch trailers
That sounds like The Right Thing to do.
> , and if so, have these places been identified?
Not yet, but, it should be easy to check any remaining cases other than
the one I identified up above.
> Obviously the reason why I ask is that testing cannot be the goal by
> itself.
I now seem to recall an offline discussion where I said I wanted to
enrich the API to make other parts of Git also use this new API. Somehow
I've left that motivation out of the cover letter (will include in the
next reroll).
> The "alternative" ...
>
>> As an alternative to this patch series, we could keep trailer.h intact and
>> decide to unit-test the existing "trailer_info_get()" function which does
>> most of the trailer parsing work.
>
> ... may allow you to "test", but it would make it more difficult in
> the future to revamp the trailer API, if it is needed, in order to
> cover code paths that ought to be using but currently bypassing the
> trailer API.
Agreed. I should include this rationale as well in the next cover
letter, thanks.
>> This series breaks up "process_trailers()" into smaller pieces, exposing
>> many of the parts relevant to trailer-related processing in trailer.h. This
>> forces us to start writing unit tests for these now public functions, but
>> that is a good thing because those same unit tests should be easy to write
>> (due to their small(er) sizes), but also, because those unit tests will now
>> ensure some degree of stability across new versions of trailer.h (we will
>> start noticing when the behavior of any of these API functions change).
>
> And helper functions, each of which does one small thing well, may
> be more applicable to other code paths that are currently bypassing
> the API.
Yep.
>> Thanks to the aggressive refactoring in this series, I've been able to
>> identify and fix a couple 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.
>
> It would be nicer to have a concise list of these fixes (in the form
> of "git shortlog") as a teaser here ;-). That would hopefully
> entice others into reviewing this part that forms the foundation.
Ah, good idea. Here's a shortlog (with a short summary of each one) of
bug fixes that are forthcoming:
trailer: trailer replacement should not change its position
(Summary: 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.)
interpret-trailers: preserve trailers coming from the input
(Summary: 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 are coming directly from the input get formatted the same way,
regardless of this flag.)
interpret-trailers: fail if given unrecognized arguments
(Summary: 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".)
The last one is a different class of bug than the first two, and perhaps
less interesting.
Thanks.
[1] https://github.com/git/git/blob/d4dbce1db5cd227a57074bcfc7ec9f0655961bba/sequencer.c#L5299-L5301
