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

From: Jeff King <peff@peff.net>
Date: Feb 22, 2018, 09:58 UTC
Message-ID: <20180222095833.GC12442@sigill.intra.peff.net>
In-Reply-To: <20180221134422.2386e1aca39fe673235590e7@google.com>

On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote:

Show 11 quoted lines

> On Tue,  6 Feb 2018 17:12:41 -0800
> Brandon Williams <bmwill@google.com> 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 <bmwill@google.com>
> 
> 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.

(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).

Show 6 quoted lines

> 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