]> granicus.if.org Git - git/commitdiff
fetch: stop clobbering existing tags without --force
authorÆvar Arnfjörð Bjarmason <avarab@gmail.com>
Fri, 31 Aug 2018 20:10:04 +0000 (20:10 +0000)
committerJunio C Hamano <gitster@pobox.com>
Fri, 31 Aug 2018 21:04:06 +0000 (14:04 -0700)
Change "fetch" to treat "+" in refspecs (aka --force) to mean we
should clobber a local tag of the same name.

This changes the long-standing behavior of "fetch" added in
853a3697dc ("[PATCH] Multi-head fetch.", 2005-08-20). Before this
change, all tag fetches effectively had --force enabled. See the
git-fetch-script code in fast_forward_local() with the comment:

    > Tags need not be pointing at commits so there is no way to
    > guarantee "fast-forward" anyway.

That commit and the rest of the history of "fetch" shows that the
"+" (--force) part of refpecs was only conceived for branch updates,
while tags have accepted any changes from upstream unconditionally and
clobbered the local tag object. Changing this behavior has been
discussed as early as 2011[1].

The current behavior doesn't make sense to me, it easily results in
local tags accidentally being clobbered. We could namespace our tags
per-remote and not locally populate refs/tags/*, but as with my
97716d217c ("fetch: add a --prune-tags option and fetch.pruneTags
config", 2018-02-09) it's easier to work around the current
implementation than to fix the root cause.

So this change implements suggestion #1 from Jeff's 2011 E-Mail[1],
"fetch" now only clobbers the tag if either "+" is provided as part of
the refspec, or if "--force" is provided on the command-line.

This also makes it nicely symmetrical with how "tag" itself works when
creating tags. I.e. we refuse to clobber any existing tags unless
"--force" is supplied. Now we can refuse all such clobbering, whether
it would happen by clobbering a local tag with "tag", or by fetching
it from the remote with "fetch".

Ref updates outside refs/{tags,heads/* are still still not symmetrical
with how "git push" works, as discussed in the recently changed
pull-fetch-param.txt documentation. This change brings the two
divergent behaviors more into line with one another. I don't think
there's any reason "fetch" couldn't fully converge with the behavior
used by "push", but that's a topic for another change.

One of the tests added in 31b808a032 ("clone --single: limit the fetch
refspec to fetched branch", 2012-09-20) is being changed to use
--force where a clone would clobber a tag. This changes nothing about
the existing behavior of the test.

1. https://public-inbox.org/git/20111123221658.GA22313@sigill.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Documentation/pull-fetch-param.txt
builtin/fetch.c
t/t5516-fetch-push.sh
t/t5612-clone-refspec.sh

index ab9617ad010beba136b07fd2aaaa7c82410943c7..293c6b967decca67999ebd5f2cf5fea410efff15 100644 (file)
@@ -43,10 +43,13 @@ same rules apply for fetching as when pushing, see the `<refspec>...`
 section of linkgit:git-push[1] for what those are. Exceptions to those
 rules particular to 'git fetch' are noted below.
 +
-Unlike when pushing with linkgit:git-push[1], any updates to
-`refs/tags/*` will be accepted without `+` in the refspec (or
-`--force`). The receiving promiscuously considers all tag updates from
-a remote to be forced fetches.
+Until Git version 2.20, and unlike when pushing with
+linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
+without `+` in the refspec (or `--force`). The receiving promiscuously
+considered all tag updates from a remote to be forced fetches.  Since
+Git version 2.20, fetching to update `refs/tags/*` work the same way
+as when pushing. I.e. any updates will be rejected without `+` in the
+refspec (or `--force`).
 +
 Unlike when pushing with linkgit:git-push[1], any updates outside of
 `refs/{tags,heads}/*` will be accepted without `+` in the refspec (or
@@ -54,6 +57,10 @@ Unlike when pushing with linkgit:git-push[1], any updates outside of
 a commit for another commit that's doesn't have the previous commit as
 an ancestor etc.
 +
+Unlike when pushing with linkgit:git-push[1], there is no
+configuration which'll amend these rules, and nothing like a
+`pre-fetch` hook analogous to the `pre-receive` hook.
++
 As with pushing with linkgit:git-push[1], all of the rules described
 above about what's not allowed as an update can be overridden by
 adding an the optional leading `+` to a refspec (or using `--force`
index 767406ee05b8bf575ba4d819fe443eea5de7b92e..683f70d71eda87196c4a4dd86cdce87bcc347e99 100644 (file)
@@ -664,12 +664,18 @@ static int update_local_ref(struct ref *ref,
 
        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);
index a2d4f375fc38f0cae255af4c06d17e51221914fa..c545b16d13229728bcc216fe91e8507b7cabfcd0 100755 (executable)
@@ -1015,7 +1015,7 @@ test_force_fetch_tag () {
        tag_type_description=$1
        tag_args=$2
 
-       test_expect_success "fetch will clobber an existing $tag_type_description" "
+       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 &&
@@ -1027,7 +1027,8 @@ test_force_fetch_tag () {
                        git add file1 &&
                        git commit -m 'file1' &&
                        git tag $tag_args testTag &&
-                       git -C ../child1 fetch origin tag testTag
+                       test_must_fail git -C ../child1 fetch origin tag testTag &&
+                       git -C ../child1 fetch origin '+refs/tags/*:refs/tags/*'
                )
        "
 }
index fac5a738519fe4aec0c3b2328c410db693c11f63..6ea8f50dae40ec0860b2f394c81ccb58122dee32 100755 (executable)
@@ -104,7 +104,7 @@ test_expect_success 'clone with --no-tags' '
 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
@@ -115,7 +115,7 @@ test_expect_success '--single-branch while HEAD pointing at master' '
        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 &&