On Thu, Jan 29, 2015 at 12:45:22PM -0800, Junio C Hamano wrote: > +static int path_is_beyond_symlink(const char *name_) > +{ > + struct strbuf name = STRBUF_INIT; > + > + strbuf_addstr(&name, name_); > + do { > + struct patch *previous; > + > + while (--name.len && name.buf[name.len] != '/') > + ; /* scan backwards */ > + if (!name.len) > + break;
I imagine it is impossible here for "name_" to be initially empty, but it would make the backwards-scan loop go quite badly. Worth a comment or an assert()? > + name.buf[name.len] = '\0'; > + previous = in_fn_table(name.buf); > + if (previous) { > + if (!was_deleted(previous) && > + !to_be_deleted(previous) && > + previous->new_mode && > + S_ISLNK(previous->new_mode)) > + goto symlink_found; > + } else if (check_index) { > + int pos = cache_name_pos(name.buf, name.len); > + if (0 <= pos && > + S_ISLNK(active_cache[pos]->ce_mode)) > + goto symlink_found; > + } else { > + struct stat st; > + if (!lstat(name.buf, &st) && S_ISLNK(st.st_mode)) > + goto symlink_found; > + } > + } while (1); > + > + strbuf_release(&name); > + return 0; > +symlink_found: > + strbuf_release(&name); > + return 1; Style nit, but might this be easier to follow the logic without the gotos, by putting the setup and cleanup in a wrapper function and returning directly from the main logic? static int path_is_beyond_symlink(const char *name) { struct strbuf buf = STRBUF_INIT; int ret; strbuf_addstr(&buf, name); ret = path_is_beyond_symlink_1(name); strbuf_release(&buf); return ret; } I can live with it either way, though. > + if (!patch->is_delete && path_is_beyond_symlink(patch->new_name)) > + return error(_("affected file '%s' is beyond a symbolic link"), > + patch->new_name); Why does this not kick in when deleting a file? If it is not OK to add across a symlink, why is it OK to delete? IOW, why should this test fail: diff --git a/t/t4122-apply-symlink-inside.sh b/t/t4122-apply-symlink-inside.sh index 0a8de4a..f03b604 100755 --- a/t/t4122-apply-symlink-inside.sh +++ b/t/t4122-apply-symlink-inside.sh @@ -64,6 +64,7 @@ test_expect_success SYMLINKS 'do not follow symbolic link (setup)' ' >arch/x86_64/dir/file && git add arch/x86_64/dir/file && git diff HEAD >add_file.patch && + git diff -R HEAD >del_file.patch && git reset --hard && rm -fr arch/x86_64/dir && @@ -111,7 +112,11 @@ test_expect_success SYMLINKS 'do not follow symbolic link (existing)' ' test_must_fail git apply --cached add_file.patch 2>error-ct-file && test_i18ngrep "beyond a symbolic link" error-ct-file && - test_must_fail git ls-files --error-unmatch arch/i386/dir + test_must_fail git ls-files --error-unmatch arch/i386/dir && + + >arch/i386/dir/file && + test_must_fail git apply del_file.patch && + test_path_is_file arch/i386/dir/file ' test_done > + test ! -e arch/x86_64/dir && > + test ! -e arch/i386/dir/file && Minor nit: use test_path_is_missing here (and elsewhere in the added tests). -Peff -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/