[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