Merge branch 'jt/upload-pack-v2-fix-shallow'
authorJunio C Hamano <gitster@pobox.com>
Tue, 6 Nov 2018 06:50:19 +0000 (15:50 +0900)
committerJunio C Hamano <gitster@pobox.com>
Tue, 6 Nov 2018 06:50:19 +0000 (15:50 +0900)
"git fetch" over protocol v2 into a shallow repository failed to
fetch full history behind a new tip of history that was diverged
before the cut-off point of the history that was previously fetched
shallowly.

* jt/upload-pack-v2-fix-shallow:
upload-pack: clear flags before each v2 request
upload-pack: make want_obj not global
upload-pack: make have_obj not global

1  2 
t/t5702-protocol-v2.sh
upload-pack.c
diff --combined t/t5702-protocol-v2.sh
index 8360188c01037abe45ef701561fd19ddac6e7ecb,8e3b773e3f5b96b8429ec75ba7d5e233276f34ba..6ab8dea8cd896f5c7656755c89cee9759804bef9
@@@ -79,19 -79,6 +79,19 @@@ test_expect_success 'fetch with git:// 
        grep "fetch< version 2" log
  '
  
 +test_expect_success 'fetch by hash without tag following with protocol v2 does not list refs' '
 +      test_when_finished "rm -f log" &&
 +
 +      test_commit -C "$daemon_parent" two_a &&
 +      git -C "$daemon_parent" rev-parse two_a >two_a_hash &&
 +
 +      GIT_TRACE_PACKET="$(pwd)/log" git -C daemon_child -c protocol.version=2 \
 +              fetch --no-tags origin $(cat two_a_hash) &&
 +
 +      grep "fetch< version 2" log &&
 +      ! grep "fetch> command=ls-refs" log
 +'
 +
  test_expect_success 'pull with git:// using protocol v2' '
        test_when_finished "rm -f log" &&
  
@@@ -299,10 -286,6 +299,10 @@@ test_expect_success 'dynamically fetch 
        grep "version 2" trace
  '
  
 +test_expect_success 'when dynamically fetching missing object, do not list refs' '
 +      ! grep "git> command=ls-refs" trace
 +'
 +
  test_expect_success 'partial fetch' '
        rm -rf client "$(pwd)/trace" &&
        git init client &&
@@@ -446,6 -429,31 +446,31 @@@ test_expect_success 'fetch supports inc
        git -C client cat-file -e $(git -C client rev-parse annotated_tag)
  '
  
+ test_expect_success 'upload-pack respects client shallows' '
+       rm -rf server client trace &&
+       git init server &&
+       test_commit -C server base &&
+       test_commit -C server client_has &&
+       git clone --depth=1 "file://$(pwd)/server" client &&
+       # Add extra commits to the client so that the whole fetch takes more
+       # than 1 request (due to negotiation)
+       for i in $(test_seq 1 32)
+       do
+               test_commit -C client c$i
+       done &&
+       git -C server checkout -b newbranch base &&
+       test_commit -C server client_wants &&
+       GIT_TRACE_PACKET="$(pwd)/trace" git -C client -c protocol.version=2 \
+               fetch origin newbranch &&
+       # Ensure that protocol v2 is used
+       grep "fetch< version 2" trace
+ '
  # Test protocol v2 with 'http://' transport
  #
  . "$TEST_DIRECTORY"/lib-httpd.sh
diff --combined upload-pack.c
index 3f906ccb40e11ccafbeee9a318f5c040ab591689,14e42526ce4c002ecba9d572d8fc4ff12611d33b..5e81f1ff24f141fc5357522cb0745c7ff0aeb189
@@@ -38,6 -38,9 +38,9 @@@
  #define CLIENT_SHALLOW        (1u << 18)
  #define HIDDEN_REF    (1u << 19)
  
