On Mon, Nov 26, 2012 at 12:53 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Chris Rorvick <chris@xxxxxxxxxxx> writes: > >> Pushes must already (by default) update to a commit-ish due the fast- >> forward check in set_ref_status_for_push(). But rejecting for not being >> a fast-forward suggests the situation can be resolved with a merge. >> Flag these updates (i.e., to a blob or a tree) as not forwardable so the >> user is presented with more appropriate advice. >> >> Signed-off-by: Chris Rorvick <chris@xxxxxxxxxxx> >> --- >> remote.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/remote.c b/remote.c >> index f5bc4e7..ee0c1e5 100644 >> --- a/remote.c >> +++ b/remote.c >> @@ -1291,6 +1291,11 @@ static inline int is_forwardable(struct ref* ref) >> if (!o || o->type != OBJ_COMMIT) >> return 0; >> >> + /* new object must be commit-ish */ >> + o = deref_tag(parse_object(ref->new_sha1), NULL, 0); >> + if (!o || o->type != OBJ_COMMIT) >> + return 0; >> + > > I think the original code used ref_newer() which took commit-ish on > both sides. That is still called later in set_ref_status_for_push() to calculate the nonfastforward flag. The only reason for even checking the new here is to exclude trees and blobs now so they are flagged as already-existing and thus avoid nonsensical fetch-and-merge advice. Otherwise the behavior is unchanged by this last patch. ref_newer() does end up redoing computation now done in the new is_forwardable() function. I could probably factor this out of ref_newer() into a commit_newer() function that could be reused in set_ref_status_for_push() to avoid this overhead, but it didn't seem like a big deal. Thoughts? > With this code, the old must be a commit but new can be a tag that > points at a commit? Why? The old must not be a tag because fast-forwarding from it is potentially destructive; a tag would likely be left dangling in this case. This is not true for the new, though. I'm not sure fast-forwarding from a commit to a tag is useful, but I didn't see a reason to prevent it either. The refs/tags/ hierarchy is excluded from fast-forwarding before this check, and refs/heads/ is already protected against anything but commits. So it seems very unlikely that someone would accidentally make use of this behavior. So, fast-forwarding to a tag seemed fairly benign and unlikely to cause confusion, so I leaned towards allowing it in case someone found a use case for it. -- 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