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