[PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.

[PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.

[Johannes Sixt]

After this sequence:

$ ln -s foo A
$ git add A
$ git commit -m link
$ rm A && echo bar > A

the working copy contains a regular file A but HEAD contains A as a link.

Normally, at this point 'git diff HEAD' displays this change as a removal
of the link followed by an addition of a new file. But in a repository where
core.filemode is false this is displayed as a modification of the link.
The reason is that the check when the cached mode is allowed to override the
file's actual mode is not strict enough.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 diff-lib.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 91cd877..5fc1910 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -171,7 +171,8 @@ static int get_stat_data(struct cache_entry *ce,
 		changed = ce_match_stat(ce, &st, 0);
 		if (changed) {
 			mode = create_ce_mode(st.st_mode);
-			if (!trust_executable_bit && S_ISREG(st.st_mode))
+			if (!trust_executable_bit &&
+			    S_ISREG(st.st_mode) && S_ISREG(ntohl(ce->ce_mode)))
 				mode = ce->ce_mode;
 			sha1 = no_sha1;
 		}
-- 
1.5.0.19.gddff

Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> After this sequence:
>
> $ ln -s foo A
> $ git add A
> $ git commit -m link
> $ rm A && echo bar > A
>
> the working copy contains a regular file A but HEAD contains A as a link.
>
> Normally, at this point 'git diff HEAD' displays this change as a removal
> of the link followed by an addition of a new file. But in a repository where
> core.filemode is false this is displayed as a modification of the link.
> The reason is that the check when the cached mode is allowed to override the
> file's actual mode is not strict enough.
>
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>

I do not follow.  core.filemode aka trust_executable_bit is
about executable bit and not symlink.

Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.

[Johannes Sixt]

On Saturday 17 February 2007 00:13, Junio C Hamano wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> > After this sequence:
> >
> > $ ln -s foo A
> > $ git add A
> > $ git commit -m link
> > $ rm A && echo bar > A
> >
> > the working copy contains a regular file A but HEAD contains A as a link.
> >
> > Normally, at this point 'git diff HEAD' displays this change as a removal
> > of the link followed by an addition of a new file. But in a repository
> > where core.filemode is false this is displayed as a modification of the
> > link. The reason is that the check when the cached mode is allowed to
> > override the file's actual mode is not strict enough.
> >
> > Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
>
> I do not follow.  core.filemode aka trust_executable_bit is
> about executable bit and not symlink.

Correct. But the breakage is still there. As you recall, the properties 
S_IFREG and S_IFLNK are recorded in the st_mode, and the line of code that I 
fixed trusts the index in too many cases, and copies the index's S_IFLNK over 
the the stat() result, which said S_IFREG. So the diff engine loses the 
information that it is should diff against a regular file, and thinks that 
the working copy is a symlink.

Please change the last sentence of the commit message, which hopefully makes 
the issue a bit clearer:

The fix is that the cached mode must only be allowed to override the file's 
actual mode (which includes the type information) if _both_ the working tree 
entry and the cached entry are regular files.

-- Hannes

Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false.

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> The fix is that the cached mode must only be allowed to override the file's 
> actual mode (which includes the type information) if _both_ the working tree 
> entry and the cached entry are regular files.

Ah, I misunderstood.

I suspect you do not want to get random, unreliable executable
bit from lstat(), even when previously index recorded non
regular file for a path that is now a regular file.  In
builtin-update-index.c, add_file_to_cache() has this:

	ce->ce_mode = create_ce_mode(st.st_mode);
	if (!trust_executable_bit) {
		/* If there is an existing entry, pick the mode bits
		 * from it, otherwise assume unexecutable.
		 */
		int pos = cache_name_pos(path, namelen);
		if (0 <= pos)
			ce->ce_mode = active_cache[pos]->ce_mode;
		else if (S_ISREG(st.st_mode))
			ce->ce_mode = create_ce_mode(S_IFREG | 0666);
	}

and I agree what it _tries_ to do, although I think the above
also makes the regular file being added to a symlink and needs
to be fixed.  You helped us find another bug.

	# In a new, empty directory...
	$ git init
        $ ln -s foo A
        $ git add A
        $ git ls-files -s
	120000 19102815663d23f8b75a47e7a01965dcdc96468c 0	A
        $ git config core.filemode false
        $ rm A ; echo foo >A
        $ git add A
        $ git ls-files -s
	120000 19102815663d23f8b75a47e7a01965dcdc96468c 0	A

The fix is probably like this:

	ce->ce_mode = create_ce_mode(st.st_mode);
	if (!trust_executable_bit && S_ISREG(st.st_mode)) {
		/* If there is an existing entry, pick the mode bits
		 * from it, otherwise assume unexecutable.
		 */
		int pos = cache_name_pos(path, namelen);
		if (0 <= pos &&
                    S_ISREG(ntohl(active_cache[pos]->ce_mode)))
			ce->ce_mode = active_cache[pos]->ce_mode;
		else 
			ce->ce_mode = create_ce_mode(S_IFREG | 0666);
	}

Back to the part of the code you patched, I think the fix should
be something like this instead:

	mode = create_ce_mode(st.st_mode);
	if (!trust_executable_bit && S_ISREG(st.st_mode)) {
		/* If there is an existing entry, pick the mode bits
		 * from it, otherwise assume unexecutable.
		 */
                if (S_ISREG(ntohl(ce->ce_mode)))
			mode = ce->ce_mode;
                else
                	mode = create_ce_mode(S_IFREG | 0666);
                
        }

[PATCH] Do not take mode bits from index after type change.

[Junio C Hamano]

When we do not trust executable bit from lstat(2), we copied
existing ce_mode bits without checking if the filesystem object
is a regular file (which is the only thing we apply the "trust
executable bit" business) nor if the blob in the index is a
regular file (otherwise, we should do the same as registering a
new regular file, which is to default non-executable).

Noticed by Johannes Sixt.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 * Adds a test -- how about this as a replacement?

 builtin-apply.c        |    2 +-
 builtin-update-index.c |   13 +++++++------
 cache.h                |   10 ++++++++++
 diff-lib.c             |    4 +---
 read-cache.c           |   13 +++++++------
 t/t3700-add.sh         |   20 ++++++++++++++++++++
 6 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 3fefdac..abe3538 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1988,7 +1988,7 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
 			return error("%s: %s", old_name, strerror(errno));
 
 		if (!cached)
-			st_mode = ntohl(create_ce_mode(st.st_mode));
+			st_mode = ntohl(ce_mode_from_stat(ce, st.st_mode));
 
 		if (patch->is_new < 0)
 			patch->is_new = 0;
diff --git a/builtin-update-index.c b/builtin-update-index.c
index 1ac613a..772aaba 100644
--- a/builtin-update-index.c
+++ b/builtin-update-index.c
@@ -109,16 +109,17 @@ static int add_file_to_cache(const char *path)
 	ce->ce_flags = htons(namelen);
 	fill_stat_cache_info(ce, &st);
 
-	ce->ce_mode = create_ce_mode(st.st_mode);
-	if (!trust_executable_bit) {
+	if (trust_executable_bit)
+		ce->ce_mode = create_ce_mode(st.st_mode);
+	else {
 		/* If there is an existing entry, pick the mode bits
 		 * from it, otherwise assume unexecutable.
 		 */
+		struct cache_entry *ent;
 		int pos = cache_name_pos(path, namelen);
-		if (0 <= pos)
-			ce->ce_mode = active_cache[pos]->ce_mode;
-		else if (S_ISREG(st.st_mode))
-			ce->ce_mode = create_ce_mode(S_IFREG | 0666);
+
+		ent = (0 <= pos) ? active_cache[pos] : NULL;
+		ce->ce_mode = ce_mode_from_stat(ent, st.st_mode);
 	}
 
 	if (index_path(ce->sha1, path, &st, !info_only))
diff --git a/cache.h b/cache.h
index c62b0b0..04f8e63 100644
--- a/cache.h
+++ b/cache.h
@@ -106,6 +106,16 @@ static inline unsigned int create_ce_mode(unsigned int mode)
 		return htonl(S_IFLNK);
 	return htonl(S_IFREG | ce_permissions(mode));
 }
+static inline unsigned int ce_mode_from_stat(struct cache_entry *ce, unsigned int mode)
+{
+	extern int trust_executable_bit;
+	if (!trust_executable_bit && S_ISREG(mode)) {
+		if (ce && S_ISREG(ntohl(ce->ce_mode)))
+			return ce->ce_mode;
+		return create_ce_mode(0666);
+	}
+	return create_ce_mode(mode);
+}
 #define canon_mode(mode) \
 	(S_ISREG(mode) ? (S_IFREG | ce_permissions(mode)) : \
 	S_ISLNK(mode) ? S_IFLNK : S_IFDIR)
diff --git a/diff-lib.c b/diff-lib.c
index 91cd877..556d534 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -170,9 +170,7 @@ static int get_stat_data(struct cache_entry *ce,
 		}
 		changed = ce_match_stat(ce, &st, 0);
 		if (changed) {
-			mode = create_ce_mode(st.st_mode);
-			if (!trust_executable_bit && S_ISREG(st.st_mode))
-				mode = ce->ce_mode;
+			mode = ce_mode_from_stat(ce, st.st_mode);
 			sha1 = no_sha1;
 		}
 	}
diff --git a/read-cache.c b/read-cache.c
index c54a611..605b352 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -344,16 +344,17 @@ int add_file_to_index(const char *path, int verbose)
 	ce->ce_flags = htons(namelen);
 	fill_stat_cache_info(ce, &st);
 
-	ce->ce_mode = create_ce_mode(st.st_mode);
-	if (!trust_executable_bit) {
+	if (trust_executable_bit)
+		ce->ce_mode = create_ce_mode(st.st_mode);
+	else {
 		/* If there is an existing entry, pick the mode bits
 		 * from it, otherwise assume unexecutable.
 		 */
+		struct cache_entry *ent;
 		int pos = cache_name_pos(path, namelen);
-		if (pos >= 0)
-			ce->ce_mode = active_cache[pos]->ce_mode;
-		else if (S_ISREG(st.st_mode))
-			ce->ce_mode = create_ce_mode(S_IFREG | 0666);
+
+		ent = (0 <= pos) ? active_cache[pos] : NULL;
+		ce->ce_mode = ce_mode_from_stat(ent, st.st_mode);
 	}
 
 	if (index_path(ce->sha1, path, &st, 1))
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index caaab26..08e0352 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -30,6 +30,16 @@ test_expect_success \
 	 *) echo fail; git-ls-files --stage xfoo1; (exit 1);;
 	 esac'
 
+test_expect_success 'git-add: filemode=0 should not get confused by symlink' '
+	rm -f xfoo1 &&
+	ln -s foo xfoo1 &&
+	git-add xfoo1 &&
+	case "`git-ls-files --stage xfoo1`" in
+	120000" "*xfoo1) echo ok;;
+	*) echo fail; git-ls-files --stage xfoo1; (exit 1);;
+	esac
+'
+
 test_expect_success \
 	'git-update-index --add: Test that executable bit is not used...' \
 	'git config core.filemode 0 &&
