Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish
Chris Rorvick ch...@rorvick.com writes: 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. Sounds sensible. Perhaps some of that thinking behind this change (i.e. earlier we checked the forwardee was any commit-ish, but the new code only allows a commit object if it were to be fast-forwarded) belongs to the log message? Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish
Chris Rorvick ch...@rorvick.com 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 ch...@rorvick.com --- 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. With this code, the old must be a commit but new can be a tag that points at a commit? Why? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish
On Mon, Nov 26, 2012 at 12:53 PM, Junio C Hamano gits...@pobox.com wrote: Chris Rorvick ch...@rorvick.com 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 ch...@rorvick.com --- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
