On Wed, 18 Oct 2017 22:33:51 +0200
Jeremie Courreges-Anglas <[email protected]> wrote:
>
> It would be nice to support arbitrarily long lines, but a first step
> would be to skip them gracefuly.
>
> The code modifies the loop condition: no tests against ferror(3)/feof(3)
> are performed any more (I don't see their point).
>
> We could print a warning on ferror(), though, but that would be another
> diff.
>
> ok?
>
>
> Index: history.c
> ===================================================================
> RCS file: /d/cvs/src/bin/ksh/history.c,v
> retrieving revision 1.72
> diff -u -p -p -u -r1.72 history.c
> --- history.c 18 Oct 2017 15:41:25 -0000 1.72
> +++ history.c 18 Oct 2017 20:18:24 -0000
> @@ -739,24 +739,45 @@ history_close(void)
> static void
> history_load(Source *s)
> {
> - char *p, encoded[LINE + 1], line[LINE + 1];
> + char *nl, *p, encoded[LINE + 1], line[LINE + 1];
>
> rewind(histfh);
> + line_co = 1;
>
> /* just read it all; will auto resize history upon next command */
> - for (line_co = 1; ; line_co++) {
> - p = fgets(encoded, sizeof(encoded), histfh);
> - if (p == NULL || feof(histfh) || ferror(histfh))
> - break;
> - if ((p = strchr(encoded, '\n')) == NULL) {
> - bi_errorf("history file is corrupt");
> - return;
> + for (p = fgets(encoded, sizeof(encoded), histfh); p != NULL;
> + p = fgets(encoded, sizeof(encoded), histfh)) {
The redundant calls prevent this code from scanning as well as it
could for me. Perhaps:
for(;;) {
if ((p = fgets(...) == NULL)
break;
Just a minor cosmetic nitpick, it's not a big deal either way.
> + if ((nl = strchr(encoded, '\n')) == NULL) {
> + static int corrupt, toolong;
I'm not sure why these need to be static. I'm not an expert on
the code, but it looks like histsave can be called multiple
times as the shell runs, which triggers a history reload.
Keeping this state seems like it would be wrong.
Maybe hoist this outside of the loop.
> + /* no trailing newline? */
> + if (strlen(p) != sizeof(encoded) - 1) {
> + if (!corrupt) {
> + corrupt = 1;
> + bi_errorf("history file is corrupt");
> + }
> + return;
Why does `corrupt` need to be recorded at all? There's a
return right after it.
> + }
> +
> + if (!toolong) {
> + toolong = 1;
> + bi_errorf(
> + "ignoring history line(s) longer than %d"
> + " bytes", LINE);
> + }
> +
> + do {
> + p = fgets(encoded, sizeof(encoded), histfh);
> + } while (p != NULL && strchr(p, '\n') == NULL);
> +
> + continue;
> }
> - *p = '\0';
> + *nl = '\0';
> s->line = line_co;
> s->cmd_offset = line_co;
> strunvis(line, encoded);
> histsave(line_co, line, 0);
> + line_co++;
> }
>
> history_write();
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
>
--
Ori Bernstein <[email protected]>
