Possible d/f conflict bug or regression

Possible d/f conflict bug or regression

[Christian Couder]

Hi,

When doing something like:

mkdir testdir &&
        cd testdir &&
        touch foo &&
        git init &&
        git add . &&
        git commit -m 'Initial commit.' &&
        rm foo &&
        mkdir foo &&
        git commit -a -m 'Test.'

I get:

Initialized empty Git repository in .git/
Created initial commit 3f945ca: Initial commit.
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo
fatal: unable to index file foo

I think it's quite bad that it doesn't work.

It seems it also doesn't work when adding "touch foo/bar" before "git 
commit -a -m 'Test.'".

I used:

$ git --version
git version 1.5.5.rc2

It worked with 1.5.3 and I bisected it to the git commit port to C:

commit f5bbc3225c4b073a7ff3218164a0c820299bc9c6
Author: Kristian H<C3><B8>gsberg <krh@redhat.com>
Date:   Thu Nov 8 11:59:00 2007 -0500

    Port git commit to C.

    This makes git commit a builtin and moves git-commit.sh to
    contrib/examples.  This also removes the git-runstatus
    helper, which was mostly just a git-status.sh implementation detail.

    Signed-off-by: Kristian H<C3><B8>gsberg <krh@redhat.com>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Thanks,
Christian.

Re: Possible d/f conflict bug or regression

[Christian Couder]

Le samedi 29 mars 2008, Christian Couder a écrit :
>
> mkdir testdir &&
>         cd testdir &&
>         touch foo &&
>         git init &&
>         git add . &&
>         git commit -m 'Initial commit.' &&
>         rm foo &&
>         mkdir foo &&
>         git commit -a -m 'Test.'

I don't know if this helps but with "git rm foo" instead of "rm foo" it 
works like this:

Initialized empty Git repository in .git/
Created initial commit e784a71: Initial commit.
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 foo
rm 'foo'
Created commit 232e3ae: Test.
 0 files changed, 0 insertions(+), 0 deletions(-)
 delete mode 100644 foo

Christian.

Re: Possible d/f conflict bug or regression

[Bryan Donlan]

On Sat, Mar 29, 2008 at 3:13 AM, Christian Couder
<chriscool@tuxfamily.org> wrote:
> Hi,
>
>  When doing something like:
>
>  mkdir testdir &&
>         cd testdir &&
>         touch foo &&
>         git init &&
>         git add . &&
>         git commit -m 'Initial commit.' &&
>         rm foo &&
>         mkdir foo &&
>         git commit -a -m 'Test.'
>
>  I get:
>
>  Initialized empty Git repository in .git/
>  Created initial commit 3f945ca: Initial commit.
>   0 files changed, 0 insertions(+), 0 deletions(-)
>   create mode 100644 foo
>  fatal: unable to index file foo
>
>  I think it's quite bad that it doesn't work.

What behavior would you expect this to have? IMO, it's not entirely
clear what the user means to do if they replace a file with an empty
directory, as an empty directory cannot be added to the index. Even
with a directory with contents, some of the contents may be junk (.o
for example) as far as the user is concerned.

Would a clearer diagnostic be a good solution? Something like:
fatal: foo: file replaced by directory.
Use git rm --cached or git add to specify how this should be handled.

Thanks,

Bryan

Re: Possible d/f conflict bug or regression

[Christian Couder]

Le dimanche 30 mars 2008, Bryan Donlan a écrit :
> On Sat, Mar 29, 2008 at 3:13 AM, Christian Couder
> <chriscool@tuxfamily.org> wrote:
> >
> >  Initialized empty Git repository in .git/
> >  Created initial commit 3f945ca: Initial commit.
> >   0 files changed, 0 insertions(+), 0 deletions(-)
> >   create mode 100644 foo
> >  fatal: unable to index file foo
> >
> >  I think it's quite bad that it doesn't work.
>
> What behavior would you expect this to have? IMO, it's not entirely 
> clear what the user means to do if they replace a file with an empty
> directory, as an empty directory cannot be added to the index. Even
> with a directory with contents, some of the contents may be junk (.o
> for example) as far as the user is concerned.

