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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 16, 2017, 15:58 UTC
Message-ID: <xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAP8UFD2jLdR7HTi-T6J_xWvxKyoQf_6pUTX1CWsd9v3TXh9FXw@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 10 quoted lines

>>> 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, 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?

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.

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. Or the other side wanted to ask for one of the capabilities we advertised, but the code has typo and their wish to enable a capability that its correct operation relies on is not understood on this end. The result is the same garbage-in-garbage-out.

So "program X asked capability C, which we cannot handle" is a good diagnosis to give and it is a good change to spell out the name of the offending program that needs to be fixed, but at the same time, I do not think it is safe to continue without the other side knowing that one of the capabilities they need to correctly operate cannot be turned on.