Subject: Re: [PATCH v3 03/10] trailer: unify trailer formatting machinery

From: Junio C Hamano <[email-masked]>
Date: Wed, 31 Jan 2024 14:16:07 -0800

Junio C Hamano <[email-masked]> writes:

>> +				if (opts->unfold)
>> +					unfold_value(&val);
>
> So, ... how would this affect an invocation of
>
>     $ git interpret-trailers --unfold
>
> which would have set opts.unfold to true in cmd_interpret_trailers()
> and eventually called process_trailers() with it, which eventually
> called print_all(), which conditionally called print_tok_val() but
> never gave the val to unfold_value()?
>
>  ... goes and digs a bit more ...
>
> Ahhhh, original process_trailers() did this by calling
> parse_trailers() that munged the value.  So its print_all()
> codepath only had to print what has already been munged.
>
> But then in this new code, do we unfold only here?  I thought that
> in the new code you moved around, the caller, whose name is now
> interpret_trailers(), still calls parse_trailers() and that calls
> the unfold_value().  So, are we doing redundant work that may merely
> be unneeded (if we are lucky) or could be harmful (if things, other
> than the unfold thing I just noticed, that are done both in
> parse_trailers() and this function are not idempotent)?

I was too confused by the code flow and resorted to tracing what
happens when "git interpret-trailers --unfold" is run in a way
similar to how t7513 does in gdb.  Shame shame.

In any case, the updated code does try to call unfold_value() in
format_trailers() on "val" that has already been unfolded in
parse_trailers().  This may not produce an incorrect result if
unfold_value() is truly idempotent (I do not know), but even if it
is correct for its handling of .unfold bit, this episode lowered my
confidence level on the new code significantly, as I do not know
what unintended effects [*] all the other new things done in this
function have, or if none of these unintended effects are
error-free.

> It could be that a bit more preparatory refactoring would clarify.
> I dunno.

Thanks.

[Footnote] 

 * I am assuming that calling unfold twice on the same value and
   relying for its idempotent-ness for correctness was not your
   intention.