[v3,00/13] upload-pack: use 'struct upload_pack_data' thoroughly, part 2
mbox series

Message ID 20200604175450.17385-1-chriscool@tuxfamily.org
Headers show
Series
  • upload-pack: use 'struct upload_pack_data' thoroughly, part 2
Related show

Message

Christian Couder June 4, 2020, 5:54 p.m. UTC
This patch series is the second part of an effort to move all static
variables in 'upload-pack.c' into 'struct upload_pack_data'.

It is based on 'cc/upload-pack-data' which contains "part 1" of this
effort. (See also: https://lore.kernel.org/git/20200515100454.14486-1-chriscool@tuxfamily.org/)

A part 3 will follow with the rest of the patches needed to get rid of
the static variables left after this patch series.

Thanks to Peff for his review of the first and second versions of this
part 2:

V1: https://lore.kernel.org/git/20200527164742.23067-1-chriscool@tuxfamily.org/
V2: https://lore.kernel.org/git/20200602041657.7132-1-chriscool@tuxfamily.org/

Thanks to Jonathan Tan for his review of V2 too.

Compared to V2 the changes are the following:

  - patch 01/13 integrates Jonathan's commit message improvement

  - patch 07/13 integrates Jonathan's fixup patch which makes
    multi_ack enum values all-caps and fixes a spacing issue (2 spaces
    between "enum" and "{")

There is a lot of noise in the range-diff with V2, because V3 contains
Peff's 'Acked-by:' and Junio's 'Signed-off-by', but here it is:

 1:  e5c31f30ec !  1:  12f8a9c953 upload-pack: actually use some upload_pack_data bitfields
    @@ Commit message
     
         As we cleanup 'upload-pack.c' by using 'struct upload_pack_data'
         more thoroughly, let's actually start using some bitfields of
    -    that struct, which were previously unused.
    +    that struct. These bitfields were introduced in 3145ea957d
    +    ("upload-pack: introduce fetch server command", 2018-03-15), but
    +    were never used.
     
         We could instead have just removed the following bitfields
         from the struct:
    @@ Commit message
         fixing a bug, but one nobody is likely to see, because it would
         imply the client sending different capabilities for each request.
     
    +    Helped-by: Jonathan Tan <jonathantanmy@google.com>
         Signed-off-by: Jeff King <peff@peff.net>
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static timestamp_t oldest_have;
 2:  225be644f4 !  2:  b74c2f4d64 upload-pack: annotate upload_pack_data fields
    @@ Commit message
         ones only for protocol v2.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static int allow_ref_in_want;
 3:  5adca8d70c !  3:  05cb66320a upload-pack: move static vars to upload_pack_data
    @@ Commit message
         now is more coherent and cleaner.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@
 4:  1507a8134a !  4:  be32b64b0a upload-pack: move use_sideband to upload_pack_data
    @@ Commit message
         definition.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static unsigned int allow_unadvertised_object_request;
 5:  0ef8d28958 !  5:  154651241d upload-pack: move filter_capability_requested to upload_pack_data
    @@ Commit message
         now is more coherent and cleaner.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static struct object_array extra_edge_obj;
 6:  319c09ce02 !  6:  ba6999dc74 upload-pack: move multi_ack to upload_pack_data
    @@ Commit message
         now is more coherent and cleaner.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@
 7:  cd3a0a1e5c !  7:  988e2e1d9a upload-pack: change multi_ack to an enum
    @@ Commit message
     
         This will make it clear which values this variable can take.
     
    +    Helped-by: Jonathan Tan <jonathantanmy@google.com>
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: struct upload_pack_data {
    @@ upload-pack.c: struct upload_pack_data {
      
        unsigned int timeout;                                   /* v0 only */
     -  int multi_ack;                                          /* v0 only */
    -+  enum  {
    -+          no_multi_ack = 0,
    -+          multi_ack = 1,
    -+          multi_ack_detailed = 2
    ++  enum {
    ++          NO_MULTI_ACK = 0,
    ++          MULTI_ACK = 1,
    ++          MULTI_ACK_DETAILED = 2
     +  } multi_ack;                                            /* v0 only */
      
        /* 0 for no sideband, otherwise DEFAULT_PACKET_MAX or LARGE_PACKET_MAX */
    @@ upload-pack.c: static int get_common_commits(struct upload_pack_data *data,
      
                if (packet_reader_read(reader) != PACKET_READ_NORMAL) {
     -                  if (data->multi_ack == 2
    -+                  if (data->multi_ack == multi_ack_detailed
    ++                  if (data->multi_ack == MULTI_ACK_DETAILED
                            && got_common
                            && !got_other
                            && ok_to_give_up(&data->have_obj, &data->want_obj)) {
    @@ upload-pack.c: static int get_common_commits(struct upload_pack_data *data,
                                    && ok_to_give_up(&data->have_obj, &data->want_obj)) {
                                        const char *hex = oid_to_hex(&oid);
     -                                  if (data->multi_ack == 2) {
    -+                                  if (data->multi_ack == multi_ack_detailed) {
    ++                                  if (data->multi_ack == MULTI_ACK_DETAILED) {
                                                sent_ready = 1;
                                                packet_write_fmt(1, "ACK %s ready\n", hex);
                                        } else
    @@ upload-pack.c: static int get_common_commits(struct upload_pack_data *data,
                                got_common = 1;
                                oid_to_hex_r(last_hex, &oid);
     -                          if (data->multi_ack == 2)
    -+                          if (data->multi_ack == multi_ack_detailed)
    ++                          if (data->multi_ack == MULTI_ACK_DETAILED)
                                        packet_write_fmt(1, "ACK %s common\n", last_hex);
                                else if (data->multi_ack)
                                        packet_write_fmt(1, "ACK %s continue\n", last_hex);
    @@ upload-pack.c: static void receive_needs(struct upload_pack_data *data,
                        data->deepen_relative = 1;
                if (parse_feature_request(features, "multi_ack_detailed"))
     -                  data->multi_ack = 2;
    -+                  data->multi_ack = multi_ack_detailed;
    ++                  data->multi_ack = MULTI_ACK_DETAILED;
                else if (parse_feature_request(features, "multi_ack"))
     -                  data->multi_ack = 1;
    -+                  data->multi_ack = multi_ack;
    ++                  data->multi_ack = MULTI_ACK;
                if (parse_feature_request(features, "no-done"))
                        data->no_done = 1;
                if (parse_feature_request(features, "thin-pack"))
 8:  c23e37a8da !  8:  8dcd7dddc2 upload-pack: pass upload_pack_data to upload_pack_config()
    @@ Commit message
         that are set in upload_pack_config() into 'upload_pack_data'.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static int find_symref(const char *refname, const struct object_id *oid,
 9:  26893a4098 !  9:  8c165eef8f upload-pack: move keepalive to upload_pack_data
    @@ Commit message
         It is used by code common to protocol v0 and protocol v2.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static timestamp_t oldest_have;
    @@ upload-pack.c: struct upload_pack_data {
     +  int keepalive;
      
        unsigned int timeout;                                   /* v0 only */
    -   enum  {
    +   enum {
     @@ upload-pack.c: static void upload_pack_data_init(struct upload_pack_data *data)
        data->shallows = shallows;
        data->deepen_not = deepen_not;
10:  2fa59d6c89 ! 10:  17ac2a015a upload-pack: move allow_filter to upload_pack_data
    @@ Commit message
         It is used by both protocol v0 and protocol v2 code.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static int shallow_nr;
11:  d501b0f694 ! 11:  74245639d8 upload-pack: move allow_ref_in_want to upload_pack_data
    @@ Commit message
         It is used only by protocol v2 code.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static int shallow_nr;
12:  90513f0543 ! 12:  d866ea2741 upload-pack: move allow_sideband_all to upload_pack_data
    @@ Commit message
         It is used only by protocol v2 code.
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static int shallow_nr;
13:  f2851482f5 ! 13:  fce53edae4 upload-pack: move pack_objects_hook to upload_pack_data
    @@ Commit message
         While at it let's also free() it in upload_pack_data_clear().
     
         Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
    +    Acked-by: Jeff King <peff@peff.net>
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## upload-pack.c ##
     @@ upload-pack.c: static timestamp_t oldest_have;

Christian Couder (12):
  upload-pack: annotate upload_pack_data fields
  upload-pack: move static vars to upload_pack_data
  upload-pack: move use_sideband to upload_pack_data
  upload-pack: move filter_capability_requested to upload_pack_data
  upload-pack: move multi_ack to upload_pack_data
  upload-pack: change multi_ack to an enum
  upload-pack: pass upload_pack_data to upload_pack_config()
  upload-pack: move keepalive to upload_pack_data
  upload-pack: move allow_filter to upload_pack_data
  upload-pack: move allow_ref_in_want to upload_pack_data
  upload-pack: move allow_sideband_all to upload_pack_data
  upload-pack: move pack_objects_hook to upload_pack_data

Jeff King (1):
  upload-pack: actually use some upload_pack_data bitfields

 upload-pack.c | 199 ++++++++++++++++++++++++++++----------------------
 1 file changed, 110 insertions(+), 89 deletions(-)

Comments

Junio C Hamano June 4, 2020, 6:07 p.m. UTC | #1
Christian Couder <christian.couder@gmail.com> writes:

> This patch series is the second part of an effort to move all static
> variables in 'upload-pack.c' into 'struct upload_pack_data'.
>
> It is based on 'cc/upload-pack-data' which contains "part 1" of this
> effort. (See also: https://lore.kernel.org/git/20200515100454.14486-1-chriscool@tuxfamily.org/)
>
> A part 3 will follow with the rest of the patches needed to get rid of
> the static variables left after this patch series.
>
> Thanks to Peff for his review of the first and second versions of this
> part 2:
>
> V1: https://lore.kernel.org/git/20200527164742.23067-1-chriscool@tuxfamily.org/
> V2: https://lore.kernel.org/git/20200602041657.7132-1-chriscool@tuxfamily.org/
>
> Thanks to Jonathan Tan for his review of V2 too.
>
> Compared to V2 the changes are the following:
>
>   - patch 01/13 integrates Jonathan's commit message improvement
>
>   - patch 07/13 integrates Jonathan's fixup patch which makes
>     multi_ack enum values all-caps and fixes a spacing issue (2 spaces
>     between "enum" and "{")

Thanks, all.

I notice that enumeration constants for "enum multi_ack" have been
upcased; I do not think it matters too much either way.  A situation
that strongly favours upper case is when the enum is the result of
converting from C preprocessor macros that were spelled with upper
case, but this is not one of those.

> There is a lot of noise in the range-diff with V2, because V3 contains
> Peff's 'Acked-by:' and Junio's 'Signed-off-by', but here it is:

Would it work as a "trick" to fetch what has been queued in 'pu' and
compare against that lets you avoid such a "noise", I wonder?  If it
works well, it would have an added benefit of having the
publically-known commit object names on the left hand side
(e.g. instead of e5c31f30ec, you'd see d293806610 as the patch 1
from the second iteration) of the range-diff.

>  1:  e5c31f30ec !  1:  12f8a9c953 upload-pack: actually use some upload_pack_data bitfields
Christian Couder June 5, 2020, 10:38 a.m. UTC | #2
On Thu, Jun 4, 2020 at 8:07 PM Junio C Hamano <gitster@pobox.com> wrote:

>
> Christian Couder <christian.couder@gmail.com> writes:

> > There is a lot of noise in the range-diff with V2, because V3 contains
> > Peff's 'Acked-by:' and Junio's 'Signed-off-by', but here it is:
>
> Would it work as a "trick" to fetch what has been queued in 'pu' and
> compare against that lets you avoid such a "noise", I wonder?

Yeah, probably. I will try it next time.

> If it
> works well, it would have an added benefit of having the
> publically-known commit object names on the left hand side
> (e.g. instead of e5c31f30ec, you'd see d293806610 as the patch 1
> from the second iteration) of the range-diff.

Yeah, thanks for the suggestion!

By the way another issue I had was that for some reason send-email
inserted a "From: Christian Couder <christian.couder@gmail.com>" line
at the beginning of each patch. Sorry about that.