Ticket #2327 (closed defect: fixed)

Opened 6 years ago

Last modified 6 years ago

Can't put ? into a new name

Reported by: gotar Owned by: slavazanko
Priority: critical Milestone: 4.7.5
Component: mc-core Version: master
Keywords: Cc: zaytsev
Blocked By: Blocking:
Branch state: Votes for changeset: commited-master

Description (last modified by andrew_b) (diff)

It's not possible to rename a file to something having question mark. Literal ? is being replaced with entire pattern just like it was * (an asterisk), despite of documentation saying nothing about ? in target pattern:

"In the target mask only the '*' and '\<digit>' wildcards are allowed"

One more funny effect of this issue is duplication of names having single '?': F6, enter
However it's getting more serious when source file has multiple '?'s: F6, enter -> segfault.
(for the above to happen question marks must not be preceded by *).

The source of this problem is treating ? as * (and not escaping it in default target pattern).

Escaping itself is buggy too: \? or \* are translated to consecutive \1, \2, \3, etc.

Everyting with shell patterns on, as disabling them makes rename impossible, but that's for next ticket.

This ticket may be duplicate, I remembering chasing this issue some time ago but can't find it in trac now, so sorry if it's already reported.

Change History

comment:1 Changed 6 years ago by andrew_b

comment:2 follow-up: ↓ 14 Changed 6 years ago by andrew_b

Would you test the recent master with fixed #2123?

comment:3 follow-up: ↓ 4 Changed 6 years ago by gotar

It's still broken, but in different way:

  1. renaming discards '?' (e.g. one gets 'akljdjkasd' while renaming to 'akljd?j?kasd'),

1a. renaming to '?' renames to '\',

  1. renaming 'd?d' to 'd?d' gives 'Cannot move directory [...] No such file or directory (2)' instead 'Invalid argument (22)' like in case of names without '?' (as for trying to move directory into itself).
  2. '*' in target mask is now broken - get's discarded the same way '?' is.

Due to the last one current state is absolutely useless.

comment:4 in reply to: ↑ 3 Changed 6 years ago by gotar

  • Priority changed from minor to major
  • Milestone changed from 4.7 to 4.7.5

Replying to gotar:

  1. '*' in target mask is now broken - get's discarded the same way '?' is.

Due to the last one current state is absolutely useless.

As you must have not read this issue and released 4.7.4, I'm rising priority and setting milestone to next release. Maybe another ticket should be opened - the serious problem now is with *, still ? from summary is minor.

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

  • Cc zaytsev added
  • Version changed from 4.7.3 to master

Hi!

The releases happen on schedule unless there are some real super critical things. This one has been broken for awhile so doesn't make sense to rush and get it wrong again.

The other ticket is about crash. The crash does not happen anymore. So I'd prefer to track all the bugs concerning shell patterns in this ticket.

I also wanted to report the problems with treating ? as *, but I didn't test thoroughly for other issues. As it turns out, right now, it's completely broken.

comment:6 in reply to: ↑ 5 Changed 6 years ago by gotar

  • Priority changed from major to critical

Replying to zaytsev:

The releases happen on schedule unless there are some real super critical things. This one has been broken for awhile so doesn't make sense to rush and get it wrong again.

*-issue is brand new, it didn't happen before 4.7.4 (only '?' did). And I've just checked what exactly happens:

~/mc-test: echo a > a
~/mc-test: echo b > b
~/mc-test: echo c > c

F6: * -> *1
"[...] are the same file"

OK, nothing too bad here, just no files were renamed.

F6: * -> *d

"Target file already exists! ./d" "Overwrite this target?"

Oops, default choice was 'Yes' and I pressed enter, but the dialog remains unchanged. But wait... this is dialog for file 'c'. exit exit exit

~/mc-test: cat *
c
b

~/mc-test: ls
c d

Oh, I see - 'a' -> 'd', 'b' -> 'd' (first overwrite dialog), 'c' didn't overwrite again only because I've escaped mc.

Raising this to critical - using asterisk equals n-1 file overwrite dialogs (and so possible data loss).

comment:7 follow-up: ↓ 8 Changed 6 years ago by gotar

BTW it's funny, because now, when shell patterns don't work here, and regular patterns don't work in #2328, mc is totally incapable of renaming more than 1 file. So basically only PanelRenameLocal (shift-f6) and move operations are available.

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

Replying to gotar:

regular patterns don't work in #2328,

The alone * is invalid regexp. Try .*. But anyway, the error message must be raised in this case.

comment:9 in reply to: ↑ 8 ; follow-up: ↓ 12 Changed 6 years ago by gotar

Replying to andrew_b:

regular patterns don't work in #2328,

The alone * is invalid regexp. Try .*. But anyway, the error message must be raised in this case.

