Notes |
(0001880)
vda
12-18-06 17:10
|
The patch is buggy. It won't free() and NULL identical matches if you have more than 2 of them in a row. [Patch is also severely whitespace-damaged and doesn't match bbox style.]
But still, thanks... better than nothing. |
| |
(0001881)
vda
12-18-06 17:11
|
Fixed in rev 17003. |
| |
(0001882)
Marty
12-19-06 04:07
|
Hi Denis,
Thank you for catching the oversight in my patch. I am not hugely concerned about whitespace or style, but at getting the algorithm correct and fast.
Your patch contains one very important bug, in that, on my system at least, qsort doesn't actually sort the array. I have modified my patch to fix this problem, and also to collapse all three loops in this area of code into a single loop. I also modified 'showfiles' to reduce the amount of IO.
New patch is:
Index: shell/cmdedit.c
===================================================================
--- shell/cmdedit.c (revision 17004)
+++ shell/cmdedit.c (working copy)
@@ -989,18 +989,16 @@
for(nc = 1; nc < ncols && n+nrows < nfiles; n += nrows, nc++) {
str_add_chr[0] = add_char_to_match[n];
acol = str_add_chr[0] ? column_width - 1 : column_width;
- printf("%s%s", matches[n], str_add_chr);
- l = strlen(matches[n]);
- while (l < acol) {
- putchar(' ');
- l++;
- }
+ printf("%-*s", acol, matches[n] );
}
str_add_chr[0] = add_char_to_match[n];
printf("%s%s\n", matches[n], str_add_chr);
}
}
-
+static int match_compare(const void *a, const void *b)
+{
+ return strcmp(*(char **) a, *(char **) b);
+}
static void input_tab(int *lastWasTab)
{
/* Do TAB completion */
@@ -1045,35 +1043,26 @@
#endif
/* Try to match any executable in our path and everything
* in the current working directory that matches. */
- exe_n_cwd_tab_completion(matchBuf, find_type);
- /* Remove duplicate found and sort */
+ exe_n_cwd_tab_completion(matchBuf, find_type);
+ /* Sort, then remove any duplicates found */
if (matches) {
- int i, n;
- /* strcmp is int(*f)(const char*, const char*) */
- /* qsort wants int(*f)(const void*, const void*) */
- /* We cheat here :) */
- qsort(matches, num_matches, sizeof(char*), (void*)strcmp);
- i = 0;
- while (i < num_matches - 1) {
- n = i + 1;
- if (matches[i] && matches[n]) {
- while (n < num_matches
- && !strcmp(matches[i], matches[n])) {
- free(matches[n]);
- matches[n] = 0;
- n++;
- }
- }
- i = n;
- }
- n = 0;
- for(i = 0; i < num_matches; i++)
- if (matches[i]) {
- matches[n] = matches[i];
- add_char_to_match[n] = add_char_to_match[i];
- n++;
- }
- num_matches = n;
+ int i, n = 0;
+ qsort(matches, num_matches, sizeof(char*), match_compare);
+ for ( i = 0 ; i < num_matches -1 ; ++i ) {
+ if ( matches[i] != NULL && matches[i+1] != NULL ) {
+ if ( 0 == strcmp ( matches[i], matches[i+1] ) ) {
+ free ( matches[i] );
+ matches[i] = 0;
+ }
+ else {
+ add_char_to_match[n] = add_char_to_match[i];
+ matches[n++] = matches[i];
+ }
+ }
+ }
+ add_char_to_match[n] = add_char_to_match[num_matches-1];
+ matches[n++] = matches[num_matches-1];
+ num_matches = n;
}
/* Did we find exactly one match? */
if (!matches || num_matches > 1) { |
| |
(0001883)
Marty
12-19-06 04:16
|
Hi Denis,
Sorry, realised when reading the patch after uploading that my change to 'showfiles' didn't output the 'str_add_chr'. The latest and greatest patch is aptly named 'latest.busybox.patch'.
Thanks,
Martin |
| |
(0001884)
vda
12-19-06 11:30
|
Applied to svn. Try attached cmdedit.c file... |
| |
(0001885)
vda
12-19-06 11:31
|
And, btw, thanks for pointing out my bug! |
| |
(0001896)
rockeychu
12-19-06 21:49
|
sqort() only sort "matches", but not "add_char_to_match" respectively, and result is files shown as dirs or dirs shown as files randomly.
This bug can be solved by following patch:
Index: shell/cmdedit.c
===================================================================
--- shell/cmdedit.c (revision 17012)
+++ shell/cmdedit.c (working copy)
@@ -539,20 +539,18 @@
static char **matches;
static int num_matches;
-static char *add_char_to_match;
-static void add_match(char *matched, int add_char)
+static void add_match(char *matched)
{
int nm = num_matches;
int nm1 = nm + 1;
matches = xrealloc(matches, nm1 * sizeof(char *));
- add_char_to_match = xrealloc(add_char_to_match, nm1);
matches[nm] = matched;
- add_char_to_match[nm] = (char)add_char;
num_matches++;
}
+/*
static int is_execute(const struct stat *st)
{
if ((!my_euid && (st->st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))) ||
@@ -561,6 +559,7 @@
(st->st_mode & S_IXOTH)) return TRUE;
return FALSE;
}
+*/
#if ENABLE_FEATURE_COMMAND_USERNAME_COMPLETION
@@ -605,7 +604,7 @@
while ((entry = getpwent()) != NULL) {
/* Null usernames should result in all users as possible completions. */
if ( /*!userlen || */ !strncmp(ud, entry->pw_name, userlen)) {
- add_match(xasprintf("~%s", entry->pw_name), '/');
+ add_match(xasprintf("~%s/", entry->pw_name));
}
}
@@ -672,7 +671,7 @@
return npth;
}
-static char *add_quote_for_spec_chars(char *found, int add)
+static char *add_quote_for_spec_chars(char *found)
{
int l = 0;
char *s = xmalloc((strlen(found) + 1) * 2);
@@ -682,8 +681,6 @@
s[l++] = '\\';
s[l++] = *found++;
}
- if (add)
- s[l++] = (char)add;
s[l] = 0;
return s;
}
@@ -732,7 +729,6 @@
while ((next = readdir(dir)) != NULL) {
char *str_found = next->d_name;
- int add_chr = 0;
/* matched ? */
if (strncmp(str_found, pfind, strlen(pfind)))
@@ -751,23 +747,24 @@
/* find with dirs ? */
if (paths[i] != dirbuf)
strcpy(found, next->d_name); /* only name */
+
+ int len1 = strlen(found);
+ found = xrealloc(found, len1+2);
+ found[len1] = '\0';
+ found[len1+1] = '\0';
+
if (S_ISDIR(st.st_mode)) {
/* name is directory */
- char *e = found + strlen(found) - 1;
-
- add_chr = '/';
- if (*e == '/')
- *e = '\0';
+ if(found[len1-1] !='/') {
+ found[len1] = '/';
+ }
} else {
/* not put found file if search only dirs for cd */
if (type == FIND_DIR_ONLY)
goto cont;
- if (type == FIND_FILE_ONLY ||
- (type == FIND_EXE_ONLY && is_execute(&st)))
- add_chr = ' ';
}
/* Add it to the list */
- add_match(found, add_chr);
+ add_match(found);
continue;
cont:
free(found);
@@ -959,14 +956,11 @@
int column_width = 0;
int nfiles = num_matches;
int nrows = nfiles;
- char str_add_chr[2];
int l;
/* find the longest file name- use that as the column width */
for (row = 0; row < nrows; row++) {
l = strlen(matches[row]);
- if (add_char_to_match[row])
- l++;
if (column_width < l)
column_width = l;
}
@@ -980,20 +974,15 @@
} else {
ncols = 1;
}
- str_add_chr[1] = 0;
for (row = 0; row < nrows; row++) {
int n = row;
int nc;
- int acol;
for(nc = 1; nc < ncols && n+nrows < nfiles; n += nrows, nc++) {
- str_add_chr[0] = add_char_to_match[n];
- acol = str_add_chr[0] ? column_width - 1 : column_width;
- printf("%s%s%-*s", matches[n], str_add_chr,
- acol - strlen(matches[n]), "");
+ printf("%s%-*s", matches[n],
+ column_width - strlen(matches[n]), "");
}
- str_add_chr[0] = add_char_to_match[n];
- printf("%s%s\n", matches[n], str_add_chr);
+ printf("%s\n", matches[n]);
}
}
@@ -1011,8 +1000,6 @@
free(matches[--num_matches]);
free(matches);
matches = (char **) NULL;
- free(add_char_to_match);
- add_char_to_match = NULL;
}
return;
}
@@ -1056,12 +1043,10 @@
free(matches[i]);
matches[i] = 0;
} else {
- add_char_to_match[n] = add_char_to_match[i];
matches[n++] = matches[i];
}
}
}
- add_char_to_match[n] = add_char_to_match[num_matches-1];
matches[n++] = matches[num_matches-1];
num_matches = n;
}
@@ -1082,12 +1067,18 @@
free(tmp1);
return;
}
- tmp = add_quote_for_spec_chars(tmp1, 0);
+ tmp = add_quote_for_spec_chars(tmp1);
free(tmp1);
} else { /* one match */
- tmp = add_quote_for_spec_chars(matches[0], add_char_to_match[0]);
+ tmp = add_quote_for_spec_chars(matches[0]);
/* for next completion current found */
*lastWasTab = FALSE;
+
+ len_found = strlen(tmp);
+ if(tmp[len_found-1] != '/') {
+ tmp[len_found] = ' ';
+ tmp[len_found+1] = '\0';
+ }
}
len_found = strlen(tmp);
/* have space to placed match? */ |
| |
(0001904)
vda
12-21-06 13:11
|
Patch is damaged (spaces instead of tabs everywhere) |
| |
(0001905)
vda
12-21-06 14:28
|
Fixed in rev 17037. Thanks for the patch. Can you test updated cmdedit.c (I deleted old and attached newer one) |
| |
(0001906)
rockeychu
12-21-06 17:52
|
That's OK.
Only with a trivial flaw: the order of "./" and "../" is not as expected. |
| |
(0001922)
Marty
12-22-06 05:34
|
Looks good. Here is a patch to fix the problem that rockeychu describes:
Index: shell/cmdedit.c
===================================================================
--- shell/cmdedit.c (revision 17042)
+++ shell/cmdedit.c (working copy)
@@ -992,7 +992,10 @@
static int match_compare(const void *a, const void *b)
{
- return strcmp(*(char**)a, *(char**)b);
+ char *s1 = *(char**)a, *s2 = *(char**)b;
+ while (*s1 && *s1 == *s2)
+ s1++, s2++;
+ return ((*s1 == '/' && *s2 != '/' ? '\0' : *s1) - (*s2 == '/' && *s1 != '/' ? '\0' : *s2) );
}
static void input_tab(int *lastWasTab) |
| |
(0001925)
rockeychu
12-22-06 06:25
|
Nice. Your patch sovles the trivial flaw intrduced in my previous patch.
Your patch can optimized by following:
Index: shell/cmdedit.c
===================================================================
--- shell/cmdedit.c (revision 17042)
+++ shell/cmdedit.c (working copy)
@@ -992,7 +992,10 @@
static int match_compare(const void *a, const void *b)
{
- return strcmp(*(char**)a, *(char**)b);
+ char *s1 = *(char**)a, *s2 = *(char**)b;
+ while (*s1 && *s1 == *s2)
+ s1++, s2++;
+ return ((*s1 == '/' ? '\0' : *s1) - (*s2 == '/' ? '\0' : *s2) );
}
static void input_tab(int *lastWasTab) |
| |
busybox.patch [^] (1,670 bytes) 12-18-06 06:33
