> On 11 Sep 2017, at 05:27, Junio C Hamano <gits...@pobox.com> wrote:
>
> Junio C Hamano <gits...@pobox.com> writes:
>
>> I still think we would want to turn warning() to die(), but it
>> probably is better to do so in a separate follow-up patch. That
>> will give us a good place to record the reason why the current "just
>> call a warning() and pretend as if nothing bad happend" is wrong.
>
> And here is such an update. It seems that pretty much all comments
> in the original thread were "warning is wrong--we should die here",
> but nobody seems to have bothered following it through.
>
> cf. <20170815111725.5d009...@twelve2.svl.corp.google.com>
>
> -- >8 --
> Subject: [PATCH] subprocess: loudly die when subprocess asks for an
> unsupported capability
>
> 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. Then 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.
>
> Instead of sweeping such a potential bug under the rug, die loudly
> when we see a request for an unsupported capability in order to
> force sloppily-written filter scripts to get corrected.
>
> Signed-off-by: Junio C Hamano <gits...@pobox.com>
> ---
> sub-process.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sub-process.c b/sub-process.c
> index fcc4832c14..ec9a51b7b1 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -181,8 +181,8 @@ static int handshake_capabilities(struct child_process
> *process,
> if (supported_capabilities)
> *supported_capabilities |= capabilities[i].flag;
> } else {
> - warning("subprocess '%s' requested unsupported
> capability '%s'",
> - process->argv[0], p);
> + die("subprocess '%s' requested unsupported capability
> '%s'",
> + process->argv[0], p);
Looks good!
Thank you,
Lars
