[PATCH] Document check option to git diff.

[PATCH] Document check option to git diff.

[rael]

From: Bill Lear <rael@zopyra.com>


Signed-off-by: Bill Lear <rael@zopyra.com>
---
 Documentation/SubmittingPatches |    3 ++-
 Documentation/diff-options.txt  |    5 +++++
 Documentation/git-diff.txt      |    6 ++++++
 3 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 41b76d8..ce85d06 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -23,7 +23,8 @@ probably need to split up your commit to finer grained pieces.
 
 Oh, another thing.  I am picky about whitespaces.  Make sure your
 changes do not trigger errors with the sample pre-commit hook shipped
-in templates/hooks--pre-commit.
+in templates/hooks--pre-commit.  To help ensure this does not happen,
+run git diff --check on your changes before you commit.
 
 
 (2) Generate your patch using git tools out of your commits.
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index da1cc60..501e0df 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -58,6 +58,11 @@
 	Turn off rename detection, even when the configuration
 	file gives the default to do so.
 
+--check::
+	Check whether differences form a valid patch.  Warns if
+	differences include a space before a tab or a space at the end
+	of a line.
+
 --full-index::
 	Instead of the first handful characters, show full
 	object name of pre- and post-image blob on the "index"
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 6a098df..5054fb7 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -70,6 +70,7 @@ Various ways to check your working tree::
 $ git diff            <1>
 $ git diff --cached   <2>
 $ git diff HEAD       <3>
+$ git diff --check    <4>
 ------------
 +
 <1> changes in the working tree not yet staged for the next commit.
@@ -77,6 +78,11 @@ $ git diff HEAD       <3>
 would be committing if you run "git commit" without "-a" option.
 <3> changes in the working tree since your last commit; what you
 would be committing if you run "git commit -a"
+<4> display whether what you are about to commit is a valid patch,
+including whether you have spaces at the end of lines, or spaces
+before tabs.  Violations are displayed in this form:
+
+<file name>:<line number>:<violation>:<offending line>
 
 Comparing with arbitrary commits::
 +
-- 
1.5.0-rc2.GIT-dirty

Re: [PATCH] Document check option to git diff.

[Junio C Hamano]

rael@zopyra.com writes:

> From: Bill Lear <rael@zopyra.com>
>
> Signed-off-by: Bill Lear <rael@zopyra.com>
> ---
>  Documentation/SubmittingPatches |    3 ++-
>  Documentation/diff-options.txt  |    5 +++++
>  Documentation/git-diff.txt      |    6 ++++++
>  3 files changed, 13 insertions(+), 1 deletions(-)

Thanks.  It's nice to see somebody new getting more and more
comfortable with git.

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index da1cc60..501e0df 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -58,6 +58,11 @@
>  	Turn off rename detection, even when the configuration
>  	file gives the default to do so.
>  
> +--check::
> +	Check whether differences form a valid patch.  Warns if
> +	differences include a space before a tab or a space at the end
> +	of a line.
> +

It is supposed to be only about new lines; correcting an
existing line that had trailing whitespaces should not trigger
the check.  "SP before a TAB" check is supposed to apply only in
the indent part (if the code does not work that way, you have
spotted bugs).

Also I fear that 'valid' is a bit too strong a word here.  A
patch that introduces new lines that have trailing whitespaces
is still a valid patch in the sense that 'patch' and 'git apply'
would accept it as input.  How about rewording it like this?

	Look for and warn about changes that introduce trailing
        whitespaces at the end of the line or an indent that
        uses a whitespace before a tab.

> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> index 6a098df..5054fb7 100644
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -70,6 +70,7 @@ Various ways to check your working tree::
>  $ git diff            <1>
>  $ git diff --cached   <2>
>  $ git diff HEAD       <3>
> +$ git diff --check    <4>
>  ------------
>  +
>  <1> changes in the working tree not yet staged for the next commit.
> @@ -77,6 +78,11 @@ $ git diff HEAD       <3>
>  would be committing if you run "git commit" without "-a" option.
>  <3> changes in the working tree since your last commit; what you
>  would be committing if you run "git commit -a"
> +<4> display whether what you are about to commit is a valid patch,
> +including whether you have spaces at the end of lines, or spaces
> +before tabs.  Violations are displayed in this form:
> +
> +<file name>:<line number>:<violation>:<offending line>
>  
>  Comparing with arbitrary commits::
>  +

        ... what you are about to commit introduces funny whitespaces,
        such as traling whitespaces at the end of the line or an indent
        that uses a whitespace before a tab.  Violations are...

