Re: [RFC/PATCH] Documentation/remote-helpers: explain capabilities first

Hi again,

Sorry for the spam.  Just ran into a quick puzzle:

Jonathan Nieder wrote:

+'push'::
+	Can discover remote refs and push local commits and the
+	history leading up to them to new or existing remote refs.
++
+Supported commands: 'list for-push', 'push'.

[...]

+'fetch'::
+	Can discover remote refs and transfer objects reachable from
+	them to the local object store.
++
+Supported commands: 'list', 'fetch'.
+
+'import'::
+	Can discover remote refs and output objects reachable from
+	them as a stream in fast-import format.
++
+Supported commands: 'list', 'import'.

In the actual code:

 * "list for-push" is used if:
    - we are scouting out for "refs in common" in preparation for a push;
    - the "push" capability has been advertised.
 * "list" is used in all other cases.
 * in particular, even if no capabilities are advertised, the "list"
   command will be used.

It's been this way since v1.6.6-rc0~22^2~26 (2009-10-30).  Anyway, the
rule doesn't sound very principled --- if the "push" capability is not
advertised, isn't the push going to error out anyway?  Why fall back
to a normal "list" which is likely not to work, either?

One might suggest:

A.
 - if scouting for "refs in common", use "list for-push";
 - otherwise, use "list" without for-push;
 - in all cases, check for an appropriate capability first
   (push/export in the for-push case, fetch/import otherwise)

At first it sounds ok but this one makes for a lousy story when new
capabilities are invented.  If "git remote-helper" only supports
fetching with a "better-import" capability that my copy of git does
not support, I will still try "git ls-remote helper::url" to learn
what I am missing.

B.
 - if scouting for "refs in common", use "list for-push";
 - otherwise, use "list" without for-push;
 - all remote helpers must implement both "list" and "list for-push".

This rule sounds better, and it doesn't seem to break remote-testgit
(which currently only receives "list" commands without "for-push"
because it only advertises "export" and not "push").  What do you
think?

Signed-off-by: Jonathan Nieder <redacted>
---
Probably I confused myself too far after reading in the manpage that
for-push is an attribute.  Note to self: it isn't.

 transport-helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 0c5b1bd..6d3d15e 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -802,7 +802,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 		return transport->get_refs_list(transport, for_push);
 	}
 
-	if (data->push && for_push)
+	if (for_push)
 		write_str_in_full(helper->in, "list for-push\n");
 	else
 		write_str_in_full(helper->in, "list\n");

-- 
1.7.4.1