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

[Jeff King <peff@peff.net>]

On Thu, Jan 17, 2013 at 09:18:50PM -0600, Chris Rorvick wrote:

> On Thu, Jan 17, 2013 at 7:06 PM, Jeff King <peff@peff.net> wrote:
> > However, if instead of the rule being
> > "blobs on the remote side cannot be replaced", if it becomes "the old
> > value on the remote side must be referenced by what we replace it with",
> > that _is_ something we can calculate reliably on the sending side.
> 
> Interesting.  I would have thought knowing reachability implied having
> the old object in the sending repository.

No, because if you do not have it, then you know it is not reachable
from your refs (or your repository is corrupted). If you do have it, it
_might_ be reachable. For commits, checking is cheap (merge-base) and we
already do it. For trees and blobs, it is much more expensive, as you
have to walk the whole object graph.  While it might be "more correct"
in some sense to say "it's OK to replace a tree with a commit that
points to it", in practice I doubt anyone cares, so you can probably
just punt on those ones and say "no, it's not a fast forward".

> > And
> > that is logically an extension of the fast-forward rule, which is why I
> > suggested placing it with ref_newer (but the latter should probably be
> > extended to not suggest merging if we _know_ it is a non-commit object).
> 
> Sounds great, especially if it is not dependent on the sender actually
> having the old object.  Until this is implemented, though, I don't
> understand what was wrong with doing the checks in the
> is_forwardable() helper function (of course after fixing the
> regression/bug.)

I don't think it is wrong per se; I just think that the check would go
more naturally where we are checking whether the object does indeed
fast-forward. Because is_forwardable in some cases must say "I don't
know; I don't have the object to check its type, so maybe it is
forwardable, and maybe it is not". Whereas when we do the actual
reachability check, we can say definitely "this is not reachable because
I don't have it, or this is not reachable because it is a commit and I
checked, or this might be reachable but I don't care to check because it
has a funny type".

I think looking at it as the latter makes it more obvious how to handle
the "maybe" situation (e.g., the bug in is_forwardable was hard to see).

Anyway, I do not care that much where it goes. To me, the important
thing is the error message. I do think the error "already exists" is a
reasonable one for refs/tags (we do not allow non-force pushes of
existing tags), but not necessarily for other cases, like trying to push
a blob over a blob. The problem there is not "already exists" but rather
"a blob is not something that can fast-forward". Using the existing
REJECT_NONFASTFORWARD is insufficient (because later code will recommend
pull-then-push, which is wrong). So I'd be in favor of creating a new
error status for it.

-Peff