+ #define ALL_FLAGS (THEY_HAVE | OUR_REF | WANTED | COMMON_KNOWN | SHALLOW | \
+               NOT_SHALLOW | CLIENT_SHALLOW | HIDDEN_REF)
  static timestamp_t oldest_have;
  
  static int deepen_relative;
@@@ -53,8 -56,6 +56,6 @@@ static int no_progress, daemon_mode
  #define ALLOW_ANY_SHA1        07
  static unsigned int allow_unadvertised_object_request;
  static int shallow_nr;
- static struct object_array have_obj;
- static struct object_array want_obj;
  static struct object_array extra_edge_obj;
  static unsigned int timeout;
  static int keepalive = 5;
@@@ -100,7 -101,8 +101,8 @@@ static int write_one_shallow(const stru
        return 0;
  }
  
- static void create_pack_file(void)
+ static void create_pack_file(const struct object_array *have_obj,
+                            const struct object_array *want_obj)
  {
        struct child_process pack_objects = CHILD_PROCESS_INIT;
        char data[8193], progress[128];
        if (shallow_nr)
                for_each_commit_graft(write_one_shallow, pipe_fd);
  
-       for (i = 0; i < want_obj.nr; i++)
+       for (i = 0; i < want_obj->nr; i++)
                fprintf(pipe_fd, "%s\n",
-                       oid_to_hex(&want_obj.objects[i].item->oid));
+                       oid_to_hex(&want_obj->objects[i].item->oid));
        fprintf(pipe_fd, "--not\n");
-       for (i = 0; i < have_obj.nr; i++)
+       for (i = 0; i < have_obj->nr; i++)
                fprintf(pipe_fd, "%s\n",
-                       oid_to_hex(&have_obj.objects[i].item->oid));
+                       oid_to_hex(&have_obj->objects[i].item->oid));
        for (i = 0; i < extra_edge_obj.nr; i++)
                fprintf(pipe_fd, "%s\n",
                        oid_to_hex(&extra_edge_obj.objects[i].item->oid));
        die("git upload-pack: %s", abort_msg);
  }
  
- static int got_oid(const char *hex, struct object_id *oid)
+ static int got_oid(const char *hex, struct object_id *oid,
+                  struct object_array *have_obj)
  {
        struct object *o;
        int we_knew_they_have = 0;
                        parents->item->object.flags |= THEY_HAVE;
        }
        if (!we_knew_they_have) {
-               add_object_array(o, NULL, &have_obj);
+               add_object_array(o, NULL, have_obj);
                return 1;
        }
        return 0;
  }
  
