| Message ID | c89c1841008dfc2d111369fb682b946a0c33b7be.1589393036.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote-curl: partial fix for a deadlock with stateless rpc | expand |
On Wed, May 13, 2020 at 2:07 PM Denton Liu <liu.denton@gmail.com> 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@gmail.com> > --- > 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.
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.
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.
diff --git a/transport.c b/transport.c index 15f5ba4e8f..475f94564a 100644 --- 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,
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 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@gmail.com> --- transport.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)