{"thread":{"id":"32247","subject":"[PATCH v6 4/8] push: flag updates that require force","startedAt":"2012-11-30T01:41:32Z","lastAt":"2013-01-25T05:14:41Z","messageCount":61,"participants":["Chris Rorvick","Junio C Hamano","Max Horn","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":6,"patchTotal":8},"messages":[{"id":"204314","messageId":"1354239700-3325-1-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":null,"subject":"[PATCH v6 0/8] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:32Z","receivedAt":"2012-11-30T01:41:32Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"This patch series originated in response to the following thread:\n\n http://thread.gmane.org/gmane.comp.version-control.git/208354\n\nI made some adjustments based on Junio's last round of feedback\nincluding a new patch reworking the \"push rules\" comment in remote.c.\nAlso refined some of the log messages--nothing major. Finally, took a\nstab at putting something together for the release notes, see below.\n\nChris\n\nRelease notes:\n\n\"git push\" no longer updates tags (lightweight or annotated) by default.\nSpecifically, if the destination reference already exists and is under\nrefs/tags/ or it points to a tag object, it is not allowed to fast-\nforward (unless forced using +A:B notation or by passing --force.) This\nis consistent with how a tag is normally thought of: a reference that\ndoes not move once defined. It also ensures a push will not\ninadvertently clobber an already existing tag--something that can go\nunnoticed if fast-forwarding is allowed.\n\nChris Rorvick (8):\n push: return reject reasons as a bitset\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 push: cleanup push rules comment\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 | 83 +++++++++++++++++++++++++++++++++++-----------\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, 167 insertions(+), 51 deletions(-)\n\n-- \n1.8.0.158.g0c4328c\n"},{"id":"204312","messageId":"1354239700-3325-2-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"[PATCH v6 1/8] push: return reject reasons as a bitset","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:33Z","receivedAt":"2012-11-30T01:41: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 \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..9d17fc7 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_reasons;\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_reasons);\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_reasons & REJECT_NON_FF_HEAD) {\n \t\tadvise_pull_before_push();\n-\t\tbreak;\n-\tcase NON_FF_OTHER:\n+\t} else if (reject_reasons & 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..9f98607 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_reasons;\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_reasons);\n \n \tif (!args.dry_run && remote) {\n \t\tstruct ref *ref;\ndiff --git a/transport.c b/transport.c\nindex 9932f40..d4568e7 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_reasons)\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_reasons = 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_reasons |= REJECT_NON_FF_HEAD;\n \t\t\telse\n-\t\t\t\t*nonfastforward = NON_FF_OTHER;\n+\t\t\t\t*reject_reasons |= 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_reasons)\n {\n-\t*nonfastforward = 0;\n+\t*reject_reasons = 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_reasons);\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..404b113 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_reasons);\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_reasons);\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.158.g0c4328c\n"},{"id":"204309","messageId":"1354239700-3325-3-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"[PATCH v6 2/8] push: add advice for rejected tag reference","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:34Z","receivedAt":"2012-11-30T01:41: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 are for branches, just\ntell the user the reference already exists.\n\nSigned-off-by: Chris Rorvick \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 9d17fc7..e08485d 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_reasons & 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 d4568e7..bc31e8e 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_reasons |= REJECT_ALREADY_EXISTS;\n \t\t\tif (!strcmp(head, ref->name))\n \t\t\t\t*reject_reasons |= REJECT_NON_FF_HEAD;\n \t\t\telse\ndiff --git a/transport.h b/transport.h\nindex 404b113..bfd2df5 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.158.g0c4328c\n"},{"id":"204315","messageId":"1354239700-3325-4-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"[PATCH v6 3/8] push: flag updates","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:35Z","receivedAt":"2012-11-30T01:41: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 it is not being removed, then\nmark as an update. This is in preparation for handling tags (lightweight\nand annotated) exceptionally.\n\nSigned-off-by: Chris Rorvick \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.158.g0c4328c\n"},{"id":"204307","messageId":"1354239700-3325-5-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"[PATCH v6 4/8] push: flag updates that require force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:36Z","receivedAt":"2012-11-30T01:41: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 is used for this when generating the\nstatus message. A separate flag insulates dependent logic from the\ndetails of set_ref_status_for_push().\n\nSigned-off-by: Chris Rorvick \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 bc31e8e..f3160b1 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.158.g0c4328c\n"},{"id":"204313","messageId":"1354239700-3325-6-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"[PATCH v6 5/8] push: require force for refs under refs/tags/","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:37Z","receivedAt":"2012-11-30T01:41: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 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 \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 `:` is omitted, the same ref as will be\n updated.\n +\n The object referenced by is used to update the reference\n-on the remote side, but by default this is only allowed if the\n-update can fast-forward . By having the optional leading `+`,\n-you can tell git to update the ref even when the update is not a\n-fast-forward. This does *not* attempt to merge into . See\n-EXAMPLES below for details.\n+on the remote side. By default this is only allowed if is not\n+under refs/tags/, and then only if it can fast-forward . By having\n+the optional leading `+`, you can tell git to update the 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 into . See EXAMPLES below for\n+details.\n +\n `tag ` means the same as `refs/tags/:refs/tags/`.\n +\ndiff --git a/builtin/push.c b/builtin/push.c\nindex e08485d..83a3cc8 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 9f98607..f849e0a 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 f3160b1..2673d27 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_reasons |= REJECT_ALREADY_EXISTS;\n \t\t\tif (!strcmp(head, ref->name))\n \t\t\t\t*reject_reasons |= REJECT_NON_FF_HEAD;\n \t\t\telse\n \t\t\t\t*reject_reasons |= REJECT_NON_FF_OTHER;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {\n+\t\t\t*reject_reasons |= REJECT_ALREADY_EXISTS;\n \t\t}\n \t}\n }\n-- \n1.8.0.158.g0c4328c\n"},{"id":"204310","messageId":"1354239700-3325-7-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"[PATCH v6 6/8] push: require force for annotated tags","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:38Z","receivedAt":"2012-11-30T01:41:38Z","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.\nUpdating from a tag is potentially destructive since it would likely\nleave the tag dangling. Disallowing updates to a tag also makes sense\nsemantically and is consistent with the behavior of lightweight tags.\n\nSigned-off-by: Chris Rorvick \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 is used to update the reference\n on the remote side. By default this is only allowed if is not\n-under refs/tags/, and then only if it can fast-forward . By having\n-the optional leading `+`, you can tell git to update the 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 into . See EXAMPLES below for\n-details.\n+a tag (annotated or lightweight), and then only if it can fast-forward\n+. By having the optional leading `+`, you can tell git to update\n+the ref even if it is not allowed by default (e.g., it is not a\n+fast-forward.) This does *not* attempt to merge into . See\n+EXAMPLES below for details.\n +\n `tag ` means the same as `refs/tags/:refs/tags/`.\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.158.g0c4328c\n"},{"id":"204308","messageId":"1354239700-3325-8-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"[PATCH v6 7/8] push: clarify rejection of update to non-commit-ish","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:39Z","receivedAt":"2012-11-30T01:41:39Z","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 to 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\nWhile updating *from* a tag object is potentially destructive, updating\n*to* a tag is not. Additionally, a push to the refs/tags/ hierarchy is\nalready excluded from fast-forwarding, and refs/heads/ is protected from\nanything but commit objects by a check in write_ref_sha1(). Thus\nsomeone fast-forwarding to a tag is probably not doing so by accident.\nSince updating to a tag is benign and unlikely to cause confusion, allow\nit in case someone finds the behavior useful.\n\nSigned-off-by: Chris Rorvick \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.158.g0c4328c\n"},{"id":"204311","messageId":"1354239700-3325-9-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"[PATCH v6 8/8] push: cleanup push rules comment","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-11-30T01:41:40Z","receivedAt":"2012-11-30T01:41:40Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Rewrite to remove inter-dependencies amongst the rules.\n\nSigned-off-by: Chris Rorvick \n---\n remote.c | 32 +++++++++++++++++---------------\n 1 file changed, 17 insertions(+), 15 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex ee0c1e5..6309a87 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 * 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 * (0) you can always use --force or +A:B notation to\n-\t\t * selectively force individual ref pairs.\n+\t\t * (1) the remote reference B does not exist\n \t\t *\n-\t\t * (1) if the old thing does not exist, it is OK.\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 * (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 * (3) the update meets all fast-forwarding criteria:\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 * (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 * (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 * 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 * (5) regardless of all of the above, removing :B is\n-\t\t * always allowed.\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.158.g0c4328c\n"},{"id":"204423","messageId":"7vmwxwka6f.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"1354239700-3325-3-git-send-email-chris@rorvick.com","subject":"Re: [PATCH v6 2/8] push: add advice for rejected tag reference","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-02T10:42:00Z","receivedAt":"2012-12-02T10:42:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Rorvick writes:\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_reasons & REJECT_ALREADY_EXISTS) {\n> +\t\tadvise_ref_already_exists();\n> \t}\n\nThe existing advise_* functions that are called from this function\nhonor the advice.* configuration, and advise_ref_already_exists()\nwould want to follow suit here (it is OK to do so as a follow-up\npatch without further rerolling the entire series).\n\nThanks.\n"},{"id":"204431","messageId":"1354481003-6704-1-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354239700-3325-9-git-send-email-chris@rorvick.com","subject":"[PATCH] remote.c: fix grammatical error in comment","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-12-02T20:43:23Z","receivedAt":"2012-12-02T20:43:23Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"The sentence originally began \"Note that ...\" and was changed to\n\"NOTE: ...\" This change should have been made at the same time.\n\nSigned-off-by: Chris Rorvick \n---\n\nThis applies to the current cr/push-force-tag-update branch. It can\nprobably just be folded into the last commit.\n\nThanks,\n\nChris\n\n remote.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 6309a87..aa6b719 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1337,8 +1337,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\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 * and the new object must be commit-ish. These are\n+\t\t * 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-- \n1.8.0.1.541.g73be2da\n"},{"id":"204436","messageId":"1354505271-25657-1-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"7vmwxwka6f.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] push: honor advice.* configuration","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-12-03T03:27:49Z","receivedAt":"2012-12-03T03:27:49Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Added a new config option to turn off the already-exists advice. We\nalso want to observe the 'pushNonFastForward' setting, but the name of\nthis config is too narrow after this addition. Renamed to have broader\nscope while retaining the old name as an alias for backward-\ncompatibility.\n\nChris Rorvick (2):\n push: rename config variable for more general use\n push: allow already-exists advice to be disabled\n\n Documentation/config.txt | 10 +++++++---\n advice.c | 9 +++++++--\n advice.h | 3 ++-\n builtin/push.c | 8 +++++---\n 4 files changed, 21 insertions(+), 9 deletions(-)\n\n-- \n1.8.0.1.541.g73be2da\n"},{"id":"204437","messageId":"1354505271-25657-2-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354505271-25657-1-git-send-email-chris@rorvick.com","subject":"[PATCH 1/2] push: rename config variable for more general use","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-12-03T03:27:50Z","receivedAt":"2012-12-03T03:27:50Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"The 'pushNonFastForward' advice config can be used to squelch several\ninstances of push-related advice. Rename it to 'pushUpdateRejected' to\ncover other reject scenarios that are unrelated to fast-forwarding.\nRetain the old name for compatibility.\n\nSigned-off-by: Chris Rorvick \n---\n Documentation/config.txt | 2 +-\n advice.c | 7 +++++--\n advice.h | 2 +-\n builtin/push.c | 6 +++---\n 4 files changed, 10 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 9a0544c..92903f2 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -140,7 +140,7 @@ advice.*::\n \tcan tell Git that you do not need help by setting these to 'false':\n +\n --\n-\tpushNonFastForward::\n+\tpushUpdateRejected::\n \t\tSet this variable to 'false' if you want to disable\n \t\t'pushNonFFCurrent', 'pushNonFFDefault', and\n \t\t'pushNonFFMatching' simultaneously.\ndiff --git a/advice.c b/advice.c\nindex edfbd4a..329e077 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -1,6 +1,6 @@\n #include \"cache.h\"\n \n-int advice_push_nonfastforward = 1;\n+int advice_push_update_rejected = 1;\n int advice_push_non_ff_current = 1;\n int advice_push_non_ff_default = 1;\n int advice_push_non_ff_matching = 1;\n@@ -14,7 +14,7 @@ static struct {\n \tconst char *name;\n \tint *preference;\n } advice_config[] = {\n-\t{ \"pushnonfastforward\", &advice_push_nonfastforward },\n+\t{ \"pushupdaterejected\", &advice_push_update_rejected },\n \t{ \"pushnonffcurrent\", &advice_push_non_ff_current },\n \t{ \"pushnonffdefault\", &advice_push_non_ff_default },\n \t{ \"pushnonffmatching\", &advice_push_non_ff_matching },\n@@ -23,6 +23,9 @@ static struct {\n \t{ \"resolveconflict\", &advice_resolve_conflict },\n \t{ \"implicitidentity\", &advice_implicit_identity },\n \t{ \"detachedhead\", &advice_detached_head },\n+\n+\t/* make this an alias for backward compatibility */\n+\t{ \"pushnonfastforward\", &advice_push_update_rejected }\n };\n \n void advise(const char *advice, ...)\ndiff --git a/advice.h b/advice.h\nindex f3cdbbf..c28ef8a 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -3,7 +3,7 @@\n \n #include \"git-compat-util.h\"\n \n-extern int advice_push_nonfastforward;\n+extern int advice_push_update_rejected;\n extern int advice_push_non_ff_current;\n extern int advice_push_non_ff_default;\n extern int advice_push_non_ff_matching;\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 83a3cc8..cf5ecfa 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -226,21 +226,21 @@ static const char message_advice_ref_already_exists[] =\n \n static void advise_pull_before_push(void)\n {\n-\tif (!advice_push_non_ff_current || !advice_push_nonfastforward)\n+\tif (!advice_push_non_ff_current || !advice_push_update_rejected)\n \t\treturn;\n \tadvise(_(message_advice_pull_before_push));\n }\n \n static void advise_use_upstream(void)\n {\n-\tif (!advice_push_non_ff_default || !advice_push_nonfastforward)\n+\tif (!advice_push_non_ff_default || !advice_push_update_rejected)\n \t\treturn;\n \tadvise(_(message_advice_use_upstream));\n }\n \n static void advise_checkout_pull_push(void)\n {\n-\tif (!advice_push_non_ff_matching || !advice_push_nonfastforward)\n+\tif (!advice_push_non_ff_matching || !advice_push_update_rejected)\n \t\treturn;\n \tadvise(_(message_advice_checkout_pull_push));\n }\n-- \n1.8.0.1.541.g73be2da\n"},{"id":"204438","messageId":"1354505271-25657-3-git-send-email-chris@rorvick.com","threadId":"32247","inReplyTo":"1354505271-25657-1-git-send-email-chris@rorvick.com","subject":"[PATCH 2/2] push: allow already-exists advice to be disabled","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2012-12-03T03:27:51Z","receivedAt":"2012-12-03T03:27:51Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"Add 'advice.pushAlreadyExists' option to disable the advice shown when\nan update is rejected for a reference that is not allowed to update at\nall (verses those that are allowed to fast-forward.)\n\nSigned-off-by: Chris Rorvick \n---\n Documentation/config.txt | 8 ++++++--\n advice.c | 2 ++\n advice.h | 1 +\n builtin/push.c | 2 ++\n 4 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 92903f2..90e7d10 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -142,8 +142,9 @@ advice.*::\n --\n \tpushUpdateRejected::\n \t\tSet this variable to 'false' if you want to disable\n-\t\t'pushNonFFCurrent', 'pushNonFFDefault', and\n-\t\t'pushNonFFMatching' simultaneously.\n+\t\t'pushNonFFCurrent', 'pushNonFFDefault',\n+\t\t'pushNonFFMatching', and 'pushAlreadyExists'\n+\t\tsimultaneously.\n \tpushNonFFCurrent::\n \t\tAdvice shown when linkgit:git-push[1] fails due to a\n \t\tnon-fast-forward update to the current branch.\n@@ -158,6 +159,9 @@ advice.*::\n \t\t'matching refs' explicitly (i.e. you used ':', or\n \t\tspecified a refspec that isn't your current branch) and\n \t\tit resulted in a non-fast-forward error.\n+\tpushAlreadyExists::\n+\t\tShown when linkgit:git-push[1] rejects an update that\n+\t\tdoes not qualify for fast-forwarding (e.g., a tag.)\n \tstatusHints::\n \t\tShow directions on how to proceed from the current\n \t\tstate in the output of linkgit:git-status[1] and in\ndiff --git a/advice.c b/advice.c\nindex 329e077..d287927 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -4,6 +4,7 @@ int advice_push_update_rejected = 1;\n int advice_push_non_ff_current = 1;\n int advice_push_non_ff_default = 1;\n int advice_push_non_ff_matching = 1;\n+int advice_push_already_exists = 1;\n int advice_status_hints = 1;\n int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n@@ -18,6 +19,7 @@ static struct {\n \t{ \"pushnonffcurrent\", &advice_push_non_ff_current },\n \t{ \"pushnonffdefault\", &advice_push_non_ff_default },\n \t{ \"pushnonffmatching\", &advice_push_non_ff_matching },\n+\t{ \"pushalreadyexists\", &advice_push_already_exists },\n \t{ \"statushints\", &advice_status_hints },\n \t{ \"commitbeforemerge\", &advice_commit_before_merge },\n \t{ \"resolveconflict\", &advice_resolve_conflict },\ndiff --git a/advice.h b/advice.h\nindex c28ef8a..8bf6356 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -7,6 +7,7 @@ extern int advice_push_update_rejected;\n extern int advice_push_non_ff_current;\n extern int advice_push_non_ff_default;\n extern int advice_push_non_ff_matching;\n+extern int advice_push_already_exists;\n extern int advice_status_hints;\n extern int advice_commit_before_merge;\n extern int advice_resolve_conflict;\ndiff --git a/builtin/push.c b/builtin/push.c\nindex cf5ecfa..8491e43 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -247,6 +247,8 @@ static void advise_checkout_pull_push(void)\n \n static void advise_ref_already_exists(void)\n {\n+\tif (!advice_push_already_exists || !advice_push_update_rejected)\n+\t\treturn;\n \tadvise(_(message_advice_ref_already_exists));\n }\n \n-- \n1.8.0.1.541.g73be2da\n"},{"id":"204460","messageId":"7vd2yrrmpr.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-03T18:53:52Z","receivedAt":"2012-12-03T18:53:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; will queue.\n"},{"id":"207052","messageId":"DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de","threadId":"32247","inReplyTo":"1354239700-3325-1-git-send-email-chris@rorvick.com","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Max Horn","fromEmail":"max@quendi.de","sentAt":"2013-01-16T13:32:03Z","receivedAt":"2013-01-16T13:32:03Z","isPatch":true,"sender":{"key":"max@quendi.de","avatar":"https://avatars.githubusercontent.com/u/241512?v=4"},"body":"Hi there,\n\nI was just working on improving git-remote-helper.txt by documenting how remote helper can signal error conditions to git. This lead me to notice a (to me) surprising change in behavior between master and next that I traced back to this patch series.\n\nSpecifically:\n\nOn 30.11.2012, at 02:41, Chris Rorvick wrote:\n\n> This patch series originated in response to the following thread:\n> \n> http://thread.gmane.org/gmane.comp.version-control.git/208354\n> \n> I made some adjustments based on Junio's last round of feedback\n> including a new patch reworking the \"push rules\" comment in remote.c.\n> Also refined some of the log messages--nothing major. Finally, took a\n> stab at putting something together for the release notes, see below.\n\n>From the discussion in that gmane thread and from the commits in this series, I had the impression that it should mostly affect pushing tags. However, this is not the case: It also changes messages upon regular push \"conflicts. Consider this test script:\n\n\n#!/bin/sh -ex\ngit init repo_orig\ncd repo_orig\necho a > a\ngit add a\ngit commit -m a\ncd ..\n\ngit clone repo_orig repo_clone\n\ncd repo_orig\necho b > b\ngit add b\ngit commit -m b\ncd ..\n\ncd repo_clone\necho B > b\ngit add b\ngit commit -m B\ngit push\n\n\nWith git 1.8.1, I get this message:\n\n ! [rejected] master -> master (non-fast-forward)\nerror: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'\nhint: Updates were rejected because the tip of your current branch is behind\nhint: its remote counterpart. Merge the remote changes (e.g. 'git pull')\nhint: before pushing again.\nhint: See the 'Note about fast-forwards' in 'git push --help' for details.\n\n\n\nBut with next, I get this:\n\n\n ! [rejected] master -> master (already exists)\nerror: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'\nhint: Updates were rejected because the destination reference already exists\nhint: in the remote.\n\n\nThis looks like a regression to me. No tags were involve, and the new message is very confusing if not outright wrong -- at least in my mind, but perhaps I am missing a way to interpret it \"correctly\" ? What am I missing?\n\n\nCheers,\nMax\n"},{"id":"207068","messageId":"7vwqvdxgmo.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T16:00:15Z","receivedAt":"2013-01-16T16:00:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn writes:\n\n> But with next, I get this:\n>\n> ! [rejected] master -> master (already exists)\n> error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'\n> hint: Updates were rejected because the destination reference already exists\n> hint: in the remote.\n>\n> This looks like a regression to me.\n\nIt is in master now X-<, and this looks like a bug to me.\n"},{"id":"207067","messageId":"20130116160131.GB22400@sigill.intra.peff.net","threadId":"32247","inReplyTo":"DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-16T16:01:31Z","receivedAt":"2013-01-16T16:01:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 02:32:03PM +0100, Max Horn wrote:\n\n> With git 1.8.1, I get this message:\n> \n> ! [rejected] master -> master (non-fast-forward)\n> [...]\n> But with next, I get this:\n> \n> ! [rejected] master -> master (already exists)\n\nThanks for the detailed report. I was able to reproduce easily here.\n\nThe problem is the logic in is_forwardable:\n\nstatic inline int is_forwardable(struct ref* ref)\n{\n struct object *o;\n\n if (!prefixcmp(ref->name, \"refs/tags/\"))\n return 0;\n\n /* old object must be a commit */\n o = parse_object(ref->old_sha1);\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 return 1;\n}\n\nThe intent is to allow fast-forward only between objects that both point\nto commits eventually. But we are doing this check on the client, which\ndoes not necessarily have the object for ref->old_sha1 at all. So it\ncannot know the type, and cannot enforce this condition accurately.\n\nI.e., we trigger the \"!o\" branch after the parse_object in your example.\n\n-Peff\n"},{"id":"207063","messageId":"7vsj61xez2.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T16:36:01Z","receivedAt":"2013-01-16T16:36:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Horn writes:\n\n> But with next, I get this:\n>\n>\n> ! [rejected] master -> master (already exists)\n> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'\n> hint: Updates were rejected because the destination reference already exists\n> hint: in the remote.\n>\n> This looks like a regression to me.\n\nIt is an outright bug. The new helper function is_forwrdable() is\nbogus to assume that both original and updated objects can be\nlocally inspected, but you do not necessarily have the original\nlocally.\n\n remote.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex aa6b719..4a253ef 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1286,9 +1286,12 @@ static inline int is_forwardable(struct ref* ref)\n \tif (!prefixcmp(ref->name, \"refs/tags/\"))\n \t\treturn 0;\n \n-\t/* old object must be a commit */\n+\t/*\n+\t * old object must be a commit, but we may be forcing\n+\t * without having it in the first place!\n+\t */\n \to = parse_object(ref->old_sha1);\n-\tif (!o || o->type != OBJ_COMMIT)\n+\tif (o && o->type != OBJ_COMMIT)\n \t\treturn 0;\n \n \t/* new object must be commit-ish */\n"},{"id":"207065","messageId":"7vobgpxeel.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"7vsj61xez2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T16:48:18Z","receivedAt":"2013-01-16T16:48:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Max Horn writes:\n>\n>> But with next, I get this:\n>>\n>> ! [rejected] master -> master (already exists)\n>> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'\n>> hint: Updates were rejected because the destination reference already exists\n>> hint: in the remote.\n>>\n>> This looks like a regression to me.\n>\n> It is an outright bug. The new helper function is_forwrdable() is\n> bogus to assume that both original and updated objects can be\n> locally inspected, but you do not necessarily have the original\n> locally.\n\nThe way the caller uses the result of this function is equally\nquestionable. If this function says \"we do not want to let this\npush go through\", it translates that unconditionally into \"we\nblocked it because the destination already exists\".\n\nIt is fine when pushing into \"refs/tags/\" hierarchy. It is *NOT*\nOK if the type check does not satisfy this function. In that case,\nwe do not actually see the existence of the destination as a\nproblem, but it is reported as such. We are blocking because we do\nnot like the type of the new object or the type of the old object.\nIf the destination points at a commit, the push can succeed if the\nuser changes what object to push, so saying \"you cannot push because\nthe destination already exists\" is just wrong in such a case.\n"},{"id":"207069","messageId":"7vfw21xde5.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"20130116160131.GB22400@sigill.intra.peff.net","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T17:10:10Z","receivedAt":"2013-01-16T17:10:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I.e., we trigger the \"!o\" branch after the parse_object in your example.\n\nHeh, I didn't see this message until now (gmane seems to be lagging\na bit).\n\nI am very tempted to do this.\n\n * Remove unnecessary not_forwardable from \"struct ref\"; it is only\n used inside set_ref_status_for_push();\n\n * \"refs/tags/\" is the only hierarchy that cannot be replaced\n without --force;\n\n * Remove the misguided attempt to force that everything that\n updates an existing ref has to be a commit outside \"refs/tags/\"\n hierarchy. This code does not know what kind of objects the user\n wants to place in \"refs/frotz/\" hierarchy it knows nothing about.\n\nI feel moderately strongly about the last point. Defining special\nsemantics for one hierarchy (e.g. \"refs/tags/\") and implementing a\npolicy for enforcement is one thing, but a random policy that\ndepends on object type that applies globally is simply insane. The\nuser may want to do \"refs/tested/\" hierarchy that is meant to hold\nreferences to commit, with one annotated tag \"refs/tested/latest\"\nthat points at the \"latest tested version\" with some commentary, and\nmaintain the latter by keep pushing to it. If that is the semantics\nthe user wanted to ahve in the \"refs/tested/\" hierarchy, it is not\nreasonable to require --force for such a workflow. The user knows\nbetter than Git in such a case.\n\n\n cache.h | 1 -\n remote.c | 24 +-----------------------\n t/t5516-fetch-push.sh | 21 ---------------------\n 3 files changed, 1 insertion(+), 45 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a32a0ea..a942bbd 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1004,7 +1004,6 @@ struct ref {\n \t\trequires_force:1,\n \t\tmerge:1,\n \t\tnonfastforward:1,\n-\t\tnot_forwardable:1,\n \t\tupdate:1,\n \t\tdeletion:1;\n \tenum {\ndiff --git a/remote.c b/remote.c\nindex aa6b719..2c747c4 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1279,26 +1279,6 @@ 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-\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-\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 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \tint force_update)\n {\n@@ -1344,8 +1324,6 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t * passing the --force argument\n \t\t */\n \n-\t\tref->not_forwardable = !is_forwardable(ref);\n-\n \t\tref->update =\n \t\t\t!ref->deletion &&\n \t\t\t!is_null_sha1(ref->old_sha1);\n@@ -1355,7 +1333,7 @@ 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->not_forwardable) {\n+\t\t\tif (!prefixcmp(ref->name, \"refs/tags/\")) {\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;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 6009372..8f024a0 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -950,27 +950,6 @@ 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"},{"id":"207075","messageId":"20130116174325.GA27525@sigill.intra.peff.net","threadId":"32247","inReplyTo":"7vfw21xde5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-16T17:43:25Z","receivedAt":"2013-01-16T17:43:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I.e., we trigger the \"!o\" branch after the parse_object in your example.\n> \n> Heh, I didn't see this message until now (gmane seems to be lagging\n> a bit).\n\nI think it is vger lagging, actually.\n\n> I am very tempted to do this.\n> \n> * Remove unnecessary not_forwardable from \"struct ref\"; it is only\n> used inside set_ref_status_for_push();\n> \n> * \"refs/tags/\" is the only hierarchy that cannot be replaced\n> without --force;\n\nAgreed.\n\n> * Remove the misguided attempt to force that everything that\n> updates an existing ref has to be a commit outside \"refs/tags/\"\n> hierarchy. This code does not know what kind of objects the user\n> wants to place in \"refs/frotz/\" hierarchy it knows nothing about.\n\nI agree with what your patch does, but my thinking is a bit different.\n\nMy original suggestion with respect to object types was that the rule\nfor --force should be \"do not ever lose any objects without --force\". So\na fast-forward is OK, as the new objects reference the old. A non-fast\nforward is not, because objects become unreferenced. Replacing a tag\nobject is not OK, even if it points to the same commit, as you are\nlosing the old tag object (replacing an object with a tag that points to\nthe original object or its descendent is OK in theory, though I doubt it\nis common enough to worry about).\n\nI think that is a reasonable rule that could be applied across all parts\nof the namespace hierarchy. And it could be applied by the client,\nbecause all you need to know is whether ref->old_sha1 is reachable from\nref->new_sha1.\n\nBut it is somewhat orthogonal to the \"already exists\" idea, and checking\nrefs/tags/. Those ideas are about enforcing sane rules on the tag\nhierarchy. My rule is a safety valve that is meant to extend the idea of\n\"is fast-forwardable\" to non-commit object types. If we do it at all, it\nshould be part of the fast-forward check (e.g., as part of ref_newer).\n\nThe current code conflates the two under the \"already exists\" condition,\nwhich is just wrong. I think the best thing at this point is to split\nthe two ideas apart, keep the refs/tags check (and translate it to\n\"already exists\" in the UI, as we do), and table the safety valve. I am\nnot even sure if it is something that is useful, and it can come later\nif we decide it is.\n\n> I feel moderately strongly about the last point. Defining special\n> semantics for one hierarchy (e.g. \"refs/tags/\") and implementing a\n> policy for enforcement is one thing, but a random policy that\n> depends on object type that applies globally is simply insane. The\n> user may want to do \"refs/tested/\" hierarchy that is meant to hold\n> references to commit, with one annotated tag \"refs/tested/latest\"\n> that points at the \"latest tested version\" with some commentary, and\n> maintain the latter by keep pushing to it. If that is the semantics\n> the user wanted to ahve in the \"refs/tested/\" hierarchy, it is not\n> reasonable to require --force for such a workflow. The user knows\n> better than Git in such a case.\n\nI see what you are saying, but I think the ship has already sailed to\nsome degree. We already implement the non-fast-forward check everywhere,\nand I cannot have a \"refs/tested\" hierarchy that pushes arbitrary\ncommits without regard to their history. If I have such a hierarchy, I\nhave to use \"--force\" (or more likely, mark the refspec with \"+\").\n\nIn my mind, the object-type checking is just making that fast-forward\ncheck more thorough (i.e., extending it to non-commit objects).\n\n> cache.h | 1 -\n> remote.c | 24 +-----------------------\n> t/t5516-fetch-push.sh | 21 ---------------------\n> 3 files changed, 1 insertion(+), 45 deletions(-)\n\nThe patch itself looks fine to me. Whether we agree on the fast-forward\nobject-type checking or not, it is the correct first step to take in\neither case.\n\n-Peff\n"},{"id":"207107","messageId":"7va9s8x2n0.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"20130116174325.GA27525@sigill.intra.peff.net","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-16T21:02:27Z","receivedAt":"2013-01-16T21:02:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I see what you are saying, but I think the ship has already sailed to\n> some degree. We already implement the non-fast-forward check everywhere,\n> and I cannot have a \"refs/tested\" hierarchy that pushes arbitrary\n> commits without regard to their history. If I have such a hierarchy, I\n> have to use \"--force\" (or more likely, mark the refspec with \"+\").\n\nYeah, actually in that example, I meant refs/tested/ would have\npointers to bare tree objects. I often rebuild 'pu' and another\nprivate integration branch for testing, reordering the series that\nare still not in 'next' and also after rewriting log messages of\nsome commits. It is not unusual to end up the updated 'pu' having\nthe identical tree as 'pu' before update, and I want to skip testing\nthe result (tree equality matters while commit equality does not in\nsuch a use case).\n\n> In my mind, the object-type checking is just making that fast-forward\n> check more thorough (i.e., extending it to non-commit objects).\n\nYes, I agree with that point of view.\n\nThanks.\n\nHere is what I am planning to queue (the patch is the same, but the\nmessage is different on the third point). \n\n-- >8 --\nSubject: [PATCH] push: fix \"refs/tags/ hierarchy cannot be updated without --force\"\n\nWhen pushing to update a branch with a commit that is not a\ndescendant of the commit at the tip, a wrong message \"already\nexists\" was given, instead of the correct \"non-fast-forward\", if we\ndo not have the object sitting in the destination repository at the\ntip of the ref we are updating.\n\nThe primary cause of the bug is that the check in a new helper\nfunction is_forwardable() assumed both old and new objects are\navailable and can be checked, which is not always the case.\n\nThe way the caller uses the result of this function is also wrong.\nIf the helper says \"we do not want to let this push go through\", the\ncaller unconditionally translates it into \"we blocked it because the\ndestination already exists\", which is not true at all in this case.\n\nFix this by doing these three things:\n\n * Remove unnecessary not_forwardable from \"struct ref\"; it is only\n used inside set_ref_status_for_push();\n\n * Make \"refs/tags/\" the only hierarchy that cannot be replaced\n without --force;\n\n * Remove the misguided attempt to force that everything that\n updates an existing ref has to be a commit outside \"refs/tags/\"\n hierarchy.\n\nThe policy last one tried to implement may later be resurrected and\nextended to ensure fast-forwardness (defined as \"not losing\nobjects\", extending from the traditional \"not losing commits from\nthe resulting history\") when objects that are not commit are\ninvolved (e.g. an annotated tag in hierarchies outside refs/tags),\nbut such a logic belongs to \"is this a fast-forward?\" check that is\ndone by ref_newer(); is_forwardable(), which is now removed, was not\nthe right place to do so.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 1 -\n remote.c | 43 +++++++------------------------------------\n t/t5516-fetch-push.sh | 21 ---------------------\n 3 files changed, 7 insertions(+), 58 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a32a0ea..a942bbd 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1004,7 +1004,6 @@ struct ref {\n \t\trequires_force:1,\n \t\tmerge:1,\n \t\tnonfastforward:1,\n-\t\tnot_forwardable:1,\n \t\tupdate:1,\n \t\tdeletion:1;\n \tenum {\ndiff --git a/remote.c b/remote.c\nindex aa6b719..d3a1ca2 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1279,26 +1279,6 @@ 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-\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-\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 void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \tint force_update)\n {\n@@ -1320,32 +1300,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t}\n \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 * Decide whether an individual refspec A:B can be\n+\t\t * pushed. The push will succeed if any of the\n+\t\t * 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 the new object must be commit-ish. These are\n-\t\t * implied by (b) and (c) respectively.\n+\t\t * (3) the destination is not under refs/tags/, and\n+\t\t * if the old and new value is a commit, the new\n+\t\t * is a descendant of the old.\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-\n \t\tref->update =\n \t\t\t!ref->deletion &&\n \t\t\t!is_null_sha1(ref->old_sha1);\n@@ -1355,7 +1326,7 @@ 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->not_forwardable) {\n+\t\t\tif (!prefixcmp(ref->name, \"refs/tags/\")) {\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;\ndiff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh\nindex 6009372..8f024a0 100755\n--- a/t/t5516-fetch-push.sh\n+++ b/t/t5516-fetch-push.sh\n@@ -950,27 +950,6 @@ 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.1.1.426.g616047d\n"},{"id":"207130","messageId":"CAEUsAPY8T9TYCrZLWB-0Mwae_NtnqqVvGwY+4jGfqh5Lh3=Dgw@mail.gmail.com","threadId":"32247","inReplyTo":"20130116174325.GA27525@sigill.intra.peff.net","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-17T02:19:28Z","receivedAt":"2013-01-17T02:19:28Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Wed, Jan 16, 2013 at 11:43 AM, Jeff King wrote:\n> I think that is a reasonable rule that could be applied across all parts\n> of the namespace hierarchy. And it could be applied by the client,\n> because all you need to know is whether ref->old_sha1 is reachable from\n> ref->new_sha1.\n\nis_forwardable() did solve a UI issue. Previously all instances where\nold is not reachable by new were assumed to be addressable with a\nmerge. is_forwardable() attempted to determine if the concept of\nforwarding made sense given the inputs. For example, if old is a blob\nit is useless to suggest merging it.\n\nChris\n"},{"id":"207133","messageId":"20130117031100.GA7264@sigill.intra.peff.net","threadId":"32247","inReplyTo":"CAEUsAPY8T9TYCrZLWB-0Mwae_NtnqqVvGwY+4jGfqh5Lh3=Dgw@mail.gmail.com","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-17T03:11:00Z","receivedAt":"2013-01-17T03:11:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 16, 2013 at 08:19:28PM -0600, Chris Rorvick wrote:\n\n> On Wed, Jan 16, 2013 at 11:43 AM, Jeff King wrote:\n> > I think that is a reasonable rule that could be applied across all parts\n> > of the namespace hierarchy. And it could be applied by the client,\n> > because all you need to know is whether ref->old_sha1 is reachable from\n> > ref->new_sha1.\n> \n> is_forwardable() did solve a UI issue. Previously all instances where\n> old is not reachable by new were assumed to be addressable with a\n> merge. is_forwardable() attempted to determine if the concept of\n> forwarding made sense given the inputs. For example, if old is a blob\n> it is useless to suggest merging it.\n\nI think it makes sense to mark such a case as different from a regular\nnon-fast-forward (because \"git pull\" is not the right advice), but:\n\n 1. is_forwardable should assume a missing object is a commit not to\n regress the common case; otherwise we do not show the pull advice\n when we probably should, and most of the time it is going to be a\n commit\n\n 2. When we know that we are not working with commits, I am not sure\n that \"already exists\" is the right advice to give for such a case.\n It is neither \"this tag already exists, so we do not update it\",\n nor is it strictly \"cannot fast forward this commit\", but rather\n something else.\n\n The expanded definition of \"what is a fast forward\" that I\n suggested would let this fall naturally between the two.\n\n-Peff\n"},{"id":"207134","messageId":"CAEUsAPZZ7YNCBRZRj1e1ivDL__u_9v=cBJvF0sKcKWjJ1fPRTQ@mail.gmail.com","threadId":"32247","inReplyTo":"20130117031100.GA7264@sigill.intra.peff.net","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-17T03:42:55Z","receivedAt":"2013-01-17T03:42:55Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Wed, Jan 16, 2013 at 9:11 PM, Jeff King wrote:\n>> is_forwardable() did solve a UI issue. Previously all instances where\n>> old is not reachable by new were assumed to be addressable with a\n>> merge. is_forwardable() attempted to determine if the concept of\n>> forwarding made sense given the inputs. For example, if old is a blob\n>> it is useless to suggest merging it.\n>\n> I think it makes sense to mark such a case as different from a regular\n> non-fast-forward (because \"git pull\" is not the right advice), but:\n>\n> 1. is_forwardable should assume a missing object is a commit not to\n> regress the common case; otherwise we do not show the pull advice\n> when we probably should, and most of the time it is going to be a\n> commit\n\nYes, obviously this was a bug, thus the use of \"attempted\" above. It\nwould have been better to assume a missing 'old' was potentially\nforwardable to present the user with the most helpful advice.\n\n> 2. When we know that we are not working with commits, I am not sure\n> that \"already exists\" is the right advice to give for such a case.\n> It is neither \"this tag already exists, so we do not update it\",\n> nor is it strictly \"cannot fast forward this commit\", but rather\n> something else.\n\nBut the reference already existing in the remote is a substantial\nreason for not allowing the push in all of these cases. You can break\nthis out further if you like to explain why the specific reference\nshouldn't be moved on the remote, but this is even more complicated a\nsimple \"is old reachable from new?\" test.\n\nChris\n"},{"id":"207137","messageId":"CAEUsAPb0Zg0x78e+12NqXA4PRBkOUO89KTgxtwxujS1KOx9NYg@mail.gmail.com","threadId":"32247","inReplyTo":"7vobgpxeel.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-17T06:20:13Z","receivedAt":"2013-01-17T06:20:13Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano wrote:\n> It is fine when pushing into \"refs/tags/\" hierarchy. It is *NOT*\n> OK if the type check does not satisfy this function. In that case,\n> we do not actually see the existence of the destination as a\n> problem, but it is reported as such. We are blocking because we do\n> not like the type of the new object or the type of the old object.\n> If the destination points at a commit, the push can succeed if the\n> user changes what object to push, so saying \"you cannot push because\n> the destination already exists\" is just wrong in such a case.\n\nSo the solution is to revert back to recommending a merge?\n"},{"id":"207138","messageId":"7vehhkuwg5.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"CAEUsAPb0Zg0x78e+12NqXA4PRBkOUO89KTgxtwxujS1KOx9NYg@mail.gmail.com","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-17T06:59:06Z","receivedAt":"2013-01-17T06:59:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Rorvick writes:\n\n> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano wrote:\n>> It is fine when pushing into \"refs/tags/\" hierarchy. It is *NOT*\n>> OK if the type check does not satisfy this function. In that case,\n>> we do not actually see the existence of the destination as a\n>> problem, but it is reported as such. We are blocking because we do\n>> not like the type of the new object or the type of the old object.\n>> If the destination points at a commit, the push can succeed if the\n>> user changes what object to push, so saying \"you cannot push because\n>> the destination already exists\" is just wrong in such a case.\n>\n> So the solution is to revert back to recommending a merge?\n\nOf course not, because at that point you may not even have what you\nwere attempting to overwrite. Nobody says it is even something you\ncould merge.\n\nThe recommended solution certainly will involve a \"fetch\" (not\n\"pull\" or \"pull --rebase\"). You fetch from over there to check what\nyou were about to overwrite, examine the situation to decide what\nthe appropriate action is.\n\nThe point is that Git in general, and the codepath that was touched\nby the patch in particular, does not have enough information to\ndecide what the appropriate action is for the user, especially when\nthe ref is outside the ones we know what the conventional uses of\nthem are. We can make policy decisions like \"tags are meant to be\nunmoving anchor points, so it is unusual to overwrite any old with\nany new\", \"heads are meant to be branch tips, and because rewinding\nthem while more than one repositories are working with them will\ncause issues to other repositories, it is unusual to push a\nnon-fast-forward\" and enforcement mechanism for such policy\ndecisions will help users, but that is only because we know what\ntheir uses are.\n\nThe immediate action we should take is to get closer to the original\nbehaviour of not complaining with \"ref already exists\", which is\nnonsensical. That does not mean that we will forbid improving the\ncodepath by giving different advices depending on the case.\n\nOne of the new advices could tell them to \"fetch it and inspect the\nsituation\", if old is not something we do not even have (hence we\ncannot check its type, let alone the ancestry relationship of it\nwith new), for example.\n"},{"id":"207144","messageId":"CAEUsAPYAL6TD_nzu-YumRK_b-kFy7mNz1VivmSxGeuFYVxVL4g@mail.gmail.com","threadId":"32247","inReplyTo":"7vehhkuwg5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-17T13:09:16Z","receivedAt":"2013-01-17T13:09:16Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Thu, Jan 17, 2013 at 12:59 AM, Junio C Hamano wrote:\n> Chris Rorvick writes:\n>\n>> On Wed, Jan 16, 2013 at 10:48 AM, Junio C Hamano wrote:\n>>> It is fine when pushing into \"refs/tags/\" hierarchy. It is *NOT*\n>>> OK if the type check does not satisfy this function. In that case,\n>>> we do not actually see the existence of the destination as a\n>>> problem, but it is reported as such. We are blocking because we do\n>>> not like the type of the new object or the type of the old object.\n>>> If the destination points at a commit, the push can succeed if the\n>>> user changes what object to push, so saying \"you cannot push because\n>>> the destination already exists\" is just wrong in such a case.\n>>\n>> So the solution is to revert back to recommending a merge?\n>\n> Of course not, because at that point you may not even have what you\n> were attempting to overwrite. Nobody says it is even something you\n> could merge.\n\nI was referring to your concern about rejecting based on type. A push\ncausing a reference to move (for example) from a commit to a blob is\nrejected as \"already exists\" with this patch. You emphatically state\nthis is not OK and your solution is to revert back to behavior that\nadvises a merge.\n\nClearly the bug regarding an 'old' unknown to the client should be\nfixed. This is a obvious test case I should have covered and it's\nunfortunate it made it into master. But I don't understand why\nis_forwardable() was misguided (maybe poorly named) nor why\nref_newer() is a better place to solve the issues it was addressing.\n\nChris\n"},{"id":"207188","messageId":"20130118010638.GA29453@sigill.intra.peff.net","threadId":"32247","inReplyTo":"CAEUsAPYAL6TD_nzu-YumRK_b-kFy7mNz1VivmSxGeuFYVxVL4g@mail.gmail.com","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-18T01:06:38Z","receivedAt":"2013-01-18T01:06:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 17, 2013 at 07:09:16AM -0600, Chris Rorvick wrote:\n\n> I was referring to your concern about rejecting based on type. A push\n> causing a reference to move (for example) from a commit to a blob is\n> rejected as \"already exists\" with this patch. You emphatically state\n> this is not OK and your solution is to revert back to behavior that\n> advises a merge.\n> \n> Clearly the bug regarding an 'old' unknown to the client should be\n> fixed. This is a obvious test case I should have covered and it's\n> unfortunate it made it into master. But I don't understand why\n> is_forwardable() was misguided (maybe poorly named) nor why\n> ref_newer() is a better place to solve the issues it was addressing.\n\nI think that a type-based rule that relies on knowing the type of the\nother side will always have to guess in some cases, because we do not\nnecessarily have that information. However, if instead of the rule being\n\"blobs on the remote side cannot be replaced\", if it becomes \"the old\nvalue on the remote side must be referenced by what we replace it with\",\nthat _is_ something we can calculate reliably on the sending side. And\nthat is logically an extension of the fast-forward rule, which is why I\nsuggested placing it with ref_newer (but the latter should probably be\nextended to not suggest merging if we _know_ it is a non-commit object).\n\n-Peff\n"},{"id":"207189","messageId":"CAEUsAPZr+bNNA-pqrQbBGvku4T3h58Ub66mK2zLeHqghEKw5Aw@mail.gmail.com","threadId":"32247","inReplyTo":"20130118010638.GA29453@sigill.intra.peff.net","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-18T03:18:50Z","receivedAt":"2013-01-18T03:18:50Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Thu, Jan 17, 2013 at 7:06 PM, Jeff King wrote:\n> However, if instead of the rule being\n> \"blobs on the remote side cannot be replaced\", if it becomes \"the old\n> value on the remote side must be referenced by what we replace it with\",\n> that _is_ something we can calculate reliably on the sending side.\n\nInteresting. I would have thought knowing reachability implied having\nthe old object in the sending repository.\n\n> And\n> that is logically an extension of the fast-forward rule, which is why I\n> suggested placing it with ref_newer (but the latter should probably be\n> extended to not suggest merging if we _know_ it is a non-commit object).\n\nSounds great, especially if it is not dependent on the sender actually\nhaving the old object. Until this is implemented, though, I don't\nunderstand what was wrong with doing the checks in the\nis_forwardable() helper function (of course after fixing the\nregression/bug.)\n\nChris\n"},{"id":"207193","messageId":"7vfw1zrttq.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"20130118010638.GA29453@sigill.intra.peff.net","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-18T04:36:17Z","receivedAt":"2013-01-18T04:36:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> However, if instead of the rule being \"blobs on the remote side\n> cannot be replaced\", if it becomes \"the old value on the remote\n> side must be referenced by what we replace it with\", that _is_\n> something we can calculate reliably on the sending side. And that\n> is logically an extension of the fast-forward rule,...\n\nIt may be an extension of the fast-forward, but only in the graph\nreachability sense. I can buy that it is mathmatically consistent\nwith the mode that has proven to be useful for commits at the branch\ntips, which we know why \"fast-forward\" rule is an appropriate\ndefault for. You haven't shown if that mathmatical consistency is\nuseful for non-commit case.\n\n\nThe primary reason \"fast-forward\" is a good default for branches is\nnot that \"we do not want to lose objects to gc\" (you have reflog for\nthat). The reason is non fast-forward is a sign of unintended\nrewind, and later will cause duplicated history with merge\nconflicts.\n\nThat comes from the way objects pointed by refs/heads aka branches\nare used. It is not just \"commit\" (as object type), but how these\nobjects are used. Think why we decided it was a good idea to do one\nthing in the topic that introduced the regression under discussion:\n\"Even if the new commit is a descendant of the old commit, we do not\nwant to fast-forward a ref if it is under refs/tags/\". Type of object\nmay be one factor, but how it is used is more important factor in\ndeciding what kind of policy is appropriate.\n\nIf users have workflows that want to have a ref hierarchy that point\nat a blob, there will not be any update to such a ref that will\nsatisfy your definition of \"extended\" fast-forward requirement, and\nthat requirement came solely from mathematical purity (i.e. graph\nreachability), not from any workflow consideration. That is very\ndisturbing to me.\n\nA workflow that employes such a \"blob at a ref\" may perfectly be\nhappy with replacing the blob as last-one-wins basis. I do not think\nthe client side should enforce a policy to forbid such a push.\n\nI personally think the current client side that insists that updates\nto any ref has to have the current object and must fast-forward and\nrequires --force otherwise was a mistake (this predates the change\nby Chris). The receiving end does not implement such an arbitrary\nrestriction outside refs/heads/, and does so only for refs/heads/\nonly when deny-non-fast-forwards is set.\n"},{"id":"207455","messageId":"20130121234002.GE17156@sigill.intra.peff.net","threadId":"32247","inReplyTo":"CAEUsAPZr+bNNA-pqrQbBGvku4T3h58Ub66mK2zLeHqghEKw5Aw@mail.gmail.com","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-21T23:40:02Z","receivedAt":"2013-01-21T23:40:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:\n\n> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King wrote:\n> > However, if instead of the rule being\n> > \"blobs on the remote side cannot be replaced\", if it becomes \"the old\n> > value on the remote side must be referenced by what we replace it with\",\n> > that _is_ something we can calculate reliably on the sending side.\n> \n> Interesting. I would have thought knowing reachability implied having\n> the old object in the sending repository.\n\nNo, because if you do not have it, then you know it is not reachable\nfrom your refs (or your repository is corrupted). If you do have it, it\n_might_ be reachable. For commits, checking is cheap (merge-base) and we\nalready do it. For trees and blobs, it is much more expensive, as you\nhave to walk the whole object graph. While it might be \"more correct\"\nin some sense to say \"it's OK to replace a tree with a commit that\npoints to it\", in practice I doubt anyone cares, so you can probably\njust punt on those ones and say \"no, it's not a fast forward\".\n\n> > And\n> > that is logically an extension of the fast-forward rule, which is why I\n> > suggested placing it with ref_newer (but the latter should probably be\n> > extended to not suggest merging if we _know_ it is a non-commit object).\n> \n> Sounds great, especially if it is not dependent on the sender actually\n> having the old object. Until this is implemented, though, I don't\n> understand what was wrong with doing the checks in the\n> is_forwardable() helper function (of course after fixing the\n> regression/bug.)\n\nI don't think it is wrong per se; I just think that the check would go\nmore naturally where we are checking whether the object does indeed\nfast-forward. Because is_forwardable in some cases must say \"I don't\nknow; I don't have the object to check its type, so maybe it is\nforwardable, and maybe it is not\". Whereas when we do the actual\nreachability check, we can say definitely \"this is not reachable because\nI don't have it, or this is not reachable because it is a commit and I\nchecked, or this might be reachable but I don't care to check because it\nhas a funny type\".\n\nI think looking at it as the latter makes it more obvious how to handle\nthe \"maybe\" situation (e.g., the bug in is_forwardable was hard to see).\n\nAnyway, I do not care that much where it goes. To me, the important\nthing is the error message. I do think the error \"already exists\" is a\nreasonable one for refs/tags (we do not allow non-force pushes of\nexisting tags), but not necessarily for other cases, like trying to push\na blob over a blob. The problem there is not \"already exists\" but rather\n\"a blob is not something that can fast-forward\". Using the existing\nREJECT_NONFASTFORWARD is insufficient (because later code will recommend\npull-then-push, which is wrong). So I'd be in favor of creating a new\nerror status for it.\n\n-Peff\n"},{"id":"207457","messageId":"7vtxqadrex.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"20130121234002.GE17156@sigill.intra.peff.net","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-21T23:53:26Z","receivedAt":"2013-01-21T23:53:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... The problem there is not \"already exists\" but rather\n> \"a blob is not something that can fast-forward\". Using the existing\n> REJECT_NONFASTFORWARD is insufficient (because later code will recommend\n> pull-then-push, which is wrong). So I'd be in favor of creating a new\n> error status for it.\n\nVery well said.\n\nPlease make it so ;-) or should I?\n"},{"id":"207471","messageId":"CAEUsAPYaK3PP67fc89-J3a83wzYcmu7HRyh7y1Kctg6d166LEQ@mail.gmail.com","threadId":"32247","inReplyTo":"20130121234002.GE17156@sigill.intra.peff.net","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-22T04:59:22Z","receivedAt":"2013-01-22T04:59:22Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Mon, Jan 21, 2013 at 5:40 PM, Jeff King wrote:\n> On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:\n>\n>> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King wrote:\n>> > However, if instead of the rule being\n>> > \"blobs on the remote side cannot be replaced\", if it becomes \"the old\n>> > value on the remote side must be referenced by what we replace it with\",\n>> > that _is_ something we can calculate reliably on the sending side.\n>>\n>> Interesting. I would have thought knowing reachability implied having\n>> the old object in the sending repository.\n>\n> No, because if you do not have it, then you know it is not reachable\n> from your refs (or your repository is corrupted). If you do have it, it\n> _might_ be reachable. For commits, checking is cheap (merge-base) and we\n> already do it. For trees and blobs, it is much more expensive, as you\n> have to walk the whole object graph. While it might be \"more correct\"\n> in some sense to say \"it's OK to replace a tree with a commit that\n> points to it\", in practice I doubt anyone cares, so you can probably\n> just punt on those ones and say \"no, it's not a fast forward\".\n\nThanks for explaining this further. I'm not exactly sure what I was\nthinking when I wrote the above other than I didn't fully grasp you\npoint and responded in a confused state. Clear on all fronts now.\n\n>> > And\n>> > that is logically an extension of the fast-forward rule, which is why I\n>> > suggested placing it with ref_newer (but the latter should probably be\n>> > extended to not suggest merging if we _know_ it is a non-commit object).\n>>\n>> Sounds great, especially if it is not dependent on the sender actually\n>> having the old object. Until this is implemented, though, I don't\n>> understand what was wrong with doing the checks in the\n>> is_forwardable() helper function (of course after fixing the\n>> regression/bug.)\n>\n> I don't think it is wrong per se; I just think that the check would go\n> more naturally where we are checking whether the object does indeed\n> fast-forward. Because is_forwardable in some cases must say \"I don't\n> know; I don't have the object to check its type, so maybe it is\n> forwardable, and maybe it is not\". Whereas when we do the actual\n> reachability check, we can say definitely \"this is not reachable because\n> I don't have it, or this is not reachable because it is a commit and I\n> checked, or this might be reachable but I don't care to check because it\n> has a funny type\".\n>\n> I think looking at it as the latter makes it more obvious how to handle\n> the \"maybe\" situation (e.g., the bug in is_forwardable was hard to see).\n>\n> Anyway, I do not care that much where it goes. To me, the important\n> thing is the error message. I do think the error \"already exists\" is a\n> reasonable one for refs/tags (we do not allow non-force pushes of\n> existing tags), but not necessarily for other cases, like trying to push\n> a blob over a blob. The problem there is not \"already exists\" but rather\n> \"a blob is not something that can fast-forward\". Using the existing\n> REJECT_NONFASTFORWARD is insufficient (because later code will recommend\n> pull-then-push, which is wrong). So I'd be in favor of creating a new\n> error status for it.\n\nI agree with everything above. I just don't understand why reverting\nthe \"already exists\" behavior for non-commit-ish objects was a\nprerequisite to fixing this. Despite the flaws (I am not referring to\nthe buggy behavior) you and Junio have pointed out, this still seems\nlike an improvement over the previous (and soon-to-be current)\nbehavior. Saying the remote reference already exists is true, and it\nimplies that removing it might solve the problem which is also true.\nAdding another error status will allow the error message to be made\nclearer in both cases (i.e., I avoided the word \"tag\" specifically so\nthat it would apply to other cases, or so I thought.)\n\nChris\n"},{"id":"207474","messageId":"1358834027-32039-1-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"20130121234002.GE17156@sigill.intra.peff.net","subject":"[PATCH 0/3] Finishing touches to \"push\" advises","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T05:53:44Z","receivedAt":"2013-01-22T05:53:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This builds on Chris Rorvick's earlier effort to forbid unforced\nupdates to refs/tags/ hierarchy and giving sensible error and advise\nmessages for that case (we are not rejecting such a push due to fast\nforwardness, and suggesting to fetch and integrate before pushing\nagain does not make sense).\n\nThe main change is in the second patch. When we\n\n * do not have the object at the tip of the remote;\n * the object at the tip of the remote is not a commit; or\n * the object we are pushing is not a commit,\n\nthere is no point suggesting to fetch, integrate and push again.\n\nIf we do not have the current object at the tip of the remote, we\nshould tell the user to fetch first and evaluate the situation\nbefore deciding what to do next.\n\nOtherwise, if the current object is not a commit, or if we are\ntrying to push an object that is not a commit, then the user does\nnot have to fetch first (we already have the object), but it still\ndoes not make sense to suggest to integrate and re-push. Just tell\nthem that such a push requires a force in such a case.\n\nJunio C Hamano (3):\n push: further clean up fields of \"struct ref\"\n push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE\n push: further reduce \"struct ref\" and simplify the logic\n\n advice.c | 4 ++++\n advice.h | 2 ++\n builtin/push.c | 25 +++++++++++++++++++++++++\n builtin/send-pack.c | 10 ++++++++++\n cache.h | 6 +++---\n remote.c | 38 ++++++++++++++++----------------------\n send-pack.c | 2 ++\n transport-helper.c | 10 ++++++++++\n transport.c | 14 +++++++++++++-\n transport.h | 2 ++\n 10 files changed, 87 insertions(+), 26 deletions(-)\n\n-- \n1.8.1.1.498.gfdee8be\n"},{"id":"207473","messageId":"1358834027-32039-2-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358834027-32039-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/3] push: further clean up fields of \"struct ref\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T05:53:45Z","receivedAt":"2013-01-22T05:53:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"nonfastforward\" field is only used to decide what value to\nassign to the \"status\" locally in a single function. Remove it from\nthe \"struct ref\" and make it into a local variable.\n\nThe \"requires_force\" field is not used to decide if the proposed\nupdate requires a --force option to succeed, or to record such a\ndecision made elsewhere. It is used by status reporting code that\nthe particular update was \"forced\". Rename it to \"forced_udpate\",\nand move the code to assign to it around to further clarify how it\nis used and what it is used for.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 3 +--\n remote.c | 10 +++++-----\n transport.c | 2 +-\n 3 files changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a942bbd..baa47b4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1001,9 +1001,8 @@ struct ref {\n \tchar *symref;\n \tunsigned int\n \t\tforce:1,\n-\t\trequires_force:1,\n+\t\tforced_update:1,\n \t\tmerge:1,\n-\t\tnonfastforward:1,\n \t\tupdate:1,\n \t\tdeletion:1;\n \tenum {\ndiff --git a/remote.c b/remote.c\nindex d3a1ca2..875296c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1322,22 +1322,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\t!is_null_sha1(ref->old_sha1);\n \n \t\tif (ref->update) {\n-\t\t\tref->nonfastforward =\n+\t\t\tint 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+\t\t\t\t|| !ref_newer(ref->new_sha1, ref->old_sha1);\n \n \t\t\tif (!prefixcmp(ref->name, \"refs/tags/\")) {\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\tref->forced_update = 1;\n+\t\t\t} else if (nonfastforward) {\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\tref->forced_update = 1;\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/transport.c b/transport.c\nindex 2673d27..585ebcd 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->requires_force) {\n+\t\tif (ref->forced_update) {\n \t\t\tstrcat(quickref, \"...\");\n \t\t\ttype = '+';\n \t\t\tmsg = \"forced update\";\n-- \n1.8.1.1.498.gfdee8be\n"},{"id":"207476","messageId":"1358834027-32039-3-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358834027-32039-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T05:53:46Z","receivedAt":"2013-01-22T05:53:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When pushing update an existing ref, we wouldn't even know if we are\nfast-forwarding the ref on the other end if:\n\n * we do not have the object currently at the tip of remote;\n * the object currently at the tip of remote is not a committish; or\n * the object we are pushing is not a committish.\n\nIn such a case, the push has been rejected on the client end, but we\nused the same error and advice messages as the ones used when\nrejecting a non-fast-forward push, i.e. pull from there and\nintegrate before pushing again. This did not make much sense.\n\nIntroduce two error classes and suggest fetching from the other side\nfirst and evaluate the situation, if we do not have the current\nobject, or just tell the user the update needs --force when we do\nhave the current object and either it or the object we are trying to\npush is not a committish, in which case it can never fast-forward\nand we know there is no point suggesting to merge.\n\nSigned-off-by: Junio C Hamano \n---\n advice.c | 4 ++++\n advice.h | 2 ++\n builtin/push.c | 25 +++++++++++++++++++++++++\n builtin/send-pack.c | 10 ++++++++++\n cache.h | 2 ++\n remote.c | 22 ++++++++++++++++------\n send-pack.c | 2 ++\n transport-helper.c | 10 ++++++++++\n transport.c | 12 ++++++++++++\n transport.h | 2 ++\n 10 files changed, 85 insertions(+), 6 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex d287927..780f58d 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;\n int advice_push_non_ff_default = 1;\n int advice_push_non_ff_matching = 1;\n int advice_push_already_exists = 1;\n+int advice_push_fetch_first = 1;\n+int advice_push_needs_force = 1;\n int advice_status_hints = 1;\n int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n@@ -20,6 +22,8 @@ static struct {\n \t{ \"pushnonffdefault\", &advice_push_non_ff_default },\n \t{ \"pushnonffmatching\", &advice_push_non_ff_matching },\n \t{ \"pushalreadyexists\", &advice_push_already_exists },\n+\t{ \"pushfetchfirst\", &advice_push_fetch_first },\n+\t{ \"pushneedsforce\", &advice_push_needs_force },\n \t{ \"statushints\", &advice_status_hints },\n \t{ \"commitbeforemerge\", &advice_commit_before_merge },\n \t{ \"resolveconflict\", &advice_resolve_conflict },\ndiff --git a/advice.h b/advice.h\nindex 8bf6356..fad36df 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;\n extern int advice_push_non_ff_default;\n extern int advice_push_non_ff_matching;\n extern int advice_push_already_exists;\n+extern int advice_push_fetch_first;\n+extern int advice_push_needs_force;\n extern int advice_status_hints;\n extern int advice_commit_before_merge;\n extern int advice_resolve_conflict;\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 8491e43..da928fa 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -224,6 +224,13 @@ 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.\");\n \n+static const char message_advice_ref_fetch_first[] =\n+\tN_(\"Updates were rejected; you need to fetch the destination reference\\n\"\n+\t \"to decide what to do.\\n\");\n+\n+static const char message_advice_ref_needs_force[] =\n+\tN_(\"Updates were rejected; you need to force update.\\n\");\n+\n static void advise_pull_before_push(void)\n {\n \tif (!advice_push_non_ff_current || !advice_push_update_rejected)\n@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)\n \tadvise(_(message_advice_ref_already_exists));\n }\n \n+static void advise_ref_fetch_first(void)\n+{\n+\tif (!advice_push_fetch_first || !advice_push_update_rejected)\n+\t\treturn;\n+\tadvise(_(message_advice_ref_fetch_first));\n+}\n+\n+static void advise_ref_needs_force(void)\n+{\n+\tif (!advice_push_needs_force || !advice_push_update_rejected)\n+\t\treturn;\n+\tadvise(_(message_advice_ref_needs_force));\n+}\n+\n static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)\n \t\t\tadvise_checkout_pull_push();\n \t} else if (reject_reasons & REJECT_ALREADY_EXISTS) {\n \t\tadvise_ref_already_exists();\n+\t} else if (reject_reasons & REJECT_FETCH_FIRST) {\n+\t\tadvise_ref_fetch_first();\n+\t} else if (reject_reasons & REJECT_NEEDS_FORCE) {\n+\t\tadvise_ref_needs_force();\n \t}\n \n \treturn 1;\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f849e0a..57a46b2 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -44,6 +44,16 @@ 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_FETCH_FIRST:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"fetch first\";\n+\t\t\tbreak;\n+\n+\t\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"needs force\";\n+\t\t\tbreak;\n+\n \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n \t\t\tres = \"error\";\n \t\t\tmsg = \"already exists\";\ndiff --git a/cache.h b/cache.h\nindex baa47b4..360bba5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1011,6 +1011,8 @@ struct ref {\n \t\tREF_STATUS_REJECT_NONFASTFORWARD,\n \t\tREF_STATUS_REJECT_ALREADY_EXISTS,\n \t\tREF_STATUS_REJECT_NODELETE,\n+\t\tREF_STATUS_REJECT_FETCH_FIRST,\n+\t\tREF_STATUS_REJECT_NEEDS_FORCE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\n \t\tREF_STATUS_EXPECTING_REPORT\ndiff --git a/remote.c b/remote.c\nindex 875296c..689dcf7 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1322,17 +1322,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\t!is_null_sha1(ref->old_sha1);\n \n \t\tif (ref->update) {\n-\t\t\tint 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 (!prefixcmp(ref->name, \"refs/tags/\")) {\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\tref->forced_update = 1;\n-\t\t\t} else if (nonfastforward) {\n+\t\t\t} else if (!has_sha1_file(ref->old_sha1) ||\n+\t\t\t\t !lookup_commit_reference_gently(ref->old_sha1, 1)) {\n+\t\t\t\tif (!force_ref_update) {\n+\t\t\t\t\tref->status = REF_STATUS_REJECT_FETCH_FIRST;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tref->forced_update = 1;\n+\t\t\t} else if (!lookup_commit_reference_gently(ref->new_sha1, 1)) {\n+\t\t\t\tif (!force_ref_update) {\n+\t\t\t\t\tref->status = REF_STATUS_REJECT_NEEDS_FORCE;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tref->forced_update = 1;\n+\t\t\t} else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {\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@@ -1521,7 +1530,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)\n \tstruct commit_list *list, *used;\n \tint found = 0;\n \n-\t/* Both new and old must be commit-ish and new is descendant of\n+\t/*\n+\t * Both new and old must be commit-ish and new is descendant of\n \t * old. Otherwise we require --force.\n \t */\n \to = deref_tag(parse_object(old_sha1), NULL, 0);\ndiff --git a/send-pack.c b/send-pack.c\nindex 1c375f0..97ab336 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,\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_REJECT_FETCH_FIRST:\n+\t\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 965b778..cb3ef7d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -666,6 +666,16 @@ 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, \"fetch first\")) {\n+\t\t\tstatus = REF_STATUS_REJECT_FETCH_FIRST;\n+\t\t\tfree(msg);\n+\t\t\tmsg = NULL;\n+\t\t}\n+\t\telse if (!strcmp(msg, \"needs force\")) {\n+\t\t\tstatus = REF_STATUS_REJECT_NEEDS_FORCE;\n+\t\t\tfree(msg);\n+\t\t\tmsg = NULL;\n+\t\t}\n \t}\n \n \tif (*ref)\ndiff --git a/transport.c b/transport.c\nindex 585ebcd..5105562 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -699,6 +699,14 @@ 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 \"already exists\", porcelain);\n \t\tbreak;\n+\tcase REF_STATUS_REJECT_FETCH_FIRST:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"fetch first\", porcelain);\n+\t\tbreak;\n+\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"needs force\", 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@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t\t\t*reject_reasons |= REJECT_NON_FF_OTHER;\n \t\t} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {\n \t\t\t*reject_reasons |= REJECT_ALREADY_EXISTS;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {\n+\t\t\t*reject_reasons |= REJECT_FETCH_FIRST;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {\n+\t\t\t*reject_reasons |= REJECT_NEEDS_FORCE;\n \t\t}\n \t}\n }\ndiff --git a/transport.h b/transport.h\nindex bfd2df5..c818763 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -143,6 +143,8 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\n #define REJECT_NON_FF_HEAD 0x01\n #define REJECT_NON_FF_OTHER 0x02\n #define REJECT_ALREADY_EXISTS 0x04\n+#define REJECT_FETCH_FIRST 0x08\n+#define REJECT_NEEDS_FORCE 0x10\n \n int transport_push(struct transport *connection,\n \t\t int refspec_nr, const char **refspec, int flags,\n-- \n1.8.1.1.498.gfdee8be\n"},{"id":"207475","messageId":"1358834027-32039-4-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358834027-32039-1-git-send-email-gitster@pobox.com","subject":"[PATCH 3/3] push: further reduce \"struct ref\" and simplify the logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T05:53:47Z","receivedAt":"2013-01-22T05:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"update\" field in \"struct ref\" is only used in a very narrow\nscope in a single function. Remove it.\n\nAlso simplify the code that rejects an attempted push by first\nchecking if the proposed update is forced (in which case we do not\nneed any check on our end).\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 1 -\n remote.c | 42 +++++++++++++-----------------------------\n 2 files changed, 13 insertions(+), 30 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 360bba5..377a3df 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1003,7 +1003,6 @@ struct ref {\n \t\tforce:1,\n \t\tforced_update:1,\n \t\tmerge: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 689dcf7..248910f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1317,37 +1317,21 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t * passing the --force argument\n \t\t */\n \n-\t\tref->update =\n-\t\t\t!ref->deletion &&\n-\t\t\t!is_null_sha1(ref->old_sha1);\n-\n-\t\tif (ref->update) {\n-\t\t\tif (!prefixcmp(ref->name, \"refs/tags/\")) {\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\tref->forced_update = 1;\n-\t\t\t} else if (!has_sha1_file(ref->old_sha1) ||\n-\t\t\t\t !lookup_commit_reference_gently(ref->old_sha1, 1)) {\n-\t\t\t\tif (!force_ref_update) {\n-\t\t\t\t\tref->status = REF_STATUS_REJECT_FETCH_FIRST;\n-\t\t\t\t\tcontinue;\n-\t\t\t\t}\n-\t\t\t\tref->forced_update = 1;\n-\t\t\t} else if (!lookup_commit_reference_gently(ref->new_sha1, 1)) {\n-\t\t\t\tif (!force_ref_update) {\n-\t\t\t\t\tref->status = REF_STATUS_REJECT_NEEDS_FORCE;\n-\t\t\t\t\tcontinue;\n-\t\t\t\t}\n-\t\t\t\tref->forced_update = 1;\n-\t\t\t} else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {\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\tif (!ref->deletion && !is_null_sha1(ref->old_sha1)) {\n+\t\t\tif (force_ref_update) {\n \t\t\t\tref->forced_update = 1;\n+\t\t\t\tcontinue;\n \t\t\t}\n+\n+\t\t\tif (!prefixcmp(ref->name, \"refs/tags/\"))\n+\t\t\t\tref->status = REF_STATUS_REJECT_ALREADY_EXISTS;\n+\t\t\telse if (!has_sha1_file(ref->old_sha1) ||\n+\t\t\t\t !lookup_commit_reference_gently(ref->old_sha1, 1))\n+\t\t\t\tref->status = REF_STATUS_REJECT_FETCH_FIRST;\n+\t\t\telse if (!lookup_commit_reference_gently(ref->new_sha1, 1))\n+\t\t\t\tref->status = REF_STATUS_REJECT_NEEDS_FORCE;\n+\t\t\telse if (!ref_newer(ref->new_sha1, ref->old_sha1))\n+\t\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n \t\t}\n \t}\n }\n-- \n1.8.1.1.498.gfdee8be\n"},{"id":"207477","messageId":"7vd2wxda91.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"1358834027-32039-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T06:04:10Z","receivedAt":"2013-01-22T06:04:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This one has a logic flaw. The logic outlined in the cover letter\nis correct, and the one described in the log message of this one is\nnot.\n\nWe should say \"fetch first\" only when we do not have old_sha1.\n\n\ndiff --git a/remote.c b/remote.c\nindex 248910f..8c39ea2 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1325,10 +1325,10 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \n \t\t\tif (!prefixcmp(ref->name, \"refs/tags/\"))\n \t\t\t\tref->status = REF_STATUS_REJECT_ALREADY_EXISTS;\n-\t\t\telse if (!has_sha1_file(ref->old_sha1) ||\n-\t\t\t\t !lookup_commit_reference_gently(ref->old_sha1, 1))\n+\t\t\telse if (!has_sha1_file(ref->old_sha1))\n \t\t\t\tref->status = REF_STATUS_REJECT_FETCH_FIRST;\n-\t\t\telse if (!lookup_commit_reference_gently(ref->new_sha1, 1))\n+\t\t\telse if (!lookup_commit_reference_gently(ref->new_sha1, 1) ||\n+\t\t\t\t !lookup_commit_reference_gently(ref->old_sha1, 1))\n \t\t\t\tref->status = REF_STATUS_REJECT_NEEDS_FORCE;\n \t\t\telse if (!ref_newer(ref->new_sha1, ref->old_sha1))\n \t\t\t\tref->status = REF_STATUS_REJECT_NONFASTFORWARD;\n"},{"id":"207478","messageId":"7v622pd9gp.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"1358834027-32039-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 3/3] push: further reduce \"struct ref\" and simplify the logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T06:21:10Z","receivedAt":"2013-01-22T06:21:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The \"update\" field in \"struct ref\" is only used in a very narrow\n> scope in a single function. Remove it.\n>\n> Also simplify the code that rejects an attempted push by first\n> checking if the proposed update is forced (in which case we do not\n> need any check on our end).\n\nActually, the latter is a bad idea; it changes the semantics and\nmark a push that was done with an unnecessary --force option.\n\nI'm rerolling these three patches (the \"update\" removal will also be\nmoved to the preparatory clean-up patch).\n"},{"id":"207479","messageId":"1358836230-9197-1-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"20130121234002.GE17156@sigill.intra.peff.net","subject":"[PATCH 0/3] Finishing touches to \"push\" advises","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T06:30:27Z","receivedAt":"2013-01-22T06:30:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This builds on Chris Rorvick's earlier effort to forbid unforced\nupdates to refs/tags/ hierarchy and giving sensible error and advise\nmessages for that case (we are not rejecting such a push due to fast\nforwardness, and suggesting to fetch and integrate before pushing\nagain does not make sense).\n\nThe series applies on top of 256b9d7 (push: fix \"refs/tags/\nhierarchy cannot be updated without --force\", 2013-01-16).\n\nThe main change is in the second patch. When we\n\n * do not have the object at the tip of the remote;\n * the object at the tip of the remote is not a commit; or\n * the object we are pushing is not a commit,\n\nthere is no point suggesting to fetch, integrate and push again.\n\nIf we do not have the current object at the tip of the remote, we\nshould tell the user to fetch first and evaluate the situation\nbefore deciding what to do next.\n\nOtherwise, if the current object is not a commit, or if we are\ntrying to push an object that is not a commit, then the user does\nnot have to fetch first (we already have the object), but it still\ndoes not make sense to suggest to integrate and re-push. Just tell\nthem that such a push requires a force in such a case.\n\nJunio C Hamano (3):\n push: further clean up fields of \"struct ref\"\n push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE\n push: further simplify the logic to assign rejection status\n\n advice.c | 4 ++++\n advice.h | 2 ++\n builtin/push.c | 25 +++++++++++++++++++++++++\n builtin/send-pack.c | 10 ++++++++++\n cache.h | 6 +++---\n remote.c | 42 +++++++++++++++++++-----------------------\n send-pack.c | 2 ++\n transport-helper.c | 10 ++++++++++\n transport.c | 14 +++++++++++++-\n transport.h | 2 ++\n 10 files changed, 90 insertions(+), 27 deletions(-)\n\n-- \n1.8.1.1.498.gfdee8be\n"},{"id":"207480","messageId":"1358836230-9197-2-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358836230-9197-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 1/3] push: further clean up fields of \"struct ref\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T06:30:28Z","receivedAt":"2013-01-22T06:30:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"nonfastforward\" and \"update\" fields are only used while\ndeciding what value to assign to the \"status\" locally in a single\nfunction. Remove them from the \"struct ref\".\n\nThe \"requires_force\" field is not used to decide if the proposed\nupdate requires a --force option to succeed, or to record such a\ndecision made elsewhere. It is used by status reporting code that\nthe particular update was \"forced\". Rename it to \"forced_udpate\",\nand move the code to assign to it around to further clarify how it\nis used and what it is used for.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The \"update\" removal in v1 has been moved to this.\n\n cache.h | 4 +---\n remote.c | 16 ++++++----------\n transport.c | 2 +-\n 3 files changed, 8 insertions(+), 14 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a942bbd..12631a1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1001,10 +1001,8 @@ struct ref {\n \tchar *symref;\n \tunsigned int\n \t\tforce:1,\n-\t\trequires_force:1,\n+\t\tforced_update:1,\n \t\tmerge:1,\n-\t\tnonfastforward: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 d3a1ca2..3375914 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1317,27 +1317,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t * passing the --force argument\n \t\t */\n \n-\t\tref->update =\n-\t\t\t!ref->deletion &&\n-\t\t\t!is_null_sha1(ref->old_sha1);\n-\n-\t\tif (ref->update) {\n-\t\t\tref->nonfastforward =\n+\t\tif (!ref->deletion && !is_null_sha1(ref->old_sha1)) {\n+\t\t\tint 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+\t\t\t\t|| !ref_newer(ref->new_sha1, ref->old_sha1);\n \n \t\t\tif (!prefixcmp(ref->name, \"refs/tags/\")) {\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\tref->forced_update = 1;\n+\t\t\t} else if (nonfastforward) {\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\tref->forced_update = 1;\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/transport.c b/transport.c\nindex 2673d27..585ebcd 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->requires_force) {\n+\t\tif (ref->forced_update) {\n \t\t\tstrcat(quickref, \"...\");\n \t\t\ttype = '+';\n \t\t\tmsg = \"forced update\";\n-- \n1.8.1.1.498.gfdee8be\n"},{"id":"207481","messageId":"1358836230-9197-3-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358836230-9197-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T06:30:29Z","receivedAt":"2013-01-22T06:30:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we push to update an existing ref, if:\n\n * we do not have the object at the tip of the remote; or\n * the object at the tip of the remote is not a commit; or\n * the object we are pushing is not a commit,\n\nthere is no point suggesting to fetch, integrate and push again.\n\nIf we do not have the current object at the tip of the remote, we\nshould tell the user to fetch first and evaluate the situation\nbefore deciding what to do next.\n\nOtherwise, if the current object is not a commit, or if we are\ntrying to push an object that is not a commit, then the user does\nnot have to fetch first (we already have the object), but it still\ndoes not make sense to suggest to integrate and re-push. Just tell\nthem that such a push requires a force in such a case.\n\nIn these cases, the push was locally rejected on the client end, but\nwe used the same error and advice messages as the ones used when\nrejecting a non-fast-forward push, i.e. pull from there and\nintegrate before pushing again. This did not make much sense.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Updated log message and fixed the logic to decide \"fetch first\";\n we should say \"fetch first\" only when we do not have the current\n tip of the remote end.\n\n send-pack.c has style violation that \"else\" is not on the same\n line as closing brace of its corresponding \"if\", but I followed\n the existing style of surrounding code. Cleaning them up is for\n a separate topic.\n\n advice.c | 4 ++++\n advice.h | 2 ++\n builtin/push.c | 25 +++++++++++++++++++++++++\n builtin/send-pack.c | 10 ++++++++++\n cache.h | 2 ++\n remote.c | 22 ++++++++++++++++------\n send-pack.c | 2 ++\n transport-helper.c | 10 ++++++++++\n transport.c | 12 ++++++++++++\n transport.h | 2 ++\n 10 files changed, 85 insertions(+), 6 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex d287927..780f58d 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;\n int advice_push_non_ff_default = 1;\n int advice_push_non_ff_matching = 1;\n int advice_push_already_exists = 1;\n+int advice_push_fetch_first = 1;\n+int advice_push_needs_force = 1;\n int advice_status_hints = 1;\n int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n@@ -20,6 +22,8 @@ static struct {\n \t{ \"pushnonffdefault\", &advice_push_non_ff_default },\n \t{ \"pushnonffmatching\", &advice_push_non_ff_matching },\n \t{ \"pushalreadyexists\", &advice_push_already_exists },\n+\t{ \"pushfetchfirst\", &advice_push_fetch_first },\n+\t{ \"pushneedsforce\", &advice_push_needs_force },\n \t{ \"statushints\", &advice_status_hints },\n \t{ \"commitbeforemerge\", &advice_commit_before_merge },\n \t{ \"resolveconflict\", &advice_resolve_conflict },\ndiff --git a/advice.h b/advice.h\nindex 8bf6356..fad36df 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;\n extern int advice_push_non_ff_default;\n extern int advice_push_non_ff_matching;\n extern int advice_push_already_exists;\n+extern int advice_push_fetch_first;\n+extern int advice_push_needs_force;\n extern int advice_status_hints;\n extern int advice_commit_before_merge;\n extern int advice_resolve_conflict;\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 8491e43..da928fa 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -224,6 +224,13 @@ 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.\");\n \n+static const char message_advice_ref_fetch_first[] =\n+\tN_(\"Updates were rejected; you need to fetch the destination reference\\n\"\n+\t \"to decide what to do.\\n\");\n+\n+static const char message_advice_ref_needs_force[] =\n+\tN_(\"Updates were rejected; you need to force update.\\n\");\n+\n static void advise_pull_before_push(void)\n {\n \tif (!advice_push_non_ff_current || !advice_push_update_rejected)\n@@ -252,6 +259,20 @@ static void advise_ref_already_exists(void)\n \tadvise(_(message_advice_ref_already_exists));\n }\n \n+static void advise_ref_fetch_first(void)\n+{\n+\tif (!advice_push_fetch_first || !advice_push_update_rejected)\n+\t\treturn;\n+\tadvise(_(message_advice_ref_fetch_first));\n+}\n+\n+static void advise_ref_needs_force(void)\n+{\n+\tif (!advice_push_needs_force || !advice_push_update_rejected)\n+\t\treturn;\n+\tadvise(_(message_advice_ref_needs_force));\n+}\n+\n static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n@@ -285,6 +306,10 @@ static int push_with_options(struct transport *transport, int flags)\n \t\t\tadvise_checkout_pull_push();\n \t} else if (reject_reasons & REJECT_ALREADY_EXISTS) {\n \t\tadvise_ref_already_exists();\n+\t} else if (reject_reasons & REJECT_FETCH_FIRST) {\n+\t\tadvise_ref_fetch_first();\n+\t} else if (reject_reasons & REJECT_NEEDS_FORCE) {\n+\t\tadvise_ref_needs_force();\n \t}\n \n \treturn 1;\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f849e0a..57a46b2 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -44,6 +44,16 @@ 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_FETCH_FIRST:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"fetch first\";\n+\t\t\tbreak;\n+\n+\t\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"needs force\";\n+\t\t\tbreak;\n+\n \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n \t\t\tres = \"error\";\n \t\t\tmsg = \"already exists\";\ndiff --git a/cache.h b/cache.h\nindex 12631a1..377a3df 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1010,6 +1010,8 @@ struct ref {\n \t\tREF_STATUS_REJECT_NONFASTFORWARD,\n \t\tREF_STATUS_REJECT_ALREADY_EXISTS,\n \t\tREF_STATUS_REJECT_NODELETE,\n+\t\tREF_STATUS_REJECT_FETCH_FIRST,\n+\t\tREF_STATUS_REJECT_NEEDS_FORCE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\n \t\tREF_STATUS_EXPECTING_REPORT\ndiff --git a/remote.c b/remote.c\nindex 3375914..c991915 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1318,17 +1318,26 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t */\n \n \t\tif (!ref->deletion && !is_null_sha1(ref->old_sha1)) {\n-\t\t\tint 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 (!prefixcmp(ref->name, \"refs/tags/\")) {\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\tref->forced_update = 1;\n-\t\t\t} else if (nonfastforward) {\n+\t\t\t} else if (!has_sha1_file(ref->old_sha1)) {\n+\t\t\t\tif (!force_ref_update) {\n+\t\t\t\t\tref->status = REF_STATUS_REJECT_FETCH_FIRST;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tref->forced_update = 1;\n+\t\t\t} else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||\n+\t\t\t\t !lookup_commit_reference_gently(ref->new_sha1, 1)) {\n+\t\t\t\tif (!force_ref_update) {\n+\t\t\t\t\tref->status = REF_STATUS_REJECT_NEEDS_FORCE;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\t\t\t\tref->forced_update = 1;\n+\t\t\t} else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {\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@@ -1517,7 +1526,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)\n \tstruct commit_list *list, *used;\n \tint found = 0;\n \n-\t/* Both new and old must be commit-ish and new is descendant of\n+\t/*\n+\t * Both new and old must be commit-ish and new is descendant of\n \t * old. Otherwise we require --force.\n \t */\n \to = deref_tag(parse_object(old_sha1), NULL, 0);\ndiff --git a/send-pack.c b/send-pack.c\nindex 1c375f0..97ab336 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,\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_REJECT_FETCH_FIRST:\n+\t\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 965b778..cb3ef7d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -666,6 +666,16 @@ 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, \"fetch first\")) {\n+\t\t\tstatus = REF_STATUS_REJECT_FETCH_FIRST;\n+\t\t\tfree(msg);\n+\t\t\tmsg = NULL;\n+\t\t}\n+\t\telse if (!strcmp(msg, \"needs force\")) {\n+\t\t\tstatus = REF_STATUS_REJECT_NEEDS_FORCE;\n+\t\t\tfree(msg);\n+\t\t\tmsg = NULL;\n+\t\t}\n \t}\n \n \tif (*ref)\ndiff --git a/transport.c b/transport.c\nindex 585ebcd..5105562 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -699,6 +699,14 @@ 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 \"already exists\", porcelain);\n \t\tbreak;\n+\tcase REF_STATUS_REJECT_FETCH_FIRST:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"fetch first\", porcelain);\n+\t\tbreak;\n+\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"needs force\", 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@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t\t\t*reject_reasons |= REJECT_NON_FF_OTHER;\n \t\t} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {\n \t\t\t*reject_reasons |= REJECT_ALREADY_EXISTS;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {\n+\t\t\t*reject_reasons |= REJECT_FETCH_FIRST;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {\n+\t\t\t*reject_reasons |= REJECT_NEEDS_FORCE;\n \t\t}\n \t}\n }\ndiff --git a/transport.h b/transport.h\nindex bfd2df5..c818763 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -143,6 +143,8 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\n #define REJECT_NON_FF_HEAD 0x01\n #define REJECT_NON_FF_OTHER 0x02\n #define REJECT_ALREADY_EXISTS 0x04\n+#define REJECT_FETCH_FIRST 0x08\n+#define REJECT_NEEDS_FORCE 0x10\n \n int transport_push(struct transport *connection,\n \t\t int refspec_nr, const char **refspec, int flags,\n-- \n1.8.1.1.498.gfdee8be\n"},{"id":"207482","messageId":"1358836230-9197-4-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358836230-9197-1-git-send-email-gitster@pobox.com","subject":"[PATCH v2 3/3] push: further simplify the logic to assign rejection status","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T06:30:30Z","receivedAt":"2013-01-22T06:30:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead of using deeply nested if/else statements, first decide what\nrejection status we would get if this push weren't forced, and then\nassign the rejection reason to the ref->status field and flip the\nref->forced_update field when we forced a push for a ref that indeed\nrequired forcing.\n\nSigned-off-by: Junio C Hamano \n---\n\n * The first one mistakenly changed the semantics and reported a\n forced push even when the push was done with useless and\n unnecessary --force option (e.g. the update was properly\n fast-forwarding but --force was given from the command line).\n This fixes it.\n\n remote.c | 40 +++++++++++++++-------------------------\n 1 file changed, 15 insertions(+), 25 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex c991915..af2136d 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1318,32 +1318,22 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t */\n \n \t\tif (!ref->deletion && !is_null_sha1(ref->old_sha1)) {\n-\t\t\tif (!prefixcmp(ref->name, \"refs/tags/\")) {\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\tint status = 0;\n+\n+\t\t\tif (!prefixcmp(ref->name, \"refs/tags/\"))\n+\t\t\t\tstatus = REF_STATUS_REJECT_ALREADY_EXISTS;\n+\t\t\telse if (!has_sha1_file(ref->old_sha1))\n+\t\t\t\tstatus = REF_STATUS_REJECT_FETCH_FIRST;\n+\t\t\telse if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||\n+\t\t\t\t !lookup_commit_reference_gently(ref->new_sha1, 1))\n+\t\t\t\tstatus = REF_STATUS_REJECT_NEEDS_FORCE;\n+\t\t\telse if (!ref_newer(ref->new_sha1, ref->old_sha1))\n+\t\t\t\tstatus = REF_STATUS_REJECT_NONFASTFORWARD;\n+\n+\t\t\tif (!force_ref_update)\n+\t\t\t\tref->status = status;\n+\t\t\telse if (status)\n \t\t\t\tref->forced_update = 1;\n-\t\t\t} else if (!has_sha1_file(ref->old_sha1)) {\n-\t\t\t\tif (!force_ref_update) {\n-\t\t\t\t\tref->status = REF_STATUS_REJECT_FETCH_FIRST;\n-\t\t\t\t\tcontinue;\n-\t\t\t\t}\n-\t\t\t\tref->forced_update = 1;\n-\t\t\t} else if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||\n-\t\t\t\t !lookup_commit_reference_gently(ref->new_sha1, 1)) {\n-\t\t\t\tif (!force_ref_update) {\n-\t\t\t\t\tref->status = REF_STATUS_REJECT_NEEDS_FORCE;\n-\t\t\t\t\tcontinue;\n-\t\t\t\t}\n-\t\t\t\tref->forced_update = 1;\n-\t\t\t} else if (!ref_newer(ref->new_sha1, ref->old_sha1)) {\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\tref->forced_update = 1;\n-\t\t\t}\n \t\t}\n \t}\n }\n-- \n1.8.1.1.498.gfdee8be\n"},{"id":"207483","messageId":"7v1uddd8dm.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"CAEUsAPYaK3PP67fc89-J3a83wzYcmu7HRyh7y1Kctg6d166LEQ@mail.gmail.com","subject":"Re: [PATCH v6 0/8] push: update remote tags only with force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T06:44:37Z","receivedAt":"2013-01-22T06:44:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Rorvick writes:\n\n> I agree with everything above. I just don't understand why reverting\n> the \"already exists\" behavior for non-commit-ish objects was a\n> prerequisite to fixing this.\n\nBecause it is a regression. People who did not force such a push\ndid not get \"already exists\", but with your patch they do.\n\nBy reverting the wrong message so that we get the old wrong message\ninstead, people will only have to deal with an already known\nbreakage; a known devil is better than an unknown new devil (or an\nunknown angel).\n\nWhen a change that brings in a regression and an improvement at the\nsame time, it does not matter what the improvement is; we fix the\nregression first as soon as safely possible and we then attempt to\nresurrect and polish the improvement.\n"},{"id":"207484","messageId":"7vwqv5brvd.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"1358836230-9197-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/3] Finishing touches to \"push\" advises","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-22T07:26:30Z","receivedAt":"2013-01-22T07:26:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As far as I am concerned, I am pretty much done with this topic, at\nleast for now. Of course if there are bugreports I'll try to help\nresolving them, but I do not expect myself adding new object-type\nbased policy decision to this codepath.\n\nThe call the updated call makes to ref_newer() no longer feeds\ncertain combinations to the function, because the NULL-ness of the\nold and commit-ness of both are checked before making a call.\n\nI notice that builtin/remote.c has another callsite for ref_newer().\nAlthough I didn't look at the code, I think it is trying to see if\nthe branch can be pushed as a fast-forward to the remote (or the\nremote tip moved since you started building on top of it).\n\nIt probably makes sense to refactor the logic that is run per-ref in\nthe loop in the set_ref_status_for_push() function into a new helper\nfunction, inline ref_newer() there, and have the remaining callers\nof ref_newer() to use that new helper function, which knows the new\nrules such as \"refs/tags/ cannot be replaced with anything without\nforce\".\n"},{"id":"207564","messageId":"20130123064357.GA10306@sigill.intra.peff.net","threadId":"32247","inReplyTo":"1358836230-9197-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 1/3] push: further clean up fields of \"struct ref\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:43:58Z","receivedAt":"2013-01-23T06:43:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 21, 2013 at 10:30:28PM -0800, Junio C Hamano wrote:\n\n> The \"nonfastforward\" and \"update\" fields are only used while\n> deciding what value to assign to the \"status\" locally in a single\n> function. Remove them from the \"struct ref\".\n> \n> The \"requires_force\" field is not used to decide if the proposed\n> update requires a --force option to succeed, or to record such a\n> decision made elsewhere. It is used by status reporting code that\n> the particular update was \"forced\". Rename it to \"forced_udpate\",\n\nTypo.\n\n> and move the code to assign to it around to further clarify how it\n> is used and what it is used for.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> \n> * The \"update\" removal in v1 has been moved to this.\n> \n> cache.h | 4 +---\n> remote.c | 16 ++++++----------\n> transport.c | 2 +-\n> 3 files changed, 8 insertions(+), 14 deletions(-)\n\nLooks much better.\n\nI wondered briefly why nonfastforward was even there, as I recall that I\nwas the one who added it many years ago. It turns out that it used to\nserve the purpose of the new forced_update, but Chris's series from a\nfew months ago split it out to \"nonfastforward\" and \"not_forwardable\",\nand then added \"requires_force\" to give a single flag that is set in\neither case.\n\nSo I think your simplification is correct; the first two can be local\nvariables, and the only thing that matters to carry forward is\nrequires_force (and I agree that forced_update is a better name).\n\n-Peff\n"},{"id":"207567","messageId":"20130123065640.GB10306@sigill.intra.peff.net","threadId":"32247","inReplyTo":"1358836230-9197-3-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:56:40Z","receivedAt":"2013-01-23T06:56:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 21, 2013 at 10:30:29PM -0800, Junio C Hamano wrote:\n\n> When we push to update an existing ref, if:\n> \n> * we do not have the object at the tip of the remote; or\n> * the object at the tip of the remote is not a commit; or\n> * the object we are pushing is not a commit,\n> \n> there is no point suggesting to fetch, integrate and push again.\n> \n> If we do not have the current object at the tip of the remote, we\n> should tell the user to fetch first and evaluate the situation\n> before deciding what to do next.\n\nShould we? I know that it is more correct to do so, because we do not\neven know for sure that the remote object is a commit, and fetching\n_might_ lead to us saying \"hey, this is not something that can be\nfast-forwarded\".\n\nBut by far the common case will be that it _is_ a commit, and the right\nthing is going to be to pull. Adding in the extra steps makes the\nworkflow longer and more complicated, and most of the time doesn't\nmatter. For example, imagine that Alice is working on \"master\", and when\nshe goes to push, she finds that Bob has already pushed his work. With\nthe current code, she sees:\n\n $ git push\n To ...\n ! [rejected] HEAD -> master (non-fast-forward)\n error: failed to push some refs to '...'\n hint: Updates were rejected because the tip of your current branch is behind\n hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')\n hint: before pushing again.\n\nand she presumably pulls, and all is well with the follow-up push.\n\nWith your patch, she sees:\n\n $ git push\n To ...\n ! [rejected] HEAD -> master (fetch first)\n error: failed to push some refs to '...'\n hint: Updates were rejected; you need to fetch the destination reference\n hint: to decide what to do.\n\n $ git fetch\n ...\n\n $ git push\n To ...\n ! [rejected] HEAD -> master (non-fast-forward)\n error: failed to push some refs to '...'\n hint: Updates were rejected because the tip of your current branch is behind\n hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')\n hint: before pushing again.\n hint: See the 'Note about fast-forwards' in 'git push --help' for details.\n\nwhich is technically more correct (it's possible that in the second\nstep, she would find that Bob pushed a tree or something). But in the\ncommon case that it is a commit, we've needlessly added two extra steps\n(a fetch and another failed push), both of which involve network access\n(so they are slow, and may involve Alice having to type her credentials).\n\nIs the extra hassle in the common case worth it for the off chance that\nwe might give a more accurate message? Should the \"fetch first\" message\nbe some hybrid that covers both cases accurately, but still points the\nuser towards \"git pull\" (which will fail anyway if the remote ref is not\na commit)?\n\n-Peff\n"},{"id":"207605","messageId":"7vip6nj22m.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"20130123065640.GB10306@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T16:28:49Z","receivedAt":"2013-01-23T16:28:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jan 21, 2013 at 10:30:29PM -0800, Junio C Hamano wrote:\n>\n>> When we push to update an existing ref, if:\n>> \n>> * we do not have the object at the tip of the remote; or\n>> * the object at the tip of the remote is not a commit; or\n>> * the object we are pushing is not a commit,\n>> \n>> there is no point suggesting to fetch, integrate and push again.\n>> \n>> If we do not have the current object at the tip of the remote, we\n>> should tell the user to fetch first and evaluate the situation\n>> before deciding what to do next.\n>\n> Should we? I know that it is more correct to do so, because we do not\n> even know for sure that the remote object is a commit, and fetching\n> _might_ lead to us saying \"hey, this is not something that can be\n> fast-forwarded\".\n>\n> But by far the common case will be that it _is_ a commit, and the right\n> thing is going to be to pull....\n> Is the extra hassle in the common case worth it for the off chance that\n> we might give a more accurate message? Should the \"fetch first\" message\n> be some hybrid that covers both cases accurately, but still points the\n> user towards \"git pull\" (which will fail anyway if the remote ref is not\n> a commit)?\n\nI was actually much less happy with \"needs force\" than this one, as\nyou have to assume too many things for the message to be a useful\nand a safe advise: the user has actually examined the situation and\nforcing the push is the right thing to do. Both old and new objects\nexist, so the user _could_ have done so, but did he really check\nthem, thought about the situation and made the right decision?\nPerhaps the attempted push had a typo in the object name it wanted\nto update the other end with, and the right thing to do is not to\nforce but to fix the refspec instead? \"You need --force to perform\nthis push\" was a very counter-productive advice in this case, but I\ndidn't think of a better wording.\n\nThe \"fetch first and inspect\" was an attempt to reduce the risk of\nthat \"needs force\" message that could encourage brainless forced\npushes. Perhaps if we reword \"needs force\" to something less risky,\nwe do not have to be so explicit in \"You have to fetch first and\nexamine\".\n\nHow about doing this?\n\nFor \"needs force\" cases, we say this instead:\n\n hint: you cannot update a ref that points at a non-commit object, or\n hint: update a ref to point at a non-commit object, without --force.\n\nBeing explicit about \"non-commit\" twice will catch user's eyes and\ncause him to double check that it is not a mistyped LHS of the push\nrefspec (if he is sending a non-commit) or mistyped RHS (if the ref\nis pointing at a non-commit). If he _is_ trying to push a blob out,\nthe advice makes it clear what to do next: he does want to force it.\n\nIf we did that, then we could loosen the \"You should fetch first\"\ncase to say something like this:\n\n hint: you do not have the object at the tip of the remote ref;\n hint: perhaps you want to pull from there first?\n\nThis explicitly denies one of Chris's wish \"we shouldn't suggest to\nmerge something that we may not be able to\", but in the \"You should\nfetch first\" case, we cannot fundamentally know if we can merge\nuntil we fetch. I agree with you that the most common case is that\nthe unknown object is a commit, and that suggesting to pull is a\ngood compromise.\n\nNote that you _could_ split the \"needs force\" case into two, namely,\n\"cannot replace a non-commit\" and \"cannot push a non-commit\". You\ncould even further split them into combinations (e.g. an attempt to\nreplace an annotated tag with a commit and an attempt to replace a\ntree with a commit may be different situations), but I think the\nadvices we can give to these cases would end up being the same, so I\ntend to think it is not worth it. That is what I meant by \"I do not\nexpect me doing the type-based policy myself\" in the concluding\nmessage of the series.\n"},{"id":"207641","messageId":"1358978130-12216-1-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"20130121234002.GE17156@sigill.intra.peff.net","subject":"[PATCH v4 0/3] Finishing touches to \"push\" advises","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T21:55:27Z","receivedAt":"2013-01-23T21:55:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This builds on Chris Rorvick's earlier effort to forbid unforced\nupdates to refs/tags/ hierarchy and giving sensible error and advise\nmessages for that case (we are not rejecting such a push due to fast\nforwardness, and suggesting to fetch and integrate before pushing\nagain does not make sense).\n\nThe series applies on top of 256b9d7 (push: fix \"refs/tags/\nhierarchy cannot be updated without --force\", 2013-01-16).\n\nThis fourth round swaps the order of clean-up patches and now the\nbottom two are clean-up patches. The main change is in the third\none.\n\nWhen the object at the tip of the remote is not a committish, or the\nobject we are pushing is not a committish, the existing code already\nrejects such a push on the client end, but we used the same error\nand advice messages as the ones used when rejecting a push that does\nnot fast-forward, i.e. pull from there and integrate before pushing\nagain. Introduce a new rejection reason NEEDS_FORCE and explain why\nthe push was rejected, stressing the fact that --force is required\nwhen non committish objects are involved, so that the user can (1)\nnotice a possibly mistyped source object name or destination ref\nname, when the user is trying to push an ordinary commit, or (2)\nlearn that \"--force\" is an appropriate thing to use when the user is\nsure that s/he wants to push a non-committish (which is unusual).\n\nUnlike the third round, we do not say \"fetch first, inspect the\nsituation to decide what to do\", when we do not have the object\nsitting at the tip of the remote. Most likely, it is a commit\nsomebody who has been working on the same branch pushed that we\nhaven't fetched yet, so suggesting to pull is often sufficient and\nappropriate, and in a more uncommon case in which the unknown object\nis not a committish, the suggested pull will fail without making\npermanent damage anywhere. Next atttempt to push without changing\nanything (e.g. \"reset --hard\") will then trigger the NEEDS_FORCE\n\"Your push involves non-commit objects\" case.\n\n\nJunio C Hamano (3):\n push: further clean up fields of \"struct ref\"\n push: further simplify the logic to assign rejection reason\n push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE\n\n Documentation/config.txt | 12 +++++++++++-\n advice.c | 4 ++++\n advice.h | 2 ++\n builtin/push.c | 29 +++++++++++++++++++++++++++++\n builtin/send-pack.c | 10 ++++++++++\n cache.h | 6 +++---\n remote.c | 42 +++++++++++++++++++-----------------------\n send-pack.c | 2 ++\n transport-helper.c | 10 ++++++++++\n transport.c | 14 +++++++++++++-\n transport.h | 2 ++\n 11 files changed, 105 insertions(+), 28 deletions(-)\n\n-- \n1.8.1.1.517.g0318d2b\n"},{"id":"207642","messageId":"1358978130-12216-2-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358978130-12216-1-git-send-email-gitster@pobox.com","subject":"[PATCH v4 1/3] push: further clean up fields of \"struct ref\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T21:55:28Z","receivedAt":"2013-01-23T21:55:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The \"nonfastforward\" and \"update\" fields are only used while\ndeciding what value to assign to the \"status\" locally in a single\nfunction. Remove them from the \"struct ref\".\n\nThe \"requires_force\" field is not used to decide if the proposed\nupdate requires a --force option to succeed, or to record such a\ndecision made elsewhere. It is used by status reporting code that\nthe particular update was \"forced\". Rename it to \"forced_udpate\",\nand move the code to assign to it around to further clarify how it\nis used and what it is used for.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 4 +---\n remote.c | 16 ++++++----------\n transport.c | 2 +-\n 3 files changed, 8 insertions(+), 14 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex a942bbd..12631a1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1001,10 +1001,8 @@ struct ref {\n \tchar *symref;\n \tunsigned int\n \t\tforce:1,\n-\t\trequires_force:1,\n+\t\tforced_update:1,\n \t\tmerge:1,\n-\t\tnonfastforward: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 d3a1ca2..3375914 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1317,27 +1317,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t * passing the --force argument\n \t\t */\n \n-\t\tref->update =\n-\t\t\t!ref->deletion &&\n-\t\t\t!is_null_sha1(ref->old_sha1);\n-\n-\t\tif (ref->update) {\n-\t\t\tref->nonfastforward =\n+\t\tif (!ref->deletion && !is_null_sha1(ref->old_sha1)) {\n+\t\t\tint 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+\t\t\t\t|| !ref_newer(ref->new_sha1, ref->old_sha1);\n \n \t\t\tif (!prefixcmp(ref->name, \"refs/tags/\")) {\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\tref->forced_update = 1;\n+\t\t\t} else if (nonfastforward) {\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\tref->forced_update = 1;\n \t\t\t}\n \t\t}\n \t}\ndiff --git a/transport.c b/transport.c\nindex 2673d27..585ebcd 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->requires_force) {\n+\t\tif (ref->forced_update) {\n \t\t\tstrcat(quickref, \"...\");\n \t\t\ttype = '+';\n \t\t\tmsg = \"forced update\";\n-- \n1.8.1.1.517.g0318d2b\n"},{"id":"207643","messageId":"1358978130-12216-3-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358978130-12216-1-git-send-email-gitster@pobox.com","subject":"[PATCH v4 2/3] push: further simplify the logic to assign rejection reason","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T21:55:29Z","receivedAt":"2013-01-23T21:55:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"First compute the reason why this push would fail if done without\n\"--force\", and then fail it by assigning that reason when the push\nwas not forced (or if there is no reason to require force, allow it\nto succeed).\n\nRecord the fact that the push was forced in the forced_update field\nonly when the push would have failed without the option.\n\nThe code becomes shorter, less repetitive and easier to read this\nway, especially given that the set of rejection reasons will be\nextended in a later patch.\n\nSigned-off-by: Junio C Hamano \n---\n remote.c | 27 +++++++++++----------------\n 1 file changed, 11 insertions(+), 16 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 3375914..969aa11 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1318,23 +1318,18 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t */\n \n \t\tif (!ref->deletion && !is_null_sha1(ref->old_sha1)) {\n-\t\t\tint 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 (!prefixcmp(ref->name, \"refs/tags/\")) {\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\tref->forced_update = 1;\n-\t\t\t} else if (nonfastforward) {\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\tint why = 0; /* why would this push require --force? */\n+\n+\t\t\tif (!prefixcmp(ref->name, \"refs/tags/\"))\n+\t\t\t\twhy = REF_STATUS_REJECT_ALREADY_EXISTS;\n+\t\t\telse if (!has_sha1_file(ref->old_sha1)\n+\t\t\t\t || !ref_newer(ref->new_sha1, ref->old_sha1))\n+\t\t\t\twhy = REF_STATUS_REJECT_NONFASTFORWARD;\n+\n+\t\t\tif (!force_ref_update)\n+\t\t\t\tref->status = why;\n+\t\t\telse if (why)\n \t\t\t\tref->forced_update = 1;\n-\t\t\t}\n \t\t}\n \t}\n }\n-- \n1.8.1.1.517.g0318d2b\n"},{"id":"207644","messageId":"1358978130-12216-4-git-send-email-gitster@pobox.com","threadId":"32247","inReplyTo":"1358978130-12216-1-git-send-email-gitster@pobox.com","subject":"[PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T21:55:30Z","receivedAt":"2013-01-23T21:55:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When we push to update an existing ref, if:\n\n * the object at the tip of the remote is not a commit; or\n * the object we are pushing is not a commit,\n\nit won't be correct to suggest to fetch, integrate and push again,\nas the old and new objects will not \"merge\".\n\nIf we do not have the current object at the tip of the remote, we do\nnot even know that object, when fetched, is something that can be\nmerged. In such a case, suggesting to pull first just like\nnon-fast-forward case may not be technically correct, but in\npractice, most such failures are seen when you try to push your work\nto a branch without knowing that somebody else already pushed to\nupdate the same branch since you forked, so \"pull first\" would work\nas a suggestion most of the time.\n\nIn these cases, the current code already rejects such a push on the\nclient end, but we used the same error and advice messages as the\nones used when rejecting a non-fast-forward push, i.e. pull from\nthere and integrate before pushing again. Introduce new\nrejection reasons and reword the messages appropriately.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 12 +++++++++++-\n advice.c | 4 ++++\n advice.h | 2 ++\n builtin/push.c | 29 +++++++++++++++++++++++++++++\n builtin/send-pack.c | 10 ++++++++++\n cache.h | 2 ++\n remote.c | 11 ++++++++---\n send-pack.c | 2 ++\n transport-helper.c | 10 ++++++++++\n transport.c | 12 ++++++++++++\n transport.h | 2 ++\n 11 files changed, 92 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 90e7d10..1f47761 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -143,7 +143,8 @@ advice.*::\n \tpushUpdateRejected::\n \t\tSet this variable to 'false' if you want to disable\n \t\t'pushNonFFCurrent', 'pushNonFFDefault',\n-\t\t'pushNonFFMatching', and 'pushAlreadyExists'\n+\t\t'pushNonFFMatching', 'pushAlreadyExists',\n+\t\t'pushFetchFirst', and 'pushNeedsForce'\n \t\tsimultaneously.\n \tpushNonFFCurrent::\n \t\tAdvice shown when linkgit:git-push[1] fails due to a\n@@ -162,6 +163,15 @@ advice.*::\n \tpushAlreadyExists::\n \t\tShown when linkgit:git-push[1] rejects an update that\n \t\tdoes not qualify for fast-forwarding (e.g., a tag.)\n+\tpushFetchFirst::\n+\t\tShown when linkgit:git-push[1] rejects an update that\n+\t\ttries to overwrite a remote ref that points at an\n+\t\tobject we do not have.\n+\tpushNeedsForce::\n+\t\tShown when linkgit:git-push[1] rejects an update that\n+\t\ttries to overwrite a remote ref that points at an\n+\t\tobject that is not a committish, or make the remote\n+\t\tref point at an object that is not a committish.\n \tstatusHints::\n \t\tShow directions on how to proceed from the current\n \t\tstate in the output of linkgit:git-status[1] and in\ndiff --git a/advice.c b/advice.c\nindex d287927..780f58d 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -5,6 +5,8 @@ int advice_push_non_ff_current = 1;\n int advice_push_non_ff_default = 1;\n int advice_push_non_ff_matching = 1;\n int advice_push_already_exists = 1;\n+int advice_push_fetch_first = 1;\n+int advice_push_needs_force = 1;\n int advice_status_hints = 1;\n int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n@@ -20,6 +22,8 @@ static struct {\n \t{ \"pushnonffdefault\", &advice_push_non_ff_default },\n \t{ \"pushnonffmatching\", &advice_push_non_ff_matching },\n \t{ \"pushalreadyexists\", &advice_push_already_exists },\n+\t{ \"pushfetchfirst\", &advice_push_fetch_first },\n+\t{ \"pushneedsforce\", &advice_push_needs_force },\n \t{ \"statushints\", &advice_status_hints },\n \t{ \"commitbeforemerge\", &advice_commit_before_merge },\n \t{ \"resolveconflict\", &advice_resolve_conflict },\ndiff --git a/advice.h b/advice.h\nindex 8bf6356..fad36df 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -8,6 +8,8 @@ extern int advice_push_non_ff_current;\n extern int advice_push_non_ff_default;\n extern int advice_push_non_ff_matching;\n extern int advice_push_already_exists;\n+extern int advice_push_fetch_first;\n+extern int advice_push_needs_force;\n extern int advice_status_hints;\n extern int advice_commit_before_merge;\n extern int advice_resolve_conflict;\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 8491e43..92ca3d7 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -220,10 +220,21 @@ 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_fetch_first[] =\n+\tN_(\"Updates were rejected because you do not have the object at the tip\\n\"\n+\t \"of the remote. You may want to first merge the remote changes (e.g.\\n\"\n+\t \" '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.\");\n \n+static const char message_advice_ref_needs_force[] =\n+\tN_(\"You cannot update a remote ref that points at a non-commit object,\\n\"\n+\t \"or update a remote ref to make it point at a non-commit object,\\n\"\n+\t \"without using the '--force' option.\\n\");\n+\n static void advise_pull_before_push(void)\n {\n \tif (!advice_push_non_ff_current || !advice_push_update_rejected)\n@@ -252,6 +263,20 @@ static void advise_ref_already_exists(void)\n \tadvise(_(message_advice_ref_already_exists));\n }\n \n+static void advise_ref_fetch_first(void)\n+{\n+\tif (!advice_push_fetch_first || !advice_push_update_rejected)\n+\t\treturn;\n+\tadvise(_(message_advice_ref_fetch_first));\n+}\n+\n+static void advise_ref_needs_force(void)\n+{\n+\tif (!advice_push_needs_force || !advice_push_update_rejected)\n+\t\treturn;\n+\tadvise(_(message_advice_ref_needs_force));\n+}\n+\n static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n@@ -285,6 +310,10 @@ static int push_with_options(struct transport *transport, int flags)\n \t\t\tadvise_checkout_pull_push();\n \t} else if (reject_reasons & REJECT_ALREADY_EXISTS) {\n \t\tadvise_ref_already_exists();\n+\t} else if (reject_reasons & REJECT_FETCH_FIRST) {\n+\t\tadvise_ref_fetch_first();\n+\t} else if (reject_reasons & REJECT_NEEDS_FORCE) {\n+\t\tadvise_ref_needs_force();\n \t}\n \n \treturn 1;\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f849e0a..57a46b2 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -44,6 +44,16 @@ 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_FETCH_FIRST:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"fetch first\";\n+\t\t\tbreak;\n+\n+\t\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"needs force\";\n+\t\t\tbreak;\n+\n \t\tcase REF_STATUS_REJECT_ALREADY_EXISTS:\n \t\t\tres = \"error\";\n \t\t\tmsg = \"already exists\";\ndiff --git a/cache.h b/cache.h\nindex 12631a1..377a3df 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1010,6 +1010,8 @@ struct ref {\n \t\tREF_STATUS_REJECT_NONFASTFORWARD,\n \t\tREF_STATUS_REJECT_ALREADY_EXISTS,\n \t\tREF_STATUS_REJECT_NODELETE,\n+\t\tREF_STATUS_REJECT_FETCH_FIRST,\n+\t\tREF_STATUS_REJECT_NEEDS_FORCE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\n \t\tREF_STATUS_EXPECTING_REPORT\ndiff --git a/remote.c b/remote.c\nindex 969aa11..a772e74 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1322,8 +1322,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \n \t\t\tif (!prefixcmp(ref->name, \"refs/tags/\"))\n \t\t\t\twhy = REF_STATUS_REJECT_ALREADY_EXISTS;\n-\t\t\telse if (!has_sha1_file(ref->old_sha1)\n-\t\t\t\t || !ref_newer(ref->new_sha1, ref->old_sha1))\n+\t\t\telse if (!has_sha1_file(ref->old_sha1))\n+\t\t\t\twhy = REF_STATUS_REJECT_FETCH_FIRST;\n+\t\t\telse if (!lookup_commit_reference_gently(ref->old_sha1, 1) ||\n+\t\t\t\t !lookup_commit_reference_gently(ref->new_sha1, 1))\n+\t\t\t\twhy = REF_STATUS_REJECT_NEEDS_FORCE;\n+\t\t\telse if (!ref_newer(ref->new_sha1, ref->old_sha1))\n \t\t\t\twhy = REF_STATUS_REJECT_NONFASTFORWARD;\n \n \t\t\tif (!force_ref_update)\n@@ -1512,7 +1516,8 @@ int ref_newer(const unsigned char *new_sha1, const unsigned char *old_sha1)\n \tstruct commit_list *list, *used;\n \tint found = 0;\n \n-\t/* Both new and old must be commit-ish and new is descendant of\n+\t/*\n+\t * Both new and old must be commit-ish and new is descendant of\n \t * old. Otherwise we require --force.\n \t */\n \to = deref_tag(parse_object(old_sha1), NULL, 0);\ndiff --git a/send-pack.c b/send-pack.c\nindex 1c375f0..97ab336 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -230,6 +230,8 @@ int send_pack(struct send_pack_args *args,\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_REJECT_FETCH_FIRST:\n+\t\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 965b778..cb3ef7d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -666,6 +666,16 @@ 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, \"fetch first\")) {\n+\t\t\tstatus = REF_STATUS_REJECT_FETCH_FIRST;\n+\t\t\tfree(msg);\n+\t\t\tmsg = NULL;\n+\t\t}\n+\t\telse if (!strcmp(msg, \"needs force\")) {\n+\t\t\tstatus = REF_STATUS_REJECT_NEEDS_FORCE;\n+\t\t\tfree(msg);\n+\t\t\tmsg = NULL;\n+\t\t}\n \t}\n \n \tif (*ref)\ndiff --git a/transport.c b/transport.c\nindex 585ebcd..5105562 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -699,6 +699,14 @@ 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 \"already exists\", porcelain);\n \t\tbreak;\n+\tcase REF_STATUS_REJECT_FETCH_FIRST:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"fetch first\", porcelain);\n+\t\tbreak;\n+\tcase REF_STATUS_REJECT_NEEDS_FORCE:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"needs force\", 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@@ -750,6 +758,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t\t\t*reject_reasons |= REJECT_NON_FF_OTHER;\n \t\t} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {\n \t\t\t*reject_reasons |= REJECT_ALREADY_EXISTS;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_FETCH_FIRST) {\n+\t\t\t*reject_reasons |= REJECT_FETCH_FIRST;\n+\t\t} else if (ref->status == REF_STATUS_REJECT_NEEDS_FORCE) {\n+\t\t\t*reject_reasons |= REJECT_NEEDS_FORCE;\n \t\t}\n \t}\n }\ndiff --git a/transport.h b/transport.h\nindex bfd2df5..c818763 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -143,6 +143,8 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\n #define REJECT_NON_FF_HEAD 0x01\n #define REJECT_NON_FF_OTHER 0x02\n #define REJECT_ALREADY_EXISTS 0x04\n+#define REJECT_FETCH_FIRST 0x08\n+#define REJECT_NEEDS_FORCE 0x10\n \n int transport_push(struct transport *connection,\n \t\t int refspec_nr, const char **refspec, int flags,\n-- \n1.8.1.1.517.g0318d2b\n"},{"id":"207667","messageId":"20130124064326.GB610@sigill.intra.peff.net","threadId":"32247","inReplyTo":"7vip6nj22m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-24T06:43:26Z","receivedAt":"2013-01-24T06:43:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 23, 2013 at 08:28:49AM -0800, Junio C Hamano wrote:\n\n> How about doing this?\n> \n> For \"needs force\" cases, we say this instead:\n> \n> hint: you cannot update a ref that points at a non-commit object, or\n> hint: update a ref to point at a non-commit object, without --force.\n> \n> Being explicit about \"non-commit\" twice will catch user's eyes and\n> cause him to double check that it is not a mistyped LHS of the push\n> refspec (if he is sending a non-commit) or mistyped RHS (if the ref\n> is pointing at a non-commit). If he _is_ trying to push a blob out,\n> the advice makes it clear what to do next: he does want to force it.\n\nYeah, I think that is sensible.\n\n> Note that you _could_ split the \"needs force\" case into two, namely,\n> \"cannot replace a non-commit\" and \"cannot push a non-commit\". You\n> could even further split them [...etc...]\n\nI do not think it is worth worrying too much about. This should really\nnot happen very often, and the user should be able to investigate and\nfigure out what is going on. I think making the error message extremely\nspecific is just going to end up making it harder to understand.\n\n> If we did that, then we could loosen the \"You should fetch first\"\n> case to say something like this:\n> \n> hint: you do not have the object at the tip of the remote ref;\n> hint: perhaps you want to pull from there first?\n\nYeah, better. I'll comment on the specific message you used in response\nto the patch itself.\n\n> This explicitly denies one of Chris's wish \"we shouldn't suggest to\n> merge something that we may not be able to\", but in the \"You should\n> fetch first\" case, we cannot fundamentally know if we can merge\n> until we fetch. I agree with you that the most common case is that\n> the unknown object is a commit, and that suggesting to pull is a\n> good compromise.\n\nI thought the wish was more about \"we shouldn't suggest to merge\nsomething we _know_ we will not be able to\", and you are still handling\nthat (i.e., the \"needs force\" case).\n\n-Peff\n"},{"id":"207668","messageId":"20130124065842.GC610@sigill.intra.peff.net","threadId":"32247","inReplyTo":"1358978130-12216-4-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-24T06:58:42Z","receivedAt":"2013-01-24T06:58:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 23, 2013 at 01:55:30PM -0800, Junio C Hamano wrote:\n\n> If we do not have the current object at the tip of the remote, we do\n> not even know that object, when fetched, is something that can be\n> merged. In such a case, suggesting to pull first just like\n> non-fast-forward case may not be technically correct, but in\n> practice, most such failures are seen when you try to push your work\n> to a branch without knowing that somebody else already pushed to\n> update the same branch since you forked, so \"pull first\" would work\n> as a suggestion most of the time.\n> \n> In these cases, the current code already rejects such a push on the\n> client end, but we used the same error and advice messages as the\n> ones used when rejecting a non-fast-forward push, i.e. pull from\n> there and integrate before pushing again. Introduce new\n> rejection reasons and reword the messages appropriately.\n\nSo obviously from our previous discussion, I agree with the general\nbehavior of this patch. Let me get nit-picky on the message itself,\nthough:\n\n> +static const char message_advice_ref_fetch_first[] =\n> +\tN_(\"Updates were rejected because you do not have the object at the tip\\n\"\n> +\t \"of the remote. You may want to first merge the remote changes (e.g.\\n\"\n> +\t \" 'git pull') before pushing again.\\n\"\n> +\t \"See the 'Note about fast-forwards' in 'git push --help' for details.\");\n> +\n\nThe condition that triggers this message is going to come up fairly\noften for new git users (e.g., anyone using a central repo model), which\nI think is why the original message_advice_pull_before_push has gotten\nso much attention. And in most cases, users will be seeing this message\nnow instead of \"pull before push\", because the common triggering cause\nis somebody else pushing unrelated work.\n\nThe existing message says:\n\n Updates were rejected because a pushed branch tip is behind its remote\n counterpart. Check out this branch and merge the remote changes\n (e.g. 'git pull') before pushing again.\n\nI wonder: will the new message be as comprehensible to a new user as the\nold?\n\nThey are quite similar, but something about the presence of the word\n\"behind\" in the latter makes me think it helps explain what is going on\na bit more. When I read the new one, my first question is \"why don't I\nhave that object?\". Of course, saying \"behind\" in this case would not be\nstrictly accurate, because we do not even know the remote has a commit.\n\nI wonder if we can reword it to explain more about why we do not have\nthe object, without getting too inaccurate. Something like:\n\n Updates were rejected because the remote contains objects that you do\n not have locally. This is usually caused by another repository pushing\n to the same ref. You may want to first merge the remote changes (e.g.,\n 'git pull') before pushing again.\n\nI was also tempted to s/objects/work/, which is more vague, but is less\njargon-y for new users who do not know how git works.\n\nAlso, how should this interact with the checkout-then-pull-then-push\nadvice? We make a distinction for the non-fastforward case between HEAD\nand other refs. Should we be making the same distinction here?\n\n-Peff\n"},{"id":"207695","messageId":"7vvcamcxct.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"20130124065842.GC610@sigill.intra.peff.net","subject":"Re: [PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T17:19:30Z","receivedAt":"2013-01-24T17:19:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wonder if we can reword it to explain more about why we do not have\n> the object, without getting too inaccurate. Something like:\n>\n> Updates were rejected because the remote contains objects that you do\n> not have locally. This is usually caused by another repository pushing\n> to the same ref. You may want to first merge the remote changes (e.g.,\n> 'git pull') before pushing again.\n>\n> I was also tempted to s/objects/work/, which is more vague, but is less\n> jargon-y for new users who do not know how git works.\n\nAfter all this is \"hint\", and there is a value in being more\napproachable at the cost of being less accurate, over being\nimpenetrable to achieve perfect correctness.\n\n> Also, how should this interact with the checkout-then-pull-then-push\n> advice? We make a distinction for the non-fastforward case between HEAD\n> and other refs. Should we be making the same distinction here?\n\nPossibly, but I am not among the people who cared most about the\ndistinction there; with the default behaviour switching to 'simple',\nthat distinction will start mattering even less, I suspect.\n"},{"id":"207729","messageId":"CAPig+cQL81tSWLz=QOOD-_2yws12jYLayQ8wvUaHXrURPBEFTw@mail.gmail.com","threadId":"32247","inReplyTo":"1358978130-12216-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v4 1/3] push: further clean up fields of \"struct ref\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-01-24T22:22:12Z","receivedAt":"2013-01-24T22:22:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jan 23, 2013 at 4:55 PM, Junio C Hamano wrote:\n> The \"nonfastforward\" and \"update\" fields are only used while\n> deciding what value to assign to the \"status\" locally in a single\n> function. Remove them from the \"struct ref\".\n>\n> The \"requires_force\" field is not used to decide if the proposed\n> update requires a --force option to succeed, or to record such a\n> decision made elsewhere. It is used by status reporting code that\n> the particular update was \"forced\". Rename it to \"forced_udpate\",\n\ns/udpate/update/\n"},{"id":"207754","messageId":"CAEUsAPYAikZUTf9OE=PoGBYot6Udnw9XTYDs6Ug7h=PWbCYM1Q@mail.gmail.com","threadId":"32247","inReplyTo":"1358978130-12216-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH v4 0/3] Finishing touches to \"push\" advises","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-25T04:31:43Z","receivedAt":"2013-01-25T04:31:43Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Wed, Jan 23, 2013 at 3:55 PM, Junio C Hamano wrote:\n> This builds on Chris Rorvick's earlier effort to forbid unforced\n> updates to refs/tags/ hierarchy and giving sensible error and advise\n> messages for that case (we are not rejecting such a push due to fast\n> forwardness, and suggesting to fetch and integrate before pushing\n> again does not make sense).\n\nFWIW, these changes look good to me. The logic in\nset_ref_status_for_push() is easier to follow and the additional error\nstatuses (and associated advice) make things much clearer.\n\nHad I written the the \"already exists\" advice in the context of these\nadditional statuses I would have said \"the destination *tag* reference\nalready exists\", or maybe even just \"the destination *tag* already\nexists\". It's probably fine the way it is, but I only avoided using\n\"tag\" in the advice because I was abusing it.\n\nThanks,\n\nChris\n"},{"id":"207759","messageId":"7va9rx7t0e.fsf@alter.siamese.dyndns.org","threadId":"32247","inReplyTo":"CAEUsAPYAikZUTf9OE=PoGBYot6Udnw9XTYDs6Ug7h=PWbCYM1Q@mail.gmail.com","subject":"Re: [PATCH v4 0/3] Finishing touches to \"push\" advises","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-25T05:04:33Z","receivedAt":"2013-01-25T05:04:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Rorvick writes:\n\n> Had I written the the \"already exists\" advice in the context of these\n> additional statuses I would have said \"the destination *tag* reference\n> already exists\", or maybe even just \"the destination *tag* already\n> exists\".\n\nYeah, now we do not use \"already exists\" for anything other than\nrefs/tags/, right? Your rewording sounds like the right thing to\nmake it even clearer.\n\nThanks for bringing it up. \n\nWould it be sufficient to do this? I think \"the tag already exists\nin the remote\" is already clear that we are talking about the\ndestination.\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex a2b3fbe..78789be 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =\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+\tN_(\"Updates were rejected because the tag already exists\\n\"\n \t \"in the remote.\");\n \n static const char message_advice_ref_needs_force[] =\n"},{"id":"207760","messageId":"CAEUsAPZfLafLUiQmk8GhF4hUHGDFP-4X85Nfv8Q7-hHy_Mp5OA@mail.gmail.com","threadId":"32247","inReplyTo":"7va9rx7t0e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 0/3] Finishing touches to \"push\" advises","fromName":"Chris Rorvick","fromEmail":"chris@rorvick.com","sentAt":"2013-01-25T05:14:41Z","receivedAt":"2013-01-25T05:14:41Z","isPatch":true,"sender":{"key":"chris@rorvick.com","avatar":"https://avatars.githubusercontent.com/u/824726?v=4"},"body":"On Thu, Jan 24, 2013 at 11:04 PM, Junio C Hamano wrote:\n> Would it be sufficient to do this? I think \"the tag already exists\n> in the remote\" is already clear that we are talking about the\n> destination.\n\nGood point.\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index a2b3fbe..78789be 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =\n> \"See the 'Note about fast-forwards' in 'git push --help' for details.\");\n>\n> static const char message_advice_ref_already_exists[] =\n> - N_(\"Updates were rejected because the destination reference already exists\\n\"\n> + N_(\"Updates were rejected because the tag already exists\\n\"\n> \"in the remote.\");\n>\n> static const char message_advice_ref_needs_force[] =\n\nLooks like the new-line is now unnecessary, but that looks good to me.\n\nThanks,\n\nChris\n"}]}