Chris Rorvick <[email protected]> writes:
> 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.
> 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?".
> 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?
> 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?
> - 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.
> +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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
