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

> 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?

> > 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?