Ticket #3578 (closed defect: fixed)
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 9 years ago by andrew_b
comment:3 follow-up: ↓ 6 Changed 9 years ago by zaytsev
- Owner set to zaytsev
- Status changed from new to accepted
- Branch state changed from no branch to on review
Branch: 3578_mcext_fixes.
Initial changeset: [cb146b74237aeae7acb96dde94ea3e655c6b84d5].
Please review!
comment:5 Changed 9 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 9 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 9 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:8 in reply to: ↑ description Changed 9 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 9 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 9 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
comment:11 in reply to: ↑ 10 Changed 9 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 9 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 9 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 9 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 9 years ago by zaytsev
Andrew, thank you for your help. Cleaned up everything, rebased against master. OK?
New initial changeset: e7bdd9.
comment:18 Changed 9 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
Merged to master: [f74754c03de399b372e884c691ad1ea4b156e82c].
comment:20 Changed 9 years ago by andrew_b
Don't forget to remove merged branches.
comment:21 Changed 9 years ago by zaytsev
Ok, sorry.
Replying to zaytsev:
#3115