Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec

[John Keeping <john@keeping.me.uk>]

On Thu, Apr 18, 2013 at 04:27:57AM -0500, Felipe Contreras wrote:
> On Thu, Apr 18, 2013 at 3:24 AM, John Keeping <john@keeping.me.uk> wrote:
> > On Wed, Apr 17, 2013 at 11:14:28PM -0500, Felipe Contreras wrote:
> >> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt
> >> index f506031..0c91aba 100644
> >> --- a/Documentation/gitremote-helpers.txt
> >> +++ b/Documentation/gitremote-helpers.txt
> >> @@ -174,8 +174,8 @@ 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 a helper does not need a specific 'refspec'
> >> -capability then it should advertise `refspec *:*`.
> >> +the list command.  If no 'refspec' capability is advertised,
> >> +there is an implied `refspec *:*`.
> >
> > This is wrong.
> 
> It has been like that since v1.7.0.

That doesn't mean it's right.  I suspect that it was originally correct,
but then 'export' was added at which point this became stale.

> > As your later patch makes clearer, there is no implied
> > refspec for push - it only works for fetch.  I found the wording you've
> > reverted to extremely misleading.  How about something like this:
> >
> >     For historical reasons, 'import' treats the absence of a 'refspec'
> >     line as equivalent to `refspec *:*`; remote helpers should always
> >     specify an explicit refspec.
> 
> Maybe my version was "misleading" because it didn't mention it was for
> historical reasons, but yours is plain wrong; remote helpers might not
> be using 'import' or 'export' at all, so not all remote helpers should
> always specify an explicit refspec. And if the previous text "It is
> recommended that all importers providing the 'import' capability use
> this. It's mandatory for 'export'." does not convey the idea these
> remote helpers should always specify an explicit refspec, I don't know
> what it does.

I didn't say mine was correct, but there was a reason that I wanted to
change the existing text.  Just going back to what was there before is
not a good way to make things better.

In my copy of pu I don't see the text "It's mandatory for 'export'", it
just stops after "It is recommended that all importers providing the
'import' capability use this".  That paragraph also starts with "This
modifies the 'import' capability" making no mention of export.

Perhaps we need a slightly more extensive re-write of the documentation
for refspec to make it very clear where it applies and when it is
needed.