Re: [PATCH 09/10] hook: show config scope in git hook list

From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Mar 11, 2026, 11:47 UTC
Message-ID: <87h5qma8pe.fsf@collabora.com>
In-Reply-To: <abFC4uLP-JwDoKde@pks.im>

On Wed, 11 Mar 2026, Patrick Steinhardt <ps@pks.im> wrote:

Show 19 quoted lines

> On Mon, Mar 09, 2026 at 02:54:15AM +0200, Adrian Ratiu wrote:
>> diff --git a/builtin/hook.c b/builtin/hook.c
>> index 8fc647a4de..c806640361 100644
>> --- a/builtin/hook.c
>> +++ b/builtin/hook.c
>> @@ -70,7 +73,14 @@ static int list(int argc, const char **argv, const char *prefix,
>>  			printf("%s%c", _("hook from hookdir"), line_terminator);
>>  			break;
>>  		case HOOK_CONFIGURED:
>> -			printf("%s%c", h->u.configured.friendly_name, line_terminator);
>> +			if (show_scope)
>> +				printf("%s (%s)%c",
>> +				       h->u.configured.friendly_name,
>> +				       config_scope_name(h->u.configured.scope),
>> +				       line_terminator);
>
> Are we sure that this is always unambiguous? Can the friendly name for
> example contain a space itself, or is it possible that the scope gets
> extended eventually so that parsing becomes ambiguous?

Indeed, good catch, yes, friendly-name can contain a space and the scope can also be extended, so this is not very parseable as Junio also pointed out in the other message.

We need to come up with something better. :)

> I'm not sure about this myself, but that may indicate that we should
> maybe also separate the name and scope with a NUL byte.

Looking closer at how git config --show-scope does it, I think we could mirror that, i.e. print the scope as a tab separated prefix.

That would also avoid an awkward intra-line NUL separator, since NUL is already used for line termination with -z.

I have no strong opinions on this btw and am very open to suggestions.

Show 18 quoted lines

>> +			if (!ctx->kvi)
>> +				BUG("hook config callback called without key-value info");
>> +
>> +			/*
>> +			 * Stash the config scope in the util pointer for
>> +			 * later retrieval in build_hook_config_map(). This
>> +			 * intermediate struct is transient and never leaves
>> +			 * that function, so we pack the enum value into the
>> +			 * pointer rather than heap-allocating a wrapper.
>> +			 */
>> +			string_list_append(hooks, hook_name)->util =
>> +				(void *)(uintptr_t)ctx->kvi->scope;
>>  		}
>>  	} else if (!strcmp(subkey, "command")) {
>>  		/* Store command overwriting the old value */
>
> Okay. This is a bit ugly, but I guess it should work in practice? The
> alternative would be to allocate the scope and store the pointer here.

Yes, I just tried the simplest thing and it worked. If we need more than just the scope, then we could heap-allocate a struct wrapper and deal with the associated memory management complexity.