FYI: I did the followup as D11030 because I found myself with a few minutes.

Could this be an attr.s instead of a bespoke class with slots?

In D10770#165223, @marmoute wrote:

In D10770#165221, @durin42 wrote:

In D10770#164629, @marmoute wrote:

In D10770#164628, @martinvonz wrote:

In D10770#164626, @marmoute wrote:

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>

Okay, so my assertion is correct and Python behavior is… creative. Maybe we should build a pycompat wrapper for struct.format access ?

AFAICT your assertion was wrong: the struct module just isn't preserving the input type at all, and always emits str on 3.7+, but does bytes on 3.6. So we're not mixing bytes/str input types to struct, but instead struct on 3.6 is ninja-converting back to bytes "for" us.

Yep, this is a typo on my side (typing is hard) (is right because isn't right)

A pycompat wrapper would make sense for this if we see more than one case of this. Is that likely?

It feels wrong to me to have this kind of compatibility weirdness crawl into normal code. I though more about it and in this case I think the simplest would probably to have the format for INDEX_HEADER in a INDEX_HEADER _FMT constant and concatenate with that.

In D10770#164629, @marmoute wrote:

In D10770#164628, @martinvonz wrote:

In D10770#164626, @marmoute wrote:

Looks like this is happening because of an inconsistency betwen the fact we passe a binary 'struct' for INDEX_HEADER and a sysstr for the extra once. I would rather fix this by making them consistent.

I'm not sure I understand. Can you show me how?

$ python3.6 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'bytes'>
$ python3.6 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'bytes'>
$ python3.7 -c 'import struct; print(type(struct.Struct(b"I").format))'
<class 'str'>
$ python3.7 -c 'import struct; print(type(struct.Struct("I").format))'
<class 'str'>

Okay, so my assertion is correct and Python behavior is… creative. Maybe we should build a pycompat wrapper for struct.format access ?

In D10538#160330, @spectral wrote:

I think that the worst that can happen with this change is:
a. clang-format makes a backwards-incompatible change

This looks good to me too, but needs rebased.

(+ infra list, some folks that I think volunteered in the past)

I'm getting conflicts in the hghave part, but otherwise LGTM

Wait, I'm dumb. I just marked this as superceded by itself?

superceded by D10673

This looks good, but we need an email address in the username - can you add one and re-upload the change?

Thanks! This needed some tweaking on the commit message to line up with our format (which is checked by the test suite, but I've edited in-flight and am pushing now.)

It looks superficially correct - I didn't give it a detailed
inspection, but I trust you and Valentin on this.

Fails tests, eg:

--- tests/test-push-checkheads-unpushed-D7.t
+++ tests/test-push-checkheads-unpushed-D7.t.err
@@ -65,27 +65,16 @@
   $ mkcommit C0
   created new head
   $ hg debugobsolete `getid "desc(A0)"` `getid "desc(A1)"`
-  1 new obsolescence markers
-  obsoleted 1 changesets
+  abort: cannot import name 'noderev' from 'mercurial.node' (/tmp/hgtests.5j0okzgk/install/lib/python/mercurial/node.py)
+  abort: cannot import name 'noderev' from 'mercurial.node' (/tmp/hgtests.5j0okzgk/install/lib/python/mercurial/node.py)
   $ hg debugobsolete `getid "desc(A1)"` `getid "desc(A2)"`
-  1 new obsolescence markers
-  obsoleted 1 changesets
+  abort: cannot import name 'noderev' from 'mercurial.node' (/tmp/hgtests.5j0okzgk/install/lib/python/mercurial/node.py)
+  abort: cannot import name 'noderev' from 'mercurial.node' (/tmp/hgtests.5j0okzgk/install/lib/python/mercurial/node.py)
   $ hg debugobsolete --record-parents `getid "desc(A2)"`
-  1 new obsolescence markers
-  obsoleted 1 changesets
+  abort: cannot import name 'noderev' from 'mercurial.node' (/tmp/hgtests.5j0okzgk/install/lib/python/mercurial/node.py)
   $ hg log -G --hidden
-  @  0f88766e02d6 (draft): C0
-  |
-  | x  c1f8d089020f (draft): A2
-  |/
-  | x  ba93660aff8d (draft): A1
-  | |
-  | o  74ff5441d343 (draft): B0
-  |/
-  | x  8aaa48160adc (draft): A0
-  |/
-  o  1e4be0697311 (public): root
-
+  abort: cannot import name 'noderev' from 'mercurial.node' (/tmp/hgtests.5j0okzgk/install/lib/python/mercurial/node.py)
+  [255]

I hate to be That Guy but can you replace "simplify" in all these with some boilerplate about "compare revs and not ids because $REASON"? otherwise some enthusiastic architecture astronaut (probably me) is going to come back in the not-too-distant future and make it more generic and phrase things in terms of nodes rather than IDs.

So, I've gone through several of these now, and I realize: why are we moving towards revs and away from nodes? That's the opposite of the refactor I'd expect...

Landing this chain, but it'd be good to see some documentation for it show up soon.

Needs rebased.

Fair enough. This is ready, but needs rebased.

I'm sorry to report this needs rebased

(marking as request changes in that case)

I suspect this test works without network if setuptools is already installed, but I don't care enough to block it on that pedantry.

AFAICT this should be fine, but I'll give Yuya a while to catch problems.

I'm still confused here.

This still requires changes to resolve my comments.

I'm not happy about this footgun option, but see why we need it.