Merge branch 'ab/fetch-tags-noclobber'
authorJunio C Hamano <gitster@pobox.com>
Mon, 17 Sep 2018 20:54:00 +0000 (13:54 -0700)
committerJunio C Hamano <gitster@pobox.com>
Mon, 17 Sep 2018 20:54:00 +0000 (13:54 -0700)
The rules used by "git push" and "git fetch" to determine if a ref
can or cannot be updated were inconsistent; specifically, fetching
to update existing tags were allowed even though tags are supposed
to be unmoving anchoring points. "git fetch" was taught to forbid
updates to existing tags without the "--force" option.

* ab/fetch-tags-noclobber:
fetch: stop clobbering existing tags without --force
fetch: document local ref updates with/without --force
push doc: correct lies about how push refspecs work
push doc: move mention of "tag <tag>" later in the prose
push doc: remove confusing mention of remote merger
fetch tests: add a test for clobbering tag behavior
push tests: use spaces in interpolated string
push tests: make use of unused $1 in test description
fetch: change "branch" to "reference" in --force -h output

1  2 
Documentation/fetch-options.txt
builtin/fetch.c
t/t5516-fetch-push.sh
t/t5612-clone-refspec.sh
index 8bc36af4b1b10375c81c6f700ea300cee2295cc1,5b624caf5854b5ee1d4386b78f9e0a7aa09180b1..fa0a3151b3f7e96ee61e65669e057e48ee053aae
@@@ -42,25 -42,6 +42,25 @@@ the current repository has the same his
        .git/shallow. This option updates .git/shallow and accept such
        refs.
  
 +--negotiation-tip=<commit|glob>::
 +      By default, Git will report, to the server, commits reachable
 +      from all local refs to find common commits in an attempt to
 +      reduce the size of the to-be-received packfile. If specified,
 +      Git will only report commits reachable from the given tips.
 +      This is useful to speed up fetches when the user knows which
 +      local ref is likely to have commits in common with the
 +      upstream ref being fetched.
 ++
 +This option may be specified more than once; if so, Git will report
 +commits reachable from any of the given commits.
 ++
 +The argument to this option may be a glob on ref names, a ref, or the (possibly
 +abbreviated) SHA-1 of a commit. Specifying a glob is equivalent to specifying
 +this option multiple times, one for each matching ref name.
 ++
 +See also the `fetch.negotiationAlgorithm` configuration variable
 +documented in linkgit:git-config[1].
 +
  ifndef::git-pull[]
  --dry-run::
        Show what would be done, without making any changes.
@@@ -68,11 -49,16 +68,16 @@@ endif::git-pull[
  
  -f::
  --force::
-       When 'git fetch' is used with `<rbranch>:<lbranch>`
-       refspec, it refuses to update the local branch
-       `<lbranch>` unless the remote branch `<rbranch>` it
-       fetches is a descendant of `<lbranch>`.  This option
-       overrides that check.
+       When 'git fetch' is used with `<src>:<dst>` refspec it may
+       refuse to update the local branch as discussed
+ ifdef::git-pull[]
+       in the `<refspec>` part of the linkgit:git-fetch[1]
+       documentation.
+ endif::git-pull[]
+ ifndef::git-pull[]
+       in the `<refspec>` part below.
+ endif::git-pull[]
+       This option overrides that check.
  
  -k::
  --keep::
diff --combined builtin/fetch.c
index dc0931fb464e5b81badf3a0a81f5dfd4b52129bf,683f70d71eda87196c4a4dd86cdce87bcc347e99..0696abfc2a158baa3177fa9e71e2c16bc64655a4
@@@ -22,7 -22,6 +22,7 @@@
  #include "utf8.h"
  #include "packfile.h"
  #include "list-objects-filter-options.h"
 +#include "commit-reach.h"
  
  static const char * const builtin_fetch_usage[] = {
        N_("git fetch [<options>] [<repository> [<refspec>...]]"),
@@@ -65,7 -64,6 +65,7 @@@ static int shown_url = 0
  static struct refspec refmap = REFSPEC_INIT_FETCH;
  static struct list_objects_filter_options filter_options;
  static struct string_list server_options = STRING_LIST_INIT_DUP;
 +static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
  
  static int git_fetch_config(const char *k, const char *v, void *cb)
  {
@@@ -115,7 -113,7 +115,7 @@@ static struct option builtin_fetch_opti
                 N_("append to .git/FETCH_HEAD instead of overwriting")),
        OPT_STRING(0, "upload-pack", &upload_pack, N_("path"),
                   N_("path to upload pack on remote end")),
-       OPT__FORCE(&force, N_("force overwrite of local branch"), 0),
+       OPT__FORCE(&force, N_("force overwrite of local reference"), 0),
        OPT_BOOL('m', "multiple", &multiple,
                 N_("fetch from multiple remotes")),
        OPT_SET_INT('t', "tags", &tags,
                        TRANSPORT_FAMILY_IPV4),
        OPT_SET_INT('6', "ipv6", &family, N_("use IPv6 addresses only"),
                        TRANSPORT_FAMILY_IPV6),
 +      OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
 +                      N_("report that we have only objects reachable from this object")),
        OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
        OPT_END()
  };
