Re: [PATCH 6/6] upload-pack: provide a hook for running pack-objects

[Jeff King <peff@peff.net>]

On Wed, May 18, 2016 at 06:45:37PM -0400, Jeff King wrote:

> @@ -93,6 +94,14 @@ static void create_pack_file(void)
>  	int i;
>  	FILE *pipe_fd;
>  
> +	if (!pack_objects_hook)
> +		pack_objects.git_cmd = 1;
> +	else {
> +		argv_array_push(&pack_objects.args, pack_objects_hook);
> +		argv_array_push(&pack_objects.args, "git");
> +		pack_objects.use_shell = 1;
> +	}

I waffled on the "shell" thing here. It's more flexible, and matches
other commands we let the user specify (like diff.external). But it also
makes it harder to do more hook-like things, like "run this hook if it
exists and has the executable bit set".

So should it just be a path to a single command/script?

With the shell thing, you can set it to:

  test -e /path/to/script && /path/to/script ...

if you want, but that's a little more arcane (and incurs an extra shell
invocation that we could otherwise skip).

And on the same note, if it _were_ just path-to-script, would we want it
to skip the hook if it's missing or not executable? For normal hooks, if
that's the case, we know it's because the user doesn't want to use the
hook. But here, the user has taken an overt action to tell us about the
hook; would it be surprising if we silently skipped it because it wasn't
available?

I like that it would "fail open", so a misconfiguration doesn't break a
server. But maybe people would find that off-putting.

-Peff