Re: [PATCH] archive: add archive.restrictRemote option

From: Jeff King <peff@peff.net>
Date: Feb 28, 2014, 09:07 UTC
Message-ID: <20140228090709.GB11709@sigill.intra.peff.net>
In-Reply-To: <xmqqtxbkz9jp.fsf@gitster.dls.corp.google.com>

On Thu, Feb 27, 2014 at 10:37:30AM -0800, Junio C Hamano wrote:

Show 7 quoted lines

> > Signed-off-by: Jeff King <peff@peff.net>
> 
> Thanks.
> 
> Do GitHub people have general aversion against signing off (or
> sending out, for that matter) their own patches, unless they were
> already active here before they joined GitHub, by the way?

Mostly it is that I clean up the patches and commit messages before sending them out. Michael sends out his own patches because they are already perfect by the time I see them. :)

I can certainly get S-O-B from GitHubbers, but my impression of the DCO is that it does not matter; as the first link in the signoff chain, I am certifying that the patch meets the licensing requirements. Of course, I know that because of my relationship to the author and our employer, which is something that isn't encoded in the headers. A S-O-B from the author would perhaps make it more obvious what happened.

Show 16 quoted lines

> I like the general idea and this escape hatch would be a good thing
> to have.
> 
> A few comments:
> 
>  - Seeing the word combination "restrict"+"remote" before reading
>    the explanation made me think "hmph, only allow remote archive
>    from certain hosts but not from others?"  We are restricting the
>    objects and only on the remote usage, not restricting remotes, so
>    somebody else may be able to come up with a less misleading name.
>
>  - It might be better to call the escape hatch "allow something"
>    that defaults to "false".  It is merely the issue of perception,
>    but having a knob to be limiting that defaults to true gives a
>    wrong impression that in an ideal world remote archive ought to
>    be loose and we are artificially limiting it by default.

After reading your first point, I came up with "archive.allowRemoteUnreachable", which also satisfies the second. I do not have a strong opinion.

Show 6 quoted lines

> > +archive.restrictRemote::
> > +	If true, archives can only be requested by refnames. If false,
> 
> As this does not affect local use of "git archive", "requested by
> refnames" may need to be clarified further.  Perhaps "remote
> archives can be requested only for published refnames" or something.

I was hoping to be vague. If we really want to get into specifics, we should probably document the current rules (refnames, and sub-trees of refnames). It might be a good thing to document that anyway, though. And by doing so, it would become obvious why one would want to set this option. I'll see what I can come up with.

-Peff