On 4/6/19 9:22 PM, Thomas De Schampheleire wrote:
El jue., 4 abr. 2019 a las 0:41, Mads Kiilerich (<m...@kiilerich.com
<mailto:m...@kiilerich.com>>) escribió:
> And pushing to such repo would result in following client errors:
>
> $ git push
> Password for 'http://user@localhost:5050':
> Enumerating objects: 3, done.
> Counting objects: 100% (3/3), done.
> Writing objects: 100% (3/3), 241 bytes | 241.00 KiB/s, done.
> Total 3 (delta 0), reused 0 (delta 0)
> remote: unable to load configuration from hooks/pre-receive
> To http://localhost:5050/gitrepo-new
> ! [remote rejected] master -> master (pre-receive hook
declined)
> error: failed to push some refs to
'http://user@localhost:5050/gitrepo-new'
Can we somehow make it show a more useful error message in this
case? I
guess not - none of our code is executed ...
Actually, there is some of our code executed: SimpleGit. But, at that
point you don't know that the git hook call is going to fail.
First: Since we install Git hooks in the file system, they will also be
invoked if someone push directly. That may or may not be a feature, but
we have to handle it.
But yes, usually, SimpleGit will invoke the `git` command which then
invoke the hooks.
Also, I have significant refactorings in this area lined up, preparing
for clean hook handling, also for ssh. Our first goal on the stable
branch should be to fix regressions - perhaps by recommending workarounds.
> The approach taken by this commit is:
FWIW, this:
> - Introduce a new configuration setting: hook_python_path, and
use its value
> as the Python interpreter to use in git hooks.
> - If the value is absent or empty, fall back to the current logic
> 'sys.executable' or if that is also invalid, 'python2'.
and this:
> - Let `kallithea-cli config-create` fill in its own interpreter
as default
> value, which should be valid as long as the virtual
environment is not
> moved/replaced.
*could* be separate "milestones" and separate changesets. But no big
deal. Split or keep together as you like.
Yes, I considered it, but got a bit stuck with the comments referring
to code that does not yet exist.
I'll think about it a second time.
Yes, the first changeset will need comments about the potential, and the
second changeset might update these comments. But the the phrasing and
change of the comments might help to be aware what actually is changed
and why ... and how it ideally should be done.
> The main downside of this approach is its fragility in case a
new virtualenv
> is used. Previously, the configuration file was the same
regardless of
> virtualenv, but this is no longer true after this patch.
But also, it is crucial that the *right* virtualenv is used, also
if Git
should be invoked directly, outside any virtual env. It is thus
inherent
that the new virtualenv *has* to be written to the hooks somehow. The
previous use of same 'python2' was wrong and fragile. I thus don't
think
the new way is more fragile. Explicit expectations and failing
early is
*less* fragile ;-)
In fact, my understanding was not fully complete before: when users
push to a git repo, they actually communicate with our Git middleware,
which itself uses dulwich. All this should normally happen inside the
virtualenv (if used). Only when the hooks get called, there is a
decoupling into another process. (I am theorizing here, not explicitly
checked)
So if that is correct, then I wonder if we could not avoid the decoupling.
We are already handling pull hooks in
kallithea.lib.middleware.simplegit._handle_githooks .
Can we not handle push hooks pre-receive/post-receive in the same
place? In this case, we are definitely in the right environment, we
are and stay within Python. I guess it will also simplify the
situation for Windows.
Am I overlooking something?
We kind of *have* to run something inside the hook. That is the only way
to know exactly what is pushed (or pulled). If we want branch access
control, it will(!?) also have to run inside hooks.
We can perhaps revisit after landing my cleanups and see if we can find
a fundamentally new way to do it.
Thanks for these comments, I generally agree.
But before going further, I'd like to hear your opinion on the idea of
calling the hooks directly from our own code (see above).
Summarizing: I don't think that is feasible. But if it is, it will not
be simple. Not suitable for the stable branch.
/Mads
_______________________________________________
kallithea-general mailing list
kallithea-general@sfconservancy.org
https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
