Subject: Re: [PATCH v3 04/35] upload-pack: convert to a builtin

From: Brandon Williams <[email-masked]>
Date: Thu, 22 Feb 2018 10:07:15 -0800

On 02/22, Jeff King wrote:
> On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote:
> 
> > On Tue,  6 Feb 2018 17:12:41 -0800
> > Brandon Williams <[email-masked]> wrote:
> > 
> > > In order to allow for code sharing with the server-side of fetch in
> > > protocol-v2 convert upload-pack to be a builtin.
> > > 
> > > Signed-off-by: Brandon Williams <[email-masked]>
> > 
> > As Stefan mentioned in [1], also mention in the commit message that this
> > means that the "git-upload-pack" invocation gains additional
> > capabilities (for example, invoking a pager for --help).
> 
> And possibly respecting pager.upload-pack, which would violate our rule
> that it is safe to run upload-pack in untrusted repositories.

And this isn't an issue with receive-pack because this same guarantee
doesn't exist?

> 
> (This actually doesn't work right now because pager.* is broken for
> builtins that don't specify RUN_SETUP; but I think with the fixes last
> year to the config code, we can now drop that restriction).
> 
> Obviously we can work around this with an extra RUN_NO_PAGER_CONFIG
> flag. But I think it points to a general danger in making upload-pack a
> builtin. I'm not sure what other features it would want to avoid (or
> what might grow in the future).
> 
> > Having said that, the main purpose of this patch seems to be to libify
> > upload-pack, and the move to builtin is just a way of putting the
> > program somewhere - we could have easily renamed upload-pack.c and
> > created a new upload-pack.c containing the main(), preserving the
> > non-builtin-ness of upload-pack, while still gaining the benefits of
> > libifying upload-pack.
> 
> Yeah, this seems like a better route to me.
> 
> -Peff

-- 
Brandon Williams