Junio C Hamano <gitster@xxxxxxxxx> 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.
