Re: [PATCH 3/6] transport: combine common cases with a fallthrough

From: Denton Liu <liu.denton@gmail.com>
Date: May 18, 2020, 09:18 UTC
Message-ID: <20200518091825.GB24557@generichostname>
In-Reply-To: <CAPig+cQyCSMtSrdk54E6MVYL4prhNyWgBKkkBFHXo5hShqQ+=Q@mail.gmail.com>

Hi Eric,

On Wed, May 13, 2020 at 07:14:35PM -0400, Eric Sunshine wrote:

Show 22 quoted lines

> > diff --git a/transport.c b/transport.c
> > @@ -370,15 +370,11 @@ static int fetch_refs_via_pack(struct transport *transport,
> >         switch (data->version) {
> > -       case protocol_v2:
> > -               refs = fetch_pack(&args, data->fd,
> > -                                 refs_tmp ? refs_tmp : transport->remote_refs,
> > -                                 to_fetch, nr_heads, &data->shallow,
> > -                                 &transport->pack_lockfile, data->version);
> > -               break;
> > -       case protocol_v1:
> >         case protocol_v0:
> > +       case protocol_v1:
> >                 die_if_server_options(transport);
> > +               /* fallthrough */
> > +       case protocol_v2:
> >                 refs = fetch_pack(&args, data->fd,
> >                                   refs_tmp ? refs_tmp : transport->remote_refs,
> >                                   to_fetch, nr_heads, &data->shallow,
> 
> I can't say that I'm a fan of this change. While it may make it clear
> that the two calls are identical, it makes it more difficult to reason
> about the distinct v0, v1, and v2 cases.

I would argue that it would make it easier to reason about the distinct cases. From the rewritten version, it's more obvious that code used in the v2 case is a subset of the code used in v0 and v1.

> It also makes it more
> difficult to extend or make changes to one or another case, should
> that become necessary in the future.

I would say that's the point of this change ;) When I was looking over this code initially, I was scratching my head over the difference between the two cases because I expected them to be different in the two cases. If a change is necessary in the future, then it'll make sense to write it as two separate calls to fetch_pack() or whatever but until that happens, I think it makes more sense remove the duplication.

Actually, thinking about it some more, do you think it would make more sense to just pull the fetch_pack() call out of the switch statement entirely? We could entirely eliminate the fallthrough if we do this.