threads / patch / 46593

patchsub-process: print the cmd when a capability is unsupported

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

## tl;dr

16 messages between Aug 15, 2017 and Aug 17, 2017. Diffs are folded; open one to read it.

replies: 15people: 6as markdown or json

Christian Couder· Aug 15, 2017, 17:36 UTC · lore

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(-)

Show changes to sub-process.c +8 −5

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

Jonathan Tan· Aug 15, 2017, 18:17 UTC · re: Christian Couder · lore

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

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

Show 16 quoted lines

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

Jonathan Nieder· Aug 16, 2017, 00:22 UTC · re: Jonathan Tan · lore

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

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.

[...]

Show 8 quoted lines

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

Christian Couder· Aug 16, 2017, 12:37 UTC · re: Jonathan Nieder · lore

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

On Wed, Aug 16, 2017 at 2:22 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 18 quoted lines

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

Junio C Hamano· Aug 16, 2017, 15:58 UTC · re: Christian Couder · lore

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

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.

Christian Couder· Aug 17, 2017, 05:34 UTC · re: Junio C Hamano · lore

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

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.

Junio C Hamano· Aug 17, 2017, 21:01 UTC · re: Christian Couder · lore

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

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.

Lars Schneider· Aug 17, 2017, 21:34 UTC · re: Junio C Hamano · lore

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

Show 13 quoted lines

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

Jonathan Tan· Aug 17, 2017, 21:49 UTC · re: Lars Schneider · lore

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

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

Show 16 quoted lines

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

Show 10 quoted lines

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

Lars Schneider· Aug 15, 2017, 19:00 UTC · re: Christian Couder · lore

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

Show 44 quoted lines

> 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

Junio C Hamano· Aug 15, 2017, 19:26 UTC · re: Lars Schneider · lore

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

Lars Schneider <larsxschneider@gmail.com> writes:

Show 47 quoted lines

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

Christian Couder· Aug 15, 2017, 19:29 UTC · re: Lars Schneider · lore

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

On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 48 quoted lines

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

Christian Couder· Aug 15, 2017, 19:32 UTC · re: Christian Couder · lore

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

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:

Show 23 quoted lines

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

Lars Schneider· Aug 15, 2017, 19:35 UTC · re: Christian Couder · lore

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

Show 64 quoted lines

> 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

Christian Couder· Aug 15, 2017, 20:30 UTC · re: Lars Schneider · lore

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

On Tue, Aug 15, 2017 at 9:35 PM, Lars Schneider <larsxschneider@gmail.com> wrote:

Show 25 quoted lines

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

Show changes to diff +3 −3

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

Ben Peart· Aug 15, 2017, 19:01 UTC · re: Christian Couder · lore

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

On 8/15/2017 1:36 PM, Christian Couder wrote:

Show 59 quoted lines

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