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

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

[Christian Couder]

In handshake_capabilities() we use warning() when a capability
is not supported, so the exit code of the function is 0 and no
further error is shown. This is a problem because the warning
message doesn't tell us which subprocess cmd failed.

On the contrary if we cannot write a packet from this function,
we use error() and then subprocess_start() outputs:

    initialization for subprocess '<cmd>' failed

so we can know which subprocess cmd failed.

Let's improve the warning() message, so that we can know which
subprocess cmd failed.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 sub-process.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sub-process.c b/sub-process.c
index 6edb97c1c6..6b133f8dce 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,
 
 static int handshake_capabilities(struct child_process *process,
 				  struct subprocess_capability *capabilities,
-				  unsigned int *supported_capabilities)
+				  unsigned int *supported_capabilities,
+				  const char *cmd)
 {
 	int i;
 	char *line;
@@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,
 			if (supported_capabilities)
 				*supported_capabilities |= capabilities[i].flag;
 		} else {
-			warning("external filter requested unsupported filter capability '%s'",
-				p);
+			warning("subprocess '%s' requested unsupported capability '%s'",
+				cmd, p);
 		}
 	}
 
@@ -206,8 +207,10 @@ int subprocess_handshake(struct subprocess_entry *entry,
 
 	retval = handshake_version(process, welcome_prefix, versions,
 				   chosen_version) ||
-		 handshake_capabilities(process, capabilities,
-					supported_capabilities);
+		 handshake_capabilities(process,
+					capabilities,
+					supported_capabilities,
+					entry->cmd);
 
 	sigchain_pop(SIGPIPE);
 	return retval;
-- 
2.14.1.187.gd6d46550f4

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

[Jonathan Tan]

On Tue, 15 Aug 2017 19:36:11 +0200
Christian Couder <christian.couder@gmail.com> wrote:

> In handshake_capabilities() we use warning() when a capability
> is not supported, so the exit code of the function is 0 and no
> further error is shown. This is a problem because the warning
> message doesn't tell us which subprocess cmd failed.
> 
> On the contrary if we cannot write a packet from this function,
> we use error() and then subprocess_start() outputs:
> 
>     initialization for subprocess '<cmd>' failed
> 
> so we can know which subprocess cmd failed.
> 
> Let's improve the warning() message, so that we can know which
> subprocess cmd failed.
> 
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>

This looks reasonable to me.

I am still wondering if protocol errors should be fatal, but that is
unrelated to this patch.

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

[Lars Schneider]

> On 15 Aug 2017, at 19:36, Christian Couder <christian.couder@gmail.com> wrote:
> 
> In handshake_capabilities() we use warning() when a capability
> is not supported, so the exit code of the function is 0 and no
> further error is shown. This is a problem because the warning
> message doesn't tell us which subprocess cmd failed.
> 
> On the contrary if we cannot write a packet from this function,
> we use error() and then subprocess_start() outputs:
> 
>    initialization for subprocess '<cmd>' failed
> 
> so we can know which subprocess cmd failed.
> 
> Let's improve the warning() message, so that we can know which
> subprocess cmd failed.
> 
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> sub-process.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/sub-process.c b/sub-process.c
> index 6edb97c1c6..6b133f8dce 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,
> 
> static int handshake_capabilities(struct child_process *process,
> 				  struct subprocess_capability *capabilities,
> -				  unsigned int *supported_capabilities)
> +				  unsigned int *supported_capabilities,
> +				  const char *cmd)
> {
> 	int i;
> 	char *line;
> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,
> 			if (supported_capabilities)
> 				*supported_capabilities |= capabilities[i].flag;
> 		} else {
> -			warning("external filter requested unsupported filter capability '%s'",
> -				p);
> +			warning("subprocess '%s' requested unsupported capability '%s'",
> +				cmd, p);

Wouldn't it be possible to use "process->argv[0]"? 
Shouldn't that be the same as "cmd"?

- Lars

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

[Ben Peart]

On 8/15/2017 1:36 PM, Christian Couder wrote:
> In handshake_capabilities() we use warning() when a capability
> is not supported, so the exit code of the function is 0 and no
> further error is shown. This is a problem because the warning
> message doesn't tell us which subprocess cmd failed.
> 
> On the contrary if we cannot write a packet from this function,
> we use error() and then subprocess_start() outputs:
> 
>      initialization for subprocess '<cmd>' failed
> 
> so we can know which subprocess cmd failed.
> 
> Let's improve the warning() message, so that we can know which
> subprocess cmd failed.
> 
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>   sub-process.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/sub-process.c b/sub-process.c
> index 6edb97c1c6..6b133f8dce 100644
> --- a/sub-process.c
> +++ b/sub-process.c
> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,
>   
>   static int handshake_capabilities(struct child_process *process,
>   				  struct subprocess_capability *capabilities,
> -				  unsigned int *supported_capabilities)
> +				  unsigned int *supported_capabilities,
> +				  const char *cmd)
>   {
>   	int i;
>   	char *line;
> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,
>   			if (supported_capabilities)
>   				*supported_capabilities |= capabilities[i].flag;
>   		} else {
> -			warning("external filter requested unsupported filter capability '%s'",
> -				p);
> +			warning("subprocess '%s' requested unsupported capability '%s'",
> +				cmd, p);
>   		}
>   	}
>   
> @@ -206,8 +207,10 @@ int subprocess_handshake(struct subprocess_entry *entry,
>   
>   	retval = handshake_version(process, welcome_prefix, versions,
>   				   chosen_version) ||
> -		 handshake_capabilities(process, capabilities,
> -					supported_capabilities);
> +		 handshake_capabilities(process,
> +					capabilities,
> +					supported_capabilities,
> +					entry->cmd);
>   
>   	sigchain_pop(SIGPIPE);
>   	return retval;
> 

Looks good to me.

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

[Junio C Hamano]

Lars Schneider <larsxschneider@gmail.com> writes:

>> On 15 Aug 2017, at 19:36, Christian Couder <christian.couder@gmail.com> wrote:
>> 
>> In handshake_capabilities() we use warning() when a capability
>> is not supported, so the exit code of the function is 0 and no
>> further error is shown. This is a problem because the warning
>> message doesn't tell us which subprocess cmd failed.
>> 
>> On the contrary if we cannot write a packet from this function,
>> we use error() and then subprocess_start() outputs:
>> 
>>    initialization for subprocess '<cmd>' failed
>> 
>> so we can know which subprocess cmd failed.
>> 
>> Let's improve the warning() message, so that we can know which
>> subprocess cmd failed.
>> 
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> sub-process.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>> 
>> diff --git a/sub-process.c b/sub-process.c
>> index 6edb97c1c6..6b133f8dce 100644
>> --- a/sub-process.c
>> +++ b/sub-process.c
>> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,
>> 
>> static int handshake_capabilities(struct child_process *process,
>> 				  struct subprocess_capability *capabilities,
>> -				  unsigned int *supported_capabilities)
>> +				  unsigned int *supported_capabilities,
>> +				  const char *cmd)
>> {
>> 	int i;
>> 	char *line;
>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,
>> 			if (supported_capabilities)
>> 				*supported_capabilities |= capabilities[i].flag;
>> 		} else {
>> -			warning("external filter requested unsupported filter capability '%s'",
>> -				p);
>> +			warning("subprocess '%s' requested unsupported capability '%s'",
>> +				cmd, p);
>
> Wouldn't it be possible to use "process->argv[0]"? 
> Shouldn't that be the same as "cmd"?

It is good to see many people are in agreement and in favor of
giving more information.  It is even better to see somebody noticing
a room for improvement ;-)

