normal/cmdline.c : history contains empty lines

Hi.
Here is my first patch to grub2 :) .

Changelog :

2004-12-13 Vincent Pelletier <[email protected]>
* grub_history_get : doesn't add empty lines, exits if called with NULL argument
* grub_cmdline_get : command is added to history after user hit "return", when pressing "down" arrow when on the most recent item in history an empty line is shown

Vincent "Subdino" Pelletier

Index: cmdline.c
===================================================================
RCS file: /cvsroot/grub/grub2/normal/cmdline.c,v
retrieving revision 1.10

diff -p -u -r1.10 cmdline.c
@@ -102,26 +102,30 @@ grub_history_get (int pos)
 static void
 grub_history_add (char *s)
 {
-  /* Remove the oldest entry in the history to make room for a new
-     entry.  */
-  if (hist_used + 1 > hist_size)
-    {
-      hist_end--;
-      if (hist_end < 0)
-	hist_end = hist_size + hist_end;
+  /* avoid inserting empty lines & protect from mistakes */
+  if(s && grub_strlen(s))
+    {
+      /* Remove the oldest entry in the history to make room for a new
+         entry.  */
+      if (hist_used + 1 > hist_size)
+        {
+          hist_end--;
+          if (hist_end < 0)
+            hist_end = hist_size + hist_end;
 
-      grub_free (hist_lines[hist_end]);
-    }
-  else
-    hist_used++;
+          grub_free (hist_lines[hist_end]);
+        }
+      else
+        hist_used++;
 
-  /* Move to the next position.  */
-  hist_pos--;
-  if (hist_pos < 0)
-    hist_pos = hist_size + hist_pos;
+      /* Move to the next position.  */
+      hist_pos--;
+      if (hist_pos < 0)
+        hist_pos = hist_size + hist_pos;
 
-  /* Insert into history.  */
-  hist_lines[hist_pos] = grub_strdup (s);
+      /* Insert into history.  */
+      hist_lines[hist_pos] = grub_strdup (s);
+    }
 }
 
 /* Replace the history entry on position POS with the string S.  */

