Merge branch 'fc/fetch-with-import-fix' into maint
authorJunio C Hamano <gitster@pobox.com>
Mon, 29 Jul 2019 19:38:23 +0000 (12:38 -0700)
committerJunio C Hamano <gitster@pobox.com>
Mon, 29 Jul 2019 19:38:23 +0000 (12:38 -0700)
Code restructuring during 2.20 period broke fetching tags via
"import" based transports.

* fc/fetch-with-import-fix:
fetch: fix regression with transport helpers
fetch: make the code more understandable
fetch: trivial cleanup
t5801 (remote-helpers): add test to fetch tags
t5801 (remote-helpers): cleanup refspec stuff

1  2 
builtin/fetch.c
t/t5801-remote-helpers.sh
t/t5801/git-remote-testgit
diff --combined builtin/fetch.c
index fc6c879bcf9ace22492166379ca7fad8186a8d7d,e485d429c9e7f580010a4ee6a011106954d17ad5..c9b92b1e52448bb1537a229b3ed7d1587db28ecf
@@@ -98,8 -98,6 +98,8 @@@ static int git_fetch_config(const char 
  
  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
  {
 +      BUG_ON_OPT_NEG(unset);
 +
        /*
         * "git fetch --refmap='' origin foo"
         * can be used to tell the command not to store anywhere
@@@ -239,6 -237,7 +239,7 @@@ static int will_fetch(struct ref **head
  struct refname_hash_entry {
        struct hashmap_entry ent; /* must be the first member */
        struct object_id oid;
+       int ignore;
        char refname[FLEX_ARRAY];
  };
  
@@@ -287,6 -286,11 +288,11 @@@ static int refname_hash_exists(struct h
        return !!hashmap_get_from_hash(map, strhash(refname), refname);
  }
  
+ static void clear_item(struct refname_hash_entry *item)
+ {
+       item->ignore = 1;
+ }
  static void find_non_local_tags(const struct ref *refs,
                                struct ref **head,
                                struct ref ***tail)
                            !has_object_file_with_flags(&ref->old_oid,
                                                        OBJECT_INFO_QUICK) &&
                            !will_fetch(head, ref->old_oid.hash) &&
 -                          !has_sha1_file_with_flags(item->oid.hash,
 -                                                    OBJECT_INFO_QUICK) &&
 +                          !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
                            !will_fetch(head, item->oid.hash))
-                               oidclr(&item->oid);
+                               clear_item(item);
                        item = NULL;
                        continue;
                }
                 * fetch.
                 */
                if (item &&
 -                  !has_sha1_file_with_flags(item->oid.hash, OBJECT_INFO_QUICK) &&
 +                  !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
                    !will_fetch(head, item->oid.hash))
-                       oidclr(&item->oid);
+                       clear_item(item);
  
                item = NULL;
  
         * checked to see if it needs fetching.
         */
        if (item &&
 -          !has_sha1_file_with_flags(item->oid.hash, OBJECT_INFO_QUICK) &&
 +          !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
            !will_fetch(head, item->oid.hash))
-               oidclr(&item->oid);
+               clear_item(item);
  
        /*
         * For all the tags in the remote_refs_list,
         */
        for_each_string_list_item(remote_ref_item, &remote_refs_list) {
                const char *refname = remote_ref_item->string;
+               struct ref *rm;
  
                item = hashmap_get_from_hash(&remote_refs, strhash(refname), refname);
                if (!item)
                        BUG("unseen remote ref?");
  
                /* Unless we have already decided to ignore this item... */
-               if (!is_null_oid(&item->oid)) {
-                       struct ref *rm = alloc_ref(item->refname);
-                       rm->peer_ref = alloc_ref(item->refname);
-                       oidcpy(&rm->old_oid, &item->oid);
-                       **tail = rm;
-                       *tail = &rm->next;
-               }
+               if (item->ignore)
+                       continue;
+               rm = alloc_ref(item->refname);
+               rm->peer_ref = alloc_ref(item->refname);
+               oidcpy(&rm->old_oid, &item->oid);
+               **tail = rm;
+               *tail = &rm->next;
        }
        hashmap_free(&remote_refs, 1);
        string_list_clear(&remote_refs_list, 0);
