Re: [PATCH 3/9] fetch: pass through `fetch_config` directly

From: Patrick Steinhardt <ps@pks.im>
Date: May 22, 2023, 08:58 UTC
Message-ID: <ZGsuy6T_yiK4qxxJ@ncase>
In-Reply-To: <20230519001803.GC2442034@coredump.intra.peff.net>

On Thu, May 18, 2023 at 08:18:03PM -0400, Jeff King wrote:

Show 36 quoted lines

> 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