Give better error messages if invalid or munged history file

[ScottPJones]

The Julia history file gave an error if there was anything but a '\t' character at the beginning of each code line, however that could cause problems, both if the file were edited with an editor set to convert leading indentation from tabs to spaces, and if the unindent code is fixed to be able to handle tabs correctly.

[ScottPJones]

The repl unit tests failed without this change... the unindent function is called from the "bracketed paste in search mode` code.

[StefanKarpinski]

Can you be a little more specific about what failed and how?

[ScottPJones]

I reran it with a different build, this is what I got: (the first time, it was missing the line number in the backtrace [is there some problem with that?], so I had to just look at the code where the error was reported in REPL.jl, which led me to relaxing the check for tabs to check for whitespace).

julia> Base.runtests("repl")
     * repl                exception on 1: ERROR: LoadError: Invalid history format. If you have a ~/.julia_history file left over from an older version of Julia, try renaming or deleting it.

 in hist_from_file at no file
 in include at ./boot.jl:254
 in runtests at no file
 in anonymous at no file
 in run_work_thunk at no file
 in remotecall_fetch at no file (repeats 2 times)
 in anonymous at no file
while loading /j/julia/test/repl.jl, in expression starting on line 204
ERROR: LoadError: LoadError: Invalid history format. If you have a ~/.julia_history file left over from an older version of Julia, try renaming or deleting it.

 in hist_from_file at no file
 in include at ./boot.jl:254
 in runtests at no file
 in anonymous at no file
 in run_work_thunk at no file
 in remotecall_fetch at no file (repeats 2 times)
 in anonymous at no file
while loading /j/julia/test/repl.jl, in expression starting on line 204
while loading /j/julia/usr/share/julia/test/runtests.jl, in expression starting on line 5

ERROR: A test has failed. Please submit a bug report (https://github.com/JuliaLang/julia/issues)
including error messages above and the output of versioninfo():
Julia Version 0.4.0-dev+5394
Commit b0a6aaf* (2015-06-15 23:17 UTC)
Platform Info:
  System: Darwin (x86_64-apple-darwin14.4.0)
  CPU: Intel(R) Core(TM) i7-4980HQ CPU @ 2.80GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Haswell)
  LAPACK: libopenblas
  LIBM: libopenlibm
  LLVM: libLLVM-3.6.1

 in error at ./error.jl:21
 in runtests at no file (repeats 2 times)

