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

[Manuel Jacob]

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?

>> 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. ;)

>> 
>> 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.