{"thread":{"id":"46593","subject":"[PATCH] sub-process: print the cmd when a capability is unsupported","startedAt":"2017-08-15T17:36:35Z","lastAt":"2017-08-17T21:49:47Z","messageCount":16,"participants":["Christian Couder","Jonathan Tan","Lars Schneider","Ben Peart","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"326418","messageId":"20170815173611.2267-1-chriscool@tuxfamily.org","threadId":"46593","inReplyTo":null,"subject":"[PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-08-15T17:36:11Z","receivedAt":"2017-08-15T17:36:35Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"In handshake_capabilities() we use warning() when a capability\nis not supported, so the exit code of the function is 0 and no\nfurther error is shown. This is a problem because the warning\nmessage doesn't tell us which subprocess cmd failed.\n\nOn the contrary if we cannot write a packet from this function,\nwe use error() and then subprocess_start() outputs:\n\n initialization for subprocess '' failed\n\nso we can know which subprocess cmd failed.\n\nLet's improve the warning() message, so that we can know which\nsubprocess cmd failed.\n\nSigned-off-by: Christian Couder \n---\n sub-process.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/sub-process.c b/sub-process.c\nindex 6edb97c1c6..6b133f8dce 100644\n--- a/sub-process.c\n+++ b/sub-process.c\n@@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,\n \n static int handshake_capabilities(struct child_process *process,\n \t\t\t\t struct subprocess_capability *capabilities,\n-\t\t\t\t unsigned int *supported_capabilities)\n+\t\t\t\t unsigned int *supported_capabilities,\n+\t\t\t\t const char *cmd)\n {\n \tint i;\n \tchar *line;\n@@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,\n \t\t\tif (supported_capabilities)\n \t\t\t\t*supported_capabilities |= capabilities[i].flag;\n \t\t} else {\n-\t\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n-\t\t\t\tp);\n+\t\t\twarning(\"subprocess '%s' requested unsupported capability '%s'\",\n+\t\t\t\tcmd, p);\n \t\t}\n \t}\n \n@@ -206,8 +207,10 @@ int subprocess_handshake(struct subprocess_entry *entry,\n \n \tretval = handshake_version(process, welcome_prefix, versions,\n \t\t\t\t chosen_version) ||\n-\t\t handshake_capabilities(process, capabilities,\n-\t\t\t\t\tsupported_capabilities);\n+\t\t handshake_capabilities(process,\n+\t\t\t\t\tcapabilities,\n+\t\t\t\t\tsupported_capabilities,\n+\t\t\t\t\tentry->cmd);\n \n \tsigchain_pop(SIGPIPE);\n \treturn retval;\n-- \n2.14.1.187.gd6d46550f4\n\n"},{"id":"326426","messageId":"20170815111725.5d009b66@twelve2.svl.corp.google.com","threadId":"46593","inReplyTo":"20170815173611.2267-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-08-15T18:17:25Z","receivedAt":"2017-08-15T18:17:38Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 15 Aug 2017 19:36:11 +0200\nChristian Couder wrote:\n\n> In handshake_capabilities() we use warning() when a capability\n> is not supported, so the exit code of the function is 0 and no\n> further error is shown. This is a problem because the warning\n> message doesn't tell us which subprocess cmd failed.\n> \n> On the contrary if we cannot write a packet from this function,\n> we use error() and then subprocess_start() outputs:\n> \n> initialization for subprocess '' failed\n> \n> so we can know which subprocess cmd failed.\n> \n> Let's improve the warning() message, so that we can know which\n> subprocess cmd failed.\n> \n> Signed-off-by: Christian Couder \n\nThis looks reasonable to me.\n\nI am still wondering if protocol errors should be fatal, but that is\nunrelated to this patch.\n"},{"id":"326439","messageId":"836F754B-C78C-4D19-8365-875D96B3F115@gmail.com","threadId":"46593","inReplyTo":"20170815173611.2267-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-15T19:00:53Z","receivedAt":"2017-08-15T19:01:02Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 15 Aug 2017, at 19:36, Christian Couder wrote:\n> \n> In handshake_capabilities() we use warning() when a capability\n> is not supported, so the exit code of the function is 0 and no\n> further error is shown. This is a problem because the warning\n> message doesn't tell us which subprocess cmd failed.\n> \n> On the contrary if we cannot write a packet from this function,\n> we use error() and then subprocess_start() outputs:\n> \n> initialization for subprocess '' failed\n> \n> so we can know which subprocess cmd failed.\n> \n> Let's improve the warning() message, so that we can know which\n> subprocess cmd failed.\n> \n> Signed-off-by: Christian Couder \n> ---\n> sub-process.c | 13 ++++++++-----\n> 1 file changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/sub-process.c b/sub-process.c\n> index 6edb97c1c6..6b133f8dce 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,\n> \n> static int handshake_capabilities(struct child_process *process,\n> \t\t\t\t struct subprocess_capability *capabilities,\n> -\t\t\t\t unsigned int *supported_capabilities)\n> +\t\t\t\t unsigned int *supported_capabilities,\n> +\t\t\t\t const char *cmd)\n> {\n> \tint i;\n> \tchar *line;\n> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,\n> \t\t\tif (supported_capabilities)\n> \t\t\t\t*supported_capabilities |= capabilities[i].flag;\n> \t\t} else {\n> -\t\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n> -\t\t\t\tp);\n> +\t\t\twarning(\"subprocess '%s' requested unsupported capability '%s'\",\n> +\t\t\t\tcmd, p);\n\nWouldn't it be possible to use \"process->argv[0]\"? \nShouldn't that be the same as \"cmd\"?\n\n- Lars"},{"id":"326440","messageId":"b9a458ca-6e4a-1ef2-79e1-f2a3e6d0a665@gmail.com","threadId":"46593","inReplyTo":"20170815173611.2267-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2017-08-15T19:01:15Z","receivedAt":"2017-08-15T19:01:24Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 8/15/2017 1:36 PM, Christian Couder wrote:\n> In handshake_capabilities() we use warning() when a capability\n> is not supported, so the exit code of the function is 0 and no\n> further error is shown. This is a problem because the warning\n> message doesn't tell us which subprocess cmd failed.\n> \n> On the contrary if we cannot write a packet from this function,\n> we use error() and then subprocess_start() outputs:\n> \n> initialization for subprocess '' failed\n> \n> so we can know which subprocess cmd failed.\n> \n> Let's improve the warning() message, so that we can know which\n> subprocess cmd failed.\n> \n> Signed-off-by: Christian Couder \n> ---\n> sub-process.c | 13 ++++++++-----\n> 1 file changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/sub-process.c b/sub-process.c\n> index 6edb97c1c6..6b133f8dce 100644\n> --- a/sub-process.c\n> +++ b/sub-process.c\n> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,\n> \n> static int handshake_capabilities(struct child_process *process,\n> \t\t\t\t struct subprocess_capability *capabilities,\n> -\t\t\t\t unsigned int *supported_capabilities)\n> +\t\t\t\t unsigned int *supported_capabilities,\n> +\t\t\t\t const char *cmd)\n> {\n> \tint i;\n> \tchar *line;\n> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,\n> \t\t\tif (supported_capabilities)\n> \t\t\t\t*supported_capabilities |= capabilities[i].flag;\n> \t\t} else {\n> -\t\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n> -\t\t\t\tp);\n> +\t\t\twarning(\"subprocess '%s' requested unsupported capability '%s'\",\n> +\t\t\t\tcmd, p);\n> \t\t}\n> \t}\n> \n> @@ -206,8 +207,10 @@ int subprocess_handshake(struct subprocess_entry *entry,\n> \n> \tretval = handshake_version(process, welcome_prefix, versions,\n> \t\t\t\t chosen_version) ||\n> -\t\t handshake_capabilities(process, capabilities,\n> -\t\t\t\t\tsupported_capabilities);\n> +\t\t handshake_capabilities(process,\n> +\t\t\t\t\tcapabilities,\n> +\t\t\t\t\tsupported_capabilities,\n> +\t\t\t\t\tentry->cmd);\n> \n> \tsigchain_pop(SIGPIPE);\n> \treturn retval;\n> \n\nLooks good to me.\n\n"},{"id":"326446","messageId":"xmqq378sr5sw.fsf@gitster.mtv.corp.google.com","threadId":"46593","inReplyTo":"836F754B-C78C-4D19-8365-875D96B3F115@gmail.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-15T19:26:23Z","receivedAt":"2017-08-15T19:26:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider writes:\n\n>> On 15 Aug 2017, at 19:36, Christian Couder wrote:\n>> \n>> In handshake_capabilities() we use warning() when a capability\n>> is not supported, so the exit code of the function is 0 and no\n>> further error is shown. This is a problem because the warning\n>> message doesn't tell us which subprocess cmd failed.\n>> \n>> On the contrary if we cannot write a packet from this function,\n>> we use error() and then subprocess_start() outputs:\n>> \n>> initialization for subprocess '' failed\n>> \n>> so we can know which subprocess cmd failed.\n>> \n>> Let's improve the warning() message, so that we can know which\n>> subprocess cmd failed.\n>> \n>> Signed-off-by: Christian Couder \n>> ---\n>> sub-process.c | 13 ++++++++-----\n>> 1 file changed, 8 insertions(+), 5 deletions(-)\n>> \n>> diff --git a/sub-process.c b/sub-process.c\n>> index 6edb97c1c6..6b133f8dce 100644\n>> --- a/sub-process.c\n>> +++ b/sub-process.c\n>> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,\n>> \n>> static int handshake_capabilities(struct child_process *process,\n>> \t\t\t\t struct subprocess_capability *capabilities,\n>> -\t\t\t\t unsigned int *supported_capabilities)\n>> +\t\t\t\t unsigned int *supported_capabilities,\n>> +\t\t\t\t const char *cmd)\n>> {\n>> \tint i;\n>> \tchar *line;\n>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,\n>> \t\t\tif (supported_capabilities)\n>> \t\t\t\t*supported_capabilities |= capabilities[i].flag;\n>> \t\t} else {\n>> -\t\t\twarning(\"external filter requested unsupported filter capability '%s'\",\n>> -\t\t\t\tp);\n>> +\t\t\twarning(\"subprocess '%s' requested unsupported capability '%s'\",\n>> +\t\t\t\tcmd, p);\n>\n> Wouldn't it be possible to use \"process->argv[0]\"? \n> Shouldn't that be the same as \"cmd\"?\n\nIt is good to see many people are in agreement and in favor of\ngiving more information. It is even better to see somebody noticing\na room for improvement ;-)\n\nI also wonder if this should be left as a silent warning that the\ncaller cannot notice as in the corrent code. Dying here might not\nbe desirable for some callers, but even if we don't die here, we may\nwant to give the caller a chance to react to the protocol error.\n"},{"id":"326447","messageId":"CAP8UFD2KBEVwZE8obaDxf+0AyU4zH0+5bWJzzHazFywgWT9QjQ@mail.gmail.com","threadId":"46593","inReplyTo":"836F754B-C78C-4D19-8365-875D96B3F115@gmail.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-08-15T19:29:42Z","receivedAt":"2017-08-15T19:29:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider\n wrote:\n>\n>> On 15 Aug 2017, at 19:36, Christian Couder wrote:\n>>\n>> In handshake_capabilities() we use warning() when a capability\n>> is not supported, so the exit code of the function is 0 and no\n>> further error is shown. This is a problem because the warning\n>> message doesn't tell us which subprocess cmd failed.\n>>\n>> On the contrary if we cannot write a packet from this function,\n>> we use error() and then subprocess_start() outputs:\n>>\n>> initialization for subprocess '' failed\n>>\n>> so we can know which subprocess cmd failed.\n>>\n>> Let's improve the warning() message, so that we can know which\n>> subprocess cmd failed.\n>>\n>> Signed-off-by: Christian Couder \n>> ---\n>> sub-process.c | 13 ++++++++-----\n>> 1 file changed, 8 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/sub-process.c b/sub-process.c\n>> index 6edb97c1c6..6b133f8dce 100644\n>> --- a/sub-process.c\n>> +++ b/sub-process.c\n>> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,\n>>\n>> static int handshake_capabilities(struct child_process *process,\n>> struct subprocess_capability *capabilities,\n>> - unsigned int *supported_capabilities)\n>> + unsigned int *supported_capabilities,\n>> + const char *cmd)\n>> {\n>> int i;\n>> char *line;\n>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,\n>> if (supported_capabilities)\n>> *supported_capabilities |= capabilities[i].flag;\n>> } else {\n>> - warning(\"external filter requested unsupported filter capability '%s'\",\n>> - p);\n>> + warning(\"subprocess '%s' requested unsupported capability '%s'\",\n>> + cmd, p);\n>\n> Wouldn't it be possible to use \"process->argv[0]\"?\n> Shouldn't that be the same as \"cmd\"?\n\nWell in sub-process.h there is:\n\n/* Members should not be accessed directly. */\nstruct subprocess_entry {\n struct hashmap_entry ent; /* must be the first member! */\n const char *cmd;\n struct child_process process;\n};\n\nso if cmd is always the same as process->argv[0], maybe there is no\nneed for the cmd member in the first place?\n"},{"id":"326448","messageId":"CAP8UFD18KbTkQZqEXK6AsPDABda-diaMLLhyFvACeVQLb+qwrA@mail.gmail.com","threadId":"46593","inReplyTo":"CAP8UFD2KBEVwZE8obaDxf+0AyU4zH0+5bWJzzHazFywgWT9QjQ@mail.gmail.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-08-15T19:32:27Z","receivedAt":"2017-08-15T19:32:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Aug 15, 2017 at 9:29 PM, Christian Couder\n wrote:\n> On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider\n> wrote:\n>>\n>>> On 15 Aug 2017, at 19:36, Christian Couder wrote:\n\n>>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,\n>>> if (supported_capabilities)\n>>> *supported_capabilities |= capabilities[i].flag;\n>>> } else {\n>>> - warning(\"external filter requested unsupported filter capability '%s'\",\n>>> - p);\n>>> + warning(\"subprocess '%s' requested unsupported capability '%s'\",\n>>> + cmd, p);\n>>\n>> Wouldn't it be possible to use \"process->argv[0]\"?\n>> Shouldn't that be the same as \"cmd\"?\n>\n> Well in sub-process.h there is:\n>\n> /* Members should not be accessed directly. */\n> struct subprocess_entry {\n> struct hashmap_entry ent; /* must be the first member! */\n> const char *cmd;\n> struct child_process process;\n> };\n>\n> so if cmd is always the same as process->argv[0], maybe there is no\n> need for the cmd member in the first place?\n\nIn case it is not clear, what I mean is that if we consider that they\nshould always be the same, it could be considered a different patch\naltogether to just remove the cmd member of this struct.\n"},{"id":"326449","messageId":"FC865401-2FE6-45ED-BBA5-524C3E29477E@gmail.com","threadId":"46593","inReplyTo":"CAP8UFD2KBEVwZE8obaDxf+0AyU4zH0+5bWJzzHazFywgWT9QjQ@mail.gmail.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-15T19:35:31Z","receivedAt":"2017-08-15T19:35:39Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 15 Aug 2017, at 21:29, Christian Couder wrote:\n> \n> On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider\n> wrote:\n>> \n>>> On 15 Aug 2017, at 19:36, Christian Couder wrote:\n>>> \n>>> In handshake_capabilities() we use warning() when a capability\n>>> is not supported, so the exit code of the function is 0 and no\n>>> further error is shown. This is a problem because the warning\n>>> message doesn't tell us which subprocess cmd failed.\n>>> \n>>> On the contrary if we cannot write a packet from this function,\n>>> we use error() and then subprocess_start() outputs:\n>>> \n>>> initialization for subprocess '' failed\n>>> \n>>> so we can know which subprocess cmd failed.\n>>> \n>>> Let's improve the warning() message, so that we can know which\n>>> subprocess cmd failed.\n>>> \n>>> Signed-off-by: Christian Couder \n>>> ---\n>>> sub-process.c | 13 ++++++++-----\n>>> 1 file changed, 8 insertions(+), 5 deletions(-)\n>>> \n>>> diff --git a/sub-process.c b/sub-process.c\n>>> index 6edb97c1c6..6b133f8dce 100644\n>>> --- a/sub-process.c\n>>> +++ b/sub-process.c\n>>> @@ -158,7 +158,8 @@ static int handshake_version(struct child_process *process,\n>>> \n>>> static int handshake_capabilities(struct child_process *process,\n>>> struct subprocess_capability *capabilities,\n>>> - unsigned int *supported_capabilities)\n>>> + unsigned int *supported_capabilities,\n>>> + const char *cmd)\n>>> {\n>>> int i;\n>>> char *line;\n>>> @@ -184,8 +185,8 @@ static int handshake_capabilities(struct child_process *process,\n>>> if (supported_capabilities)\n>>> *supported_capabilities |= capabilities[i].flag;\n>>> } else {\n>>> - warning(\"external filter requested unsupported filter capability '%s'\",\n>>> - p);\n>>> + warning(\"subprocess '%s' requested unsupported capability '%s'\",\n>>> + cmd, p);\n>> \n>> Wouldn't it be possible to use \"process->argv[0]\"?\n>> Shouldn't that be the same as \"cmd\"?\n> \n> Well in sub-process.h there is:\n> \n> /* Members should not be accessed directly. */\n> struct subprocess_entry {\n> struct hashmap_entry ent; /* must be the first member! */\n> const char *cmd;\n> struct child_process process;\n> };\n> \n> so if cmd is always the same as process->argv[0], maybe there is no\n> need for the cmd member in the first place?\n\nThe struct is a hash map entry. `cmd` is the key for a `process`.\nTherefore, I think this is still necessary.\n\nDoes this make sense?\n\n- Lars\n\n"},{"id":"326454","messageId":"CAP8UFD0OUKUu0bqf6BxHBf+51CgFjjmtOk06J_D-=8maa41G7w@mail.gmail.com","threadId":"46593","inReplyTo":"FC865401-2FE6-45ED-BBA5-524C3E29477E@gmail.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-08-15T20:30:20Z","receivedAt":"2017-08-15T20:30:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Aug 15, 2017 at 9:35 PM, Lars Schneider\n wrote:\n>\n>> On 15 Aug 2017, at 21:29, Christian Couder wrote:\n>>\n>> On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider\n>> wrote:\n>>>\n>>> Wouldn't it be possible to use \"process->argv[0]\"?\n>>> Shouldn't that be the same as \"cmd\"?\n>>\n>> Well in sub-process.h there is:\n>>\n>> /* Members should not be accessed directly. */\n>> struct subprocess_entry {\n>> struct hashmap_entry ent; /* must be the first member! */\n>> const char *cmd;\n>> struct child_process process;\n>> };\n>>\n>> so if cmd is always the same as process->argv[0], maybe there is no\n>> need for the cmd member in the first place?\n>\n> The struct is a hash map entry. `cmd` is the key for a `process`.\n> Therefore, I think this is still necessary.\n>\n> Does this make sense?\n\nNot sure it makes sense. A quick try to remove the cmd member, failed\nt0021 with:\n\nInitialized empty Git repository in /home/christian/git/git/t/trash\ndirectory.t0021-conversion/repo/.git/\n[master (root-commit) 56d459b] test commit 1\n Author: A U Thor \n 1 file changed, 1 insertion(+)\n create mode 100644 .gitattributes\n--- expected.log.tmp 2017-08-15 20:27:51.658818467 +0000\n+++ debug.log.tmp 2017-08-15 20:27:51.662818526 +0000\n@@ -2,6 +2,6 @@\n x IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]\n x IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]\n x IN: clean testsubdir/test3 'sq',$x=.r 49 [OK] -- OUT: 49 . [OK]\n- 1 START\n- 1 STOP\n- 1 init handshake complete\n+ 4 START\n+ 4 STOP\n+ 4 init handshake complete\nnot ok 15 - required process filter should filter data\n\nand it is not clear to me why, so I guess you are right and I will\njust use process->argv[0].\n"},{"id":"326474","messageId":"20170816002257.GG13924@aiede.mtv.corp.google.com","threadId":"46593","inReplyTo":"20170815111725.5d009b66@twelve2.svl.corp.google.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-08-16T00:22:57Z","receivedAt":"2017-08-16T00:23:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n> Christian Couder wrote:\n\n>> In handshake_capabilities() we use warning() when a capability\n>> is not supported, so the exit code of the function is 0 and no\n>> further error is shown. This is a problem because the warning\n>> message doesn't tell us which subprocess cmd failed.\n[...]\n>> Let's improve the warning() message, so that we can know which\n>> subprocess cmd failed.\n>>\n>> Signed-off-by: Christian Couder \n>\n> This looks reasonable to me.\n>\n> I am still wondering if protocol errors should be fatal,\n\nYes, please.\n\nThanks,\nJonathan\n"},{"id":"326503","messageId":"CAP8UFD2jLdR7HTi-T6J_xWvxKyoQf_6pUTX1CWsd9v3TXh9FXw@mail.gmail.com","threadId":"46593","inReplyTo":"20170816002257.GG13924@aiede.mtv.corp.google.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-08-16T12:37:01Z","receivedAt":"2017-08-16T12:37:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Aug 16, 2017 at 2:22 AM, Jonathan Nieder wrote:\n> Jonathan Tan wrote:\n>> Christian Couder wrote:\n>\n>>> In handshake_capabilities() we use warning() when a capability\n>>> is not supported, so the exit code of the function is 0 and no\n>>> further error is shown. This is a problem because the warning\n>>> message doesn't tell us which subprocess cmd failed.\n> [...]\n>>> Let's improve the warning() message, so that we can know which\n>>> subprocess cmd failed.\n>>>\n>>> Signed-off-by: Christian Couder \n>>\n>> This looks reasonable to me.\n>>\n>> I am still wondering if protocol errors should be fatal,\n>\n> Yes, please.\n\nUnfortunately I think it would prevent new filters or new\nsub-processes to work with older versions of Git.\n\nFor example if filters are upgraded company wide to support the new\n\"delay\" capability, that would force everyone using the filters to\nupgrade Git.\n"},{"id":"326510","messageId":"xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com","threadId":"46593","inReplyTo":"CAP8UFD2jLdR7HTi-T6J_xWvxKyoQf_6pUTX1CWsd9v3TXh9FXw@mail.gmail.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-16T15:58:12Z","receivedAt":"2017-08-16T15:58:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n>>> I am still wondering if protocol errors should be fatal,\n>>\n>> Yes, please.\n>\n> Unfortunately I think it would prevent new filters or new\n> sub-processes to work with older versions of Git.\n>\n> For example if filters are upgraded company wide to support the new\n> \"delay\" capability, that would force everyone using the filters to\n> upgrade Git.\n\nI must say that your filter is broken in that case, and it is much\nmore prudent to die than continuing. Why is that upgraded filter\nasking for \"delay\" to an older Git that does not yet know it in the\nfirst place?\n\nI just re-read the subprocess_handshake() codepath, and here is my\nunderstand. The handshake_capabilities() function first advertises\nthe set of capabilities it supports, so that the other side can pick\nand choose which ones to use and ask us to enable in its response.\n\nThe code under discussion in this thread comes after that, where we\nread the response that tells us what choice the other side made. If\nwe saw something that we never advertised, that indicates one of two\nthings. The other side, i.e. the \"upgraded\" filter, is not paying\nattention of the capabilities advertisement, and asking something\nits correct operation relies on, but we are not capable of giving\nthat unknown feature and operate without it, so after that point the\nexchange of data is a garbage-in-garbage-out. Or the other side\nwanted to ask for one of the capabilities we advertised, but the\ncode has typo and their wish to enable a capability that its correct\noperation relies on is not understood on this end. The result is\nthe same garbage-in-garbage-out.\n\nSo \"program X asked capability C, which we cannot handle\" is a good\ndiagnosis to give and it is a good change to spell out the name of\nthe offending program that needs to be fixed, but at the same time,\nI do not think it is safe to continue without the other side knowing\nthat one of the capabilities they need to correctly operate cannot\nbe turned on.\n"},{"id":"326577","messageId":"CAP8UFD1H4Pb5e2_pioQ5neROc+64e55RfvRhiyz5Df5AwJg-FQ@mail.gmail.com","threadId":"46593","inReplyTo":"xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-08-17T05:34:49Z","receivedAt":"2017-08-17T05:34:56Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Aug 16, 2017 at 5:58 PM, Junio C Hamano wrote:\n> Christian Couder writes:\n>\n>>>> I am still wondering if protocol errors should be fatal,\n>>>\n>>> Yes, please.\n>>\n>> Unfortunately I think it would prevent new filters or new\n>> sub-processes to work with older versions of Git.\n>>\n>> For example if filters are upgraded company wide to support the new\n>> \"delay\" capability, that would force everyone using the filters to\n>> upgrade Git.\n>\n> I must say that your filter is broken in that case,\n\nPerhaps it is just sloppily written.\n\n> and it is much\n> more prudent to die than continuing. Why is that upgraded filter\n> asking for \"delay\" to an older Git that does not yet know it in the\n> first place?\n\nMaybe because in our tests (like in t/t0021/rot13-filter.pl) the\nfilter just outputs all its capabilities, so the filter writer thought\nit should be ok to do the same.\n\n> I just re-read the subprocess_handshake() codepath, and here is my\n> understand. The handshake_capabilities() function first advertises\n> the set of capabilities it supports, so that the other side can pick\n> and choose which ones to use and ask us to enable in its response.\n\nYeah, that sounds like the right thing the filter should do. Though I\nthink that if we really want the filters/subprocesses to always do\nthis, we have some work on our plate...\n\n> The code under discussion in this thread comes after that, where we\n> read the response that tells us what choice the other side made. If\n> we saw something that we never advertised, that indicates one of two\n> things. The other side, i.e. the \"upgraded\" filter, is not paying\n> attention of the capabilities advertisement, and asking something\n> its correct operation relies on, but we are not capable of giving\n> that unknown feature and operate without it, so after that point the\n> exchange of data is a garbage-in-garbage-out.\n\nMaybe it is not paying attention and just following the bad example of\ngiving all the capabilities it supports even if it can work if some of\nthem are not supported.\n\nIn this case if we error out, we prevent everything to work even if it\ncould work if we just also \"ignored\" (though printing a warning is not\nexactly ignoring and is the right and the least thing to do) what the\nfilter told us.\n\nAnyway I don't really mind being very strict and just erroring out in\nthis case, but I think we should then emphasize more in our test\nscripts (maybe by giving a good example) and perhaps also in the doc\nthat the filters/sub-processes should really pay attention and not\noutput any capability that are not supported by Git.\n"},{"id":"326641","messageId":"xmqqbmndgb8w.fsf@gitster.mtv.corp.google.com","threadId":"46593","inReplyTo":"CAP8UFD1H4Pb5e2_pioQ5neROc+64e55RfvRhiyz5Df5AwJg-FQ@mail.gmail.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-17T21:01:03Z","receivedAt":"2017-08-17T21:01:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> ... but I think we should then emphasize more in our test\n> scripts (maybe by giving a good example) and perhaps also in the doc\n> that the filters/sub-processes should really pay attention and not\n> output any capability that are not supported by Git.\n\nOh, absolutely. If you know there is such a breakage in our test\nscript, please do fix it.\n\nThanks.\n"},{"id":"326646","messageId":"25BB25E4-DD3F-46A5-8CD5-D784CBAB6F52@gmail.com","threadId":"46593","inReplyTo":"xmqqbmndgb8w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-08-17T21:34:33Z","receivedAt":"2017-08-17T21:34:43Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 17 Aug 2017, at 23:01, Junio C Hamano wrote:\n> \n> Christian Couder writes:\n> \n>> ... but I think we should then emphasize more in our test\n>> scripts (maybe by giving a good example) and perhaps also in the doc\n>> that the filters/sub-processes should really pay attention and not\n>> output any capability that are not supported by Git.\n> \n> Oh, absolutely. If you know there is such a breakage in our test\n> script, please do fix it.\n> \n> Thanks.\n\nJunio's reasoning [1] is spot on from my point of view.\n\nI intentionally did not add the negotiation to the test code to keep\nthe test as simple as possible. However, I wrote this in the\ngitattributes docs [2]:\n\n After the version negotiation Git sends a list of all capabilities that\n it supports and a flush packet. Git expects to read a list of desired\n capabilities, which must be a subset of the supported capabilities list,\n and a flush packet as response:\n\nMaybe we should revive \"Documentation/technical/api-sub-process.txt\" [3]\nafter all to explain these kind of things?\n\n- Lars\n\n\n[1] https://public-inbox.org/git/xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com/\n[2] https://github.com/git/git/blob/b3622a4ee94e4916cd05e6d96e41eeb36b941182/Documentation/gitattributes.txt#L408-L411\n[3] https://public-inbox.org/git/20170807102136.30b23023@twelve2.svl.corp.google.com/"},{"id":"326655","messageId":"20170817144939.72fcd998@twelve2.svl.corp.google.com","threadId":"46593","inReplyTo":"25BB25E4-DD3F-46A5-8CD5-D784CBAB6F52@gmail.com","subject":"Re: [PATCH] sub-process: print the cmd when a capability is unsupported","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-08-17T21:49:39Z","receivedAt":"2017-08-17T21:49:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Thu, 17 Aug 2017 23:34:33 +0200\nLars Schneider wrote:\n\n> \n> > On 17 Aug 2017, at 23:01, Junio C Hamano wrote:\n> > \n> > Christian Couder writes:\n> > \n> >> ... but I think we should then emphasize more in our test\n> >> scripts (maybe by giving a good example) and perhaps also in the doc\n> >> that the filters/sub-processes should really pay attention and not\n> >> output any capability that are not supported by Git.\n> > \n> > Oh, absolutely. If you know there is such a breakage in our test\n> > script, please do fix it.\n> > \n> > Thanks.\n> \n> Junio's reasoning [1] is spot on from my point of view.\n\nAgreed.\n\n> \n> I intentionally did not add the negotiation to the test code to keep\n> the test as simple as possible.\n\nI think this is the correct approach - the test was testing Git's\nbehavior, not the filter's behavior. (Although, if someone wanted to add\na test for a misbehaving filter, that would be great, although such a\ntest would have hardcoded output from the filter anyway.)\n\n> However, I wrote this in the\n> gitattributes docs [2]:\n> \n> After the version negotiation Git sends a list of all capabilities that\n> it supports and a flush packet. Git expects to read a list of desired\n> capabilities, which must be a subset of the supported capabilities list,\n> and a flush packet as response:\n> \n> Maybe we should revive \"Documentation/technical/api-sub-process.txt\" [3]\n> after all to explain these kind of things?\n\nAs for reviving that specific file, I saw you wrote a similar comment\nbut I didn't reply to it - sorry. The commit in question is 7e2e1bb\n(\"Documentation: migrate sub-process docs to header\", 2017-07-26), and\nin that commit, everything in api-sub-process.txt was migrated. I think\nit's better for such documentation to be in a .h file, rather than a\n.txt file.\n\nAs for describing the Long Running Process Protocol in a\nDocumentation/.../txt file, my plan was to create a new file\nspecifically for that, as you can see in an e-mail I sent [4]. I'm OK\nwith putting it in a different place, but it probably shouldn't be named\n\"api\", because those are for internal Git APIs.\n\n[4] https://public-inbox.org/git/eadce97b6a1e80345a2621e71ce187e9e6bc05bf.1501532294.git.jonathantanmy@google.com/\n\n> [1] https://public-inbox.org/git/xmqq8tijpkrv.fsf@gitster.mtv.corp.google.com/\n> [2] https://github.com/git/git/blob/b3622a4ee94e4916cd05e6d96e41eeb36b941182/Documentation/gitattributes.txt#L408-L411\n> [3] https://public-inbox.org/git/20170807102136.30b23023@twelve2.svl.corp.google.com/\n"}]}