@@ -475,6 +479,7 @@ grub_cmdline_get (const char *prompt, ch
   auto void cl_delete (unsigned len);
   auto void cl_print (int pos, int c);
   auto void cl_set_pos (void);
+  const char empty_line[] = "";
 
   void cl_set_pos (void)
     {

@@ -556,8 +561,6 @@ grub_cmdline_get (const char *prompt, ch
   
   cl_insert (cmdline);
 
-  grub_history_add (buf);
-
   while ((key = GRUB_TERM_ASCII_CHAR (grub_getkey ())) != '\n' && key != '\r')
     {
       if (readline)

@@ -640,12 +643,16 @@ grub_cmdline_get (const char *prompt, ch
 
 		lpos = 0;
 
+		cl_delete (llen);
 		if (histpos > 0)
-		  histpos--;
+                  {
+		    histpos--;
+		    hist = grub_history_get (histpos);
+                  }
+                else
+                  hist = empty_line;
 
-		cl_delete (llen);
-		hist = grub_history_get (histpos);
-		cl_insert (hist);
+                cl_insert (hist);
 
 		break;
 	      }

@@ -726,6 +733,7 @@ grub_cmdline_get (const char *prompt, ch
 
       grub_history_replace (histpos, buf);
     }
+  grub_history_add (buf);
 
   grub_putchar ('\n');
   grub_refresh ();

I agree that empty lines should not be added into the history, but I don't think it is a good idea to show an empty line when you go down the history. Because I really like to have compatibility with BASH.

I don't know how the current implementation behaves very well, but it should work in the same way as GRUB legacy. It should be like this:

PROMPT HISTORY
grub> []
foo[RET]
grub> ["foo"]
bar[RET]
grub> ["foo", "bar"]
[RET]
grub> ["foo", "bar"]
[C-p]
grub> bar ["foo", "bar"]
[C-n]
grub> ["foo", "bar"]
[C-p]
grub> bar ["foo", "bar"]
baz[RET]
grub> ["foo", "bar", "barbaz"]

Okuji

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Yoshinori K. Okuji a écrit :

| I agree that empty lines should not be added into the history, but I
| don't think it is a good idea to show an empty line when you go down
| the history. Because I really like to have compatibility with BASH.

I meant not to add the empty line to the history, but to clear the line :

PROMPT HISTORY
grub> barbaz ["foo", "bar", "barbaz"]
[C-n]
grub> ["foo", "bar", "barbaz"]

Last time I tried that before modifying cmdline.c , the last line was still displayed in front of the prompt, IIRC.

Vincent Pelletier
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFBxaJIFEQoKRQyjtURAkp9AJ46NW87th/7ZVc3cj4oteaotSisRQCdGGzq qFzY3psuiW/At4/t6Likla8=
=Becq
-----END PGP SIGNATURE-----

I didn't say adding it. Try the same thing with BASH.

Okuji

Thanks. :)

Your patch was kinda hard to comprehend, especially because you rewrote grub_history_add. And unfortunately it caused a segfault here. :/

The main problem was in the logic of the history code. I should be terribly ashamed for doing it wrong like this in the first place. I made a fix to change the logic there, instead of in grub_history_add. In the meanwhile I made it work just like bash, like Okuji requested.

Vincent, does this patch solve your problem? I will commit it soon.

Thanks,
Marco

2004-12-29 Marco Gerards <[email protected]>

* normal/cmdline.c (grub_cmdline_get): Redone logic so no empty lines are inserted and make it work like readline. Reported by Vincent Pelletier <[email protected]>.

Index: normal/cmdline.c
===================================================================
RCS file: /cvsroot/grub/grub2/normal/cmdline.c,v
retrieving revision 1.10

diff -u -p -u -p -r1.10 cmdline.c
@@ -556,7 +556,8 @@ grub_cmdline_get (const char *prompt, ch
   
   cl_insert (cmdline);
 
-  grub_history_add (buf);
+  if (hist_used == 0)
+    grub_history_add (buf);
 
   while ((key = GRUB_TERM_ASCII_CHAR (grub_getkey ())) != '\n' && key != '\r')
     {

@@ -641,7 +642,10 @@ grub_cmdline_get (const char *prompt, ch
 		lpos = 0;
 
 		if (histpos > 0)
-		  histpos--;
+		  {
+		    grub_history_replace (histpos, buf);
+		    histpos--;
+		  }
 
 		cl_delete (llen);
 		hist = grub_history_get (histpos);

@@ -656,7 +660,10 @@ grub_cmdline_get (const char *prompt, ch
 		lpos = 0;
 
 		if (histpos < hist_used - 1)
-		  histpos++;
+		  {
+		    grub_history_replace (histpos, buf);
+		    histpos++;
+		  }
 
 		cl_delete (llen);
 		hist = grub_history_get (histpos);

@@ -723,8 +730,6 @@ grub_cmdline_get (const char *prompt, ch
 	    }
 	  break;
 	}
-
-      grub_history_replace (histpos, buf);
     }
 
   grub_putchar ('\n');

@@ -736,6 +741,13 @@ grub_cmdline_get (const char *prompt, ch
     while (buf[lpos] == ' ')
       lpos++;
 
+  histpos = 0;
+  if (grub_strlen (buf) > 0)
+    {
+      grub_history_replace (histpos, buf);
+      grub_history_add ("");
+    }
+  
   grub_memcpy (cmdline, buf + lpos, llen - lpos + 1);
 
   return 1;

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Marco Gerards a ?crit :

| Your patch was kinda hard to comprehend, especially because you
| rewrote grub_history_add.  And unfortunately it caused a segfault
| here. :/
|
| The main problem was in the logic of the history code.  I should be
| terribly ashamed for doing it wrong like this in the first place.  I
| made a fix to change the logic there, instead of in grub_history_add.
| In the meanwhile I made it work just like bash, like Okuji requested.
|
| Vincent, does this patch solve your problem?  I will commit it soon.
|
| Thanks,
| Marco
|
| 2004-12-29  Marco Gerards  <[email protected]>
|
| 	* normal/cmdline.c (grub_cmdline_get): Redone logic so no empty
| 	lines are inserted and make it work like readline.  Reported by
| 	Vincent Pelletier <[email protected]>.

Nice for the grub_history_replace, I missed that function and was trying to rewrite it (I was redesigning the patch according to Okuji's requests). I don't understand how the changes I made on grub_history_add could made it segfault, as I only checks for null pointer and empty string (to prevent adding an empty line or - ironically - to prevent it from segfaulting when duplicating an invalid buffer). The empty string could be checked using a buf[0]==0 instead of calling grub_strlen as I did (faster, but maybe less portable in case of wide chars).

grub_history_add should be moved after the while loop too.

The use of "empty_line" was only a dirty hack, I should have 0-ed the first char of hist instead.

Vincent Pelletier
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.5 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFB0wpSFEQoKRQyjtURAsekAJ0QEuqGVGEDwa5J14t9tCoejHFUiwCgnVGl XqsCAjSJpxbDY2C0PeC8BKE=
=d/Hc
-----END PGP SIGNATURE-----

Yeah, but this makes it more readable. Speed is not really an issue in handling a command line. :)

In my code?

--
Marco