I think Git should behave the same as when using "git rm foo" instead of "rm 
foo", that is the file "foo" should be deleted without errors. That's what 
version 1.5.3 did too.

> Would a clearer diagnostic be a good solution? Something like:
> fatal: foo: file replaced by directory.
> Use git rm --cached or git add to specify how this should be handled.

No, I think we should fix the regression. Using "git rm stuff" instead 
of "rm stuff" should not be required.

Regards,
Christian.

Re: Possible d/f conflict bug or regression

[Bryan Donlan]

On Sun, Mar 30, 2008 at 12:44 AM, Christian Couder
<chriscool@tuxfamily.org> wrote:
> Le dimanche 30 mars 2008, Bryan Donlan a écrit :
>
> > On Sat, Mar 29, 2008 at 3:13 AM, Christian Couder
>  > <chriscool@tuxfamily.org> wrote:
>  > >
>
> > >  Initialized empty Git repository in .git/
>  > >  Created initial commit 3f945ca: Initial commit.
>  > >   0 files changed, 0 insertions(+), 0 deletions(-)
>  > >   create mode 100644 foo
>  > >  fatal: unable to index file foo
>  > >
>  > >  I think it's quite bad that it doesn't work.
>  >
>  > What behavior would you expect this to have? IMO, it's not entirely
>  > clear what the user means to do if they replace a file with an empty
>  > directory, as an empty directory cannot be added to the index. Even
>  > with a directory with contents, some of the contents may be junk (.o
>  > for example) as far as the user is concerned.
>
>  I think Git should behave the same as when using "git rm foo" instead of "rm
>  foo", that is the file "foo" should be deleted without errors. That's what
>  version 1.5.3 did too.
>
>
>  > Would a clearer diagnostic be a good solution? Something like:
>  > fatal: foo: file replaced by directory.
>  > Use git rm --cached or git add to specify how this should be handled.
>
>  No, I think we should fix the regression. Using "git rm stuff" instead
>  of "rm stuff" should not be required.

This is inconsistent with git's behavior when replacing a file with a
symlink then - you can rm file; ln -s something file, and the symlink
will be checked in...

As-is, if you "rm stuff" but do not "mkdir stuff", you can commit
without problems. Likewise, you can "rm stuff", and "echo foo >
stuff", and the file will be updated. "rm stuff" -> "mkdir stuff; vim
stuff/bar.c" could equally imply that the user wants to replace
"stuff" with a directory, could it not?

I don't think git should be inconsistent in this case, but equally
it's difficult to know what the user wants to do if they put in an
empty directory... That's why I think it'd be more sensible to let the
user know so they can decide which action they want to take. It
shouldn't happen often anyway; I'd be interested in hearing about a
use-case that involves frequent replacement of files with directories,
though :)

Thanks,

Bryan

Re: Possible d/f conflict bug or regression

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> mkdir testdir &&
>         cd testdir &&
>         touch foo &&
>         git init &&
>         git add . &&
>         git commit -m 'Initial commit.' &&
>         rm foo &&
>         mkdir foo &&
>         git commit -a -m 'Test.'
>
> I get:
>
> Initialized empty Git repository in .git/
> Created initial commit 3f945ca: Initial commit.
>  0 files changed, 0 insertions(+), 0 deletions(-)
>  create mode 100644 foo
> fatal: unable to index file foo

I haven't had time to fully clean-up the patch series, but I have a fix
for this (and a bit broader set of cases).  "git add -u" shares the same
issue as the "git commit -a" at the last step in your sequence.

