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

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

Jeff King [off-list ref] writes:

quoted

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