Re: [PATCH v5 03/10] transport-helper: add 'force' to 'export' helpers

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> On Thu, Oct 31, 2013 at 12:21 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>>
>>>>> Otherwise they cannot know when to force the push or not (other than
>>>>> hacks).
>>>>> ...
>>>>> diff --git a/transport-helper.c b/transport-helper.c
>>>>> index d05fc7c..ed238e5 100644
>>>>> --- a/transport-helper.c
>>>>> +++ b/transport-helper.c
>>>>> @@ -854,6 +854,11 @@ static int push_refs_with_export(struct transport *transport,
>>>>>                       die("helper %s does not support dry-run", data->name);
>>>>>       }
>>>>>
>>>>> +     if (flags & TRANSPORT_PUSH_FORCE) {
>>>>> +             if (set_helper_option(transport, "force", "true") != 0)
>>>>> +                     die("helper %s does not support 'force'", data->name);
>>>>> +     }
>>>>> +
>>>>
>>>> Does this cause a "git push --force $there A:B" to fail when $there
>>>> is a destination that goes via an existing helper does not suport
>>>> "force" option?
>>>
>>> Yes.
>>>
>>>> Should it fail even when the current value of B is an ancestor of A
>>>> (i.e. when an unforced push would succeed)?
>>>
>>> It might make sense to fail only when the push is non-fast-forward,
>>> but it's not so straight-forward to implement.
>>
>> OK; let's see if anybody screams by cooking the series in 'next'
>> (that is, when other issues people may discover in the series are
>> addressed).
>>
>> Thanks.
>
> Actually, thinking about it again, would it be a better alternative
> to issue an error message and continue, instead of dying here
> (i.e. replace the above die() with error() or even warning())?
>
> That way, if the helper has not been updated to support 'force', but
> the user has been happily accepting the result he gets from "git
> push $there +master" (perhaps because he has never pushed a non-ff
> history so far), he now gets a warning that essentially says that
> his push has been working by accident, even when the particular
> ff-push that showed the error message goes through successfully.
>
> If his push through the helper that has not been updated does need
> 'force', he gets the same old behaviour, possibly a broken one, out
> of the helper, and again he does get the warning.
>
> Because updated helpers know 'force' option, such a s/die/warning/
> change would not affect them at all.