Ticket #3865 (closed defect: fixed)

Opened 7 years ago

Last modified 6 years ago

extfs: rpm: INSTALL is truncated in the viewer

Reported by: anatoly.borodin Owned by: andrew_b
Priority: minor Milestone: 4.8.21
Component: mc-vfs Version: master
Keywords: extfs, rpm Cc: egmont
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

GNU Midnight Commander 4.8.19
Built with GLib 2.53.4
Using the S-Lang library with terminfo database
With builtin Editor
With subshell support as default
With support for background operations
With mouse support on xterm and Linux console
With support for X11 events
With internationalization support
With multiple codepages support
Virtual File Systems: cpiofs, tarfs, sfs, extfs, ext2undelfs, ftpfs, sftpfs, fish
Data types: char: 8; int: 32; long: 64; void *: 64; size_t: 64; off_t: 64;

If you try to view the INSTALL file with the internal viewer, only this text is shown:

# Run this script to install this RPM p

The reason: the size of INSTALL / UPDATE / REBUILD is hardcoded as 39 in src/vfs/extfs/helpers/rpm.

Attachments

0001-Ticket-3865-extfs-rpm-INSTALL-is-truncated-in-the-vie.diff (2.0 KB) - added by anatoly.borodin 7 years ago.
The first solution

Change History

comment:1 Changed 7 years ago by anatoly.borodin

I can see three possible solutions to the problem:

1) Remove the rpm commands from mcrpmfs_copyout() (have been added in e7ed071be7fda2df59191d103c47bf30a6b98ca8), revert the INSTALL line for consistency (remove the 'script' word). Now the files are 39 bytes again.

2) Set the size of the files to 0 in mcrpmfs_list(). I don't know, if its the right thing to do, but it works with the INFO/* files, and it triggers mcview to show the full file contents.

3) Recalculate the size of the files in mcrpmfs_list(), like it's done for HEADERSIZE. Of course, some refactoring should be done to avoid the code duplication.

NB:

1) I propose the removal of the word 'script' in any case: it is inconsistent with UPDATE and REBUILD.

2) I personally prefer the first solution, as the easiest one. See the attached patch.

3) The second and the third solution add the command to the text. But the command should be fixed as well: the file name is written unescaped, it will not work with whitespace in the path if just copy&pasted.

Last edited 7 years ago by anatoly.borodin (previous) (diff)

comment:2 Changed 7 years ago by anatoly.borodin

  • Summary changed from extfs: rpm: INSTALL is truncated to extfs: rpm: INSTALL is truncated in the viewer

Changed 7 years ago by anatoly.borodin

The first solution

comment:3 Changed 7 years ago by anatoly.borodin

git fetch https://github.com/anatolyborodin/mc 3865_extfs_rpm_install_truncated:3865_extfs_rpm_install_truncated
Last edited 7 years ago by anatoly.borodin (previous) (diff)

comment:4 Changed 7 years ago by egmont

  • Cc egmont added

Thanks for the bugreport and investigation!

I'd prefer the 2nd approach if it indeed works. Especially given the recent (post-4.8.19) "grow file" fixes why not use this feature if we can? In virtual file systems I think it's pretty common and acceptable not to know the file size in advance and report some fake value (e.g. 0 or 4096) in the file listing.

Any hardcoded number such as 39 is bound to break in the future, let alone it's not translatable by design (not sure if we're planning to translate these strings but at least with the 2nd approach we could if we wanted to).

Last edited 7 years ago by egmont (previous) (diff)

comment:5 Changed 7 years ago by anatoly.borodin

Ok. Should we leave the rpm command call? It differs from other extfs scripts (like e.g. deb), it can be buggy (whitespace etc), and I don't think it's more useful than man rpm.

comment:6 Changed 7 years ago by egmont

If you remove the rpm command then the virtual INSTALL, UPGRADE and REBUILD files will remain useless, right? We could remove these files completely, sure that's a way to fix this bug :-) I used to use this feature a lot when I had rpm-based systems and yum/dnf wasn't yet available.

What do you mean "It differs from other extfs scripts (like e.g. deb)"? "deb" also has a virtual INSTALL file, and by the way, also misbehaves if the filename has a special character in it.

I'd rather fix the issue which is probably as simple as adding $(printf %q ...) at the right places.

comment:7 follow-up: ↓ 8 Changed 7 years ago by anatoly.borodin

If you remove the rpm command then the virtual INSTALL, UPGRADE and REBUILD files will remain useless, right?

You can still run the corresponding rpm command by pressing Enter on a file. See the function mcrpmfs_run() in src/vfs/extfs/helpers/rpm. They will not become useless.

What do you mean "It differs from other extfs scripts (like e.g. deb)"?

If you go into a deb file, the contents of INSTALL is

                              WARNING
     Don't use this method if you are not willing to reinstall everything...

This is not a real file. It is a way to install the package you are browsing.

To install this package go back to the panel and press Enter on this file.

In Debian systems, a package is automatically upgraded when you install a new
version of it. There is no special upgrade option. Install always works.

No command calls, just text. But thanks to sub mcdebfs_run, it can be run as well.

"deb" also has a virtual INSTALL file, and by the way, also misbehaves if the filename has a special character in it.

Is it so? It's a bug then, and should be fixed, like the bug fixed in 7ddc29649bcdad82dad30e36bed6df136421f519 and f029a529d26c8c9cda13ee6930374aa60b84877e.

Unfortunately, I don't speak Perl. I don't know which special characters can break it, but it works with spaces (in the path and in the file name).

Last edited 7 years ago by anatoly.borodin (previous) (diff)

comment:8 in reply to: ↑ 7 ; follow-up: ↓ 9 Changed 7 years ago by egmont

Replying to anatoly.borodin:

You can still run the corresponding rpm command by pressing Enter on a file.

Sorry, I'm lost, or I'm missing something.

If you press Enter on the rpm file itself, it enters the virtual file system and you get to see its contents along with files such as INSTALL. If you press Enter on the virtual file INSTALL, it'll install the archive.

Sounds to me from your words that there's another "automatic" way (not typing the entire "rpm -i ..." in the command line) of installing the rpm as well. What is that?

comment:9 in reply to: ↑ 8 Changed 7 years ago by anatoly.borodin

Replying to egmont:

Replying to anatoly.borodin:

You can still run the corresponding rpm command by pressing Enter on a file.

Sorry, I'm lost, or I'm missing something.

If you press Enter on the rpm file itself, it enters the virtual file system and you get to see its contents along with files such as INSTALL. If you press Enter on the virtual file INSTALL, it'll install the archive.

That's what I was talking about: pressing Enter on the INSTALL file. It will execute the installation command, and it doesn't matter what is being shown by F3. So it will not be useless, even if there will be only some text without commands.

comment:10 Changed 7 years ago by egmont

My bad. I understand you now. I didn't realize that long version of the text that is truncated now (what this bugreport is about) is also supposed to show the command to the user that (s)he can execute.

I agree that there's not much point in showing this command where they can just run this script. Hence I wouldn't mind removing the rpm command from the copyout function.

comment:11 Changed 6 years 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.21

Branch: 3865_exfs_rpm_scripts.
Initial changeset:e9cc914e2efbc31689338302fa29ff0b7ad8279a

comment:12 Changed 6 years ago by zaytsev

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

I'm ok with removing the commands...

comment:13 Changed 6 years ago by andrew_b

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

Merged to master: [218dcea54320eb0d4ed029dbc0ac661eb4f8fe62].

git log --pretty=oneline 38ce185..218dcea

comment:14 Changed 6 years ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.