git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v6 0/8] push: update remote tags only with force

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 16, 2013, 21:02 UTC
Message-ID
<7va9s8x2n0.fsf@alter.siamese.dyndns.org>
In-Reply-To
<20130116174325.GA27525@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> I see what you are saying, but I think the ship has already sailed to
> some degree. We already implement the non-fast-forward check everywhere,
> and I cannot have a "refs/tested" hierarchy that pushes arbitrary
> commits without regard to their history. If I have such a hierarchy, I
> have to use "--force" (or more likely, mark the refspec with "+").

Yeah, actually in that example, I meant refs/tested/ would have pointers to bare tree objects. I often rebuild 'pu' and another private integration branch for testing, reordering the series that are still not in 'next' and also after rewriting log messages of some commits. It is not unusual to end up the updated 'pu' having the identical tree as 'pu' before update, and I want to skip testing the result (tree equality matters while commit equality does not in such a use case).

> In my mind, the object-type checking is just making that fast-forward
> check more thorough (i.e., extending it to non-commit objects).
Yes, I agree with that point of view.
Thanks.

Here is what I am planning to queue (the patch is the same, but the message is different on the third point). 

-- >8 --
Subject: [PATCH] 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>
---
 cache.h               |  1 -
 remote.c              | 43 +++++++------------------------------------
 t/t5516-fetch-push.sh | 21 ---------------------
 3 files changed, 7 insertions(+), 58 deletions(-)
diff --git a/cache.h b/cache.h
index a32a0ea..a942bbd 100644
--- 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 aa6b719..d3a1ca2 100644
--- 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 6009372..8f024a0 100755
--- 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" &&

-- 
1.8.1.1.426.g616047d
Previous: Jeff KingNext: Chris Rorvick
Message 21 of 61 in “push: update remote tags only with force”
  1. 0/8 push: update remote tags only with forceChris Rorvick, Nov 30, 2012
  2. 1/8 push: return reject reasons as a bitsetChris Rorvick, Nov 30, 2012
  3. 2/8 push: add advice for rejected tag referenceChris Rorvick, Nov 30, 2012
  4. Junio C HamanoDec 2, 2012
  5. 0/2 push: honor advice.* configurationChris Rorvick, Dec 3, 2012
  6. 1/2 push: rename config variable for more general useChris Rorvick, Dec 3, 2012
  7. 2/2 push: allow already-exists advice to be disabledChris Rorvick, Dec 3, 2012
  8. 3/8 push: flag updatesChris Rorvick, Nov 30, 2012
  9. 4/8 push: flag updates that require forceChris Rorvick, Nov 30, 2012
  10. 5/8 push: require force for refs under refs/tags/Chris Rorvick, Nov 30, 2012
  11. 6/8 push: require force for annotated tagsChris Rorvick, Nov 30, 2012
  12. 7/8 push: clarify rejection of update to non-commit-ishChris Rorvick, Nov 30, 2012
  13. 8/8 push: cleanup push rules commentChris Rorvick, Nov 30, 2012
  14. remote.c: fix grammatical error in commentChris Rorvick, Dec 2, 2012
  15. Junio C HamanoDec 3, 2012
  16. Max HornJan 16, 2013
  17. Junio C HamanoJan 16, 2013
  18. Jeff KingJan 16, 2013
  19. Junio C HamanoJan 16, 2013
  20. Jeff KingJan 16, 2013
  21. Junio C HamanoJan 16, 2013
  22. Chris RorvickJan 17, 2013
  23. Jeff KingJan 17, 2013
  24. Chris RorvickJan 17, 2013
  25. Junio C HamanoJan 16, 2013
  26. Junio C HamanoJan 16, 2013
  27. Chris RorvickJan 17, 2013
  28. Junio C HamanoJan 17, 2013
  29. Chris RorvickJan 17, 2013
  30. Jeff KingJan 18, 2013
  31. Chris RorvickJan 18, 2013
  32. Jeff KingJan 21, 2013
  33. Junio C HamanoJan 21, 2013
  34. Chris RorvickJan 22, 2013
  35. Junio C HamanoJan 22, 2013
  36. 0/3 Finishing touches to "push" advisesJunio C Hamano, Jan 22, 2013
  37. 1/3 push: further clean up fields of "struct ref"Junio C Hamano, Jan 22, 2013
  38. 2/3 push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCEJunio C Hamano, Jan 22, 2013
  39. Junio C HamanoJan 22, 2013
  40. 3/3 push: further reduce "struct ref" and simplify the logicJunio C Hamano, Jan 22, 2013
  41. Junio C HamanoJan 22, 2013
  42. 0/3 Finishing touches to "push" advisesJunio C Hamano, Jan 22, 2013
  43. 1/3 push: further clean up fields of "struct ref"Junio C Hamano, Jan 22, 2013
  44. Jeff KingJan 23, 2013
  45. 2/3 push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCEJunio C Hamano, Jan 22, 2013
  46. Jeff KingJan 23, 2013
  47. Junio C HamanoJan 23, 2013
  48. Jeff KingJan 24, 2013
  49. 3/3 push: further simplify the logic to assign rejection statusJunio C Hamano, Jan 22, 2013
  50. Junio C HamanoJan 22, 2013
  51. 0/3 Finishing touches to "push" advisesJunio C Hamano, Jan 23, 2013
  52. 1/3 push: further clean up fields of "struct ref"Junio C Hamano, Jan 23, 2013
  53. Eric SunshineJan 24, 2013
  54. 2/3 push: further simplify the logic to assign rejection reasonJunio C Hamano, Jan 23, 2013
  55. 3/3 push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCEJunio C Hamano, Jan 23, 2013
  56. Jeff KingJan 24, 2013
  57. Junio C HamanoJan 24, 2013
  58. Chris RorvickJan 25, 2013
  59. Junio C HamanoJan 25, 2013
  60. Chris RorvickJan 25, 2013
  61. Junio C HamanoJan 18, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.