[PATCH] Documentation/diff-options.txt: unify options

[PATCH] Documentation/diff-options.txt: unify options

[jidanni]

Signed-off-by: jidanni <jidanni@jidanni.org>

diff --git a/diff-options.txt b/diff-options.txt
index 5721548..b05503a 100644
--- a/diff-options.txt
+++ b/diff-options.txt
@@ -21,0 +22 @@ ifndef::git-format-patch[]
+-u::
@@ -26,3 +26,0 @@ endif::git-format-patch[]
--u::
-	Synonym for "-p".
-
@@ -30,2 +27,0 @@ endif::git-format-patch[]
-	Shorthand for "--unified=<n>".
-
@@ -189,0 +186 @@ endif::git-format-patch[]
+-a::
@@ -193,3 +189,0 @@ endif::git-format-patch[]
--a::
-	Shorthand for "--text".
-
@@ -198,0 +193 @@ endif::git-format-patch[]
+-b::
@@ -204,3 +199 @@ endif::git-format-patch[]
--b::
-	Shorthand for "--ignore-space-change".
-
+-w::
@@ -212,3 +204,0 @@ endif::git-format-patch[]
--w::
-	Shorthand for "--ignore-all-space".
-
-- 
1.5.6.5

Re: [PATCH] Documentation/diff-options.txt: unify options

[Junio C Hamano]

jidanni@jidanni.org writes:

> Signed-off-by: jidanni <jidanni@jidanni.org>
>
> diff --git a/diff-options.txt b/diff-options.txt
> index 5721548..b05503a 100644
> --- a/diff-options.txt
> +++ b/diff-options.txt
> @@ -21,0 +22 @@ ifndef::git-format-patch[]
> +-u::
> @@ -26,3 +26,0 @@ endif::git-format-patch[]
> --u::
> -	Synonym for "-p".
> -

Sorry, but this patch is very unusual in that it lacks any context lines,
which makes it impossible to review.

Re: [PATCH] Documentation/diff-options.txt: unify options

[jidanni]

JCH> Sorry, but this patch is very unusual in that it lacks any context lines,
JCH> which makes it impossible to review.

Trust me, I tried it with the default context lines and it was just
the same hard reading. OK, next time I send patches for other files, I
will always use the default and forget about saving bytes.

Re: [PATCH] Documentation/diff-options.txt: unify options

[Junio C Hamano]

jidanni@jidanni.org writes:

> JCH> Sorry, but this patch is very unusual in that it lacks any context lines,
> JCH> which makes it impossible to review.
>
> Trust me, I tried it with the default context lines and it was just
> the same hard reading.

Nonsense.

Here is a snippet from your patch.

        diff --git a/diff-options.txt b/diff-options.txt
        index 5721548..b05503a 100644
        --- a/diff-options.txt
        +++ b/diff-options.txt
        @@ -21,0 +22 @@ ifndef::git-format-patch[]
        +-u::
        @@ -26,3 +26,0 @@ endif::git-format-patch[]
        --u::
        -	Synonym for "-p".
        -

The only thing anybody can guess without looking at the original (that is
what "sending a patch without context" means) is that you moved "-u::" to
somewhere else, and stripped of its description.  There is absolutely no
clue to judge if the new home to "-u::" is an appropriate place.

In a normal patch with context, the same hunk would have looked like this:

        diff --git i/Documentation/diff-options.txt w/Documentation/diff-options.txt
        index c62b45c..c4ca0a9 100644
        --- i/Documentation/diff-options.txt
        +++ w/Documentation/diff-options.txt
        @@ -19,16 +19,12 @@ endif::git-format-patch[]

         ifndef::git-format-patch[]
         -p::
        +-u::
                Generate patch (see section on generating patches).
                {git-diff? This is the default.}
         endif::git-format-patch[]

        --u::
        -	Synonym for "-p".
        -
         -U<n>::
        -	Shorthand for "--unified=<n>".
        -
         --unified=<n>::

Presented this way, it is much more clear what is going on, as there is no
need to go back to the original and see if the new location for "-u::"
makes sense (and I think it does, but that is something I can say after
applying the patch and reviewing the result, because the patch itself is
not reviewable).

If you find yours just as easy to read as the one with context, your patch
reading ability far exceeds mine, and I'd refuse to read your patches in
the future to preserve my sanity.

There is another issue that should be obvious to people who deal with
patches every day.  The context-free patch you sent can be applied *ONLY*
after locating the *EXACT* preimage of the file you used to produce your
patch.  Before your patch is reviewed, other people may have already
modified the same file, perhaps introducing a few new lines at the top of
the file, and then what?  Your first hunk tells us that you would want to
insert a line with "-u::" at line #21, but the context does not match
anymore when your patch is reviewed.

