| Message ID | 20200515100454.14486-6-chriscool@tuxfamily.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | upload-pack: use 'struct upload_pack_data' thoroughly, part 1 | expand |
On Fri, May 15, 2020 at 12:04:46PM +0200, Christian Couder wrote: > As we cleanup 'upload-pack.c' by using 'struct upload_pack_data' > more thoroughly, let's pass 'struct upload_pack_data' to > get_common_commits(), so that this function and the functions > it calls can use all the fields of that struct in followup > commits. This one is optional, I think. We're not removing any globals nor moving towards removing any (I don't think), because we're really just replacing two object_array parameters with the big data struct: > -static int get_common_commits(struct packet_reader *reader, > - struct object_array *have_obj, > - struct object_array *want_obj) > +static int get_common_commits(struct upload_pack_data *data, > + struct packet_reader *reader) I say "I don't think" because it's possible that get_common_commits() could use one of the other fields (or pass it to another function) in a later patch. But I don't see it. So I think one _could_ argue that it should continue to take the minimum set of arguments it needs. I doubt it would ever be useful outside of the context of upload_pack, so I think in practice it doesn't matter much (beyond perhaps documenting which parts of the global data it needs). So I don't feel strongly either way. -Peff
On Fri, May 15, 2020 at 02:17:54PM -0400, Jeff King wrote: > On Fri, May 15, 2020 at 12:04:46PM +0200, Christian Couder wrote: > > > As we cleanup 'upload-pack.c' by using 'struct upload_pack_data' > > more thoroughly, let's pass 'struct upload_pack_data' to > > get_common_commits(), so that this function and the functions > > it calls can use all the fields of that struct in followup > > commits. > > This one is optional, I think. We're not removing any globals nor moving > towards removing any (I don't think), because we're really just > replacing two object_array parameters with the big data struct: > > > -static int get_common_commits(struct packet_reader *reader, > > - struct object_array *have_obj, > > - struct object_array *want_obj) > > +static int get_common_commits(struct upload_pack_data *data, > > + struct packet_reader *reader) > > I say "I don't think" because it's possible that get_common_commits() > could use one of the other fields (or pass it to another function) in a > later patch. But I don't see it. > > So I think one _could_ argue that it should continue to take the minimum > set of arguments it needs. I doubt it would ever be useful outside of the > context of upload_pack, so I think in practice it doesn't matter much > (beyond perhaps documenting which parts of the global data it needs). So > I don't feel strongly either way. OK, I take it back. I finally go to the spot later in the series where you use data->stateless_rpc, etc. So I think this is a positive movement. I think it might have been easier to review if the changes were made in the opposite order: globals removed and passed down the call stack, and then the big struct used to simplify passing the data around. But that would have been a lot more tedious to write (and perhaps to review, as I'd have made the opposite complaint ;) ). And definitely not worth doing now. -Peff
diff --git a/upload-pack.c b/upload-pack.c index cb336c5713..7953a33189 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -414,9 +414,8 @@ static int ok_to_give_up(const struct object_array *have_obj, min_generation); } -static int get_common_commits(struct packet_reader *reader, - struct object_array *have_obj, - struct object_array *want_obj) +static int get_common_commits(struct upload_pack_data *data, + struct packet_reader *reader) { struct object_id oid; char last_hex[GIT_MAX_HEXSZ + 1]; @@ -432,12 +431,14 @@ static int get_common_commits(struct packet_reader *reader, reset_timeout(); if (packet_reader_read(reader) != PACKET_READ_NORMAL) { - if (multi_ack == 2 && got_common - && !got_other && ok_to_give_up(have_obj, want_obj)) { + if (multi_ack == 2 + && got_common + && !got_other + && ok_to_give_up(&data->have_obj, &data->want_obj)) { sent_ready = 1; packet_write_fmt(1, "ACK %s ready\n", last_hex); } - if (have_obj->nr == 0 || multi_ack) + if (data->have_obj.nr == 0 || multi_ack) packet_write_fmt(1, "NAK\n"); if (no_done && sent_ready) { @@ -451,10 +452,11 @@ static int get_common_commits(struct packet_reader *reader, continue; } if (skip_prefix(reader->line, "have ", &arg)) { - switch (got_oid(arg, &oid, have_obj)) { + switch (got_oid(arg, &oid, &data->have_obj)) { case -1: /* they have what we do not */ got_other = 1; - if (multi_ack && ok_to_give_up(have_obj, want_obj)) { + if (multi_ack + && ok_to_give_up(&data->have_obj, &data->want_obj)) { const char *hex = oid_to_hex(&oid); if (multi_ack == 2) { sent_ready = 1; @@ -470,14 +472,14 @@ static int get_common_commits(struct packet_reader *reader, packet_write_fmt(1, "ACK %s common\n", last_hex); else if (multi_ack) packet_write_fmt(1, "ACK %s continue\n", last_hex); - else if (have_obj->nr == 1) + else if (data->have_obj.nr == 1) packet_write_fmt(1, "ACK %s\n", last_hex); break; } continue; } if (!strcmp(reader->line, "done")) { - if (have_obj->nr > 0) { + if (data->have_obj.nr > 0) { if (multi_ack) packet_write_fmt(1, "ACK %s\n", last_hex); return 0; @@ -1176,9 +1178,7 @@ void upload_pack(struct upload_pack_options *options) receive_needs(&reader, &data.want_obj, &data.filter_options); if (data.want_obj.nr) { - get_common_commits(&reader, - &data.have_obj, - &data.want_obj); + get_common_commits(&data, &reader); create_pack_file(&data.have_obj, &data.want_obj, &data.filter_options);
As we cleanup 'upload-pack.c' by using 'struct upload_pack_data' more thoroughly, let's pass 'struct upload_pack_data' to get_common_commits(), so that this function and the functions it calls can use all the fields of that struct in followup commits. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- upload-pack.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)