Re: [PATCH] Document check option to git diff.

[Bill Lear]

On Friday, January 26, 2007 at 15:46:02 (-0800) Junio C Hamano writes:
>...
>Thanks.  It's nice to see somebody new getting more and more
>comfortable with git.

I, and my colleagues, are grateful for all the work put into it.

>Also I fear that 'valid' is a bit too strong a word here.  ...
>                     ....  How about rewording it like this?
>
>	Look for and warn about changes that introduce trailing
>        whitespaces at the end of the line or an indent that
>        uses a whitespace before a tab.

I was indeed too strong in wording it as "valid": yet another example
of haste-induced waste.

I believe that an accurate and concise statement would be:

    Warn if changes introduce trailing whitespace
    or an indent that uses a space before a tab.

I think it should be explicitly limited to "space" and not
"whitespace" before the tab, as "whitespace" really includes tab.

Do I really need to say "trailing whitespace at the end of the line"?
That seems overly verbose: trailing whitespace is, I think, understood
to trail at the end of the line.

I'll re-do the patch when I hear back, probably tomorrow.

Also: I suppose I am wondering about the motivation for this switch.
It seems to reflect the aesthetics of the git project.  Whitespace at
the end of a line is meaningless and wasteful, so I understand and
sympathize with the judgment that this is undesirable.  Spaces
preceding tabs are somewhat murkier: two tabs, a space, and text pass
the check, but a tab, space, tab and text do not.  Why is this bad?

I'm sure there is a better way to categorize these violations other
than "funny".  Should we not say "wasteful and inelegant, and
therefore anathema to any decent, self-respecting person"?


Bill

Re: [PATCH] Document check option to git diff.

[Junio C Hamano]

Bill Lear <rael@zopyra.com> writes:

> I believe that an accurate and concise statement would be:
>
>     Warn if changes introduce trailing whitespace
>     or an indent that uses a space before a tab.
>
> I think it should be explicitly limited to "space" and not
> "whitespace" before the tab, as "whitespace" really includes tab.
>
> Do I really need to say "trailing whitespace at the end of the line"?
> That seems overly verbose: trailing whitespace is, I think, understood
> to trail at the end of the line.

All true.  Thanks for rewording.

> Also: I suppose I am wondering about the motivation for this switch.
> It seems to reflect the aesthetics of the git project.  Whitespace at
> the end of a line is meaningless and wasteful, so I understand and
> sympathize with the judgment that this is undesirable.  Spaces
> preceding tabs are somewhat murkier: two tabs, a space, and text pass
> the check, but a tab, space, tab and text do not.  Why is this bad?

The trailing space removal comes from the kernel project
aesthetics:

	http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt
	linux-2.6/Documentation/CodingStyle (end of Chapter 1)

"TAB SP TAB blah" visually is the same as "TAB TAB blah".  It is
bad for the same reason as "fofofofo SP TAB EOL" (which is
visually the same as "fofofofo EOL") is bad.  This is not from
the kernel project, so if the above reasoning is flawed, it is
my fault.  I once considered saying 8 spaces are bad and should
be replaced with a tab, but I refrained from going that far ;-).

> I'm sure there is a better way to categorize these violations other
> than "funny".  Should we not say "wasteful and inelegant, and
> therefore anathema to any decent, self-respecting person"?

"Wasteful" is probably better than "funny"; I do not think of a
good wording.

Sometimes we do want to keep the trailing whitespaces (the patch
that came over e-mail to produce commit c7c24889, for example),
so getting warnings from "git-diff --check" (or its counterpart,
"git-apply --whitespace=warn") is not a crime.  But most of the
time, they only waste space and bandwidth and many people who
are conscious about hygiene of their sources seem to avoid them
like the plague.