Re: [PATCH 8/9] hook: teach 'hookcmd' config to alias hook scripts

On Thu, Jul 22 2021, Emily Shaffer wrote:

The gain is that "git hook list" becomes a trivial "git config -show-origin --show-scope --get-regexp" wrapper.

So the series either doesn't need "git hook list" or such a thing becomes much less complex, especially given the proposed addition of other features in the area like "git hook edit", i.e. (quoting the linked E-Mail):

    As just one example; surely "git config edit <name>" would need to
    run around and find config files to edit, then open them in a loop
    for you, no?

    Which we'd eventually want for "git config" in general with an
    --edit-regexp option or whatever, which brings us (well, at least
    me) back to "then let's just add it to git-config?".

That design choice also means that you can't expand the path using "git config --get --type=path.

We do have that with the "includeIf" construct, but if we can avoid it we should, it makes it play nicer with other assumptions and features of the config system.

As noted in the follow-up reply while we don't case normalize the LeVeL" part of "ThReE.LeVeL.KeY" that's tolower(), which we know isn't a 1=1 mapping on some FS's. https://lore.kernel.org/git/87y2ebo16v.fsf@evledraar.gmail.com/

Before diving into that, I'll just say I don't care about the trivial specifics of how this is done, i.e. the bikeshedding of what the config keys etc. are named.

Just in (as noted above) design choices here forcing avoidable complexity in other areas.

...

To reply to all the above, yes, your suggestion comes out ahead in being less verbose.

But isn't the real difference not in the differing prefixes, i.e. hook.* and hookcmd.* (A) v.s. always hook.* (B, which is what I'm mainly focusing on, i.e. what requires the added complexity.

But that in that in your proposed way of doing it it's:

    hook.<event-or-name>.*

V.s. my suggestion of:

    hook.<name>.*

And thus whenever you have a <event-or-name> that just happens to be a built-in hook listed in githooks(5) we in (A) implicitly expand config like:

    hook.post-commit.command = echo foo

To:

    hook.post-commit.command = echo hi
    hook.post-commit.type    = post-commit

But not knowing about "foo" we don't expand:

    hook.foo.command = echo foo

To:

    hook.foo.command = echo hi
    hook.foo.type    = foo # This would be an error, or ignored.

But rather leave "dangling" for the user to later supply the "*.event" themselves, i.e.:

    hook.foo.command = echo hi
    hook.foo.event = post-commit

And means that you presumably need to detect this case and error about it, but my proposed model does not:

    hook.post-commit.command = echo hi
    # User error: "*.type" and <event-or-name>" does not match for
    # "hook.*.command"
    hook.post-commit.type    = pre-commit

And furthermore, that if someone in your model were to do:

    hook.verify-commit.command = echo hi

It's "dangling" today, but if a later version of git learns about a "verify-commit" hook we'll start running it unexpectedly.

Because your design conflates the slot for known hook type names and user-supplied names.

On balance I think it's better just to always supply two lines per-hook, but whether we have this proposed shorthand or not is mostly orthogonal to everything I mentioned in https://lore.kernel.org/git/87mtv8fww3.fsf@evledraar.gmail.com/

I.e. my proposed version could also have it, but thinking about it I think it's not worth it, we should always use <name>, not <event-or-name> for the reasons noted if you'll read ahead...

...which are among other things that no, my proposed version doesn't have "skip" at all, see point #3 at https://lore.kernel.org/git/87mtv8fww3.fsf@evledraar.gmail.com/

I.e. I think the "skip" is a thing that falls out of a complexity of your design that I'm proposing to do away with.

That complexity being that you use <event-or-name> and I use <name>, and you want to in turn support N number of "*.command" for any "hook.<event-or-name>".

I'm suggesting we use the typical "last key wins" semantics, if you want multiple commands for a given hook type you'll just supply multiple "hook.<name>" sections with the same "hook.*.event = type" in all.

The way to skip hooks in my proposal is:

    hook.<name>.command = true

Or whatever noop command you want on the RHS. In practice we wouldn't invoke "true", just like we don't invoke "cat" for "PAGER=cat".

But unlike "*.skip" that doesn't require complexity in implementation or user understanding, it just falls naturally out of the rest of the model.

It's been so long since I wrote the original E-Mail that I'm having to skim my own summary there & come up with things as I go along, so maybe this conflicts with something I said earlier.

But no, I don't think I meant that *.event should be multi-value, or that that magic is worthwhile. I.e. I think we just want:

    [hook "not-a-reusable-section-1"]
        command = /long/path/reusable-hook
        event = post-commit
    [hook "not-a-reusable-section-2"]
        command = /long/path/reusable-hook
        event = pre-commit
    [hook "not-a-reusable-section-3"]
        command = /long/path/reusable-hook
        event = prepare-commit-msg

I.e. is wanting to use the same command for N different hooks really that common a use-case to avoid a little verbosity, and quite a lot of complexity?

How would such a hook even be implemented?

We don't have anything in your hook execution model (and I don't remember you adding this) which passes down a "you are this type of hook" to hooks.

It's now implicit in that the hook invoked at .git/hooks/post-commit or .git/hooks/pre-commit can check its own basename, but it won't be with configurable hooks.

We could start passing down a GIT_HOOK_TYPE in the environment or whatever, but I think the simpler case of just having the user do it is better.

I'm assuming that mainly because people have wanted a "/long/path/reusable-hook" router script because we have not supported executing N hooks, or supported concurrency. Once we do that the complexity of such routing scripts (mostly, not everyone's needs will be met) will be replaced by a little bit of config.

