[PATCH] normal/cmdline.c : history contains empty lines

[PATCH] normal/cmdline.c : history contains empty lines

[Vincent Pelletier]

[-- Attachment #1.1: Type: text/plain, Size: 382 bytes --]

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

Changelog :

2004-12-13 Vincent Pelletier <subdino2004@yahoo.fr>
* 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


[-- Attachment #1.2: normal_cmdline_history_fixes.diff --]
[-- Type: text/plain, Size: 2646 bytes --]

Index: cmdline.c
===================================================================
RCS file: /cvsroot/grub/grub2/normal/cmdline.c,v
retrieving revision 1.10
diff -p -u -r1.10 cmdline.c
--- cmdline.c	13 Dec 2004 17:26:17 -0000	1.10
+++ cmdline.c	13 Dec 2004 22:35:46 -0000
@@ -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 ();


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 256 bytes --]

Re: [PATCH] normal/cmdline.c : history contains empty lines

[Yoshinori K. Okuji]

On Tuesday 14 December 2004 08:08, Vincent Pelletier wrote:
> 2004-12-13 Vincent Pelletier <subdino2004@yahoo.fr>
> * 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

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

Re: [PATCH] normal/cmdline.c : history contains empty lines

[Vincent Pelletier]

-----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-----

Re: [PATCH] normal/cmdline.c : history contains empty lines

[Yoshinori K. Okuji]

On Sunday 19 December 2004 16:46, Vincent Pelletier wrote:
> I meant not to add the empty line to the history, but to clear the
> line :

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

Okuji

Re: [PATCH] normal/cmdline.c : history contains empty lines

[Marco Gerards]

Vincent Pelletier <subdino2004@yahoo.fr> writes:

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

Thanks. :)

> Changelog :
>
> 2004-12-13 Vincent Pelletier <subdino2004@yahoo.fr>
> * 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

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  <metgerards@student.han.nl>

	* normal/cmdline.c (grub_cmdline_get): Redone logic so no empty
	lines are inserted and make it work like readline.  Reported by
	Vincent Pelletier <subdino2004@yahoo.fr>.


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
--- normal/cmdline.c	13 Dec 2004 17:26:17 -0000	1.10
+++ normal/cmdline.c	29 Dec 2004 19:19:03 -0000
@@ -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;

Re: [PATCH] normal/cmdline.c : history contains empty lines

[Vincent Pelletier]

-----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  <metgerards@student.han.nl>
|
| 	* normal/cmdline.c (grub_cmdline_get): Redone logic so no empty
| 	lines are inserted and make it work like readline.  Reported by
| 	Vincent Pelletier <subdino2004@yahoo.fr>.

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-----

Re: [PATCH] normal/cmdline.c : history contains empty lines

[Marco Gerards]

Vincent Pelletier <subdino2004@yahoo.fr> writes:

> 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).

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

> grub_history_add should be moved after the while loop too.

In my code?

--
Marco