Re: [PATCH v6 0/8] push: update remote tags only with force

From: Jeff King <peff@peff.net>
Date: Jan 16, 2013, 17:43 UTC
Message-ID: <20130116174325.GA27525@sigill.intra.peff.net>
In-Reply-To: <7vfw21xde5.fsf@alter.siamese.dyndns.org>

On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote:

Show 6 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > I.e., we trigger the "!o" branch after the parse_object in your example.
> 
> Heh, I didn't see this message until now (gmane seems to be lagging
> a bit).

I think it is vger lagging, actually.

Show 7 quoted lines

> I am very tempted to do this.
> 
>  * Remove unnecessary not_forwardable from "struct ref"; it is only
>    used inside set_ref_status_for_push();
> 
>  * "refs/tags/" is the only hierarchy that cannot be replaced
>    without --force;

Agreed.

>  * Remove the misguided attempt to force that everything that
>    updates an existing ref has to be a commit outside "refs/tags/"
>    hierarchy.  This code does not know what kind of objects the user
>    wants to place in "refs/frotz/" hierarchy it knows nothing about.

I agree with what your patch does, but my thinking is a bit different.

My original suggestion with respect to object types was that the rule for --force should be "do not ever lose any objects without --force". So a fast-forward is OK, as the new objects reference the old. A non-fast forward is not, because objects become unreferenced. Replacing a tag object is not OK, even if it points to the same commit, as you are losing the old tag object (replacing an object with a tag that points to the original object or its descendent is OK in theory, though I doubt it is common enough to worry about).

I think that is a reasonable rule that could be applied across all parts of the namespace hierarchy. And it could be applied by the client, because all you need to know is whether ref->old_sha1 is reachable from ref->new_sha1.

But it is somewhat orthogonal to the "already exists" idea, and checking refs/tags/. Those ideas are about enforcing sane rules on the tag hierarchy. My rule is a safety valve that is meant to extend the idea of "is fast-forwardable" to non-commit object types. If we do it at all, it should be part of the fast-forward check (e.g., as part of ref_newer).

The current code conflates the two under the "already exists" condition, which is just wrong. I think the best thing at this point is to split the two ideas apart, keep the refs/tags check (and translate it to "already exists" in the UI, as we do), and table the safety valve. I am not even sure if it is something that is useful, and it can come later if we decide it is.

Show 11 quoted lines

> I feel moderately strongly about the last point.  Defining special
> semantics for one hierarchy (e.g. "refs/tags/") and implementing a
> policy for enforcement is one thing, but a random policy that
> depends on object type that applies globally is simply insane.  The
> user may want to do "refs/tested/" hierarchy that is meant to hold
> references to commit, with one annotated tag "refs/tested/latest"
> that points at the "latest tested version" with some commentary, and
> maintain the latter by keep pushing to it.  If that is the semantics
> the user wanted to ahve in the "refs/tested/" hierarchy, it is not
> reasonable to require --force for such a workflow.  The user knows
> better than Git in such a case.

I see what you are saying, but I think the ship has already sailed to some degree. We already implement the non-fast-forward check everywhere, and I cannot have a "refs/tested" hierarchy that pushes arbitrary commits without regard to their history. If I have such a hierarchy, I have to use "--force" (or more likely, mark the refspec with "+").

In my mind, the object-type checking is just making that fast-forward check more thorough (i.e., extending it to non-commit objects).

>  cache.h               |  1 -
>  remote.c              | 24 +-----------------------
>  t/t5516-fetch-push.sh | 21 ---------------------
>  3 files changed, 1 insertion(+), 45 deletions(-)

The patch itself looks fine to me. Whether we agree on the fast-forward object-type checking or not, it is the correct first step to take in either case.

-Peff