[PATCH] push: disallow fast-forwarding tags without --force

[PATCH] push: disallow fast-forwarding tags without --force

[Dave Olszewski]

Generally, tags are considered a write-once ref (or object), and updates
to them are the exception to the rule.  This is evident from the
behavior of "git fetch", which will not update a tag it already has
unless --tags is specified, and from the --force option to "git tag".

However, there is presently nothing preventing a tag from being
fast-forwarded, which can happen intentionally or accidentally.  In both
cases, the user should be aware that they are changing something that is
expected to be immutable and stable.

This change forces a user to specify "git push --force" to push a tag
which points to a different object than it does on the upstream
repository, regardless of whether it's a fast-forward or not.

The config option receive.denyMovingTags can be set on the upstream
repository to disallow this, even with --force.

Signed-off-by: Dave Olszewski <cxreg@pobox.com>
---
 Documentation/config.txt               |    8 ++++++++
 Documentation/git-push.txt             |   21 ++++++++++++++++++---
 Documentation/git-receive-pack.txt     |    3 ++-
 advice.c                               |    2 ++
 advice.h                               |    1 +
 builtin/push.c                         |   10 ++++++++--
 builtin/receive-pack.c                 |   14 ++++++++++++++
 builtin/send-pack.c                    |    9 ++++++++-
 cache.h                                |    1 +
 contrib/completion/git-completion.bash |    1 +
 remote.c                               |   13 +++++++++++++
 t/t5400-send-pack.sh                   |   13 +++++++++++++
 transport-helper.c                     |    6 ++++++
 transport.c                            |   15 ++++++++++++---
 transport.h                            |    4 ++--
 15 files changed, 109 insertions(+), 12 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 05ec3fe..587cccd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -122,6 +122,9 @@ advice.*::
 	pushNonFastForward::
 		Advice shown when linkgit:git-push[1] refuses
 		non-fast-forward refs. Default: true.
+	pushMovingTag::
+		Advice shown when linkgit:git-push[1] refuses
+		to push a changed tag. Default: true.
 	statusHints::
 		Directions on how to stage/unstage/add shown in the
 		output of linkgit:git-status[1] and the template shown
@@ -1593,6 +1596,11 @@ receive.denyNonFastForwards::
 	even if that push is forced. This configuration variable is
 	set when initializing a shared repository.
 
+receive.denyMovingTags::
+	If set to true, git-receive-pack will deny an update to a tag which
+	already points to a different object.  Use this to prevent such an
+	update via a push, even if that push is forced.
+
 receive.updateserverinfo::
 	If set to true, git-receive-pack will run git-update-server-info
 	after receiving data from git-push and updating refs.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 658ff2f..f7f8f17 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -112,7 +112,9 @@ nor in any Push line of the corresponding remotes file---see below).
 	Usually, the command refuses to update a remote ref that is
 	not an ancestor of the local ref used to overwrite it.
 	This flag disables the check.  This can cause the
-	remote repository to lose commits; use it with care.
+	remote repository to lose commits; use it with care.  This
+	flag will also allow a previously pushed tag to be updated
+	to point to a new commit, which is refused by default.
 
 --repo=<repository>::
 	This option is only relevant if no <repository> argument is
@@ -215,8 +217,9 @@ remote rejected::
 	of the following safety options in effect:
 	`receive.denyCurrentBranch` (for pushes to the checked out
 	branch), `receive.denyNonFastForwards` (for forced
-	non-fast-forward updates), `receive.denyDeletes` or
-	`receive.denyDeleteCurrent`.  See linkgit:git-config[1].
+	non-fast-forward updates), `receive.denyDeletes`,
+	`receive.denyDeleteCurrent`, or `receive.denyMovingTags`.  See
+	linkgit:git-config[1].
 
 remote failure::
 	The remote end did not report the successful update of the ref,
@@ -324,6 +327,18 @@ overwrite it. In other words, "git push --force" is a method reserved for
 a case where you do mean to lose history.
 
 
+Note about moving tags
+----------------------
+
+Tags are widely considered 'read-only', and are not expected to change.
+See the 'On Re-tagging' section of linkgit:git-tag[1] to learn more about
+why this is so.
+
+If you really need to change an already-propagated tag, you must force it
+with "git push --force".  This can be overridden on the upstream repository
+with receive.denyMovingTags.
+
+
 Examples
 --------
 
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 2790eeb..55f2830 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -30,7 +30,8 @@ post-update hooks found in the Documentation/howto directory.
 
 'git-receive-pack' honours the receive.denyNonFastForwards config
 option, which tells it if updates to a ref should be denied if they
-are not fast-forwards.
+are not fast-forwards, and the receive.denyMovingTags config option,
+which disallows updating a tag to point to a new object.
 
 OPTIONS
 -------
diff --git a/advice.c b/advice.c
index 0be4b5f..51021d8 100644
--- a/advice.c
+++ b/advice.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 
 int advice_push_nonfastforward = 1;
+int advice_push_moving_tag = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
@@ -12,6 +13,7 @@ static struct {
 	int *preference;
 } advice_config[] = {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
+	{ "pushmovingtag", &advice_push_moving_tag},
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
 	{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 3244ebb..8b1a33c 100644
--- a/advice.h
+++ b/advice.h
@@ -4,6 +4,7 @@
 #include "git-compat-util.h"
 
 extern int advice_push_nonfastforward;
+extern int advice_push_moving_tag;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index e655eb7..8547554 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -106,7 +106,7 @@ static void setup_default_push_refspecs(void)
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
-	int nonfastforward;
+	int nonfastforward, moving_tag;
 
 	transport_set_verbosity(transport, verbosity, progress);
 
@@ -119,7 +119,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (verbosity > 0)
 		fprintf(stderr, "Pushing to %s\n", transport->url);
 	err = transport_push(transport, refspec_nr, refspec, flags,
-			     &nonfastforward);
+			     &nonfastforward, &moving_tag);
 	if (err != 0)
 		error("failed to push some refs to '%s'", transport->url);
 
@@ -134,6 +134,12 @@ static int push_with_options(struct transport *transport, int flags)
 				"'Note about fast-forwards' section of 'git push --help' for details.\n");
 	}
 
