Incorrect detection of already existing key?

[rchady]

I was stepping through in the debugger and came across what I believe is a bug. In particular, in git_hosting.rb:973 it uses index() to determine if a key is already in the old_keynames. However, if the key happens to be the first item in old_keynames, index() will return 0... and that unless block gets run. I believe include? would be the better method to use here?

[n-rodriguez]

Thanks for the feedback!
Interesting, I'm gonna take a look ;)

By the way, if you wanna do debug, you can test the new plugin version (http://n-rodriguez.github.io/redmine_git_hosting/)
It includes new features such as : JQuery Modal Box, Git mailing lists, Sidekiq workers for async jobs and some more :)

[rchady]

I took a peak at that version the other day, but some of the added requirements prevented me from switching to it during my recent production deploy. Wish there was a way of doing the async jobs without redis, etc.

I forgot to mention, another possible bug in git_hosting.rb where you call Dir.foreach(keydir). The issue being you do not skip over '.', and '..'. You are prevented from having a disastrous outcome because of the call to GitolitePublicKey#ident_to_user_token, but in general I would sanitize the directory entries anyhow!

[n-rodriguez]

Thanks again for your advices, I'm gonna fix them (hopefully ^^)

Btw, what requirement prevented you from switching? Redis server? If so, why?

Wish there was a way of doing the async jobs without redis

Yes, there are other ways, such as daemonize or Rescue but daemonize does not respect the order of things (very bad in our use case), and for Rescue I wasn't able to do a clean implementation of it in Redmine so the last to try was Sidekiq. Fortunately Gitlab uses Sidekiq so I could get some inspiration in their code to implement it in the plugin :) It was also the occasion of testing the Gitolite gem wich simplifiy the code a lot.

I also read about the 'thread safe' problem, and that's why I forked the Gitolite gem to use the Gitlab's (once again) fork of Grit wich is now thread safe. I've made some modifications on the gem to make it partially thread safe (see here wingrunr21/gitolite#42) and the whole works now for a month without any bug or crash (with Puma and Nginx).

[n-rodriguez]

Fixed in commit ff48c2a