ab/config-based-hooks-2 etc. (was: What's cooking in git.git (Nov 2021, #03; Tue, 9))

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Nov 11 2021, Emily Shaffer wrote:

> Once more, updates to submodule-UX-overhaul related work.
>
> On Tue, Nov 09, 2021 at 04:59:31PM -0800, Junio C Hamano wrote:
[...]
>
>> * ab/config-based-hooks-2 (2021-11-01) 18 commits
>>  - run-command: remove old run_hook_{le,ve}() hook API
>>  - receive-pack: convert push-to-checkout hook to hook.h
>>  - read-cache: convert post-index-change to use hook.h
>>  - commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
>>  - git-p4: use 'git hook' to run hooks
>>  - send-email: use 'git hook run' for 'sendemail-validate'
>>  - git hook run: add an --ignore-missing flag
>>  - hooks: convert worktree 'post-checkout' hook to hook library
>>  - hooks: convert non-worktree 'post-checkout' hook to hook library
>>  - merge: convert post-merge to use hook.h
>>  - am: convert applypatch-msg to use hook.h
>>  - rebase: convert pre-rebase to use hook.h
>>  - hook API: add a run_hooks_l() wrapper
>>  - am: convert {pre,post}-applypatch to use hook.h
>>  - gc: use hook library for pre-auto-gc hook
>>  - hook API: add a run_hooks() wrapper
>>  - hook: add 'run' subcommand
>>  - Merge branch 'ab/config-based-hooks-1' into ab/config-based-hooks-2
>> 
>>  More "config-based hooks".
>
> I think I owe another review, but as always with these topics, I wrote a
> lot of the code so I'm not sure how much I can really help. Other eyes
> appreciated.

It's quite a bit of work, but one bit of valuable independent
verification would be to take your version of the eventual patches that
you've got and try to keep a version rebased on top of these
submissions.

As a spoiler my version is (you'll need gitster's and my github
remotes):
    
    git range-diff \
        gitster/ab/config-based-hooks-base..gitster/es/config-based-hooks \
        avar/es-avar/config-based-hooks-7..avar/avar-nasamuffin/config-based-hooks-restart-5

I.e. that's the difference between "your"
https://lore.kernel.org/git/cover-v4-0.5-00000000000-20210909T122802Z-avarab@gmail.com/
and what I've currently got. I was intending to find the last version
you'd submitted, but I either missed it or I'd have to apply it from the
ML.

In any case, I've found that when I've made changes to the re-rolled
"base" topic based on on-list feedback that needing to roll those
changes forward can inform whether changes in the "base" are good ideas
or not. I.e. whether subsequent changes on top are just cosmetic, or
would require rewrites or a changed design.

That range-diff above doesn't represent any sort of "good" or ready
state, I've just been making the bare minimal set of changes to keep the
"really config-based" topic compiling and passing tests.

>> * es/superproject-aware-submodules (2021-11-04) 4 commits
>>  - submodule: record superproject gitdir during 'update'
>>  - submodule: record superproject gitdir during absorbgitdirs
>>  - introduce submodule.superprojectGitDir record
>>  - t7400-submodule-basic: modernize inspect() helper
>> 
>>  A configuration variable in a submodule points at the location of
>>  the superproject it is bound to (RFC).
>
> To summarize the discussion from here: Ævar suggested this topic might
> not be necessary anymore, and that we should rely on in-process
> discovery of the superproject's gitdir. However, after some more
> thought, I think it's valuable to strive for a definitive way to tell
> "yes, I am a submodule" - and I'd like for this topic to be it. I'm
> planning a reroll (and an explanation in the cover letter), and to drop
> language referring to that as a "cache" (because it isn't a cheap
> version of an operation the submodule would be doing otherwise). I will
> also add another patch to demonstrate how we can use that new
> information as a point of truth, instead of a performance shim.

Hopefully it's clear from the feedback I had there, but my opinion in
this topic is not that I don't think it's unnecessary, I honestly don't
know enough about submodules to say.

Rather that per [1] and [2] for someone who's got that ignorance on the
topic it would be really helpful if we clearly delineate what's a cache
and what's changed behavior.

And even if something is much slower non-cached having a canonical "slow
but correct" path is really valuable, because it may be the case that we
can drop (parts of?) the caching in the future/soon/now if we take the
*.sh<->*.c boundary out of the equation, and at that point being able to
test against the "canonical but slow" version is really helpful.

Especially as a "canonical cache" always runs the risk of what was
assumed to be mere caching becoming changed semantics inadvertently, and
then we'd be stuck with it. Maybe it's good that we'd be stuck with it
(the "point of truth" you mention above), but then let's go into that
behavior change with eyes wide open

1. https://lore.kernel.org/git/211109.86r1bqdsmu.gmgdl@evledraar.gmail.com/
2. https://lore.kernel.org/git/211109.86v912dtfw.gmgdl@evledraar.gmail.com/