Re: [PATCH 2/2] Add a remote helper to interact with mediawiki (fetch & push)

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Sep 1, 2011, 23:44 UTC
Message-ID: <20110901234446.GA25817@elie>
In-Reply-To: <vpqobz53ig3.fsf@bauges.imag.fr>

Hi Matthieu,

Matthieu Moy wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Here's an old attempt to make the documentation a little easier to read,
>> and hopefully also to add to.
>
> Thanks, that helps. I wish we had all this earlier ;-).

Glad you like it.

>> +Git sends the remote helper a list of commands on standard input, one
>> +per line.  The first command is always the 'capabilities' command, 
>
> Do we want to set this in stone?

Yes, I think we should set it in stone. Helpers can use that as a sanity check to know they are actually being used as a remote helper. And specifying it makes the protocol more concrete.

> Wouldn't a Git implementation calling
> "option" before "capabilities" be correct?

At that point git doesn't know if the helper implements the "option" capability.

>> +Capabilities
>> +~~~~~~~~~~~~
>
> (perhaps name the section "Overview of Capabilities"?)

Yes, good idea.

[...]

Show 9 quoted lines

> Since this "Capabilities" section is meant to be an overview, I'd
> shorten this to
>
> +'refspec' <refspec>::
> +	This modifies the 'import' capability, allowing the produced
> +	fast-import stream to modify refs in a private namespace
> +	instead of writing to refs/heads or refs/remotes directly.
>
> and drop the detailed explanation here.

Another good idea.

Show 12 quoted lines

>> +Capabilities for Fetching
>> +~~~~~~~~~~~~~~~~~~~~~~~~~
> [...]
>> +'refspec' <refspec>::
>> +	This modifies the 'import' capability.
>
> Since this would be the "detailed explanation" part, this is the one
> readers will read more carefully, so I'd put the recommandation right
> here:
>
> +	It is recommended that all importers providing the 'import'
> +	capability use this.

Yet another. :)

Show 15 quoted lines

> and of course, keep this:
>
>> ++
>> +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}`.
>> +

Thanks much!  Will try to find time to resend later today.