[PATCH 0/3] fix "diff --raw" on the work tree side

The second patch fixes the inconsistency between "git-diff --raw" and
"git-diff-{index,files} --raw" when they are used for submodules.

The third one is a bit controversial and changes the semantics and
existing callers and won't be considered for 1.5.5.

Junio C Hamano (3):
      diff-lib.c: constness strengthening
      diff: make sure work tree side is shown as 0{40} when different
      diff: show submodule object name when available even on the work tree side

 diff-lib.c                |   45 +++++++++++++++++++++++++++++++------
 diff.c                    |    7 +----
 t/t4027-diff-submodule.sh |   53 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 92 insertions(+), 13 deletions(-)
 create mode 100755 t/t4027-diff-submodule.sh

The internal implementation of diff-index codepath used to use non const
pointer to pass sha1 around, but it did not have to.  With this, we can
also lose the private no_sha1[] array, as we can use the public null_sha1[]
array that exists exactly for the same purpose.

Signed-off-by: Junio C Hamano <redacted>
---
 diff-lib.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 94b150e..4581b59 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -472,22 +472,21 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 static void diff_index_show_file(struct rev_info *revs,
 				 const char *prefix,
 				 struct cache_entry *ce,
-				 unsigned char *sha1, unsigned int mode)
+				 const unsigned char *sha1, unsigned int mode)
 {
 	diff_addremove(&revs->diffopt, prefix[0], mode,
 		       sha1, ce->name, NULL);
 }
 
 static int get_stat_data(struct cache_entry *ce,
-			 unsigned char **sha1p,
+			 const unsigned char **sha1p,
 			 unsigned int *modep,
 			 int cached, int match_missing)
 {
-	unsigned char *sha1 = ce->sha1;
+	const unsigned char *sha1 = ce->sha1;
 	unsigned int mode = ce->ce_mode;
 
 	if (!cached) {
-		static unsigned char no_sha1[20];
 		int changed;
 		struct stat st;
 		if (lstat(ce->name, &st) < 0) {

@@ -501,7 +500,7 @@ static int get_stat_data(struct cache_entry *ce,
 		changed = ce_match_stat(ce, &st, 0);
 		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
-			sha1 = no_sha1;
+			sha1 = null_sha1;
 		}
 	}
 

@@ -514,7 +513,7 @@ static void show_new_file(struct rev_info *revs,
 			  struct cache_entry *new,
 			  int cached, int match_missing)
 {
-	unsigned char *sha1;
+	const unsigned char *sha1;
 	unsigned int mode;
 
 	/* New file in the index: it might actually be different in

@@ -533,7 +532,7 @@ static int show_modified(struct rev_info *revs,
 			 int cached, int match_missing)
 {
 	unsigned int mode, oldmode;
-	unsigned char *sha1;
+	const unsigned char *sha1;
 
 	if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0) {
 		if (report_missing)

-- 
1.5.4.3.468.g36990

Ping Yin noticed that "git diff-index --raw" shows 0{40} when work tree
has submodule difference, but "git diff --raw" didn't correctly do so.

There was a mistake in the diffcore_skip_stat_unmatch() that was meant to
clean up the stat-only difference for running diff between the index and
work tree and diff between the tree and the work tree, to cause it re-read
from the submodule repository HEAD.  When ce_stat_match() says work tree
is different, we should always say 0{40} on the work tree side.

This patch fixes the issue, and adds tests.

Signed-off-by: Junio C Hamano <redacted>
---

 * The real change is a net deletion of 3 lines.

 diff.c                    |    7 +----
 t/t4027-diff-submodule.sh |   53 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 5 deletions(-)
 create mode 100755 t/t4027-diff-submodule.sh

diff --git a/diff.c b/diff.c
index b80f656..00e1590 100644
--- a/diff.c
+++ b/diff.c

@@ -3187,11 +3187,8 @@ static void diffcore_apply_filter(const char *filter)
 static int diff_filespec_is_identical(struct diff_filespec *one,
 				      struct diff_filespec *two)
 {
-	if (S_ISGITLINK(one->mode)) {
-		diff_fill_sha1_info(one);
-		diff_fill_sha1_info(two);
-		return !hashcmp(one->sha1, two->sha1);
-	}
+	if (S_ISGITLINK(one->mode))
+		return 0;
 	if (diff_populate_filespec(one, 0))
 		return 0;
 	if (diff_populate_filespec(two, 0))

diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
new file mode 100755
index 0000000..3d2d081
--- /dev/null
+++ b/t/t4027-diff-submodule.sh

@@ -0,0 +1,53 @@
+#!/bin/sh
+
+test_description='difference in submodules'
+
+. ./test-lib.sh
+. ../diff-lib.sh
+
+_z40=0000000000000000000000000000000000000000
+test_expect_success setup '
+	test_tick &&
+	test_create_repo sub &&
+	(
+		cd sub &&
+		echo hello >world &&
+		git add world &&
+		git commit -m submodule
+	) &&
+
+	test_tick &&
+	echo frotz >nitfol &&
+	git add nitfol sub &&
+	git commit -m superproject &&
+
+	(
+		cd sub &&
+		echo goodbye >world &&
+		git add world &&
+		git commit -m "submodule #2"
+	) &&
+
+	set x $(
+		cd sub &&
+		git rev-list HEAD
+	) &&
+	echo ":160000 160000 $3 $_z40 M	sub" >expect
+'
+
+test_expect_success 'git diff --raw HEAD' '
+	git diff --raw --abbrev=40 HEAD >actual &&
+	diff -u expect actual
+'
+
+test_expect_success 'git diff-index --raw HEAD' '
+	git diff-index --raw HEAD >actual.index &&
+	diff -u expect actual.index
+'
+
+test_expect_success 'git diff-files --raw' '
+	git diff-files --raw >actual.files &&
+	diff -u expect actual.files
+'
+
+test_done

-- 
1.5.4.3.468.g36990

Traditionally, we consistently showed 0{40} when work tree side was
different from what it was being compared against.  This was primarily
because it did not make sense to re-hash potentially large blobs every
time diff-files and non-cached diff-index were asked for.

However, we can afford to read from submodule HEAD, as it is much cheaper
than hashing blobs.

This makes the output somewhat inconsistent, but it probably is a good
move to give easily available information than not giving it.

Signed-off-by: Junio C Hamano <redacted>
---

 * This could be enhanced to also hash symlink blobs as they tend to be
   very short.

   HOWEVER.

   This changes the semantics established since almost day-one of git (at
   least this 0{40} convention can be traced back to show-diff as of Apr
   26, 2005), and tests (e.g. t3040) demonstrate that it breaks existing
   callers such as git-commit.

   Which means that I am sending this out only for discussion at this
   time, not for inclusion in the near term.

 diff-lib.c                |   32 +++++++++++++++++++++++++++++++-
 t/t4027-diff-submodule.sh |    2 +-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 4581b59..94e17a6 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -10,6 +10,26 @@
 #include "cache-tree.h"
 #include "path-list.h"
 #include "unpack-trees.h"
+#include "refs.h"
+
+/*
+ * Traditionally, we have _always_ showed 0{40} on the work tree
+ * side if there is a difference.  However, reading from HEAD of
+ * a submodule is much cheaper than rehashing regular blob objects.
+ */
+static const unsigned char *get_gitlink_data(const char *path)
+{
+	static unsigned char subhead[20];
+	/*
+	 * NOTE: strictly speaking, this makes it non re-entrant, but
+	 * we know the nature of the callchain makes it very
+	 * short-lived --- the caller will give this to
+	 * diff_addremove() or diff_change() immediately.
+	 */
+	if (!resolve_gitlink_ref(path, "HEAD", subhead))
+		return subhead;
+	return null_sha1;
+}
 
 /*
  * diff-files

@@ -349,6 +369,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		struct stat st;
 		unsigned int oldmode, newmode;
 		struct cache_entry *ce = active_cache[i];
+		const unsigned char *worktree_sha1;
 		int changed;
 
 		if (DIFF_OPT_TST(&revs->diffopt, QUIET) &&

@@ -454,8 +475,15 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			continue;
 		oldmode = ce->ce_mode;
 		newmode = ce_mode_from_stat(ce, st.st_mode);
+
+		if (S_ISGITLINK(newmode))
+			worktree_sha1 = get_gitlink_data(ce->name);
+		else if (changed)
+			worktree_sha1 = null_sha1;
+		else
+			worktree_sha1 = ce->sha1;
 		diff_change(&revs->diffopt, oldmode, newmode,
-			    ce->sha1, (changed ? null_sha1 : ce->sha1),
+			    ce->sha1, worktree_sha1,
 			    ce->name, NULL);
 
 	}

@@ -501,6 +529,8 @@ static int get_stat_data(struct cache_entry *ce,
 		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			sha1 = null_sha1;
+			if (S_ISGITLINK(mode))
+				sha1 = get_gitlink_data(ce->name);
 		}
 	}
 

diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 3d2d081..c1c2500 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh

@@ -32,7 +32,7 @@ test_expect_success setup '
 		cd sub &&
 		git rev-list HEAD
 	) &&
-	echo ":160000 160000 $3 $_z40 M	sub" >expect
+	echo ":160000 160000 $3 $2 M	sub" >expect
 '
 
 test_expect_success 'git diff --raw HEAD' '

-- 
1.5.4.3.468.g36990

On Sun, Mar 2, 2008 at 5:43 PM, Junio C Hamano [off-list ref] wrote:

The second patch fixes the inconsistency between "git-diff --raw" and
 "git-diff-{index,files} --raw" when they are used for submodules.

 The third one is a bit controversial and changes the semantics and
 existing callers and won't be considered for 1.5.5.

Unfortunately, my submodule summary patch series will depend on
git-diff or git-diff-index. So should i resend my improved patch only
after the thrid one is applied?


-- 
Ping Yin

"Ping Yin" [off-list ref] writes:

On Sun, Mar 2, 2008 at 5:43 PM, Junio C Hamano [off-list ref] wrote:

quoted

The second patch fixes the inconsistency between "git-diff --raw" and
 "git-diff-{index,files} --raw" when they are used for submodules.

 The third one is a bit controversial and changes the semantics and
 existing callers and won't be considered for 1.5.5.

Unfortunately, my submodule summary patch series will depend on
git-diff or git-diff-index. So should i resend my improved patch only
after the thrid one is applied?

I do not think re-introducing the inconsistency like the third one does is
a palatable option.

Wouldn't grabbing (cd $submodule && git rev-parse HEAD) yourself be more
portable across git before and after the bugfix of "git diff --raw"?

On 3/3/08, Junio C Hamano [off-list ref] wrote:

"Ping Yin" [off-list ref] writes:

quoted

On Sun, Mar 2, 2008 at 5:43 PM, Junio C Hamano [off-list ref] wrote:

quoted

The second patch fixes the inconsistency between "git-diff --raw" and
 "git-diff-{index,files} --raw" when they are used for submodules.

 The third one is a bit controversial and changes the semantics and
 existing callers and won't be considered for 1.5.5.

Unfortunately, my submodule summary patch series will depend on
git-diff or git-diff-index. So should i resend my improved patch only
after the thrid one is applied?

I do not think re-introducing the inconsistency like the third one does is
a palatable option.

Wouldn't grabbing (cd $submodule && git rev-parse HEAD) yourself be more
portable across git before and after the bugfix of "git diff --raw"?

OK, i will parse it myself


-- 
Ping Yin