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

From: Christian Couder <christian.couder@gmail.com>
Date: Aug 17, 2017, 05:34 UTC
Message-ID: <CAP8UFD1H4Pb5e2_pioQ5neROc+64e55RfvRhiyz5Df5AwJg-FQ@mail.gmail.com>
In-Reply-To: <xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com>

On Wed, Aug 16, 2017 at 5:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> 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...

Show 8 quoted lines

> 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.