Kallithea issues archive

Issue #153: Git hooks are not properly verified when doing "Remap and Rescan"

Reported by: Branko Majic
State: resolved
Created on: 2015-08-26 07:37
Updated on: 2015-09-03 20:55

Description

When performing "remap and rescan" with "Install Git hooks" option, Kallithea will not update the hooks "pre-receive" or "post-receive" if they already exist.

I think the issue is mainly relevant for people upgrading/migrating from Rhodecode, since it will probably keep the Rhodecode hooks in place, making it impossible to push changes (old code would be called) to Git repository.

Reproduction steps:

  1. Create a new Git repository through Kallithea.

  2. Go to the repository, and replace hooks/pre-receive and hooks/post-receive with empty files.

  3. Log-in as admin, and open page "Settings" -> "Remap and Rescan".

  4. Turn on option "Install Git hooks".

  5. Click on the "Rescan repositories" button.

Expected results:

  1. Empty git hook files are replaced with Kalliethea's hooks.

Actual results:

  1. Empty git hook files are left in place.

Workaround:

  1. Remove all the git hooks by hand before starting the rescan.

Additional information:

Not sure if Kallithea should unconditionally replace the hooks or not. Some people might want to do tweaks on these git hooks, and would not like them to be overwritten perhaps.

Attachments

Comments

Comment by Mads Kiilerich, on 2015-08-26 14:31

Can you try to debug that? kallithea/lib/utils.py repo2db_mapper should be calling kallithea/model/scm.py install_git_hook . Why doesn't that work? Did the hook file name change?

Comment by Branko Majic, on 2015-08-26 19:16

Ok, I've had a small look at the code you suggested

So, the function install_git_hook has an option force_create=False. From what I can tell, there is no way to select the force_create=True "option" from any other part of code.

The current implemented logic in the method is:

  1. If hook does not exist, create it.
  2. If hook already exists, and it is a Kallithea hook, replace hook.
  3. If hook already exists, but it is not a Kallithea hook, and force_create is True, replace hook.
  4. If hook already exists, but it is not a Kallithea hook, and force_create is False, don't touch it.

So:

  1. Is this the intended behaviour?
  2. If not, are people actively deploying their own custom hooks instead of default one?
  3. Would it be useful to have an option in GUI to overwrite hooks?
  4. The simplest fix for upgrade scenario could be to have instructions for removing Rhodecode hooks by hand during upgrade.

Comment by Mads Kiilerich, on 2015-08-26 23:48

  1. I don't know. We inherited this code when forking. By know I think you are the expert in this area. I want you to succeed but my experience is in Mercurial and I trust you (and the people who will review the changes you propose).
  2. I guess that if people host their repos with Kallithea, that is their primary purpose and it would be fine to remove other hooks. But perhaps make UI options for "update kallithea hoooks" and "replace all existing hoks".
  3. I guess so ... if you think so?
  4. That would also be fine. Can you suggest a patch for the documentation (README)? We plan to drop backwards DB compatibility "soon" and might drop the 'upgrade from RhodeCode' step soon ... but people will be able to use 0.3 as a stepping stone so improved documentation or update process would be a great help.

Comment by Branko Majic, on 2015-08-27 17:56

I have thought about this some more, and have another idea for a fix. Something along the lines of:

  1. If hook does not exist, it is a Kallithea hook, or it is a Rhodecode hook, replace the hook.
  2. If hook exists looks to be a custom hook, do not replace it.
  3. Notify user about any hooks that did not get replaced that manual action may be required.

This way upgrades would work, existing behaviour will remain, custom hooks won't be touched, and user will get more information if the requested operation (git hook replacement) has not been carried out completely.

Comment by Mads Kiilerich, on 2015-08-27 22:55

It seems reasonable, but I guess such heuristics can also be error prone. Instead, I think I also would consider exposing the 'replace existing git hooks' flag as a checkbox in the UI.

If you try to come up with a patch for it, you get to decide which way the problem should be solved ;-)

Comment by Mads Kiilerich, on 2015-09-02 18:46

Branko provided a patch

Comment by Branko Majic, on 2015-09-03 20:51

Can I reopen this one for adding some tests while we're at it?

Or should I create a new one?

Comment by Mads Kiilerich, on 2015-09-03 20:55

I suggest creating a new PR.