Re: [PATCH] sub-process: print the cmd when a capability is unsupported

[Christian Couder <christian.couder@gmail.com>]

On Wed, Aug 16, 2017 at 5:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>>>> I am still wondering if protocol errors should be fatal,
>>>
>>> Yes, please.
>>
>> Unfortunately I think it would prevent new filters or new
>> sub-processes to work with older versions of Git.
>>
>> For example if filters are upgraded company wide to support the new
>> "delay" capability, that would force everyone using the filters to
>> upgrade Git.
>
> I must say that your filter is broken in that case,

Perhaps it is just sloppily written.

> and it is much
> more prudent to die than continuing.  Why is that upgraded filter
> asking for "delay" to an older Git that does not yet know it in the
> first place?

Maybe because in our tests (like in t/t0021/rot13-filter.pl) the
filter just outputs all its capabilities, so the filter writer thought
it should be ok to do the same.

> I just re-read the subprocess_handshake() codepath, and here is my
> understand.  The handshake_capabilities() function first advertises
> the set of capabilities it supports, so that the other side can pick
> and choose which ones to use and ask us to enable in its response.

Yeah, that sounds like the right thing the filter should do. Though I
think that if we really want the filters/subprocesses to always do
this, we have some work on our plate...

> The code under discussion in this thread comes after that, where we
> read the response that tells us what choice the other side made.  If
> we saw something that we never advertised, that indicates one of two
> things.  The other side, i.e. the "upgraded" filter, is not paying
> attention of the capabilities advertisement, and asking something
> its correct operation relies on, but we are not capable of giving
> that unknown feature and operate without it, so after that point the
> exchange of data is a garbage-in-garbage-out.

Maybe it is not paying attention and just following the bad example of
giving all the capabilities it supports even if it can work if some of
them are not supported.

In this case if we error out, we prevent everything to work even if it
could work if we just also "ignored" (though printing a warning is not
exactly ignoring and is the right and the least thing to do) what the
filter told us.

Anyway I don't really mind being very strict and just erroring out in
this case, but I think we should then emphasize more in our test
scripts (maybe by giving a good example) and perhaps also in the doc
that the filters/sub-processes should really pay attention and not
output any capability that are not supported by Git.