RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Ævar Arnfjörð Bjarmason]

On Sat, Apr 23, 2016 at 1:33 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Change the hardcoded lookup for .git/hooks/* to optionally lookup in
> $(git config core.hooksDirectory)/* instead if that config key is set.

I think this'll do for my use-case, but I started with a rather more
ambitious patch that I could forsee not finishing today.

I wanted to support executing e.g. the pre-commit hook, in order, from any of:

    .git/hooks/pre-commit
    .git/hooks/pre-commit.d/*
     [We could add some ~-wide path here I guess...]
     /etc/git/hooks/pre-commit
     /etc/git/hooks/pre-commit.d/*

Where the * would be resolved in glob() order.

The motivation was solving the use-case I'm solving with
core.hooksDirectory, perhaps with a config variable to set whether you
wanted to skip per-repo or system-wide hooks, but also having
something more general.

The reason for supporting the *.d directories was that I spotted a lot
of hooks people had hacked up at work using the pee(1) command[1] to
run sequences of other unrelated hook commands.

Just symlinking stuff is simpler and more portable if we do the work
in git.git once. We'd run  The pee(1) command also doesn't quit on the
first command that returns nonzero, which would make sense e.g. for
pre-commit hooks.

I have it working for the hooks that use the simple run_hook_ve()
interface, but the ones that have to e.g. pass input on stdin just
find_hook() directly & do a custom run_command(), so all of those
callsites would have to be patched and/or I'd have to hack up some
custom callback mechanism.

1. http://manpages.ubuntu.com/manpages/xenial/en/man1/pee.1.html

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> The reason for supporting the *.d directories was that I spotted a lot
> of hooks people had hacked up at work using the pee(1) command[1] to
> run sequences of other unrelated hook commands.

IIRC, we wanted to do this several years ago but after discussion
decided that we didn't want to have this in the core, because we
didn't want to hardcode the policy on interaction among multiple
hooks.

You can easily resolve the ordering of hooks--just declare that they
are executed sequentially in strcmp() order of filenames and users
will know to prefix them with fixed-number-of-digits to force their
desired ordering without complaining.

What is harder and the core part cannot unilaterally dictate is what
should happen after seeing a failure/rejection from a hook.  Some
hooks among the remainder would not want to be even called.  Some
others do want to be called but want to learn that the previous
hooks already have decided to fail/reject the operation.  There may
even be some others that cannot be moved to earlier part of the hook
chain for other external constraints (e.g. side effect of some
previous hook is part of its input), but would want to override the
previous decision to reject and let the operation pass.

I am happy to see that the idea brought back alive again, but I
think we prefer this start its life clearly marked as "highly
experimental and subject to change", then invite interested and
brave users who tolerate backward incompatible changes to
experiment, in order to allow us to gauge what the right semantics
and flexibility the users would want.  One way to do so may be an
opt-in configuration variable e.g. "experimental.multiHooks";
another may be to implement the logic as a pair of scripts (one for
the command line argument variant, the other for stdin variant) and
ship them in contrib/.

The latter approach (i.e. scripting) might be easier for people to
experiment and tweak, and in the olden days that would certainly be
the approach would would have taken, but I am not too afraid of
appearing uninviting to casual scripters anymore these days, so...

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 25, 2016 at 7:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> The reason for supporting the *.d directories was that I spotted a lot
>> of hooks people had hacked up at work using the pee(1) command[1] to
>> run sequences of other unrelated hook commands.
>
> IIRC, we wanted to do this several years ago but after discussion
> decided that we didn't want to have this in the core, because we
> didn't want to hardcode the policy on interaction among multiple
> hooks.

Ah, would be interesting to see that discussion if someone knows
enough to dig it up, didn't find it with some brief searching.

> You can easily resolve the ordering of hooks--just declare that they
> are executed sequentially in strcmp() order of filenames and users
> will know to prefix them with fixed-number-of-digits to force their
> desired ordering without complaining.

In principle you're describing glob() order here:
http://pubs.opengroup.org/onlinepubs/7908799/xsh/glob.html

We don't set LC_COLLATE in git so this'll be C order, which I think
will just fall back to strcmp().

If it doesn't and there's a functional difference, I'm not sure, it's
probably less confusing to use glob() order, since that's what you'll
get with shell expansion.

I.e. it would be confusing if you expand the hooks in the shell, and
git executes them in a slightly different order.

> What is harder and the core part cannot unilaterally dictate is what
> should happen after seeing a failure/rejection from a hook.  Some
> hooks among the remainder would not want to be even called.  Some
> others do want to be called but want to learn that the previous
> hooks already have decided to fail/reject the operation.  There may
> even be some others that cannot be moved to earlier part of the hook
> chain for other external constraints (e.g. side effect of some
> previous hook is part of its input), but would want to override the
> previous decision to reject and let the operation pass.

I think it's fair enough to say that if we had this facility this
would be good enough:

 * Your hooks are executed in glob() order, local .git first, then /etc/git/...

 * If it's a hook like pre-commit that can reject something the first
hook to fail short-circuits. I.e. none of the rest get executed.

 * If it's not a hook like that e.g. post-commit all of the hooks will
get executed.

 * If you need anything more complex you can just wrap your hooks in
your own shellscript.

I.e. it takes care of the common case where:

 * You just want to execute N hooks and don't want to write a wrapper.

 * For pre-* hooks the common case is it doesn't matter /what/
rejected things, just that it gets rejected, e.g. for pre-receive.
Also if you care about performance you can order them in
cheapest-first order.

> I am happy to see that the idea brought back alive again, but I
> think we prefer this start its life clearly marked as "highly
> experimental and subject to change", then invite interested and
> brave users who tolerate backward incompatible changes to
> experiment, in order to allow us to gauge what the right semantics
> and flexibility the users would want.  One way to do so may be an
> opt-in configuration variable e.g. "experimental.multiHooks";
> another may be to implement the logic as a pair of scripts (one for
> the command line argument variant, the other for stdin variant) and
> ship them in contrib/.

Makes sense to have an experimental.* config tree for git for stuff like this.

> The latter approach (i.e. scripting) might be easier for people to
> experiment and tweak, and in the olden days that would certainly be
> the approach would would have taken, but I am not too afraid of
> appearing uninviting to casual scripters anymore these days, so...

Yeah, actually one thing I didn't think of is that the core.hooksPath
patch I submitted makes this rather trivial to implement as a
collection of scripts...

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Marc Branchaud]

On 2016-04-26 06:58 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> Makes sense to have an experimental.* config tree for git for stuff like this.

I disagree.

* If the point is to express some kind of warning to users, I think the
community has been much better served by leaving experimental settings
undocumented (or documented only in unmerged topic branches).  It feels like
an experimental.* tree is a doorway to putting experimental features in
official releases, which seems odd considering that (IMHO) git has so far
done very well with the carefully-planned-out integration of all sorts of
features.

* Part of the experiment is coming up with appropriate configuration knobs,
including where those knobs should live.  Often such considerations lead to a
better implementation for the feature.  Dumping things into an experimental.*
tree would merely postpone that part of the feature's design.

* Such a tree creates a flag day when the experimental feature eventually
becomes a "real" feature. That'll annoy any early adopters. Sure, they
*should* be prepared to deal with config tree bike-shedding, but still that
extra churn seems unnecessary.

		M.

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 26, 2016 at 3:40 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
> On 2016-04-26 06:58 AM, Ævar Arnfjörð Bjarmason wrote:
>>
>> Makes sense to have an experimental.* config tree for git for stuff like this.
>
> I disagree.
>
> * If the point is to express some kind of warning to users, I think the
> community has been much better served by leaving experimental settings
> undocumented (or documented only in unmerged topic branches).  It feels like
> an experimental.* tree is a doorway to putting experimental features in
> official releases, which seems odd considering that (IMHO) git has so far
> done very well with the carefully-planned-out integration of all sorts of
> features.
>
> * Part of the experiment is coming up with appropriate configuration knobs,
> including where those knobs should live.  Often such considerations lead to a
> better implementation for the feature.  Dumping things into an experimental.*
> tree would merely postpone that part of the feature's design.
>
> * Such a tree creates a flag day when the experimental feature eventually
> becomes a "real" feature. That'll annoy any early adopters. Sure, they
> *should* be prepared to deal with config tree bike-shedding, but still that
> extra churn seems unnecessary.

By "stuff like this", yeah I did mean, and I assume Junio means,
putting "experimental" features in official releases.

E.g. perl does this, if you type "perldoc experimental" on a Linux box
you'll get the documentation.

Basically you can look at patches to a project on a spectrum of:

 1. You hacked something up locally
 2. It's in someone's *.git repo as a POC
 3. It's a third-party patch series used by a bunch of people
 4. In an official release but documented as experimental
 5. In an official release as a first-rate feature

Something like an experimental.WHATEVER=bool flag provides #4.

I think aside from this feature just leaving these things undocumented
really provides the worst of both worlds.

Now you have some feature that's undocumented *because* you're not
sure about it, but you can't ever be sure about it unless people
actually use it, and if it's not documented at all effectively it's as
visible as some third-party patch series. I.e. only people really
involved in the project will ever use it.

Which is why perl has the "experimental" subsystem, it allows for
playing around with features the maintainers aren't quite sure about
in official releases, and the users know they're opting in to trying
something unstable that may go away or have its semantics changed from
under them.

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Christian Couder]

On Tue, Apr 26, 2016 at 6:09 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Tue, Apr 26, 2016 at 3:40 PM, Marc Branchaud <marcnarc@xiplink.com> wrote:
>> On 2016-04-26 06:58 AM, Ævar Arnfjörð Bjarmason wrote:
>>>
>>> Makes sense to have an experimental.* config tree for git for stuff like this.
>>
>> I disagree.
>>
>> * If the point is to express some kind of warning to users, I think the
>> community has been much better served by leaving experimental settings
>> undocumented (or documented only in unmerged topic branches).

There are features considered experimental that are documented, like
--split-index in `git update-index` and `git interpret-trailers`.

As far as I know the possible reasons they are considered experimental are:

- in the release notes they were described as "experimental",
- they are not considered complete, because of missing features,
- they might not be useful as is,
- they might be buggy in the real world.

>> It feels like
>> an experimental.* tree is a doorway to putting experimental features in
>> official releases, which seems odd considering that (IMHO) git has so far
>> done very well with the carefully-planned-out integration of all sorts of
>> features.

Some people have been happily experimenting with or even using some of
the experimental features, like the ones I am talking about above.

>> * Part of the experiment is coming up with appropriate configuration knobs,
>> including where those knobs should live.

I agree that it can be difficult to find the right knobs for any new feature.

>> Often such considerations lead to a
>> better implementation for the feature.  Dumping things into an experimental.*
>> tree would merely postpone that part of the feature's design.

I am not so sure about this. For example `git update-index
--untracked-cache` used to be considered experimental, but it
definitely had knobs that were not right, so its UI has been reworked
recently.

Maybe if it had first been created with an UI in an experimental.*
config tree, rather than only options to `git update-index` it would
have been an easier transition.

>> * Such a tree creates a flag day when the experimental feature eventually
>> becomes a "real" feature. That'll annoy any early adopters. Sure, they
>> *should* be prepared to deal with config tree bike-shedding, but still that
>> extra churn seems unnecessary.

Not sure about that. New config options outside the experimental.*
config tree could always take over config options in the
experimental.* config tree, as if they appear, it means that the user
is aware that they are the new way to configure the feature. Then the
cost of supporting both the old options in the experimental.* config
tree and the new ones outside should be very small, especially if
there are not many changes between the two set of options.

If there are a lot of changes between these two sets, it means that we
were probably right to have the old ones in the experimental.* config
tree. And in the worst case, which should hardly ever happen, we can
probably afford to just emit warnings saying "old 'experimental.XXXX'
config option is not supported anymore, please use YYYY config option
instead" and just ignore the 'experimental.XXXX' config option.

> By "stuff like this", yeah I did mean, and I assume Junio means,
> putting "experimental" features in official releases.
>
> E.g. perl does this, if you type "perldoc experimental" on a Linux box
> you'll get the documentation.
>
> Basically you can look at patches to a project on a spectrum of:
>
>  1. You hacked something up locally
>  2. It's in someone's *.git repo as a POC
>  3. It's a third-party patch series used by a bunch of people
>  4. In an official release but documented as experimental
>  5. In an official release as a first-rate feature

Yeah, and it seems to me that Git also has:

4.5. In an official release, documented, but scarcely documented as experimental

and in fact more stuff under 4.5. than under 4.

And in Git there is also the notion of porcelain vs plumbing, where
porcelain output can more easily be changed a bit than plumbing
output.

> Something like an experimental.WHATEVER=bool flag provides #4.
>
> I think aside from this feature just leaving these things undocumented
> really provides the worst of both worlds.

Yeah I agree. Undocumented stuff in Git is already for stuff that we
don't want people to use.

> Now you have some feature that's undocumented *because* you're not
> sure about it, but you can't ever be sure about it unless people
> actually use it, and if it's not documented at all effectively it's as
> visible as some third-party patch series. I.e. only people really
> involved in the project will ever use it.
>
> Which is why perl has the "experimental" subsystem, it allows for
> playing around with features the maintainers aren't quite sure about
> in official releases, and the users know they're opting in to trying
> something unstable that may go away or have its semantics changed from
> under them.

An "experimental" subsystem is perhaps overkill for Git though.

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Marc Branchaud]

On 2016-04-26 12:09 PM, Ævar Arnfjörð Bjarmason wrote:
>
> Basically you can look at patches to a project on a spectrum of:
>
>   1. You hacked something up locally
>   2. It's in someone's *.git repo as a POC
>   3. It's a third-party patch series used by a bunch of people
>   4. In an official release but documented as experimental
>   5. In an official release as a first-rate feature
>
> Something like an experimental.WHATEVER=bool flag provides #4.

But the git project already does #4 without needing a special 
configuration tree.  In fact, you ignored the git project's "pu" branch 
entirely.  Once a feature gets onto the "next" branch, it's already much 
less experimental.  If it needs to put the word "experimental" in its 
config settings, then maybe shouldn't leave the "pu" branch in the first 
place.

> I think aside from this feature just leaving these things undocumented
> really provides the worst of both worlds.

Yes, I apologize.  I did not mean that things should remain 
undocumented, only that if you're afraid of users harming themselves 
then you're better off not documenting settings than labelling them as 
experimental.

> Now you have some feature that's undocumented *because* you're not
> sure about it, but you can't ever be sure about it unless people
> actually use it, and if it's not documented at all effectively it's as
> visible as some third-party patch series. I.e. only people really
> involved in the project will ever use it.

Slapping "experimental" on the configuration only serves to muddy the 
waters.  Either the feature is good enough to be tried by normal users, 
or it isn't.  If it isn't ready for normal users, let it cook in pu (or 
next) for a while longer.

Git has got on just fine without having some special designation for 
not-ready-for-prime-time features, mostly because the development 
process lends itself naturally to gradually exposing things as they 
mature: topics move from the list to pu to next to master.  (The string 
"experiment" only appears 16 times in all the release notes, which I 
think is something the git project can be proud of.)

To me, designating part of the config tree as "experimental" enables 
sloppier practices, where a feature can be released with a bit less 
effort than might otherwise be acceptable, because it's clearly marked 
as experimental, and so anyone trying it out surely has the requisite 
bulletproof footwear.  (I don't mean to imply that you or any other git 
contributor might slack off on any work you do for the project. It's 
more that the ability to easily designate something as experimental 
lowers the bar a bit, and makes it more tempting to release 
not-quite-ready features.)

Far better to instead work on the feature until it's as ready as can be, 
and release it properly.

In this particular case, for example, I think your proposed approach is 
perfectly fine and does not need to be designated as "experimental" in 
any way.  With a reasonable "hooks.something" config variable to turn on 
directory support, what you've described sounds like a great feature. 
Sure, it may have bugs, it may have unintended consequences, it may not 
fulfill some odd corner cases.  But that's true for almost everything. 
As with every other git feature, it'll be developed to the best of the 
project's abilities and released.  Future patches are welcome.

		M.

Re: RFC: Supporting .git/hooks/$NAME.d/* && /etc/git/hooks/$NAME.d/*

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I think it's fair enough to say that if we had this facility this
> would be good enough:
>
>  * Your hooks are executed in glob() order, local .git first, then /etc/git/...
>
>  * If it's a hook like pre-commit that can reject something the first
> hook to fail short-circuits. I.e. none of the rest get executed.
>
>  * If it's not a hook like that e.g. post-commit all of the hooks will
> get executed.
>
>  * If you need anything more complex you can just wrap your hooks in
> your own shellscript.
>
> I.e. it takes care of the common case where:
>
>  * You just want to execute N hooks and don't want to write a wrapper.
>
>  * For pre-* hooks the common case is it doesn't matter /what/
> rejected things, just that it gets rejected, e.g. for pre-receive.
> Also if you care about performance you can order them in
> cheapest-first order.

Stop using the word "common" to describe what is not demonstratably
"common".

The above only covers a very limited subset of the use cases, which
is the two bullet points above (one of them i.e. "I do not bother to
write a wrapper" is not even a valid use case).  That may be a good
starting point, but it is so simple that can be done with a wrapper
with several lines at most.  So I am not sympathetic to that line of
reasoning at all.

I can buy "It is too cumbersome to require everybody to reinvent and
script the cascading logic, and the core side should help by giving
a standard interface that is flexible enough to suit people's need",
though.

And I have to say that a sequential execution that always
short-circuits at the first failure is below that threshold.

One reason I care about allowing the users to specify "do not
shortcut" is that I anticipate that people would want to have a
logging of the result at the end of the chain.