<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Apr 7, 2019, 00:25 Mads Kiilerich <<a href="mailto:mads@kiilerich.com">mads@kiilerich.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div class="m_5835952659545531946moz-cite-prefix">On 4/6/19 9:22 PM, Thomas De
Schampheleire wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div dir="ltr"><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">El jue., 4 abr. 2019 a las
0:41, Mads Kiilerich (<<a href="mailto:mads@kiilerich.com" target="_blank" rel="noreferrer">mads@kiilerich.com</a>>)
escribió:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> And pushing to such repo would result in following
client errors:<br>
><br>
> $ git push<br>
> Password for '<a class="m_5835952659545531946moz-txt-link-freetext" href="http://user@localhost:5050" target="_blank" rel="noreferrer">http://user@localhost:5050</a>':<br>
> Enumerating objects: 3, done.<br>
> Counting objects: 100% (3/3), done.<br>
> Writing objects: 100% (3/3), 241 bytes | 241.00
KiB/s, done.<br>
> Total 3 (delta 0), reused 0 (delta 0)<br>
> remote: unable to load configuration from
hooks/pre-receive<br>
> To <a href="http://localhost:5050/gitrepo-new" rel="noreferrer noreferrer" target="_blank">http://localhost:5050/gitrepo-new</a><br>
> ! [remote rejected] master -> master
(pre-receive hook declined)<br>
> error: failed to push some refs to
'<a class="m_5835952659545531946moz-txt-link-freetext" href="http://user@localhost:5050/gitrepo-new" target="_blank" rel="noreferrer">http://user@localhost:5050/gitrepo-new</a>'<br>
<br>
<br>
Can we somehow make it show a more useful error message in
this case? I <br>
guess not - none of our code is executed ...<br>
</blockquote>
<div><br>
</div>
<div>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.<br>
</div>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>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.</p></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I don't think this is a valid argument: for mercurial repos the hooks are not installed on the file system. Direct pushing is not supported.</div><div dir="auto"><br></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF">
<p>But yes, usually, SimpleGit will invoke the `git` command which
then invoke the hooks.</p>
<p>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.</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> The approach taken by this commit is:<br>
<br>
<br>
FWIW, this:<br>
<br>
> - Introduce a new configuration setting:
hook_python_path, and use its value<br>
> as the Python interpreter to use in git hooks.<br>
> - If the value is absent or empty, fall back to the
current logic<br>
> 'sys.executable' or if that is also invalid,
'python2'.<br>
<br>
<br>
and this:<br>
<br>
<br>
> - Let `kallithea-cli config-create` fill in its own
interpreter as default<br>
> value, which should be valid as long as the
virtual environment is not<br>
> moved/replaced.<br>
<br>
<br>
*could* be separate "milestones" and separate changesets.
But no big <br>
deal. Split or keep together as you like.<br>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>Yes, I considered it, but got a bit stuck with the
comments referring to code that does not yet exist.</div>
<div>I'll think about it a second time.<br>
</div>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>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.</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> The main downside of this approach is its fragility
in case a new virtualenv<br>
> is used. Previously, the configuration file was the
same regardless of<br>
> virtualenv, but this is no longer true after this
patch.<br>
<br>
<br>
But also, it is crucial that the *right* virtualenv is
used, also if Git <br>
should be invoked directly, outside any virtual env. It is
thus inherent <br>
that the new virtualenv *has* to be written to the hooks
somehow. The <br>
previous use of same 'python2' was wrong and fragile. I
thus don't think <br>
the new way is more fragile. Explicit expectations and
failing early is <br>
*less* fragile ;-)<br>
</blockquote>
<div><br>
</div>
<div>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)<br>
</div>
<div> </div>
<div>So if that is correct, then I wonder if we could not
avoid the decoupling.</div>
<div>We are already handling pull hooks in
kallithea.lib.middleware.simplegit._handle_githooks .</div>
<div>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.</div>
<div><br>
</div>
<div>Am I overlooking something?<br>
</div>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>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.<br></p></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Knowing what was pushed and what not is a more convincing argument. But how do we know this in mercurial? Does mercurial tell us, or do we find out? Is it impossible/fragile to do the same in git?</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><p>
</p>
<p>We can perhaps revisit after landing my cleanups and see if we
can find a fundamentally new way to do it.</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>Thanks for these comments, I generally agree.</div>
<div><br>
</div>
<div>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).</div>
</div>
</div>
</div>
</blockquote>
<p><br>
</p>
<p>Summarizing: I don't think that is feasible. But if it is, it
will not be simple. Not suitable for the stable branch.</p>
<p></p></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Ok. But when deciding what to do on the stable branch, I think it makes sense to have an idea what we'll do for default. I think we should avoid introducing user visible changes like ini settings if they will not apply on default.</div><div dir="auto"><br></div><div dir="auto">/Thomas</div></div>