On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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. > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html