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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 6, 2010, 17:22 UTC
Message-ID: <7vk4lv44os.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201010061823.47475.agruen@suse.de>

Andreas Gruenbacher <agruen@suse.de> writes:

Show 10 quoted lines

> 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?

Show 15 quoted lines

>  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.

Show 8 quoted lines

> @@ -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).

Show 8 quoted lines

>  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?

Show 15 quoted lines

> +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.