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

[Chris Rorvick <chris@rorvick.com>]

On Mon, Nov 26, 2012 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.