Ticket #4502 (closed defect: fixed)

Opened 10 months ago

Last modified 2 weeks ago

File bindings don't work with GLib 2.77.3

Reported by: sachahony Owned by: andrew_b
Priority: major Milestone: 4.8.32
Component: mc-config-ini Version: 4.8.31
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

I have updated my glib2 from 2.77.2 to 2.77.3. After the update I can no longer enter into tar.bz2 files. Reverting back 2.77.2 solves the issue. Also the view archive shows a "binary" view rather than a "direcory" as if the file is not recognised.

Changelog of glib: https://download.gnome.org/sources/glib/2.77/glib-2.77.3.news

Attachments

sample.war (4.5 KB) - added by zaytsev 3 weeks ago.

Change History

comment:1 Changed 10 months ago by andrew_b

Are any other regressions with glib-2.77.3 besides tar?

Why component is mcview?

comment:2 Changed 10 months ago by andrew_b

  • Component changed from mcview to mc-config-ini

The significant change in GLib-2.77.3 is gkeyfile: Fix overwriting of GError.
Commit 71b7efd08.

Solution: escape backslashes in mc.ext.ini: replace single \ with double one: \\.

comment:3 Changed 10 months ago by Toolybird

We just hit this in Arch Linux because that commit made it into stable release glib2-2.76.5

comment:4 Changed 10 months ago by dimich

I can confirm the issue with glib-2.76.5 in Arch: https://bbs.archlinux.org/viewtopic.php?pid=2118521

Not only compressed tar archives affected but other suffixes too, e.g. .mp4, .odt

comment:5 follow-up: ↓ 7 Changed 10 months ago by zaytsev

Oh man, why they would handle escape sequences in INI files in the first place o_O ?

Maybe we can just switch from g_key_file_get_string to g_key_file_get_value API, because otherwise every stupid backslash would have to be escaped and it will make a bloody hell out of our regexes.

comment:6 Changed 10 months ago by ossi

the ini parser is also used to parse .desktop files and such stuff, so supporting escapes in strings makes perfect sense.

it is a bit surprising that such a seemingly obvious bugfix has such side effects. i guess mc does something ... "interesting"?

comment:7 in reply to: ↑ 5 Changed 10 months ago by andrew_b

Replying to zaytsev:

Maybe we can just switch from g_key_file_get_string to g_key_file_get_value API, because otherwise every stupid backslash would have to be escaped and it will make a bloody hell out of our regexes.

g_key_file_* API is used in mc indirectly via mcconfig engine. Changing g_key_file_get_string to g_key_file_get_value in mcconfig may break the handling of other INI files.

We already have INI files with escaped backslashes: mc.lib, Syntax (it is not INI but nonetheless), probably some others. I think mc.ext.ini should be consistent with them.

comment:8 follow-up: ↓ 15 Changed 10 months ago by zaytsev

it is a bit surprising that such a seemingly obvious bugfix has such side effects. i guess mc does something ... "interesting"?

I'm not sure I would define this as "interesting". I think it's just missing error handling, the usual C stuff.

We already have INI files with escaped backslashes: mc.lib, Syntax (it is not INI but nonetheless), probably some others. I think mc.ext.ini should be consistent with them.

You are right, but exactly for this reason I find them to be disgusting and actually see no reason why we should support escape sequences in the configuration files. So I'd rather break everything while we are at it :-) But it's up to you.

comment:9 Changed 10 months ago by andrew_b

  • Summary changed from VFS of compressed tar archives appears broken with GLib 2.77.3 to File bindings don't work with GLib 2.77.3

comment:10 Changed 10 months ago by andrew_b

Probably the story isn't finished yet: https://gitlab.gnome.org/GNOME/glib/-/issues/3095

comment:12 Changed 9 months ago by andrew_b

Ticket #4505 has been marked as a duplicate of this ticket.

comment:13 Changed 9 months ago by birdie

I thought I was going crazy. Thanks, Andrew.

comment:14 Changed 9 months ago by birdie

A quick temporary fix for the affected users:

sudo cp -a /etc/mc/mc.ext.ini /etc/mc/mc.ext.ini.bak
sudo sed '/Regex=/s/\\/\\\\/g' -i /etc/mc/mc.ext.ini

