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

From: Jeff King <peff@peff.net>
Date: Jan 21, 2013, 23:40 UTC
Message-ID: <20130121234002.GE17156@sigill.intra.peff.net>
In-Reply-To: <CAEUsAPZr+bNNA-pqrQbBGvku4T3h58Ub66mK2zLeHqghEKw5Aw@mail.gmail.com>

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

Show 8 quoted lines

> 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".

Show 10 quoted lines

> > 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