commit fab4e18432bae0f11a9ed3d5c682c2442d2ef007 breaks pull request updates

[Thomas De Schampheleire]

Hi Mads,

On Tue, Jan 27, 2015 at 10:55 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 01/27/2015 04:04 PM, Thomas De Schampheleire wrote:
>>
>> Hi Mads,
>>
>> I just updated to the latest tip of kallithea, and now see that
>> updating a pull request with a new descendant does not work.
>> Kallithea detects there is a new descendant (text 'This pull request
>> can be updated with changes on default:') is shown, but no commits are
>> shown.
>>
>> In commit fab4e18432bae0f11a9ed3d5c682c2442d2ef007, you added:
>>
>> show = set(org_scm_instance._repo.revs('::%ld & !::%s & !::%s',
>> avail_revs, revs[0], c.a_branch_name))
>>
>> but the c.a_branch_name is 'default' in my case, while the avail_revs
>> are also on default. This causes anything on the default branch to be
>> ignored.
>>
>> If I remove the last '& !::default' clause, I get the expected list.
>>
>> Could you clarify the reason for this extra clause, what branch you
>> expect there?
>> I assume that just removing the clause does not match your own use case?
>
>
> I'm sorry about that. Yes, it works just fine for our use where we abuse
> named branches for feature branches ;-)
>
> I guess your case is that you have server side forks and don't use named
> branches?

Correct.

>
> That point with this change is that a PR is a preview of a merge. The
> preview is a diff from the common ancestor. But when it is merged, it will
> usually not be merged to the ancestor but to the head of the target branch.
> It is thus not relevant to update to or show anything that already is an
> ancestor of the target branch.
>
> Will
> https://bitbucket.org/Unity-Technologies/kallithea/commits/6cb95b08cc0b27a61b5df0cc0cec145e8e987e9c
> fix your case?
>

Yes, I tested and it does. Please push the fix to kallithea central.

> I can promise that your use case will keep working in the future if you add
> test coverage for it ;-)

Is there any coverage already related to pull requests and possibly
pull request updates? I don't see any.
It would indeed be much better to have all this tested, but without a
good example I'm a bit at lost here.

Thanks,
Thomas