On Sun, Nov 12, 2023 at 07:38:22PM +0100, René Scharfe wrote: > > 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. > > (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. > > 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). > > 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. > > 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
