| Message ID | 3a42575bd5d124b6b2e536b1511107ebf5ec1091.1589816719.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remote-curl: fix deadlocks when remote server disconnects | expand |
Denton Liu <liu.denton@gmail.com> writes: > In the switch statement, the difference between the `protocol_v2` and > `protocol_v{1,0}` arms is a preparatory 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. Sure. if (data->version < protocol_v2) die_if_server_options(transport); else if (data->version == protocol_unknown_version) BUG("..."); refs = fetch_pack(...); might have been even easier to follow, but that's OK. > 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); > + /* do nothing */ > break; > case protocol_v1: > case protocol_v0: > die_if_server_options(transport); > - 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_unknown_version: > BUG("unknown protocol version"); > } > + refs = fetch_pack(&args, data->fd, > + refs_tmp ? refs_tmp : transport->remote_refs, > + to_fetch, nr_heads, &data->shallow, > + &transport->pack_lockfile, data->version); > > close(data->fd[0]); > close(data->fd[1]);
diff --git a/transport.c b/transport.c index 15f5ba4e8f..a6002e502f 100644 --- a/transport.c +++ b/transport.c @@ -371,22 +371,19 @@ 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); + /* do nothing */ break; case protocol_v1: case protocol_v0: die_if_server_options(transport); - 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_unknown_version: BUG("unknown protocol version"); } + refs = fetch_pack(&args, data->fd, + refs_tmp ? refs_tmp : transport->remote_refs, + to_fetch, nr_heads, &data->shallow, + &transport->pack_lockfile, data->version); close(data->fd[0]); close(data->fd[1]);
In the switch statement, the difference between the `protocol_v2` and `protocol_v{1,0}` arms is a preparatory 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. Extract the common fetch_pack() call out of the switch statement so that code duplication is reduced and the logic is more clear for future readers. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- transport.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)