Re: [PATCH v4 6/6] fetch: stop clobbering existing tags without --force

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  +
> -Unlike when pushing with linkgit:git-push[1], any updates to
> -`refs/tags/*` will be accepted without `+` in the refspec (or
> -`--force`). The receiving promiscuously considers all tag updates from
> -a remote to be forced fetches.
> +Until Git version 2.20, and unlike when pushing with
> +linkgit:git-push[1], any updates to `refs/tags/*` would be accepted
> +without `+` in the refspec (or `--force`). The receiving promiscuously
> +considered all tag updates from a remote to be forced fetches. Since
> +Git version 2.20 updates to `refs/tags/*` work the same way as when
> +pushing. I.e. any updates will be rejected without `+` in the refspec
> +(or `--force`).

Have a comma after 2.20; otherwise it was unreadable, at least to
me, who took three attempts before realizing that the "updates" is
not a verb whose subject is "Git version 2.20".  Or

	Since Git version 2.20, fetching to update `refs/tags/*`
	work the same way as pushing into it

perhaps.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b0706b3803..ed4ed9d8c4 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -667,12 +667,18 @@ static int update_local_ref(struct ref *ref,
>  
>  	if (!is_null_oid(&ref->old_oid) &&
>  	    starts_with(ref->name, "refs/tags/")) {
> -		int r;
> -		r = s_update_ref("updating tag", ref, 0);
> -		format_display(display, r ? '!' : 't', _("[tag update]"),
> -			       r ? _("unable to update local ref") : NULL,
> -			       remote, pretty_ref, summary_width);
> -		return r;
> +		if (force || ref->force) {
> +			int r;
> +			r = s_update_ref("updating tag", ref, 0);
> +			format_display(display, r ? '!' : 't', _("[tag update]"),
> +				       r ? _("unable to update local ref") : NULL,
> +				       remote, pretty_ref, summary_width);
> +			return r;
> +		} else {
> +			format_display(display, '!', _("[rejected]"), _("would clobber existing tag"),
> +				       remote, pretty_ref, summary_width);
> +			return 1;
> +		}
>  	}

A straight-forward change to turn an unconditional update to either
an unconditonal rejection (when force is not given) or an
unconditional acceptance (when forced), which makes sense and has
near-zero chance of being wrong ;-)

It is a huge change in behaviour, but in a very good way.  I'd
imagine that users will welcome it very much.