savannah: tab completion vs. spaces and escaping

[mc-butler]

Important

This issue was migrated from Trac:

Origin	https://midnight-commander.org/ticket/55
Reporter	egmont (@egmontkob)
Mentions	ossi (@ossilator), egmont@….com (@egmontkob)

Original: http://savannah.gnu.org/bugs/?18695

Submitted by:	Egmont Koblinger <egmont>	Submitted on:	Fri 05 Jan 2007 01:07:06 PM UTC
Category:	None	Severity:	3 - Normal
Status:	None	Privacy:	Public
Assigned to:	None	Open/Closed:	Open
Release:	current (CVS or snapshot)	Operating System:	GNU/Linux

Original submission:

Tab completion (Alt+Tab or Escape+Tab, I'll refer to these simply 
as Tab) works exactly the same way in the command line and in 
dialog boxes (such as copy/move file). The function complete() in 
src/complete.c only takes a widget as its input and performs the 
completion.

This approach is wrong, since the dialog boxes and the command line 
have different word splitting and escaping rules, they cannot be 
treated the same way. Currently they both have bugs. (I found these 
while trying to tidy up my mp3s, where lots of file and directory 
names contain spaces.)

In the file copy and similar dialogs, filenames are typed as pure 
strings without any kind of escaping, and a text entry only takes 
one filename. However, splitting at spaces is still performed. 
Suppose I have a directory named "/tmp/a bcde". I press F5/F6 on a 
file, and type "/tmp/a" and then Tab, it completes to "/tmp/a 
bcde". This is okay. But if I type "/tmp/a bc" and press Tab now, 
it completes to the files in the current directory whose name 
begins with "bc", due to the splitting at the space. Similarly, if 
I already have "/tmp/a bcde/" in the edit field, it won't complete 
to subdirectories or files of this directory. In this case the 
whole content of the field should be treated as one, not being 
split at spaces.

In the command line, splitting at spaces is okay, however, we would 
need some escaping and unescaping too, which doesn't happen. (Note: 
Esc followed by Enter correctly escapes the filename when it puts 
it into the cmdline.) Example: type "ls /tmp/a" and then Tab, 
you'll have "ls /tmp/a bcdefgh/" in the command line, which clearly 
won't list the contents of that directory due to the unescaped 
space character. Another example: type "ls /tmp/a\ b" and hit Tab, 
it won't complete to "ls /tmp/a\ bcde/", but it should.

I understand that it is beyond the scope of mc to perform a full 
shell syntax parsing (single and double quotes, nesting them etc.). 
This doesn't even happen when Esc Enter is pressed: backslashes are 
inserted even between quotes, but I'm fine with that. However, mc 
should produce proper escaping when inserting spaces or other 
special chars as a result of completion, and should be able to 
parse the backslash escaping it created (or the user typed) when 
doing further completion.

Obviously the current design of the complete() function to take 
only a widget is not okay (unless there's some room in the widget 
to store splitting and escaping rules). There are two main possible 
solutions I can see, but probably they'd eventually lead to 
basically the same code.

1) modify complete() so that it receives information on word 
splitting and escaping, so that it becomes more complex and 
probably more buggy and harder to maintain, but solves everything 
we need.

2) remove the word splitting code from complete(), so that it 
becomes simpler and perfect for the file copy/move widgets. 
Slightly modify its interface to take and return string and cursor 
positions instead of widget. Then make the command line editor 
split at unescaped spaces, unescape the resulted word, call 
complete() on this temporary string, escape the result of 
completion and put back in the command line.

Finally, we should check how the internal "cd" command handles its 
arguments. Currently it seems to do a lot of heuristics. For 
example, if I type "cd a\\\\b" (four backslashes) then it tries to 
change to the directory called "a\\\\b" (four backslashes) but if 
there's no such directory, it changes to "a\\b" (two backslashes). 
This seems to be terrible, and leads to bugs, e.g. if the cursor is 
on "a\\b" and I type "cd " followed by a Tab, I get the command "cd 
a\\\\b" and hence it'll change to "a\\\\b" which is not the desired 
directory. I'd remove all the heuristics and perform the usual 
command line unescaping, just as if it was an external command. 

Comment 1 by Pavel Tsekov <ptsekov> at Fri 05 Jan 2007 01:53:04 PM UTC:

Sounds familiar...