According to documentation * is valid in target mask (where I've tried * and positional \1-9). So far I haven't managed to get ANY renaming without shell patterns, I have no idea which - source or target, pattern doesn't match. Did you try this?

comment:10 follow-up: ↓ 11 Changed 6 years ago by zaytsev

I think the documentation has to be fixed :) The reason why all these bugs exists is that someone was trying to be smart and convert the patterns to regexps and regexps to patterns automatically. Now we suffer the consequences.

comment:11 in reply to: ↑ 10 Changed 6 years ago by gotar

Replying to zaytsev:

I think the documentation has to be fixed :)

If it is misdocumentation - maybe someone gives me here some working example? Anyone? I've tried dozens of patterns, none of them worked (and it's PITA to uncheck 'Use shell patterns' over and over again).

comment:12 in reply to: ↑ 9 ; follow-up: ↓ 13 Changed 6 years ago by andrew_b

Replying to gotar:

Replying to andrew_b:

regular patterns don't work in #2328,

The alone * is invalid regexp. Try .*. But anyway, the error message must be raised in this case.

According to documentation * is valid in target mask (where I've tried * and positional \1-9). So far I haven't managed to get ANY renaming without shell patterns, I have no idea which - source or target, pattern doesn't match. Did you try this?

I'm talking about source mask in #2238. The * source mask is invalid regexp but valid shell pattern. This the root of #2238.

comment:13 in reply to: ↑ 12 Changed 6 years ago by gotar

Replying to andrew_b:

According to documentation * is valid in target mask

I'm talking about source mask in #2238. The * source mask is invalid regexp but valid shell pattern. This the root of #2238.

No, once again: TARGET mask. I'm not using bare * in source mask, please read #2328 carefully and just peek into help: it DOES mention dot-asterisk notation and I'm aware of it (as perl is my primary language). It simply does not work.
If you have any working example - paste it for me please, but first test if that works for you.

comment:14 in reply to: ↑ 2 Changed 6 years ago by gotar

Replying to andrew_b:

Would you test the recent master with fixed #2123?

OK, once again, the 4.7.4 with shell patterns ON, summarizing:

  • * and ? in target patterns are ignored,
  • \N backreferences work fine

while:

  • * should be interpreted as consecutive \N (N=1..n) backreferences (just like in 4.7.3 - that has been broken recently),
  • ? should be left literal (in 4.7.3 was treated like * which was wrong too).

No documentation needs to be fixed here, it's wrong code.

comment:15 Changed 6 years ago by andrew_b

comment:16 Changed 6 years ago by vda

The bug is caused by extra "continue" statement in case '*'/case '?' branch.
As a result, \ is added to converted string, but following N is not.
Example: when one renames many files using "*" to "a*z", "a*z" gets converted to "a\z" instead of correct "a\1z".

Basically, the fix is:

@@ -144,11 +146,8 @@ mc_searchtranslate_replace_glob_to_reg

{

g_string_append_c (buff, '
');
c = ++cnt;

  • continue;

}
break;

  • /* breaks copying: mc uses "\0" internally, it must not be changed */
  • /*case '
    ': */

case '&':

g_string_append_c (buff, '
');
break;

It also removes an obsolete commented out case '
' code (there is now another, non-commented-out case '
' code a dozen lines up).

comment:17 Changed 6 years ago by slavazanko

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

comment:18 Changed 6 years ago by slavazanko

  • severity changed from no branch to on review

Created branch 2327_cant_put_question_mark (parent: master)
Changeset:822c774d2b116818a49ee361f99685f4a6e5d372

review, please.

comment:19 Changed 6 years ago by slavazanko

  • Votes for changeset set to slavazanko

comment:20 Changed 6 years ago by andrew_b

  • Keywords stable-candidate added
  • Votes for changeset changed from slavazanko to slavazanko andrew_b
  • severity changed from on review to approved
  • Description modified (diff)

comment:21 in reply to: ↑ description Changed 6 years ago by zaytsev

Why change the description removing the blank lines??? It's hard to read this way :-(

comment:22 Changed 6 years ago by andrew_b

  • Description modified (diff)

Sorry, it was ziproxy's work. I didn't change description on purpose.

comment:23 Changed 6 years ago by zaytsev

Got it. I thought you did it on purpose. Sorry & thanks.

comment:24 Changed 6 years ago by slavazanko

  • Status changed from accepted to testing
  • Votes for changeset changed from slavazanko andrew_b to commited-master
  • Resolution set to fixed
  • severity changed from approved to merged

comment:25 Changed 6 years ago by andrew_b

  • Status changed from testing to closed
  • Keywords stable-candidate removed
Note: See TracTickets for help on using tickets.