@@@ -628,14 -635,9 +636,14 @@@ static int find_and_replace(struct strb
                            const char *needle,
                            const char *placeholder)
  {
 -      const char *p = strstr(haystack->buf, needle);
 +      const char *p = NULL;
        int plen, nlen;
  
 +      nlen = strlen(needle);
 +      if (ends_with(haystack->buf, needle))
 +              p = haystack->buf + haystack->len - nlen;
 +      else
 +              p = strstr(haystack->buf, needle);
        if (!p)
                return 0;
  
                return 0;
  
        plen = strlen(p);
 -      nlen = strlen(needle);
        if (plen > nlen && p[nlen] != '/')
                return 0;
  
@@@ -766,6 -769,9 +774,6 @@@ static int update_local_ref(struct ref 
                        what = _("[new ref]");
                }
  
 -              if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
 -                  (recurse_submodules != RECURSE_SUBMODULES_ON))
 -                      check_for_new_submodule_commits(&ref->new_oid);
                r = s_update_ref(msg, ref, 0);
                format_display(display, r ? '!' : '*', what,
                               r ? _("unable to update local ref") : NULL,
                strbuf_add_unique_abbrev(&quickref, &current->object.oid, DEFAULT_ABBREV);
                strbuf_addstr(&quickref, "..");
                strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 -              if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
 -                  (recurse_submodules != RECURSE_SUBMODULES_ON))
 -                      check_for_new_submodule_commits(&ref->new_oid);
                r = s_update_ref("fast-forward", ref, 1);
                format_display(display, r ? '!' : ' ', quickref.buf,
                               r ? _("unable to update local ref") : NULL,
                strbuf_add_unique_abbrev(&quickref, &current->object.oid, DEFAULT_ABBREV);
                strbuf_addstr(&quickref, "...");
                strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV);
 -              if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
 -                  (recurse_submodules != RECURSE_SUBMODULES_ON))
 -                      check_for_new_submodule_commits(&ref->new_oid);
                r = s_update_ref("forced-update", ref, 1);
                format_display(display, r ? '!' : '+', quickref.buf,
                               r ? _("unable to update local ref") : _("forced update"),
@@@ -886,8 -898,6 +894,8 @@@ static int store_updated_refs(const cha
                                ref->force = rm->peer_ref->force;
                        }
  
 +                      if (recurse_submodules != RECURSE_SUBMODULES_OFF)
 +                              check_for_new_submodule_commits(&rm->old_oid);
  
                        if (!strcmp(rm->name, "HEAD")) {
                                kind = "";
   * everything we are going to fetch already exists and is connected
   * locally.
   */
 -static int quickfetch(struct ref *ref_map)
 +static int check_exist_and_connected(struct ref *ref_map)
  {
        struct ref *rm = ref_map;
        struct check_connected_options opt = CHECK_CONNECTED_INIT;
 +      struct ref *r;
  
        /*
         * If we are deepening a shallow clone we already have these
         */
        if (deepen)
                return -1;
 +
 +      /*
 +       * check_connected() allows objects to merely be promised, but
 +       * we need all direct targets to exist.
 +       */
 +      for (r = rm; r; r = r->next) {
 +              if (!has_object_file(&r->old_oid))
 +                      return -1;
 +      }
 +
        opt.quiet = 1;
        return check_connected(iterate_ref_map, &rm, &opt);
  }
  
  static int fetch_refs(struct transport *transport, struct ref *ref_map)
  {
 -      int ret = quickfetch(ref_map);
 +      int ret = check_exist_and_connected(ref_map);
        if (ret)
                ret = transport_fetch_refs(transport, ref_map);
        if (!ret)
@@@ -1168,7 -1167,6 +1176,7 @@@ static void add_negotiation_tips(struc
  static struct transport *prepare_transport(struct remote *remote, int deepen)
  {
        struct transport *transport;
 +
        transport = transport_get(remote, NULL);
        transport_set_verbosity(transport, verbosity, progress);
        transport->family = family;
        if (update_shallow)
                set_option(transport, TRANS_OPT_UPDATE_SHALLOW, "yes");
        if (filter_options.choice) {
 +              struct strbuf expanded_filter_spec = STRBUF_INIT;
 +              expand_list_objects_filter_spec(&filter_options,
 +                                              &expanded_filter_spec);
                set_option(transport, TRANS_OPT_LIST_OBJECTS_FILTER,
 -                         filter_options.filter_spec);
 +                         expanded_filter_spec.buf);
                set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
 +              strbuf_release(&expanded_filter_spec);
        }
        if (negotiation_tip.nr) {
                if (transport->smart_options)
@@@ -1243,7 -1237,6 +1251,7 @@@ static int do_fetch(struct transport *t
        int retcode = 0;
        const struct ref *remote_refs;
        struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
 +      int must_list_refs = 1;
  
        if (tags == TAGS_DEFAULT) {
                if (transport->remote->fetch_tags == 2)
                        goto cleanup;
        }
  
 -      if (rs->nr)
 +      if (rs->nr) {
 +              int i;
 +
                refspec_ref_prefixes(rs, &ref_prefixes);
 -      else if (transport->remote && transport->remote->fetch.nr)
 +
 +              /*
 +               * We can avoid listing refs if all of them are exact
 +               * OIDs
 +               */
 +              must_list_refs = 0;
 +              for (i = 0; i < rs->nr; i++) {
 +                      if (!rs->items[i].exact_sha1) {
 +                              must_list_refs = 1;
 +                              break;
 +                      }
 +              }
 +      } else if (transport->remote && transport->remote->fetch.nr)
                refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
  
 -      if (ref_prefixes.argc &&
 -          (tags == TAGS_SET || (tags == TAGS_DEFAULT))) {
 -              argv_array_push(&ref_prefixes, "refs/tags/");
 +      if (tags == TAGS_SET || tags == TAGS_DEFAULT) {
 +              must_list_refs = 1;
 +              if (ref_prefixes.argc)
 +                      argv_array_push(&ref_prefixes, "refs/tags/");
        }
  
 -      remote_refs = transport_get_remote_refs(transport, &ref_prefixes);
 +      if (must_list_refs)
 +              remote_refs = transport_get_remote_refs(transport, &ref_prefixes);
 +      else
 +              remote_refs = NULL;
 +
        argv_array_clear(&ref_prefixes);
  
        ref_map = get_ref_map(transport->remote, remote_refs, rs,
@@@ -1479,8 -1453,7 +1487,8 @@@ static inline void fetch_one_setup_part
         */
        if (strcmp(remote->name, repository_format_partial_clone)) {
                if (filter_options.choice)
 -                      die(_("--filter can only be used with the remote configured in core.partialClone"));
 +                      die(_("--filter can only be used with the remote "
 +                            "configured in extensions.partialClone"));
                return;
        }
  
@@@ -1556,9 -1529,7 +1564,9 @@@ static int fetch_one(struct remote *rem
  
        sigchain_push_common(unlock_pack_on_signal);
        atexit(unlock_pack);
 +      sigchain_push(SIGPIPE, SIG_IGN);
        exit_code = do_fetch(gtransport, &rs);
 +      sigchain_pop(SIGPIPE);
        refspec_clear(&rs);
        transport_disconnect(gtransport);
        gtransport = NULL;
@@@ -1650,8 -1621,7 +1658,8 @@@ int cmd_fetch(int argc, const char **ar
                result = fetch_one(remote, argc, argv, prune_tags_ok);
        } else {
                if (filter_options.choice)
 -                      die(_("--filter can only be used with the remote configured in core.partialClone"));
 +                      die(_("--filter can only be used with the remote "
 +                            "configured in extensions.partialclone"));
                /* TODO should this also die if we have a previous partial-clone? */
                result = fetch_multiple(&list);
        }
  
        string_list_clear(&list, 0);
  
 -      close_all_packs(the_repository->objects);
 +      close_object_store(the_repository->objects);
  
        argv_array_pushl(&argv_gc_auto, "gc", "--auto", NULL);
        if (verbosity < 0)
index d04f8007e0e34fa938bf4fccb91eff384743e616,1829ef29c75b2224c61081ee8c57c091cd2e1451..2d6c4a281edb3f0678a600fd3e780825536da437
@@@ -8,8 -8,6 +8,8 @@@ test_description='Test remote-helper im
  . ./test-lib.sh
  . "$TEST_DIRECTORY"/lib-gpg.sh
  
 +PATH="$TEST_DIRECTORY/t5801:$PATH"
 +
  compare_refs() {
        git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
        git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
@@@ -126,7 -124,7 +126,7 @@@ test_expect_success 'forced push' 
  '
  
  test_expect_success 'cloning without refspec' '
-       GIT_REMOTE_TESTGIT_REFSPEC="" \
+       GIT_REMOTE_TESTGIT_NOREFSPEC=1 \
        git clone "testgit::${PWD}/server" local2 2>error &&
        test_i18ngrep "this remote helper should implement refspec capability" error &&
        compare_refs local2 HEAD server HEAD
  test_expect_success 'pulling without refspecs' '
        (cd local2 &&
        git reset --hard &&
-       GIT_REMOTE_TESTGIT_REFSPEC="" git pull 2>../error) &&
+       GIT_REMOTE_TESTGIT_NOREFSPEC=1 git pull 2>../error) &&
        test_i18ngrep "this remote helper should implement refspec capability" error &&
        compare_refs local2 HEAD server HEAD
  '
@@@ -145,8 -143,8 +145,8 @@@ test_expect_success 'pushing without re
        (cd local2 &&
        echo content >>file &&
        git commit -a -m ten &&
-       GIT_REMOTE_TESTGIT_REFSPEC="" &&
-       export GIT_REMOTE_TESTGIT_REFSPEC &&
+       GIT_REMOTE_TESTGIT_NOREFSPEC=1 &&
+       export GIT_REMOTE_TESTGIT_NOREFSPEC &&
        test_must_fail git push 2>../error) &&
        test_i18ngrep "remote-helper doesn.t support push; refspec needed" error
  '
@@@ -303,4 -301,14 +303,14 @@@ test_expect_success 'fetch url' 
        compare_refs server HEAD local FETCH_HEAD
  '
  
+ test_expect_success 'fetch tag' '
+       (cd server &&
+        git tag v1.0
+       ) &&
+       (cd local &&
+        git fetch
+       ) &&
+       compare_refs local v1.0 server v1.0
+ '
  test_done
index 752c763eb666e197304efbc7ea006325a36ff870,0000000000000000000000000000000000000000..6b9f0b5dc79cf0239daf4f6a210baaccf8612d74
mode 100755,000000..100755
--- /dev/null
@@@ -1,147 -1,0 +1,151 @@@
- prefix="refs/testgit/$alias"
 +#!/bin/sh
 +# Copyright (c) 2012 Felipe Contreras
 +
 +# The first argument can be a url when the fetch/push command was a url
 +# instead of a configured remote. In this case, use a generic alias.
 +if test "$1" = "testgit::$2"; then
 +      alias=_
 +else
 +      alias=$1
 +fi
 +url=$2
 +
 +dir="$GIT_DIR/testgit/$alias"
- default_refspec="refs/heads/*:${prefix}/heads/*"
 +
- refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}"
- test -z "$refspec" && prefix="refs"
++h_refspec="refs/heads/*:refs/testgit/$alias/heads/*"
++t_refspec="refs/tags/*:refs/testgit/$alias/tags/*"
 +
-               test -n "$refspec" && echo "refspec $refspec"
++if test -n "$GIT_REMOTE_TESTGIT_NOREFSPEC"
++then
++      h_refspec=""
++      t_refspec=""
++fi
 +
 +GIT_DIR="$url/.git"
 +export GIT_DIR
 +
 +force=
 +
 +mkdir -p "$dir"
 +
 +if test -z "$GIT_REMOTE_TESTGIT_NO_MARKS"
 +then
 +      gitmarks="$dir/git.marks"
 +      testgitmarks="$dir/testgit.marks"
 +      test -e "$gitmarks" || >"$gitmarks"
 +      test -e "$testgitmarks" || >"$testgitmarks"
 +fi
 +
 +while read line
 +do
 +      case $line in
 +      capabilities)
 +              echo 'import'
 +              echo 'export'
-               git for-each-ref --format='? %(refname)' 'refs/heads/'
++              test -n "$h_refspec" && echo "refspec $h_refspec"
++              test -n "$t_refspec" && echo "refspec $t_refspec"
 +              if test -n "$gitmarks"
 +              then
 +                      echo "*import-marks $gitmarks"
 +                      echo "*export-marks $gitmarks"
 +              fi
 +              test -n "$GIT_REMOTE_TESTGIT_SIGNED_TAGS" && echo "signed-tags"
 +              test -n "$GIT_REMOTE_TESTGIT_NO_PRIVATE_UPDATE" && echo "no-private-update"
 +              echo 'option'
 +              echo
 +              ;;
 +      list)
-                       $refs |
-               sed -e "s#refs/heads/#${prefix}/heads/#g"
++              git for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/'
 +              head=$(git symbolic-ref HEAD)
 +              echo "@$head HEAD"
 +              echo
 +              ;;
 +      import*)
 +              # read all import lines
 +              while true
 +              do
 +                      ref="${line#* }"
 +                      refs="$refs $ref"
 +                      read line
 +                      test "${line%% *}" != "import" && break
 +              done
 +
 +              if test -n "$gitmarks"
 +              then
 +                      echo "feature import-marks=$gitmarks"
 +                      echo "feature export-marks=$gitmarks"
 +              fi
 +
 +              if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
 +              then
 +                      echo "feature done"
 +                      exit 1
 +              fi
 +
 +              echo "feature done"
 +              git fast-export \
++                      ${h_refspec:+"--refspec=$h_refspec"} \
++                      ${t_refspec:+"--refspec=$t_refspec"} \
 +                      ${testgitmarks:+"--import-marks=$testgitmarks"} \
 +                      ${testgitmarks:+"--export-marks=$testgitmarks"} \
++                      $refs
 +              echo "done"
 +              ;;
 +      export)
 +              if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
 +              then
 +                      # consume input so fast-export doesn't get SIGPIPE;
 +                      # git would also notice that case, but we want
 +                      # to make sure we are exercising the later
 +                      # error checks
 +                      while read line; do
 +                              test "done" = "$line" && break
 +                      done
 +                      exit 1
 +              fi
 +
 +              before=$(git for-each-ref --format=' %(refname) %(objectname) ')
 +
 +              git fast-import \
 +                      ${force:+--force} \
 +                      ${testgitmarks:+"--import-marks=$testgitmarks"} \
 +                      ${testgitmarks:+"--export-marks=$testgitmarks"} \
 +                      --quiet
 +
 +              # figure out which refs were updated
 +              git for-each-ref --format='%(refname) %(objectname)' |
 +              while read ref a
 +              do
 +                      case "$before" in
 +                      *" $ref $a "*)
 +                              continue ;;     # unchanged
 +                      esac
 +                      if test -z "$GIT_REMOTE_TESTGIT_PUSH_ERROR"
 +                      then
 +                              echo "ok $ref"
 +                      else
 +                              echo "error $ref $GIT_REMOTE_TESTGIT_PUSH_ERROR"
 +                      fi
 +              done
 +
 +              echo
 +              ;;
 +      option\ *)
 +              read cmd opt val <<-EOF
 +              $line
 +              EOF
 +              case $opt in
 +              force)
 +                      test $val = "true" && force="true" || force=
 +                      echo "ok"
 +                      ;;
 +              *)
 +                      echo "unsupported"
 +                      ;;
 +              esac
 +              ;;
 +      '')
 +              exit
 +              ;;
 +      esac
 +done