I also wonder if this should be left as a silent warning that the
caller cannot notice as in the corrent code.  Dying here might not
be desirable for some callers, but even if we don't die here, we may
want to give the caller a chance to react to the protocol error.

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

[Christian Couder]

On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> On 15 Aug 2017, at 19:36, Christian Couder <christian.couder@gmail.com> wrote:
>>
>> In handshake_capabilities() we use warning() when a capability
>> is not supported, so the exit code of the function is 0 and no
>> further error is shown. This is a problem because the warning
>> message doesn't tell us which subprocess cmd failed.
>>
>> On the contrary if we cannot write a packet from this function,
>> we use error() and then subprocess_start() outputs:
>>
>>    initialization for subprocess '<cmd>' failed
>>
>> so we can know which subprocess cmd failed.
>>
>> Let's improve the warning() message, so that we can know which
>> subprocess cmd failed.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>> sub-process.c | 13 ++++++++-----
>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/sub-process.c b/sub-process.c
>> index 6edb97c1c6..6b133f8dce 100644
>> --- a/sub-process.c
>> +++ b/sub-process.c
>> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,
>>
>> static int handshake_capabilities(struct child_process *process,
>>                                 struct subprocess_capability *capabilities,
>> -                               unsigned int *supported_capabilities)
>> +                               unsigned int *supported_capabilities,
>> +                               const char *cmd)
>> {
>>       int i;
>>       char *line;
>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,
>>                       if (supported_capabilities)
>>                               *supported_capabilities |= capabilities[i].flag;
>>               } else {
>> -                     warning("external filter requested unsupported filter capability '%s'",
>> -                             p);
>> +                     warning("subprocess '%s' requested unsupported capability '%s'",
>> +                             cmd, p);
>
> Wouldn't it be possible to use "process->argv[0]"?
> Shouldn't that be the same as "cmd"?

Well in sub-process.h there is:

/* Members should not be accessed directly. */
struct subprocess_entry {
    struct hashmap_entry ent; /* must be the first member! */
    const char *cmd;
    struct child_process process;
};

so if cmd is always the same as process->argv[0], maybe there is no
need for the cmd member in the first place?

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

[Christian Couder]

On Tue, Aug 15, 2017 at 9:29 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>>
>>> On 15 Aug 2017, at 19:36, Christian Couder <christian.couder@gmail.com> wrote:

>>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,
>>>                       if (supported_capabilities)
>>>                               *supported_capabilities |= capabilities[i].flag;
>>>               } else {
>>> -                     warning("external filter requested unsupported filter capability '%s'",
>>> -                             p);
>>> +                     warning("subprocess '%s' requested unsupported capability '%s'",
>>> +                             cmd, p);
>>
>> Wouldn't it be possible to use "process->argv[0]"?
>> Shouldn't that be the same as "cmd"?
>
> Well in sub-process.h there is:
>
> /* Members should not be accessed directly. */
> struct subprocess_entry {
>     struct hashmap_entry ent; /* must be the first member! */
>     const char *cmd;
>     struct child_process process;
> };
>
> so if cmd is always the same as process->argv[0], maybe there is no
> need for the cmd member in the first place?

In case it is not clear, what I mean is that if we consider that they
should always be the same, it could be considered a different patch
altogether to just remove the cmd member of this struct.

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

[Lars Schneider]

> On 15 Aug 2017, at 21:29, Christian Couder <christian.couder@gmail.com> wrote:
> 
> On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider
> <larsxschneider@gmail.com> wrote:
>> 
>>> On 15 Aug 2017, at 19:36, Christian Couder <christian.couder@gmail.com> wrote:
>>> 
>>> In handshake_capabilities() we use warning() when a capability
>>> is not supported, so the exit code of the function is 0 and no
>>> further error is shown. This is a problem because the warning
>>> message doesn't tell us which subprocess cmd failed.
>>> 
>>> On the contrary if we cannot write a packet from this function,
>>> we use error() and then subprocess_start() outputs:
>>> 
>>>   initialization for subprocess '<cmd>' failed
>>> 
>>> so we can know which subprocess cmd failed.
>>> 
>>> Let's improve the warning() message, so that we can know which
>>> subprocess cmd failed.
>>> 
>>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>>> ---
>>> sub-process.c | 13 ++++++++-----
>>> 1 file changed, 8 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/sub-process.c b/sub-process.c
>>> index 6edb97c1c6..6b133f8dce 100644
>>> --- a/sub-process.c
>>> +++ b/sub-process.c
>>> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,
>>> 
>>> static int handshake_capabilities(struct child_process *process,
>>>                                struct subprocess_capability *capabilities,
>>> -                               unsigned int *supported_capabilities)
>>> +                               unsigned int *supported_capabilities,
>>> +                               const char *cmd)
>>> {
>>>      int i;
>>>      char *line;
>>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,
>>>                      if (supported_capabilities)
>>>                              *supported_capabilities |= capabilities[i].flag;
>>>              } else {
>>> -                     warning("external filter requested unsupported filter capability '%s'",
>>> -                             p);
>>> +                     warning("subprocess '%s' requested unsupported capability '%s'",
>>> +                             cmd, p);
>> 
>> Wouldn't it be possible to use "process->argv[0]"?
>> Shouldn't that be the same as "cmd"?
> 
> Well in sub-process.h there is:
> 
> /* Members should not be accessed directly. */
> struct subprocess_entry {
>    struct hashmap_entry ent; /* must be the first member! */
>    const char *cmd;
>    struct child_process process;
> };
> 
> so if cmd is always the same as process->argv[0], maybe there is no
> need for the cmd member in the first place?

The struct is a hash map entry. `cmd` is the key for a `process`.
Therefore, I think this is still necessary.

Does this make sense?

- Lars

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

[Christian Couder]

On Tue, Aug 15, 2017 at 9:35 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
>
>> On 15 Aug 2017, at 21:29, Christian Couder <christian.couder@gmail.com> wrote:
>>
>> On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider
>> <larsxschneider@gmail.com> wrote:
>>>
>>> Wouldn't it be possible to use "process->argv[0]"?
>>> Shouldn't that be the same as "cmd"?
>>
>> Well in sub-process.h there is:
>>
>> /* Members should not be accessed directly. */
>> struct subprocess_entry {
>>    struct hashmap_entry ent; /* must be the first member! */
>>    const char *cmd;
>>    struct child_process process;
>> };
>>
>> so if cmd is always the same as process->argv[0], maybe there is no
>> need for the cmd member in the first place?
>
> The struct is a hash map entry. `cmd` is the key for a `process`.
> Therefore, I think this is still necessary.
>
> Does this make sense?

Not sure it makes sense. A quick try to remove the cmd member, failed
t0021 with:

Initialized empty Git repository in /home/christian/git/git/t/trash
directory.t0021-conversion/repo/.git/
[master (root-commit) 56d459b] test commit 1
 Author: A U Thor <author@example.com>
 1 file changed, 1 insertion(+)
 create mode 100644 .gitattributes
--- expected.log.tmp    2017-08-15 20:27:51.658818467 +0000
+++ debug.log.tmp       2017-08-15 20:27:51.662818526 +0000
@@ -2,6 +2,6 @@
 x IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
 x IN: clean test4-empty.r 0 [OK] -- OUT: 0  [OK]
 x IN: clean testsubdir/test3 'sq',$x=.r 49 [OK] -- OUT: 49 . [OK]
-      1 START
-      1 STOP
-      1 init handshake complete
+      4 START
+      4 STOP
+      4 init handshake complete
not ok 15 - required process filter should filter data

and it is not clear to me why, so I guess you are right and I will
just use process->argv[0].

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

[Jonathan Nieder]

Jonathan Tan wrote:
> Christian Couder <christian.couder@gmail.com> wrote:

>> In handshake_capabilities() we use warning() when a capability
>> is not supported, so the exit code of the function is 0 and no
>> further error is shown. This is a problem because the warning
>> message doesn't tell us which subprocess cmd failed.
[...]
>> Let's improve the warning() message, so that we can know which
>> subprocess cmd failed.
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>
> This looks reasonable to me.
>
> I am still wondering if protocol errors should be fatal,

Yes, please.

Thanks,
Jonathan

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

[Christian Couder]

On Wed, Aug 16, 2017 at 2:22 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Tan wrote:
>> Christian Couder <christian.couder@gmail.com> wrote:
>
>>> In handshake_capabilities() we use warning() when a capability
>>> is not supported, so the exit code of the function is 0 and no
>>> further error is shown. This is a problem because the warning
>>> message doesn't tell us which subprocess cmd failed.
> [...]
>>> Let's improve the warning() message, so that we can know which
>>> subprocess cmd failed.
>>>
>>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>>
>> This looks reasonable to me.
>>
>> 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.

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

[Junio C Hamano]

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

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

[Christian Couder]

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.

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

[Junio C Hamano]

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

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

Oh, absolutely.  If you know there is such a breakage in our test
script, please do fix it.

Thanks.

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

[Lars Schneider]

> On 17 Aug 2017, at 23:01, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Christian Couder <christian.couder@gmail.com> writes:
> 
>> ... 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.
> 
> Oh, absolutely.  If you know there is such a breakage in our test
> script, please do fix it.
> 
> Thanks.

Junio's reasoning [1] is spot on from my point of view.

I intentionally did not add the negotiation to the test code to keep
the test as simple as possible. However, I wrote this in the
gitattributes docs [2]:

  After the version negotiation Git sends a list of all capabilities that
  it supports and a flush packet. Git expects to read a list of desired
  capabilities, which must be a subset of the supported capabilities list,
  and a flush packet as response:

Maybe we should revive "Documentation/technical/api-sub-process.txt" [3]
after all to explain these kind of things?

- Lars


[1] https://public-inbox.org/git/xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com/
[2] https://github.com/git/git/blob/b3622a4ee94e4916cd05e6d96e41eeb36b941182/Documentation/gitattributes.txt#L408-L411
[3] https://public-inbox.org/git/20170807102136.30b23023@twelve2.svl.corp.google.com/

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

[Jonathan Tan]

On Thu, 17 Aug 2017 23:34:33 +0200
Lars Schneider <larsxschneider@gmail.com> wrote:

> 
> > On 17 Aug 2017, at 23:01, Junio C Hamano <gitster@pobox.com> wrote:
> > 
> > Christian Couder <christian.couder@gmail.com> writes:
> > 
> >> ... 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.
> > 
> > Oh, absolutely.  If you know there is such a breakage in our test
> > script, please do fix it.
> > 
> > Thanks.
> 
> Junio's reasoning [1] is spot on from my point of view.

Agreed.

> 
> I intentionally did not add the negotiation to the test code to keep
> the test as simple as possible.

I think this is the correct approach - the test was testing Git's
behavior, not the filter's behavior. (Although, if someone wanted to add
a test for a misbehaving filter, that would be great, although such a
test would have hardcoded output from the filter anyway.)

