Re: [PATCH 7/7] push: clarify rejection of update to non-commit-ish

From: Chris Rorvick <chris@rorvick.com>
Date: Nov 27, 2012, 03:52 UTC
Message-ID: <CAEUsAPZq1POKQZd5vZy55nEj2kB4CqgxL9_d_0sQs33P_Gcemg@mail.gmail.com>
In-Reply-To: <7vk3t8ryap.fsf@alter.siamese.dyndns.org>

On Mon, Nov 26, 2012 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> Chris Rorvick <chris@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 <chris@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.