0.4.0: git repo is detected as False

Issue #342 new
Edmund Wong created an issue

While trying to figure out why Kallithea was ignoring the pushes (but still allowing said pushes to the repo) and not showing the changesets in the UI, I looked at the debug logs and came across something which struck me as strange.

2019-04-03 01:10:57.903 DEBUG [kallithea.lib.middleware.simplegit] pathinfo: /infrastructure/testgit2 detected as Git False
2019-04-03 01:10:57.903 DEBUG [kallithea.lib.middleware.simplehg] pathinfo: /infrastructure/testgit2 detected as Mercurial False

infrastructure/testgit2 (a new git repo) isn't being detected as a Git repo.

Comments (18)

  1. Edmund Wong reporter

    In kallithea/lib/middleware/simplegit.py:

    def is_git(environ):
        path_info = environ['PATH_INFO']
        log.debug('pathinfo: path_info is %s' % path_info)
        isgit_path = GIT_PROTO_PAT.match(path_info)
        log.debug('pathinfo: %s detected as Git %s',
            path_info, isgit_path is not None
        )
        return isgit_path
    

    path_info is /infrastructure/testgit2.

    Looking at the re.compile line:

    GIT_PROTO_PAT = re.compile(r'^/(.+)/(info/refs|git-upload-pack|git-receive-pack)')
    

    the path_info clearly (that is, if I understand the regex), it's checking to see if info/refs or git-upload-pack or git-receive-pack is at the end of it.

    and since it doesn't, it doesn't detect it as a git repo.

    I think I've missed the point of that regex. That or something was supposed to return the path_info (that contains the /info/refs part) but didn't.

  2. Edmund Wong reporter

    Actually.. my mistake in understanding the code. Apparently the two simple* detections don't work, even for Mercurial repos. This means that the git detection is sometime after that.

  3. Edmund Wong reporter

    Sorry.. apparently this is another problem with the system's package version. Kallithea expects a git version that supports '-c'. v1.7 (which was installed on this system from the packages) doesn't know anything about '-c'. (This is from line 118.:

      _copts = ['-c', 'core.quotepath=false', ]
    

    My apologies for the noise.

  4. Mads Kiilerich

    I would expect the logs to show the output from

        req_ver = StrictVersion('1.7.4')
    
        log.debug('Git executable: "%s" version %s detected: %s',
                  settings.GIT_EXECUTABLE_PATH, ver, stdout)
        if stderr:
            log.warning('Error detecting git version: %r', stderr)
        elif ver < req_ver:
            log.warning('Kallithea detected git version %s, which is too old '
                        'for the system to function properly. '
                        'Please upgrade to version %s or later.' % (ver, req_ver))
    
  5. Thomas De Schampheleire

    @Edmund Wong Please confirm whether you see this warning message in your log (while you had git v1.7).

    If that is the case, I would classify this issue #342 as a user error: I don't see what we can do more. Making it a blocking error is not desired because some users may not need git at all.

  6. Mads Kiilerich

    But we could perhaps also help the user by complaining more loud and clear, for example when kallithea-cli is invoked?

  7. Edmund Wong reporter

    @Thomas De Schampheleire Yes. Please do classify it as user error. It does show the warning message.. and I missed it. It was only when I looked through the long apache logs does it show. My humblest apologies.

    @Mads Kiilerich That would actually help a lot. As it would be the first thing a user sees when setting it up instead of finding the error in a long log.

  8. Thomas De Schampheleire

    Perhaps we could add and document a new kallithea-cli command, like 'systemcheck' that would verify certain things, like the version of git and maybe others. Same command could perhaps be used as a command-line alternative to the 'system info' tab in the Admin UI, i.e. listing the versions of tools used.

    It seems cleaner to me than adding unrelated checks to exiting commands, like config-create, db-create, etc.

  9. Mads Kiilerich

    That would be something like hg debuginstall.

    The problem is that those with problems often not will run that command. We need something that really is thrown in the face of people and give them a hint about what is wrong or where to look.

    One alternative could be to stop the server process after logging such severe errors.

  10. Thomas De Schampheleire

    But the error is only a problem if you actually use git repos. Right now the git backend can be disabled by editing some python file, but not by editing just the ini file.

    Stopping the server when git is too old could be done if we tell users to explicitly disable git if they want to proceed without updating git itself.

    For gists git is also used, I don't know if it has the same requirements.

    An other alternative is to require a "i know what I'm doing" setting to allow continuing with too old git.

  11. Mads Kiilerich

    We could have that through using existing knobs: Fail badly if git is too old, and people can specify another / non-existing git to work around it.

  12. Thomas De Schampheleire

    Like this?

    diff --git a/kallithea/lib/utils.py b/kallithea/lib/utils.py
    --- a/kallithea/lib/utils.py
    +++ b/kallithea/lib/utils.py
    @@ -606,7 +606,7 @@ def check_git_version():
         if stderr:
             log.warning('Error detecting git version: %r', stderr)
         elif ver < req_ver:
    -        log.warning('Kallithea detected git version %s, which is too old '
    +        raise Exception('Kallithea detected git version %s, which is too old '
                         'for the system to function properly. '
                         'Please upgrade to version %s or later.' % (ver, req_ver))
         return ver
    
  13. Mads Kiilerich

    Yes, it seems to me like that would be better (and very simple and low risk).

    But perhaps also change the message to say something like "Please upgrade the Git configured in git_path (${config.git_path}) to yada yada ... or make it not point at an old Git if you don't care about Git support."

    What's your thoughts about pros and cons about that?

  14. Mads Kiilerich

    But pointing at a non-existing git should perhaps ideally have the same effect as the mentioned "the git backend can be disabled by editing some python file"?

    I don't know if that is feasible or a good idea, but it seems like a potential option ...

  15. Thomas De Schampheleire

    It would then actually be even better to automatically remove 'git' from BACKENDS (kallithea/init.py) if the version is not suitable. Then either the user has suitable git binary, and git can be used. Or not, and then git is disabled. The 'log.warning' should then perhaps be 'log.error' (but not an exception).

  16. Mads Kiilerich

    Right. And then log an error whenever someone try to access a git repo without having a suitable git available.

  17. Mads Kiilerich

    It is only a matter of more clear error reporting. There is a workaround, so it is not essential. Also, the fix might be disruptive, and I think it will be problematic to introduce it in a minor update. I thus suggest doing “fail on error” on default branch.

  18. Log in to comment