Ticket #3611 (closed defect: fixed)

Opened 3 years ago

Last modified 3 years ago

[PATCH] Fish: fix perl ls helper

Reported by: and Owned by: zaytsev
Priority: blocker Milestone: 4.8.17
Component: mc-vfs Version: 4.8.16
Keywords: Cc:
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

With #3599 I introduce a perl warning in fish_list_perl()
so fish_list_perl() was skipped (return code 255) and fallback ls
function was used instead.

Plus all % chars must quoted because of g_strconcat() after reading
script file into string.

Attachments

mc-3611-fish-fix-perl-ls-helper.patch (1.3 KB) - added by and 3 years ago.

Change History

Changed 3 years ago by and

comment:1 Changed 3 years ago by zaytsev

  • Status changed from new to accepted
  • Owner set to zaytsev

comment:2 Changed 3 years ago by andrew_b

  • Blocking 3613 added

comment:3 Changed 3 years ago by zaytsev

Nah, I should have found some way to test the Perl helper as well, I only actually tested the ls part, which worked :-/

comment:4 follow-ups: ↓ 9 ↓ 11 Changed 3 years ago by zaytsev

@and, since you broke it in #3599, do you think that in addition to providing a patch, you could cover the FISH scripts with unit tests as a punishment?

This shouldn't be too complicated (specifically, see chapters about Simple Tests), but this will make for the first time possible to change the venerable FISH scripts without panic fear:

https://www.gnu.org/software/automake/manual/html_node/Tests.html

Right now, if you change anything there you have to do rigorous manual testing with different hosts and pray that nothing has got broken by the new changes...

comment:5 Changed 3 years ago by zaytsev

  • Blocking 3613 removed

(In #3613) Closed as duplicate of #3611.

comment:6 Changed 3 years ago by zaytsev

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

comment:7 Changed 3 years ago by andrew_b

  • Priority changed from major to blocker
  • Version changed from master to 4.8.16

comment:8 Changed 3 years ago by zaytsev-work

Also see comments in #3620.

comment:9 in reply to: ↑ 4 Changed 3 years ago by mooffie

Replying to zaytsev:

@and, since you broke it in #3599, do you think that in addition to providing a patch, you could cover the FISH scripts with unit tests as a punishment?


BTW, do you know of a public SSH server where FISH tests could be run against? (I googled a bit but couldn't find any).

Now, I know mc2 is off-topic, but I want to mention that it's fun to write tests in it. For example, to test FISH:

local ensure = devel.ensure

local function test_fish()

  -- ticket #3599.
  local stat = assert(fs.stat('/sh://mooffie@mooffie.typo.co.il/dev/tty1'))
  ensure.equal(stat.rdev, 0x401,
    'The filesystem reports major:minor correctly for device files.')

  -- ticket #3611.
  ensure.equal(tglob('/sh://mooffie@mooffie.typo.co.il/etc/fs[t]ab'), {'/etc/fstab'},
    'The filesystem reports its files.') 

end

(Manual pages: fs, devel.ensure)

There's already a test that covers many file operations. Right now it does its stuff in '/tmp', but by changing this to '/sh://whatever-server.you-want/tmp' you'd get a complete coverage of FISH.

As another example, here's a trivial test for the subshell:

local function test_subshell()

  fs.chdir('/')
  mc.execute('cd /usr/bin')
  ensure.equal(fs.current_dir(), '/usr/bin',
    'The subshell works and reports back its directory.')

end

(Manual entries: mc.execute)

comment:10 follow-up: ↓ 13 Changed 3 years ago by zaytsev-work

BTW, do you know of a public SSH server where FISH tests could be run against? (I googled a bit but couldn't find any).

To be honest, I don't know of any; of course, we could set up an account on our development VM, but that doesn't sound like a good solution to me at any rate.

However, I don't think that's really necessary in the first place. One could write a test script that would spawn dropbear (installed from a package or even compiled on the spot, as you have witnessed is very easily possible) and test against that.

(It is even possible to elevate to root on Travis, so one could just as well set up an sshd to listen on 127.0.0.1 and be done with it.)

Now, I know mc2 is off-topic, but I want to mention that it's fun ​to write tests in it.

Yes, I've seen that ;-) and you know that I'm very enthusiastic about it in general, but I'm not sure whether this is the right approach in this particular case.

I've got the feeling that it's great for the integration kind of tests, but here we'd rather need to break up the scripts further, such that they can be unit tested against fake outputs from ls and friends from different systems (~normal and ~busyboxy Linux, ~BSD, ~Solaris).

Maybe if you have a deeper look at the scripts, you'll see what I mean.

comment:11 in reply to: ↑ 4 Changed 3 years ago by and

Replying to zaytsev:

@and, since you broke it in #3599, do you think that in addition to providing a patch, you could cover the FISH scripts with unit tests as a punishment?

This shouldn't be too complicated (specifically, see chapters about Simple Tests), but this will make for the first time possible to change the venerable FISH scripts without panic fear:

https://www.gnu.org/software/automake/manual/html_node/Tests.html

Right now, if you change anything there you have to do rigorous manual testing with different hosts and pray that nothing has got broken by the new changes...

Jep and thanks for the pointer, sounds great by "simple" shell check script(s), I put it on my TODO list (low urgency).

comment:12 Changed 3 years ago by woodsb02

Please note that FreeBSD bug report 208104 has been raised to include the proposed patch to fix this issue in the FreeBSD 4.8.16 packages.
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=208104

comment:13 in reply to: ↑ 10 Changed 3 years ago by mooffie

Replying to zaytsev-work:

Now, I know mc2 is off-topic, but I want to mention that it's fun ​to write tests in it.

Yes, I've seen that ;-) and you know that I'm very enthusiastic about it in general, but I'm not sure whether this is the right approach in this particular case.

I've got the feeling that it's great for the integration kind of tests, but here we'd rather need to break up the scripts further, such that they can be unit tested against fake outputs from ls and friends from different systems (~normal and ~busyboxy Linux, ~BSD, ~Solaris).

Maybe if you have a deeper look at the scripts, you'll see what I mean.


Yes, I now see what you mean.

I also see, for example, that there's the same concern with ticket:3622: a test that tests it through the VFS might work (when the machine it runs on happens to have uzip compiled with MDY order) but some users will encounter a problem still (when using YMD order).

I'll use this opportunity to thank you for the way you compose your posts. They're laden with helpful overviews (e.g., ticket:2633, ticket:3622, ticket:2742, mentioning "integration kind of tests" vs others), with tips (e.g., mentioning dropbear etc, 'autoreconf' vs './autogen.sh') and that's just from today. Your posts certainly enlighten me (and others no doubt).

comment:14 Changed 3 years ago by zaytsev

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

comment:15 Changed 3 years ago by andrew_b

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

comment:16 Changed 3 years ago by andrew_b

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

comment:17 Changed 3 years ago by andrew_b

Let fix this without tests.

comment:18 Changed 3 years ago by zaytsev-work

I agree, will try today or tomorrow and create a separate ticket for tests, which can be acted upon later. I remember there was a related problem for patchfs, but that one can be tested easier.

comment:19 Changed 3 years ago by zaytsev

I have created #3635 for FISH tests.

comment:20 Changed 3 years ago by zaytsev

  • Branch state changed from no branch to on review

Branch: 3611_fish_ls_perl_fix
Initial changeset: 263012123cb1e4bf8201da00a9c248497b326076

comment:21 Changed 3 years ago by andrew_b

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

comment:22 Changed 3 years ago by zaytsev

  • 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:23 Changed 3 years ago by zaytsev

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