Now that I get back the line in test/repl.jl of the failing test, it seems it is just the test case that is broken,
expecting """ to not touch the tabs when it reindents the lines.

That doesn't change the fact that the hard check for tabs, instead of check for tab or whitespace,
means that editing the history file (which was one of your reasons for changing the history file format)
wouldn't work with any editor (such as Emacs) where you can have it output spaces instead of tabs when adding indentation, so this PR is still needed.

[pao]

wouldn't work with any editor (such as Emacs) where you can have it output spaces instead of tabs when adding indentation

C-q TAB, M-x package-install smart-tabs RET, or (setq-default indent-tabs-mode t), or do something fancier to detect that it's a Julia history file and do some buffer-local stuff. Emacs seems like the wrong example here. Any editor that can edit a Makefile without breaking it would have no problem with leading tab characters.

[ScottPJones]

@pao Yes, somebody would have to add that sort of detection, but it isn't there now, and editors like Emacs and Epsilon have modes to output spaces for tabs... and since the recommended format for Julia files is without spaces, I think people would tend to have smart-tabs that expand to spaces turned on...
Is relaxing the format, which is supposed to be readable, a problem?

[pao]

No, I don't really have a dog in this fight. But there's no detection involved with quoting tabs, and the default in Emacs is to do things the other way around--indent-tabs-mode defaults to t--so that part of the argument didn't make sense.

[ScottPJones]

What fight? Please, this isn't supposed to be adversarial!
What I meant was, that I think people may have set their own tabs mode default to output spaces, which is what I've done, because of the "no tabs wanted" rule in Julia (where I'm consulting also has the same rule). That means that, if you edit a history file, you're likely to accidentally remove the tabs... (which happened to me).

[pao]

Please, this isn't supposed to be adversarial

er, it's not: https://en.wiktionary.org/wiki/have_a_dog_in_this_fight (if that was actually sarcasm, then this is the classic sarcasm-doesn't-work-on-the-internet problem)

[ScottPJones]

@pao Just joking, not sarcasm! I understood the slang. Anything that even sounds like contention makes me cringe now! (like being told about the git "blame" command!) I really do just want to help fix up any bugs I stumble across, not aggravate anybody.

[StefanKarpinski]

Here's what I'd propose: fix the test so that it passes even with your change, which it should since the leading tabs in the history file should not leave the history file. Then we can have a separate discussion of whether the history file format should allow tabs or four spaces since some editors might mangle the history file.

[ScottPJones]

I already did that in #11719 a couple days ago, and this PR is precisely about releasing the history file format.

[ScottPJones]

@nolta What's your opinion on allowing the format to be a little more flexible, to avoid the problem I ran into?
@tkelman In your copious spare time, could you take a look (you give thorough review!)

[StefanKarpinski]

I don't really like it – it introduces an assumption about the width of a tab into the format. Currently the format is dirt simple, doesn't care at all about what you think a tab's width is, and it can encode any kind of multiline history data with metadata without any assumptions other than that it's line-oriented text.

[ScottPJones]

@StefanKarpinski Go look at the code!!! There already IS a broken assumption about the width of a tab!
(edit: never mind - I thought your comment was on the other PR, #11719 sorry!)

[nolta]

Why are you editing your history file?

[StefanKarpinski]

I'm gonna say no to loosening up the history file format. What would be a good idea, however, is giving a helpful error message if a history line starts with a space instead of a tab and suggesting that the user's editor might have converted tabs to spaces and they should try converting back to fix the issue.

[ScottPJones]

What about the following: instead of using a single tab, which is more likely to be munged by editors,
and if munged is basically invisible when you look at the file, use a single space.
I can change this to 1) use single space going forwards for the marker 2) if the 1st character after the metadata line is a tab, only allow tabs at the beginning (until the next metadata line).
3) if there are any spaces, give the error about editor having munged the file.
4) if the first character is a space, then eat just single spaces at the beginning, and error about editor munging if it sees a tab.
5) any other character besides tab or space after a metadata line gets an invalid history file error.

I think that would handle any issues of editors munging files, not increase the space used for the history file, and not know anything about tab size.

[ScottPJones]

OK, forget the last comment... I tried that out, but I didn't like the look of the history file...
(interestingly, things line up best with an 8-space tab).
So, what I did instead is to count the lines read in the file, and report both the line number and the character (if it isn't a space), and if it is a space, give an error message about editing possibly converting the tab to spaces, with the line number.
Do you think this approach (just adding better error reporting, so the history file can be fixed up) works?
It solves the problem I had, of not knowing why I was getting the error...

[ScottPJones]

@nolta @StefanKarpinski @pao What do you all think of the this way of resolving the issue (simply detecting when the tab has been munged or the file is otherwise corrupt, and outputting a better error message with the invalid character and line number?

[StefanKarpinski]

I like the sound of that. Can't look at the code just now, but will take a
look later/tomorrow.

On Sunday, June 21, 2015, Scott P. Jones notifications@github.com wrote:

@nolta https://github.com/nolta @StefanKarpinski
https://github.com/StefanKarpinski @pao https://github.com/pao What
do you all think of the this way of resolving the issue (simply detecting
when the tab has been munged or the file is otherwise corrupt, and
outputting a better error message with the invalid character and line
number?

—
Reply to this email directly or view it on GitHub
#11718 (comment).

[StefanKarpinski]

Why print this as hex(line[1])? Seems like repr(line[1]) would be easier to read.

[ScottPJones]

Ah, simply because I hadn't used repr ever before! (Still learning things about julia!) Thanks, I'll change that as soon as I get home.

[ScottPJones]

@StefanKarpinski Hopefully this improves things correctly the way you suggested, thanks for the review!

[StefanKarpinski]

Do you want to get rid of the 0x here now?

[ScottPJones]

Yes! Thanks for catching that!

[StefanKarpinski]

Does this need a trailing space?

[ScottPJones]

Yes, thanks!

[ScottPJones]

Should be all set now.

[StefanKarpinski]

Ok, this seems good and gives much more helpful error messages.

[ScottPJones]

Thanks again!