Am 16.01.2017 um 18:06 schrieb Johannes Schindelin:
On Mon, 16 Jan 2017, Jeff King wrote:
Hmm. I am not sure to what degree CRLFs are actually a problem here.
Keep in mind these are error messages generated via error(), and so not
processing arbitrary data. I can imagine that CRs might come from:
Please note the regression test I added. It uses rev-parse's --abbrev-ref
option which quotes the argument when erroring out. This argument then
gets munged.
So error() (or in this case, die()) *very much* processes arbitrary data.
I *know* that rev-parse --abbrev-ref is an artificial example, it is
highly unlikely that anybody will use
git rev-parse --abbrev-ref "$(<call an external program here that
generates CR/LF line endings>)"
However, there are plenty other cases in regular Git usage where arguments
are generated by external programs to which we have no business dictating a
specific line ending style.
However, Jeff's patch is intended to catch exactly these cases (not for
the cases where this happens accidentally, but when they happen with
malicious intent).
We are talking about user-provided data that is reproduced by die() or
error(). I daresay that we do not have a single case where it is
intended that this data is intentionally multi-lined, like a commit
message. It can only be an accident or malicious when it spans across lines.
I know we allow CR and LF in file names, but in all cases where such a
name appears in an error message, it is *not important* that the data is
reproduced exactly. On the contrary, it is usually more helpful to know
that something strange is going on. The question marks are a strong
indication to the user for this.
If you absolutely insist, I will spend time to find a plausible example
and use that in the regression test.
I don't want to see you on an endeavor with dubious results. I'd prefer
to wait until the first case of "incorrectly munged data" is reported
because, as I said, I have a gut feeling that there is none.
I am certainly open to loosening the sanitizing for CR to make things
work seamlessly on Windows. But I am having trouble imagining a case
that is actually negatively impacted.
I came to the same conclusion. I regret having sent out a warning
message in, well, such a haste(*), without thinking the case through
first. IMHO, Jeff's patch should be fine as is.
(*) literally; I had to catch a train.
-- Hannes