"commit -a" and "add -u" are about "check the index and work tree to
see if anything that is in the index is changed in the work tree, and
update the entry (either remove or add)".  When we are looking at an
existing index entry "foo", possible cases include:

 - it has not been changed (=> do nothing);
 - it is not there anymore (=> do "git update-index --add --remove foo")
 - its contents or executableness changed (ditto);
 - its type changed (e.g. reg-to-symlink) (ditto);

If you did "rm foo; mkdir foo", then that is "it is not there anymore"
case.

If you did "rm foo; mkdir foo; (cd foo && git init)", and worked in this
new foo/ repository to cause its HEAD to point at a commit, then that is
"its type changed to a gitlink" case (iow, you added a submodule).

The above two cases are not handled properly; the breakage is in
diff-files but non-cached diff-index shares the same issue.  The root
cause is not Kristian's "rewrite commit in C", but is much more older
"gitlink to support submodules" series, that started at f35a6d3 (Teach
core object handling functions about gitlinks, 2007-04-09).

Will send the patches out tonight but I have to tend some other chores
first.

[PATCH 1/3] Add corner case tests for diff-index and diff-files

[Junio C Hamano]

diff-index and diff-files can get confused in corner cases when an indexed
blob turns into something else in the work tree.  This patch adds tests to
expose such breakages.

The test is classified under t2XXX series instead of t4XXX series, because
the ultimate objective is to fix "add -u" (and "commit -a" that shares the
same issue).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t2201-add-update-typechange.sh |  140 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 140 insertions(+), 0 deletions(-)
 create mode 100755 t/t2201-add-update-typechange.sh

diff --git a/t/t2201-add-update-typechange.sh b/t/t2201-add-update-typechange.sh
new file mode 100755
index 0000000..75c440c
--- /dev/null
+++ b/t/t2201-add-update-typechange.sh
@@ -0,0 +1,140 @@
+#!/bin/sh
+
+test_description='more git add -u'
+
+. ./test-lib.sh
+
+_z40=0000000000000000000000000000000000000000
+
+test_expect_success setup '
+	>xyzzy &&
+	_empty=$(git hash-object --stdin <xyzzy) &&
+	>yomin &&
+	>caskly &&
+	ln -s frotz nitfol &&
+	mkdir rezrov &&
+	>rezrov/bozbar &&
+	git add caskly xyzzy yomin nitfol rezrov/bozbar &&
+
+	test_tick &&
+	git commit -m initial
+
+'
+
+test_expect_success modify '
+	rm -f xyzzy yomin nitfol caskly &&
+	# caskly disappears (not a submodule)
+	mkdir caskly &&
+	# nitfol changes from symlink to regular
+	>nitfol &&
+	# rezrov/bozbar disappears
+	rm -fr rezrov &&
+	ln -s xyzzy rezrov &&
+	# xyzzy disappears (not a submodule)
+	mkdir xyzzy &&
+	echo gnusto >xyzzy/bozbar &&
+	# yomin gets replaced with a submodule
+	mkdir yomin &&
+	>yomin/yomin &&
+	(
+		cd yomin &&
+		git init &&
+		git add yomin &&
+		git commit -m "sub initial"
+	) &&
+	yomin=$(GIT_DIR=yomin/.git git rev-parse HEAD) &&
+	# yonk is added and then turned into a submodule
+	# this should appear as T in diff-files and as A in diff-index
+	>yonk &&
+	git add yonk &&
+	rm -f yonk &&
+	mkdir yonk &&
+	>yonk/yonk &&
+	(
+		cd yonk &&
+		git init &&
+		git add yonk &&
+		git commit -m "sub initial"
+	) &&
+	yonk=$(GIT_DIR=yonk/.git git rev-parse HEAD) &&
+	# zifmia is added and then removed
+	# this should appear in diff-files but not in diff-index.
+	>zifmia &&
+	git add zifmia &&
+	rm -f zifmia &&
+	mkdir zifmia &&
+	{
+		git ls-tree -r HEAD |
+		sed -e "s/^/:/" -e "
+			/	caskly/{
+				s/	caskly/ $_z40 D&/
+				s/blob/000000/
+			}
+			/	nitfol/{
+				s/	nitfol/ $_z40 T&/
+				s/blob/100644/
+			}
+			/	rezrov.bozbar/{
+				s/	rezrov.bozbar/ $_z40 D&/
+				s/blob/000000/
+			}
+			/	xyzzy/{
+				s/	xyzzy/ $_z40 D&/
+				s/blob/000000/
+			}
+			/	yomin/{
+			    s/	yomin/ $_z40 T&/
+				s/blob/160000/
+			}
+		"
+	} >expect &&
+	{
+		cat expect
+		echo ":100644 160000 $_empty $_z40 T	yonk"
+		echo ":100644 000000 $_empty $_z40 D	zifmia"
+	} >expect-files &&
+	{
+		cat expect
+		echo ":000000 160000 $_z40 $_z40 A	yonk"
+	} >expect-index &&
+	{
+		echo "100644 $_empty 0	nitfol"
+		echo "160000 $yomin 0	yomin"
+		echo "160000 $yonk 0	yonk"
+	} >expect-final
+'
+
+test_expect_failure diff-files '
+	git diff-files --raw >actual &&
+	diff -u expect-files actual
+'
+
+test_expect_failure diff-index '
+	git diff-index --raw HEAD -- >actual &&
+	diff -u expect-index actual
+'
+
+test_expect_failure 'add -u' '
+	rm -f ".git/saved-index" &&
+	cp -p ".git/index" ".git/saved-index" &&
+	git add -u &&
+	git ls-files -s >actual &&
+	diff -u expect-final actual
+'
+
+test_expect_failure 'commit -a' '
+	if test -f ".git/saved-index"
+	then
+		rm -f ".git/index" &&
+		mv ".git/saved-index" ".git/index"
+	fi &&
+	git commit -m "second" -a &&
+	git ls-files -s >actual &&
+	diff -u expect-final actual &&
+	rm -f .git/index &&
+	git read-tree HEAD &&
+	git ls-files -s >actual &&
+	diff -u expect-final actual
+'
+
+test_done
-- 
1.5.5.rc2.131.g3d2f0