> However, I wrote this in the
> gitattributes docs [2]:
> 
>   After the version negotiation Git sends a list of all capabilities that
>   it supports and a flush packet. Git expects to read a list of desired
>   capabilities, which must be a subset of the supported capabilities list,
>   and a flush packet as response:
> 
> Maybe we should revive "Documentation/technical/api-sub-process.txt" [3]
> after all to explain these kind of things?

As for reviving that specific file, I saw you wrote a similar comment
but I didn't reply to it - sorry.  The commit in question is 7e2e1bb
("Documentation: migrate sub-process docs to header", 2017-07-26), and
in that commit, everything in api-sub-process.txt was migrated. I think
it's better for such documentation to be in a .h file, rather than a
.txt file.

As for describing the Long Running Process Protocol in a
Documentation/.../txt file, my plan was to create a new file
specifically for that, as you can see in an e-mail I sent [4]. I'm OK
with putting it in a different place, but it probably shouldn't be named
"api", because those are for internal Git APIs.

[4] https://public-inbox.org/git/eadce97b6a1e80345a2621e71ce187e9e6bc05bf.1501532294.git.jonathantanmy@google.com/

> [1] https://public-inbox.org/git/xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com/
> [2] https://github.com/git/git/blob/b3622a4ee94e4916cd05e6d96e41eeb36b941182/Documentation/gitattributes.txt#L408-L411
> [3] https://public-inbox.org/git/20170807102136.30b23023@twelve2.svl.corp.google.com/