On Mon, Jan 21, 2013 at 5:40 PM, Jeff King <peff@xxxxxxxx> wrote: > 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@xxxxxxxx> 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". Thanks for explaining this further. I'm not exactly sure what I was thinking when I wrote the above other than I didn't fully grasp you point and responded in a confused state. Clear on all fronts now. >> > 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. I agree with everything above. I just don't understand why reverting the "already exists" behavior for non-commit-ish objects was a prerequisite to fixing this. Despite the flaws (I am not referring to the buggy behavior) you and Junio have pointed out, this still seems like an improvement over the previous (and soon-to-be current) behavior. Saying the remote reference already exists is true, and it implies that removing it might solve the problem which is also true. Adding another error status will allow the error message to be made clearer in both cases (i.e., I avoided the word "tag" specifically so that it would apply to other cases, or so I thought.) Chris -- 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