ocaml-opam / Camelus

Bot posting reports on opam-repository pull-request using a web hook
Other
18 stars 9 forks source link

Removed installable packages fails on renames #30

Open dra27 opened 5 years ago

dra27 commented 5 years ago

Camelus is not reporting the removal of https://github.com/ocaml/opam-repository/pull/14582 genprint.0.1. I wonder if it's because of the rename?

dra27 commented 5 years ago

Actually, looking further (I was also suspicious that GitHub has identified that user as a first time contributor). The user carried out the following steps:

So in this case Camelus has detected the merge-base as being the original merge-base as in the original PR and so genprint.0.1 appears to have been both added and removed in the same PR and is therefore not reported.

Not sure what the best fix would be - probably doing a diff-tree of the merge commit (see for example OCaml's Travis CI scripts which do this).

AltGr commented 5 years ago

Thanks for the investigation! It's interesting because I originally programmed the 'best common ancestor' search myself (which was quite the headache!) ; but since we moved away from ocaml-git (which wasn't stable enough yet at the time), I relied on the git merge-base command instead.

I'd like to check whether my algo did give the better result in this case :) ; it was a bit slow though, but it's planned to get back to ocaml-git at some point and check the performance. When we manage to get the time...

As for testing on a "virtual merge commit", I know that's what CI implementations usually do, but I am not fond of it for several reasons. With a correct ancestor search, we should be OK.

So here we have:

         o---o---2---o---B
        /         \
---o---1---o---o---M---o---A

And we want to test branch B against master A. 1 and 2 are the two candidate merge-bases, but since 1 is an ancestor of 2, clearly 2 should be the chosen one here :thinking:

Testing the diff between 2 and B would have been correct here. Maybe we should just check what arguments exactly we passed to git merge-base...

thomasblanc commented 5 years ago

Just read through merge-base's man page, it seems that 2 should indeed be selected, maybe Github sends some info that Camelus gets wrong.

thomasblanc commented 5 years ago

@dra27 are you sure that the merge-base failed? From looking at the code everything seems find on that end. Maybe it is the rename.

dra27 commented 5 years ago

~@AltGr - I don't think that's correct - what's weird here is that contributor has repeated commits which are already in the branch, so there's no M or 2.~ (oops: got my branches the wrong way around)

dra27 commented 5 years ago

@thomasblanc - I agree, git merge-base is indeed identifying the correct merge-base.

What's really interesting is that it's confused GitHub too which is showing this contributors second PR as "first time contributor"