Ticket #147 (closed defect: fixed)

Opened 11 years ago

Last modified 2 years ago

Fix escaping for completion on cmdline (spaces, backslashes, etc).

Reported by: Patrick Winnertz <winnie@…> Owned by: winnie
Priority: major Milestone: 4.6.2
Component: mc-core Version: 4.6.1
Keywords: vote-angel_il vote-winnie committed-master committed-mc-4.6 Cc:
Blocked By: #157 Blocking:
Branch state: Votes for changeset:

Description (last modified by winnie) (diff)

Hey,

This patch will fix problems with escaping on cmdline basically. The patch is
not perfect and should, at least, partially reworked.

Greetings
Winnie

Attachments

61_escaping.patch (4.5 KB) - added by Patrick Winnertz 11 years ago.
Added by email2trac
escaping-patch-rev2.patch (4.8 KB) - added by Patrick Winnertz 11 years ago.
Added by email2trac
part0001.pgp (197 bytes) - added by Patrick Winnertz 11 years ago.
Added by email2trac
escaping-patch-rev3.patch (4.8 KB) - added by winnie 11 years ago.
Use now g_str_append_c and use ascii values to sort symbols
escaping-patch-rev4.patch (5.3 KB) - added by slavazanko 11 years ago.
fix some memory leaks. WARNING: patch don't allyped to '147_escaping' branch
147_escape_use_MHL.patch (5.9 KB) - added by slavazanko 11 years ago.

Change History

Changed 11 years ago by Patrick Winnertz

Added by email2trac

comment:1 Changed 11 years ago by Patrick Winnertz

  • id set to 147

This message has 1 attachment(s)

comment:2 Changed 11 years ago by Patrick Winnertz

  • Keywords review added
  • Status changed from new to accepted
  • Owner set to winnie

I'll work on this..

please review the patch which is attached to this ticket

comment:3 Changed 11 years ago by Patrick Winnertz

  • Blocking 149 added

(In #149) Sadly it's not possible to set the stuff without writing a bit or in the first
mail:

I'll work on this and this should be fixe in 4.6.2.

comment:4 Changed 11 years ago by winnie

  • Description modified (diff)
  • Milestone changed from 4.7 to 4.6.2

comment:5 Changed 11 years ago by winnie

  • Keywords rework added; review removed

Maybe it's a good idea to try to use

g_strescape and g_strcompress instead of home-made functions...

comment:6 Changed 11 years ago by Patrick Winnertz

  • Keywords review added; rework removed

Hey,

Maybe it's a good idea to try to use

g_strescape and g_strcompress instead of home-made functions...

I tried it with these functions.. unfortunatly g_strescape doesn't escape the
whitespace .. therefore this function is useless for us.

I rewrote the patch a little bit to escape everything even if it's the leading
char.

Changed 11 years ago by Patrick Winnertz

Added by email2trac

Changed 11 years ago by Patrick Winnertz

Added by email2trac

comment:7 Changed 11 years ago by Patrick Winnertz

Hey Roland,

See the branch this commit belongs too, and you'll see that this belongs to a
patch where some rework is needed. :) See ticket:147.

In order to get your remark not only onto this list but also into the ticket
'm replying not the the list but to the ticketsystem which will then forward
the mail back to the list.

I don't get why there has to be a variable k in the functions. Its
completely unnecessary. A much simpler, faster, and more reliable way is
the following (untested):

I'll test it and merge it into the bugfix branch.

char *
unescape_string(const char *in) {

GString *str;
const char * src;
char *result;

str = g_string_new("");

for (src = in; *src != '\0'; src++) {

if (src[0] == '
' && strchr(" \t*|;<>~#()?[]{}&", src[1])) {

g_string_append_c(str, src[1]);
src++;

} else {

g_string_append_c(str, src[0]);

}

}

result = str->str;
g_string_free(str, FALSE);
return result;

}

