* [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false. @ 2007-02-16 23:09 Johannes Sixt 2007-02-16 23:13 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2007-02-16 23:09 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false. 2007-02-16 23:09 [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false Johannes Sixt @ 2007-02-16 23:13 ` Junio C Hamano 2007-02-16 23:30 ` Johannes Sixt 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2007-02-16 23:13 UTC (permalink / raw) To: Johannes Sixt; +Cc: git 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false. 2007-02-16 23:13 ` Junio C Hamano @ 2007-02-16 23:30 ` Johannes Sixt 2007-02-17 0:15 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2007-02-16 23:30 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false. 2007-02-16 23:30 ` Johannes Sixt @ 2007-02-17 0:15 ` Junio C Hamano 2007-02-17 6:56 ` [PATCH] Do not take mode bits from index after type change Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2007-02-17 0:15 UTC (permalink / raw) To: Johannes Sixt; +Cc: git 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); } ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] Do not take mode bits from index after type change. 2007-02-17 0:15 ` Junio C Hamano @ 2007-02-17 6:56 ` Junio C Hamano 2007-02-17 10:31 ` Johannes Sixt 0 siblings, 1 reply; 7+ messages in thread From: Junio C Hamano @ 2007-02-17 6:56 UTC (permalink / raw) To: Johannes Sixt; +Cc: git 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] Do not take mode bits from index after type change. 2007-02-17 6:56 ` [PATCH] Do not take mode bits from index after type change Junio C Hamano @ 2007-02-17 10:31 ` Johannes Sixt 2007-02-17 17:49 ` Junio C Hamano 0 siblings, 1 reply; 7+ messages in thread From: Johannes Sixt @ 2007-02-17 10:31 UTC (permalink / raw) To: git; +Cc: Junio C Hamano 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Do not take mode bits from index after type change. 2007-02-17 10:31 ` Johannes Sixt @ 2007-02-17 17:49 ` Junio C Hamano 0 siblings, 0 replies; 7+ messages in thread From: Junio C Hamano @ 2007-02-17 17:49 UTC (permalink / raw) To: Johannes Sixt; +Cc: git 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2007-02-17 17:49 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-02-16 23:09 [PATCH] Fix incorrect diff of a link -> file change if core.filemode = false Johannes Sixt 2007-02-16 23:13 ` Junio C Hamano 2007-02-16 23:30 ` Johannes Sixt 2007-02-17 0:15 ` Junio C Hamano 2007-02-17 6:56 ` [PATCH] Do not take mode bits from index after type change Junio C Hamano 2007-02-17 10:31 ` Johannes Sixt 2007-02-17 17:49 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.