I don't see why it's worth it to micro-optimize for lines in that config at the cost of increased config complexity.

For example, how do you "skip" an "event" type? You have it for "*.command"? So let's say in your model (A) I have system config like:

    [hook "my-reusable"]
    command = /some/commit-msg-hook
    event = prepare-commit-msg

Now I want to skip that, easy enough, in my local config:

    [hook "my-reusable"]
    skip = true

Or if I wanted to also invoke it for commit-msg events, in my local config (which piggy-backs on the system one):

    # Now runs for both prepare-commit-msg and commit-message
    [hook "my-reusable"]
    event = commit-msg

Or, if I want to skip the command name and substitute my own:

    [hook "my-reusable"]
    skip = true
    command = /my/commit-msg-hook

I'm just assuming you mean that to work, as discussed above I think all this is leading us down the road of too much complexity.

But anyway, now I want to instead not run it as a prepare-commit-msg hook, but a commit-msg hook.

As far as I can tell you haven't specified that, but I think something consistent with your model would be

    [hook "my-reusable"]
    event = skip
    event = commit-msg

Or rather, because that conflates "skip" with a hook event type name:

    [hook "my-reusable"]
    skipEvent = true
    event = commit-msg

I.e. the "skip" you have now really means "skipCommand", so we'll need a skipSomeName to skip other "hook.*.someName".

As noted above I think all of this is too complex, let's just do the system config like this (exact same as the above):

    [hook "my-reusable"]
    command = /some/commit-msg-hook
    event = prepare-commit-msg

To skip it, in my local config:

    [hook "my-reusable"]
    command = true

And to re-use /some/commit-msg-hook without having to re-include it in your local config the answer is that we don't support that level of cleverness. Just do this:

    # First skip the system hook
    [hook "my-reusable"]
    command = true

    # Set up another hook, just copy/pasting the /some/commit-msg-hook command
    [hook "my-local"]
    command = /some/commit-msg-hook
    event = commit-msg

I.e. I think these cases of someone having say a /etc/gitconfig hook on their system not under their control that they want to not run, *but* not as the "event" there, but as another event type is too specific a use-case for us to worry about.

Even with I think it's fair to say deep knowledge of your proposal at this point I still needed to read this a few times to see if that:

    command = reusable-hook

Is referring to:

    [hookcmd "reusable-hook"]

I.e. is it going to run:

    command = /long/path/reusable-hook

Or is it just re-specifying /long/path/reusable-hook but relying on a $PATH lookup?

Having reasoned through that I think the answer is the former. But that also means that in your model:

    [hookcmd "rm -rf /"]
    command = echo this will not actually destroy your data
    [hook "pre-commit"]
    command = rm -rf /

Is going to run that friendly "echo" command, since "command = rm -rf /" just refers to the "rm -rf /" <name>, not <command>, right the "hookcmd" line is removed, at which point we'll stop treating it as a <name> and run it as a <command>?

In practice I think users are unlikely to use actively harmful names like that.

I'm just making the point that I should not need to know about previous config to see if a "hook.pre-commit.command = rm -rf /" is harmless or not, or need to carefully squint to see if the "reusable-hook" is referring to a section name or command name.

Or am I just confused at this point?

No, that's not what I'm proposing. I.e. "*.event" is reserved for built-in events listed in githooks(5) that git itself knows about.

If you say "event = reusable-hook" that's going to be one of ignored, warned or errored about.

(Probably "ignored" is the best thing to support multi-version setups, but that's not hook-specific, just how we treat unknown config in general)

My version of this would be the same as noted with "we don't support that level of cleverness[...]" above.

I.e. you'd not re-use that "/long/path/reusable-hook", you can skip it through "command = true", then just copy the relevnt part into your new config. So:

        (global)
	[hook "reusable-hook"]
		command = /long/path/reusable-hook
		event = pre-commit

	(local)
	[hook "reusable-hook"]
		command = true # skip it

        # The "hooks" name is arbitrary, "my-hooks" or whatever would be
        # clearer, but just going with your example...

	[hook "hooks"]
		event = post-commit
		command = hooks
        # Not very reusable then...   
	[hook "reusable-hook"]
                command = /long/path/reusable-hook
		event = pre-commit

I couldn't find this at a quick glance but I think we also had a disussion at some point about hooks controlling parallelism. AFAICT your current implementation just has global:

    hook.jobs=N

And we then blacklist certain hooks to always have hook.jobs=1, e.g. the commit-msg hook that needs an implicit "lock" on that file (or rather, we think that's the most common use-case).

I think my version of always having hook.<name>.{event,command} be one value is also better in that case, i.e. we'd then:

    [hook "myhook"]
    command = some-command
    event = pre-receive
    parallel = true # the default

    [hook "myhook2"]
    command = some-command2
    event = pre-receive
    parallel = true # the default

    [hook "myhook3"]
    command = some-unconcurrent-command
    event = pre-receive
    parallel = false # I'm not OK with concurrency

To go along with something like:

    hook
	jobs = 8

Or:

    [hookEvent "pre-receive"]
    jobs = 4

But if you have N numer of "command" in a section it gets murky, does "parallel = false" then apply to the whole section, but sections can have one or more values. So we'd need both a "hookEvent.pre-receive.jobs=N" and per-section config to control/suppress parallelism?

I haven't thought about it deeply, but have a hunch that having sections be a 1=1 mapping with a specific command instead of either 1=1 or 1=many is going to make that easier to implement and understand.