http://savannah.gnu.org/bugs/?16176

[mc-butler]

Changed by slavazanko (@slavaz) on Dec 25, 2008 at 22:32 UTC (comment 1)

Related ticket: #41

[mc-butler]

Changed by winnie (@winnieXY) on Jan 5, 2009 at 14:18 UTC (comment 2)

• Milestone set to VFS Standardisation

[mc-butler]

Changed by metux (@metux) on Jan 9, 2009 at 20:22 UTC

• Blocked by set to #157

[mc-butler]

Changed by andrew_b (@aborodin) on Jun 21, 2010 at 7:32 UTC (comment 4)

• Version set to master
• Severity set to no branch
• Blocked by #157 deleted

[mc-butler]

Changed by ossi (@ossilator) on Jul 11, 2010 at 13:01 UTC (comment 5)

• Cc set to ossi@….org

[mc-butler]

Changed by andrew_b (@aborodin) on Jan 8, 2011 at 14:14 UTC

• Blocking set to #2450

[mc-butler]

Changed by slavazanko (@slavaz) on Dec 19, 2011 at 21:26 UTC (comment 7)

• Status changed from new to accepted
• Branch state set to no branch
• Owner set to slavazanko

[mc-butler]

Changed by slavazanko (@slavaz) on Dec 19, 2011 at 21:34 UTC (comment 8)

• Cc changed from ossi@….org to ossi@….org, egmont
• Branch state changed from no branch to on review

Created branch 55_filename_complete (parent: master)
Initial 55_filename_complete

Review, please.

[mc-butler]

Changed by andrew_b (@aborodin) on Jan 10, 2012 at 7:13 UTC (comment 9)

• Branch state changed from on review to on rework

[mc-butler]

Changed by andrew_b (@aborodin) on Jan 11, 2012 at 7:48 UTC

• Blocking changed from #2450 to #2450, #2626

[mc-butler]

Changed by slavazanko (@slavaz) on Apr 2, 2012 at 9:08 UTC (comment 11)

• Branch state changed from on rework to on review
• Milestone changed from 4.8 to 4.8.3

[mc-butler]

Changed by ossi (@ossilator) on Apr 5, 2012 at 7:00 UTC (comment 12)

in lib/widget/input_complete.c, the tab char should also be treated like that. you can insert a tab with ctrl-q tab easily, even if this is not really useful in the normal case.

INPUT_COMPLETE_FILES_ESC sounds like a misnomer to me. at the level of the input api it doesn't relate to files at all (or at least it shouldn't - i can't see from the patch alone whether that's actually the case). INPUT_COMPLETE_SPACE_ESC sounds like a better name.

[mc-butler]

Changed by slavazanko (@slavaz) on Apr 5, 2012 at 10:28 UTC (comment 13)

Ossi, your wishes have been realized in temporary changesets dac8b78fb [dac8b78fb53c76ce6d018f41cc679c97c83aef28] and b530609cb [b530609cb335d13c04bd9169cf2a21d06c29f7d2]. Thanks.

[mc-butler]

Changed by ossi (@ossilator) on Apr 6, 2012 at 10:20 UTC (comment 14)

actually, the logic in lib/widget/input_complete.c is broken. i think it should be more or less like this:

// no space, no tab here.
// XXX this list seems pretty arbitrary to me. if shell, missing at least: ()&
if (strchr (";|<>", *s) != NULL)
    break;
if ((*s == 32 || *s == 9) &&
    (!(in->completion_flags & INPUT_COMPLETE_SPACE_ESC) ||
     !strutils_is_char_escaped (in->buffer, s)))
    break; 

[mc-butler]

Changed by slavazanko (@slavaz) on May 3, 2012 at 8:38 UTC (comment 15)

• Branch state changed from on review to on rework

[mc-butler]

Changed by slavazanko (@slavaz) on May 4, 2012 at 13:49 UTC (comment 16)

• Milestone changed from 4.8.3 to 4.8.4
• Branch state changed from on rework to on review

rebased to latest master and have respected ossi's thoughts.

Review, please.

[mc-butler]

Changed by ossi (@ossilator) on May 17, 2012 at 10:33 UTC (comment 17)

a recommendation for the commit message: use the past tense for the broken things that your commit fixes. it makes no sense to describe *some* state in the present tense, because it is not clear whether that's the new or the old state.

