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

On 02/22, Jeff King wrote:

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

quoted

On Tue,  6 Feb 2018 17:12:41 -0800
Brandon Williams [off-list ref] wrote:

quoted

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 <redacted>

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

quoted

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