It's much simpler that way. It doesn't call strlen() unnecessarily. It
doesn't have unneeded variables. And it doesn't crash if in[-1] is an
illegal address.

Well It is indeed much simpler.. but do you tested this patch once? mc won't
crash...(tested).

Greetings
Winnie

Version 0, edited 11 years ago by Patrick Winnertz (next)

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

In the unescape_string function, there is no need to check for (i == 0). Otherwise the first character of the string may be skipped, which I think is not intended.

Why do the following characters do not need escaping:?

$ `

and why aren't the characters sorted according to their ASCII values?

Roland

comment:9 Changed 11 years ago by Patrick Winnertz

Hey,

Why do the following characters do not need escaping:?

$ `

Added.

and why aren't the characters sorted according to their ASCII values?

This is done now.

Please note that I had not much time to rewrite the escape_string function in
order to use g_str_append_c. It uses atm a negative index for a check
(src[-1]). It looks ugly but it doesn't fail. Please send a patch if you have
a better idea for this ifclause.

Greetings
Winnie

comment:10 Changed 11 years ago by winnie

  • Blocking 10 added

(In #10) There is a fix in the 10_fish_stalls_on_symlink branch.. however this fix is uncomplete as it will fail in this scenario:

echo "ALL OK" >"this -> file"
ln -s "this -> file" "this -> link"

The link will point to -> file instead of "this -> file".

To fix this issue we need quoted output. (This means ls -Q). As this is no standard option but only used in gnu ls we have to check prior using it if this option is available.
However this is related to ticket:149.

Changed 11 years ago by winnie

Use now g_str_append_c and use ascii values to sort symbols

comment:11 Changed 11 years ago by slavazanko

  1. run mc with patch
  2. type cd ~ and press twice ESC,TAB
  3. select from list your homedir (\~slavaz/ in my case)
  4. press twice ESC,TAB

no any reaction (only bells).

Without patch continued to navigate:

  1. select from list subdir in your homedir
  2. press twice ESC,TAB
  3. ...

After remove line src/complete.c:947 (see escaping-patch-rev3.patch) navigate to subdirs worked.

comment:12 Changed 11 years ago by slavazanko

BTW:

diff --git a/src/complete.c b/src/complete.c
index a11b9c9..cb18f42 100644
--- a/src/complete.c
+++ b/src/complete.c
@@ -944,7 +944,9 @@ complete_engine (WInput *in, int what_to_do)
            WListbox *query_list;

            for (p=in->completions + 1; *p; count++, p++) {
-           *p = escape_string(*p);
+               q = escape_string(*p);
+               g_free(*p);
+               *p = q;
                if ((i = strlen (*p)) > maxlen)
                    maxlen = i;
                        }

I think, this remove some memory leaks...

And need to review some other parts in patch. For example src/file.c:1876

source_with_path = unescape_string(source_with_path); 

I think, in any case need to free memory by old pointer. Example:

char *old_pnt;

...
old_pnt = source_with_path;
source_with_path = unescape_string(source_with_path); 
g_free(old_pnt);

and others like this...

Changed 11 years ago by slavazanko

fix some memory leaks. WARNING: patch don't allyped to '147_escaping' branch

comment:13 Changed 11 years ago by slavazanko

fix for attachment comment:

- allyped
+ applyed

:)

Next testing stuff:

  1. press F7 (make dir)
  2. type: this dir
  3. press SHIFT+F6 (rename)
  4. change 'this dir' to 'this \ dir'

Got error.

  1. press SHIFT+F6 (rename) again
  2. change 'this dir' to 'this
    dir'

Got error.

comment:14 Changed 11 years ago by slavazanko

duplicate #152

comment:15 Changed 11 years ago by metux

  • Blocked By 157 added

comment:16 Changed 11 years ago by winnie

  • Keywords rework added; review removed

This needs a rework by me after #157 is merged.

comment:17 Changed 11 years ago by slavazanko

