empty directory cause internal error

[luxu1220]

I got a lot of git project under the KLAUS_REPOS_ROOT, but when I add an empty directory
in KLAUS_REPOS_ROOT by mistake, it will cause an internal error and the services are all failed. Is there any option to set so i can make klaus just ignore the bad project so other projects can not be affected? Thank you.

[jonashaag]

Maybe you can fix the issue yourself? Patch welcome.

[guhetier]

Hi,

I just got the same issue, here is the result of some investigation:

When creating the app, all folder in KLAUS_REPOS_ROOT are collected as parameter:

klaus/klaus/contrib/wsgi_autoreloading.py

Line 40 in 62d08c3

[os.path.join(repos_root, x) for x in os.listdir(repos_root)],

Klaus tries to create a dulwich repo for every folder in parameter:

klaus/klaus/__init__.py

Line 20 in 62d08c3

repo_objs = [FancyRepo(path) for path in repo_paths]

Dulwich raise an exception when the folder does not correspond to a git repository:
https://github.com/dulwich/dulwich/blob/458bbb4bdefa9d3fcfbb266f74dc7014ce1fd490/dulwich/repo.py#L871

This exception is currently not handled and cause the internal error.

The error could be fix by handling the exception or by validating the path before, it mainly depends on the vision @jonashaag has about klaus: should it crash when it receive an invalid parameter? Should it try to handle it gracefully?

I will try to propose a fix for the issue if I find some time to set up a dev environment for klaus.

EDIT: Just saw the similar and still open issue (#176)

[jonashaag]

Thanks for investigating!

Actually I think it's fine to simply ignore failing directories (with a warning on the command line) for the autoreloader. But only for the autoreloader, so that if you invoke klaus using the normal command line it crashes for failing directories.

We can either check directories before passing them to make_app (by directly invoking Dulwich), which is easy but maybe not the most elegant. (I really don't care about the elegance bit for the autoreloading code.) Or we can have an option to make_app to somehow return failing repositories so we can warn on the command line, or something along these lines. That probably requires some minor refactoring though.

[guhetier]

Thanks for the answer! I just saw the similar issue #176, and I added a proposition there (ignore hidden folders).
I don't really like calling dullwhich from the autoreloading code. Maybe having an option fail on invalid repository?

[jonashaag]

Maybe having an option fail on invalid repository?

Sounds good, but we'll have to turn this into a logging message without adding actual logging code to the __init__ method (wrong place from architectural point of view).

Suggestions:

1. Allow for passing FancyRepo instances to __init__; this way we can check if the repo's ok beforehand "without using Dulwich"
2. Have an attribute invalid_repos in the Klaus class and partition the repos passed to __init__ into the repos and invalid_repos attributes, and then check the invalid_repos attribute for any entries. Sort of a hack but just a tiny one!

[jonashaag]

I guess the correct way to do this is to add a new .set_repos method to set the .repos attribute that can return a list of failed repos

[guhetier]

I think I prefer your suggestion 1. It if klaus.__init__ only accept FancyRepo instances, it would ensure the instance can be constructed. The FancyRepos could be constructed in make_app and the exception raised on an invalid repo could be either handled there or not depending on a parameter.

The issue I see with a .set_repos method is that its behavior would not be consistent with the __init__ method (one fail on invalid repos, not the other). If we add this method, in my opinion, we should also have __init__ only accept FancyRepo.

I am not very familiar with web applications, let me know if I am wrong somehow!

[jelmer]

I think it would be ideal if directories that can not be opened would show in the web UI, but colored in red (and not clickable). That way you don't have to read the console logs to know that a repository was not picked up.

[jonashaag]

@jelmer That is indeed a very good idea. Maybe put it to the bottom of the index page.

@westonlu @NaoPross @xNephe what do you guys think?

[guhetier]

It would be a nice feature as long as it only display broken git repos only, and does not show other folders: I would not like to have some .ssh, .config or .DS_Store folder shown in red at all time.

Either we need a way to differentiate broken git repos from non git folders or we should allow the user to filter the folder to include (this is already the case with KLAUS_REPOS, for KLAUS_REPOS_ROOT we could add an optional regular expression parameter so that the user can specify which folders to include?).

[jelmer]

It may make sense to follow the common convention of hiding anything that starts with a "." - that would cover @xNephe's examples.

[jonashaag]

Fixed with #240