In D11757#180388, @Alphare wrote:

In D11757#180387, @martinvonz wrote:

Until this is done (hopefully this cycle), we're keeping Python 2.

To make sure I understand correctly, "this cycle" here refers to the cycle *after* 6.0? So we're instead hoping that 6.1 will be the last release with support for Python 2?

This patch basically mean that if we fix thg packaging before 6.1 is released, we can drop Python 2. For us developers, that means still supporting Python 2 until this is done. Could be tomorrow (fingers crossed heh), at the end of the cycle, or in Mercurial 7.8 if nothing moves on. As said previously we're trying to make that work currently internally at Octobus, but it's really not something that we have time (or really expertise) for right now unfortunately.

Seems like a good fix, needs rebased

Seems fine, needs rebased.

In D11669#179233, @martinvonz wrote:

Sure, I could send a patch for adding another line to the help text. Do you think that's clear enough? I don't know where else it would go. (We would probably still want to override it internally to highlight it when we roll it out to Google devs.)

In D11669#179228, @martinvonz wrote:

In D11669#179227, @durin42 wrote:

Doing cross review: is there any indication of the direction of history when the UI pops up, or does the user just have to figure it out?

The user will have to figure it out (hopefully they remembered if they set the config option). Internally at Google, I was going to override helplines() (and some other function which I've forgotten now) to make it print a red banner saying if the direction has changed.

In D10893#174783, @marmoute wrote:

I have not forgotten and I meant to get to that last week. Unfortunately some IRL situation came up and I have not be able to spend time on this (and even writing this message took a week longer that I wanted). Sorry for the delay. The second half of ext week should be more "normal" and I hope to get to that at that time.

Doing cross review: is there any indication of the direction of history when the UI pops up, or does the user just have to figure it out?

In D10538#179050, @marmoute wrote:

In D10538#179035, @spectral wrote:

I'm still struggling to identify where my proposed fix makes any of this worse.
Current state: people with a version of clang-format <11, or people with a 'trunk' version of clang-format, don't get their code run through clang-format at all, and send changes that will break CI when they land (unless the issue is caught during review).
Proposed state: that no longer happens for people running a 'trunk' version of clang-format, just those using clang-format <11.

I feel like the proposal is going in the wrong direction because it make a more diverse set of version passe the version checks while I feel like we need to more toward a narrower check. Different formater version tends to produce different results and it seems saner to pin the project to specific versions (that we update from time to time).
For the record, right now we have :

• black: check says ">= 20.8b1", CI uses "20.8b1"
• clang: check says ">= 11", CI uses debian's clang-format-11 package (in practice "Debian clang-format version 11.1.0-++20210622113218+1fdec59bffc1-1~exp1~20210622213839.163")
• rust: check says "nightly-2020-10-04", CI uses "nightly-2020-10-04"

Is my feedback clearer and does it make sense to you? Or do you feel like I missed something?

I think that's a buggy test? @martinvonz and @spectral might have an opinion.

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.