On Sun, Nov 12, 2023 at 07:38:22PM +0100, René Scharfe wrote:

quoted

Grepping for other
     code that does the same thing, I see that show_log() and
     cmd_format_patch() copy a lot more.

show_log() copies almost half of the struct 6d167fd7cc members
from struct rev_info!

cmd_format_patch() doesn't seem have a struct pretty_print_context,
though...

Doh, you're right. I grepped for spots setting encode_email_headers, but
the one in cmd_format_patch() is copying it from the config-default into
the rev_info, not into the pretty-print context.

Which makes sense. It is going to call show_log() to show each commit,
which is where the value is copied into the pretty-print context.

quoted

(For that matter, why doesn't
     make_cover_letter() just use the context set up by
     cmd_format_patch()?).

... which answers this question, but did you perhaps mean a different
function?

Right, I was just confused.

quoted

  2. Why are we copying this stuff at all? When we introduced the
     pretty-print context back in 6bf139440c (clean up calling
     conventions for pretty.c functions, 2011-05-26), the idea was just
     to keep all of the format options together. But later, 6d167fd7cc
     (pretty: use fmt_output_email_subject(), 2017-03-01) added a
     pointer to the rev_info directly.

Hmm.  Was sticking the rev_info pointer in unwise?  Does it tangle up
things that should be kept separate?  It uses force_in_body_from,
grep_filter, sources, nr, total and subject_prefix from struct rev_info.
That seems a lot, but is just a small fraction of its total members and
we could just copy those we need.  Or prepare the subject string and
pass it in, as before 6d167fd7cc.

I don't know that it was unwise. I was mostly just noting the history
because that explains why we _didn't_ simply refer to ctx->revs in
6bf139440c. Has the separation between the two been valuable since then?
I'm not sure. We do have some code paths that do not have a rev_info at
all (e.g., pp_commit_easy(), which makes an ad-hoc empty context
struct).

quoted

So could/should we just be using
     pp->rev->encode_email_headers here?

Perhaps.  If we want struct pretty_print_context to be a collection of
named parameters, though, then it makes sense to avoid using
complicated types to provide a nice interface to its callers, and
struct rev_info is one of our largest structs we have.

Yeah, philosophically it may be better to keep the modules separated.
But if we end up having to just copy a ton of fields, it may not be as
practical. If we can at least factor that out into a single spot,
though, it may not be so bad.

quoted

     Or if that field is not always set (or we want to override some
     elements), should there be a single helper function to initialize
     the pretty_print_context from a rev_info, that could be shared
     between spots like show_log() and make_cover_letter()?

That would help avoid forgetting to copy something.  But copying is
questionable in general, as you mentioned.  Given the extent of the
overlap, would it make sense to embed struct pretty_print_context in
struct rev_info instead?  Or a subset thereof?

I had a similar thought, but the pretty_print_context carries both input
options from the caller, as well as computed state used internally by
the pretty-print code. So you'd have to split those two up, I would
think. And now all of the pretty-print functions have to pass around
_two_ contexts.

That's more annoying, but arguably is a cleaner design (the internal
struct can be a private thing that is not even defined outside of
pretty.c). I dunno.

-Peff