comment:15 in reply to: ↑ 8 Changed 9 months ago by andrew_b

So, I propose the following:

  • escape backslashes in mc.ext.ini to be consistent with other files were regexps are used;
  • use g_key_file_get_value instead of g_key_file_get_string to avoid parsing of key value in GLib.

Replying to zaytsev:

actually see no reason why we should support escape sequences in the configuration files

Escape sequence is a part of key definition for various terminals: https://midnight-commander.org/browser/misc/mc.lib#L15

comment:16 Changed 9 months ago by andrew_b

  • Owner set to andrew_b
  • Status changed from new to accepted
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.31

Branch: 4502_mc.ext.ini
Initial changeset:2da7ef347d1c48e4ad8e73f5135cfe8a363e6c1f

comment:17 Changed 9 months ago by andrew_b

  • Branch state changed from on review to on rework

Some tests failed.

comment:18 Changed 9 months ago by andrew_b

  • Branch state changed from on rework to on review

The usage g_key_file_get_value instead of g_key_file_get_string requires a lot of changes in test. Let's leave it as it is.

comment:19 Changed 9 months ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

comment:20 Changed 9 months ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

comment:21 Changed 9 months ago by andrew_b

  • Status changed from testing to closed

comment:22 follow-up: ↓ 24 Changed 3 weeks ago by zaytsev

  • Status changed from closed to reopened
  • Votes for changeset committed-master deleted
  • Version changed from 4.8.30 to 4.8.31
  • Branch state changed from merged to no branch
  • Milestone 4.8.31 deleted
  • Resolution fixed deleted

On macOS:

zaytsev@Yurys-MBP Downloads % file sample.war 
sample.war: Java archive data (JAR)

This is what we have to support:

# Java Jar files
>(26.s+30)	leshort	0xcafe		Java Jar file data (zip)
!:mime	application/jar
>(26.s+30)	leshort	0xcafe		Java archive data (JAR)
!:mime	application/java-archive

In mc.ext:

[jar]
Type=\(Java\ (Jar\ file|archive)\ data\ \((zip|JAR)\)

Enter does not work. Also confirmed on Ubuntu 22.04:

https://bugs.launchpad.net/mc/+bug/2062968

What works is the following:

[jar]
Type=\\(Java (Jar file|archive) data \\((zip|JAR)\\)\\)

It looks like we have a big problem with escaping:

  1. Spaces should not be escaped
  2. Literal parens should be escaped like this: \\( and not like this \(

Andrew, what do you think?

comment:23 Changed 3 weeks ago by zaytsev

  • Milestone set to 4.8.32

Changed 3 weeks ago by zaytsev

comment:24 in reply to: ↑ 22 Changed 3 weeks ago by andrew_b

Replying to zaytsev:

  1. Spaces should not be escaped

Should not or may not? Are correctly escaped spaces worse than not escaped?

  1. Literal parens should be escaped like this: \\( and not like this \(

Agree. Moreover, . is escaped as \\., [ as \\[.

comment:25 Changed 3 weeks ago by zaytsev

Should not or may not? Are correctly escaped spaces worse than not escaped?

Good question. In my opinion they are worse, because it's not obvious, which lead to current problems and also it looks like a wood. If escaping is not necessary, I would not use it to keep the patterns simpler, more readable and more understandable.

Agree.

I think at the moment we only have escaped spaces, which I would unescape, wrongly escaped parens and a missing dot:

# dot unescaped
Regex=.g?mo$

comment:26 Changed 3 weeks ago by andrew_b

  • Branch state changed from no branch to on review

Branch: 4502_mc.ext.ini_fixups
Initial changeset:2384a486f7a9ae3e6da6039648edbc8bad58159f

comment:27 Changed 2 weeks ago by zaytsev

  • Votes for changeset set to zaytsev
  • Branch state changed from on review to approved

The JAR thing was still wrong, pushed some fixups, thank you!

comment:28 Changed 2 weeks ago by andrew_b

Merged to master: [942869713448691f377b91f26cf9ebdac9e2e4c9].

git log --oneline cb97cd263..942869713

comment:29 Changed 2 weeks ago by andrew_b

  • Status changed from reopened to closed
  • Votes for changeset changed from zaytsev to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged
Note: See TracTickets for help on using tickets.