- static int ok_to_give_up(void)
+ static int ok_to_give_up(const struct object_array *have_obj,
+                        struct object_array *want_obj)
  {
        uint32_t min_generation = GENERATION_NUMBER_ZERO;
  
-       if (!have_obj.nr)
+       if (!have_obj->nr)
                return 0;
  
-       return can_all_from_reach_with_flag(&want_obj, THEY_HAVE,
+       return can_all_from_reach_with_flag(want_obj, THEY_HAVE,
                                            COMMON_KNOWN, oldest_have,
                                            min_generation);
  }
  
- static int get_common_commits(void)
+ static int get_common_commits(struct object_array *have_obj,
+                             struct object_array *want_obj)
  {
        struct object_id oid;
        char last_hex[GIT_MAX_HEXSZ + 1];
  
                if (!line) {
                        if (multi_ack == 2 && got_common
-                           && !got_other && ok_to_give_up()) {
+                           && !got_other && ok_to_give_up(have_obj, want_obj)) {
                                sent_ready = 1;
                                packet_write_fmt(1, "ACK %s ready\n", last_hex);
                        }
-                       if (have_obj.nr == 0 || multi_ack)
+                       if (have_obj->nr == 0 || multi_ack)
                                packet_write_fmt(1, "NAK\n");
  
                        if (no_done && sent_ready) {
                        continue;
                }
                if (skip_prefix(line, "have ", &arg)) {
-                       switch (got_oid(arg, &oid)) {
+                       switch (got_oid(arg, &oid, have_obj)) {
                        case -1: /* they have what we do not */
                                got_other = 1;
-                               if (multi_ack && ok_to_give_up()) {
+                               if (multi_ack && ok_to_give_up(have_obj, want_obj)) {
                                        const char *hex = oid_to_hex(&oid);
                                        if (multi_ack == 2) {
                                                sent_ready = 1;
                                        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 (have_obj->nr == 1)
                                        packet_write_fmt(1, "ACK %s\n", last_hex);
                                break;
                        }
                        continue;
                }
                if (!strcmp(line, "done")) {
-                       if (have_obj.nr > 0) {
+                       if (have_obj->nr > 0) {
                                if (multi_ack)
                                        packet_write_fmt(1, "ACK %s\n", last_hex);
                                return 0;
@@@ -444,7 -449,6 +449,7 @@@ static int do_reachable_revlist(struct 
        struct object *o;
        char namebuf[GIT_MAX_HEXSZ + 2]; /* ^ + hash + LF */
        int i;
 +      const unsigned hexsz = the_hash_algo->hexsz;
  
        cmd->argv = argv;
        cmd->git_cmd = 1;
                goto error;
  
        namebuf[0] = '^';
 -      namebuf[GIT_SHA1_HEXSZ + 1] = '\n';
 +      namebuf[hexsz + 1] = '\n';
        for (i = get_max_object_index(); 0 < i; ) {
                o = get_indexed_object(--i);
                if (!o)
                        o->flags &= ~TMP_MARK;
                if (!is_our_ref(o))
                        continue;
 -              memcpy(namebuf + 1, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
 -              if (write_in_full(cmd->in, namebuf, GIT_SHA1_HEXSZ + 2) < 0)
 +              memcpy(namebuf + 1, oid_to_hex(&o->oid), hexsz);
 +              if (write_in_full(cmd->in, namebuf, hexsz + 2) < 0)
                        goto error;
        }
 -      namebuf[GIT_SHA1_HEXSZ] = '\n';
 +      namebuf[hexsz] = '\n';
        for (i = 0; i < src->nr; i++) {
                o = src->objects[i].item;
                if (is_our_ref(o)) {
                }
                if (reachable && o->type == OBJ_COMMIT)
                        o->flags |= TMP_MARK;
 -              memcpy(namebuf, oid_to_hex(&o->oid), GIT_SHA1_HEXSZ);
 -              if (write_in_full(cmd->in, namebuf, GIT_SHA1_HEXSZ + 1) < 0)
 +              memcpy(namebuf, oid_to_hex(&o->oid), hexsz);
 +              if (write_in_full(cmd->in, namebuf, hexsz + 1) < 0)
                        goto error;
        }
        close(cmd->in);
@@@ -583,7 -587,7 +588,7 @@@ error
        return 1;
  }
  
- static void check_non_tip(void)
+ static void check_non_tip(struct object_array *want_obj)
  {
        int i;
  
         */
        if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
                goto error;
-       if (!has_unreachable(&want_obj))
+       if (!has_unreachable(want_obj))
                /* All the non-tip ones are ancestors of what we advertised */
                return;
  
  error:
        /* Pick one of them (we know there at least is one) */
-       for (i = 0; i < want_obj.nr; i++) {
-               struct object *o = want_obj.objects[i].item;
+       for (i = 0; i < want_obj->nr; i++) {
+               struct object *o = want_obj->objects[i].item;
                if (!is_our_ref(o))
                        die("git upload-pack: not our ref %s",
                            oid_to_hex(&o->oid));
@@@ -622,7 -626,8 +627,8 @@@ static void send_shallow(struct commit_
        }
  }
  
- static void send_unshallow(const struct object_array *shallows)
+ static void send_unshallow(const struct object_array *shallows,
+                          struct object_array *want_obj)
  {
        int i;
  
                        parents = ((struct commit *)object)->parents;
                        while (parents) {
                                add_object_array(&parents->item->object,
-                                                NULL, &want_obj);
+                                                NULL, want_obj);
                                parents = parents->next;
                        }
                        add_object_array(object, NULL, &extra_edge_obj);
  }
  
  static void deepen(int depth, int deepen_relative,
-                  struct object_array *shallows)
+                  struct object_array *shallows, struct object_array *want_obj)
  {
        if (depth == INFINITE_DEPTH && !is_repository_shallow(the_repository)) {
                int i;
        } else {
                struct commit_list *result;
  
-               result = get_shallow_commits(&want_obj, depth,
+               result = get_shallow_commits(want_obj, depth,
                                             SHALLOW, NOT_SHALLOW);
                send_shallow(result);
                free_commit_list(result);
        }
  
-       send_unshallow(shallows);
+       send_unshallow(shallows, want_obj);
  }
  
  static void deepen_by_rev_list(int ac, const char **av,
-                              struct object_array *shallows)
+                              struct object_array *shallows,
+                              struct object_array *want_obj)
  {
        struct commit_list *result;
  
        result = get_shallow_commits_by_rev_list(ac, av, SHALLOW, NOT_SHALLOW);
        send_shallow(result);
        free_commit_list(result);
-       send_unshallow(shallows);
+       send_unshallow(shallows, want_obj);
  }
  
  /* Returns 1 if a shallow list is sent or 0 otherwise */
  static int send_shallow_list(int depth, int deepen_rev_list,
                             timestamp_t deepen_since,
                             struct string_list *deepen_not,
-                            struct object_array *shallows)
+                            struct object_array *shallows,
+                            struct object_array *want_obj)
  {
        int ret = 0;
  
        if (depth > 0 && deepen_rev_list)
                die("git upload-pack: deepen and deepen-since (or deepen-not) cannot be used together");
        if (depth > 0) {
-               deepen(depth, deepen_relative, shallows);
+               deepen(depth, deepen_relative, shallows, want_obj);
                ret = 1;
        } else if (deepen_rev_list) {
                struct argv_array av = ARGV_ARRAY_INIT;
                        }
                        argv_array_push(&av, "--not");
                }
-               for (i = 0; i < want_obj.nr; i++) {
-                       struct object *o = want_obj.objects[i].item;
+               for (i = 0; i < want_obj->nr; i++) {
+                       struct object *o = want_obj->objects[i].item;
                        argv_array_push(&av, oid_to_hex(&o->oid));
                }
-               deepen_by_rev_list(av.argc, av.argv, shallows);
+               deepen_by_rev_list(av.argc, av.argv, shallows, want_obj);
                argv_array_clear(&av);
                ret = 1;
        } else {
@@@ -818,7 -825,7 +826,7 @@@ static int process_deepen_not(const cha
        return 0;
  }
  
- static void receive_needs(void)
+ static void receive_needs(struct object_array *want_obj)
  {
        struct object_array shallows = OBJECT_ARRAY_INIT;
        struct string_list deepen_not = STRING_LIST_INIT_DUP;
                        if (!((allow_unadvertised_object_request & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
                              || is_our_ref(o)))
                                has_non_tip = 1;
-                       add_object_array(o, NULL, &want_obj);
+                       add_object_array(o, NULL, want_obj);
                }
        }
  
         * by another process that handled the initial request.
         */
        if (has_non_tip)
-               check_non_tip();
+               check_non_tip(want_obj);
  
        if (!use_sideband && daemon_mode)
                no_progress = 1;
                return;
  
        if (send_shallow_list(depth, deepen_rev_list, deepen_since,
-                             &deepen_not, &shallows))
+                             &deepen_not, &shallows, want_obj))
                packet_flush(1);
        object_array_clear(&shallows);
  }
@@@ -1029,23 -1036,21 +1037,24 @@@ static int upload_pack_config(const cha
                keepalive = git_config_int(var, value);
                if (!keepalive)
                        keepalive = -1;
 -      } else if (current_config_scope() != CONFIG_SCOPE_REPO) {
 -              if (!strcmp("uploadpack.packobjectshook", var))
 -                      return git_config_string(&pack_objects_hook, var, value);
        } else if (!strcmp("uploadpack.allowfilter", var)) {
                allow_filter = git_config_bool(var, value);
        } else if (!strcmp("uploadpack.allowrefinwant", var)) {
                allow_ref_in_want = git_config_bool(var, value);
        }
 +
 +      if (current_config_scope() != CONFIG_SCOPE_REPO) {
 +              if (!strcmp("uploadpack.packobjectshook", var))
 +                      return git_config_string(&pack_objects_hook, var, value);
 +      }
 +
        return parse_hide_refs_config(var, value, "uploadpack");
  }
  
  void upload_pack(struct upload_pack_options *options)
  {
        struct string_list symref = STRING_LIST_INIT_DUP;
+       struct object_array want_obj = OBJECT_ARRAY_INIT;
  
        stateless_rpc = options->stateless_rpc;
        timeout = options->timeout;
        if (options->advertise_refs)
                return;
  
-       receive_needs();
+       receive_needs(&want_obj);
        if (want_obj.nr) {
-               get_common_commits();
-               create_pack_file();
+               struct object_array have_obj = OBJECT_ARRAY_INIT;
+               get_common_commits(&have_obj, &want_obj);
+               create_pack_file(&have_obj, &want_obj);
        }
  }
  
@@@ -1122,7 -1128,7 +1132,7 @@@ static void upload_pack_data_clear(stru
        string_list_clear(&data->deepen_not, 0);
  }
  
- static int parse_want(const char *line)
+ static int parse_want(const char *line, struct object_array *want_obj)
  {
        const char *arg;
        if (skip_prefix(line, "want ", &arg)) {
  
                if (!(o->flags & WANTED)) {
                        o->flags |= WANTED;
-                       add_object_array(o, NULL, &want_obj);
+                       add_object_array(o, NULL, want_obj);
                }
  
                return 1;
        return 0;
  }
  
- static int parse_want_ref(const char *line, struct string_list *wanted_refs)
+ static int parse_want_ref(const char *line, struct string_list *wanted_refs,
+                         struct object_array *want_obj)
  {
        const char *arg;
        if (skip_prefix(line, "want-ref ", &arg)) {
                o = parse_object_or_die(&oid, arg);
                if (!(o->flags & WANTED)) {
                        o->flags |= WANTED;
-                       add_object_array(o, NULL, &want_obj);
+                       add_object_array(o, NULL, want_obj);
                }
  
                return 1;
@@@ -1197,16 -1204,18 +1208,18 @@@ static int parse_have(const char *line
  }
  
  static void process_args(struct packet_reader *request,
-                        struct upload_pack_data *data)
+                        struct upload_pack_data *data,
+                        struct object_array *want_obj)
  {
        while (packet_reader_read(request) != PACKET_READ_FLUSH) {
                const char *arg = request->line;
                const char *p;
  
                /* process want */
-               if (parse_want(arg))
+               if (parse_want(arg, want_obj))
                        continue;
-               if (allow_ref_in_want && parse_want_ref(arg, &data->wanted_refs))
+               if (allow_ref_in_want &&
+                   parse_want_ref(arg, &data->wanted_refs, want_obj))
                        continue;
                /* process have line */
                if (parse_have(arg, &data->haves))
        }
  }
  
- static int process_haves(struct oid_array *haves, struct oid_array *common)
+ static int process_haves(struct oid_array *haves, struct oid_array *common,
+                        struct object_array *have_obj)
  {
        int i;
  
                                parents->item->object.flags |= THEY_HAVE;
                }
                if (!we_knew_they_have)
-                       add_object_array(o, NULL, &have_obj);
+                       add_object_array(o, NULL, have_obj);
        }
  
        return 0;
  }
  
- static int send_acks(struct oid_array *acks, struct strbuf *response)
+ static int send_acks(struct oid_array *acks, struct strbuf *response,
+                    const struct object_array *have_obj,
+                    struct object_array *want_obj)
  {
        int i;
  
                                 oid_to_hex(&acks->oid[i]));
        }
  
-       if (ok_to_give_up()) {
+       if (ok_to_give_up(have_obj, want_obj)) {
                /* Send Ready */
                packet_buf_write(response, "ready\n");
                return 1;
        return 0;
  }
  
- static int process_haves_and_send_acks(struct upload_pack_data *data)
+ static int process_haves_and_send_acks(struct upload_pack_data *data,
+                                      struct object_array *have_obj,
+                                      struct object_array *want_obj)
  {
        struct oid_array common = OID_ARRAY_INIT;
        struct strbuf response = STRBUF_INIT;
        int ret = 0;
  
-       process_haves(&data->haves, &common);
+       process_haves(&data->haves, &common, have_obj);
        if (data->done) {
                ret = 1;
-       } else if (send_acks(&common, &response)) {
+       } else if (send_acks(&common, &response, have_obj, want_obj)) {
                packet_buf_delim(&response);
                ret = 1;
        } else {
@@@ -1368,7 -1382,8 +1386,8 @@@ static void send_wanted_ref_info(struc
        packet_delim(1);
  }
  
- static void send_shallow_info(struct upload_pack_data *data)
+ static void send_shallow_info(struct upload_pack_data *data,
+                             struct object_array *want_obj)
  {
        /* No shallow info needs to be sent */
        if (!data->depth && !data->deepen_rev_list && !data->shallows.nr &&
  
        if (!send_shallow_list(data->depth, data->deepen_rev_list,
                               data->deepen_since, &data->deepen_not,
-                              &data->shallows) &&
+                              &data->shallows, want_obj) &&
            is_repository_shallow(the_repository))
-               deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows);
+               deepen(INFINITE_DEPTH, data->deepen_relative, &data->shallows,
+                      want_obj);
  
        packet_delim(1);
  }
@@@ -1398,6 -1414,10 +1418,10 @@@ int upload_pack_v2(struct repository *r
  {
        enum fetch_state state = FETCH_PROCESS_ARGS;
        struct upload_pack_data data;
+       struct object_array have_obj = OBJECT_ARRAY_INIT;
+       struct object_array want_obj = OBJECT_ARRAY_INIT;
+       clear_object_flags(ALL_FLAGS);
  
        git_config(upload_pack_config, NULL);
  
        while (state != FETCH_DONE) {
                switch (state) {
                case FETCH_PROCESS_ARGS:
-                       process_args(request, &data);
+                       process_args(request, &data, &want_obj);
  
                        if (!want_obj.nr) {
                                /*
                        }
                        break;
                case FETCH_SEND_ACKS:
-                       if (process_haves_and_send_acks(&data))
+                       if (process_haves_and_send_acks(&data, &have_obj,
+                                                       &want_obj))
                                state = FETCH_SEND_PACK;
                        else
                                state = FETCH_DONE;
                        break;
                case FETCH_SEND_PACK:
                        send_wanted_ref_info(&data);
-                       send_shallow_info(&data);
+                       send_shallow_info(&data, &want_obj);
  
                        packet_write_fmt(1, "packfile\n");
-                       create_pack_file();
+                       create_pack_file(&have_obj, &want_obj);
                        state = FETCH_DONE;
                        break;
                case FETCH_DONE:
        }
  
        upload_pack_data_clear(&data);
+       object_array_clear(&have_obj);
+       object_array_clear(&want_obj);
        return 0;
  }