{"thread":{"id":"32171","subject":"[PATCH 4/7] push: flag updates that require force","startedAt":"2012-11-23T04:21:48Z","lastAt":"2012-11-28T16:58:36Z","messageCount":18,"participants":["Chris Rorvick","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"203681","messageId":"1353644515-17349-1-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":null,"subject":"[PATCH v5 0/7] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-23T04:21:48Z","receivedAt":"2012-11-23T04:21:48Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Incorporated Junio's feedback.  Also, I broke the last patch of the\nprevious series out into three to make the changes more clear.\n\nThis 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: require force for refs under refs/tags/\n     push: require force for annotated tags\n     push: clarify rejection of update to non-commit-ish\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 series prevents fast-forwards if the reference is under the\n     refs/tags/* hierarchy or if the old object is a tag.\n\nChris Rorvick (7):\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: require force for refs under refs/tags/\n  push: require force for annotated tags\n  push: clarify rejection of update to non-commit-ish\n\n Documentation/git-push.txt |  9 ++++---\n builtin/push.c             | 24 ++++++++++-------\n builtin/send-pack.c        |  9 +++++--\n cache.h                    |  7 ++++-\n remote.c                   | 65 +++++++++++++++++++++++++++++++++++++---------\n send-pack.c                |  1 +\n t/t5516-fetch-push.sh      | 44 ++++++++++++++++++++++++++++++-\n transport-helper.c         |  6 +++++\n transport.c                | 25 +++++++++++-------\n transport.h                | 10 ++++---\n 10 files changed, 157 insertions(+), 43 deletions(-)\n\n-- \n1.8.0.209.gf3828dc\n"},{"id":"203679","messageId":"1353644515-17349-2-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":"1353644515-17349-1-git-send-email-chris@rorvick.com","subject":"[PATCH 1/7] push: return reject reasons via a mask","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-23T04:21:49Z","receivedAt":"2012-11-23T04:21:49Z","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..4a0e7ef 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 & REJECT_NON_FF_HEAD) {\n \t\tadvise_pull_before_push();\n-\t\tbreak;\n-\tcase 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\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..b0c9f1b 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 |= REJECT_NON_FF_HEAD;\n \t\t\telse\n-\t\t\t\t*nonfastforward = NON_FF_OTHER;\n+\t\t\t\t*reject_mask |= REJECT_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..5f76ca2 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 REJECT_NON_FF_HEAD     0x01\n+#define REJECT_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.209.gf3828dc\n"},{"id":"203678","messageId":"1353644515-17349-3-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":"1353644515-17349-1-git-send-email-chris@rorvick.com","subject":"[PATCH 2/7] push: add advice for rejected tag reference","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-23T04:21:50Z","receivedAt":"2012-11-23T04:21:50Z","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 are for branches, just\ntell the user the reference already exists.\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n builtin/push.c | 11 +++++++++++\n cache.h        |  1 +\n remote.c       | 10 ++++++++++\n transport.c    |  2 ++\n transport.h    |  1 +\n 5 files changed, 25 insertions(+)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 4a0e7ef..1391983 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 the destination reference already exists\\n\"\n+\t   \"in 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@@ -272,6 +281,8 @@ static int push_with_options(struct transport *transport, int flags)\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..d72b64d 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\tnot_forwardable:1,\n \t\tdeletion:1;\n \tenum {\n \t\tREF_STATUS_NONE = 0,\ndiff --git a/remote.c b/remote.c\nindex 04fd9ea..5101683 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1279,6 +1279,14 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \treturn 0;\n }\n \n+static inline int is_forwardable(struct ref* ref)\n+{\n+\tif (!prefixcmp(ref->name, \"refs/tags/\"))\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \tint force_update)\n {\n@@ -1316,6 +1324,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->not_forwardable = !is_forwardable(ref);\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 b0c9f1b..271965e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -740,6 +740,8 @@ 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->not_forwardable)\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\ndiff --git a/transport.h b/transport.h\nindex 5f76ca2..7e86352 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -142,6 +142,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\n \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.209.gf3828dc\n"},{"id":"203680","messageId":"1353644515-17349-4-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":"1353644515-17349-1-git-send-email-chris@rorvick.com","subject":"[PATCH 3/7] push: flag updates","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-23T04:21:51Z","receivedAt":"2012-11-23T04:21:51Z","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 update is not a delete,\nthen mark as an update.  This is in preparation for handling tags and\nbranches 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 d72b64d..722321c 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\tnot_forwardable: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 5101683..07040b8 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1326,15 +1326,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \n \t\tref->not_forwardable = !is_forwardable(ref);\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.209.gf3828dc\n"},{"id":"203677","messageId":"1353644515-17349-5-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":"1353644515-17349-1-git-send-email-chris@rorvick.com","subject":"[PATCH 4/7] push: flag updates that require force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-23T04:21:52Z","receivedAt":"2012-11-23T04:21:52Z","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 722321c..b7ab4ac 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\tnot_forwardable:1,\ndiff --git a/remote.c b/remote.c\nindex 07040b8..4a6f822 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1293,6 +1293,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@@ -1335,9 +1337,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 271965e..ea8bbbd 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.209.gf3828dc\n"},{"id":"203684","messageId":"1353644515-17349-6-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":"1353644515-17349-1-git-send-email-chris@rorvick.com","subject":"[PATCH 5/7] push: require force for refs under refs/tags/","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-23T04:21:53Z","receivedAt":"2012-11-23T04:21:53Z","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 an 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\nsomething under refs/tags/ should be rejected unless the update is\nforced.\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n Documentation/git-push.txt | 11 ++++++-----\n builtin/push.c             |  2 +-\n builtin/send-pack.c        |  5 +++++\n cache.h                    |  1 +\n remote.c                   | 18 ++++++++++++++----\n send-pack.c                |  1 +\n t/t5516-fetch-push.sh      | 23 ++++++++++++++++++++++-\n transport-helper.c         |  6 ++++++\n transport.c                |  8 ++++++--\n 9 files changed, 62 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex fe46c42..09bdec7 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -51,11 +51,12 @@ 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 <dst> is not\n+under refs/tags/, and then only if it can fast-forward <dst>.  By having\n+the optional leading `+`, you can tell git to update the <dst> ref even\n+if it is not allowed by default (e.g., it is not a fast-forward.)  This\n+does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for\n+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 1391983..2143833 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 the destination reference already exists\\n\"\n-\t   \"in the remote and the update is not a fast-forward.\");\n+\t   \"in 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 b7ab4ac..a32a0ea 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1011,6 +1011,7 @@ struct ref {\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 4a6f822..012b52f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1315,14 +1315,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t *\n \t\t * (1) if the old thing does not exist, it is OK.\n \t\t *\n-\t\t * (2) if you do not have the old thing, you are not allowed\n+\t\t * (2) if the destination is under refs/tags/ you are\n+\t\t *     not allowed to overwrite it; tags are expected\n+\t\t *     to be static once created\n+\t\t *\n+\t\t * (3) if you do not have the old thing, you are not allowed\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 * (4) if both new and old are commit-ish, and new is a\n \t\t *     descendant of old, it is OK.\n \t\t *\n-\t\t * (4) regardless of all of the above, removing :B is\n+\t\t * (5) regardless of all of the above, removing :B is\n \t\t *     always allowed.\n \t\t */\n \n@@ -1337,7 +1341,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->not_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..8f024a0 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,27 @@ 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\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 --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 ea8bbbd..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->not_forwardable)\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.209.gf3828dc\n"},{"id":"203682","messageId":"1353644515-17349-7-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":"1353644515-17349-1-git-send-email-chris@rorvick.com","subject":"[PATCH 6/7] push: require force for annotated tags","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-23T04:21:54Z","receivedAt":"2012-11-23T04:21:54Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Do not allow fast-forwarding of references that point to a tag object.\nThis keeps the behavior consistent with lightweight tags.  Additionally,\nallowing the reference to update could leave the old object dangling.\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n Documentation/git-push.txt | 10 +++++-----\n remote.c                   | 11 +++++++++--\n t/t5516-fetch-push.sh      | 21 +++++++++++++++++++++\n 3 files changed, 35 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 09bdec7..7a04ce5 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -52,11 +52,11 @@ updated.\n +\n The object referenced by <src> is used to update the <dst> reference\n on the remote side.  By default this is only allowed if <dst> is not\n-under refs/tags/, and then only if it can fast-forward <dst>.  By having\n-the optional leading `+`, you can tell git to update the <dst> ref even\n-if it is not allowed by default (e.g., it is not a fast-forward.)  This\n-does *not* attempt to merge <src> into <dst>.  See EXAMPLES below for\n-details.\n+a tag (annotated or lightweight), and then only if it can fast-forward\n+<dst>.  By having the optional leading `+`, you can tell git to update\n+the <dst> ref even if it is not allowed by default (e.g., it is not a\n+fast-forward.)  This does *not* attempt to merge <src> into <dst>.  See\n+EXAMPLES below for details.\n +\n `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.\n +\ndiff --git a/remote.c b/remote.c\nindex 012b52f..f5bc4e7 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1281,9 +1281,16 @@ int match_push_refs(struct ref *src, struct ref **dst,\n \n static inline int is_forwardable(struct ref* ref)\n {\n+\tstruct object *o;\n+\n \tif (!prefixcmp(ref->name, \"refs/tags/\"))\n \t\treturn 0;\n \n+\t/* old object must be a commit */\n+\to = parse_object(ref->old_sha1);\n+\tif (!o || o->type != OBJ_COMMIT)\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \n@@ -1323,8 +1330,8 @@ 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 * (4) if both new and old are commit-ish, and new is a\n-\t\t *     descendant of old, it is OK.\n+\t\t * (4) if old is a commit and new is a descendant of old\n+\t\t *     (implying new is commit-ish), it is OK.\n \t\t *\n \t\t * (5) regardless of all of the above, removing :B is\n \t\t *     always allowed.\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 8f024a0..6009372 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -950,6 +950,27 @@ test_expect_success 'push requires --force to update lightweight 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\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\" &&\n-- \n1.8.0.209.gf3828dc\n"},{"id":"203683","messageId":"1353644515-17349-8-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":"1353644515-17349-1-git-send-email-chris@rorvick.com","subject":"[PATCH 7/7] push: clarify rejection of update to non-commit-ish","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-23T04:21:55Z","receivedAt":"2012-11-23T04:21:55Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Pushes must already (by default) update to a commit-ish due the fast-\nforward check in set_ref_status_for_push().  But rejecting for not being\na fast-forward suggests the situation can be resolved with a merge.\nFlag these updates (i.e., to a blob or a tree) as not forwardable so the\nuser is presented with more appropriate advice.\n\nSigned-off-by: Chris Rorvick <chris@rorvick.com>\n---\n remote.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex f5bc4e7..ee0c1e5 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref)\n \tif (!o || o->type != OBJ_COMMIT)\n \t\treturn 0;\n \n+\t/* new object must be commit-ish */\n+\to = deref_tag(parse_object(ref->new_sha1), NULL, 0);\n+\tif (!o || o->type != OBJ_COMMIT)\n+\t\treturn 0;\n+\n \treturn 1;\n }\n \n-- \n1.8.0.209.gf3828dc\n"},{"id":"203903","messageId":"7vobikryrc.fsf@alter.siamese.dyndns.org","threadId":"32171","inReplyTo":"1353644515-17349-2-git-send-email-chris@rorvick.com","subject":"Re: [PATCH 1/7] push: return reject reasons via a mask","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-26T18:43:19Z","receivedAt":"2012-11-26T18:43:19Z","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> Pass all rejection reasons back from transport_push().  The logic is\n> simpler and more flexible with regard to providing useful feedback.\n>\n> Signed-off-by: Chris Rorvick <chris@rorvick.com>\n> ---\n\nThanks for a reroll.\n\nI do not think they are \"masks\", by the way.  They are set of flags\n(i.e. a bitset).\n\nA bitset is often called \"a mask\" when it is used to \"mask\" a subset\nof bits in another bitset that has the real information, either to\nignore irrelevant bits or to pick only the relevant bits from the\nlatter.  And your \"reject_mask\" is never used as a mask in this\npatch---it is the bitset that has the real information and it gets\nmasked by constant masks like REJECT_NON_FF_HEAD.\n\nIn any case, naming it as \"reject_mask\" is like calling a counter as\n\"counter_int\".  It is more important to name it after its purpose\nthan after its type, and because this is to record the reasons why\nthe push was rejected, \"rejection_reason\" might be a better name for\nit.\n"},{"id":"203904","messageId":"7vk3t8ryap.fsf@alter.siamese.dyndns.org","threadId":"32171","inReplyTo":"1353644515-17349-8-git-send-email-chris@rorvick.com","subject":"Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-26T18:53:18Z","receivedAt":"2012-11-26T18:53:18Z","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> Pushes must already (by default) update to a commit-ish due the fast-\n> forward check in set_ref_status_for_push().  But rejecting for not being\n> a fast-forward suggests the situation can be resolved with a merge.\n> Flag these updates (i.e., to a blob or a tree) as not forwardable so the\n> user is presented with more appropriate advice.\n>\n> Signed-off-by: Chris Rorvick <chris@rorvick.com>\n> ---\n>  remote.c | 5 +++++\n>  1 file changed, 5 insertions(+)\n>\n> diff --git a/remote.c b/remote.c\n> index f5bc4e7..ee0c1e5 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref)\n>  \tif (!o || o->type != OBJ_COMMIT)\n>  \t\treturn 0;\n>  \n> +\t/* new object must be commit-ish */\n> +\to = deref_tag(parse_object(ref->new_sha1), NULL, 0);\n> +\tif (!o || o->type != OBJ_COMMIT)\n> +\t\treturn 0;\n> +\n\nI think the original code used ref_newer() which took commit-ish on\nboth sides.\n\nWith this code, the old must be a commit but new can be a tag that\npoints at a commit?  Why?\n"},{"id":"203905","messageId":"7vfw3wry4d.fsf@alter.siamese.dyndns.org","threadId":"32171","inReplyTo":"1353644515-17349-6-git-send-email-chris@rorvick.com","subject":"Re: [PATCH 5/7] push: require force for refs under refs/tags/","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-26T18:57:06Z","receivedAt":"2012-11-26T18:57:06Z","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> diff --git a/remote.c b/remote.c\n> index 4a6f822..012b52f 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -1315,14 +1315,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n>  \t\t *\n>  \t\t * (1) if the old thing does not exist, it is OK.\n>  \t\t *\n> -\t\t * (2) if you do not have the old thing, you are not allowed\n> +\t\t * (2) if the destination is under refs/tags/ you are\n> +\t\t *     not allowed to overwrite it; tags are expected\n> +\t\t *     to be static once created\n> +\t\t *\n> +\t\t * (3) if you do not have the old thing, you are not allowed\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 * (4) if both new and old are commit-ish, and new is a\n>  \t\t *     descendant of old, it is OK.\n>  \t\t *\n> -\t\t * (4) regardless of all of the above, removing :B is\n> +\t\t * (5) regardless of all of the above, removing :B is\n>  \t\t *     always allowed.\n>  \t\t */\n\nWe may want to reword (0) to make it clear that --force\n(and +A:B) can be used to defeat all the other rules.\n\nThe updated logic in the patch looks sensible.  Thanks.\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\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"},{"id":"203962","messageId":"CAEUsAPaPPZPR3VM1w=nFhdRG_enKwRpiW2X29z0731VdEMezMg@mail.gmail.com","threadId":"32171","inReplyTo":"7vobikryrc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] push: return reject reasons via a mask","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-27T03:00:14Z","receivedAt":"2012-11-27T03:00:14Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Mon, Nov 26, 2012 at 12:43 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Chris Rorvick <chris@rorvick.com> writes:\n>\n>> Pass all rejection reasons back from transport_push().  The logic is\n>> simpler and more flexible with regard to providing useful feedback.\n>>\n>> Signed-off-by: Chris Rorvick <chris@rorvick.com>\n>> ---\n>\n> In any case, naming it as \"reject_mask\" is like calling a counter as\n> \"counter_int\".  It is more important to name it after its purpose\n> than after its type\n\nAgreed.\n\n> and because this is to record the reasons why\n> the push was rejected, \"rejection_reason\" might be a better name for\n> it.\n\nYes, that is better for all the reasons you mention.  I will fix this up.\n"},{"id":"203963","messageId":"CAEUsAPZq1POKQZd5vZy55nEj2kB4CqgxL9_d_0sQs33P_Gcemg@mail.gmail.com","threadId":"32171","inReplyTo":"7vk3t8ryap.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-27T03:52:40Z","receivedAt":"2012-11-27T03:52:40Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Mon, Nov 26, 2012 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Chris Rorvick <chris@rorvick.com> writes:\n>\n>> Pushes must already (by default) update to a commit-ish due the fast-\n>> forward check in set_ref_status_for_push().  But rejecting for not being\n>> a fast-forward suggests the situation can be resolved with a merge.\n>> Flag these updates (i.e., to a blob or a tree) as not forwardable so the\n>> user is presented with more appropriate advice.\n>>\n>> Signed-off-by: Chris Rorvick <chris@rorvick.com>\n>> ---\n>>  remote.c | 5 +++++\n>>  1 file changed, 5 insertions(+)\n>>\n>> diff --git a/remote.c b/remote.c\n>> index f5bc4e7..ee0c1e5 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref)\n>>       if (!o || o->type != OBJ_COMMIT)\n>>               return 0;\n>>\n>> +     /* new object must be commit-ish */\n>> +     o = deref_tag(parse_object(ref->new_sha1), NULL, 0);\n>> +     if (!o || o->type != OBJ_COMMIT)\n>> +             return 0;\n>> +\n>\n> I think the original code used ref_newer() which took commit-ish on\n> both sides.\n\nThat is still called later in set_ref_status_for_push() to calculate\nthe nonfastforward flag.  The only reason for even checking the new\nhere is to exclude trees and blobs now so they are flagged as\nalready-existing and thus avoid nonsensical fetch-and-merge advice.\nOtherwise the behavior is unchanged by this last patch.\n\nref_newer() does end up redoing computation now done in the new\nis_forwardable() function.  I could probably factor this out of\nref_newer() into a commit_newer() function that could be reused in\nset_ref_status_for_push() to avoid this overhead, but it didn't seem\nlike a big deal.  Thoughts?\n\n> With this code, the old must be a commit but new can be a tag that\n> points at a commit?  Why?\n\nThe old must not be a tag because fast-forwarding from it is\npotentially destructive; a tag would likely be left dangling in this\ncase.  This is not true for the new, though.   I'm not sure\nfast-forwarding from a commit to a tag is useful, but I didn't see a\nreason to prevent it either.   The refs/tags/ hierarchy is excluded\nfrom fast-forwarding before this check, and refs/heads/ is already\nprotected against anything but commits.  So it seems very unlikely\nthat someone would accidentally make use of this behavior.\n\nSo, fast-forwarding to a tag seemed fairly benign and unlikely to\ncause confusion, so I leaned towards allowing it in case someone found\na use case for it.\n"},{"id":"203965","messageId":"CAEUsAPZTycJS_USj-tYNN2Bpwn8XvYDTd4c7wFMFDYfNeSCUtw@mail.gmail.com","threadId":"32171","inReplyTo":"7vfw3wry4d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/7] push: require force for refs under refs/tags/","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-27T04:17:49Z","receivedAt":"2012-11-27T04:17:49Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Mon, Nov 26, 2012 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Chris Rorvick <chris@rorvick.com> writes:\n>\n>> diff --git a/remote.c b/remote.c\n>> index 4a6f822..012b52f 100644\n>> --- a/remote.c\n>> +++ b/remote.c\n>> @@ -1315,14 +1315,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n>>                *\n>>                * (1) if the old thing does not exist, it is OK.\n>>                *\n>> -              * (2) if you do not have the old thing, you are not allowed\n>> +              * (2) if the destination is under refs/tags/ you are\n>> +              *     not allowed to overwrite it; tags are expected\n>> +              *     to be static once created\n>> +              *\n>> +              * (3) if you do not have the old thing, you are not allowed\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>> +              * (4) if both new and old are commit-ish, and new is a\n>>                *     descendant of old, it is OK.\n>>                *\n>> -              * (4) regardless of all of the above, removing :B is\n>> +              * (5) regardless of all of the above, removing :B is\n>>                *     always allowed.\n>>                */\n>\n> We may want to reword (0) to make it clear that --force\n> (and +A:B) can be used to defeat all the other rules.\n\nOn that note, having (5) be \"regardless of all of the above ...\" seems\na little awkward.  That would seem to fit better towards the top.\n"},{"id":"203995","messageId":"7vboejnf73.fsf@alter.siamese.dyndns.org","threadId":"32171","inReplyTo":"CAEUsAPZq1POKQZd5vZy55nEj2kB4CqgxL9_d_0sQs33P_Gcemg@mail.gmail.com","subject":"Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-27T17:11:44Z","receivedAt":"2012-11-27T17:11:44Z","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>> With this code, the old must be a commit but new can be a tag that\n>> points at a commit?  Why?\n>\n> The old must not be a tag because fast-forwarding from it is\n> potentially destructive; a tag would likely be left dangling in this\n> case.  This is not true for the new, though.   I'm not sure\n> fast-forwarding from a commit to a tag is useful, but I didn't see a\n> reason to prevent it either.   The refs/tags/ hierarchy is excluded\n> from fast-forwarding before this check, and refs/heads/ is already\n> protected against anything but commits.  So it seems very unlikely\n> that someone would accidentally make use of this behavior.\n>\n> So, fast-forwarding to a tag seemed fairly benign and unlikely to\n> cause confusion, so I leaned towards allowing it in case someone found\n> a use case for it.\n\nSounds sensible.\n\nPerhaps some of that thinking behind this change (i.e. earlier\nwe checked the forwardee was any commit-ish, but the new code only\nallows a commit object if it were to be fast-forwarded) belongs to\nthe log message?\n\nThanks.\n"},{"id":"203996","messageId":"7v7gp7nf5e.fsf@alter.siamese.dyndns.org","threadId":"32171","inReplyTo":"CAEUsAPZTycJS_USj-tYNN2Bpwn8XvYDTd4c7wFMFDYfNeSCUtw@mail.gmail.com","subject":"Re: [PATCH 5/7] push: require force for refs under refs/tags/","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-27T17:12:45Z","receivedAt":"2012-11-27T17:12:45Z","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 26, 2012 at 12:57 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Chris Rorvick <chris@rorvick.com> writes:\n>>\n>>> diff --git a/remote.c b/remote.c\n>>> index 4a6f822..012b52f 100644\n>>> --- a/remote.c\n>>> +++ b/remote.c\n>>> @@ -1315,14 +1315,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n>>>                *\n>>>                * (1) if the old thing does not exist, it is OK.\n>>>                *\n>>> -              * (2) if you do not have the old thing, you are not allowed\n>>> +              * (2) if the destination is under refs/tags/ you are\n>>> +              *     not allowed to overwrite it; tags are expected\n>>> +              *     to be static once created\n>>> +              *\n>>> +              * (3) if you do not have the old thing, you are not allowed\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>>> +              * (4) if both new and old are commit-ish, and new is a\n>>>                *     descendant of old, it is OK.\n>>>                *\n>>> -              * (4) regardless of all of the above, removing :B is\n>>> +              * (5) regardless of all of the above, removing :B is\n>>>                *     always allowed.\n>>>                */\n>>\n>> We may want to reword (0) to make it clear that --force\n>> (and +A:B) can be used to defeat all the other rules.\n>\n> On that note, having (5) be \"regardless of all of the above ...\" seems\n> a little awkward.  That would seem to fit better towards the top.\n\nSure.  (0) you can always force; (1) you can always delete; and then\nother requirements.  That may indeed read better.\n\nThanks.\n"},{"id":"204134","messageId":"1354079933-2488-1-git-send-email-chris@rorvick.com","threadId":"32171","inReplyTo":"7v7gp7nf5e.fsf@alter.siamese.dyndns.org","subject":"[PATCH] push: cleanup push rules comment","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-28T05:18:53Z","receivedAt":"2012-11-28T05:18:53Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"---\n\nI ended up rewriting most of the comment.  The new version removes\ninter-rule dependencies (e.g., rule 5 overrides rule 3) which I think\nmakes it more readable.\n\nThis patch applies on top of the latest patch series regarding\npushing tags.  If will include this in a re-roll of that series if\nthese changes are deemed a good idea.\n\nAlso, I hand-edited the patch so that the changes were not interleaved\nto make it much easier to read.  Can this be done automatically?\nSomething like a minimum # of matching lines required between\ndifferences?\n\nChris\n\n remote.c | 32 +++++++++++++++++---------------\n 1 file changed, 17 insertions(+), 15 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex ee0c1e5..3fb1068 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1319,27 +1319,29 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\tcontinue;\n \t\t}\n \n-\t\t/* This part determines what can overwrite what.\n-\t\t * The rules are:\n-\t\t *\n-\t\t * (0) you can always use --force or +A:B notation to\n-\t\t *     selectively force individual ref pairs.\n-\t\t *\n-\t\t * (1) if the old thing does not exist, it is OK.\n-\t\t *\n-\t\t * (2) if the destination is under refs/tags/ you are\n-\t\t *     not allowed to overwrite it; tags are expected\n-\t\t *     to be static once created\n-\t\t *\n-\t\t * (3) if you do not have the old thing, you are not allowed\n-\t\t *     to overwrite it; you would not know what you are losing\n-\t\t *     otherwise.\n-\t\t *\n-\t\t * (4) if old is a commit and new is a descendant of old\n-\t\t *     (implying new is commit-ish), it is OK.\n-\t\t *\n-\t\t * (5) regardless of all of the above, removing :B is\n-\t\t *     always allowed.\n+\t\t/*\n+\t\t * The below logic determines whether an individual\n+\t\t * refspec A:B can be pushed.  The push will succeed\n+\t\t * if any of the following are true:\n+\t\t *\n+\t\t * (1) the remote reference B does not exist\n+\t\t *\n+\t\t * (2) the remote reference B is being removed (i.e.\n+\t\t *     pushing :B where no source is specified)\n+\t\t *\n+\t\t * (3) the update meets all fast-forwarding criteria:\n+\t\t *\n+\t\t *     (a) the destination is not under refs/tags/\n+\t\t *     (b) the old is a commit\n+\t\t *     (c) the new is a descendant of the old\n+\t\t *\n+\t\t *     NOTE: We must actually have the old object in\n+\t\t *     order to overwrite it in the remote reference,\n+\t\t *     and that the new object must be commit-ish.\n+\t\t *     These are implied by (b) and (c) respectively.\n+\t\t *\n+\t\t * (4) it is forced using the +A:B notation, or by\n+\t\t *     passing the --force argument\n \t\t */\n \n \t\tref->not_forwardable = !is_forwardable(ref);\n-- \n1.8.0.209.gf3828dc\n"},{"id":"204168","messageId":"7vip8phdfn.fsf@alter.siamese.dyndns.org","threadId":"32171","inReplyTo":"1354079933-2488-1-git-send-email-chris@rorvick.com","subject":"Re: [PATCH] push: cleanup push rules comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-28T16:58:36Z","receivedAt":"2012-11-28T16:58:36Z","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> ---\n>\n> I ended up rewriting most of the comment.  The new version removes\n> inter-rule dependencies (e.g., rule 5 overrides rule 3) which I think\n> makes it more readable.\n\nNice; thanks.\n"}]}