Taylor Blau <me@xxxxxxxxxxxx> writes: > On Tue, Nov 19, 2024 at 04:36:41PM +0100, Karthik Nayak wrote: >> --- >> midx-write.c | 57 +++++++++++++++++++++++++++++++-------------------------- >> 1 file changed, 31 insertions(+), 26 deletions(-) > > This patch all seems reasonable to me. When reviewing it in this way, it > is a little unclear to see which functions need just a repository, just > a hash_algo, or the whole write_midx_context structure. > > I think I might have erred on the side of just passing the > write_midx_context structure throughout the midx-write.c internals. That > exposes more of the implementation details than is strictly necessary, > but it does so to the benefit of making the code (and this patch) easier > to follow. > > I don't feel strongly about it, and I trust that you modified the right > functions to pass the right data. I just wanted to mention it in case > you hadn't considered an alternative way to structure this patch. > I remember doing something like that and it seemed a lot more noisy than the current iteration. Regarding `write_midx_context`, I wonder if all these functions need to receive it. Passing only what’s necessary might make the functions' requirements clearer, instead of bundling everything into a large struct. But then again, if you feel otherwise, I'm happy to change it, I don't feel so strongly about it :) > Thanks, > Taylor
Attachment:
signature.asc
Description: PGP signature
