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

From: Junio C Hamano <junkio@cox.net>
Date: Feb 17, 2007, 00:15 UTC
Message-ID: <7vodntzljb.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <200702170030.04014.johannes.sixt@telecom.at>

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);
                
        }