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

From: Jeff King <peff@peff.net>
Date: May 19, 2016, 00:14 UTC
Message-ID: <20160519001433.GB22543@sigill.intra.peff.net>
In-Reply-To: <20160518224537.GF22443@sigill.intra.peff.net>

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

Show 11 quoted lines

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