On Thu, May 18, 2023 at 08:18:03PM -0400, Jeff King wrote:
> On Wed, May 17, 2023 at 01:48:51PM +0200, Patrick Steinhardt wrote:
>
> > The `fetch_config` structure currently only has a single member, which
> > is the `display_format`. We're about extend it to contain all parsed
> > config values and will thus need it available in most of the code.
> >
> > Prepare for this change by passing through the `fetch_config` directly
> > instead of only passing its single member.
>
> Makes sense.
>
> One small nit:
>
> > static int do_fetch(struct transport *transport,
> > struct refspec *rs,
> > - enum display_format display_format)
> > + const struct fetch_config *config)
> > {
> > struct ref_transaction *transaction = NULL;
> > struct ref *ref_map = NULL;
> > @@ -1639,7 +1639,8 @@ static int do_fetch(struct transport *transport,
> > if (retcode)
> > goto cleanup;
> >
> > - display_state_init(&display_state, ref_map, transport->url, display_format);
> > + display_state_init(&display_state, ref_map, transport->url,
> > + config->display_format);
>
> If the point is that fetch_config may start carrying new information,
> wouldn't we want to pass it as a whole down to display_state_init()? It
> might eventually want to see some of that other config, too.
>
> It's presumably academic for now, and it would not be too hard to change
> later if needed, so I don't know that it's worth a re-roll. I just found
> it especially funny here since the purpose of the patch is to treat the
> config struct as a single unit.
Well, I decided against passing in the full configuration as it feels a
bit like a layering violation: the other code really is about the fetch
itself, while this code here is only about display logic. So passing in
the `fetch_config` felt weird to me, even more so because we continue to
only need that single value at the end of this series. I do see your
point though.
Given that none of your other comments require a reroll I'll leave this
as-is for now. Thanks for your review!
Patrick
Attachment:
signature.asc
Description: PGP signature
