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

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

Thanks for the critique and comments

Dave Olszewski [off-list ref] writes:

quoted

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?

quoted

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.

quoted

@@ -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.

quoted

+		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.

quoted

+				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.

quoted

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