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.
