Re: [PATCH] Improve the "diff --git" format documentation

[Junio C Hamano <gitster@pobox.com>]

Andreas Gruenbacher <agruen@suse.de> writes:

> Hello,
>
> here is a small improvement to the documentation of git's extended diff
> format.  Can this please be included?
>
> Thanks,
> Andreas
>
> Signed-off-by: Andreas Gruenbacher <agruen@suse.de>
> ---

I thought you have been here long enough to send a patch with some more
meaningful log message than that.  Could you objectively describe in what
way is it an "improvement" on the subject line?

>  Documentation/diff-generate-patch.txt |   23 ++++++++++++++++++++++-
>  1 files changed, 22 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-
> generate-patch.txt
> index 8f9a241..05f2164 100644
> --- a/Documentation/diff-generate-patch.txt
> +++ b/Documentation/diff-generate-patch.txt
> @@ -18,7 +18,8 @@ diff format.
>  +
>  The `a/` and `b/` filenames are the same unless rename/copy is
>  involved.  Especially, even for a creation or a deletion,
> -`/dev/null` is _not_ used in place of `a/` or `b/` filenames.
> +`/dev/null` is _not_ used in place of `a/` or `b/` filenames in the
> +`diff --git` line.

With a bit more context, the original reads like this:

    What the -p option produces is slightly different from the traditional
    diff format.

    1.   It is preceded with a "git diff" header, that looks like
         this:

           diff --git a/file1 b/file2
    +
    The `a/` and `b/` filenames are the same unless rename/copy is
    involved.  Especially, even for a creation or a deletion,
    `/dev/null` is _not_ used in place of `a/` or `b/` filenames.

I think the first sentence makes it clear that this section is about '"git
diff" header', without repeating it like your patch does.

> @@ -38,11 +39,31 @@ the file that rename/copy produces, respectively.
>         dissimilarity index <number>
>         index <hash>..<hash> <mode>
>  
> +    Path names in extended header lines do not include the `a/` and `b/`
> +    prefixes.  The index header includes the <mode> only if the file
> +    mode does not change; otherwise, explicit mode headers are included.
> +

Have you looked at generated output in man and html formats?  I suspect
that it needs some asciidoc formatting magic, similar to what we already
have for the first section (namely, no indent, but the new block is marked
as a continuation with a lone plus sign at the beginning, instead of being
separated by a blank line).

>  3.  TAB, LF, double quote and backslash characters in pathnames
>      are represented as `\t`, `\n`, `\"` and `\\`, respectively.
>      If there is need for such substitution then the whole
>      pathname is put in double quotes.
>  
> +    Space characters are not quoted and so when files are copied or
> +    renamed, the file names in the "diff --git" line can be
> +    ambiguous.

Why do you even need to say that, especially after you made it clear that
rename information is available in the extended header section in an
unambiguous form?

> +4.  All the `a/` files refer to files before the commit, and all the `b/`
> +    files refer to files after the commit; it is incorrect to apply the
> +    changes to each file sequentially.  For example, this patch will
> +    swap a and b:
> +
> +      diff --git a/a b/b
> +      rename from a
> +      rename to b
> +      diff --git a/b b/a
> +      rename from b
> +      rename to a
> +
>  The similarity index is the percentage of unchanged lines, and
>  the dissimilarity index is the percentage of changed lines.  It
>  is a rounded down integer, followed by a percent sign.  The

The new section is a worthwhile addition; I however think this addition
makes the description of the similarity/dissimilarity indices further from
the section it relates to (it logically is part of section 2), so perhaps
it should move 3. down and add 4. while at it.

Thanks.