{"thread":{"id":"24883","subject":"[PATCH] push: disallow fast-forwarding tags without --force","startedAt":"2010-08-27T07:14:44Z","lastAt":"2010-09-01T15:18:55Z","messageCount":10,"participants":["Dave Olszewski","Junio C Hamano","Jonathan Nieder","Tay Ray Chuan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"149110","messageId":"1282893284-17829-1-git-send-email-cxreg@pobox.com","threadId":"24883","inReplyTo":null,"subject":"[PATCH] push: disallow fast-forwarding tags without --force","fromName":"Dave Olszewski","fromEmail":"cxreg@pobox.com","sentAt":"2010-08-27T07:14:44Z","receivedAt":"2010-08-27T07:14:44Z","isPatch":true,"sender":{"key":"cxreg@pobox.com","avatar":"https://avatars.githubusercontent.com/u/55474?v=4"},"body":"Generally, tags are considered a write-once ref (or object), and updates\nto them are the exception to the rule.  This is evident from the\nbehavior of \"git fetch\", which will not update a tag it already has\nunless --tags is specified, and from the --force option to \"git tag\".\n\nHowever, there is presently nothing preventing a tag from being\nfast-forwarded, which can happen intentionally or accidentally.  In both\ncases, the user should be aware that they are changing something that is\nexpected to be immutable and stable.\n\nThis change forces a user to specify \"git push --force\" to push a tag\nwhich points to a different object than it does on the upstream\nrepository, regardless of whether it's a fast-forward or not.\n\nThe config option receive.denyMovingTags can be set on the upstream\nrepository to disallow this, even with --force.\n\nSigned-off-by: Dave Olszewski <cxreg@pobox.com>\n---\n Documentation/config.txt               |    8 ++++++++\n Documentation/git-push.txt             |   21 ++++++++++++++++++---\n Documentation/git-receive-pack.txt     |    3 ++-\n advice.c                               |    2 ++\n advice.h                               |    1 +\n builtin/push.c                         |   10 ++++++++--\n builtin/receive-pack.c                 |   14 ++++++++++++++\n builtin/send-pack.c                    |    9 ++++++++-\n cache.h                                |    1 +\n contrib/completion/git-completion.bash |    1 +\n remote.c                               |   13 +++++++++++++\n t/t5400-send-pack.sh                   |   13 +++++++++++++\n transport-helper.c                     |    6 ++++++\n transport.c                            |   15 ++++++++++++---\n transport.h                            |    4 ++--\n 15 files changed, 109 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 05ec3fe..587cccd 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -122,6 +122,9 @@ advice.*::\n \tpushNonFastForward::\n \t\tAdvice shown when linkgit:git-push[1] refuses\n \t\tnon-fast-forward refs. Default: true.\n+\tpushMovingTag::\n+\t\tAdvice shown when linkgit:git-push[1] refuses\n+\t\tto push a changed tag. Default: true.\n \tstatusHints::\n \t\tDirections on how to stage/unstage/add shown in the\n \t\toutput of linkgit:git-status[1] and the template shown\n@@ -1593,6 +1596,11 @@ receive.denyNonFastForwards::\n \teven if that push is forced. This configuration variable is\n \tset when initializing a shared repository.\n \n+receive.denyMovingTags::\n+\tIf set to true, git-receive-pack will deny an update to a tag which\n+\talready points to a different object.  Use this to prevent such an\n+\tupdate via a push, even if that push is forced.\n+\n receive.updateserverinfo::\n \tIf set to true, git-receive-pack will run git-update-server-info\n \tafter receiving data from git-push and updating refs.\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 658ff2f..f7f8f17 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -112,7 +112,9 @@ nor in any Push line of the corresponding remotes file---see below).\n \tUsually, the command refuses to update a remote ref that is\n \tnot an ancestor of the local ref used to overwrite it.\n \tThis flag disables the check.  This can cause the\n-\tremote repository to lose commits; use it with care.\n+\tremote repository to lose commits; use it with care.  This\n+\tflag will also allow a previously pushed tag to be updated\n+\tto point to a new commit, which is refused by default.\n \n --repo=<repository>::\n \tThis option is only relevant if no <repository> argument is\n@@ -215,8 +217,9 @@ remote rejected::\n \tof the following safety options in effect:\n \t`receive.denyCurrentBranch` (for pushes to the checked out\n \tbranch), `receive.denyNonFastForwards` (for forced\n-\tnon-fast-forward updates), `receive.denyDeletes` or\n-\t`receive.denyDeleteCurrent`.  See linkgit:git-config[1].\n+\tnon-fast-forward updates), `receive.denyDeletes`,\n+\t`receive.denyDeleteCurrent`, or `receive.denyMovingTags`.  See\n+\tlinkgit:git-config[1].\n \n remote failure::\n \tThe remote end did not report the successful update of the ref,\n@@ -324,6 +327,18 @@ overwrite it. In other words, \"git push --force\" is a method reserved for\n a case where you do mean to lose history.\n \n \n+Note about moving tags\n+----------------------\n+\n+Tags are widely considered 'read-only', and are not expected to change.\n+See the 'On Re-tagging' section of linkgit:git-tag[1] to learn more about\n+why this is so.\n+\n+If you really need to change an already-propagated tag, you must force it\n+with \"git push --force\".  This can be overridden on the upstream repository\n+with receive.denyMovingTags.\n+\n+\n Examples\n --------\n \ndiff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\nindex 2790eeb..55f2830 100644\n--- a/Documentation/git-receive-pack.txt\n+++ b/Documentation/git-receive-pack.txt\n@@ -30,7 +30,8 @@ post-update hooks found in the Documentation/howto directory.\n \n 'git-receive-pack' honours the receive.denyNonFastForwards config\n option, which tells it if updates to a ref should be denied if they\n-are not fast-forwards.\n+are not fast-forwards, and the receive.denyMovingTags config option,\n+which disallows updating a tag to point to a new object.\n \n OPTIONS\n -------\ndiff --git a/advice.c b/advice.c\nindex 0be4b5f..51021d8 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n \n int advice_push_nonfastforward = 1;\n+int advice_push_moving_tag = 1;\n int advice_status_hints = 1;\n int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n@@ -12,6 +13,7 @@ static struct {\n \tint *preference;\n } advice_config[] = {\n \t{ \"pushnonfastforward\", &advice_push_nonfastforward },\n+\t{ \"pushmovingtag\", &advice_push_moving_tag},\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 3244ebb..8b1a33c 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -4,6 +4,7 @@\n #include \"git-compat-util.h\"\n \n extern int advice_push_nonfastforward;\n+extern int advice_push_moving_tag;\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 e655eb7..8547554 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -106,7 +106,7 @@ static void setup_default_push_refspecs(void)\n static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n-\tint nonfastforward;\n+\tint nonfastforward, moving_tag;\n \n \ttransport_set_verbosity(transport, verbosity, progress);\n \n@@ -119,7 +119,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     &nonfastforward, &moving_tag);\n \tif (err != 0)\n \t\terror(\"failed to push some refs to '%s'\", transport->url);\n \n@@ -134,6 +134,12 @@ static int push_with_options(struct transport *transport, int flags)\n \t\t\t\t\"'Note about fast-forwards' section of 'git push --help' for details.\\n\");\n \t}\n \n+\tif (moving_tag && advice_push_moving_tag) {\n+\t\tfprintf(stderr, \"A tag which already exists upstream was attempted to be pushed while\\n\"\n+\t\t\t\t\"pointing to a different object.  This is unsafe, and disabled by default.\\n\"\n+\t\t\t\t\"See the 'Note about moving tags' section of 'git push --help' for details.\\n\");\n+\t}\n+\n \treturn 1;\n }\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 760817d..1a96e55 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -22,6 +22,7 @@ enum deny_action {\n \n static int deny_deletes;\n static int deny_non_fast_forwards;\n+static int deny_moving_tags;\n static enum deny_action deny_current_branch = DENY_UNCONFIGURED;\n static enum deny_action deny_delete_current = DENY_UNCONFIGURED;\n static int receive_fsck_objects;\n@@ -63,6 +64,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.denymovingtags\") == 0) {\n+\t\tdeny_moving_tags = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.unpacklimit\") == 0) {\n \t\treceive_unpack_limit = git_config_int(var, value);\n \t\treturn 0;\n@@ -416,6 +422,14 @@ static const char *update(struct command *cmd)\n \t\t\treturn \"non-fast-forward\";\n \t\t}\n \t}\n+\tif (deny_moving_tags && !is_null_sha1(new_sha1) &&\n+\t    !is_null_sha1(old_sha1) &&\n+\t    hashcmp(old_sha1, new_sha1) &&\n+\t    !prefixcmp(name, \"refs/tags/\")) {\n+\t\trp_error(\"denying moving tag %s\", name);\n+\t\treturn \"moving tag\";\n+\t}\n+\n \tif (run_update_hook(cmd)) {\n \t\trp_error(\"hook declined to update %s\", name);\n \t\treturn \"hook declined\";\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 481602d..c41a455 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -198,6 +198,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_MOVING_TAG:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"moving tag\";\n+\t\t\tbreak;\n+\n \t\tcase REF_STATUS_REJECT_NODELETE:\n \t\tcase REF_STATUS_REMOTE_REJECT:\n \t\t\tres = \"error\";\n@@ -275,6 +280,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_MOVING_TAG:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\n@@ -395,6 +401,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tconst char *receivepack = \"git-receive-pack\";\n \tint flags;\n \tint nonfastforward = 0;\n+\tint moving_tag = 0;\n \n \targv++;\n \tfor (i = 1; i < argc; i++, argv++) {\n@@ -516,7 +523,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, &nonfastforward, &moving_tag);\n \n \tif (!args.dry_run && remote) {\n \t\tstruct ref *ref;\ndiff --git a/cache.h b/cache.h\nindex eb77e1d..cca7499 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -913,6 +913,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_MOVING_TAG,\n \t\tREF_STATUS_REJECT_NODELETE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 6756990..dc29e0b 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1964,6 +1964,7 @@ _git_config ()\n \t\treceive.denyCurrentBranch\n \t\treceive.denyDeletes\n \t\treceive.denyNonFastForwards\n+\t\treceive.denyMovingTags\n \t\treceive.fsckObjects\n \t\treceive.unpackLimit\n \t\trepack.usedeltabaseoffset\ndiff --git a/remote.c b/remote.c\nindex 9143ec7..cc7ce93 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1266,6 +1266,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\tcontinue;\n \t\t}\n \n+\t\t/* If a tag already exists on the remote and points to\n+\t\t * a different object, we don't want to push it again\n+\t\t * without requiring the user to indicate that they know\n+\t\t * what they are doing.\n+\t\t */\n+\t\tif (!prefixcmp(ref->name, \"refs/tags/\") &&\n+\t\t    !ref->deletion &&\n+\t\t    !is_null_sha1(ref->old_sha1) &&\n+\t\t    !ref->force &&\n+\t\t    !force_update) {\n+\t\t\tref->status = REF_STATUS_REJECT_MOVING_TAG;\n+\t\t}\n+\n \t\t/* This part determines what can overwrite what.\n \t\t * The rules are:\n \t\t *\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex c718253..573eb20 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -106,6 +106,19 @@ test_expect_success 'denyNonFastforwards trumps --force' '\n \ttest \"$victim_orig\" = \"$victim_head\"\n '\n \n+test_expect_success 'denyMovingTags trumps --force' '\n+\t(\n+\t    cd victim &&\n+\t    ( git tag moving_tag master^ || : ) &&\n+\t    git config receive.denyMovingTags true\n+\t) &&\n+\tgit tag moving_tag &&\n+\tvictim_orig=$(cd victim && git rev-parse --verify moving_tag) &&\n+\ttest_must_fail git send-pack --force ./victim moving_tag &&\n+\tvictim_tag=$(cd victim && git rev-parse --verify moving_tag) &&\n+\ttest \"$victim_orig\" = \"$victim_tag\"\n+'\n+\n test_expect_success 'push --all excludes remote tracking hierarchy' '\n \tmkdir parent &&\n \t(\ndiff --git a/transport-helper.c b/transport-helper.c\nindex acfc88e..e9730a0 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -575,6 +575,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_MOVING_TAG:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\n@@ -655,6 +656,11 @@ static int push_refs_with_push(struct transport *transport,\n \t\t\t\tfree(msg);\n \t\t\t\tmsg = NULL;\n \t\t\t}\n+\t\t\telse if (!strcmp(msg, \"moving tag\")) {\n+\t\t\t\tstatus = REF_STATUS_REJECT_MOVING_TAG;\n+\t\t\t\tfree(msg);\n+\t\t\t\tmsg = NULL;\n+\t\t\t}\n \t\t}\n \n \t\tif (ref)\ndiff --git a/transport.c b/transport.c\nindex 4dba6f8..e3b2ee8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -692,6 +692,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_MOVING_TAG:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"moving tag\", 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@@ -711,7 +715,8 @@ 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,\n+\t\t\t\t  int *nonfastforward, int *moving_tag)\n {\n \tstruct ref *ref;\n \tint n = 0;\n@@ -727,6 +732,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n \n \t*nonfastforward = 0;\n+\t*moving_tag = 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@@ -734,6 +740,8 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n \t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)\n \t\t\t*nonfastforward = 1;\n+\t\tif (ref->status == REF_STATUS_REJECT_MOVING_TAG)\n+\t\t\t*moving_tag = 1;\n \t}\n }\n \n@@ -1004,9 +1012,10 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\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   int *nonfastforward, int *moving_tag)\n {\n \t*nonfastforward = 0;\n+\t*moving_tag = 0;\n \ttransport_verify_remote_names(refspec_nr, refspec);\n \n \tif (transport->push) {\n@@ -1047,7 +1056,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\tnonfastforward, moving_tag);\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 c59d973..8e95e09 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -138,7 +138,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\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   int *nonfastforward, int *moving_tag);\n \n const struct ref *transport_get_remote_refs(struct transport *transport);\n \n@@ -163,6 +163,6 @@ 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, int *nonfastforward, int *moving_tag);\n \n #endif\n-- \n1.7.2.2.176.g3e15d\n"},{"id":"149131","messageId":"7vfwy0hsn1.fsf@alter.siamese.dyndns.org","threadId":"24883","inReplyTo":"1282893284-17829-1-git-send-email-cxreg@pobox.com","subject":"Re: [PATCH] push: disallow fast-forwarding tags without --force","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-27T17:28:34Z","receivedAt":"2010-08-27T17:28:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Olszewski <cxreg@pobox.com> writes:\n\n> Generally, tags are considered a write-once ref (or object), and updates\n> to them are the exception to the rule.  This is evident from the\n> behavior of \"git fetch\", which will not update a tag it already has\n> unless --tags is specified, and from the --force option to \"git tag\".\n\nThe title and what you describe later in your proposed log message do not\nmatch.  This is about \"push: disallow updating an existing tag by default\"\nisn't it?\n\nThis proposes a big change in the policy, and I do not like it starting\nout as the new default to forbid people from doing something they have\nbeen allowed to do for a long time.  I recall hearing some people auto\ntagging the latest version their autobuilder/tester tested successfully\nand updating the same tag nightly---your change will break their cron\nscript, no?\n\nIf you ship the feature disabled by default first, it will still allow\npeople to take advantage of it by simply flipping the feature on, instead\nof having to install their own update hook.  In a later version, if and\nwhen enough people agree that this should be on by default, we can do so\nat a version bump.\n"},{"id":"149136","messageId":"alpine.DEB.2.00.1008271046080.20874@narbuckle.genericorp.net","threadId":"24883","inReplyTo":"7vfwy0hsn1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] push: disallow fast-forwarding tags without --force","fromName":"Dave Olszewski","fromEmail":"cxreg@pobox.com","sentAt":"2010-08-27T18:01:16Z","receivedAt":"2010-08-27T18:01:16Z","isPatch":true,"sender":{"key":"cxreg@pobox.com","avatar":"https://avatars.githubusercontent.com/u/55474?v=4"},"body":"On Fri, 27 Aug 2010, Junio C Hamano wrote:\n\n> Dave Olszewski <cxreg@pobox.com> writes:\n> \n> > Generally, tags are considered a write-once ref (or object), and updates\n> > to them are the exception to the rule.  This is evident from the\n> > behavior of \"git fetch\", which will not update a tag it already has\n> > unless --tags is specified, and from the --force option to \"git tag\".\n> \n> The title and what you describe later in your proposed log message do not\n> match.  This is about \"push: disallow updating an existing tag by default\"\n> isn't it?\n\nI don't see a major difference in meaning, since only fast-forwards are\nallowed without --force, but you're right about my intent.  I'm happy to\nchange the commit message.\n\n> This proposes a big change in the policy, and I do not like it starting\n> out as the new default to forbid people from doing something they have\n> been allowed to do for a long time.  I recall hearing some people auto\n> tagging the latest version their autobuilder/tester tested successfully\n> and updating the same tag nightly---your change will break their cron\n> script, no?\n> \n> If you ship the feature disabled by default first, it will still allow\n> people to take advantage of it by simply flipping the feature on, instead\n> of having to install their own update hook.  In a later version, if and\n> when enough people agree that this should be on by default, we can do so\n> at a version bump.\n\nYes that's true.  I suspected based on the lack of any documentation or\ntests promising such behavior, the inclination for git to want to\npretend that changed tags haven't changed, and the social stigma against\nit, that this was a bug.\n\nIt's trivial for someone to update build software from \"git push remote\ntag\" to \"git push remote +tag\" or \"git push -f remote tag\", but I can\nunderstand your objection.  It's the reason I didn't also add\nreceive.denyMovingTags to the default config for bare repositories.\n\nIt seems unlikely that many people are ever going to \"flip on\" this\nfeature; either they won't know about it (and for them, it should be\non), or they'll have a reason to move a tag, and want it off.  That, and\nmy perception that this was unintentional behavior, is the reason I\npatched it to be default.\n\nHowever, If you still feel strongly about this, I can rework it to be\noptional.  Thanks for your feedback!\n\n    Dave\n"},{"id":"149148","messageId":"20100828012101.GB2004@burratino","threadId":"24883","inReplyTo":"alpine.DEB.2.00.1008271046080.20874@narbuckle.genericorp.net","subject":"Re: [PATCH] push: disallow fast-forwarding tags without --force","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-28T01:21:01Z","receivedAt":"2010-08-28T01:21:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDave Olszewski wrote:\n\n> It's trivial for someone to update build software from \"git push remote\n> tag\" to \"git push remote +tag\" or \"git push -f remote tag\", but I can\n> understand your objection.\n\nRight.  The thing to prevent is unhappy surprises: it is best if\nusers upgrading get a chance to update their scripts before it matters.\n\n> It seems unlikely that many people are ever going to \"flip on\" this\n> feature; either they won't know about it (and for them, it should be\n> on), or they'll have a reason to move a tag, and want it off.\n\nThis is why a switch of some kind is useful: after reading the release\nnotes, a user can flip the switch for a glimpse of the future, forsee\nthe upcoming disaster, and fix everything up before it really matters.\nAfter the default changes, the switch has the opposite purpose: users\nwho were not prepared can use it to turn back time and avoid having\nto change their code.\n\nSo the deprecation process for unwanted features tends to look like\nthis:\n\n 1. complain about use of the feature, with an option to suppress\n    the warnings.  or: loudly proclaim that the feature is going\n    away in release notes\n\n 2. add an option to disable the feature, to help people transition\n\n 3. change the default to true\n\n 4. remove the option\n\nStep 1 is the most important one imho.  See\nDocumentation/RelNotes-1.6.6.txt for an example.\n\nI don't think we've ever reached step 4, but we should try it some\ntime.\n\nHope that helps,\nJonathan\n"},{"id":"149155","messageId":"1282983736-3233-1-git-send-email-cxreg@pobox.com","threadId":"24883","inReplyTo":"20100828012101.GB2004@burratino","subject":"[PATCH] push: warn users about updating existing tags on push","fromName":"Dave Olszewski","fromEmail":"cxreg@pobox.com","sentAt":"2010-08-28T08:22:16Z","receivedAt":"2010-08-28T08:22:16Z","isPatch":true,"sender":{"key":"cxreg@pobox.com","avatar":"https://avatars.githubusercontent.com/u/55474?v=4"},"body":"Generally, tags are considered a write-once ref (or object), and updates\nto them are the exception to the rule.  This is evident from the\nbehavior of \"git fetch\", which will not update a tag it already has\nunless --tags is specified, from the --force option to \"git tag\", and\nthe fact that Git does not keep reflogs for tags.\n\nHowever, there is presently nothing preventing a tag from being\nfast-forwarded, which can happen intentionally or accidentally.  In both\ncases, the user should be aware that they are changing something that is\nexpected to be immutable and stable.\n\nThis change adds a warning when a user pushes a tag which points to a\ndifferent object than it does on the upstream repository.  If the user\nspecifies \"git push --force\", the action is determined to be intentional\nand the warning is not shown.\n\nIf the user wishes to have these pushes refused instead of simply\nwarning them, they can set push.denyMovingTags to true.  This new option\ndefaults to false, but might later default to true.\n\nThe config option receive.denyMovingTags can be set on the upstream\nrepository to disallow this, even with --force.\n\nSigned-off-by: Dave Olszewski <cxreg@pobox.com>\n---\n Documentation/config.txt               |   12 ++++++++++++\n Documentation/git-push.txt             |   25 ++++++++++++++++++++++---\n Documentation/git-receive-pack.txt     |    3 ++-\n advice.c                               |    2 ++\n advice.h                               |    1 +\n builtin/push.c                         |   11 +++++++++--\n builtin/receive-pack.c                 |   14 ++++++++++++++\n builtin/send-pack.c                    |    9 ++++++++-\n cache.h                                |    1 +\n contrib/completion/git-completion.bash |    2 ++\n remote.c                               |   31 +++++++++++++++++++++++++++++++\n t/t5400-send-pack.sh                   |   14 ++++++++++++++\n transport-helper.c                     |    6 ++++++\n transport.c                            |   15 ++++++++++++---\n transport.h                            |    4 ++--\n 15 files changed, 138 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 05ec3fe..02dfc96 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -122,6 +122,9 @@ advice.*::\n \tpushNonFastForward::\n \t\tAdvice shown when linkgit:git-push[1] refuses\n \t\tnon-fast-forward refs. Default: true.\n+\tpushMovingTags::\n+\t\tAdvice shown when linkgit:git-push[1] refuses\n+\t\tto push a changed tag. Default: true.\n \tstatusHints::\n \t\tDirections on how to stage/unstage/add shown in the\n \t\toutput of linkgit:git-status[1] and the template shown\n@@ -1545,6 +1548,10 @@ push.default::\n * `tracking` push the current branch to its upstream branch.\n * `current` push the current branch to a branch of the same name.\n \n+push.denyMovingTags::\n+\tWhether or not a user will be allowed to push a tag that already\n+\texists on the remote for a different object.  False by default.\n+\n rebase.stat::\n \tWhether to show a diffstat of what changed upstream since the last\n \trebase. False by default.\n@@ -1593,6 +1600,11 @@ receive.denyNonFastForwards::\n \teven if that push is forced. This configuration variable is\n \tset when initializing a shared repository.\n \n+receive.denyMovingTags::\n+\tIf set to true, git-receive-pack will deny an update to a tag which\n+\talready points to a different object.  Use this to prevent such an\n+\tupdate via a push, even if that push is forced.\n+\n receive.updateserverinfo::\n \tIf set to true, git-receive-pack will run git-update-server-info\n \tafter receiving data from git-push and updating refs.\ndiff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\nindex 658ff2f..1d53e04 100644\n--- a/Documentation/git-push.txt\n+++ b/Documentation/git-push.txt\n@@ -112,7 +112,10 @@ nor in any Push line of the corresponding remotes file---see below).\n \tUsually, the command refuses to update a remote ref that is\n \tnot an ancestor of the local ref used to overwrite it.\n \tThis flag disables the check.  This can cause the\n-\tremote repository to lose commits; use it with care.\n+\tremote repository to lose commits; use it with care.  This\n+\tflag will also allow a previously pushed tag to be updated\n+\tto point to a new commit, which is refused if\n+\tpush.denyMovingTags is set to true.\n \n --repo=<repository>::\n \tThis option is only relevant if no <repository> argument is\n@@ -215,8 +218,9 @@ remote rejected::\n \tof the following safety options in effect:\n \t`receive.denyCurrentBranch` (for pushes to the checked out\n \tbranch), `receive.denyNonFastForwards` (for forced\n-\tnon-fast-forward updates), `receive.denyDeletes` or\n-\t`receive.denyDeleteCurrent`.  See linkgit:git-config[1].\n+\tnon-fast-forward updates), `receive.denyDeletes`,\n+\t`receive.denyDeleteCurrent`, or `receive.denyMovingTags`.  See\n+\tlinkgit:git-config[1].\n \n remote failure::\n \tThe remote end did not report the successful update of the ref,\n@@ -324,6 +328,21 @@ overwrite it. In other words, \"git push --force\" is a method reserved for\n a case where you do mean to lose history.\n \n \n+Note about moving tags\n+----------------------\n+\n+Tags are widely considered 'read-only', and are not expected to change.\n+See the 'On Re-tagging' section of linkgit:git-tag[1] to learn more about\n+why this is so.\n+\n+In a future version of Git, pushing a tag which points to a different\n+object than the one on the remote may be be disallowed by default.\n+Presently you can configure that such a push will be rejected with\n+push.denyMovingTags.  If this is set to true, and you really need to change\n+an already-propagated tag, you must force it with \"git push --force\".  This\n+can be overridden on the upstream repository with receive.denyMovingTags.\n+\n+\n Examples\n --------\n \ndiff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt\nindex 2790eeb..55f2830 100644\n--- a/Documentation/git-receive-pack.txt\n+++ b/Documentation/git-receive-pack.txt\n@@ -30,7 +30,8 @@ post-update hooks found in the Documentation/howto directory.\n \n 'git-receive-pack' honours the receive.denyNonFastForwards config\n option, which tells it if updates to a ref should be denied if they\n-are not fast-forwards.\n+are not fast-forwards, and the receive.denyMovingTags config option,\n+which disallows updating a tag to point to a new object.\n \n OPTIONS\n -------\ndiff --git a/advice.c b/advice.c\nindex 0be4b5f..51021d8 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -1,6 +1,7 @@\n #include \"cache.h\"\n \n int advice_push_nonfastforward = 1;\n+int advice_push_moving_tag = 1;\n int advice_status_hints = 1;\n int advice_commit_before_merge = 1;\n int advice_resolve_conflict = 1;\n@@ -12,6 +13,7 @@ static struct {\n \tint *preference;\n } advice_config[] = {\n \t{ \"pushnonfastforward\", &advice_push_nonfastforward },\n+\t{ \"pushmovingtag\", &advice_push_moving_tag},\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 3244ebb..8b1a33c 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -4,6 +4,7 @@\n #include \"git-compat-util.h\"\n \n extern int advice_push_nonfastforward;\n+extern int advice_push_moving_tag;\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 e655eb7..b04adbd 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -106,7 +106,7 @@ static void setup_default_push_refspecs(void)\n static int push_with_options(struct transport *transport, int flags)\n {\n \tint err;\n-\tint nonfastforward;\n+\tint nonfastforward, moving_tag;\n \n \ttransport_set_verbosity(transport, verbosity, progress);\n \n@@ -119,7 +119,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     &nonfastforward, &moving_tag);\n \tif (err != 0)\n \t\terror(\"failed to push some refs to '%s'\", transport->url);\n \n@@ -134,6 +134,13 @@ static int push_with_options(struct transport *transport, int flags)\n \t\t\t\t\"'Note about fast-forwards' section of 'git push --help' for details.\\n\");\n \t}\n \n+\tif (moving_tag && advice_push_moving_tag) {\n+\t\tfprintf(stderr, \"A tag which already exists upstream was attempted to be pushed while\\n\"\n+\t\t\t\t\"pointing to a different object.  This is currently disabled by\\n\"\n+\t\t\t\t\"push.denyMovingTags.  See the 'Note about moving tags' section of\\n\"\n+\t\t\t\t\"'git push --help' for details.\\n\");\n+\t}\n+\n \treturn 1;\n }\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 760817d..1a96e55 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -22,6 +22,7 @@ enum deny_action {\n \n static int deny_deletes;\n static int deny_non_fast_forwards;\n+static int deny_moving_tags;\n static enum deny_action deny_current_branch = DENY_UNCONFIGURED;\n static enum deny_action deny_delete_current = DENY_UNCONFIGURED;\n static int receive_fsck_objects;\n@@ -63,6 +64,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.denymovingtags\") == 0) {\n+\t\tdeny_moving_tags = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (strcmp(var, \"receive.unpacklimit\") == 0) {\n \t\treceive_unpack_limit = git_config_int(var, value);\n \t\treturn 0;\n@@ -416,6 +422,14 @@ static const char *update(struct command *cmd)\n \t\t\treturn \"non-fast-forward\";\n \t\t}\n \t}\n+\tif (deny_moving_tags && !is_null_sha1(new_sha1) &&\n+\t    !is_null_sha1(old_sha1) &&\n+\t    hashcmp(old_sha1, new_sha1) &&\n+\t    !prefixcmp(name, \"refs/tags/\")) {\n+\t\trp_error(\"denying moving tag %s\", name);\n+\t\treturn \"moving tag\";\n+\t}\n+\n \tif (run_update_hook(cmd)) {\n \t\trp_error(\"hook declined to update %s\", name);\n \t\treturn \"hook declined\";\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 481602d..c41a455 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -198,6 +198,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_MOVING_TAG:\n+\t\t\tres = \"error\";\n+\t\t\tmsg = \"moving tag\";\n+\t\t\tbreak;\n+\n \t\tcase REF_STATUS_REJECT_NODELETE:\n \t\tcase REF_STATUS_REMOTE_REJECT:\n \t\t\tres = \"error\";\n@@ -275,6 +280,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_MOVING_TAG:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\n@@ -395,6 +401,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)\n \tconst char *receivepack = \"git-receive-pack\";\n \tint flags;\n \tint nonfastforward = 0;\n+\tint moving_tag = 0;\n \n \targv++;\n \tfor (i = 1; i < argc; i++, argv++) {\n@@ -516,7 +523,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, &nonfastforward, &moving_tag);\n \n \tif (!args.dry_run && remote) {\n \t\tstruct ref *ref;\ndiff --git a/cache.h b/cache.h\nindex eb77e1d..cca7499 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -913,6 +913,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_MOVING_TAG,\n \t\tREF_STATUS_REJECT_NODELETE,\n \t\tREF_STATUS_UPTODATE,\n \t\tREF_STATUS_REMOTE_REJECT,\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 6756990..5a38473 100755\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1960,10 +1960,12 @@ _git_config ()\n \t\tpull.octopus\n \t\tpull.twohead\n \t\tpush.default\n+\t\tpush.denyMovingTags\n \t\trebase.stat\n \t\treceive.denyCurrentBranch\n \t\treceive.denyDeletes\n \t\treceive.denyNonFastForwards\n+\t\treceive.denyMovingTags\n \t\treceive.fsckObjects\n \t\treceive.unpackLimit\n \t\trepack.usedeltabaseoffset\ndiff --git a/remote.c b/remote.c\nindex 9143ec7..fbca1e6 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -50,6 +50,8 @@ static int explicit_default_remote_name;\n static struct rewrites rewrites;\n static struct rewrites rewrites_push;\n \n+static int deny_moving_tags;\n+\n #define BUF_SIZE (2048)\n static char buffer[BUF_SIZE];\n \n@@ -385,6 +387,10 @@ static int handle_config(const char *key, const char *value, void *cb)\n \t\t\tadd_instead_of(rewrite, xstrdup(value));\n \t\t}\n \t}\n+\tif (!strcmp(key, \"push.denymovingtags\")) {\n+\t\tdeny_moving_tags = git_config_bool(key, value);\n+\t\treturn 0;\n+\t}\n \tif (prefixcmp(key,  \"remote.\"))\n \t\treturn 0;\n \tname = key + 7;\n@@ -1266,6 +1272,31 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n \t\t\tcontinue;\n \t\t}\n \n+\t\t/* If a tag already exists on the remote and points to\n+\t\t * a different object, we don't want to push it again\n+\t\t * without requiring the user to indicate that they know\n+\t\t * what they are doing.\n+\t\t */\n+\t\tif (!prefixcmp(ref->name, \"refs/tags/\") &&\n+\t\t    !ref->deletion &&\n+\t\t    !is_null_sha1(ref->old_sha1)) {\n+\t\t\tif (deny_moving_tags) {\n+\t\t\t\t/* Set `nonfastforward` for the sake of displaying\n+\t\t\t\t * this update as forced\n+\t\t\t\t */\n+\t\t\t\tref->nonfastforward = 1;\n+\t\t\t\tif (!ref->force && !force_update) {\n+\t\t\t\t\tref->status = REF_STATUS_REJECT_MOVING_TAG;\n+\t\t\t\t}\n+\t\t\t} else {\n+\t\t\t\tif (!ref->force && !force_update)\n+\t\t\t\t\twarning(\"You are changing the value of an upstream tag.  This may\\n\"\n+\t\t\t\t\t\t\"be deprecated in a future version of Git.  Please use --force\\n\"\n+\t\t\t\t\t\t\"if this was intentional, and consider setting push.denyMovingTags.\");\n+\t\t\t}\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 *\ndiff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\nindex c718253..7906ba5 100755\n--- a/t/t5400-send-pack.sh\n+++ b/t/t5400-send-pack.sh\n@@ -106,6 +106,20 @@ test_expect_success 'denyNonFastforwards trumps --force' '\n \ttest \"$victim_orig\" = \"$victim_head\"\n '\n \n+test_expect_success 'denyMovingTags trumps --force' '\n+\t(\n+\t    cd victim &&\n+\t    ( git tag moving_tag master^ || : ) &&\n+\t    git config receive.denyMovingTags true\n+\t) &&\n+\tgit tag moving_tag &&\n+\tgit config push.denyMovingTags true &&\n+\tvictim_orig=$(cd victim && git rev-parse --verify moving_tag) &&\n+\ttest_must_fail git send-pack --force ./victim moving_tag &&\n+\tvictim_tag=$(cd victim && git rev-parse --verify moving_tag) &&\n+\ttest \"$victim_orig\" = \"$victim_tag\"\n+'\n+\n test_expect_success 'push --all excludes remote tracking hierarchy' '\n \tmkdir parent &&\n \t(\ndiff --git a/transport-helper.c b/transport-helper.c\nindex acfc88e..e9730a0 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -575,6 +575,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_MOVING_TAG:\n \t\tcase REF_STATUS_UPTODATE:\n \t\t\tcontinue;\n \t\tdefault:\n@@ -655,6 +656,11 @@ static int push_refs_with_push(struct transport *transport,\n \t\t\t\tfree(msg);\n \t\t\t\tmsg = NULL;\n \t\t\t}\n+\t\t\telse if (!strcmp(msg, \"moving tag\")) {\n+\t\t\t\tstatus = REF_STATUS_REJECT_MOVING_TAG;\n+\t\t\t\tfree(msg);\n+\t\t\t\tmsg = NULL;\n+\t\t\t}\n \t\t}\n \n \t\tif (ref)\ndiff --git a/transport.c b/transport.c\nindex 4dba6f8..e3b2ee8 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -692,6 +692,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_MOVING_TAG:\n+\t\tprint_ref_status('!', \"[rejected]\", ref, ref->peer_ref,\n+\t\t\t\t\t\t \"moving tag\", 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@@ -711,7 +715,8 @@ 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,\n+\t\t\t\t  int *nonfastforward, int *moving_tag)\n {\n \tstruct ref *ref;\n \tint n = 0;\n@@ -727,6 +732,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n \n \t*nonfastforward = 0;\n+\t*moving_tag = 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@@ -734,6 +740,8 @@ void transport_print_push_status(const char *dest, struct ref *refs,\n \t\t\tn += print_one_push_status(ref, dest, n, porcelain);\n \t\tif (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)\n \t\t\t*nonfastforward = 1;\n+\t\tif (ref->status == REF_STATUS_REJECT_MOVING_TAG)\n+\t\t\t*moving_tag = 1;\n \t}\n }\n \n@@ -1004,9 +1012,10 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\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   int *nonfastforward, int *moving_tag)\n {\n \t*nonfastforward = 0;\n+\t*moving_tag = 0;\n \ttransport_verify_remote_names(refspec_nr, refspec);\n \n \tif (transport->push) {\n@@ -1047,7 +1056,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\tnonfastforward, moving_tag);\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 c59d973..8e95e09 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -138,7 +138,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,\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   int *nonfastforward, int *moving_tag);\n \n const struct ref *transport_get_remote_refs(struct transport *transport);\n \n@@ -163,6 +163,6 @@ 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, int *nonfastforward, int *moving_tag);\n \n #endif\n-- \n1.7.2.2.179.g90aca\n"},{"id":"149289","messageId":"7v7hj8frxg.fsf@alter.siamese.dyndns.org","threadId":"24883","inReplyTo":"1282983736-3233-1-git-send-email-cxreg@pobox.com","subject":"Re: [PATCH] push: warn users about updating existing tags on push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-30T08:03:39Z","receivedAt":"2010-08-30T08:03:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Olszewski <cxreg@pobox.com> writes:\n\n> Generally, tags are considered a write-once ref (or object), and updates\n> to them are the exception to the rule.\n\nThis may be just the naming issue and you could say \"moving them\",\n\"updates to them\" or \"changing them\" interchangeably in the above;\namong them, \"updates to them\" sounds the most natural.\n\nCan you change the \"moving\" in the patch to make them consistent with the\nabove description?\n\n> However, there is presently nothing preventing a tag from being\n> fast-forwarded, which can happen intentionally or accidentally.\n> ... the user should be aware that they are changing something that is\n> expected to be immutable and stable.\n\nI actually think prevention of non-fast-forward updates for tags actually\nis a misfeature that didn't even come from any concious design; the check\nfor fast-forwarding refs was to make sure we do not lose histories from\nbranches.  IOW, I would say this would have been a good feature if things\nwere like this from day one.\n\n> diff --git a/remote.c b/remote.c\n> index 9143ec7..fbca1e6 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -50,6 +50,8 @@ static int explicit_default_remote_name;\n>  static struct rewrites rewrites;\n>  static struct rewrites rewrites_push;\n>  \n> +static int deny_moving_tags;\n> +\n>  #define BUF_SIZE (2048)\n>  static char buffer[BUF_SIZE];\n>  \n> @@ -385,6 +387,10 @@ static int handle_config(const char *key, const char *value, void *cb)\n>  \t\t\tadd_instead_of(rewrite, xstrdup(value));\n>  \t\t}\n>  \t}\n> +\tif (!strcmp(key, \"push.denymovingtags\")) {\n> +\t\tdeny_moving_tags = git_config_bool(key, value);\n> +\t\treturn 0;\n> +\t}\n\nHmm, shouldn't this be per-remote (rather, shouldn't a per-remote variant\nbe allowed to override this)?\n\n> @@ -1266,6 +1272,31 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n>  \t\t\tcontinue;\n>  \t\t}\n>  \n> +\t\t/* If a tag already exists on the remote and points to\n> +\t\t * a different object, we don't want to push it again\n> +\t\t * without requiring the user to indicate that they know\n> +\t\t * what they are doing.\n> +\t\t */\n\n\t/*\n         * We try to format\n         * multi-line comment\n         * like this.\n         */\n\n> +\t\tif (!prefixcmp(ref->name, \"refs/tags/\") &&\n> +\t\t    !ref->deletion &&\n> +\t\t    !is_null_sha1(ref->old_sha1)) {\n> +\t\t\tif (deny_moving_tags) {\n> +\t\t\t\t/* Set `nonfastforward` for the sake of displaying\n> +\t\t\t\t * this update as forced\n> +\t\t\t\t */\n> +\t\t\t\tref->nonfastforward = 1;\n\nI think you are propagating this bit to print_ok_ref_status() in\ntransport.c; it indicates that after your change, \"nonfastforward\" does\nnot mean non-fast-forward anymore, doesn't it?\n\nPerhaps the bit needs to be renamed to \"update_forced\" or something?\n\n> +\t\t\t\tif (!ref->force && !force_update) {\n> +\t\t\t\t\tref->status = REF_STATUS_REJECT_MOVING_TAG;\n> +\t\t\t\t}\n> +\t\t\t} else {\n> +\t\t\t\tif (!ref->force && !force_update)\n> +\t\t\t\t\twarning(\"You are changing the value of an upstream tag.  This may\\n\"\n> +\t\t\t\t\t\t\"be deprecated in a future version of Git.  Please use --force\\n\"\n> +\t\t\t\t\t\t\"if this was intentional, and consider setting push.denyMovingTags.\");\n> +\t\t\t}\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\nYou are changing the rule that determine what can overwrite what, aren't\nyou?  It is Ok (although it is in general frowned upon if you do so when\nyou do not have to) to add your new rule before an existing rule, but your\nrule should be added as a new rule to the enumeration in the comment, and\nthe code that implements the new rule after the comment, no?\n\n> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\n> index c718253..7906ba5 100755\n> --- a/t/t5400-send-pack.sh\n> +++ b/t/t5400-send-pack.sh\n> @@ -106,6 +106,20 @@ test_expect_success 'denyNonFastforwards trumps --force' '\n>  \ttest \"$victim_orig\" = \"$victim_head\"\n>  '\n>  \n> +test_expect_success 'denyMovingTags trumps --force' '\n> +\t(\n> +\t    cd victim &&\n> +\t    ( git tag moving_tag master^ || : ) &&\n\nIn which circumstance is it allowed for this \"git tag\" command to\nfail and the entire test to succeed?\n"},{"id":"149346","messageId":"alpine.DEB.2.00.1008300924550.20874@narbuckle.genericorp.net","threadId":"24883","inReplyTo":"7v7hj8frxg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] push: warn users about updating existing tags on push","fromName":"Dave Olszewski","fromEmail":"cxreg@pobox.com","sentAt":"2010-08-30T16:38:59Z","receivedAt":"2010-08-30T16:38:59Z","isPatch":true,"sender":{"key":"cxreg@pobox.com","avatar":"https://avatars.githubusercontent.com/u/55474?v=4"},"body":"On Mon, 30 Aug 2010, Junio C Hamano wrote:\n\nThanks for the critique and comments\n\n> Dave Olszewski <cxreg@pobox.com> writes:\n> \n> > Generally, tags are considered a write-once ref (or object), and updates\n> > to them are the exception to the rule.\n> \n> This may be just the naming issue and you could say \"moving them\",\n> \"updates to them\" or \"changing them\" interchangeably in the above;\n> among them, \"updates to them\" sounds the most natural.\n> \n> Can you change the \"moving\" in the patch to make them consistent with the\n> above description?\n\nSure, no problem.  Would you like this changed in the variable and\nconfig names as well, or just the printed text?\n\n\n> > diff --git a/remote.c b/remote.c\n> > index 9143ec7..fbca1e6 100644\n> > --- a/remote.c\n> > +++ b/remote.c\n> > @@ -50,6 +50,8 @@ static int explicit_default_remote_name;\n> >  static struct rewrites rewrites;\n> >  static struct rewrites rewrites_push;\n> >  \n> > +static int deny_moving_tags;\n> > +\n> >  #define BUF_SIZE (2048)\n> >  static char buffer[BUF_SIZE];\n> >  \n> > @@ -385,6 +387,10 @@ static int handle_config(const char *key, const char *value, void *cb)\n> >  \t\t\tadd_instead_of(rewrite, xstrdup(value));\n> >  \t\t}\n> >  \t}\n> > +\tif (!strcmp(key, \"push.denymovingtags\")) {\n> > +\t\tdeny_moving_tags = git_config_bool(key, value);\n> > +\t\treturn 0;\n> > +\t}\n> \n> Hmm, shouldn't this be per-remote (rather, shouldn't a per-remote variant\n> be allowed to override this)?\n\nI wasn't sure about this.  I like the idea of a single setting with\nper-remote override, I'll implement that.\n\n\n> > @@ -1266,6 +1272,31 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,\n> >  \t\t\tcontinue;\n> >  \t\t}\n> >  \n> > +\t\t/* If a tag already exists on the remote and points to\n> > +\t\t * a different object, we don't want to push it again\n> > +\t\t * without requiring the user to indicate that they know\n> > +\t\t * what they are doing.\n> > +\t\t */\n> \n> \t/*\n>          * We try to format\n>          * multi-line comment\n>          * like this.\n>          */\n\nOk.\n\n\n> > +\t\tif (!prefixcmp(ref->name, \"refs/tags/\") &&\n> > +\t\t    !ref->deletion &&\n> > +\t\t    !is_null_sha1(ref->old_sha1)) {\n> > +\t\t\tif (deny_moving_tags) {\n> > +\t\t\t\t/* Set `nonfastforward` for the sake of displaying\n> > +\t\t\t\t * this update as forced\n> > +\t\t\t\t */\n> > +\t\t\t\tref->nonfastforward = 1;\n> \n> I think you are propagating this bit to print_ok_ref_status() in\n> transport.c; it indicates that after your change, \"nonfastforward\" does\n> not mean non-fast-forward anymore, doesn't it?\n> \n> Perhaps the bit needs to be renamed to \"update_forced\" or something?\n\nGood point.  I arrived at making this change pretty late in the patch\nand didn't consider the rename.  Thanks.\n\n\n> > +\t\t\t\tif (!ref->force && !force_update) {\n> > +\t\t\t\t\tref->status = REF_STATUS_REJECT_MOVING_TAG;\n> > +\t\t\t\t}\n> > +\t\t\t} else {\n> > +\t\t\t\tif (!ref->force && !force_update)\n> > +\t\t\t\t\twarning(\"You are changing the value of an upstream tag.  This may\\n\"\n> > +\t\t\t\t\t\t\"be deprecated in a future version of Git.  Please use --force\\n\"\n> > +\t\t\t\t\t\t\"if this was intentional, and consider setting push.denyMovingTags.\");\n> > +\t\t\t}\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> \n> You are changing the rule that determine what can overwrite what, aren't\n> you?  It is Ok (although it is in general frowned upon if you do so when\n> you do not have to) to add your new rule before an existing rule, but your\n> rule should be added as a new rule to the enumeration in the comment, and\n> the code that implements the new rule after the comment, no?\n\nThe reason I wanted to put it first is that a tag update could be either\nfast-forward or not, and I wanted to have consistent behavior for both\ncases.  I can move the comment block and describe the full set of cases.\n\n\n> > diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh\n> > index c718253..7906ba5 100755\n> > --- a/t/t5400-send-pack.sh\n> > +++ b/t/t5400-send-pack.sh\n> > @@ -106,6 +106,20 @@ test_expect_success 'denyNonFastforwards trumps --force' '\n> >  \ttest \"$victim_orig\" = \"$victim_head\"\n> >  '\n> >  \n> > +test_expect_success 'denyMovingTags trumps --force' '\n> > +\t(\n> > +\t    cd victim &&\n> > +\t    ( git tag moving_tag master^ || : ) &&\n> \n> In which circumstance is it allowed for this \"git tag\" command to\n> fail and the entire test to succeed?\n\nCargo-cult error, good catch, thanks.\n\nFixed patch forthcoming.\n\n    Dave\n"},{"id":"149362","messageId":"7v7hj7er1h.fsf@alter.siamese.dyndns.org","threadId":"24883","inReplyTo":"alpine.DEB.2.00.1008300924550.20874@narbuckle.genericorp.net","subject":"Re: [PATCH] push: warn users about updating existing tags on push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-30T21:20:26Z","receivedAt":"2010-08-30T21:20:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dave Olszewski <cxreg@pobox.com> writes:\n\n> On Mon, 30 Aug 2010, Junio C Hamano wrote:\n>\n> Thanks for the critique and comments\n>\n>> Dave Olszewski <cxreg@pobox.com> writes:\n>> \n>> > Generally, tags are considered a write-once ref (or object), and updates\n>> > to them are the exception to the rule.\n>> \n>> This may be just the naming issue and you could say \"moving them\",\n>> \"updates to them\" or \"changing them\" interchangeably in the above;\n>> among them, \"updates to them\" sounds the most natural.\n>> \n>> Can you change the \"moving\" in the patch to make them consistent with the\n>> above description?\n>\n> Sure, no problem.  Would you like this changed in the variable and\n> config names as well, or just the printed text?\n\nThe goal being making them consistent, the text and configuration variable\n(which are user-facing names) should match variables and functions (which\nare internal names).  It would be inconsistent to store the value of the\nxfer.denyupdatetag configuration in deny_moving_tags variable, no?\n\nI wondered if denyupdatetag should also forbid \"git tag -f\"; it would be\nawkward if we did so.  The configuration is only about forbidding ref\ntransfer operations from updating the tags.\n\nBut somehow core.denyupdatetag sounds as if \"git tag -f\" is also verboten\nand that is why I weatherballooned xfer.* in the first paragraph of this\nmessage.\n"},{"id":"149526","messageId":"AANLkTinn0Evi6tYMSt+FevJnFt1taQVzqhJKuiGKudOy@mail.gmail.com","threadId":"24883","inReplyTo":"1282983736-3233-1-git-send-email-cxreg@pobox.com","subject":"Re: [PATCH] push: warn users about updating existing tags on push","fromName":"Tay Ray Chuan","fromEmail":"rctay89@gmail.com","sentAt":"2010-09-01T03:51:05Z","receivedAt":"2010-09-01T03:51:05Z","isPatch":true,"sender":{"key":"rctay89@gmail.com","avatar":"https://avatars.githubusercontent.com/u/61553?v=4"},"body":"Hi,\n\nOn Sat, Aug 28, 2010 at 4:22 PM, Dave Olszewski <cxreg@pobox.com> wrote:\n> Generally, tags are considered a write-once ref (or object), and updates\n> to them are the exception to the rule.  This is evident from the\n> behavior of \"git fetch\", which will not update a tag it already has\n> unless --tags is specified, from the --force option to \"git tag\", and\n> the fact that Git does not keep reflogs for tags.\n>\n> However, there is presently nothing preventing a tag from being\n> fast-forwarded, which can happen intentionally or accidentally.  In both\n> cases, the user should be aware that they are changing something that is\n> expected to be immutable and stable.\n\nSounds like a pretty good idea.\n\nI think we could also expose this as a command-line option - say, --force-tags.\n\nAlso, how will this handle a remote config like this?\n\n  [remote \"foo\"]\n    push = +refs/tags/*\n\n> [snip]\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 05ec3fe..02dfc96 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> [snip]\n> @@ -1545,6 +1548,10 @@ push.default::\n>  * `tracking` push the current branch to its upstream branch.\n>  * `current` push the current branch to a branch of the same name.\n>\n> +push.denyMovingTags::\n> +       Whether or not a user will be allowed to push a tag that already\n> +       exists on the remote for a different object.  False by default.\n> +\n>  rebase.stat::\n>        Whether to show a diffstat of what changed upstream since the last\n>        rebase. False by default.\n\nHmm, it's a little weird to speak of \"allowing\" the user to do this\nand that. Perhaps\n\n\tWhether or not a push will be allowed to proceed if a tag...\n\n> @@ -1593,6 +1600,11 @@ receive.denyNonFastForwards::\n>        even if that push is forced. This configuration variable is\n>        set when initializing a shared repository.\n>\n> +receive.denyMovingTags::\n> +       If set to true, git-receive-pack will deny an update to a tag which\n> +       already points to a different object.  Use this to prevent such an\n> +       update via a push, even if that push is forced.\n> +\n>  receive.updateserverinfo::\n>        If set to true, git-receive-pack will run git-update-server-info\n>        after receiving data from git-push and updating refs.\n\nPerhaps\n\n\tIf set to true, git-receive-pack will refuse to update to a tag to point the\n\ttag to a\tdifferent object.  Use this...\n\n> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n> index 658ff2f..1d53e04 100644\n> --- a/Documentation/git-push.txt\n> +++ b/Documentation/git-push.txt\n> @@ -112,7 +112,10 @@ nor in any Push line of the corresponding remotes file---see below).\n>        Usually, the command refuses to update a remote ref that is\n>        not an ancestor of the local ref used to overwrite it.\n>        This flag disables the check.  This can cause the\n> -       remote repository to lose commits; use it with care.\n> +       remote repository to lose commits; use it with care.  This\n> +       flag will also allow a previously pushed tag to be updated\n> +       to point to a new commit, which is refused if\n> +       push.denyMovingTags is set to true.\n>  --repo=<repository>::\n>        This option is only relevant if no <repository> argument is\n\nPerhaps\n\n\tremote repository to lose commits; use it with care.\n\n\tNote that for tags that have already been pushed and have been updated\n\tlocally, \\--force will not update them if push.denyMovingTags is set to true.\n\n-- \nCheers,\nRay Chuan\n"},{"id":"149565","messageId":"7veidd7aqo.fsf@alter.siamese.dyndns.org","threadId":"24883","inReplyTo":"AANLkTinn0Evi6tYMSt+FevJnFt1taQVzqhJKuiGKudOy@mail.gmail.com","subject":"Re: [PATCH] push: warn users about updating existing tags on push","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-01T15:18:55Z","receivedAt":"2010-09-01T15:18:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tay Ray Chuan <rctay89@gmail.com> writes:\n\n>> +push.denyMovingTags::\n>> +       Whether or not a user will be allowed to push a tag that already\n>> +       exists on the remote for a different object.  False by default.\n>\n> Hmm, it's a little weird to speak of \"allowing\" the user to do this\n> and that. Perhaps\n>\n> \tWhether or not a push will be allowed to proceed if a tag...\n\nI think that is a sensible suggestion.  Or even stronger \"forbid updating\nan existing tag; defaults to false\".\n\n>> +receive.denyMovingTags::\n>> +       If set to true, git-receive-pack will deny an update to a tag which\n>> +       already points to a different object.  Use this to prevent such an\n>> +       update via a push, even if that push is forced.\n>> +\n>\n> Perhaps\n>\n> \tIf set to true, git-receive-pack will refuse to update to a tag to point the\n> \ttag to a\tdifferent object.  Use this...\n\nSounds better.\n\n>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt\n>> index 658ff2f..1d53e04 100644\n>> --- a/Documentation/git-push.txt\n>> +++ b/Documentation/git-push.txt\n>> @@ -112,7 +112,10 @@ nor in any Push line of the corresponding remotes file---see below).\n>>        Usually, the command refuses to update a remote ref that is\n>>        not an ancestor of the local ref used to overwrite it.\n>>        This flag disables the check.  This can cause the\n>> -       remote repository to lose commits; use it with care.\n>> +       remote repository to lose commits; use it with care.  This\n>> +       flag will also allow a previously pushed tag to be updated\n>> +       to point to a new commit, which is refused if\n>> +       push.denyMovingTags is set to true.\n>\n> Perhaps\n>\n> \tremote repository to lose commits; use it with care.\n>\n> \tNote that for tags that have already been pushed and have been updated\n> \tlocally, \\--force will not update them if push.denyMovingTags is set to true.\n\nI don't think the change to this section is necessary, _unless_ existing\nmention of \"remote ref\" is changed to \"remote branch\" to exclude tags.  If\nwe wanted to say something, probably\n\n    Note that the above applies both to branches and tags.\n\nwould be sufficient.  I don't think this is a place to enumerate\nexceptions like this new configuration and all the other existing ones\n(e.g. denynonfastforwards, denycurrentbranch, denydeletecurrent etc.)\n"}]}