Re: [PATCH] Documentation/diff-options.txt: unify options

[jidanni]

(You were right about my previous patch being bad.
At least git-am gives an error message when fed
http://article.gmane.org/gmane.comp.version-control.git/104017/raw
git-apply however does nothing and returns 0! Must be a bug.)
OK, here's a better patch:

Signed-off-by: jidanni <jidanni@jidanni.org>
---
 Documentation/diff-options.txt |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index c62b45c..b432d25 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -19,16 +19,12 @@ endif::git-format-patch[]
 
 ifndef::git-format-patch[]
 -p::
+-u::
 	Generate patch (see section on generating patches).
 	{git-diff? This is the default.}
 endif::git-format-patch[]
 
--u::
-	Synonym for "-p".
-
 -U<n>::
-	Shorthand for "--unified=<n>".
-
 --unified=<n>::
 	Generate diffs with <n> lines of context instead of
 	the usual three. Implies "-p".
@@ -190,31 +186,25 @@ endif::git-format-patch[]
 	can name which subdirectory to make the output relative
 	to by giving a <path> as an argument.
 
+-a::
 --text::
 	Treat all files as text.
 
--a::
-	Shorthand for "--text".
-
 --ignore-space-at-eol::
 	Ignore changes in whitespace at EOL.
 
+-b::
 --ignore-space-change::
 	Ignore changes in amount of whitespace.  This ignores whitespace
 	at line end, and considers all other sequences of one or
 	more whitespace characters to be equivalent.
 
--b::
-	Shorthand for "--ignore-space-change".
-
+-w::
 --ignore-all-space::
 	Ignore whitespace when comparing lines.  This ignores
 	differences even if one line has whitespace where the other
 	line has none.
 
--w::
-	Shorthand for "--ignore-all-space".
-
 --exit-code::
 	Make the program exit with codes similar to diff(1).
 	That is, it exits with 1 if there were differences and
-- 
1.5.6.5

Re: [PATCH] Documentation/diff-options.txt: unify options

[Junio C Hamano]

Thanks.

jidanni@jidanni.org writes:

> (You were right about my previous patch being bad.
> At least git-am gives an error message when fed
> http://article.gmane.org/gmane.comp.version-control.git/104017/raw
> git-apply however does nothing and returns 0! Must be a bug.)
> OK, here's a better patch:
>
> Signed-off-by: jidanni <jidanni@jidanni.org>

> ---

That commentary above your S-o-b is not a proper commit log message, but
I'll come up with something and apply.

$ wget http://article.gmane.org/gmane.comp.version-control.git/104017/raw
$ git apply raw
error: diff-options.txt: No such file or directory
$ echo $?
1

Even if you hand munge the "raw" file to have proper prefix, it will
refuse to apply a context-free diff.

$ git apply raw-edited
error: patch failed: Documentation/diff-options.txt:26
error: Documentation/diff-options.txt: patch does not apply
$ echo $?
1     

This is to avoid applying the patch only by line number without context;
see the last paragraph of <7vvdt4aj0e.fsf@gitster.siamese.dyndns.org> for
the explanation.

Upon a very rare case where you are absolutely sure that your copy is what the
patch is based on and that it is safe to applying a context-free patch
only by line number, you can give --unidiff-zero option to git-apply to
countermand this safety measure, but a patch posted on the public mailing
list for open source development rarely falls into that category.

Re: [PATCH] Documentation/diff-options.txt: unify options

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> That commentary above your S-o-b is not a proper commit log message, but
> I'll come up with something and apply.

Here is what I came up with and applied.

    Documentation/diff-options.txt: unify options
    
    Instead of listing short option (e.g. "-U<n>") as a shorthand for its
    longer counterpart (e.g. "--unified=<n>"), list the synonyms together.  It
    saves one indirection to find what the reader wants.
    
    Signed-off-by: jidanni <jidanni@jidanni.org>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks.

git apply didn't catch error? [was Re: [PATCH] Documentation/diff-options.txt: unify options]

[jidanni]

JCH> $ wget http://article.gmane.org/gmane.comp.version-control.git/104017/raw
JCH> $ git apply raw
JCH> error: diff-options.txt: No such file or directory
JCH> $ echo $?
JCH> 1
$ cd Documentation
$ git apply ../raw
$ echo $?
0
So then one thinks "why of course, the changes we already applied."
However if one makes some sneaky changes:
$ diff ../raw.original raw
46c46
< index 5721548..b05503a 100644
---
> index 6721548..c05503a 100644
70c70
< +-w::
---
> +-w::ZZZZZZZZZZZZZZZZZZ
One gets the same $?=0. git apply should scream instead of silently
returning 0 and doing nothing. git version 1.6.0.6.