Hi Ori,
thanks for your feedback. Reply and updated diff below,
On Wed, Oct 18 2017, Ori Bernstein <o...@eigenstate.org> wrote:
> On Wed, 18 Oct 2017 22:33:51 +0200
> Jeremie Courreges-Anglas <j...@wxcvbn.org> 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.
I must admit that the code is needlessly redundant, but I prefer to have
the loop condition inside... the loop condition.
>> + 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.
Those variables are static so that error messages are only printed once
in the shell lifetime. history reload happens each time the shell
detects that the histfile has been modified, which can happen often if
like me you have multiple interactives shells running at the same time.
> Maybe hoist this outside of the loop.
I don't see a reason for this.
>> + /* 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.
Also to limit the amount of crap printed on screen, though maybe we
should warn every time in this case. Here's an updated diff that does
so and hopefully makes the intents clearer. It also adds a comment
suggested by anton@
If your histfile ends with an overlong line which is also not terminated
by a newline, the do...while loop would silently ignore this last line.
I think that's acceptable, but YMMV.
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 22:16:43 -0000
@@ -739,24 +739,42 @@ 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;
+ while ((p = fgets(encoded, sizeof(encoded), histfh)) != NULL) {
+ if ((nl = strchr(encoded, '\n')) == NULL) {
+ static int warned;
+
+ /* no trailing newline? */
+ if (strlen(p) != sizeof(encoded) - 1) {
+ bi_errorf("history file is corrupt");
+ return;
+ }
+
+ if (!warned) {
+ warned = 1;
+ bi_errorf(
+ "ignoring history line(s) longer than %d"
+ " bytes", LINE);
+ }
+
+ /* discard overlong 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
