Andreas Gruenbacher [off-list ref] writes:
On Friday 01 January 2010 09:01:19 pm Junio C Hamano wrote:
quoted
quoted
Both "b file" and "c file " are parsed by "git apply" perfectly fine.
Right, the "diff --git" lines are technically still parseable when the file
name stays the same. With renames, lines like "diff --git a/f a/f b/f" or
"diff --git a/f b/f b/f" are possible, but then there will also be "renamed
from" and "renamed to" headers which will disambiguate things. Still, it
doesn't seem like a good idea to allow such ambiguities in the first place.
You already realized that there is no ambiguity because "diff --git" lines
are parsable and renames have explicit names. Why do you still maintain
that we are allowing such "ambiguities" when there is none?
quoted
Having said all that, I don't think we would mind a change to treat a
pathname with trailing SP a bit specially (iow, quoting "c file " in the
above failed attempt to reproduce the issue).
I would prefer quoting file names which contain spaces anywhere,...
The only reason I said I don't think we would mind changing the trailing
SP case is because the reduced risk of getting our patches corrupted by
MUA _might_ outweigh the benefit of not quoting to avoid an eyesore [*1*].
But what you said would add to eyesore of quoted names (which you omitted
from your quote) without any justification other than "I would prefer".
The pros-and-cons in such a change is quite different; as we have already
established that there is no ambiguity, "disambuguation" is not a "pro" in
this comparison.
[Footnote]
*1* Strictly speaking, it is not just "an eyesore" that is an issue. Our
diff output without renames are designed to be grokkable by other people's
patch implementations (e.g. GNU patch), and the quoted pathnames are not
understandable by them. Even though our final version of quoted path
format came from the GNU diff/patch maintainer (back then, at least):
http://article.gmane.org/gmane.comp.version-control.git/10103
I don't think it happened in the GNU land yet, and you would be the person
to know about it ;-).
On Saturday 02 January 2010 07:37:08 pm Junio C Hamano wrote:
Andreas Gruenbacher [off-list ref] writes:
quoted
On Friday 01 January 2010 09:01:19 pm Junio C Hamano wrote:
quoted
quoted
Both "b file" and "c file " are parsed by "git apply" perfectly fine.
Right, the "diff --git" lines are technically still parseable when the
file name stays the same. With renames, lines like "diff --git a/f a/f
b/f" or "diff --git a/f b/f b/f" are possible, but then there will also
be "renamed from" and "renamed to" headers which will disambiguate
things. Still, it doesn't seem like a good idea to allow such
ambiguities in the first place.
You already realized that there is no ambiguity because "diff --git" lines
are parsable and renames have explicit names. Why do you still maintain
that we are allowing such "ambiguities" when there is none?
Don't get so aroused ...
Right now, git generates lines like "diff --git a/f a/f b/f b/f" in some
corner cases, and from such lines alone, it is not possible to tell what the
two file names are (either "a/f a/f" and "b/f b/f", or "a/f a/f b/f" and
"b/f"). I can only find that out by looking at the other header lines.
I would prefer a format which I can parse line by line without ambiguities in
the first place, because this would keep things much simpler and easier to
debug. (Think of other implementations of the extended diff format which may
not produce the exact same output as git.)
So I would be happy with either of this:
* Also quote spaces in the "diff --git" line so that I can always reliably
parse it, or
* Add an additional extended header line with the file name in case there
are no other header lines giving the file names away already (as for
renames, copies, or when there are "---" and "+++" lines).
After our discussion so far, option two would probably be easier for everyone:
you could add it without risking to break anything, and I could avoid parsing
the "diff --git" line altogether.
quoted
quoted
Having said all that, I don't think we would mind a change to treat a
pathname with trailing SP a bit specially (iow, quoting "c file " in the
above failed attempt to reproduce the issue).
I would prefer quoting file names which contain spaces anywhere,...
The only reason I said I don't think we would mind changing the trailing
SP case is because the reduced risk of getting our patches corrupted by
MUA _might_ outweigh the benefit of not quoting to avoid an eyesore [*1*].
But what you said would add to eyesore of quoted names (which you omitted
from your quote) without any justification other than "I would prefer".
The pros-and-cons in such a change is quite different; as we have already
established that there is no ambiguity, "disambuguation" is not a "pro" in
this comparison.
[Footnote]
*1* Strictly speaking, it is not just "an eyesore" that is an issue. Our
diff output without renames are designed to be grokkable by other people's
patch implementations (e.g. GNU patch), and the quoted pathnames are not
understandable by them.
GNU patch doesn't look at "diff --git" lines or extended header lines at all
so far, so there are no compatibility issues yet. Quoting spaces in "---" and
"+++" lines would lead to problems with current GNU patch though. (So does
the quoting of several other characters like ", of course.)
Even though our final version of quoted path format came from the GNU
diff/patch maintainer (back then, at least):
http://article.gmane.org/gmane.comp.version-control.git/10103
I don't think it happened in the GNU land yet, and you would be the person
to know about it ;-).
I'm working on it ...
Thanks,
Andreas
On Saturday 02 January 2010 09:48:47 pm Andreas Gruenbacher wrote:
On Saturday 02 January 2010 07:37:08 pm Junio C Hamano wrote:
quoted
I don't think it happened in the GNU land yet, and you would be the
person to know about it ;-).
I'm working on it ...
The "experimental" branch [*] now has very lightly tested code for parsing
most extended headers ("index", "rename from", "rename to", "copy from", "copy
to", "old mode", "new mode", "deleted file mode", "new file mode"). Most
things should work, except:
* Doesn't parse filenames in "diff --git" lines. (I tried to argue why
those lines are a problem in this thread.)
* sha1 checksums are not verified right now. I'm not sure when that should
happen: always by default, or only optionally? (Like verifying file modes,
this is going to surprise a lot of users.)
* Similarity and dissimilarity headers are ignored.
* Binary diffs are no supported. I think GNU patch should recognize them and
give a reasonable message for now.
[*] http://git.savannah.gnu.org/cgit/patch.git/log/?h=experimental
Can you guys please do something to make parsing of filenames in "diff --git"
lines a lot easier, or unnecessary?
Thanks!
Andreas