[0/2] (experimental) diff --quote-path-with-sp

Message

Junio C Hamano Sept. 15, 2021, 10:33 p.m. UTC

Long time ago, we had a discussion with GNU patch/diff maintainer
and agreed that pathnames with certain "difficult" bytes needs to be
quoted to ensure the resulting patch is machine parseable in an
unambiguous way [*1*].  Recently, we saw a report that found that
GNU patch is unhappy with our diff output for a path with SP in it
[*2*].

With this experimental option, the beginning part of the patch
output will have pathnames with SP in them enclosed inside a pair of
double quotes, like so:

	diff --git "a/A Name" "b/A Name"
	--- "a/A Name"
	+++ "b/A Name"

We have always done this enclosing inside dq when the pathname had
an even more difficult byte (like LF, double quotes, and HT), so we
know Git tools can handle such a patch output just fine already.

 *1* https://lore.kernel.org/git/[e-mail]/
 *2* https://lore.kernel.org/git/[e-mail]/


Junio C Hamano (2):
  diff: simplify quote_two()
  diff: --quote-path-with-sp

 Documentation/diff-options.txt |  5 +++++
 diff.c                         | 26 ++++++++++----------------
 diff.h                         |  1 +
 quote.c                        | 23 ++++++++++++++++++-----
 quote.h                        |  3 ++-
 t/t3902-quoted.sh              | 24 +++++++++++++++++++++++-
 6 files changed, 59 insertions(+), 23 deletions(-)

Comments

Gwyneth Morgan Sept. 16, 2021, 3:33 a.m. UTC | #1

On 2021-09-15 15:33:14-0700, Junio C Hamano wrote:
> Long time ago, we had a discussion with GNU patch/diff maintainer
> and agreed that pathnames with certain "difficult" bytes needs to be
> quoted to ensure the resulting patch is machine parseable in an
> unambiguous way [*1*].  Recently, we saw a report that found that
> GNU patch is unhappy with our diff output for a path with SP in it
> [*2*].
> 
> With this experimental option, the beginning part of the patch
> output will have pathnames with SP in them enclosed inside a pair of
> double quotes, like so:
> 
> 	diff --git "a/A Name" "b/A Name"
> 	--- "a/A Name"
> 	+++ "b/A Name"

I believe GNU patch is fine with unquoted spaces in the "--- a/path"
and "+++ b/path", and only has an issue with unquoted spaces in the
"diff --git" line. busybox patch does seem to have an issue with quoted
filenames in the "---" and "+++" lines but is fine if those lines are
unquoted. Maybe we could leave spaces unquoted in those lines, only
quoted if there's some other character that needs it.

Junio C Hamano Sept. 16, 2021, 5:31 a.m. UTC | #2

Gwyneth Morgan <[e-mail]> writes:

> On 2021-09-15 15:33:14-0700, Junio C Hamano wrote:
>> Long time ago, we had a discussion with GNU patch/diff maintainer
>> and agreed that pathnames with certain "difficult" bytes needs to be
>> quoted to ensure the resulting patch is machine parseable in an
>> unambiguous way [*1*].  Recently, we saw a report that found that
>> GNU patch is unhappy with our diff output for a path with SP in it
>> [*2*].
>> 
>> With this experimental option, the beginning part of the patch
>> output will have pathnames with SP in them enclosed inside a pair of
>> double quotes, like so:
>> 
>> 	diff --git "a/A Name" "b/A Name"
>> 	--- "a/A Name"
>> 	+++ "b/A Name"
>
> I believe GNU patch is fine with unquoted spaces in the "--- a/path"
> and "+++ b/path", and only has an issue with unquoted spaces in the
> "diff --git" line. busybox patch does seem to have an issue with quoted
> filenames in the "---" and "+++" lines but is fine if those lines are
> unquoted. Maybe we could leave spaces unquoted in those lines, only
> quoted if there's some other character that needs it.

Well, if they are so inconsistent, perhaps we shouldn't bend over
backwards to cater to them, I would have to say.  I do not think we
want to break the convention that these a/ & b/ labeled paths are
spelled exactly the same way, only to please external tools.

Thanks for a quick report.