@@@ -239,7 -235,7 +239,7 @@@ static int will_fetch(struct ref **head
  {
        struct ref *rm = *head;
        while (rm) {
 -              if (!hashcmp(rm->old_oid.hash, sha1))
 +              if (hasheq(rm->old_oid.hash, sha1))
                        return 1;
                rm = rm->next;
        }
@@@ -508,7 -504,7 +508,7 @@@ static void adjust_refcol_width(const s
        int max, rlen, llen, len;
  
        /* uptodate lines are only shown on high verbosity level */
 -      if (!verbosity && !oidcmp(&ref->peer_ref->old_oid, &ref->old_oid))
 +      if (!verbosity && oideq(&ref->peer_ref->old_oid, &ref->old_oid))
                return;
  
        max    = term_columns();
@@@ -645,7 -641,7 +645,7 @@@ static int update_local_ref(struct ref 
        if (type < 0)
                die(_("object %s not found"), oid_to_hex(&ref->new_oid));
  
 -      if (!oidcmp(&ref->old_oid, &ref->new_oid)) {
 +      if (oideq(&ref->old_oid, &ref->new_oid)) {
                if (verbosity > 0)
                        format_display(display, '=', _("[up to date]"), NULL,
                                       remote, pretty_ref, summary_width);
  
        if (!is_null_oid(&ref->old_oid) &&
            starts_with(ref->name, "refs/tags/")) {
-               int r;
-               r = s_update_ref("updating tag", ref, 0);
-               format_display(display, r ? '!' : 't', _("[tag update]"),
-                              r ? _("unable to update local ref") : NULL,
-                              remote, pretty_ref, summary_width);
-               return r;
+               if (force || ref->force) {
+                       int r;
+                       r = s_update_ref("updating tag", ref, 0);
+                       format_display(display, r ? '!' : 't', _("[tag update]"),
+                                      r ? _("unable to update local ref") : NULL,
+                                      remote, pretty_ref, summary_width);
+                       return r;
+               } else {
+                       format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
+                                      remote, pretty_ref, summary_width);
+                       return 1;
+               }
        }
  
 -      current = lookup_commit_reference_gently(&ref->old_oid, 1);
 -      updated = lookup_commit_reference_gently(&ref->new_oid, 1);
 +      current = lookup_commit_reference_gently(the_repository,
 +                                               &ref->old_oid, 1);
 +      updated = lookup_commit_reference_gently(the_repository,
 +                                               &ref->new_oid, 1);
        if (!current || !updated) {
                const char *msg;
                const char *what;
@@@ -814,8 -814,7 +820,8 @@@ static int store_updated_refs(const cha
                                continue;
                        }
  
 -                      commit = lookup_commit_reference_gently(&rm->old_oid,
 +                      commit = lookup_commit_reference_gently(the_repository,
 +                                                              &rm->old_oid,
                                                                1);
                        if (!commit)
                                rm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;
@@@ -943,11 -942,13 +949,11 @@@ static int quickfetch(struct ref *ref_m
        return check_connected(iterate_ref_map, &rm, &opt);
  }
  
 -static int fetch_refs(struct transport *transport, struct ref *ref_map,
 -                    struct ref **updated_remote_refs)
 +static int fetch_refs(struct transport *transport, struct ref *ref_map)
  {
        int ret = quickfetch(ref_map);
        if (ret)
 -              ret = transport_fetch_refs(transport, ref_map,
 -                                         updated_remote_refs);
 +              ret = transport_fetch_refs(transport, ref_map);
        if (!ret)
                /*
                 * Keep the new pack's ".keep" file around to allow the caller
@@@ -1062,40 -1063,6 +1068,40 @@@ static void set_option(struct transpor
                        name, transport->url);
  }
  
 +
 +static int add_oid(const char *refname, const struct object_id *oid, int flags,
 +                 void *cb_data)
 +{
 +      struct oid_array *oids = cb_data;
 +
 +      oid_array_append(oids, oid);
 +      return 0;
 +}
 +
 +static void add_negotiation_tips(struct git_transport_options *smart_options)
 +{
 +      struct oid_array *oids = xcalloc(1, sizeof(*oids));
 +      int i;
 +
 +      for (i = 0; i < negotiation_tip.nr; i++) {
 +              const char *s = negotiation_tip.items[i].string;
 +              int old_nr;
 +              if (!has_glob_specials(s)) {
 +                      struct object_id oid;
 +                      if (get_oid(s, &oid))
 +                              die("%s is not a valid object", s);
 +                      oid_array_append(oids, &oid);
 +                      continue;
 +              }
 +              old_nr = oids->nr;
 +              for_each_glob_ref(add_oid, s, oids);
 +              if (old_nr == oids->nr)
 +                      warning("Ignoring --negotiation-tip=%s because it does not match any refs",
 +                              s);
 +      }
 +      smart_options->negotiation_tips = oids;
 +}
 +
  static struct transport *prepare_transport(struct remote *remote, int deepen)
  {
        struct transport *transport;
                           filter_options.filter_spec);
                set_option(transport, TRANS_OPT_FROM_PROMISOR, "1");
        }
 +      if (negotiation_tip.nr) {
 +              if (transport->smart_options)
 +                      add_negotiation_tips(transport->smart_options);
 +              else
 +                      warning("Ignoring --negotiation-tip because the protocol does not support it.");
 +      }
        return transport;
  }
  
@@@ -1152,7 -1113,7 +1158,7 @@@ static void backfill_tags(struct transp
        transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
        transport_set_option(transport, TRANS_OPT_DEPTH, "0");
        transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
 -      if (!fetch_refs(transport, ref_map, NULL))
 +      if (!fetch_refs(transport, ref_map))
                consume_refs(transport, ref_map);
  
        if (gsecondary) {
@@@ -1168,6 -1129,7 +1174,6 @@@ static int do_fetch(struct transport *t
        int autotags = (transport->remote->fetch_tags == 1);
        int retcode = 0;
        const struct ref *remote_refs;
 -      struct ref *updated_remote_refs = NULL;
        struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
  
        if (tags == TAGS_DEFAULT) {
                refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes);
  
        if (ref_prefixes.argc &&
 -          (tags == TAGS_SET || (tags == TAGS_DEFAULT && !rs->nr))) {
 +          (tags == TAGS_SET || (tags == TAGS_DEFAULT))) {
                argv_array_push(&ref_prefixes, "refs/tags/");
        }
  
                                   transport->url);
                }
        }
 -
 -      if (fetch_refs(transport, ref_map, &updated_remote_refs)) {
 -              free_refs(ref_map);
 -              retcode = 1;
 -              goto cleanup;
 -      }
 -      if (updated_remote_refs) {
 -              /*
 -               * Regenerate ref_map using the updated remote refs.  This is
 -               * to account for additional information which may be provided
 -               * by the transport (e.g. shallow info).
 -               */
 -              free_refs(ref_map);
 -              ref_map = get_ref_map(transport->remote, updated_remote_refs, rs,
 -                                    tags, &autotags);
 -              free_refs(updated_remote_refs);
 -      }
 -      if (consume_refs(transport, ref_map)) {
 +      if (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) {
                free_refs(ref_map);
                retcode = 1;
                goto cleanup;
diff --combined t/t5516-fetch-push.sh
index 539c25aadafdcf6aa9fbcce0988631702d3fb02d,c545b16d13229728bcc216fe91e8507b7cabfcd0..7a8f56db53eb6c3b869d525501c186bd2df1ba0a
@@@ -923,7 -923,7 +923,7 @@@ test_expect_success 'push into aliased 
        (
                cd child1 &&
                git branch foo &&
 -              git symbolic-ref refs/heads/bar refs/heads/foo
 +              git symbolic-ref refs/heads/bar refs/heads/foo &&
                git config receive.denyCurrentBranch false
        ) &&
        (
@@@ -945,7 -945,7 +945,7 @@@ test_expect_success 'push into aliased 
        (
                cd child1 &&
                git branch foo &&
 -              git symbolic-ref refs/heads/bar refs/heads/foo
 +              git symbolic-ref refs/heads/bar refs/heads/foo &&
                git config receive.denyCurrentBranch false
        ) &&
        (
@@@ -969,7 -969,7 +969,7 @@@ test_force_push_tag () 
        tag_type_description=$1
        tag_args=$2
  
-       test_expect_success 'force pushing required to update lightweight tag' "
+       test_expect_success "force pushing required to update $tag_type_description" "
                mk_test testrepo heads/master &&
                mk_child testrepo child1 &&
                mk_child testrepo child2 &&
  }
  
  test_force_push_tag "lightweight tag" "-f"
- test_force_push_tag "annotated tag" "-f -a -m'msg'"
+ test_force_push_tag "annotated tag" "-f -a -m'tag message'"
+ test_force_fetch_tag () {
+       tag_type_description=$1
+       tag_args=$2
+       test_expect_success "fetch will not clobber an existing $tag_type_description without --force" "
+               mk_test testrepo heads/master &&
+               mk_child testrepo child1 &&
+               mk_child testrepo child2 &&
+               (
+                       cd testrepo &&
+                       git tag testTag &&
+                       git -C ../child1 fetch origin tag testTag &&
+                       >file1 &&
+                       git add file1 &&
+                       git commit -m 'file1' &&
+                       git tag $tag_args testTag &&
+                       test_must_fail git -C ../child1 fetch origin tag testTag &&
+                       git -C ../child1 fetch origin '+refs/tags/*:refs/tags/*'
+               )
+       "
+ }
+ test_force_fetch_tag "lightweight tag" "-f"
+ test_force_fetch_tag "annotated tag" "-f -a -m'tag message'"
  
  test_expect_success 'push --porcelain' '
        mk_empty testrepo &&
@@@ -1036,7 -1061,7 +1061,7 @@@ test_expect_success 'push --porcelain r
        mk_empty testrepo &&
        git push testrepo refs/heads/master:refs/remotes/origin/master &&
        (cd testrepo &&
 -              git reset --hard origin/master^
 +              git reset --hard origin/master^ &&
                git config receive.denyCurrentBranch true) &&
  
        echo >.git/foo  "To testrepo"  &&
@@@ -1050,7 -1075,7 +1075,7 @@@ test_expect_success 'push --porcelain -
        mk_empty testrepo &&
        git push testrepo refs/heads/master:refs/remotes/origin/master &&
        (cd testrepo &&
 -              git reset --hard origin/master
 +              git reset --hard origin/master &&
                git config receive.denyCurrentBranch true) &&
  
        echo >.git/foo  "To testrepo"  &&
@@@ -1358,7 -1383,7 +1383,7 @@@ test_expect_success 'push --follow-tag 
                git commit --allow-empty -m "future commit" &&
                git tag -m "future" future &&
                git checkout master &&
 -              git for-each-ref refs/heads/master refs/tags/tag >../expect
 +              git for-each-ref refs/heads/master refs/tags/tag >../expect &&
                git push --follow-tag ../dst master
        ) &&
        (
diff --combined t/t5612-clone-refspec.sh
index 5582b3d5fd7118398bad00e43864d9e73c055f01,6ea8f50dae40ec0860b2f394c81ccb58122dee32..e36ac01661d1b5cd26b0ae4fe615b9ccf44e02c8
@@@ -97,13 -97,14 +97,13 @@@ test_expect_success 'clone with --no-ta
                git fetch &&
                git for-each-ref refs/tags >../actual
        ) &&
 -      >expect &&
 -      test_cmp expect actual
 +      test_must_be_empty actual
  '
  
  test_expect_success '--single-branch while HEAD pointing at master' '
        (
                cd dir_master &&
-               git fetch &&
+               git fetch --force &&
                git for-each-ref refs/remotes/origin |
                sed -e "/HEAD$/d" \
                    -e "s|/remotes/origin/|/heads/|" >../actual
        test_cmp expect actual &&
        (
                cd dir_master &&
-               git fetch --tags &&
+               git fetch --tags --force &&
                git for-each-ref refs/tags >../actual
        ) &&
        git for-each-ref refs/tags >expect &&
@@@ -139,7 -140,8 +139,7 @@@ test_expect_success '--single-branch wh
                git fetch &&
                git for-each-ref refs/tags >../actual
        ) &&
 -      >expect &&
 -      test_cmp expect actual &&
 +      test_must_be_empty actual &&
        test_line_count = 0 actual &&
        # get tags with --tags overrides tagOpt
        (
@@@ -228,7 -230,8 +228,7 @@@ test_expect_success '--single-branch wi
                    -e "s|/remotes/origin/|/heads/|" >../actual
        ) &&
        # nothing
 -      >expect &&
 -      test_cmp expect actual
 +      test_must_be_empty actual
  '
  
  test_done