Re: [PATCH] Fix loading of multi-line history entires from disk

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Jul 17,  8:58am, Augie Fackler wrote:
} Subject: [PATCH] Fix loading of multi-line history entires from disk
}
} Fixes the issue I posted about yesterday with
} history-incremental-search-backward. I have no idea as to the
} correctness of the fix, but my history file is written out in a way
} that matches 4.3.x with this patch applied, and the behavior matches
} again.

We've got a hairy problem here.  See workers/30432 -- if you write a
line that ends in two backslashes, it is NOT rewritten on output, but
if you write a line than ends in one backslash then that IS doubled
on output ...

So either the first case is broken (with Augie's patch and/or prior to
workers/30443, it treats that line as continued when it should not) or
the second case is broken (it treats continuations as multiple lines).

The complication is that a line ending in THREE backslashes (or any
odd number of them) needs to go back to being treated as continuation.
In that instance there should be a newline embedded in the internal
history entry.

To sum up:

(1) All history files containing any history entries that intentionally
end with a double backslash, are broken, for as long as zsh has been
writing history files, because those lines are impossible to distinguish
from a continuation line where zsh doubled the backslash at output.

(2) Zsh versions 5.0.0 through 5.0.5 will incorrectly reload history that
contains continuation lines, because of an attempt to fix (1).  Versions
prior to 5.0.0 will incorrectly load files that have intentional double
backslashes, because of (1).

(3) Incorrectly reloaded history files will also have been incorrectly
written back by versions 5.0.0 through 5.0.5, in any circumstance that
saves in extended history format.

(4) The only reasonable solution is to fix (1) by changing the way the
entries are written, which means old history files will remain broken.
Attempting to fix it when loading the files was misguided (my error).

So, check out the following.  This includes Augie's patch (backs out
30433) and adds an attempt at changing the output (appends a space after
an even number of backslashes appearing at the end of an entry).

An alternative would be to append a semicolon there.  Opinions?  Is this
approach also misguided, and if so, what do we do?


diff --git a/Src/hist.c b/Src/hist.c
index 64f88f5..770d559 100644
--- a/Src/hist.c
+++ b/Src/hist.c
@@ -2308,8 +2308,7 @@ readhistline(int start, char **bufp, int *bufsiz, FILE *in)
 	}
 	else {
 	    buf[len - 1] = '\0';
-	    if (len > 1 && buf[len - 2] == '\\' &&
-		(len < 3 || buf[len - 3] != '\\')) {
+	    if (len > 1 && buf[len - 2] == '\\') {
 		buf[--len - 1] = '\n';
 		if (!feof(in))
 		    return readhistline(len, bufp, bufsiz, in);
@@ -2618,6 +2617,8 @@ savehistfile(char *fn, int err, int writeflags)
 
 	ret = 0;
 	for (; he && he->histnum <= xcurhist; he = down_histent(he)) {
+	    int count_backslashes = 0;
+
 	    if ((writeflags & HFILE_SKIPDUPS && he->node.flags & HIST_DUP)
 	     || (writeflags & HFILE_SKIPFOREIGN && he->node.flags & HIST_FOREIGN)
 	     || he->node.flags & HIST_TMPSTORE)
@@ -2649,9 +2650,18 @@ savehistfile(char *fn, int err, int writeflags)
 		if (*t == '\n')
 		    if ((ret = fputc('\\', out)) < 0)
 			break;
+		if (*t == '\\')
+		    count_backslashes++;
+		else
+		    count_backslashes = 0;
 		if ((ret = fputc(*t, out)) < 0)
 		    break;
 	    }
+	    if (ret < 0)
+	    	break;
+	    if (count_backslashes && (count_backslashes % 2 == 0))
+		if ((ret = fputc(' ', out)) < 0)
+		    break;
 	    if (ret < 0 || (ret = fputc('\n', out)) < 0)
 		break;
 	}