Ticket #3578 (closed defect: fixed)

Opened 8 years ago

Last modified 8 years ago

mc.ext fixes

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

Description

Right now, there are regexes that either are too greedy, like regex/[Mm]akefile$ or simply do not work like regex/^Makefile.(PL|pl)$, because the paths tested are absolute, not relative; my suggested fix is to prepend a slash.

Additionally, I could implement https://github.com/MidnightCommander/mc/pull/83 while we are at it.

Change History

comment:1 in reply to: ↑ description Changed 8 years ago by andrew_b

Replying to zaytsev:

Additionally, I could implement https://github.com/MidnightCommander/mc/pull/83 while we are at it.

#3115

comment:2 follow-up: ↓ 4 Changed 8 years ago by zaytsev

Thank you, Andrew!

comment:3 follow-up: ↓ 6 Changed 8 years ago by zaytsev

  • Status changed from new to accepted
  • Owner set to zaytsev
  • Branch state changed from no branch to on review

Branch: 3578_mcext_fixes.
Initial changeset: [cb146b74237aeae7acb96dde94ea3e655c6b84d5].

Please review!

comment:4 in reply to: ↑ 2 Changed 8 years ago by andrew_b

Replying to zaytsev:

Thank you, Andrew!

Cannot add comment to #3115 due to circular dependencies (block and block by itself).

comment:5 Changed 8 years ago by zaytsev

Cannot add comment to #3115 due to circular dependencies (block and block by itself).

Sorry, it's my fault :-/ should be fixed now.

comment:6 in reply to: ↑ 3 Changed 8 years ago by andrew_b

Replying to zaytsev:

Branch: 3578_mcext_fixes.
Initial changeset: [cb146b74237aeae7acb96dde94ea3e655c6b84d5].

Please review!

-regex/[Mm]akefile$
+regex//[Mm]akefile$

Probably

-regex/[Mm]akefile$
+shell/i/Makefile

-regex/^Makefile.(PL|pl)$
+regex//Makefile\.(PL|pl)$

Probably

-regex/^Makefile.(PL|pl)$
+shell/i/Makefile.pl

comment:7 Changed 8 years ago by zaytsev

shell/i/Makefile

This would be wrong, because this would match *makefile; as far as I understand the idea behind this regex was to match only files with name "Makefile" or "makefile", so that if you press Enter on them it would ask for parameters and run make. Files like "strangemakefile" for example should not be matched.

shell/i/Makefile.pl

Same here, this was originally supposed to match Perl MakeMaker? files, they have the filename "Makefile.pl" or "Makefile.PL", a shell expression like you suggest will match also "MyMakefile?.pL" for example... not sure if this is desired.

comment:6 Changed 8 years ago by zaytsev

  • Blocking 3115 added

comment:7 Changed 8 years ago by zaytsev

OK, I hope the mess is sorted out now.

comment:8 in reply to: ↑ description Changed 8 years ago by andrew_b

Replying to zaytsev:

Right now, there are regexes that either are too greedy, like regex/[Mm]akefile$ or simply do not work like regex/^Makefile.(PL|pl)$, because the paths tested are absolute

I don't quite understand the essence of this bug. Could you please provide an example?

comment:9 Changed 8 years ago by zaytsev

Bug 1: regex/[Mm]akefile$

This will match "myMakefile", instead of only "Makefile" or "makefile". Reproducer:

a) touch myMakefile
b) position the cursor on "myMakefile"
c) press Enter.

A menu will appear, but it should not. Fix: add an extra / at the beginning of the expression.

Bug 2: regex/^Makefile.(PL|pl)$

This expression will NEVER be matched. Reproducer:

a) echo 'print "foo\n"' > Makefile.PL
b) position the cursor on "Makefile.PL"
c) press Enter.

Nothing will happen. Expected result: the file should be executed through perl. Fix: replace ^ with /.

comment:10 follow-up: ↓ 11 Changed 8 years ago by and

mc.ext shell documentation is unclear

shell/.myfile
will match all filenames with .myfile suffix (special handling code about '.')

shell/myfile
will only match when fullpathname is 'myfile' (will never be true)

shell//tmp/myfile
will work for /tmp/myfile but this is not what we want

new suffix word should be introduces for easy right hand matching rules

Last edited 8 years ago by and (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 8 years ago by zaytsev

Replying to and:

new suffix word should be introduces for easy right hand matching rules

I believe that my solution (regex//myfile$) doesn't have any serious disadvantages, so a new suffix is not really necessary. What do you think, am I missing something?

comment:12 Changed 8 years ago by and

Your solution is ok for the two cases (I'm unsure about ls-lR stuff)
but there are more cases that will not work as wanted
all shell/ rules without '.' definition are suspicious

$ grep -P '^shell/(i/|)?+[^.]' mc.ext 
shell/read.me
shell/Imakefile

comment:13 Changed 8 years ago by zaytsev

Hi Andreas, you are right, thanks! I've pushed another commit, what do you think?

comment:14 in reply to: ↑ description Changed 8 years ago by andrew_b

Replying to zaytsev:

because the paths tested are absolute, not relative;

I think that is the bug. mc.ext should get file name only, not full path.

my suggested fix is to prepend a slash.

User can be confused with that.

Bug with full name in mc.ext is fixed in the last commit of the branch.

comment:15 Changed 8 years ago by zaytsev

Andrew, thank you for your help. Cleaned up everything, rebased against master. OK?

New initial changeset: e7bdd9.

comment:16 Changed 8 years ago by andrew_b

  • Votes for changeset set to andrew_b

comment:17 Changed 8 years ago by zaytsev

  • Branch state changed from on review to approved

comment:18 Changed 8 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:19 Changed 8 years ago by zaytsev

  • Status changed from testing to closed

comment:20 Changed 8 years ago by andrew_b

Don't forget to remove merged branches.

comment:21 Changed 8 years ago by zaytsev

Ok, sorry.

Note: See TracTickets for help on using tickets.