+	if (moving_tag && advice_push_moving_tag) {
+		fprintf(stderr, "A tag which already exists upstream was attempted to be pushed while\n"
+				"pointing to a different object.  This is unsafe, and disabled by default.\n"
+				"See the 'Note about moving tags' section of 'git push --help' for details.\n");
+	}
+
 	return 1;
 }
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 760817d..1a96e55 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -22,6 +22,7 @@ enum deny_action {
 
 static int deny_deletes;
 static int deny_non_fast_forwards;
+static int deny_moving_tags;
 static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
 static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
 static int receive_fsck_objects;
@@ -63,6 +64,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.denymovingtags") == 0) {
+		deny_moving_tags = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.unpacklimit") == 0) {
 		receive_unpack_limit = git_config_int(var, value);
 		return 0;
@@ -416,6 +422,14 @@ static const char *update(struct command *cmd)
 			return "non-fast-forward";
 		}
 	}
+	if (deny_moving_tags && !is_null_sha1(new_sha1) &&
+	    !is_null_sha1(old_sha1) &&
+	    hashcmp(old_sha1, new_sha1) &&
+	    !prefixcmp(name, "refs/tags/")) {
+		rp_error("denying moving tag %s", name);
+		return "moving tag";
+	}
+
 	if (run_update_hook(cmd)) {
 		rp_error("hook declined to update %s", name);
 		return "hook declined";
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 481602d..c41a455 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -198,6 +198,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "non-fast forward";
 			break;
 
+		case REF_STATUS_REJECT_MOVING_TAG:
+			res = "error";
+			msg = "moving tag";
+			break;
+
 		case REF_STATUS_REJECT_NODELETE:
 		case REF_STATUS_REMOTE_REJECT:
 			res = "error";
@@ -275,6 +280,7 @@ int send_pack(struct send_pack_args *args,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_MOVING_TAG:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
@@ -395,6 +401,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	const char *receivepack = "git-receive-pack";
 	int flags;
 	int nonfastforward = 0;
+	int moving_tag = 0;
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
@@ -516,7 +523,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	ret |= finish_connect(conn);
 
 	if (!helper_status)
-		transport_print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward);
+		transport_print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward, &moving_tag);
 
 	if (!args.dry_run && remote) {
 		struct ref *ref;
diff --git a/cache.h b/cache.h
index eb77e1d..cca7499 100644
--- a/cache.h
+++ b/cache.h
@@ -913,6 +913,7 @@ struct ref {
 		REF_STATUS_NONE = 0,
 		REF_STATUS_OK,
 		REF_STATUS_REJECT_NONFASTFORWARD,
+		REF_STATUS_REJECT_MOVING_TAG,
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6756990..dc29e0b 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1964,6 +1964,7 @@ _git_config ()
 		receive.denyCurrentBranch
 		receive.denyDeletes
 		receive.denyNonFastForwards
+		receive.denyMovingTags
 		receive.fsckObjects
 		receive.unpackLimit
 		repack.usedeltabaseoffset
diff --git a/remote.c b/remote.c
index 9143ec7..cc7ce93 100644
--- a/remote.c
+++ b/remote.c
@@ -1266,6 +1266,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 			continue;
 		}
 
+		/* If a tag already exists on the remote and points to
+		 * a different object, we don't want to push it again
+		 * without requiring the user to indicate that they know
+		 * what they are doing.
+		 */
+		if (!prefixcmp(ref->name, "refs/tags/") &&
+		    !ref->deletion &&
+		    !is_null_sha1(ref->old_sha1) &&
+		    !ref->force &&
+		    !force_update) {
+			ref->status = REF_STATUS_REJECT_MOVING_TAG;
+		}
+
 		/* This part determines what can overwrite what.
 		 * The rules are:
 		 *
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index c718253..573eb20 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -106,6 +106,19 @@ test_expect_success 'denyNonFastforwards trumps --force' '
 	test "$victim_orig" = "$victim_head"
 '
 
+test_expect_success 'denyMovingTags trumps --force' '
+	(
+	    cd victim &&
+	    ( git tag moving_tag master^ || : ) &&
+	    git config receive.denyMovingTags true
+	) &&
+	git tag moving_tag &&
+	victim_orig=$(cd victim && git rev-parse --verify moving_tag) &&
+	test_must_fail git send-pack --force ./victim moving_tag &&
+	victim_tag=$(cd victim && git rev-parse --verify moving_tag) &&
+	test "$victim_orig" = "$victim_tag"
+'
+
 test_expect_success 'push --all excludes remote tracking hierarchy' '
 	mkdir parent &&
 	(
diff --git a/transport-helper.c b/transport-helper.c
index acfc88e..e9730a0 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -575,6 +575,7 @@ static int push_refs_with_push(struct transport *transport,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_MOVING_TAG:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
@@ -655,6 +656,11 @@ static int push_refs_with_push(struct transport *transport,
 				free(msg);
 				msg = NULL;
 			}
+			else if (!strcmp(msg, "moving tag")) {
+				status = REF_STATUS_REJECT_MOVING_TAG;
+				free(msg);
+				msg = NULL;
+			}
 		}
 
 		if (ref)
diff --git a/transport.c b/transport.c
index 4dba6f8..e3b2ee8 100644
--- a/transport.c
+++ b/transport.c
@@ -692,6 +692,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "non-fast-forward", porcelain);
 		break;
+	case REF_STATUS_REJECT_MOVING_TAG:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "moving tag", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -711,7 +715,8 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 }
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-				  int verbose, int porcelain, int *nonfastforward)
+				  int verbose, int porcelain,
+				  int *nonfastforward, int *moving_tag)
 {
 	struct ref *ref;
 	int n = 0;
@@ -727,6 +732,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 			n += print_one_push_status(ref, dest, n, porcelain);
 
 	*nonfastforward = 0;
+	*moving_tag = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
@@ -734,6 +740,8 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 			n += print_one_push_status(ref, dest, n, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
 			*nonfastforward = 1;
+		if (ref->status == REF_STATUS_REJECT_MOVING_TAG)
+			*moving_tag = 1;
 	}
 }
 
@@ -1004,9 +1012,10 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 
 int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
-		   int *nonfastforward)
+		   int *nonfastforward, int *moving_tag)
 {
 	*nonfastforward = 0;
+	*moving_tag = 0;
 	transport_verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push) {
@@ -1047,7 +1056,7 @@ int transport_push(struct transport *transport,
 		if (!quiet || err)
 			transport_print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
-					nonfastforward);
+					nonfastforward, moving_tag);
 
 		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
 			set_upstreams(transport, remote_refs, pretend);
diff --git a/transport.h b/transport.h
index c59d973..8e95e09 100644
--- a/transport.h
+++ b/transport.h
@@ -138,7 +138,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-		   int * nonfastforward);
+		   int *nonfastforward, int *moving_tag);
 
 const struct ref *transport_get_remote_refs(struct transport *transport);
 
@@ -163,6 +163,6 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 int transport_refs_pushed(struct ref *ref);
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-		  int verbose, int porcelain, int *nonfastforward);
+		  int verbose, int porcelain, int *nonfastforward, int *moving_tag);
 
 #endif
-- 
1.7.2.2.176.g3e15d

Re: [PATCH] push: disallow fast-forwarding tags without --force

[Junio C Hamano]

Dave Olszewski <cxreg@pobox.com> writes:

> Generally, tags are considered a write-once ref (or object), and updates
> to them are the exception to the rule.  This is evident from the
> behavior of "git fetch", which will not update a tag it already has
> unless --tags is specified, and from the --force option to "git tag".

The title and what you describe later in your proposed log message do not
match.  This is about "push: disallow updating an existing tag by default"
isn't it?

This proposes a big change in the policy, and I do not like it starting
out as the new default to forbid people from doing something they have
been allowed to do for a long time.  I recall hearing some people auto
tagging the latest version their autobuilder/tester tested successfully
and updating the same tag nightly---your change will break their cron
script, no?

If you ship the feature disabled by default first, it will still allow
people to take advantage of it by simply flipping the feature on, instead
of having to install their own update hook.  In a later version, if and
when enough people agree that this should be on by default, we can do so
at a version bump.

Re: [PATCH] push: disallow fast-forwarding tags without --force

[Dave Olszewski]

On Fri, 27 Aug 2010, Junio C Hamano wrote:

> Dave Olszewski <cxreg@pobox.com> writes:
> 
> > Generally, tags are considered a write-once ref (or object), and updates
> > to them are the exception to the rule.  This is evident from the
> > behavior of "git fetch", which will not update a tag it already has
> > unless --tags is specified, and from the --force option to "git tag".
> 
> The title and what you describe later in your proposed log message do not
> match.  This is about "push: disallow updating an existing tag by default"
> isn't it?

I don't see a major difference in meaning, since only fast-forwards are
allowed without --force, but you're right about my intent.  I'm happy to
change the commit message.

> This proposes a big change in the policy, and I do not like it starting
> out as the new default to forbid people from doing something they have
> been allowed to do for a long time.  I recall hearing some people auto
> tagging the latest version their autobuilder/tester tested successfully
> and updating the same tag nightly---your change will break their cron
> script, no?
> 
> If you ship the feature disabled by default first, it will still allow
> people to take advantage of it by simply flipping the feature on, instead
> of having to install their own update hook.  In a later version, if and
> when enough people agree that this should be on by default, we can do so
> at a version bump.

Yes that's true.  I suspected based on the lack of any documentation or
tests promising such behavior, the inclination for git to want to
pretend that changed tags haven't changed, and the social stigma against
it, that this was a bug.

It's trivial for someone to update build software from "git push remote
tag" to "git push remote +tag" or "git push -f remote tag", but I can
understand your objection.  It's the reason I didn't also add
receive.denyMovingTags to the default config for bare repositories.

It seems unlikely that many people are ever going to "flip on" this
feature; either they won't know about it (and for them, it should be
on), or they'll have a reason to move a tag, and want it off.  That, and
my perception that this was unintentional behavior, is the reason I
patched it to be default.

However, If you still feel strongly about this, I can rework it to be
optional.  Thanks for your feedback!

    Dave

Re: [PATCH] push: disallow fast-forwarding tags without --force

[Jonathan Nieder]

Hi,

Dave Olszewski wrote:

> It's trivial for someone to update build software from "git push remote
> tag" to "git push remote +tag" or "git push -f remote tag", but I can
> understand your objection.

Right.  The thing to prevent is unhappy surprises: it is best if
users upgrading get a chance to update their scripts before it matters.

> It seems unlikely that many people are ever going to "flip on" this
> feature; either they won't know about it (and for them, it should be
> on), or they'll have a reason to move a tag, and want it off.

This is why a switch of some kind is useful: after reading the release
notes, a user can flip the switch for a glimpse of the future, forsee
the upcoming disaster, and fix everything up before it really matters.
After the default changes, the switch has the opposite purpose: users
who were not prepared can use it to turn back time and avoid having
to change their code.

So the deprecation process for unwanted features tends to look like
this:

 1. complain about use of the feature, with an option to suppress
    the warnings.  or: loudly proclaim that the feature is going
    away in release notes

 2. add an option to disable the feature, to help people transition

 3. change the default to true

 4. remove the option

Step 1 is the most important one imho.  See
Documentation/RelNotes-1.6.6.txt for an example.

I don't think we've ever reached step 4, but we should try it some
time.

Hope that helps,
Jonathan

[PATCH] push: warn users about updating existing tags on push

[Dave Olszewski]

Generally, tags are considered a write-once ref (or object), and updates
to them are the exception to the rule.  This is evident from the
behavior of "git fetch", which will not update a tag it already has
unless --tags is specified, from the --force option to "git tag", and
the fact that Git does not keep reflogs for tags.

However, there is presently nothing preventing a tag from being
fast-forwarded, which can happen intentionally or accidentally.  In both
cases, the user should be aware that they are changing something that is
expected to be immutable and stable.

This change adds a warning when a user pushes a tag which points to a
different object than it does on the upstream repository.  If the user
specifies "git push --force", the action is determined to be intentional
and the warning is not shown.

If the user wishes to have these pushes refused instead of simply
warning them, they can set push.denyMovingTags to true.  This new option
defaults to false, but might later default to true.

The config option receive.denyMovingTags can be set on the upstream
repository to disallow this, even with --force.

Signed-off-by: Dave Olszewski <cxreg@pobox.com>
---
 Documentation/config.txt               |   12 ++++++++++++
 Documentation/git-push.txt             |   25 ++++++++++++++++++++++---
 Documentation/git-receive-pack.txt     |    3 ++-
 advice.c                               |    2 ++
 advice.h                               |    1 +
 builtin/push.c                         |   11 +++++++++--
 builtin/receive-pack.c                 |   14 ++++++++++++++
 builtin/send-pack.c                    |    9 ++++++++-
 cache.h                                |    1 +
 contrib/completion/git-completion.bash |    2 ++
 remote.c                               |   31 +++++++++++++++++++++++++++++++
 t/t5400-send-pack.sh                   |   14 ++++++++++++++
 transport-helper.c                     |    6 ++++++
 transport.c                            |   15 ++++++++++++---
 transport.h                            |    4 ++--
 15 files changed, 138 insertions(+), 12 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 05ec3fe..02dfc96 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -122,6 +122,9 @@ advice.*::
 	pushNonFastForward::
 		Advice shown when linkgit:git-push[1] refuses
 		non-fast-forward refs. Default: true.
+	pushMovingTags::
+		Advice shown when linkgit:git-push[1] refuses
+		to push a changed tag. Default: true.
 	statusHints::
 		Directions on how to stage/unstage/add shown in the
 		output of linkgit:git-status[1] and the template shown
@@ -1545,6 +1548,10 @@ push.default::
 * `tracking` push the current branch to its upstream branch.
 * `current` push the current branch to a branch of the same name.
 
+push.denyMovingTags::
+	Whether or not a user will be allowed to push a tag that already
+	exists on the remote for a different object.  False by default.
+
 rebase.stat::
 	Whether to show a diffstat of what changed upstream since the last
 	rebase. False by default.
@@ -1593,6 +1600,11 @@ receive.denyNonFastForwards::
 	even if that push is forced. This configuration variable is
 	set when initializing a shared repository.
 
+receive.denyMovingTags::
+	If set to true, git-receive-pack will deny an update to a tag which
+	already points to a different object.  Use this to prevent such an
+	update via a push, even if that push is forced.
+
 receive.updateserverinfo::
 	If set to true, git-receive-pack will run git-update-server-info
 	after receiving data from git-push and updating refs.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 658ff2f..1d53e04 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -112,7 +112,10 @@ nor in any Push line of the corresponding remotes file---see below).
 	Usually, the command refuses to update a remote ref that is
 	not an ancestor of the local ref used to overwrite it.
 	This flag disables the check.  This can cause the
-	remote repository to lose commits; use it with care.
+	remote repository to lose commits; use it with care.  This
+	flag will also allow a previously pushed tag to be updated
+	to point to a new commit, which is refused if
+	push.denyMovingTags is set to true.
 
 --repo=<repository>::
 	This option is only relevant if no <repository> argument is
@@ -215,8 +218,9 @@ remote rejected::
 	of the following safety options in effect:
 	`receive.denyCurrentBranch` (for pushes to the checked out
 	branch), `receive.denyNonFastForwards` (for forced
-	non-fast-forward updates), `receive.denyDeletes` or
-	`receive.denyDeleteCurrent`.  See linkgit:git-config[1].
+	non-fast-forward updates), `receive.denyDeletes`,
+	`receive.denyDeleteCurrent`, or `receive.denyMovingTags`.  See
+	linkgit:git-config[1].
 
 remote failure::
 	The remote end did not report the successful update of the ref,
@@ -324,6 +328,21 @@ overwrite it. In other words, "git push --force" is a method reserved for
 a case where you do mean to lose history.
 
 
+Note about moving tags
+----------------------
+
+Tags are widely considered 'read-only', and are not expected to change.
+See the 'On Re-tagging' section of linkgit:git-tag[1] to learn more about
+why this is so.
+
+In a future version of Git, pushing a tag which points to a different
+object than the one on the remote may be be disallowed by default.
+Presently you can configure that such a push will be rejected with
+push.denyMovingTags.  If this is set to true, and you really need to change
+an already-propagated tag, you must force it with "git push --force".  This
+can be overridden on the upstream repository with receive.denyMovingTags.
+
+
 Examples
 --------
 
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 2790eeb..55f2830 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt
@@ -30,7 +30,8 @@ post-update hooks found in the Documentation/howto directory.
 
 'git-receive-pack' honours the receive.denyNonFastForwards config
 option, which tells it if updates to a ref should be denied if they
-are not fast-forwards.
+are not fast-forwards, and the receive.denyMovingTags config option,
+which disallows updating a tag to point to a new object.
 
 OPTIONS
 -------
diff --git a/advice.c b/advice.c
index 0be4b5f..51021d8 100644
--- a/advice.c
+++ b/advice.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 
 int advice_push_nonfastforward = 1;
+int advice_push_moving_tag = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
@@ -12,6 +13,7 @@ static struct {
 	int *preference;
 } advice_config[] = {
 	{ "pushnonfastforward", &advice_push_nonfastforward },
+	{ "pushmovingtag", &advice_push_moving_tag},
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
 	{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 3244ebb..8b1a33c 100644
--- a/advice.h
+++ b/advice.h
@@ -4,6 +4,7 @@
 #include "git-compat-util.h"
 
 extern int advice_push_nonfastforward;
+extern int advice_push_moving_tag;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index e655eb7..b04adbd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -106,7 +106,7 @@ static void setup_default_push_refspecs(void)
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
-	int nonfastforward;
+	int nonfastforward, moving_tag;
 
 	transport_set_verbosity(transport, verbosity, progress);
 
@@ -119,7 +119,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (verbosity > 0)
 		fprintf(stderr, "Pushing to %s\n", transport->url);
 	err = transport_push(transport, refspec_nr, refspec, flags,
-			     &nonfastforward);
+			     &nonfastforward, &moving_tag);
 	if (err != 0)
 		error("failed to push some refs to '%s'", transport->url);
 
@@ -134,6 +134,13 @@ static int push_with_options(struct transport *transport, int flags)
 				"'Note about fast-forwards' section of 'git push --help' for details.\n");
 	}
 
+	if (moving_tag && advice_push_moving_tag) {
+		fprintf(stderr, "A tag which already exists upstream was attempted to be pushed while\n"
+				"pointing to a different object.  This is currently disabled by\n"
+				"push.denyMovingTags.  See the 'Note about moving tags' section of\n"
+				"'git push --help' for details.\n");
+	}
+
 	return 1;
 }
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 760817d..1a96e55 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -22,6 +22,7 @@ enum deny_action {
 
 static int deny_deletes;
 static int deny_non_fast_forwards;
+static int deny_moving_tags;
 static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
 static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
 static int receive_fsck_objects;
@@ -63,6 +64,11 @@ static int receive_pack_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if (strcmp(var, "receive.denymovingtags") == 0) {
+		deny_moving_tags = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (strcmp(var, "receive.unpacklimit") == 0) {
 		receive_unpack_limit = git_config_int(var, value);
 		return 0;
@@ -416,6 +422,14 @@ static const char *update(struct command *cmd)
 			return "non-fast-forward";
 		}
 	}
+	if (deny_moving_tags && !is_null_sha1(new_sha1) &&
+	    !is_null_sha1(old_sha1) &&
+	    hashcmp(old_sha1, new_sha1) &&
+	    !prefixcmp(name, "refs/tags/")) {
+		rp_error("denying moving tag %s", name);
+		return "moving tag";
+	}
+
 	if (run_update_hook(cmd)) {
 		rp_error("hook declined to update %s", name);
 		return "hook declined";
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 481602d..c41a455 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -198,6 +198,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "non-fast forward";
 			break;
 
+		case REF_STATUS_REJECT_MOVING_TAG:
+			res = "error";
+			msg = "moving tag";
+			break;
+
 		case REF_STATUS_REJECT_NODELETE:
 		case REF_STATUS_REMOTE_REJECT:
 			res = "error";
@@ -275,6 +280,7 @@ int send_pack(struct send_pack_args *args,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_MOVING_TAG:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
@@ -395,6 +401,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	const char *receivepack = "git-receive-pack";
 	int flags;
 	int nonfastforward = 0;
+	int moving_tag = 0;
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
@@ -516,7 +523,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	ret |= finish_connect(conn);
 
 	if (!helper_status)
-		transport_print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward);
+		transport_print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward, &moving_tag);
 
 	if (!args.dry_run && remote) {
 		struct ref *ref;
diff --git a/cache.h b/cache.h
index eb77e1d..cca7499 100644
--- a/cache.h
+++ b/cache.h
@@ -913,6 +913,7 @@ struct ref {
 		REF_STATUS_NONE = 0,
 		REF_STATUS_OK,
 		REF_STATUS_REJECT_NONFASTFORWARD,
+		REF_STATUS_REJECT_MOVING_TAG,
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6756990..5a38473 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1960,10 +1960,12 @@ _git_config ()
 		pull.octopus
 		pull.twohead
 		push.default
+		push.denyMovingTags
 		rebase.stat
 		receive.denyCurrentBranch
 		receive.denyDeletes
 		receive.denyNonFastForwards
+		receive.denyMovingTags
 		receive.fsckObjects
 		receive.unpackLimit
 		repack.usedeltabaseoffset
diff --git a/remote.c b/remote.c
index 9143ec7..fbca1e6 100644
--- a/remote.c
+++ b/remote.c
@@ -50,6 +50,8 @@ static int explicit_default_remote_name;
 static struct rewrites rewrites;
 static struct rewrites rewrites_push;
 
+static int deny_moving_tags;
+
 #define BUF_SIZE (2048)
 static char buffer[BUF_SIZE];
 
@@ -385,6 +387,10 @@ static int handle_config(const char *key, const char *value, void *cb)
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
+	if (!strcmp(key, "push.denymovingtags")) {
+		deny_moving_tags = git_config_bool(key, value);
+		return 0;
+	}
 	if (prefixcmp(key,  "remote."))
 		return 0;
 	name = key + 7;
@@ -1266,6 +1272,31 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 			continue;
 		}
 
+		/* If a tag already exists on the remote and points to
+		 * a different object, we don't want to push it again
+		 * without requiring the user to indicate that they know
+		 * what they are doing.
+		 */
+		if (!prefixcmp(ref->name, "refs/tags/") &&
+		    !ref->deletion &&
+		    !is_null_sha1(ref->old_sha1)) {
+			if (deny_moving_tags) {
+				/* Set `nonfastforward` for the sake of displaying
+				 * this update as forced
+				 */
+				ref->nonfastforward = 1;
+				if (!ref->force && !force_update) {
+					ref->status = REF_STATUS_REJECT_MOVING_TAG;
+				}
+			} else {
+				if (!ref->force && !force_update)
+					warning("You are changing the value of an upstream tag.  This may\n"
+						"be deprecated in a future version of Git.  Please use --force\n"
+						"if this was intentional, and consider setting push.denyMovingTags.");
+			}
+			continue;
+		}
+
 		/* This part determines what can overwrite what.
 		 * The rules are:
 		 *
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index c718253..7906ba5 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -106,6 +106,20 @@ test_expect_success 'denyNonFastforwards trumps --force' '
 	test "$victim_orig" = "$victim_head"
 '
 
+test_expect_success 'denyMovingTags trumps --force' '
+	(
+	    cd victim &&
+	    ( git tag moving_tag master^ || : ) &&
+	    git config receive.denyMovingTags true
+	) &&
+	git tag moving_tag &&
+	git config push.denyMovingTags true &&
+	victim_orig=$(cd victim && git rev-parse --verify moving_tag) &&
+	test_must_fail git send-pack --force ./victim moving_tag &&
+	victim_tag=$(cd victim && git rev-parse --verify moving_tag) &&
+	test "$victim_orig" = "$victim_tag"
+'
+
 test_expect_success 'push --all excludes remote tracking hierarchy' '
 	mkdir parent &&
 	(
diff --git a/transport-helper.c b/transport-helper.c
index acfc88e..e9730a0 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -575,6 +575,7 @@ static int push_refs_with_push(struct transport *transport,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_MOVING_TAG:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
@@ -655,6 +656,11 @@ static int push_refs_with_push(struct transport *transport,
 				free(msg);
 				msg = NULL;
 			}
+			else if (!strcmp(msg, "moving tag")) {
+				status = REF_STATUS_REJECT_MOVING_TAG;
+				free(msg);
+				msg = NULL;
+			}
 		}
 
 		if (ref)
diff --git a/transport.c b/transport.c
index 4dba6f8..e3b2ee8 100644
--- a/transport.c
+++ b/transport.c
@@ -692,6 +692,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "non-fast-forward", porcelain);
 		break;
+	case REF_STATUS_REJECT_MOVING_TAG:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "moving tag", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -711,7 +715,8 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 }
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-				  int verbose, int porcelain, int *nonfastforward)
+				  int verbose, int porcelain,
+				  int *nonfastforward, int *moving_tag)
 {
 	struct ref *ref;
 	int n = 0;
@@ -727,6 +732,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 			n += print_one_push_status(ref, dest, n, porcelain);
 
 	*nonfastforward = 0;
+	*moving_tag = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
@@ -734,6 +740,8 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 			n += print_one_push_status(ref, dest, n, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
 			*nonfastforward = 1;
+		if (ref->status == REF_STATUS_REJECT_MOVING_TAG)
+			*moving_tag = 1;
 	}
 }
 
@@ -1004,9 +1012,10 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 
 int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
-		   int *nonfastforward)
+		   int *nonfastforward, int *moving_tag)
 {
 	*nonfastforward = 0;
+	*moving_tag = 0;
 	transport_verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push) {
@@ -1047,7 +1056,7 @@ int transport_push(struct transport *transport,
 		if (!quiet || err)
 			transport_print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
-					nonfastforward);
+					nonfastforward, moving_tag);
 
 		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
 			set_upstreams(transport, remote_refs, pretend);
diff --git a/transport.h b/transport.h
index c59d973..8e95e09 100644
--- a/transport.h
+++ b/transport.h
@@ -138,7 +138,7 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
 
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-		   int * nonfastforward);
+		   int *nonfastforward, int *moving_tag);
 
 const struct ref *transport_get_remote_refs(struct transport *transport);
 
@@ -163,6 +163,6 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 int transport_refs_pushed(struct ref *ref);
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-		  int verbose, int porcelain, int *nonfastforward);
+		  int verbose, int porcelain, int *nonfastforward, int *moving_tag);
 
 #endif
-- 
1.7.2.2.179.g90aca

Re: [PATCH] push: warn users about updating existing tags on push

[Junio C Hamano]

Dave Olszewski <cxreg@pobox.com> writes:

> Generally, tags are considered a write-once ref (or object), and updates
> to them are the exception to the rule.

This may be just the naming issue and you could say "moving them",
"updates to them" or "changing them" interchangeably in the above;
among them, "updates to them" sounds the most natural.

Can you change the "moving" in the patch to make them consistent with the
above description?

> However, there is presently nothing preventing a tag from being
> fast-forwarded, which can happen intentionally or accidentally.
> ... the user should be aware that they are changing something that is
> expected to be immutable and stable.

I actually think prevention of non-fast-forward updates for tags actually
is a misfeature that didn't even come from any concious design; the check
for fast-forwarding refs was to make sure we do not lose histories from
branches.  IOW, I would say this would have been a good feature if things
were like this from day one.

> diff --git a/remote.c b/remote.c
> index 9143ec7..fbca1e6 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -50,6 +50,8 @@ static int explicit_default_remote_name;
>  static struct rewrites rewrites;
>  static struct rewrites rewrites_push;
>  
> +static int deny_moving_tags;
> +
>  #define BUF_SIZE (2048)
>  static char buffer[BUF_SIZE];
>  
> @@ -385,6 +387,10 @@ static int handle_config(const char *key, const char *value, void *cb)
>  			add_instead_of(rewrite, xstrdup(value));
>  		}
>  	}
> +	if (!strcmp(key, "push.denymovingtags")) {
> +		deny_moving_tags = git_config_bool(key, value);
> +		return 0;
> +	}

Hmm, shouldn't this be per-remote (rather, shouldn't a per-remote variant
be allowed to override this)?

> @@ -1266,6 +1272,31 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>  			continue;
>  		}
>  
> +		/* If a tag already exists on the remote and points to
> +		 * a different object, we don't want to push it again
> +		 * without requiring the user to indicate that they know
> +		 * what they are doing.
> +		 */

	/*
         * We try to format
         * multi-line comment
         * like this.
         */

> +		if (!prefixcmp(ref->name, "refs/tags/") &&
> +		    !ref->deletion &&
> +		    !is_null_sha1(ref->old_sha1)) {
> +			if (deny_moving_tags) {
> +				/* Set `nonfastforward` for the sake of displaying
> +				 * this update as forced
> +				 */
> +				ref->nonfastforward = 1;

I think you are propagating this bit to print_ok_ref_status() in
transport.c; it indicates that after your change, "nonfastforward" does
not mean non-fast-forward anymore, doesn't it?

Perhaps the bit needs to be renamed to "update_forced" or something?

> +				if (!ref->force && !force_update) {
> +					ref->status = REF_STATUS_REJECT_MOVING_TAG;
> +				}
> +			} else {
> +				if (!ref->force && !force_update)
> +					warning("You are changing the value of an upstream tag.  This may\n"
> +						"be deprecated in a future version of Git.  Please use --force\n"
> +						"if this was intentional, and consider setting push.denyMovingTags.");
> +			}
> +			continue;
> +		}
> +
>  		/* This part determines what can overwrite what.
>  		 * The rules are:
>  		 *

You are changing the rule that determine what can overwrite what, aren't
you?  It is Ok (although it is in general frowned upon if you do so when
you do not have to) to add your new rule before an existing rule, but your
rule should be added as a new rule to the enumeration in the comment, and
the code that implements the new rule after the comment, no?

> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> index c718253..7906ba5 100755
> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -106,6 +106,20 @@ test_expect_success 'denyNonFastforwards trumps --force' '
>  	test "$victim_orig" = "$victim_head"
>  '
>  
> +test_expect_success 'denyMovingTags trumps --force' '
> +	(
> +	    cd victim &&
> +	    ( git tag moving_tag master^ || : ) &&

In which circumstance is it allowed for this "git tag" command to
fail and the entire test to succeed?

Re: [PATCH] push: warn users about updating existing tags on push

[Dave Olszewski]

On Mon, 30 Aug 2010, Junio C Hamano wrote:

Thanks for the critique and comments

> Dave Olszewski <cxreg@pobox.com> writes:
> 
> > Generally, tags are considered a write-once ref (or object), and updates
> > to them are the exception to the rule.
> 
> This may be just the naming issue and you could say "moving them",
> "updates to them" or "changing them" interchangeably in the above;
> among them, "updates to them" sounds the most natural.
> 
> Can you change the "moving" in the patch to make them consistent with the
> above description?

Sure, no problem.  Would you like this changed in the variable and
config names as well, or just the printed text?


> > diff --git a/remote.c b/remote.c
> > index 9143ec7..fbca1e6 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -50,6 +50,8 @@ static int explicit_default_remote_name;
> >  static struct rewrites rewrites;
> >  static struct rewrites rewrites_push;
> >  
> > +static int deny_moving_tags;
> > +
> >  #define BUF_SIZE (2048)
> >  static char buffer[BUF_SIZE];
> >  
> > @@ -385,6 +387,10 @@ static int handle_config(const char *key, const char *value, void *cb)
> >  			add_instead_of(rewrite, xstrdup(value));
> >  		}
> >  	}
> > +	if (!strcmp(key, "push.denymovingtags")) {
> > +		deny_moving_tags = git_config_bool(key, value);
> > +		return 0;
> > +	}
> 
> Hmm, shouldn't this be per-remote (rather, shouldn't a per-remote variant
> be allowed to override this)?

I wasn't sure about this.  I like the idea of a single setting with
per-remote override, I'll implement that.


> > @@ -1266,6 +1272,31 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
> >  			continue;
> >  		}
> >  
> > +		/* If a tag already exists on the remote and points to
> > +		 * a different object, we don't want to push it again
> > +		 * without requiring the user to indicate that they know
> > +		 * what they are doing.
> > +		 */
> 
> 	/*
>          * We try to format
>          * multi-line comment
>          * like this.
>          */

Ok.


> > +		if (!prefixcmp(ref->name, "refs/tags/") &&
> > +		    !ref->deletion &&
> > +		    !is_null_sha1(ref->old_sha1)) {
> > +			if (deny_moving_tags) {
> > +				/* Set `nonfastforward` for the sake of displaying
> > +				 * this update as forced
> > +				 */
> > +				ref->nonfastforward = 1;
> 
> I think you are propagating this bit to print_ok_ref_status() in
> transport.c; it indicates that after your change, "nonfastforward" does
> not mean non-fast-forward anymore, doesn't it?
> 
> Perhaps the bit needs to be renamed to "update_forced" or something?

Good point.  I arrived at making this change pretty late in the patch
and didn't consider the rename.  Thanks.


> > +				if (!ref->force && !force_update) {
> > +					ref->status = REF_STATUS_REJECT_MOVING_TAG;
> > +				}
> > +			} else {
> > +				if (!ref->force && !force_update)
> > +					warning("You are changing the value of an upstream tag.  This may\n"
> > +						"be deprecated in a future version of Git.  Please use --force\n"
> > +						"if this was intentional, and consider setting push.denyMovingTags.");
> > +			}
> > +			continue;
> > +		}
> > +
> >  		/* This part determines what can overwrite what.
> >  		 * The rules are:
> >  		 *
> 
> You are changing the rule that determine what can overwrite what, aren't
> you?  It is Ok (although it is in general frowned upon if you do so when
> you do not have to) to add your new rule before an existing rule, but your
> rule should be added as a new rule to the enumeration in the comment, and
> the code that implements the new rule after the comment, no?

The reason I wanted to put it first is that a tag update could be either
fast-forward or not, and I wanted to have consistent behavior for both
cases.  I can move the comment block and describe the full set of cases.


> > diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> > index c718253..7906ba5 100755
> > --- a/t/t5400-send-pack.sh
> > +++ b/t/t5400-send-pack.sh
> > @@ -106,6 +106,20 @@ test_expect_success 'denyNonFastforwards trumps --force' '
> >  	test "$victim_orig" = "$victim_head"
> >  '
> >  
> > +test_expect_success 'denyMovingTags trumps --force' '
> > +	(
> > +	    cd victim &&
> > +	    ( git tag moving_tag master^ || : ) &&
> 
> In which circumstance is it allowed for this "git tag" command to
> fail and the entire test to succeed?

Cargo-cult error, good catch, thanks.

Fixed patch forthcoming.

    Dave

Re: [PATCH] push: warn users about updating existing tags on push

[Junio C Hamano]

Dave Olszewski <cxreg@pobox.com> writes:

> On Mon, 30 Aug 2010, Junio C Hamano wrote:
>
> Thanks for the critique and comments
>
>> Dave Olszewski <cxreg@pobox.com> writes:
>> 
>> > Generally, tags are considered a write-once ref (or object), and updates
>> > to them are the exception to the rule.
>> 
>> This may be just the naming issue and you could say "moving them",
>> "updates to them" or "changing them" interchangeably in the above;
>> among them, "updates to them" sounds the most natural.
>> 
>> Can you change the "moving" in the patch to make them consistent with the
>> above description?
>
> Sure, no problem.  Would you like this changed in the variable and
> config names as well, or just the printed text?

The goal being making them consistent, the text and configuration variable
(which are user-facing names) should match variables and functions (which
are internal names).  It would be inconsistent to store the value of the
xfer.denyupdatetag configuration in deny_moving_tags variable, no?

I wondered if denyupdatetag should also forbid "git tag -f"; it would be
awkward if we did so.  The configuration is only about forbidding ref
transfer operations from updating the tags.

But somehow core.denyupdatetag sounds as if "git tag -f" is also verboten
and that is why I weatherballooned xfer.* in the first paragraph of this
message.

Re: [PATCH] push: warn users about updating existing tags on push

[Tay Ray Chuan]

Hi,

On Sat, Aug 28, 2010 at 4:22 PM, Dave Olszewski <cxreg@pobox.com> wrote:
> Generally, tags are considered a write-once ref (or object), and updates
> to them are the exception to the rule.  This is evident from the
> behavior of "git fetch", which will not update a tag it already has
> unless --tags is specified, from the --force option to "git tag", and
> the fact that Git does not keep reflogs for tags.
>
> However, there is presently nothing preventing a tag from being
> fast-forwarded, which can happen intentionally or accidentally.  In both
> cases, the user should be aware that they are changing something that is
> expected to be immutable and stable.

Sounds like a pretty good idea.

I think we could also expose this as a command-line option - say, --force-tags.

Also, how will this handle a remote config like this?

  [remote "foo"]
    push = +refs/tags/*

> [snip]
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 05ec3fe..02dfc96 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> [snip]
> @@ -1545,6 +1548,10 @@ push.default::
>  * `tracking` push the current branch to its upstream branch.
>  * `current` push the current branch to a branch of the same name.
>
> +push.denyMovingTags::
> +       Whether or not a user will be allowed to push a tag that already
> +       exists on the remote for a different object.  False by default.
> +
>  rebase.stat::
>        Whether to show a diffstat of what changed upstream since the last
>        rebase. False by default.

Hmm, it's a little weird to speak of "allowing" the user to do this
and that. Perhaps

	Whether or not a push will be allowed to proceed if a tag...

> @@ -1593,6 +1600,11 @@ receive.denyNonFastForwards::
>        even if that push is forced. This configuration variable is
>        set when initializing a shared repository.
>
> +receive.denyMovingTags::
> +       If set to true, git-receive-pack will deny an update to a tag which
> +       already points to a different object.  Use this to prevent such an
> +       update via a push, even if that push is forced.
> +
>  receive.updateserverinfo::
>        If set to true, git-receive-pack will run git-update-server-info
>        after receiving data from git-push and updating refs.

Perhaps

	If set to true, git-receive-pack will refuse to update to a tag to point the
	tag to a	different object.  Use this...

> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index 658ff2f..1d53e04 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -112,7 +112,10 @@ nor in any Push line of the corresponding remotes file---see below).
>        Usually, the command refuses to update a remote ref that is
>        not an ancestor of the local ref used to overwrite it.
>        This flag disables the check.  This can cause the
> -       remote repository to lose commits; use it with care.
> +       remote repository to lose commits; use it with care.  This
> +       flag will also allow a previously pushed tag to be updated
> +       to point to a new commit, which is refused if
> +       push.denyMovingTags is set to true.
>  --repo=<repository>::
>        This option is only relevant if no <repository> argument is

Perhaps

	remote repository to lose commits; use it with care.

	Note that for tags that have already been pushed and have been updated
	locally, \--force will not update them if push.denyMovingTags is set to true.

-- 
Cheers,
Ray Chuan

Re: [PATCH] push: warn users about updating existing tags on push

[Junio C Hamano]

Tay Ray Chuan <rctay89@gmail.com> writes:

>> +push.denyMovingTags::
>> +       Whether or not a user will be allowed to push a tag that already
>> +       exists on the remote for a different object.  False by default.
>
> Hmm, it's a little weird to speak of "allowing" the user to do this
> and that. Perhaps
>
> 	Whether or not a push will be allowed to proceed if a tag...

I think that is a sensible suggestion.  Or even stronger "forbid updating
an existing tag; defaults to false".

>> +receive.denyMovingTags::
>> +       If set to true, git-receive-pack will deny an update to a tag which
>> +       already points to a different object.  Use this to prevent such an
>> +       update via a push, even if that push is forced.
>> +
>
> Perhaps
>
> 	If set to true, git-receive-pack will refuse to update to a tag to point the
> 	tag to a	different object.  Use this...

Sounds better.

>> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
>> index 658ff2f..1d53e04 100644
>> --- a/Documentation/git-push.txt
>> +++ b/Documentation/git-push.txt
>> @@ -112,7 +112,10 @@ nor in any Push line of the corresponding remotes file---see below).
>>        Usually, the command refuses to update a remote ref that is
>>        not an ancestor of the local ref used to overwrite it.
>>        This flag disables the check.  This can cause the
>> -       remote repository to lose commits; use it with care.
>> +       remote repository to lose commits; use it with care.  This
>> +       flag will also allow a previously pushed tag to be updated
>> +       to point to a new commit, which is refused if
>> +       push.denyMovingTags is set to true.
>
> Perhaps
>
> 	remote repository to lose commits; use it with care.
>
> 	Note that for tags that have already been pushed and have been updated
> 	locally, \--force will not update them if push.denyMovingTags is set to true.

I don't think the change to this section is necessary, _unless_ existing
mention of "remote ref" is changed to "remote branch" to exclude tags.  If
we wanted to say something, probably

    Note that the above applies both to branches and tags.

would be sufficient.  I don't think this is a place to enumerate
exceptions like this new configuration and all the other existing ones
(e.g. denynonfastforwards, denycurrentbranch, denydeletecurrent etc.)