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>

Junio C Hamano committed Jan 16, 2013 at 13:02 UTC 256b9d70a497534338f0c22101cb6566ab3f1665
3 files changed +7 -58
cache.h
-1
@@ -1004,7 +1004,6 @@ struct ref {
1004 requires_force:1,
1005 merge:1,
1006 nonfastforward:1,
1007 - not_forwardable:1,
1007 update:1,
1008 deletion:1;
1009 enum {
remote.c
+7 -36
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
1279 return 0;
1280 }
1281
1282 -static inline int is_forwardable(struct ref* ref)
1283 -{
1284 - struct object *o;
1285 -
1286 - if (!prefixcmp(ref->name, "refs/tags/"))
1287 - return 0;
1288 -
1289 - /* old object must be a commit */
1290 - o = parse_object(ref->old_sha1);
1291 - if (!o || o->type != OBJ_COMMIT)
1292 - return 0;
1293 -
1294 - /* new object must be commit-ish */
1295 - o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
1296 - if (!o || o->type != OBJ_COMMIT)
1297 - return 0;
1298 -
1299 - return 1;
1300 -}
1301 -
1282 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
1283 int force_update)
1284 {
@@ -1320,32 +1300,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
1300 }
1301
1302 /*
1323 - * The below logic determines whether an individual
1324 - * refspec A:B can be pushed. The push will succeed
1325 - * if any of the following are true:
1303 + * Decide whether an individual refspec A:B can be
1304 + * pushed. The push will succeed if any of the
1305 + * following are true:
1306 *
1307 * (1) the remote reference B does not exist
1308 *
1309 * (2) the remote reference B is being removed (i.e.,
1310 * pushing :B where no source is specified)
1311 *
1332 - * (3) the update meets all fast-forwarding criteria:
1333 - *
1334 - * (a) the destination is not under refs/tags/
1335 - * (b) the old is a commit
1336 - * (c) the new is a descendant of the old
1337 - *
1338 - * NOTE: We must actually have the old object in
1339 - * order to overwrite it in the remote reference,
1340 - * and the new object must be commit-ish. These are
1341 - * implied by (b) and (c) respectively.
1312 + * (3) the destination is not under refs/tags/, and
1313 + * if the old and new value is a commit, the new
1314 + * is a descendant of the old.
1315 *
1316 * (4) it is forced using the +A:B notation, or by
1317 * passing the --force argument
1318 */
1319
1347 - ref->not_forwardable = !is_forwardable(ref);
1348 -
1320 ref->update =
1321 !ref->deletion &&
1322 !is_null_sha1(ref->old_sha1);
@@ -1355,7 +1326,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
1326 !has_sha1_file(ref->old_sha1)
1327 || !ref_newer(ref->new_sha1, ref->old_sha1);
1328
1358 - if (ref->not_forwardable) {
1329 + if (!prefixcmp(ref->name, "refs/tags/")) {
1330 ref->requires_force = 1;
1331 if (!force_ref_update) {
1332 ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
t/t5516-fetch-push.sh
-21
@@ -950,27 +950,6 @@ test_expect_success 'push requires --force to update lightweight tag' '
950 )
951 '
952
953 -test_expect_success 'push requires --force to update annotated tag' '
954 - mk_test heads/master &&
955 - mk_child child1 &&
956 - mk_child child2 &&
957 - (
958 - cd child1 &&
959 - git tag -a -m "message 1" Tag &&
960 - git push ../child2 Tag:refs/tmp/Tag &&
961 - git push ../child2 Tag:refs/tmp/Tag &&
962 - >file1 &&
963 - git add file1 &&
964 - git commit -m "file1" &&
965 - git tag -f -a -m "message 2" Tag &&
966 - test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
967 - git push --force ../child2 Tag:refs/tmp/Tag &&
968 - git tag -f -a -m "message 3" Tag HEAD~ &&
969 - test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
970 - git push --force ../child2 Tag:refs/tmp/Tag
971 - )
972 -'
973 -
953 test_expect_success 'push --porcelain' '
954 mk_empty &&
955 echo >.git/foo "To testrepo" &&