threads / patch / 53679

patchbuiltin/diff: fix incorrect comment

Subject: [PATCH] builtin/diff: fix incorrect comment

## tl;dr

3 messages between Jun 15, 2020 and Jun 18, 2020. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Denton Liu· Jun 15, 2020, 11:15 UTC · lore

A comment in cmd_diff() states that if one tree-ish and no blobs are provided, it would provide a diff between the tree and the cache. This is incorrect because a diff happens between the tree-ish and the working tree. Remove the `--cached` in the comment so that the correct behavior is shown.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to builtin/diff.c +1 −1
diff --git a/builtin/diff.c b/builtin/diff.c
index 8537b17bd5..1ebab58c55 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -276,7 +276,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	 *	compare a blob with a working tree file.
 	 *
 	 * N=1, M=0:
-	 *      tree vs cache (diff-index --cached)
+	 *      tree vs files (diff-index)
 	 *
 	 * N=2, M=0:
 	 *      tree vs tree (diff-tree)
-- 
2.27.0.132.g321788e831
Junio C Hamano· Jun 17, 2020, 18:07 UTC · re: Denton Liu · lore

Re: [PATCH] builtin/diff: fix incorrect comment

Denton Liu <liu.denton@gmail.com> writes:
Show 5 quoted lines
> A comment in cmd_diff() states that if one tree-ish and no blobs are
> provided, it would provide a diff between the tree and the cache. This
> is incorrect because a diff happens between the tree-ish and the working
> tree. Remove the `--cached` in the comment so that the correct behavior
> is shown.

Perhaps "diff-index [--cached]" is more appropriate, then? After all, "git diff --cached HEAD" would be N=1 M=0 case, no?

Show 18 quoted lines
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  builtin/diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 8537b17bd5..1ebab58c55 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -276,7 +276,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  	 *	compare a blob with a working tree file.
>  	 *
>  	 * N=1, M=0:
> -	 *      tree vs cache (diff-index --cached)
> +	 *      tree vs files (diff-index)
>  	 *
>  	 * N=2, M=0:
>  	 *      tree vs tree (diff-tree)
Denton Liu· Jun 18, 2020, 10:43 UTC · re: Denton Liu · lore

[PATCH v2] builtin/diff: update usage comment

A comment in cmd_diff() states that if one tree-ish and no blobs are provided, (the "N=1, M=0" case), it will provide a diff between the tree and the cache. This is incorrect because a diff happens between the tree-ish and the working tree. Remove the `--cached` in the comment so that the correct behavior is shown. Add a new section describing the "N=1, M=0, --cached" behavior.

Next, describe the "N=0, M=0, --cached" case, similar to the above since it is undocumented.

Finally, fix some spacing issues. Add spaces between each section for consistency and readability. Also, change tabs within the comment into spaces.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Notes:
    This change can be broken down into three separate commits but I figured
    it is a little bit overkill considering how little the scope of each
    individual change would be. If it's more readable as separate commits, I
    can resubmit it, though.
Range-diff against v1:
1:  a9aea5dbb8 ! 1:  54c5de021a builtin/diff: fix incorrect comment
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    builtin/diff: fix incorrect comment
    +    builtin/diff: update usage comment
     
         A comment in cmd_diff() states that if one tree-ish and no blobs are
    -    provided, it would provide a diff between the tree and the cache. This
    -    is incorrect because a diff happens between the tree-ish and the working
    -    tree. Remove the `--cached` in the comment so that the correct behavior
    -    is shown.
    +    provided, (the "N=1, M=0" case), it will provide a diff between the tree
    +    and the cache. This is incorrect because a diff happens between the
    +    tree-ish and the working tree. Remove the `--cached` in the comment so
    +    that the correct behavior is shown. Add a new section describing the
    +    "N=1, M=0, --cached" behavior.
    +
    +    Next, describe the "N=0, M=0, --cached" case, similar to the above since
    +    it is undocumented.
    +
    +    Finally, fix some spacing issues. Add spaces between each section for
    +    consistency and readability. Also, change tabs within the comment into
    +    spaces.
    +
    +
    + ## Notes ##
    +    This change can be broken down into three separate commits but I figured
    +    it is a little bit overkill considering how little the scope of each
    +    individual change would be. If it's more readable as separate commits, I
    +    can resubmit it, though.
     
      ## builtin/diff.c ##
     @@ builtin/diff.c: int cmd_diff(int argc, const char **argv, const char *prefix)
    - 	 *	compare a blob with a working tree file.
    + 
    + 	/*
    + 	 * We could get N tree-ish in the rev.pending_objects list.
    +-	 * Also there could be M blobs there, and P pathspecs.
    ++	 * Also there could be M blobs there, and P pathspecs. --cached may
    ++	 * also be present.
      	 *
    - 	 * N=1, M=0:
    --	 *      tree vs cache (diff-index --cached)
    + 	 * N=0, M=0:
    +-	 *	cache vs files (diff-files)
    ++	 *      cache vs files (diff-files)
    ++	 *
    ++	 * N=0, M=0, --cached:
    ++	 *      HEAD vs cache (diff-index --cached)
    ++	 *
    + 	 * N=0, M=2:
    + 	 *      compare two random blobs.  P must be zero.
    ++	 *
    + 	 * N=0, M=1, P=1:
    +-	 *	compare a blob with a working tree file.
    ++	 *      compare a blob with a working tree file.
    ++	 *
    ++	 * N=1, M=0:
     +	 *      tree vs files (diff-index)
      	 *
    + 	 * N=1, M=0:
    + 	 *      tree vs cache (diff-index --cached)
    + 	 *
    ++	 * N=1, M=0, --cached:
    ++	 *      tree vs files (diff-index)
    ++	 *
      	 * N=2, M=0:
      	 *      tree vs tree (diff-tree)
    + 	 *
 builtin/diff.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)
Show changes to builtin/diff.c +15 −3
diff --git a/builtin/diff.c b/builtin/diff.c
index 8537b17bd5..0f1a882bd7 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -266,18 +266,30 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * We could get N tree-ish in the rev.pending_objects list.
-	 * Also there could be M blobs there, and P pathspecs.
+	 * Also there could be M blobs there, and P pathspecs. --cached may
+	 * also be present.
 	 *
 	 * N=0, M=0:
-	 *	cache vs files (diff-files)
+	 *      cache vs files (diff-files)
+	 *
+	 * N=0, M=0, --cached:
+	 *      HEAD vs cache (diff-index --cached)
+	 *
 	 * N=0, M=2:
 	 *      compare two random blobs.  P must be zero.
+	 *
 	 * N=0, M=1, P=1:
-	 *	compare a blob with a working tree file.
+	 *      compare a blob with a working tree file.
+	 *
+	 * N=1, M=0:
+	 *      tree vs files (diff-index)
 	 *
 	 * N=1, M=0:
 	 *      tree vs cache (diff-index --cached)
 	 *
+	 * N=1, M=0, --cached:
+	 *      tree vs files (diff-index)
+	 *
 	 * N=2, M=0:
 	 *      tree vs tree (diff-tree)
 	 *
-- 
2.27.0.132.g321788e831

← back to recent threads