@@ -41,6 +51,16 @@ test_expect_success \
 	 *) echo fail; git-ls-files --stage xfoo2; (exit 1);;
 	 esac'
 
+test_expect_success 'git-add: filemode=0 should not get confused by symlink' '
+	rm -f xfoo2 &&
+	ln -s foo xfoo2 &&
+	git update-index --add xfoo2 &&
+	case "`git-ls-files --stage xfoo2`" in
+	120000" "*xfoo2) echo ok;;
+	*) echo fail; git-ls-files --stage xfoo2; (exit 1);;
+	esac
+'
+
 test_expect_success \
 	'git-update-index --add: Test that executable bit is not used...' \
 	'git config core.filemode 0 &&
-- 
1.5.0.552.ge1b1c

Re: [PATCH] Do not take mode bits from index after type change.

[Johannes Sixt]

Your patch works here, too. Thanks!

I'd appreciate if you could publish it soon since it has a few conflicts with 
my current "don't trust symlinks" work.

-- Hannes

Re: [PATCH] Do not take mode bits from index after type change.

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> Your patch works here, too. Thanks!
>
> I'd appreciate if you could publish it soon since it has a few
> conflicts with my current "don't trust symlinks" work.

I think this bugfix should go to 'maint' and in 'master'.  I
tried to be careful, but extra sets of eyeballs to double-check
that I did not break anything are certainly appreciated.