threads / rfc / 26776

RFC patchDocumentation/remote-helpers: explain capabilities first

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

## tl;dr

6 messages between Mar 18, 2011 and Mar 19, 2011. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Jonathan Nieder· Mar 18, 2011, 17:45 UTC · lore

The current remote helper documentation is from the perspective of git, so to speak: it presents a full menu of commands for a person invoking a remote helper to choose from. In practice, that's less useful than it could be, since the daunted novice remote-helper author probably just wanted a list of commands she needs to implement to get started. So preface the command list with an overview of each capability, its purpose, and what commands it requires.

As a side effect, this makes it a little clearer that git doesn't just choose arbitrary commands to run, even if the remote helper advertises all capabilities --- instead, there are well defined command sequences for various tasks.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
No documentation for "export" and "gitdir" yet (by the way: would it
make sense to export the GIT_DIR environment variable so a round trip
for the 'gitdir' capability would eventually be unnecessary?).

The current documentation left me lost, so I wrote this (which still probably leaves one lost). Thoughts?

 Documentation/git-remote-helpers.txt |  101 +++++++++++++++++++++++++--------
 1 files changed, 76 insertions(+), 25 deletions(-)
Show changes to Documentation/git-remote-helpers.txt +76 −24
diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index 87cd11f..e7a3985 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -29,16 +29,87 @@ commands generally concern facilities like discovering and updating
 remote refs, transporting objects between the object database and
 the remote repository, and updating the local object store.
 
-Helpers supporting the 'fetch' capability can discover refs from the
-remote repository and transfer objects reachable from those refs to
-the local object store. Helpers supporting the 'push' capability can
-transfer local objects to the remote repository and update remote refs.
-
 Git comes with a "curl" family of remote helpers, that handle various
 transport protocols, such as 'git-remote-http', 'git-remote-https',
 'git-remote-ftp' and 'git-remote-ftps'. They implement the capabilities
 'fetch', 'option', and 'push'.
 
+CAPABILITIES
+------------
+
+Different remote helpers expect to communicate with git in different
+ways.  These are declared to git through the helper's response to the
+`capabilities` command (see COMMANDS, below).
+
+'option'::
+	Can accept settings like `verbosity` (how much output to write
+	to stderr) and `depth` (how much history is wanted in the case
+	of a shallow clone) that affect how other commands are carried
+	out.
++
+Supported commands: 'option'.
+
+Capabilities for Pushing
+~~~~~~~~~~~~~~~~~~~~~~~~
+'connect'::
+	Can attempt to connect to 'git receive-pack' (for pushing),
+	'git upload-pack', etc for communication using the
+	packfile protocol.
++
+Supported commands: 'connect'.
+
+'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'.
+
+If a helper advertises both 'connect' and 'push', git will use
+'connect' if possible and fall back to 'push' if the helper requests
+so when connecting (see the 'connect' command under COMMANDS).
+
+Capabilities for Fetching
+~~~~~~~~~~~~~~~~~~~~~~~~~
+'connect'::
+	Can try to connect to 'git upload-pack' (for fetching),
+	'git receive-pack', etc for communication using the
+	packfile protocol.
++
+Supported commands: 'connect'.
+
+'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'.
+
+If a helper advertises 'connect', git will use it if possible and
+fall back to another capability if the helper requests so when
+connecting (see the 'connect' command under COMMANDS).
+When choosing between 'fetch' and 'import', git prefers 'fetch'.
+Other frontends may have some other order of preference.
+
+'refspec' <refspec>::
+	This modifies the 'import' capability.
++
+A helper advertising
+`refspec refs/heads/{asterisk}:refs/svn/origin/branches/{asterisk}`
+in its capabilities is saying that, when it handles
+`import refs/heads/topic`, the stream it outputs will update the
+`refs/svn/origin/branches/topic` ref.
++
+This capability can be advertised multiple times.  The first
+applicable refspec takes precedence.  The left-hand of refspecs
+advertised with this capability must cover all refs reported by
+the list command.  If no 'refspec' capability is advertised,
+there is an implied `refspec {asterisk}:{asterisk}`.
+
 INVOCATION
 ----------
 
