On Wed, May 13, 2020 at 2:07 PM Denton Liu <liu.denton@xxxxxxxxx> wrote:
> In the switch statement, the difference between the `protocol_v2` and
> `protocol_v{1,0}` arms is a prepatory call to die_if_server_options() in
What is "prepatory"?
More below...
> the latter. The fetch_pack() call is identical in both arms. However,
> since this fetch_pack() call has so many parameters, it is not
> immediately obvious that the call is identical in both cases.
>
> Rewrite the switch statement to fallthrough from the v{1,0} case to v2
> so that they share a common fetch_pack() call. This reduces duplication
> and makes the logic more clear for future readers.
>
> Signed-off-by: Denton Liu <liu.denton@xxxxxxxxx>
> ---
> 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. It also makes it more
difficult to extend or make changes to one or another case, should
that become necessary in the future.
