Re: [PATCH v4.1 5/5] push: update remote tags only with force

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 19, 2012, 20:23 UTC
Message-ID: <7va9udxryf.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1353189237-19491-1-git-send-email-chris@rorvick.com>

Chris Rorvick <chris@rorvick.com> writes:

Show 22 quoted lines

> References are allowed to update from one commit-ish to another if the
> former is a ancestor of the latter.  This behavior is oriented to
> branches which are expected to move with commits.  Tag references are
> expected to be static in a repository, though, thus an update to a
> tag (lightweight and annotated) should be rejected unless the update is
> forced.
>
> To enable this functionality, the following checks have been added to
> set_ref_status_for_push() for updating refs (i.e, not new or deletion)
> to restrict fast-forwarding in pushes:
>
>   1) The old and new references must be commits.  If this fails,
>      it is not a valid update for a branch.
>
>   2) The reference name cannot start with "refs/tags/".  This
>      catches lightweight tags which (usually) point to commits
>      and therefore would not be caught by (1).
>
> If either of these checks fails, then it is flagged (by default) with a
> status indicating the update is being rejected due to the reference
> already existing in the remote.  This can be overridden by passing
> --force to git push.

This, if it were implemented back when we first added "git push", would have been a nice safety, but added after 1.8.0 it would be a regression, so we would need backward compatibility notes in the release notes.

Not an objection; just making a mental note.

Show 15 quoted lines

> diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> index fe46c42..479e25f 100644
> --- a/Documentation/git-push.txt
> +++ b/Documentation/git-push.txt
> @@ -51,11 +51,11 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be
>  updated.
>  +
>  The object referenced by <src> is used to update the <dst> reference
> -on the remote side, but by default this is only allowed if the
> -update can fast-forward <dst>.  By having the optional leading `+`,
> -you can tell git to update the <dst> ref even when the update is not a
> -fast-forward.  This does *not* attempt to merge <src> into <dst>.  See
> -EXAMPLES below for details.
> +on the remote side.  By default this is only allowed if the update is
> +a branch, and then only if it can fast-forward <dst>.  By having the

I can already see the next person asking "I can update refs/notes the same way. The doc says it applies only to the branches. Is this a bug?".

Show 10 quoted lines

> diff --git a/cache.h b/cache.h
> index f410d94..127e504 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1004,13 +1004,14 @@ struct ref {
>  		requires_force:1,
>  		merge:1,
>  		nonfastforward:1,
> -		is_a_tag:1,
> +		forwardable:1,

This is somewhat an unfortunate churn. Perhaps is_a_tag could have started its life under its final name in the series?

I am assuming that the struct members will be initialized to 0 (false), so everything by default is now not forwardable if somebody forgets to set this flag?

Show 25 quoted lines

>  	enum {
>  		REF_STATUS_NONE = 0,
>  		REF_STATUS_OK,
>  		REF_STATUS_REJECT_NONFASTFORWARD,
> +		REF_STATUS_REJECT_ALREADY_EXISTS,
>  		REF_STATUS_REJECT_NODELETE,
>  		REF_STATUS_UPTODATE,
>  		REF_STATUS_REMOTE_REJECT,
> diff --git a/remote.c b/remote.c
> index 44e72d6..a723f7a 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1311,14 +1311,24 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
>  		 *     to overwrite it; you would not know what you are losing
>  		 *     otherwise.
>  		 *
> -		 * (3) if both new and old are commit-ish, and new is a
> -		 *     descendant of old, it is OK.
> +		 * (3) if new is commit-ish and old is a commit, new is a
> +		 *     descendant of old, and the reference is not of the
> +		 *     refs/tags/ hierarchy it is OK.
>  		 *
>  		 * (4) regardless of all of the above, removing :B is
>  		 *     always allowed.
>  		 */

I think this is unreadable.  Isn't this more like

    (1.5) if the destination is inside refs/tags/ and already
          exists, you are not allowed to overwrite it without
          --force or +A:B notation.

early in the rule set?

Show 8 quoted lines

> -		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
> +		if (prefixcmp(ref->name, "refs/tags/")) {
> +			/* Additionally, disallow fast-forwarding if
> +			 * the old object is not a commit.  This kicks
> +			 * out annotated tags that might pass the
> +			 * is_newer() test but dangle if the reference
> +			 * is updated.
> +			 */

Huh? prefixcmp() excludes refs/tags/ hierarchy. What is an annotated tag doing there? Is this comment outdated???

	/*
         * Also please end the first line of a multi-line
         * comment with '/', '*', and nothing else.
         */

Regarding the other arm of this "if (not in refs/tags/ hierarchy)", what happens when refs/tags/foo is a reference to a blob or a tree? This series declares that the point of tag is not to let people to move it willy-nilly, and I think it is in line with its spirit if you just rejected non-creation events.

Also, I suspect that you do not even need to have old object locally when looking at refs/tags/ hierarchy. "Do not overwrite tags" can be enforced by only noticing that they already have something; you do not know what that something actually is to prevent yourself from overwriting it. You may have to update the rule (2) in remote.c around l.1311 for this.

Show 8 quoted lines

> +test_expect_success 'push requires --force to update lightweight tag' '
> +	mk_test heads/master &&
> +	mk_child child1 &&
> +	mk_child child2 &&
> +	(
> +		cd child1 &&
> +		git tag Tag &&
> +		git push ../child2 Tag &&

Don't you want to make sure that another "git push ../child2 Tag" at this point, i.e. attempt to update Tag with the same, should succeed without --force?