bug: 'unknown revision' when opening a pull request

[Thomas De Schampheleire]

Hi Mads,

On Fri, Mar 6, 2015 at 4:05 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 03/03/2015 10:49 PM, Thomas De Schampheleire wrote:
>>
>> Hi Mads,
>>
>> As discussed on IRC, there is currently a bug in the pullrequest
>> display code that can trigger for pullrequests between different
>> repositories, when the head of the destination repo branch is not
>> available in the originating repository.
>>
>> You already provided some code suggestion (below), which removes the
>> 500 server error, but there is one remaining issue: the block that
>> shows changesets that can be used to update the pullrequest is not
>> correct: in my current case it detects that there are at least one
>> candidate changesets for update, but the 'show' set calculated is
>> empty. I assume that the revset used to calculate this set needs some
>> tweaking after having moved to the unionrepo stuff.
>>
>> Could you have a look at that?
>
>
> I don't understand what can go wrong.
>
> avail_revs contains the existing PR head and additional changes based on
> that on the same branch in the source repo.
>
> We hide the PR head (I don't see any reason to subtract its ancestors too -
> that is a minor bug).
>
> We also hide anything that already is an ancestor of the target branch in
> the target repo.
>
> That all seems OK to me.
>
> Can you dig more into which part of it is causing the problem? Are you sure
> the missing update candidate not already is in the target?

Due to other activity in this repo I do not see the issue currently
and thus cannot debug further. I tried a few scenarios on a test repo
but could not reproduce the issue yet.
Nevertheless, I'm sure that I saw a problem.
I'm not 100% sure that the candidate was not already in the target,
but even then there should be no mismatch between the text 'there are
updates available' and the list of shown updates.
Currently, the text is displayed on the condition len(avail_revs) > 1,
while the list of shown items is based on another selection. There
must be cases (as I have seen one) where both do not match, which is
not good.

I think that both should be lined up, but my understanding of this
piece of code is not sufficient to make the actual change.

Do you agree that the different revsets for the string and the display
is not good? Could you improve this?

Meanwhile, I suggest you already push your fix with unionrepo. If I
see the issue again I will investigate further.

>
> ("candidate" seems like a good term that we perhaps should use instead of
> "available" in code and UI.)

Yes, sounds good.
Also 'revs[0]' is not clearly indicated as current PR head, this could
be commented on or an intermediate variable introduced.

Thanks,
Thomas