@@ -167,26 +238,6 @@ completing a valid response for the current command.
 Additional commands may be supported, as may be determined from
 capabilities reported by the helper.
 
-CAPABILITIES
-------------
-
-'fetch'::
-'option'::
-'push'::
-'import'::
-'connect'::
-	This helper supports the corresponding command with the same name.
-
-'refspec' 'spec'::
-	When using the import command, expect the source ref to have
-	been written to the destination ref. The earliest applicable
-	refspec takes precedence. For example
-	"refs/heads/*:refs/svn/origin/branches/*" means that, after an
-	"import refs/heads/name", the script has written to
-	refs/svn/origin/branches/name. If this capability is used at
-	all, it must cover all refs reported by the list command; if
-	it is not used, it is effectively "*:*"
-
 REF LIST ATTRIBUTES
 -------------------
 
-- 
1.7.4.1
Sverre Rabbelier· Mar 18, 2011, 18:07 UTC · re: Jonathan Nieder · lore

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

Heya,
On Fri, Mar 18, 2011 at 18:45, Jonathan Nieder <jrnieder@gmail.com> wrote:
> No documentation for "export" ... yet

Which is for the better, since I think the current interface is broken. It'll be easier to justify changing it if it's not documented ;).

> (by the way: would it
> make sense to export the GIT_DIR environment variable so a round trip
> for the 'gitdir' capability would eventually be unnecessary?).
Yes, that makes a lot of sense.
> The current documentation left me lost, so I wrote this (which still
> probably leaves one lost).  Thoughts?
I think it makes sense.
-- 
Cheers,

Sverre Rabbelier
Jonathan Nieder· Mar 18, 2011, 22:21 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier wrote:
> On Fri, Mar 18, 2011 at 18:45, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 5 quoted lines
>> No documentation for "export" ... yet
>
> Which is for the better, since I think the current interface is
> broken. It'll be easier to justify changing it if it's not documented
> ;).

Thanks for explaining. FWIW I'd be interested in your thoughts on this sometime (not now necessarily, just some time before the svn push support project is on the front burner :)).

Show 10 quoted lines
>> (by the way: would it
>> make sense to export the GIT_DIR environment variable so a round trip
>> for the 'gitdir' capability would eventually be unnecessary?).
>
> Yes, that makes a lot of sense.
>
>> The current documentation left me lost, so I wrote this (which still
>> probably leaves one lost).  Thoughts?
>
> I think it makes sense.
Glad to hear it.  (The GIT_DIR idea is Tom's iirc.)
Sverre Rabbelier· Mar 18, 2011, 22:34 UTC · re: Jonathan Nieder · lore

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

Heya,
On Fri, Mar 18, 2011 at 23:21, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Thanks for explaining.  FWIW I'd be interested in your thoughts on this
> sometime (not now necessarily, just some time before the svn push
> support project is on the front burner :)).

Any idea on when that will be? I've been wanting to finish push support for git-remote-hg for ages now, but haven't been able to make the time :(.

-- 
Cheers,

Sverre Rabbelier
Jonathan Nieder· Mar 18, 2011, 22:48 UTC · re: Sverre Rabbelier · lore

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

Sverre Rabbelier wrote:
> On Fri, Mar 18, 2011 at 23:21, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 5 quoted lines
>> Thanks for explaining.  FWIW I'd be interested in your thoughts on this
>> sometime (not now necessarily, just some time before the svn push
>> support project is on the front burner :)).
>
> Any idea on when that will be?
No, not sure.  My current priorities are
 - getting "git clone svndump-alpha::/home/jrn/dump/svn-kde0:1200000"
   to work;
 - mustering up the courage to clean up glibc vfprintf
After that, who knows?

But I hear that Ram has done some work on a fast-import to dumpfile converter, so push support is definitely simmering even if there is not much momentum yet.

Jonathan Nieder· Mar 19, 2011, 00:29 UTC · re: Jonathan Nieder · lore

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

Hi again,
Sorry for the spam.  Just ran into a quick puzzle:
Jonathan Nieder wrote:
Show 5 quoted lines
> +'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'.
[...]
Show 11 quoted lines
> +'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 <jrnieder@gmail.com>
---
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(-)
Show changes to transport-helper.c +1 −1
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

← back to recent threads