Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extfs: rpm: INSTALL is truncated in the viewer #3865

Closed
mc-butler opened this issue Oct 1, 2017 · 15 comments
Closed

extfs: rpm: INSTALL is truncated in the viewer #3865

mc-butler opened this issue Oct 1, 2017 · 15 comments
Assignees
Labels
area: vfs Virtual File System support prio: low Minor problem or easily worked around
Milestone

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/3865
Reporter anatoly.borodin (anatoly.borodin@….com)
Mentions egmont (@egmontkob)
Keywords extfs, rpm

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.

Note

Original attachments:

@mc-butler
Copy link
Author

Changed by anatoly.borodin (anatoly.borodin@….com) on Oct 1, 2017 at 3:34 UTC (comment 1)

I can see three possible solutions to the problem:

1) Remove the rpm commands from mcrpmfs_copyout() (have been added in [e7ed071]), 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.

@mc-butler
Copy link
Author

Changed by anatoly.borodin (anatoly.borodin@….com) on Oct 1, 2017 at 3:48 UTC (comment 2)

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

@mc-butler
Copy link
Author

Changed by anatoly.borodin (anatoly.borodin@….com) on Oct 1, 2017 at 4:08 UTC

The first solution

@mc-butler
Copy link
Author

Changed by anatoly.borodin (anatoly.borodin@….com) on Oct 1, 2017 at 4:25 UTC (comment 3)

git fetch https://github.com/anatolyborodin/mc 3865_extfs_rpm_install_truncated:3865_extfs_rpm_install_truncated

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 2, 2017 at 20:35 UTC (comment 4)

  • Cc set to egmont

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).

@mc-butler
Copy link
Author

Changed by anatoly.borodin (anatoly.borodin@….com) on Oct 4, 2017 at 4:52 UTC (comment 5)

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.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 4, 2017 at 14:49 UTC (comment 6)

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.

@mc-butler
Copy link
Author

Changed by anatoly.borodin (anatoly.borodin@….com) on Oct 6, 2017 at 13:20 UTC (comment 7)

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 [7ddc296] and [f029a52].

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).

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 6, 2017 at 13:52 UTC (comment 7.8)

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?

@mc-butler
Copy link
Author

Changed by anatoly.borodin (anatoly.borodin@….com) on Oct 6, 2017 at 17:39 UTC (comment 8.9)

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.

@mc-butler
Copy link
Author

Changed by egmont (@egmontkob) on Oct 6, 2017 at 19:48 UTC (comment 10)

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.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Dec 23, 2017 at 16:26 UTC (comment 11)

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

Branch: 3865_exfs_rpm_scripts.
Initial [e9cc914e2efbc31689338302fa29ff0b7ad8279a]

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Dec 23, 2017 at 19:01 UTC (comment 12)

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

I'm ok with removing the commands...

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Dec 24, 2017 at 6:44 UTC (comment 13)

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

Merged to master: [218dcea].

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

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Dec 24, 2017 at 6:46 UTC (comment 14)

  • Status changed from testing to closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: vfs Virtual File System support prio: low Minor problem or easily worked around
Development

No branches or pull requests

2 participants