[PATCH 2/3] diff-index: careful when inspecting work tree items

[Junio C Hamano]

Earlier, if you changed a staged path into a directory in the work tree,
we happily ran lstat(2) on it and found that it exists, and declared that
the user changed it to a gitlink.

This is wrong for two reasons:

 (1) It may be a directory, but it may not be a submodule, and in the
     latter case, the change we need to report is "the blob at the path
     has disappeared".  We need to check with resolve_gitlink_ref() to be
     consistent with what "git add" and "git update-index --add" does.

 (2) lstat(2) may have succeeded only because a leading component of the
     path was turned into a symbolic link that points at something that
     exists in the work tree.  In such a case, the path itself does not
     exist anymore, as far as the index is concerned.

This fixes these breakages in diff-index that the previous patch has
exposed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-lib.c                       |   69 ++++++++++++++++++++++++++++++--------
 t/t2201-add-update-typechange.sh |    2 +-
 2 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 52dbac3..a8e107a 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -10,6 +10,7 @@
 #include "cache-tree.h"
 #include "path-list.h"
 #include "unpack-trees.h"
+#include "refs.h"
 
 /*
  * diff-files
@@ -333,6 +334,26 @@ int run_diff_files_cmd(struct rev_info *revs, int argc, const char **argv)
 	}
 	return run_diff_files(revs, options);
 }
+/*
+ * See if work tree has an entity that can be staged.  Return 0 if so,
+ * return 1 if not and return -1 if error.
+ */
+static int check_work_tree_entity(const struct cache_entry *ce, struct stat *st, char *symcache)
+{
+	if (lstat(ce->name, st) < 0) {
+		if (errno != ENOENT && errno != ENOTDIR)
+			return -1;
+		return 1;
+	}
+	if (has_symlink_leading_path(ce->name, symcache))
+		return 1;
+	if (S_ISDIR(st->st_mode)) {
+		unsigned char sub[20];
+		if (resolve_gitlink_ref(ce->name, "HEAD", sub))
+			return 1;
+	}
+	return 0;
+}
 
 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
