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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Denton Liu <liu.denton@gmail.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 13:43:02 -0400	[thread overview]
Message-ID: <CAPig+cR97uFvxmTBHiV5zi9ZqfZnDKmAi4ZG1rUOngk6QrWEVA@mail.gmail.com> (raw)
In-Reply-To: <20200518091825.GB24557@generichostname>

On Mon, May 18, 2020 at 5:18 AM Denton Liu <liu.denton@gmail.com> wrote:
> On Wed, May 13, 2020 at 07:14:35PM -0400, Eric Sunshine wrote:
> > > +       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.
>
> 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.

Yes, I think that works better and is cleaner and easier to understand
than the "fallthrough" in v1.