{"thread":{"id":"32137","subject":"[PATCH v4 0/5] push: update remote tags only with force","startedAt":"2012-11-17T20:16:32Z","lastAt":"2012-11-20T05:44:51Z","messageCount":10,"participants":["Chris Rorvick","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":5},"messages":[{"id":"203435","messageId":"1353183397-17719-1-git-send-email-chris@rorvick.com","threadId":"32137","inReplyTo":null,"subject":"[PATCH v4 0/5] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-17T20:16:32Z","receivedAt":"2012-11-17T20:16:32Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"This patch set can be divided into two sets:\n\n  1. Provide useful advice for rejected tag references.\n\n     push: return reject reasons via a mask\n     push: add advice for rejected tag reference\n\n     Recommending a merge to resolve a rejected tag update seems\n     nonsensical since the tag does not come along for the ride.  These\n     patches change the advice for rejected tags to suggest using\n     \"push -f\".\n\n  2. Require force when updating tag references, even on a fast-forward.\n\n     push: flag updates\n     push: flag updates that require force\n     push: update remote tags only with force\n\n     This is in response to the following thread:\n\n       http://thread.gmane.org/gmane.comp.version-control.git/208354\n\n     This solution prevents fast-forwards if the reference is of the\n     refs/tags/* hierarchy or if the old object is not a commit.\n\nThese patches contain the following updates since the v3 set:\n\n  * builtin/push.c: Remove \"push --force\" suggestion from advice.\n  * remote.c: Only require old object to be a commit to be forwardable.\n      I added the check for object types based comments from Peff in\n      original thread, and I think this implementation is actually what\n      he intended.  If the new object is a tag, the operation is not\n      destructive so there is no reason to block it (at least within\n      the scope of these changes) as was done in the previous iteration.\n  * t/t5516-fetch-push.sh: Create separate tests for the lightweight and\n      annotated cases.  Do the annotated tests outside of refs/tags/\n      so that it actually tests different functionality.\n\nChris Rorvick (5):\n  push: return reject reasons via a mask\n  push: add advice for rejected tag reference\n  push: flag updates\n  push: flag updates that require force\n  push: update remote tags only with force\n\n Documentation/git-push.txt | 10 +++++-----\n builtin/push.c             | 24 +++++++++++++++---------\n builtin/send-pack.c        |  9 +++++++--\n cache.h                    |  7 ++++++-\n remote.c                   | 46 ++++++++++++++++++++++++++++++++++++----------\n send-pack.c                |  1 +\n t/t5516-fetch-push.sh      | 30 +++++++++++++++++++++++++++++-\n transport-helper.c         |  6 ++++++\n transport.c                | 25 +++++++++++++++----------\n transport.h                | 10 ++++++----\n 10 files changed, 126 insertions(+), 42 deletions(-)\n\n-- \n1.8.0.155.g3a063ad.dirty\n"},{"id":"203436","messageId":"1353183397-17719-2-git-send-email-chris@rorvick.com","threadId":"32137","inReplyTo":"1353183397-17719-1-git-send-email-chris@rorvick.com","subject":"[PATCH v4 1/5] push: return reject reasons via a mask","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-17T20:16:33Z","receivedAt":"2012-11-17T20:16:33Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Pass all rejection reasons back from transport_push().  The logic is\nsimpler and more flexible with regard to providing useful feedback.\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n builtin/push.c      | 13 ++++---------\n builtin/send-pack.c |  4 ++--\n transport.c         | 17 ++++++++---------\n transport.h         |  9 +++++----\n 4 files changed, 19 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex db9ba30..eaeaf7e 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -244,7 +244,7 @@ static void advise_checkout_pull_push(void)\n static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n-\tint nonfastforward;\n+\tunsigned int reject_mask;\n \n \ttransport_set_verbosity(transport, verbosity, progress);\n \n@@ -257,7 +257,7 @@ static int push_with_options(struct transport *transport, int flags)\n \tif (verbosity > 0)\n \t\tfprintf(stderr, _(\"Pushing to %s\\n\"), transport->url);\n \terr = transport_push(transport, refspec_nr, refspec, flags,\n-\t\t\t     &nonfastforward);\n+\t\t\t     &reject_mask);\n \tif (err != 0)\n \t\terror(_(\"failed to push some refs to '%s'\"), transport->url);\n \n@@ -265,18 +265,13 @@ static int push_with_options(struct transport *transport, int flags)\n \tif (!err)\n \t\treturn 0;\n \n-\tswitch (nonfastforward) {\n-\tdefault:\n-\t\tbreak;\n-\tcase NON_FF_HEAD:\n+\tif (reject_mask & NON_FF_HEAD) {\n \t\tadvise_pull_before_push();\n-\t\tbreak;\n-\tcase NON_FF_OTHER:\n+\t} else if (reject_mask & NON_FF_OTHER) {\n \t\tif (default_matching_used)\n \t\t\tadvise_use_upstream();\n \t\telse\n \t\t\tadvise_checkout_pull_push();\n-\t\tbreak;\n \t}\n \n \treturn 1;\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex d342013..fda28bc 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -85,7 +85,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tint send_all = 0;\n \tconst char *receivepack = \"git-receive-pack\";\n \tint flags;\n-\tint nonfastforward = 0;\n+\tunsigned int reject_mask;\n \tint progress = -1;\n \n \targv++;\n@@ -223,7 +223,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tret |= finish_connect(conn);\n \n \tif (!helper_status)\n-\t\ttransport_print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward);\n+\t\ttransport_print_push_status(dest, remote_refs, args.verbose, 0, &reject_mask);\n \n \tif (!args.dry_run && remote) {\n \t\tstruct ref *ref;\ndiff --git a/transport.c b/transport.c\nindex 9932f40..ae9fda8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -714,7 +714,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n }\n \n void transport_print_push_status(const char *dest, struct ref *refs,\n-\t\t\t\t  int verbose, int porcelain, int *nonfastforward)\n+\t\t\t\t  int verbose, int porcelain, unsigned int *reject_mask)\n {\n \tstruct ref *ref;\n \tint n = 0;\n@@ -733,18 +733,17 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\tif (ref->status == REF_STATUS_OK)\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n \n-\t*nonfastforward = 0;\n+\t*reject_mask = 0;\n \tfor (ref = refs; ref; ref = ref->next) {\n \t\tif (ref->status != REF_STATUS_NONE &&\n \t\t    ref->status != REF_STATUS_UPTODATE &&\n \t\t    ref->status != REF_STATUS_OK)\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n-\t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&\n-\t\t    *nonfastforward != NON_FF_HEAD) {\n+\t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {\n \t\t\tif (!strcmp(head, ref->name))\n-\t\t\t\t*nonfastforward = NON_FF_HEAD;\n+\t\t\t\t*reject_mask |= NON_FF_HEAD;\n \t\t\telse\n-\t\t\t\t*nonfastforward = NON_FF_OTHER;\n+\t\t\t\t*reject_mask |= NON_FF_OTHER;\n \t\t}\n \t}\n }\n@@ -1031,9 +1030,9 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)\n \n int transport_push(struct transport *transport,\n \t\t   int refspec_nr, const char **refspec, int flags,\n-\t\t   int *nonfastforward)\n+\t\t   unsigned int *reject_mask)\n {\n-\t*nonfastforward = 0;\n+\t*reject_mask = 0;\n \ttransport_verify_remote_names(refspec_nr, refspec);\n \n \tif (transport->push) {\n@@ -1099,7 +1098,7 @@ int transport_push(struct transport *transport,\n \t\tif (!quiet || err)\n \t\t\ttransport_print_push_status(transport->url, remote_refs,\n \t\t\t\t\tverbose | porcelain, porcelain,\n-\t\t\t\t\tnonfastforward);\n+\t\t\t\t\treject_mask);\n \n \t\tif (flags & TRANSPORT_PUSH_SET_UPSTREAM)\n \t\t\tset_upstreams(transport, remote_refs, pretend);\ndiff --git a/transport.h b/transport.h\nindex 4a61c0c..1f9699c 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -140,11 +140,12 @@ int transport_set_option(struct transport *transport, const char *name,\n void transport_set_verbosity(struct transport *transport, int verbosity,\n \tint force_progress);\n \n-#define NON_FF_HEAD 1\n-#define NON_FF_OTHER 2\n+#define NON_FF_HEAD     0x01\n+#define NON_FF_OTHER    0x02\n+\n int transport_push(struct transport *connection,\n \t\t   int refspec_nr, const char **refspec, int flags,\n-\t\t   int * nonfastforward);\n+\t\t   unsigned int * reject_mask);\n \n const struct ref *transport_get_remote_refs(struct transport *transport);\n \n@@ -170,7 +171,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v\n int transport_refs_pushed(struct ref *ref);\n \n void transport_print_push_status(const char *dest, struct ref *refs,\n-\t\t  int verbose, int porcelain, int *nonfastforward);\n+\t\t  int verbose, int porcelain, unsigned int *reject_mask);\n \n typedef void alternate_ref_fn(const struct ref *, void *);\n extern void for_each_alternate_ref(alternate_ref_fn, void *);\n-- \n1.8.0.155.g3a063ad.dirty\n"},{"id":"203437","messageId":"1353183397-17719-3-git-send-email-chris@rorvick.com","threadId":"32137","inReplyTo":"1353183397-17719-1-git-send-email-chris@rorvick.com","subject":"[PATCH v4 2/5] push: add advice for rejected tag reference","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-17T20:16:34Z","receivedAt":"2012-11-17T20:16:34Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Advising the user to fetch and merge only makes sense if the rejected\nreference is a branch.  If none of the rejections were for branches,\ntell the user they need to force the update(s).\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n builtin/push.c | 15 +++++++++++++--\n cache.h        |  1 +\n remote.c       |  2 ++\n transport.c    |  6 ++++--\n transport.h    |  5 +++--\n 5 files changed, 23 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex eaeaf7e..0e13e11 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -220,6 +220,10 @@ static const char message_advice_checkout_pull_push[] =\n \t   \"(e.g. 'git pull') before pushing again.\\n\"\n \t   \"See the 'Note about fast-forwards' in 'git push --help' for details.\");\n \n+static const char message_advice_ref_already_exists[] =\n+\tN_(\"Updates were rejected because a matching reference already exists in\\n\"\n+\t   \"the remote and the update is not a fast-forward.\");\n+\n static void advise_pull_before_push(void)\n {\n \tif (!advice_push_non_ff_current || !advice_push_nonfastforward)\n@@ -241,6 +245,11 @@ static void advise_checkout_pull_push(void)\n \tadvise(_(message_advice_checkout_pull_push));\n }\n \n+static void advise_ref_already_exists(void)\n+{\n+\tadvise(_(message_advice_ref_already_exists));\n+}\n+\n static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n@@ -265,13 +274,15 @@ static int push_with_options(struct transport *transport, int flags)\n \tif (!err)\n \t\treturn 0;\n \n-\tif (reject_mask & NON_FF_HEAD) {\n+\tif (reject_mask & REJECT_NON_FF_HEAD) {\n \t\tadvise_pull_before_push();\n-\t} else if (reject_mask & NON_FF_OTHER) {\n+\t} else if (reject_mask & REJECT_NON_FF_OTHER) {\n \t\tif (default_matching_used)\n \t\t\tadvise_use_upstream();\n \t\telse\n \t\t\tadvise_checkout_pull_push();\n+\t} else if (reject_mask & REJECT_ALREADY_EXISTS) {\n+\t\tadvise_ref_already_exists();\n \t}\n \n \treturn 1;\ndiff --git a/cache.h b/cache.h\nindex dbd8018..82dead1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1002,6 +1002,7 @@ struct ref {\n \tunsigned int force:1,\n \t\tmerge:1,\n \t\tnonfastforward:1,\n+\t\tis_a_tag:1,\n \t\tdeletion:1;\n \tenum {\n \t\tREF_STATUS_NONE = 0,\ndiff --git a/remote.c b/remote.c\nindex 04fd9ea..186814d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1316,6 +1316,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t *     always allowed.\n \t\t */\n \n+\t\tref->is_a_tag = !prefixcmp(ref->name, \"refs/tags/\");\n+\n \t\tref->nonfastforward =\n \t\t\t!ref->deletion &&\n \t\t\t!is_null_sha1(ref->old_sha1) &&\ndiff --git a/transport.c b/transport.c\nindex ae9fda8..5fcaea8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -740,10 +740,12 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t    ref->status != REF_STATUS_OK)\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n \t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {\n+\t\t\tif (!ref->is_a_tag)\n+\t\t\t\t*reject_mask |= REJECT_ALREADY_EXISTS;\n \t\t\tif (!strcmp(head, ref->name))\n-\t\t\t\t*reject_mask |= NON_FF_HEAD;\n+\t\t\t\t*reject_mask |= REJECT_NON_FF_HEAD;\n \t\t\telse\n-\t\t\t\t*reject_mask |= NON_FF_OTHER;\n+\t\t\t\t*reject_mask |= REJECT_NON_FF_OTHER;\n \t\t}\n \t}\n }\ndiff --git a/transport.h b/transport.h\nindex 1f9699c..7e86352 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -140,8 +140,9 @@ int transport_set_option(struct transport *transport, const char *name,\n void transport_set_verbosity(struct transport *transport, int verbosity,\n \tint force_progress);\n \n-#define NON_FF_HEAD     0x01\n-#define NON_FF_OTHER    0x02\n+#define REJECT_NON_FF_HEAD     0x01\n+#define REJECT_NON_FF_OTHER    0x02\n+#define REJECT_ALREADY_EXISTS  0x04\n \n int transport_push(struct transport *connection,\n \t\t   int refspec_nr, const char **refspec, int flags,\n-- \n1.8.0.155.g3a063ad.dirty\n"},{"id":"203439","messageId":"1353183397-17719-4-git-send-email-chris@rorvick.com","threadId":"32137","inReplyTo":"1353183397-17719-1-git-send-email-chris@rorvick.com","subject":"[PATCH v4 3/5] push: flag updates","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-17T20:16:35Z","receivedAt":"2012-11-17T20:16:35Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"If the reference exists on the remote and the the update is not a\ndelete, then mark as an update.  This is in preparation for handling\ntags and branches differently when pushing.\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n cache.h  |  1 +\n remote.c | 18 +++++++++++-------\n 2 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 82dead1..0f53d11 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1003,6 +1003,7 @@ struct ref {\n \t\tmerge:1,\n \t\tnonfastforward:1,\n \t\tis_a_tag:1,\n+\t\tupdate:1,\n \t\tdeletion:1;\n \tenum {\n \t\tREF_STATUS_NONE = 0,\ndiff --git a/remote.c b/remote.c\nindex 186814d..fe89869 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1318,15 +1318,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \n \t\tref->is_a_tag = !prefixcmp(ref->name, \"refs/tags/\");\n \n-\t\tref->nonfastforward =\n+\t\tref->update =\n \t\t\t!ref->deletion &&\n-\t\t\t!is_null_sha1(ref->old_sha1) &&\n-\t\t\t(!has_sha1_file(ref->old_sha1)\n-\t\t\t  || !ref_newer(ref->new_sha1, ref->old_sha1));\n+\t\t\t!is_null_sha1(ref->old_sha1);\n \n-\t\tif (ref->nonfastforward && !ref->force && !force_update) {\n-\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n-\t\t\tcontinue;\n+\t\tif (ref->update) {\n+\t\t\tref->nonfastforward =\n+\t\t\t\t!has_sha1_file(ref->old_sha1)\n+\t\t\t\t  || !ref_newer(ref->new_sha1, ref->old_sha1);\n+\n+\t\t\tif (ref->nonfastforward && !ref->force && !force_update) {\n+\t\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \t}\n }\n-- \n1.8.0.155.g3a063ad.dirty\n"},{"id":"203438","messageId":"1353183397-17719-5-git-send-email-chris@rorvick.com","threadId":"32137","inReplyTo":"1353183397-17719-1-git-send-email-chris@rorvick.com","subject":"[PATCH v4 4/5] push: flag updates that require force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-17T20:16:36Z","receivedAt":"2012-11-17T20:16:36Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Add a flag for indicating an update to a reference requires force.\nCurrently the nonfastforward flag of a ref is used for this when\ngenerating status the status message.  A separate flag insulates the\nstatus logic from the details of set_ref_status_for_push().\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n cache.h     |  4 +++-\n remote.c    | 11 ++++++++---\n transport.c |  2 +-\n 3 files changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 0f53d11..f410d94 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -999,7 +999,9 @@ struct ref {\n \tunsigned char old_sha1[20];\n \tunsigned char new_sha1[20];\n \tchar *symref;\n-\tunsigned int force:1,\n+\tunsigned int\n+\t\tforce:1,\n+\t\trequires_force:1,\n \t\tmerge:1,\n \t\tnonfastforward:1,\n \t\tis_a_tag:1,\ndiff --git a/remote.c b/remote.c\nindex fe89869..1e263b0 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1285,6 +1285,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \tstruct ref *ref;\n \n \tfor (ref = remote_refs; ref; ref = ref->next) {\n+\t\tint force_ref_update = ref->force || force_update;\n+\n \t\tif (ref->peer_ref)\n \t\t\thashcpy(ref->new_sha1, ref->peer_ref->new_sha1);\n \t\telse if (!send_mirror)\n@@ -1327,9 +1329,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\t\t!has_sha1_file(ref->old_sha1)\n \t\t\t\t  || !ref_newer(ref->new_sha1, ref->old_sha1);\n \n-\t\t\tif (ref->nonfastforward && !ref->force && !force_update) {\n-\t\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n-\t\t\t\tcontinue;\n+\t\t\tif (ref->nonfastforward) {\n+\t\t\t\tref->requires_force = 1;\n+\t\t\t\tif (!force_ref_update) {\n+\t\t\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/transport.c b/transport.c\nindex 5fcaea8..60a7421 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -659,7 +659,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)\n \t\tconst char *msg;\n \n \t\tstrcpy(quickref, status_abbrev(ref->old_sha1));\n-\t\tif (ref->nonfastforward) {\n+\t\tif (ref->requires_force) {\n \t\t\tstrcat(quickref, \"...\");\n \t\t\ttype = '+';\n \t\t\tmsg = \"forced update\";\n-- \n1.8.0.155.g3a063ad.dirty\n"},{"id":"203440","messageId":"1353183397-17719-6-git-send-email-chris@rorvick.com","threadId":"32137","inReplyTo":"1353183397-17719-1-git-send-email-chris@rorvick.com","subject":"[PATCH v4 5/5] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-17T20:16:37Z","receivedAt":"2012-11-17T20:16:37Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"References are allowed to update from one commit-ish to another if the\nformer is a ancestor of the latter.  This behavior is oriented to\nbranches which are expected to move with commits.  Tag references are\nexpected to be static in a repository, though, thus an update to a\ntag (lightweight and annotated) should be rejected unless the update is\nforced.\n\nTo enable this functionality, the following checks have been added to\nset_ref_status_for_push() for updating refs (i.e, not new or deletion)\nto restrict fast-forwarding in pushes:\n\n  1) The old and new references must be commits.  If this fails,\n     it is not a valid update for a branch.\n\n  2) The reference name cannot start with \"refs/tags/\".  This\n     catches lightweight tags which (usually) point to commits\n     and therefore would not be caught by (1).\n\nIf either of these checks fails, then it is flagged (by default) with a\nstatus indicating the update is being rejected due to the reference\nalready existing in the remote.  This can be overridden by passing\n--force to git push.\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n Documentation/git-push.txt | 10 +++++-----\n builtin/push.c             |  2 +-\n builtin/send-pack.c        |  5 +++++\n cache.h                    |  3 ++-\n remote.c                   | 23 +++++++++++++++++++----\n send-pack.c                |  1 +\n t/t5516-fetch-push.sh      | 30 +++++++++++++++++++++++++++++-\n transport-helper.c         |  6 ++++++\n transport.c                |  8 ++++++--\n 9 files changed, 74 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex fe46c42..479e25f 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -51,11 +51,11 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be\n updated.\n +\n The object referenced by <src> is used to update the <dst> reference\n-on the remote side, but by default this is only allowed if the\n-update can fast-forward <dst>.  By having the optional leading `+`,\n-you can tell git to update the <dst> ref even when the update is not a\n-fast-forward.  This does *not* attempt to merge <src> into <dst>.  See\n-EXAMPLES below for details.\n+on the remote side.  By default this is only allowed if the update is\n+a branch, and then only if it can fast-forward <dst>.  By having the\n+optional leading `+`, you can tell git to update the <dst> ref even when\n+the update is not a branch or it is not a fast-forward.  This does *not*\n+attempt to merge <src> into <dst>.  See EXAMPLES below for details.\n +\n `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.\n +\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 0e13e11..cd7aa3f 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -222,7 +222,7 @@ static const char message_advice_checkout_pull_push[] =\n \n static const char message_advice_ref_already_exists[] =\n \tN_(\"Updates were rejected because a matching reference already exists in\\n\"\n-\t   \"the remote and the update is not a fast-forward.\");\n+\t   \"the remote.\");\n \n static void advise_pull_before_push(void)\n {\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex fda28bc..1eabf42 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -44,6 +44,11 @@ static void print_helper_status(struct ref *ref)\n \t\t\tmsg = \"non-fast forward\";\n \t\t\tbreak;\n \n+\t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"already exists\";\n+\t\t\tbreak;\n+\n \t\tcase REF_STATUS_REJECT_NODELETE:\n \t\tcase REF_STATUS_REMOTE_REJECT:\n \t\t\tres = \"error\";\ndiff --git a/cache.h b/cache.h\nindex f410d94..127e504 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1004,13 +1004,14 @@ struct ref {\n \t\trequires_force:1,\n \t\tmerge:1,\n \t\tnonfastforward:1,\n-\t\tis_a_tag:1,\n+\t\tforwardable:1,\n \t\tupdate:1,\n \t\tdeletion:1;\n \tenum {\n \t\tREF_STATUS_NONE = 0,\n \t\tREF_STATUS_OK,\n \t\tREF_STATUS_REJECT_NONFASTFORWARD,\n+\t\tREF_STATUS_REJECT_ALREADY_EXISTS,\n \t\tREF_STATUS_REJECT_NODELETE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\ndiff --git a/remote.c b/remote.c\nindex 1e263b0..4fcd22c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1311,14 +1311,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t *     to overwrite it; you would not know what you are losing\n \t\t *     otherwise.\n \t\t *\n-\t\t * (3) if both new and old are commit-ish, and new is a\n-\t\t *     descendant of old, it is OK.\n+\t\t * (3) if new is commit-ish and old is a commit, new is a\n+\t\t *     descendant of old, and the reference is not of the\n+\t\t *     refs/tags/ hierarchy it is OK.\n \t\t *\n \t\t * (4) regardless of all of the above, removing :B is\n \t\t *     always allowed.\n \t\t */\n \n-\t\tref->is_a_tag = !prefixcmp(ref->name, \"refs/tags/\");\n+\t\tif (prefixcmp(ref->name, \"refs/tags/\")) {\n+\t\t\t// Additionally, disallow fast-forwarding if\n+\t\t\t// the old object is not a commit.  This kicks\n+\t\t\t// out annotated tags that might pass the\n+\t\t\t// is_newer() test but dangle if the reference\n+\t\t\t// is updated.\n+\t\t\tstruct object *obj = parse_object(ref->old_sha1);\n+\t\t\tref->forwardable = !obj || obj->type == OBJ_COMMIT;\n+\t\t}\n \n \t\tref->update =\n \t\t\t!ref->deletion &&\n@@ -1329,7 +1338,13 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\t\t!has_sha1_file(ref->old_sha1)\n \t\t\t\t  || !ref_newer(ref->new_sha1, ref->old_sha1);\n \n-\t\t\tif (ref->nonfastforward) {\n+\t\t\tif (!ref->forwardable) {\n+\t\t\t\tref->requires_force = 1;\n+\t\t\t\tif (!force_ref_update) {\n+\t\t\t\t\tref->status = REF_STATUS_REJECT_ALREADY_EXISTS;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t} else if (ref->nonfastforward) {\n \t\t\t\tref->requires_force = 1;\n \t\t\t\tif (!force_ref_update) {\n \t\t\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\ndiff --git a/send-pack.c b/send-pack.c\nindex f50dfd9..1c375f0 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -229,6 +229,7 @@ int send_pack(struct send_pack_args *args,\n \t\t/* Check for statuses set by set_ref_status_for_push() */\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n+\t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex b5417cc..afb9b1b 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -368,7 +368,7 @@ test_expect_success 'push with colon-less refspec (2)' '\n \t\tgit branch -D frotz\n \tfi &&\n \tgit tag -f frotz &&\n-\tgit push testrepo frotz &&\n+\tgit push -f testrepo frotz &&\n \tcheck_push_result $the_commit tags/frotz &&\n \tcheck_push_result $the_first_commit heads/frotz\n \n@@ -929,6 +929,34 @@ test_expect_success 'push into aliased refs (inconsistent)' '\n \t)\n '\n \n+test_expect_success 'push tag requires --force to update remote tag' '\n+\tmk_test heads/master &&\n+\tmk_child child1 &&\n+\tmk_child child2 &&\n+\t(\n+\t\tcd child1 &&\n+\t\tgit tag lw_tag &&\n+\t\tgit tag -a -m \"message 1\" ann_tag &&\n+\t\tgit push ../child2 lw_tag &&\n+\t\tgit push ../child2 ann_tag &&\n+\t\t>file1 &&\n+\t\tgit add file1 &&\n+\t\tgit commit -m \"file1\" &&\n+\t\tgit tag -f lw_tag &&\n+\t\tgit tag -f -a -m \"message 2\" ann_tag &&\n+\t\ttest_must_fail git push ../child2 lw_tag &&\n+\t\ttest_must_fail git push ../child2 ann_tag &&\n+\t\tgit push --force ../child2 lw_tag &&\n+\t\tgit push --force ../child2 ann_tag &&\n+\t\tgit tag -f lw_tag HEAD~ &&\n+\t\tgit tag -f -a -m \"message 3\" ann_tag &&\n+\t\ttest_must_fail git push ../child2 lw_tag &&\n+\t\ttest_must_fail git push ../child2 ann_tag &&\n+\t\tgit push --force ../child2 lw_tag &&\n+\t\tgit push --force ../child2 ann_tag\n+\t)\n+'\n+\n test_expect_success 'push --porcelain' '\n \tmk_empty &&\n \techo >.git/foo  \"To testrepo\" &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4713b69..965b778 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -661,6 +661,11 @@ static void push_update_ref_status(struct strbuf *buf,\n \t\t\tfree(msg);\n \t\t\tmsg = NULL;\n \t\t}\n+\t\telse if (!strcmp(msg, \"already exists\")) {\n+\t\t\tstatus = REF_STATUS_REJECT_ALREADY_EXISTS;\n+\t\t\tfree(msg);\n+\t\t\tmsg = NULL;\n+\t\t}\n \t}\n \n \tif (*ref)\n@@ -720,6 +725,7 @@ static int push_refs_with_push(struct transport *transport,\n \t\t/* Check for statuses set by set_ref_status_for_push() */\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n+\t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/transport.c b/transport.c\nindex 60a7421..a380ad7 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -695,6 +695,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n \t\t\t\t\t\t \"non-fast-forward\", porcelain);\n \t\tbreak;\n+\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"already exists\", porcelain);\n+\t\tbreak;\n \tcase REF_STATUS_REMOTE_REJECT:\n \t\tprint_ref_status('!', \"[remote rejected]\", ref,\n \t\t\t\t\t\t ref->deletion ? NULL : ref->peer_ref,\n@@ -740,12 +744,12 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t    ref->status != REF_STATUS_OK)\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n \t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {\n-\t\t\tif (!ref->is_a_tag)\n-\t\t\t\t*reject_mask |= REJECT_ALREADY_EXISTS;\n \t\t\tif (!strcmp(head, ref->name))\n \t\t\t\t*reject_mask |= REJECT_NON_FF_HEAD;\n \t\t\telse\n \t\t\t\t*reject_mask |= REJECT_NON_FF_OTHER;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {\n+\t\t\t*reject_mask |= REJECT_ALREADY_EXISTS;\n \t\t}\n \t}\n }\n-- \n1.8.0.155.g3a063ad.dirty\n"},{"id":"203443","messageId":"1353189237-19491-1-git-send-email-chris@rorvick.com","threadId":"32137","inReplyTo":"1353183397-17719-6-git-send-email-chris@rorvick.com","subject":"[PATCH v4.1 5/5] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-17T21:53:57Z","receivedAt":"2012-11-17T21:53:57Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"References are allowed to update from one commit-ish to another if the\nformer is a ancestor of the latter.  This behavior is oriented to\nbranches which are expected to move with commits.  Tag references are\nexpected to be static in a repository, though, thus an update to a\ntag (lightweight and annotated) should be rejected unless the update is\nforced.\n\nTo enable this functionality, the following checks have been added to\nset_ref_status_for_push() for updating refs (i.e, not new or deletion)\nto restrict fast-forwarding in pushes:\n\n  1) The old and new references must be commits.  If this fails,\n     it is not a valid update for a branch.\n\n  2) The reference name cannot start with \"refs/tags/\".  This\n     catches lightweight tags which (usually) point to commits\n     and therefore would not be caught by (1).\n\nIf either of these checks fails, then it is flagged (by default) with a\nstatus indicating the update is being rejected due to the reference\nalready existing in the remote.  This can be overridden by passing\n--force to git push.\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n\nFix C99 comment.\n\n Documentation/git-push.txt | 10 +++++-----\n builtin/push.c             |  2 +-\n builtin/send-pack.c        |  5 +++++\n cache.h                    |  3 ++-\n remote.c                   | 24 ++++++++++++++++++++----\n send-pack.c                |  1 +\n t/t5516-fetch-push.sh      | 42 +++++++++++++++++++++++++++++++++++++++++-\n transport-helper.c         |  6 ++++++\n transport.c                |  8 ++++++--\n 9 files changed, 87 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex fe46c42..479e25f 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -51,11 +51,11 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be\n updated.\n +\n The object referenced by <src> is used to update the <dst> reference\n-on the remote side, but by default this is only allowed if the\n-update can fast-forward <dst>.  By having the optional leading `+`,\n-you can tell git to update the <dst> ref even when the update is not a\n-fast-forward.  This does *not* attempt to merge <src> into <dst>.  See\n-EXAMPLES below for details.\n+on the remote side.  By default this is only allowed if the update is\n+a branch, and then only if it can fast-forward <dst>.  By having the\n+optional leading `+`, you can tell git to update the <dst> ref even when\n+the update is not a branch or it is not a fast-forward.  This does *not*\n+attempt to merge <src> into <dst>.  See EXAMPLES below for details.\n +\n `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.\n +\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 0e13e11..cd7aa3f 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -222,7 +222,7 @@ static const char message_advice_checkout_pull_push[] =\n \n static const char message_advice_ref_already_exists[] =\n \tN_(\"Updates were rejected because a matching reference already exists in\\n\"\n-\t   \"the remote and the update is not a fast-forward.\");\n+\t   \"the remote.\");\n \n static void advise_pull_before_push(void)\n {\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex fda28bc..1eabf42 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -44,6 +44,11 @@ static void print_helper_status(struct ref *ref)\n \t\t\tmsg = \"non-fast forward\";\n \t\t\tbreak;\n \n+\t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"already exists\";\n+\t\t\tbreak;\n+\n \t\tcase REF_STATUS_REJECT_NODELETE:\n \t\tcase REF_STATUS_REMOTE_REJECT:\n \t\t\tres = \"error\";\ndiff --git a/cache.h b/cache.h\nindex f410d94..127e504 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1004,13 +1004,14 @@ struct ref {\n \t\trequires_force:1,\n \t\tmerge:1,\n \t\tnonfastforward:1,\n-\t\tis_a_tag:1,\n+\t\tforwardable:1,\n \t\tupdate:1,\n \t\tdeletion:1;\n \tenum {\n \t\tREF_STATUS_NONE = 0,\n \t\tREF_STATUS_OK,\n \t\tREF_STATUS_REJECT_NONFASTFORWARD,\n+\t\tREF_STATUS_REJECT_ALREADY_EXISTS,\n \t\tREF_STATUS_REJECT_NODELETE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\ndiff --git a/remote.c b/remote.c\nindex 44e72d6..a723f7a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1311,14 +1311,24 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t *     to overwrite it; you would not know what you are losing\n \t\t *     otherwise.\n \t\t *\n-\t\t * (3) if both new and old are commit-ish, and new is a\n-\t\t *     descendant of old, it is OK.\n+\t\t * (3) if new is commit-ish and old is a commit, new is a\n+\t\t *     descendant of old, and the reference is not of the\n+\t\t *     refs/tags/ hierarchy it is OK.\n \t\t *\n \t\t * (4) regardless of all of the above, removing :B is\n \t\t *     always allowed.\n \t\t */\n \n-\t\tref->is_a_tag = !prefixcmp(ref->name, \"refs/tags/\");\n+\t\tif (prefixcmp(ref->name, \"refs/tags/\")) {\n+\t\t\t/* Additionally, disallow fast-forwarding if\n+\t\t\t * the old object is not a commit.  This kicks\n+\t\t\t * out annotated tags that might pass the\n+\t\t\t * is_newer() test but dangle if the reference\n+\t\t\t * is updated.\n+\t\t\t */\n+\t\t\tstruct object *obj = parse_object(ref->old_sha1);\n+\t\t\tref->forwardable = !obj || obj->type == OBJ_COMMIT;\n+\t\t}\n \n \t\tref->update =\n \t\t\t!ref->deletion &&\n@@ -1329,7 +1339,13 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\t\t!has_sha1_file(ref->old_sha1)\n \t\t\t\t  || !ref_newer(ref->new_sha1, ref->old_sha1);\n \n-\t\t\tif (ref->nonfastforward) {\n+\t\t\tif (!ref->forwardable) {\n+\t\t\t\tref->requires_force = 1;\n+\t\t\t\tif (!force_ref_update) {\n+\t\t\t\t\tref->status = REF_STATUS_REJECT_ALREADY_EXISTS;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t} else if (ref->nonfastforward) {\n \t\t\t\tref->requires_force = 1;\n \t\t\t\tif (!force_ref_update) {\n \t\t\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\ndiff --git a/send-pack.c b/send-pack.c\nindex f50dfd9..1c375f0 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -229,6 +229,7 @@ int send_pack(struct send_pack_args *args,\n \t\t/* Check for statuses set by set_ref_status_for_push() */\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n+\t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex b5417cc..ca800b2 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -368,7 +368,7 @@ test_expect_success 'push with colon-less refspec (2)' '\n \t\tgit branch -D frotz\n \tfi &&\n \tgit tag -f frotz &&\n-\tgit push testrepo frotz &&\n+\tgit push -f testrepo frotz &&\n \tcheck_push_result $the_commit tags/frotz &&\n \tcheck_push_result $the_first_commit heads/frotz\n \n@@ -929,6 +929,46 @@ test_expect_success 'push into aliased refs (inconsistent)' '\n \t)\n '\n \n+test_expect_success 'push requires --force to update lightweight tag' '\n+\tmk_test heads/master &&\n+\tmk_child child1 &&\n+\tmk_child child2 &&\n+\t(\n+\t\tcd child1 &&\n+\t\tgit tag Tag &&\n+\t\tgit push ../child2 Tag &&\n+\t\t>file1 &&\n+\t\tgit add file1 &&\n+\t\tgit commit -m \"file1\" &&\n+\t\tgit tag -f Tag &&\n+\t\ttest_must_fail git push ../child2 Tag &&\n+\t\tgit push --force ../child2 Tag &&\n+\t\tgit tag -f Tag &&\n+\t\ttest_must_fail git push ../child2 Tag HEAD~ &&\n+\t\tgit push --force ../child2 Tag\n+\t)\n+'\n+\n+test_expect_success 'push requires --force to update annotated tag' '\n+\tmk_test heads/master &&\n+\tmk_child child1 &&\n+\tmk_child child2 &&\n+\t(\n+\t\tcd child1 &&\n+\t\tgit tag -a -m \"message 1\" Tag &&\n+\t\tgit push ../child2 Tag:refs/tmp/Tag &&\n+\t\t>file1 &&\n+\t\tgit add file1 &&\n+\t\tgit commit -m \"file1\" &&\n+\t\tgit tag -f -a -m \"message 2\" Tag &&\n+\t\ttest_must_fail git push ../child2 Tag:refs/tmp/Tag &&\n+\t\tgit push --force ../child2 Tag:refs/tmp/Tag &&\n+\t\tgit tag -f -a -m \"message 3\" Tag HEAD~ &&\n+\t\ttest_must_fail git push ../child2 Tag:refs/tmp/Tag &&\n+\t\tgit push --force ../child2 Tag:refs/tmp/Tag\n+\t)\n+'\n+\n test_expect_success 'push --porcelain' '\n \tmk_empty &&\n \techo >.git/foo  \"To testrepo\" &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4713b69..965b778 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -661,6 +661,11 @@ static void push_update_ref_status(struct strbuf *buf,\n \t\t\tfree(msg);\n \t\t\tmsg = NULL;\n \t\t}\n+\t\telse if (!strcmp(msg, \"already exists\")) {\n+\t\t\tstatus = REF_STATUS_REJECT_ALREADY_EXISTS;\n+\t\t\tfree(msg);\n+\t\t\tmsg = NULL;\n+\t\t}\n \t}\n \n \tif (*ref)\n@@ -720,6 +725,7 @@ static int push_refs_with_push(struct transport *transport,\n \t\t/* Check for statuses set by set_ref_status_for_push() */\n \t\tswitch (ref->status) {\n \t\tcase REF_STATUS_REJECT_NONFASTFORWARD:\n+\t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/transport.c b/transport.c\nindex 60a7421..a380ad7 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -695,6 +695,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i\n \t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n \t\t\t\t\t\t \"non-fast-forward\", porcelain);\n \t\tbreak;\n+\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"already exists\", porcelain);\n+\t\tbreak;\n \tcase REF_STATUS_REMOTE_REJECT:\n \t\tprint_ref_status('!', \"[remote rejected]\", ref,\n \t\t\t\t\t\t ref->deletion ? NULL : ref->peer_ref,\n@@ -740,12 +744,12 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t    ref->status != REF_STATUS_OK)\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n \t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {\n-\t\t\tif (!ref->is_a_tag)\n-\t\t\t\t*reject_mask |= REJECT_ALREADY_EXISTS;\n \t\t\tif (!strcmp(head, ref->name))\n \t\t\t\t*reject_mask |= REJECT_NON_FF_HEAD;\n \t\t\telse\n \t\t\t\t*reject_mask |= REJECT_NON_FF_OTHER;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {\n+\t\t\t*reject_mask |= REJECT_ALREADY_EXISTS;\n \t\t}\n \t}\n }\n-- \n1.7.11.7\n"},{"id":"203511","messageId":"7va9udxryf.fsf@alter.siamese.dyndns.org","threadId":"32137","inReplyTo":"1353189237-19491-1-git-send-email-chris@rorvick.com","subject":"Re: [PATCH v4.1 5/5] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-19T20:23:20Z","receivedAt":"2012-11-19T20:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Rorvick <chris@rorvick.com> writes:\n\n> References are allowed to update from one commit-ish to another if the\n> former is a ancestor of the latter.  This behavior is oriented to\n> branches which are expected to move with commits.  Tag references are\n> expected to be static in a repository, though, thus an update to a\n> tag (lightweight and annotated) should be rejected unless the update is\n> forced.\n>\n> To enable this functionality, the following checks have been added to\n> set_ref_status_for_push() for updating refs (i.e, not new or deletion)\n> to restrict fast-forwarding in pushes:\n>\n>   1) The old and new references must be commits.  If this fails,\n>      it is not a valid update for a branch.\n>\n>   2) The reference name cannot start with \"refs/tags/\".  This\n>      catches lightweight tags which (usually) point to commits\n>      and therefore would not be caught by (1).\n>\n> If either of these checks fails, then it is flagged (by default) with a\n> status indicating the update is being rejected due to the reference\n> already existing in the remote.  This can be overridden by passing\n> --force to git push.\n\nThis, if it were implemented back when we first added \"git push\",\nwould have been a nice safety, but added after 1.8.0 it would be a\nregression, so we would need backward compatibility notes in the\nrelease notes.\n\nNot an objection; just making a mental note.\n\n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index fe46c42..479e25f 100644\n> --- a/Documentation/git-push.txt\n> +++ b/Documentation/git-push.txt\n> @@ -51,11 +51,11 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be\n>  updated.\n>  +\n>  The object referenced by <src> is used to update the <dst> reference\n> -on the remote side, but by default this is only allowed if the\n> -update can fast-forward <dst>.  By having the optional leading `+`,\n> -you can tell git to update the <dst> ref even when the update is not a\n> -fast-forward.  This does *not* attempt to merge <src> into <dst>.  See\n> -EXAMPLES below for details.\n> +on the remote side.  By default this is only allowed if the update is\n> +a branch, and then only if it can fast-forward <dst>.  By having the\n\nI can already see the next person asking \"I can update refs/notes\nthe same way.  The doc says it applies only to the branches.  Is\nthis a bug?\".\n\n> diff --git a/cache.h b/cache.h\n> index f410d94..127e504 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1004,13 +1004,14 @@ struct ref {\n>  \t\trequires_force:1,\n>  \t\tmerge:1,\n>  \t\tnonfastforward:1,\n> -\t\tis_a_tag:1,\n> +\t\tforwardable:1,\n\nThis is somewhat an unfortunate churn.  Perhaps is_a_tag could have\nstarted its life under its final name in the series?\n\nI am assuming that the struct members will be initialized to 0 (false),\nso everything by default is now not forwardable if somebody forgets\nto set this flag?\n\n>  \tenum {\n>  \t\tREF_STATUS_NONE = 0,\n>  \t\tREF_STATUS_OK,\n>  \t\tREF_STATUS_REJECT_NONFASTFORWARD,\n> +\t\tREF_STATUS_REJECT_ALREADY_EXISTS,\n>  \t\tREF_STATUS_REJECT_NODELETE,\n>  \t\tREF_STATUS_UPTODATE,\n>  \t\tREF_STATUS_REMOTE_REJECT,\n> diff --git a/remote.c b/remote.c\n> index 44e72d6..a723f7a 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1311,14 +1311,24 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n>  \t\t *     to overwrite it; you would not know what you are losing\n>  \t\t *     otherwise.\n>  \t\t *\n> -\t\t * (3) if both new and old are commit-ish, and new is a\n> -\t\t *     descendant of old, it is OK.\n> +\t\t * (3) if new is commit-ish and old is a commit, new is a\n> +\t\t *     descendant of old, and the reference is not of the\n> +\t\t *     refs/tags/ hierarchy it is OK.\n>  \t\t *\n>  \t\t * (4) regardless of all of the above, removing :B is\n>  \t\t *     always allowed.\n>  \t\t */\n\nI think this is unreadable.  Isn't this more like\n\n    (1.5) if the destination is inside refs/tags/ and already\n          exists, you are not allowed to overwrite it without\n          --force or +A:B notation.\n\nearly in the rule set?\n\n> -\t\tref->is_a_tag = !prefixcmp(ref->name, \"refs/tags/\");\n> +\t\tif (prefixcmp(ref->name, \"refs/tags/\")) {\n> +\t\t\t/* Additionally, disallow fast-forwarding if\n> +\t\t\t * the old object is not a commit.  This kicks\n> +\t\t\t * out annotated tags that might pass the\n> +\t\t\t * is_newer() test but dangle if the reference\n> +\t\t\t * is updated.\n> +\t\t\t */\n\nHuh?  prefixcmp() excludes refs/tags/ hierarchy. What is an\nannotated tag doing there?  Is this comment outdated???\n\n\t/*\n         * Also please end the first line of a multi-line\n         * comment with '/', '*', and nothing else.\n         */\n\nRegarding the other arm of this \"if (not in refs/tags/ hierarchy)\",\nwhat happens when refs/tags/foo is a reference to a blob or a tree?\nThis series declares that the point of tag is not to let people to\nmove it willy-nilly, and I think it is in line with its spirit if\nyou just rejected non-creation events.\n\nAlso, I suspect that you do not even need to have old object locally\nwhen looking at refs/tags/ hierarchy.  \"Do not overwrite tags\" can\nbe enforced by only noticing that they already have something; you\ndo not know what that something actually is to prevent yourself from\noverwriting it.  You may have to update the rule (2) in remote.c\naround l.1311 for this.\n\n> +test_expect_success 'push requires --force to update lightweight tag' '\n> +\tmk_test heads/master &&\n> +\tmk_child child1 &&\n> +\tmk_child child2 &&\n> +\t(\n> +\t\tcd child1 &&\n> +\t\tgit tag Tag &&\n> +\t\tgit push ../child2 Tag &&\n\nDon't you want to make sure that another \"git push ../child2 Tag\" at\nthis point, i.e. attempt to update Tag with the same, should succeed\nwithout --force?\n"},{"id":"203536","messageId":"CAEUsAPa9fiF9cuMqRHNsQSz_CsbbvdO-eGDS9HWW3MrAYZPA8w@mail.gmail.com","threadId":"32137","inReplyTo":"7va9udxryf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4.1 5/5] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-20T04:43:40Z","receivedAt":"2012-11-20T04:43:40Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Mon, Nov 19, 2012 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Chris Rorvick <chris@rorvick.com> writes:\n>>  The object referenced by <src> is used to update the <dst> reference\n>> -on the remote side, but by default this is only allowed if the\n>> -update can fast-forward <dst>.  By having the optional leading `+`,\n>> -you can tell git to update the <dst> ref even when the update is not a\n>> -fast-forward.  This does *not* attempt to merge <src> into <dst>.  See\n>> -EXAMPLES below for details.\n>> +on the remote side.  By default this is only allowed if the update is\n>> +a branch, and then only if it can fast-forward <dst>.  By having the\n>\n> I can already see the next person asking \"I can update refs/notes\n> the same way.  The doc says it applies only to the branches.  Is\n> this a bug?\".\n\nSure, will fix.\n\n>> diff --git a/cache.h b/cache.h\n>> index f410d94..127e504 100644\n>> --- a/cache.h\n>> +++ b/cache.h\n>> @@ -1004,13 +1004,14 @@ struct ref {\n>>               requires_force:1,\n>>               merge:1,\n>>               nonfastforward:1,\n>> -             is_a_tag:1,\n>> +             forwardable:1,\n>\n> This is somewhat an unfortunate churn.  Perhaps is_a_tag could have\n> started its life under its final name in the series?\n>\n> I am assuming that the struct members will be initialized to 0 (false),\n> so everything by default is now not forwardable if somebody forgets\n> to set this flag?\n\nYeah, this was one of a few stupid mistakes.  Previously I used\n'forwardable' throughout, but that is awkward except in the last\ncommit since until then everything is allowed to fast-forward and the\nflag is only used to output tag-specific advice.  But inverting the\nmeaning of the flag is dumb, and I didn't even do it right.\n\nBut, as I think you're suggesting, it probably makes more sense to use\na flag that prevents fast-forwarding when set.  So maybe\n\"not_forwardable\", or \"is_a_tag\" => \"not_forwardable\" if you think the\nrenaming is a good idea.  Or maybe just \"is_a_tag\"?  I think the\nrename is good because its meaning is more general in the last commit.\n\n>> diff --git a/remote.c b/remote.c\n>> index 44e72d6..a723f7a 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -1311,14 +1311,24 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n>>                *     to overwrite it; you would not know what you are losing\n>>                *     otherwise.\n>>                *\n>> -              * (3) if both new and old are commit-ish, and new is a\n>> -              *     descendant of old, it is OK.\n>> +              * (3) if new is commit-ish and old is a commit, new is a\n>> +              *     descendant of old, and the reference is not of the\n>> +              *     refs/tags/ hierarchy it is OK.\n>>                *\n>>                * (4) regardless of all of the above, removing :B is\n>>                *     always allowed.\n>>                */\n>\n> I think this is unreadable.  Isn't this more like\n>\n>     (1.5) if the destination is inside refs/tags/ and already\n>           exists, you are not allowed to overwrite it without\n>           --force or +A:B notation.\n>\n> early in the rule set?\n\nYes, something like that is much better.\n\n>> -             ref->is_a_tag = !prefixcmp(ref->name, \"refs/tags/\");\n>> +             if (prefixcmp(ref->name, \"refs/tags/\")) {\n>> +                     /* Additionally, disallow fast-forwarding if\n>> +                      * the old object is not a commit.  This kicks\n>> +                      * out annotated tags that might pass the\n>> +                      * is_newer() test but dangle if the reference\n>> +                      * is updated.\n>> +                      */\n>\n> Huh?  prefixcmp() excludes refs/tags/ hierarchy. What is an\n> annotated tag doing there?  Is this comment outdated???\n\nI think the comment is accurate, although inverting the meaning of the\nflag probably doesn't help the clarity of this change.\n\nWhat do you mean by \"there\"?  Annotated tags normally are referenced\nvia something under refs/tags/, but they could be elsewhere, right?\nSo what I meant to say was: if the reference is not under refs/tags/\nthen the starting point has to be a commit for it to be forwardable.\n\nI interpreted parse_object(ref->old_sha1) == NULL as the old thing not\nexisting (rule 1) but I'm pretty sure that was a mistake.  I should\nfirst test it with is_null_sha1, if true then it doesn't exist.\nOtherwise parse_object() returning NULL means we just don't have the\nobject and therefore should follow rule 2.\n\n> Regarding the other arm of this \"if (not in refs/tags/ hierarchy)\",\n> what happens when refs/tags/foo is a reference to a blob or a tree?\n> This series declares that the point of tag is not to let people to\n> move it willy-nilly, and I think it is in line with its spirit if\n> you just rejected non-creation events.\n\nNothing under refs/tags/ is updateable and only commits are updateable\noutside of that due to the new logic.  The ref_newer() call will\nreject updating to blobs and trees, although that probably isn't the\nright thing to do.  Previously I was ensuring both old and new objects\nwere commits if the ref was not under refs/tags/, but it occurred to\nme that fast-forwarding from a commit to a tag might be a reasonable\nthing to want to do.  But a rejected update to a blob should get the\nalready-exists advice, not a message suggesting a merge (which is what\nyou get by relying on ref_newer() to fail.)\n\n> Also, I suspect that you do not even need to have old object locally\n> when looking at refs/tags/ hierarchy.  \"Do not overwrite tags\" can\n> be enforced by only noticing that they already have something; you\n> do not know what that something actually is to prevent yourself from\n> overwriting it.  You may have to update the rule (2) in remote.c\n> around l.1311 for this.\n\nIsn't it already doing this?  I think I've either confounded things\nwith my mistakes or I'm missing this point.\n\n>> +test_expect_success 'push requires --force to update lightweight tag' '\n>> +     mk_test heads/master &&\n>> +     mk_child child1 &&\n>> +     mk_child child2 &&\n>> +     (\n>> +             cd child1 &&\n>> +             git tag Tag &&\n>> +             git push ../child2 Tag &&\n>\n> Don't you want to make sure that another \"git push ../child2 Tag\" at\n> this point, i.e. attempt to update Tag with the same, should succeed\n> without --force?\n\nYes, that is clearly a good addition.\n\nThanks for all the feedback.  Sorry I mucked up this series a bit,\ntried to get an update out too quickly I guess.\n\nChris\n"},{"id":"203538","messageId":"7vwqxgsu98.fsf@alter.siamese.dyndns.org","threadId":"32137","inReplyTo":"CAEUsAPa9fiF9cuMqRHNsQSz_CsbbvdO-eGDS9HWW3MrAYZPA8w@mail.gmail.com","subject":"Re: [PATCH v4.1 5/5] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-20T05:44:51Z","receivedAt":"2012-11-20T05:44:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Rorvick <chris@rorvick.com> writes:\n\n> On Mon, Nov 19, 2012 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Yeah, this was one of a few stupid mistakes.  Previously I used\n> 'forwardable' throughout, but that is awkward except in the last\n> commit since until then everything is allowed to fast-forward and the\n> flag is only used to output tag-specific advice.  But inverting the\n> meaning of the flag is dumb, and I didn't even do it right.\n>\n> But, as I think you're suggesting, it probably makes more sense to use\n> a flag that prevents fast-forwarding when set.  So maybe\n> \"not_forwardable\", or \"is_a_tag\" => \"not_forwardable\" if you think the\n> renaming is a good idea.\n\nYeah, calling it not-forwardable from the beginning would be a\nsensible approach, I would think.\n\nThanks.\n"}]}