Qt6 compilation branch : FOR DEVEL NOT FOR USE

[tibirna]

I pushed the branch compile-w-qt6 even if it is far from actually compiling. This work appears to be much more onerous than I first estimated. So it will take a while. Any help is welcome.

[hartwork]

Any help is welcome.

From what I can see in branch compile-w-qt6 at the moment…

• QTextCodec needs to be replaced unless Qt5compat is used, idea https://stackoverflow.com/questions/65379825/qtcore-qtextcodec-not-found-in-qt-6#comment128303637_65379825 may help
• The CI needs to install Qt6 packages rather than Qt 5
• The migration to QRegularExpression seems incomplete and either needs completion or use of QRegExp with Qt5compat

@tibirna how can I best help in a way where we don't duplicate work? My preference would be a video call with screensharing but anything that avoids duplication of effort works for me.

CC @a17r

[tibirna]

Hello @hartwork

Thanks for noticing this. And thanks for the offer for help. This is very kind and very much appreciated!

I am only now gathering some experience on migrating to Qt6, so I rely heavily on the Qt documentation and on the compiler output. This lack of previous experience made things more tedious than I hoped.

I tend to think relying on Qt5Compat is akin to kicking the dirt down the row, we'll have to get a complete solution later anyways, so perhaps better now than later.

Issues that I noticed:

• the QTextCodec migration to QStringConverter -- it seems a bit delicate because of heavy relying of the underlying git (informal) spec on codecs, and because of the variable amount of codecs covered by QStringConverter along Qt6 evolution. We'll have to require Qt6.6 from the beginning, I think. Even so, I don't yet have in mind a clear path to a successful conversion for this (without major rewriting).
• QMessageBox and family changed API (I'm not sure I get this right yet)
• QRegExp to QRegularExpression , with a rather different API (and I'm still not sure that all old functionalities that qgit code uses have equivalents in the new implem)

One thing that I thought for long and never got around to implement is a minimal set of tests for the non-GUI algorithmics in the code (like the Git, Domain, Rev classes etc.). These would be very useful now.

So you take up one of these 3 and note here which, I will focus on the others. Still, please note that I will have little time for this in the next 3-4 weeks, so I will advance slowly.

Thanks
Cristian

[hartwork]

@tibirna thanks for your reply!

So you take up one of these 3 and note here which, I will focus on the others. Still, please note that I will have little time for this in the next 3-4 weeks, so I will advance slowly.

Let me pick the third — QRegExp to QRegularExpression — and also the CI part. I will also try to get a sense of the QTextCodec situations with regard to encodings without touching the related code, so that I understand the challenge better myself.

[tibirna]

@hartwork Thanks a lot! That will be very useful!

[hartwork]

@tibirna I have some first results now. I built on top of your compile-w-qt6 at 879df64 and I have five new commits. I'll share them here as git-format-patches (to apply via git am *.patch.txt) (but they are also sitting in a branch at https://github.com/hartwork/qgit/tree/compile-w-qt6-PLUS-experiments that feeds draft pull request #153):

• 0001-Fix-use-of-QRegularExpression.patch.txt
• 0002-Resolve-use-of-QDateTime-setTime_t-for-Qt-6.patch.txt
• 0003-build.yaml-Migrate-CI-from-Qt-5-to-Qt-6.patch.txt
• 0004-CMake-Lower-minimum-Qt-version-to-6.2.4-for-Ubuntu-2.patch.txt
• 0005-REVERT-ME-Add-Core5Compat-for-a-QTextCodec-band-aid.patch.txt

With those 5 (five) I get green CI already. There is a few things I should mention:

• There are 3 (three) places using QRegExp::setMinimal(true); that is disabling greedy mode for the whole pattern which cannot be translated to QRegularExpression. The good news is that the user now can just use lazy quantifiers in the pattern if they want to which is more flexible (..). Do you consider that acceptable? There is QRegularExpression::InvertedGreedinessOption but I do not consider that like great user experience.
• In files src/patchcontent.{cpp,h} I resolved isRegExp and as a result also method doSearch so that we never have to put non-regex patterns into QRegularExpression. I hope you like the simplification, I believe performance will not be an issue. What do you think?
• Minimum Qt version 6.6.0 would have been a problem because Ubuntu 24.04 has 6.4.2 and Ubuntu 22.04 has 6.2.4. That's what mainstream users and CI images have and so I dropped the required version down to 6.2.4. I hope that's alright with you: the code does not seem to need 6.6.0 in practice yet, which is great.
• curCodec in src/settingsimpl.cpp may need attention because either it was meant to be okay to contain wildcard characters and have them work as wildcards (and then my patch is wrong) or it was not and then I fixed a potential bug as a side effect. Please review that part carefully in particular; happy to adjust as needed.

I'm looking forward to your feedback 🍻

[tibirna]

@hartwork That was fast! Thanks!

Topically:

• The modification at line 1119 in git.cpp in fc6e37d drops CaseInsensitive. I think it was necessary. What do you think?
• I guess use of .setMinimal() was a question of taste, indeed. The shortlog search doesn't work with the changes, though, so I'll debug through it when I get a moment.
• For the patchcontent.{cpp,h} change, the implementation before your changes actually reflects the UI (there is "Patch" and "Patch (regex)" search choice in the main toolbar). So it is a manual (user) choice to identify some text (that can by a regex(!)) as a plain text to be searched for, and not a regexp. So I guess the 'isRegExp' boolean was necessary.
• my intuition of Qt 6.6 came from reading some Qt forums in which the functionality of QStringConverter was described as limited to only a few UTF codecs up to 6.6. If we use Qt5Compat for now, this is solved and also avoids conflicts with Ubuntu etc., as you mention.
• I don't fully understand the use of regexp on curCodec either. It appears that the original developer wanted to ignore noise/whitespace around valid codec names, as it is coming from user input. I guess your modification works. Testing will be needed, of course.

I'll wait for your reaction, then I'll merge your PR. When time allows, I'll look at the QMessageBox warnings and test/fix some more the changes in the branch. The QTextCodec thing will require more reflection.

Thank you very much!
Cristian

[hartwork]

@tibirna thanks for the review!

• The modification at line 1119 in git.cpp in fc6e37d drops CaseInsensitive. I think it was necessary. What do you think?

Definitely an unintended loss in translation, fixed, good catch 👍

• I guess use of .setMinimal() was a question of taste, indeed. The shortlog search doesn't work with the changes, though, so I'll debug through it when I get a moment.

Please do, thanks! 👍

• For the patchcontent.{cpp,h} change, the implementation before your changes actually reflects the UI (there is "Patch" and "Patch (regex)" search choice in the main toolbar). So it is a manual (user) choice to identify some text (that can by a regex(!)) as a plain text to be searched for, and not a regexp. So I guess the 'isRegExp' boolean was necessary.

There is still the bool re in void PatchContent::on_highlightPatch(const QString& exp, bool re) that is respected via pickAxeRE.setPattern(re ? exp : QRegularExpression::escape(exp)); — is that enough? I'm happy to learn what I'm missing.

• my intuition of Qt 6.6 came from reading some Qt forums in which the functionality of QStringConverter was described as limited to only a few UTF codecs up to 6.6. If we use Qt5Compat for now, this is solved and also avoids conflicts with Ubuntu etc., as you mention.

I see!

• I don't fully understand the use of regexp on curCodec either. It appears that the original developer wanted to ignore noise/whitespace around valid codec names, as it is coming from user input. I guess your modification works. Testing will be needed, of course.

I found two related things now:

• Line codecs.append(QString("Local Codec (" + localCodec + ")")); means that the codec name in localCodec will need substring search rather than full string match to be found during search.
• QStringList does not seem to allow easy search for substrings ("partial matches?") other than through regex; this thread is with me on that (but that's no proof). Because of "the first exact match of re in the list" I have added ^.* as a prefix and .*$ for a suffix now to make it obvious that it's about exact matches with regard to QStringList.

Pushed.

I'll wait for your reaction, then I'll merge your PR.

If you learn that anything else is broken, I'm happy to adjust before merging.

When time allows, I'll look at the QMessageBox warnings and test/fix some more the changes in the branch. The QTextCodec thing will require more reflection.

Sounds like a plan 👍

Thank you very much! Cristian

Glad I can be of help, thanks for the review! 🍻

[tibirna]

Thanks! Merged. (The boolean handing on on_highlightPatch() I just missed it, sorry)

[hartwork]

@tibirna cool! How do we best continue?

[tibirna]

@hartwork I'm grateful for your help, this conversion advances much faster than I thought when I dropped the ball first a few days ago :-)

I think I cleaned up the remaining warnings on QMessageBox, type conversions and other API deprecations.

The only other thing I'll want to spend some time with is the QTextCodec thing. That one is rather finicky, both in terms of API changes and wrt. testing. Example repositories with at least one unusual codec will be needed.

Then thorough testing will be necessary, in particular in the areas touched by the porting:

• message dialogs
• those code paths relying on regular expressions
• everything else :-)

Once the dust settles, I'll think of how is this best released.

Thanks again!

[hartwork]

@hartwork I'm grateful for your help, this conversion advances much faster than I thought when I dropped the ball first a few days ago :-)

I think I cleaned up the remaining warnings on QMessageBox, type conversions and other API deprecations.

@tibirna cool!

The only other thing I'll want to spend some time with is the QTextCodec thing. That one is rather finicky, both in terms of API changes and wrt. testing. Example repositories with at least one unusual codec will be needed.

I have been playing with Git's i18n.commitencoding a bit now and my impression is that:

• When used with git commit, Git will store the encoding name in the resulting commit (proof below).
• When used with git format-patch, Git will use the encoding when writing headers Content-Type and Subject
• When used with git log, Git will re-encode commit message output to the requested encoding (instead of UTF-8).
• When used with git cat-file, i18n.commitencoding is ignored! (That could be a problem to the current Git::getTagMsg but auto-detection of the used encoding would not be simple and not 100% waterproof.)
• Replacing all git <subcommand> with git -c i18n.commitencoding=UTF-8 <subcommand>
  and dropping QTextCodec and Core5Compat altogether may be an option for all(?) but git cat-file.

For proof of Git recording non-UTF-8 encodings when creating commits (with locale en_US.UTF-8):

# cd "$(mktemp -d)"

# git init
Initialized empty Git repository in /tmp/tmp.pA6hpwXAst/.git/

# git -c i18n.commitencoding=ISO-8859-1 commit --allow-empty -m "$(enca -L none -x ISO-8859-1 <<<"Hällo Wörld (ISO-8859-1)")"
[main (root-commit) 307c205] H�llo W�rld (ISO-8859-1)

# git cat-file commit HEAD 
tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
author Sebastian Pipping <sebastian@pipping.org> 1746280089 +0200
committer Sebastian Pipping <sebastian@pipping.org> 1746280089 +0200
encoding ISO-8859-1

H�llo W�rld (ISO-8859-1)

# git -c i18n.commitencoding=UTF-8 commit --allow-empty -m "Hällo Wörld"
[main 8cb813e] Hällo Wörld

# git cat-file commit HEAD 
tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904
parent bf93e4b786b204da2cb0eb6fd98a531437863479
author Sebastian Pipping <sebastian@pipping.org> 1746280238 +0200
committer Sebastian Pipping <sebastian@pipping.org> 1746280238 +0200

Hällo Wörld

Then thorough testing will be necessary, in particular in the areas touched by the porting:

• message dialogs

• those code paths relying on regular expressions

• everything else :-)

Yes please 👍

How do you feel about creating a pull request for compile-w-qt6 to master just for everyone to see that it exists and what is coming up, even while it's still moving?

Once the dust settles, I'll think of how is this best released.

Cool!

[tibirna]

My intuition is that Marco Costalba (the original author) has included all the encoding gymnastics in order to ensure proper handling of already existing git repos in which text is in whatever encoding the user was setting up in their system (that was a time when UTF8 was not yet so well established). Anyways, Qt stores QStrings internally in UTF16, so conversion from Latin1 or UTF8 is still needed. Also, I don't think QGit can take liberties with how it stores data in the repo either, since the user will access that repo outside QGit too. The reason of the Text codec setting is there in the QGit settings is to cope with that. So, three reasons why the QTextCodec work is still necessary, if I'm not mistaken.

Yes, the plan is to merge compile-w-qt6 into master (and make Qt6 use the default). But thorough testing will be needed before, especially since, starting recently, the repo is followed (and released) in Flathub. So, as I was saying, I will let the dust settle first a little bit.