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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 31, 2024, 22:16 UTC
Message-ID: <xmqqa5ol409k.fsf@gitster.g>
In-Reply-To: <xmqqy1c545y0.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 25 quoted lines

>> +				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.