[PATCH 1 of 5 STABLE REGRESSION] exchange: backout changeset c26335fa4225

Manuel Jacob me at manueljacob.de
Fri Jul 24 12:50:07 UTC 2020


On 2020-07-24 14:31, Pierre-Yves David wrote:
> On 7/24/20 2:25 PM, Manuel Jacob wrote:
>> On 2020-07-24 14:07, Pierre-Yves David wrote:
>>> On 7/24/20 8:19 AM, Manuel Jacob wrote:
>>>> Overall, I’m fine to backout this for now and solve the problem 
>>>> during the Mercurial 5.6 development cycle.
>>> 
>>> Thanks, let do that.
>>> 
>>>> I still think that this check is not the right place for preventing 
>>>> the cases you were raising. The message refers to changesets 
>>>> included in the push, which I read as "transferred to the server", 
>>>> and including non-missing changesets in the check causes e.g. 
>>>> issue6372. For situations where the changesets are already on the 
>>>> server, the old check missed many cases (the new doesn’t attempt to 
>>>> catch them). If you find it important to retain the shotgun that 
>>>> misses part of the target and hits the wall behind it (to use a 
>>>> different metaphor than the holey safety net ;)), backing this out 
>>>> and taking a bit more time to fix it during the 5.6 development 
>>>> cycle seems reasonable.
>>> 
>>> Yeah the existing code was very ra, it is some very old code setup
>>> quickly when there was more pressing matters. The assumption was 
>>> "with
>>> a simple but wide net, we can probably catch most of the problems, 
>>> but
>>> the problem space was not really fully mapped and tested at that time
>>> (because other focus) hence the various flaw. Lets fix it in 5.6, 
>>> even
>>> if we dont handle every corner case having a good map of the problem
>>> space would be very helpful.
>> 
>> A good start would be to define what are the limits of what we can 
>> detect. If two clients are involved, there are situations where we 
>> can’t really detect anything without server support. Example (I hope 
>> my email client doesn’t mangle it, otherwise see 
>> https://dpaste.com/E8J72RMLT):
>> 
>>    $ cat >> $HGRCPATH << EOF
>>    > [phases]
>>    > publish = False
>>    > [experimental]
>>    > evolution = all
>>    > EOF
>> 
>>    $ hg init server
>>    $ cd server
>>    $ echo root > root; hg add root; hg ci -m root
>>    $ hg phase --public
>>    $ echo a > a; hg add a; hg ci -m a
>>    $ cd ..
>>    $ hg clone server client1
>>    updating to branch default
>>    2 files updated, 0 files merged, 0 files removed, 0 files 
>> unresolved
>>    $ hg clone server client2
>>    updating to branch default
>>    2 files updated, 0 files merged, 0 files removed, 0 files 
>> unresolved
>>    $ cd client1
>>    $ hg branch foo -r 1
>>    changed branch on 1 changesets
>>    $ hg push --new-branch
>>    pushing to $TESTTMP/server
>>    searching for changes
>>    adding changesets
>>    adding manifests
>>    adding file changes
>>    added 1 changesets with 0 changes to 1 files (+1 heads)
>>    1 new obsolescence markers
>>    obsoleted 1 changesets
>>    $ cd ../client2
>>    $ echo b > b; hg add b; hg ci -m b
>>    $ hg push
>>    pushing to $TESTTMP/server
>>    searching for changes
>>    adding changesets
>>    adding manifests
>>    adding file changes
>>    added 1 changesets with 1 changes to 1 files
>>    1 new orphan changesets
>> 
>> Client 1 amends A and client 2 adds B on top of A. At each client, it 
>> looks good, but when both push, we have an orphan on the server. 
>> Should the server reject this if the client doesn’t explicitly force 
>> it?
> 
> The second push will create the orphan, we should detect the situation
> at this time. (and point it to the user instead of letting him push
> silently).

At client side, we can’t detect this unless we pull first. Should the 
server detect and abort?

>>>> Backing out will reintroduce the bug that non-head outgoing 
>>>> content-divergent and phase-divergent changesets are not detected. I 
>>>> can send another patch that checks them.
>>> 
>>> That seems like a good idea.
>>> 
>>>> About the tests in the following patches: I’ve put some small 
>>>> modifications on top in 
>>>> https://foss.heptapod.net/octobus/mercurial-devel/-/commits/topic/stable/push-obscheck--small-modifications. 
>>>> If you like them, you can absorb them in your patches, or prune them 
>>>> otherwise.
>>> 
>>> All three are great, got ahead in folding them at the appropriate 
>>> location.
>> 
>> If you want them to be committed like this, you’ll probably need to 
>> send a V2. ;)
> 
> Let me know once they are folded. I'll email the V2

I thought you folded it already. I just pushed your changesets with my 
changes included. 
(https://foss.heptapod.net/octobus/mercurial-devel/-/commit/286cfe4ac350a4d003743390b0c1cf13caa2e2e0 
and ancestors)

>> 
>>>> 
>>>> The current test structure is:
>>>> 
>>>> * case 1: test pushing
>>>> * case 2: test pushing
>>>> * case 1: test publishing
>>>> * case 2: test publishing
>>>> 
>>>> An alternative structure would make it easier to add more cases 
>>>> later:
>>>> 
>>>> * case 1: test pushing
>>>> * case 1: test publishing
>>>> * case 2: test pushing
>>>> * case 2: test publishing
>>> 
>>> I considered each approach. I think the "publishing" check will be a
>>> different layer of check eventually with different sub case so it 
>>> make
>>> sense to group them. (but I don't have a super strong opinion on
>>> that).
>> 
>> Yes, leaving the structure as-is might be a good idea. We can 
>> relatively easily add a separate check for "do we publish obsolete 
>> changesets?", as we know both the published changesets and the 
>> obsolete changesets. If it’s a simple effective check, we don’t have 
>> to test it for every case where we test the obsolete / unstable check.
> 
> Maybe it would make sense to factor out the setup more so that each
> run is more independant (but that comes with extra runtime cost)

I don’t have a strong opinion on that. The current approach seems good 
enough.



More information about the Mercurial-devel mailing list