push: fix "refs/tags/ hierarchy cannot be updated without --force"

When pushing to update a branch with a commit that is not a
descendant of the commit at the tip, a wrong message "already
exists" was given, instead of the correct "non-fast-forward", if we
do not have the object sitting in the destination repository at the
tip of the ref we are updating.

The primary cause of the bug is that the check in a new helper
function is_forwardable() assumed both old and new objects are
available and can be checked, which is not always the case.

The way the caller uses the result of this function is also wrong.
If the helper says "we do not want to let this push go through", the
caller unconditionally translates it into "we blocked it because the
destination already exists", which is not true at all in this case.

Fix this by doing these three things:

 * Remove unnecessary not_forwardable from "struct ref"; it is only
   used inside set_ref_status_for_push();

 * Make "refs/tags/" the only hierarchy that cannot be replaced
   without --force;

 * Remove the misguided attempt to force that everything that
   updates an existing ref has to be a commit outside "refs/tags/"
   hierarchy.

The policy last one tried to implement may later be resurrected and
extended to ensure fast-forwardness (defined as "not losing
objects", extending from the traditional "not losing commits from
the resulting history") when objects that are not commit are
involved (e.g. an annotated tag in hierarchies outside refs/tags),
but such a logic belongs to "is this a fast-forward?" check that is
done by ref_newer(); is_forwardable(), which is now removed, was not
the right place to do so.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/cache.h b/cache.h
index a32a0ea91c11f747a4612cc5c0b773635ec426c9..a942bbd59f87600b597c452f92e9faaf1a7737bf 100644 (file)
--- a/cache.h
+++ b/cache.h
@@ -1004,7 +1004,6 @@ struct ref {
                requires_force:1,
                merge:1,
                nonfastforward:1,
-               not_forwardable:1,
                update:1,
                deletion:1;
        enum {

diff --git a/remote.c b/remote.c
index aa6b7199b7c0a5abec5dfb697fef6f03f2b419b2..d3a1ca233becf76411a4653cee4416ce97ee1217 100644 (file)
--- a/remote.c
+++ b/remote.c
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
        return 0;
 }
 
-static inline int is_forwardable(struct ref* ref)
-{
-       struct object *o;
-
-       if (!prefixcmp(ref->name, "refs/tags/"))
-               return 0;
-
-       /* old object must be a commit */
-       o = parse_object(ref->old_sha1);
-       if (!o || o->type != OBJ_COMMIT)
-               return 0;
-
-       /* new object must be commit-ish */
-       o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
-       if (!o || o->type != OBJ_COMMIT)
-               return 0;
-
-       return 1;
-}
-
 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
        int force_update)
 {
@@ -1320,32 +1300,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
                }
 
                /*
-                * The below logic determines whether an individual
-                * refspec A:B can be pushed.  The push will succeed
-                * if any of the following are true:
+                * Decide whether an individual refspec A:B can be
+                * pushed.  The push will succeed if any of the
+                * following are true:
                 *
                 * (1) the remote reference B does not exist
                 *
                 * (2) the remote reference B is being removed (i.e.,
                 *     pushing :B where no source is specified)
                 *
-                * (3) the update meets all fast-forwarding criteria:
-                *
-                *     (a) the destination is not under refs/tags/
-                *     (b) the old is a commit
-                *     (c) the new is a descendant of the old
-                *
-                *     NOTE: We must actually have the old object in
-                *     order to overwrite it in the remote reference,
-                *     and the new object must be commit-ish.  These are
-                *     implied by (b) and (c) respectively.
+                * (3) the destination is not under refs/tags/, and
+                *     if the old and new value is a commit, the new
+                *     is a descendant of the old.
                 *
                 * (4) it is forced using the +A:B notation, or by
                 *     passing the --force argument
                 */
 
-               ref->not_forwardable = !is_forwardable(ref);
-
                ref->update =
                        !ref->deletion &&
                        !is_null_sha1(ref->old_sha1);
@@ -1355,7 +1326,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
                                !has_sha1_file(ref->old_sha1)
                                  || !ref_newer(ref->new_sha1, ref->old_sha1);
 
-                       if (ref->not_forwardable) {
+                       if (!prefixcmp(ref->name, "refs/tags/")) {
                                ref->requires_force = 1;
                                if (!force_ref_update) {
                                        ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;

diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 60093728fecd626ae5e005c4d41cee330ac1112c..8f024a08f0b175c12747c33ac0a07cacb73937d9 100755 (executable)
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -950,27 +950,6 @@ test_expect_success 'push requires --force to update lightweight tag' '
        )
 '
 
-test_expect_success 'push requires --force to update annotated tag' '
-       mk_test heads/master &&
-       mk_child child1 &&
-       mk_child child2 &&
-       (
-               cd child1 &&
-               git tag -a -m "message 1" Tag &&
-               git push ../child2 Tag:refs/tmp/Tag &&
-               git push ../child2 Tag:refs/tmp/Tag &&
-               >file1 &&
-               git add file1 &&
-               git commit -m "file1" &&
-               git tag -f -a -m "message 2" Tag &&
-               test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
-               git push --force ../child2 Tag:refs/tmp/Tag &&
-               git tag -f -a -m "message 3" Tag HEAD~ &&
-               test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
-               git push --force ../child2 Tag:refs/tmp/Tag
-       )
-'
-
 test_expect_success 'push --porcelain' '
        mk_empty &&
        echo >.git/foo  "To testrepo" &&