Re: [PATCH 2/3] upload-pack: Prepare to extend allow-tip-sha1-in-want

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 12, 2015, 21:37 UTC
Message-ID: <CAPig+cQ8Ndf791b0g+UGZcVsP75Nsd9TQBiUgXv9fhuE+9P4-w@mail.gmail.com>
In-Reply-To: <1431465265-18486-2-git-send-email-fredrik.medley@gmail.com>

On Tue, May 12, 2015 at 5:14 PM, Fredrik Medley <fredrik.medley@gmail.com> wrote:

> Rename the allow_tip_sha1_in_want variable to
> allow_request_with_bare_object_name to allow for future extensions, e.g.
> allowing non-tip sha1.

'allow_request_with_bare_object_name' is quite a mouthful. Does it need to be this long?

Regarding the commit message: Isn't this preparatory step really about changing the variable from a simple boolean to a "flag"-style so that it can hold bits for multiple options? Perhaps the commit message should mention something about that?

Missing sign-off.

More below.

Show 23 quoted lines

> ---
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 48526aa..77174f9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -43,7 +43,10 @@ static int marked;
>  #define MAX_IN_VAIN 256
>
>  static struct prio_queue rev_list = { compare_commits_by_commit_date };
> -static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
> +static int non_common_revs, multi_ack, use_sideband;
> +/* Allow specifying sha1 if it is a ref tip. */
> +#define ALLOW_TIP      01
> +static int allow_request_with_bare_object_name;
>
>  static void rev_list_push(struct commit *commit, int mark)
>  {
> @@ -542,7 +545,7 @@ static void filter_refs(struct fetch_pack_args *args,
>         }
>
>         /* Append unmatched requests to the list */
> -       if (allow_tip_sha1_in_want) {
> +       if (allow_request_with_bare_object_name & ALLOW_TIP) {

Some compilers are going to warn about this (warning: did you mean "&&" rather than "&"?). Wrap it in an extra set of parentheses to avoid the warning.

Show 64 quoted lines

>                 for (i = 0; i < nr_sought; i++) {
>                         unsigned char sha1[20];
>
> @@ -821,7 +824,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>         if (server_supports("allow-tip-sha1-in-want")) {
>                 if (args->verbose)
>                         fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
> -               allow_tip_sha1_in_want = 1;
> +               allow_request_with_bare_object_name |= ALLOW_TIP;
>         }
>         if (!server_supports("thin-pack"))
>                 args->use_thin_pack = 0;
> diff --git a/upload-pack.c b/upload-pack.c
> index aa84576..708a502 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -35,7 +35,9 @@ static int multi_ack;
>  static int no_done;
>  static int use_thin_pack, use_ofs_delta, use_include_tag;
>  static int no_progress, daemon_mode;
> -static int allow_tip_sha1_in_want;
> +/* Allow specifying sha1 if it is a ref tip. */
> +#define ALLOW_TIP      01
> +static int allow_request_with_bare_object_name;
>  static int shallow_nr;
>  static struct object_array have_obj;
>  static struct object_array want_obj;
> @@ -442,8 +444,8 @@ static int get_common_commits(void)
>
>  static int is_our_ref(struct object *o)
>  {
> -       return o->flags &
> -               ((allow_tip_sha1_in_want ? HIDDEN_REF : 0) | OUR_REF);
> +       int allow_hidden_ref = (allow_request_with_bare_object_name & ALLOW_TIP);
> +       return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
>  }
>
>  static void check_non_tip(void)
> @@ -727,7 +729,8 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
>                 packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
>                              sha1_to_hex(sha1), refname_nons,
>                              0, capabilities,
> -                            allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
> +                            (allow_request_with_bare_object_name & ALLOW_TIP) ?
> +                                    " allow-tip-sha1-in-want" : "",
>                              stateless_rpc ? " no-done" : "",
>                              symref_info.buf,
>                              git_user_agent_sanitized());
> @@ -787,9 +790,10 @@ static void upload_pack(void)
>
>  static int upload_pack_config(const char *var, const char *value, void *unused)
>  {
> -       if (!strcmp("uploadpack.allowtipsha1inwant", var))
> -               allow_tip_sha1_in_want = git_config_bool(var, value);
> -       else if (!strcmp("uploadpack.keepalive", var)) {
> +       if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
> +               if (git_config_bool(var, value))
> +                       allow_request_with_bare_object_name |= ALLOW_TIP;
> +       } else if (!strcmp("uploadpack.keepalive", var)) {
>                 keepalive = git_config_int(var, value);
>                 if (!keepalive)
>                         keepalive = -1;
> --
> 1.9.1