Re: [PATCH v2 02/10] midx-write: pass down repository to static functions

From: karthik nayak <karthik.188@gmail.com>
Date: Nov 21, 2024, 15:06 UTC
Message-ID: <CAOLa=ZQhjPRnfUhsZOir-a=djC523waw8qn3E8rk0ou7pdH8Hw@mail.gmail.com>
In-Reply-To: <Zz472iMq3nZMoRMf@nand.local>

Taylor Blau <me@ttaylorr.com> writes:

Show 19 quoted lines

> 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