your re-definition of INPUT_COMPLETE_DEFAULT makes it a misnomer - it's more like INPUT_COMPLETE_NONE now.

anyway, the more i look into this code, the less i understand. what exactly is the above fragment supposed to do? the "stop chars" in the strchr are applied irrespective of mode, which makes no sense to me at all. what is INPUT_COMPLETE_SPACE_ESC meant to actually do? as far as i can see, there are only two modes: plain (regular line inputs) and shell-like (command prompt. fwiw, the built-in cd should just pretend to be a regular cd command (i.e., follow the normal shell splitting and quoting rules), then there would be no ambiguity). i can't find the code which is supposed to quote completions for the command prompt - am i or the code missing something?

try_complete() contains a lot of fuzzy logic, in particular with backslash escaping being handled rather arbitrarily or not at all. strutils_is_char_escaped() should be consistently applied, with regard to the mode.

INPUT_COMPLETE_DIRNAMES needs to be split out from INPUT_COMPLETE_CD, otherwise the "cd" detection magic is activated outside command prompt context as well.

generally, i'd suggest that you *precisely* describe the intended semantics of the INPUT_COMPLETE_* enum values next to their declarations, and then match these descriptions against both the implementation and the usages.

wow. two hours gone.

[mc-butler]

Changed by slavazanko (@slavaz) on May 22, 2012 at 14:29 UTC (comment 18)

• Branch state changed from on review to on rework

a recommendation for the commit message: use the past tense for the broken things that your commit fixes. it makes no sense to describe *some* state in the present tense, because it is not clear whether that's the new or the old state.

Thank for explanation, my English isn't good, and I accept all lessons and criticism.

your re-definition of INPUT_COMPLETE_DEFAULT makes it a misnomer - it's more like INPUT_COMPLETE_NONE now.

Yep, you right, INPUT_COMPLETE_NONE have more sense now. I changed the name of constant.

All your other notes I have to take a closer look in the evening with a relaxing cup of tea. So, ticket going to 'rework' stage.

wow. two hours gone.

But I hope, you got a fun in this two hours, because a fun it an engine of OpenSource (in most cases) :)

[mc-butler]

Changed by andrew_b (@aborodin) on Jul 18, 2012 at 16:58 UTC (comment 19)

• Milestone changed from 4.8.4 to 4.8.5

[mc-butler]

Changed by andrew_b (@aborodin) on Oct 9, 2012 at 11:01 UTC (comment 20)

• Milestone changed from 4.8.5 to Future Releases

[mc-butler]

Changed by slavazanko (@slavaz) on Jan 29, 2013 at 12:04 UTC (comment 21)

• Branch state changed from on rework to on review
• Milestone changed from Future Releases to 4.8.8

Branch was recreated. See 55_filename_complete [5b3bddeb2c1fe52180c36d0a133d77508a26af9c] for details. Review & vote, please.

[mc-butler]

Changed by andrew_b (@aborodin) on Jan 31, 2013 at 10:25 UTC (comment 22)

• Votes set to andrew_b

[mc-butler]

Changed by angel_il (@ilia-maslakov) on Feb 4, 2013 at 11:02 UTC (comment 23)

• Votes changed from andrew_b to andrew_b angel_il
• Branch state changed from on review to approved

[mc-butler]

Changed by slavazanko (@slavaz) on Feb 4, 2013 at 12:53 UTC (comment 24)

• Blocking #2450, #2626 deleted
• Branch state changed from approved to merged
• Votes changed from andrew_b angel_il to committed-master
• Resolution set to fixed
• Status changed from accepted to testing

merged to master:

git log --pretty=oneline 74d71e7..0608af2

[mc-butler]

Changed by slavazanko (@slavaz) on Feb 4, 2013 at 12:57 UTC (comment 25)

• Status changed from testing to closed

[mc-butler]

Changed by ossi (@ossilator) on Jan 11, 2014 at 16:59 UTC (comment 26)

• Description edited
• Cc changed from ossi@….org, egmont to ossi
• Reporter changed from slavazanko to egmont

[mc-butler]

Changed by egmont (@egmontkob) on Jan 11, 2014 at 23:39 UTC (comment 27)

• Cc changed from ossi to ossi, egmont@….com

[mc-butler]

Changed by zaytsev (@zyv) on Feb 20, 2025 at 15:37 UTC (comment 28)

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