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

From: Denton Liu <liu.denton@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: Git Mailing List <git@vger.kernel.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH 3/6] transport: combine common cases with a fallthrough
Date: Mon, 18 May 2020 05:18:25 -0400	[thread overview]
Message-ID: <20200518091825.GB24557@generichostname> (raw)
In-Reply-To: <CAPig+cQyCSMtSrdk54E6MVYL4prhNyWgBKkkBFHXo5hShqQ+=Q@mail.gmail.com>

Hi Eric,

On Wed, May 13, 2020 at 07:14:35PM -0400, Eric Sunshine wrote:
> > 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.