Re: [PATCH/RFC 0/6] pack-objects hook for upload-pack

[Jeff King <peff@peff.net>]

On Tue, May 24, 2016 at 05:59:15PM -0700, Junio C Hamano wrote:

> On Wed, May 18, 2016 at 3:37 PM, Jeff King <peff@peff.net> wrote:
> > I've often wanted to intercept the call from upload-pack to
> > pack-objects. The final patch in this series goes into more detail, but
> > basically it's good for:
> >
> >   1. Capturing the output from pack-objects for debugging/inspection.
> >
> >   2. Capturing the input to pack-objects to replay for debugging or
> >      performance analysis.
> >
> >   3. Caching pack-objects output.
> >
> > It's pretty trivial to add a hook to run instead of pack-objects (and
> > the hook would just run pack-objects itself). But we don't want to run
> > hooks in upload-pack, because we don't necessarily trust the repository
> > we're running in.
> 
> Although I'd need to study the final step a bit more carefully than I did,
> overall I think these are good changes. The way the callbacks learn
> about the origin of the configuration may have to be rethought in the
> long run, though. We already have been relying on a filename thing
> kept separately as a global variable, and the approach taken by this
> series extends it, so we are not making anything fundamentally worse,
> but at some point we may need to bite the bullet and pass kv-info as
> an extra callback parameter or something.

Yeah, I had the same thought while working on this, but just didn't want
to have to tweak every config callback. As you say, I don't think this
makes anything fundamentally worse, though. I'm inclined to go with this
strategy, especially with the extra die("BUG") safety added here. But I
can look into changing the callbacks if you feel strongly.

-Peff

PS Did you have any thoughts on the t/helper problem mentioned in:

     http://article.gmane.org/gmane.comp.version-control.git/295029

   I suspect it will bite you if you try merging/testing this.