Well... Now in '147_escaping' fixed completion bug, but behaviour present with renaming 'this dir' to 'this \ dir'.

Please, test this branch and say your thinks here.

comment:18 Changed 11 years ago by slavazanko

  • Keywords review added; rework removed

Well.. Bug with renaming/copying fixed. See git-log and git-diff in branch '147_escaping'.

Please, review and vote.

comment:19 Changed 11 years ago by winnie

  • Keywords rework added; review removed

This is still based on the functions escape_string & unescape_string.. This is now integrated into mhl, please use this stuff.

comment:20 Changed 11 years ago by winnie

  • Blocking 10 removed

(In #10) removed #147 as blocking bug. the escape functions are now provided inside the mhl lib which was addressed in #157

comment:21 Changed 11 years ago by winnie

  • Blocking 149 removed

remove #149 as blocked bug as #149 needs only the escaping functions which are now provides by the mhl lib which was added in #157

comment:22 Changed 11 years ago by slyfox

yeah, mhl stuff would reduce this patch a _lot_

Changed 11 years ago by slavazanko

comment:23 Changed 11 years ago by slavazanko

  • Keywords review added; rework removed

See 147_escape_use_MHL.patch - just apply to HEAD of 147_... branch
review, please.

comment:24 Changed 11 years ago by slavazanko

patch 147_escape_use_MHL.patch applied to '147' branch.

Review, test and vote, please.

comment:25 Changed 11 years ago by winnie

  • Keywords rework added; review removed

I've still some troubles with this patch.. e.g have a look on this testscenario:

  • create /tmp/test
  • in /tmp/test/ (touch them):
    -rw-r--r-- 1 winnie winnie 0 27. Jan 20:35 " file"
    -rw-r--r-- 1 winnie winnie 0 27. Jan 20:35 " file$test"
    -rw-r--r-- 1 winnie winnie 0 27. Jan 20:35 " file&test"
    

Now type on subshell:

less /tmp/test/

and press twice ESC-tab. Normally a completion dialog should open but this doesn't happen here. (You can verify this with a simple test):

less /tmp/

and now press twice ESC-tab.

So.. this needs to be fixed.

comment:26 Changed 11 years ago by metux

I've created some dir "/tmp/ foo" and opened the copy dialog.
Typing in /tmp and pressing esc-tab twice shows up " foo" properly, and selecting it works fine.

BUT: typing in "/tmp/ " and pressing esc-tab twice shows my homedir in the list.

comment:27 Changed 11 years ago by metux

Found out that the filename_completion_function doesnt get the leading spaces passed
(create some dir "/tmp/ foo" and try to expand "/tmp ")

comment:28 Changed 11 years ago by slyfox

  • Keywords review added; rework removed

Partially reworked completion engine: adedd INPUT_COMPLETION_FLAGS state to all completion functions, which helps to separate completion contexts.

147_escaping fixes:

  • completion of regular files after command:

    -rw-r--r-- 1 winnie winnie 0 27. Jan 20:35 " file"
    -rw-r--r-- 1 winnie winnie 0 27. Jan 20:35 " file$test"
    -rw-r--r-- 1 winnie winnie 0 27. Jan 20:35 " file&test"
    $ less /tmp/<complete>
    $ less ./<complete>

  • cd command now acts as \cd (accepts escaped dirs)

comment:29 Changed 11 years ago by angel_il

  • Keywords vote-angel_il added; review removed

it works

comment:30 Changed 11 years ago by winnie

  • Keywords vote-winnie approved added

Okay.. branch works very fine :) Vote from me

comment:31 Changed 11 years ago by slyfox

  • Keywords committed-master committed-mc-4.6 added; approved removed
  • Status changed from accepted to testing
  • Resolution set to fixed

comment:32 Changed 11 years ago by winnie

  • Status changed from testing to closed

Okay finally closing this bug :) This was the last showstopper for 4.6.2

Note: See TracTickets for help on using tickets.