Hi Thomas Thanks for looking into this.
On 20/10/2020 22:12, Thomas De Schampheleire wrote:
El mar., 20 oct. 2020 a las 21:12, Tim Ooms (<tim.o...@aeronomie.be <mailto:tim.o...@aeronomie.be>>) escribió:The goal of the patch is to not break the non-kallithea hook scripts that may be present when doing a rescan and reinstall (after an update for example) and at the same time supporting the execution of third-party hook scripts next to the kallithea ones. When using the force parameter, instead of overwriting the non-kallithea hook script, it renames the script by adding .<date>.<kallithea-version> to it (can off course be something else). The new version of the kallithea hook script will execute all scripts in the hook-directory matching <hook-name>.*.* (should just match previous pattern). This way, it executes the previously installed hook script and possible other hook scripts the admin added matching the pattern. Thanks for the background.In general, we encourage contributors to write clear commit messages with all necessary info to understand the what/why/how of changes. So info like the above would be great to have in such commit message.
I thought it too big for a commit message. Will do in the future.
For these changes, I think they can be seen as two separate things and should therefore better be two separate patches:- support running additional git hooks in addition to the kallithea one- rename existing git hooks rather than overwriting them, when 'force' is given.
In fact, it can be 3 separate patches: 1) support running additional git hooks 2) rename existing hooks3) keep default given permissions of created hook and add any possible necessary permission bits
I'm not sure if the second part is really necessary: it should normally only occur once, the code adds quite some complexity for such mundane task of renaming a file. Perhaps it could be enough to have clear enough documentation or messages?Let's see what Mads Kiilerich thinks.
For now, it occurs every update of Kallithea or its venv's python. But indeed, if additional hooks are supported, it should occur only once. In my opinion, if you do support additional hooks, you get a safe overwrite by moving for free. If you take out the other commits, log messages and comments, the only stuff that remains for the renaming is:
if other_hook:
moved_hook_file = name based on date and version
os.rename
So I don't think that's much complexity?
For the first part, I can see that it can be useful.See also the comments on following proposed change, for a recent discussion regarding the git hooks and how it is implemented internally:https://kallithea-scm.org/repos/kallithea-incoming/changeset/c63dd3aeacdbbd77349bb2b9100dee5ba0c3fda6
What I thought of while creating the patch is that I would create a single template for pre & post-receive to lower the code duplication. When using a wrapper in venv/bin will the correct python be executed automatically? Otherwise this (still) has to be changed (too). This can simply be done while "copying", no?
-- kind regards, Tim
<<attachment: tim_ooms.vcf>>
_______________________________________________ kallithea-general mailing list kallithea-general@sfconservancy.org https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
