On Wed, Feb 21, 2018 at 01:44:22PM -0800, Jonathan Tan wrote: > On Tue, 6 Feb 2018 17:12:41 -0800 > Brandon Williams <bmw...@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 <bmw...@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). > 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
