Re: [PATCH] fast-import: catch garbage after marks in from/merge

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 2, 2012, 15:43 UTC
Message-ID: <20120402154356.GD3365@burratino>
In-Reply-To: <20120402001354.GA12651@padd.com>

Pete Wyckoff wrote:

> jrnieder@gmail.com wrote on Sun, 01 Apr 2012 18:12 -0500:

>> Why allow whitespace after the mark number?
>
> Fear of breaking existing fast-import users that might happen
> to have stray whitespace, or \r\n terminators.

Oh. I think worrying about that sort of thing is a very good thing. In particular, the possibility of \r\n terminators from an importer that writes its output in text mode on Windows is somewhat scary.

Luckily such a problem would be caught as soon as the importer uses a filemodify (M) command:

 - if it quotes the path:

	fatal: Garbage after path in: M ...

 - if it doesn't quote the path, the resulting import would use
   filenames with trailing CRs.

So we narrowly escape that problem.

Other types of stray whitespace at eol seem less likely but it's probably worth letting a patch like this incubate in the "next" branch for a little while to see if anyone complains.

The reason to care instead of just being permissive is that git fast-import is not the only fast-import backend. The tighter we can make the parsing without breaking existing frontends, the better the chance that a frontend that was only tested against git fast-import will be usable against all other backends, too.

In other words, there are two requirements in tension here (compatibility with sloppy frontends, compatibility with fussy backends). Some day fast-import might want to learn a --permissive option to reconcile them. In this particular case being strict unconditionally seems safe enough.

My only other hint is that as Dmitry mentioned it would be nice to nice to use the same logic for other places where fast-import parses a number at the end of a line, so people don't keep on making the same print vs println mistake and sending new patches to detect it in each place one at a time.

Sane?

Thanks, Jonathan