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

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

On Tue, 15 Aug 2017 19:36:11 +0200
Christian Couder [off-list ref] 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 <redacted>

This looks reasonable to me.

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

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

- Lars

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

Looks good to me.

On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider
[off-list ref] wrote:

quoted

On 15 Aug 2017, at 19:36, Christian Couder [off-list ref] 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 <redacted>
---
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?

On Tue, Aug 15, 2017 at 9:29 PM, Christian Couder
[off-list ref] wrote:

On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider
[off-list ref] wrote:

quoted

quoted

On 15 Aug 2017, at 19:36, Christian Couder [off-list ref] wrote:

quoted

quoted

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

On 15 Aug 2017, at 21:29, Christian Couder [off-list ref] wrote:

On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider
[off-list ref] wrote:

quoted

quoted

On 15 Aug 2017, at 19:36, Christian Couder [off-list ref] 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 <redacted>
---
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

On Tue, Aug 15, 2017 at 9:35 PM, Lars Schneider
[off-list ref] wrote:

quoted

On 15 Aug 2017, at 21:29, Christian Couder [off-list ref] wrote:

On Tue, Aug 15, 2017 at 9:00 PM, Lars Schneider
[off-list ref] wrote:

quoted

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 [off-list ref]
 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].

Jonathan Tan wrote:

Christian Couder [off-list ref] wrote:

quoted

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.

[...]

quoted

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

Signed-off-by: Christian Couder <redacted>

This looks reasonable to me.

I am still wondering if protocol errors should be fatal,

Yes, please.

Thanks,
Jonathan

On Wed, Aug 16, 2017 at 2:22 AM, Jonathan Nieder [off-list ref] wrote:

Jonathan Tan wrote:

quoted

Christian Couder [off-list ref] wrote:

quoted

quoted

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.

[...]

quoted

quoted

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

Signed-off-by: Christian Couder <redacted>

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.