@@ -468,6 +489,11 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
  * diff-index
  */
 
+struct oneway_unpack_data {
+	struct rev_info *revs;
+	char symcache[PATH_MAX];
+};
+
 /* A file entry went away or appeared */
 static void diff_index_show_file(struct rev_info *revs,
 				 const char *prefix,
@@ -481,7 +507,8 @@ static void diff_index_show_file(struct rev_info *revs,
 static int get_stat_data(struct cache_entry *ce,
 			 const unsigned char **sha1p,
 			 unsigned int *modep,
-			 int cached, int match_missing)
+			 int cached, int match_missing,
+			 struct oneway_unpack_data *cbdata)
 {
 	const unsigned char *sha1 = ce->sha1;
 	unsigned int mode = ce->ce_mode;
@@ -489,8 +516,11 @@ static int get_stat_data(struct cache_entry *ce,
 	if (!cached) {
 		int changed;
 		struct stat st;
-		if (lstat(ce->name, &st) < 0) {
-			if (errno == ENOENT && match_missing) {
+		changed = check_work_tree_entity(ce, &st, NULL);
+		if (changed < 0)
+			return -1;
+		else if (changed) {
+			if (match_missing) {
 				*sha1p = sha1;
 				*modep = mode;
 				return 0;
@@ -509,23 +539,25 @@ static int get_stat_data(struct cache_entry *ce,
 	return 0;
 }
 
-static void show_new_file(struct rev_info *revs,
+static void show_new_file(struct oneway_unpack_data *cbdata,
 			  struct cache_entry *new,
 			  int cached, int match_missing)
 {
 	const unsigned char *sha1;
 	unsigned int mode;
+	struct rev_info *revs = cbdata->revs;
 
-	/* New file in the index: it might actually be different in
+	/*
+	 * New file in the index: it might actually be different in
 	 * the working copy.
 	 */
-	if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0)
+	if (get_stat_data(new, &sha1, &mode, cached, match_missing, cbdata) < 0)
 		return;
 
 	diff_index_show_file(revs, "+", new, sha1, mode);
 }
 
-static int show_modified(struct rev_info *revs,
+static int show_modified(struct oneway_unpack_data *cbdata,
 			 struct cache_entry *old,
 			 struct cache_entry *new,
 			 int report_missing,
@@ -533,8 +565,9 @@ static int show_modified(struct rev_info *revs,
 {
 	unsigned int mode, oldmode;
 	const unsigned char *sha1;
+	struct rev_info *revs = cbdata->revs;
 
-	if (get_stat_data(new, &sha1, &mode, cached, match_missing) < 0) {
+	if (get_stat_data(new, &sha1, &mode, cached, match_missing, cbdata) < 0) {
 		if (report_missing)
 			diff_index_show_file(revs, "-", old,
 					     old->sha1, old->ce_mode);
@@ -602,7 +635,8 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	struct cache_entry *idx,
 	struct cache_entry *tree)
 {
-	struct rev_info *revs = o->unpack_data;
+	struct oneway_unpack_data *cbdata = o->unpack_data;
+	struct rev_info *revs = cbdata->revs;
 	int match_missing, cached;
 
 	/*
@@ -625,7 +659,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	 * Something added to the tree?
 	 */
 	if (!tree) {
-		show_new_file(revs, idx, cached, match_missing);
+		show_new_file(cbdata, idx, cached, match_missing);
 		return;
 	}
 
@@ -638,7 +672,7 @@ static void do_oneway_diff(struct unpack_trees_options *o,
 	}
 
 	/* Show difference between old and new */
-	show_modified(revs, tree, idx, 1, cached, match_missing);
+	show_modified(cbdata, tree, idx, 1, cached, match_missing);
 }
 
 static inline void skip_same_name(struct cache_entry *ce, struct unpack_trees_options *o)
@@ -675,7 +709,8 @@ static int oneway_diff(struct cache_entry **src, struct unpack_trees_options *o)
 {
 	struct cache_entry *idx = src[0];
 	struct cache_entry *tree = src[1];
-	struct rev_info *revs = o->unpack_data;
+	struct oneway_unpack_data *cbdata = o->unpack_data;
+	struct rev_info *revs = cbdata->revs;
 
 	if (idx && ce_stage(idx))
 		skip_same_name(idx, o);
@@ -702,6 +737,7 @@ int run_diff_index(struct rev_info *revs, int cached)
 	const char *tree_name;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
+	struct oneway_unpack_data unpack_cb;
 
 	mark_merge_entries();
 
@@ -711,12 +747,14 @@ int run_diff_index(struct rev_info *revs, int cached)
 	if (!tree)
 		return error("bad tree object %s", tree_name);
 
+	unpack_cb.revs = revs;
+	unpack_cb.symcache[0] = '\0';
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 1;
 	opts.index_only = cached;
 	opts.merge = 1;
 	opts.fn = oneway_diff;
-	opts.unpack_data = revs;
+	opts.unpack_data = &unpack_cb;
 	opts.src_index = &the_index;
 	opts.dst_index = NULL;
 
@@ -738,6 +776,7 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	struct cache_entry *last = NULL;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
+	struct oneway_unpack_data unpack_cb;
 
 	/*
 	 * This is used by git-blame to run diff-cache internally;
@@ -766,12 +805,14 @@ int do_diff_cache(const unsigned char *tree_sha1, struct diff_options *opt)
 	if (!tree)
 		die("bad tree object %s", sha1_to_hex(tree_sha1));
 
+	unpack_cb.revs = &revs;
+	unpack_cb.symcache[0] = '\0';
 	memset(&opts, 0, sizeof(opts));
 	opts.head_idx = 1;
 	opts.index_only = 1;
 	opts.merge = 1;
 	opts.fn = oneway_diff;
-	opts.unpack_data = &revs;
+	opts.unpack_data = &unpack_cb;
 	opts.src_index = &the_index;
 	opts.dst_index = &the_index;
 
diff --git a/t/t2201-add-update-typechange.sh b/t/t2201-add-update-typechange.sh
index 75c440c..469a8e0 100755
--- a/t/t2201-add-update-typechange.sh
+++ b/t/t2201-add-update-typechange.sh
@@ -109,7 +109,7 @@ test_expect_failure diff-files '
 	diff -u expect-files actual
 '
 
-test_expect_failure diff-index '
+test_expect_success diff-index '
 	git diff-index --raw HEAD -- >actual &&
 	diff -u expect-index actual
 '
-- 
1.5.5.rc2.131.g3d2f0

[PATCH 3/3] diff-files: careful when inspecting work tree items

[Junio C Hamano]

This fixes the same breakage in diff-files.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 diff-lib.c                       |   17 +++++++++++------
 t/t2201-add-update-typechange.sh |    6 +++---
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index a8e107a..d4ad6b6 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -362,10 +362,12 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 	int silent_on_removed = option & DIFF_SILENT_ON_REMOVED;
 	unsigned ce_option = ((option & DIFF_RACY_IS_MODIFIED)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
+	char symcache[PATH_MAX];
 
 	if (diff_unmerged_stage < 0)
 		diff_unmerged_stage = 2;
 	entries = active_nr;
+	symcache[0] = '\0';
 	for (i = 0; i < entries; i++) {
 		struct stat st;
 		unsigned int oldmode, newmode;
@@ -397,16 +399,17 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			memset(&(dpath->parent[0]), 0,
 			       sizeof(struct combine_diff_parent)*5);
 
-			if (lstat(ce->name, &st) < 0) {
-				if (errno != ENOENT && errno != ENOTDIR) {
+			changed = check_work_tree_entity(ce, &st, symcache);
+			if (!changed)
+				dpath->mode = ce_mode_from_stat(ce, st.st_mode);
+			else {
+				if (changed < 0) {
 					perror(ce->name);
 					continue;
 				}
 				if (silent_on_removed)
 					continue;
 			}
-			else
-				dpath->mode = ce_mode_from_stat(ce, st.st_mode);
 
 			while (i < entries) {
 				struct cache_entry *nce = active_cache[i];
@@ -459,8 +462,10 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 
 		if (ce_uptodate(ce))
 			continue;
-		if (lstat(ce->name, &st) < 0) {
-			if (errno != ENOENT && errno != ENOTDIR) {
+
+		changed = check_work_tree_entity(ce, &st, symcache);
+		if (changed) {
+			if (changed < 0) {
 				perror(ce->name);
 				continue;
 			}
diff --git a/t/t2201-add-update-typechange.sh b/t/t2201-add-update-typechange.sh
index 469a8e0..e15e3eb 100755
--- a/t/t2201-add-update-typechange.sh
+++ b/t/t2201-add-update-typechange.sh
@@ -104,7 +104,7 @@ test_expect_success modify '
 	} >expect-final
 '
 
-test_expect_failure diff-files '
+test_expect_success diff-files '
 	git diff-files --raw >actual &&
 	diff -u expect-files actual
 '
@@ -114,7 +114,7 @@ test_expect_success diff-index '
 	diff -u expect-index actual
 '
 
-test_expect_failure 'add -u' '
+test_expect_success 'add -u' '
 	rm -f ".git/saved-index" &&
 	cp -p ".git/index" ".git/saved-index" &&
 	git add -u &&
@@ -122,7 +122,7 @@ test_expect_failure 'add -u' '
 	diff -u expect-final actual
 '
 
-test_expect_failure 'commit -a' '
+test_expect_success 'commit -a' '
 	if test -f ".git/saved-index"
 	then
 		rm -f ".git/index" &&
-- 
1.5.5.rc2.131.g3d2f0

Re: [PATCH 2/3] diff-index: careful when inspecting work tree items

[Junio C Hamano]

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

> diff --git a/diff-lib.c b/diff-lib.c
> index 52dbac3..a8e107a 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> ...
> @@ -489,8 +516,11 @@ static int get_stat_data(struct cache_entry *ce,
>  	if (!cached) {
>  		int changed;
>  		struct stat st;
> -		if (lstat(ce->name, &st) < 0) {
> -			if (errno == ENOENT && match_missing) {
> +		changed = check_work_tree_entity(ce, &st, NULL);

This "NULL" should be "cbdata->symcache".

Re: [PATCH 2/3] diff-index: careful when inspecting work tree items

[Christian Couder]

Le lundi 31 mars 2008, Junio C Hamano a écrit :
> Junio C Hamano <gitster@pobox.com> writes:
> > diff --git a/diff-lib.c b/diff-lib.c
> > index 52dbac3..a8e107a 100644
> > --- a/diff-lib.c
> > +++ b/diff-lib.c
> > ...
> > @@ -489,8 +516,11 @@ static int get_stat_data(struct cache_entry *ce,
> >  	if (!cached) {
> >  		int changed;
> >  		struct stat st;
> > -		if (lstat(ce->name, &st) < 0) {
> > -			if (errno == ENOENT && match_missing) {
> > +		changed = check_work_tree_entity(ce, &st, NULL);
>
> This "NULL" should be "cbdata->symcache".

Tested-by: Christian Couder <chriscool@tuxfamily.org>

Your patch series works fine for me.

Thanks,
Christian.