From 8641fb24ee3ab86bac62f88d31f6e92a9323f699 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Sun, 16 Jul 2006 03:38:40 -0700 Subject: typechange tests for git apply (currently failing) I've found that git apply is incapable of handling patches involving object type changes to the same path. Of course git itself is perfectly capable of making commits that generate these changes, as it only tracks trees states. It's just that the diffs between them are less useful if they can't be applied. Some of these are rare, but I've hit one of them (file becoming a symlink) recently in real-world usage, and was inspired to find more potential breakages :) I'm not sure when I'll have time to fix these myself and I'm not very familiar with the apply code. So if someone could get some or all of these cases working, they would be my hero :) Some of these are what I would refer to as corner-cases from hell. Most (if not all) other systems fail some of these. In fact, they aren't even capable of representing most of these changes in their histories; much less being able to handle patches to that effect. Signed-off-by: Eric Wong Signed-off-by: Junio C Hamano --- t/t4114-apply-typechange.sh | 105 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100755 t/t4114-apply-typechange.sh diff --git a/t/t4114-apply-typechange.sh b/t/t4114-apply-typechange.sh new file mode 100755 index 0000000000..ca81d72157 --- /dev/null +++ b/t/t4114-apply-typechange.sh @@ -0,0 +1,105 @@ +#!/bin/sh +# +# Copyright (c) 2006 Eric Wong +# + +test_description='git-apply should not get confused with type changes. + +' + +. ./test-lib.sh + +test_expect_success 'setup repository and commits' ' + echo "hello world" > foo && + echo "hi planet" > bar && + git update-index --add foo bar && + git commit -m initial && + git branch initial && + rm -f foo && + ln -s bar foo && + git update-index foo && + git commit -m "foo symlinked to bar" && + git branch foo-symlinked-to-bar && + rm -f foo && + echo "how far is the sun?" > foo && + git update-index foo && + git commit -m "foo back to file" && + git branch foo-back-to-file && + rm -f foo && + git update-index --remove foo && + mkdir foo && + echo "if only I knew" > foo/baz && + git update-index --add foo/baz && + git commit -m "foo becomes a directory" && + git branch "foo-becomes-a-directory" && + echo "hello world" > foo/baz && + git update-index foo/baz && + git commit -m "foo/baz is the original foo" && + git branch foo-baz-renamed-from-foo + ' + +test_expect_success 'file renamed from foo to foo/baz' ' + git checkout -f initial && + git diff-tree -M -p HEAD foo-baz-renamed-from-foo > patch && + git apply --index < patch + ' +test_debug 'cat patch' + + +test_expect_success 'file renamed from foo/baz to foo' ' + git checkout -f foo-baz-renamed-from-foo && + git diff-tree -M -p HEAD initial > patch && + git apply --index < patch + ' +test_debug 'cat patch' + + +test_expect_success 'directory becomes file' ' + git checkout -f foo-becomes-a-directory && + git diff-tree -p HEAD initial > patch && + git apply --index < patch + ' +test_debug 'cat patch' + + +test_expect_success 'file becomes directory' ' + git checkout -f initial && + git diff-tree -p HEAD foo-becomes-a-directory > patch && + git apply --index < patch + ' +test_debug 'cat patch' + + +test_expect_success 'file becomes symlink' ' + git checkout -f initial && + git diff-tree -p HEAD foo-symlinked-to-bar > patch && + git apply --index < patch + ' +test_debug 'cat patch' + + +test_expect_success 'symlink becomes file' ' + git checkout -f foo-symlinked-to-bar && + git diff-tree -p HEAD foo-back-to-file > patch && + git apply --index < patch + ' +test_debug 'cat patch' + + +test_expect_success 'symlink becomes directory' ' + git checkout -f foo-symlinked-to-bar && + git diff-tree -p HEAD foo-becomes-a-directory > patch && + git apply --index < patch + ' +test_debug 'cat patch' + + +test_expect_success 'directory becomes symlink' ' + git checkout -f foo-becomes-a-directory && + git diff-tree -p HEAD foo-symlinked-to-bar > patch && + git apply --index < patch + ' +test_debug 'cat patch' + + +test_done -- cgit v1.2.3 From c28c571c143a5145665f4bf334671ac3a7d0980c Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 16 Jul 2006 23:28:23 -0700 Subject: apply: check D/F conflicts more carefully. When creating a new file where a directory used to be (or the user had an empty directory) the code did not check the result from lstat() closely enough, and mistakenly thought the path already existed in the working tree. This does not fix the problem where you have a patch that creates a file at "foo" and removes a file at "foo/bar" (which presumably is the last file in "foo/" directory in the original). For that, we would need to restructure write_out_results() loop. Signed-off-by: Junio C Hamano --- builtin-apply.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index c903146bb6..97274425d5 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -1732,9 +1732,14 @@ static int check_patch(struct patch *patch) if (check_index && cache_name_pos(new_name, strlen(new_name)) >= 0) return error("%s: already exists in index", new_name); if (!cached) { - if (!lstat(new_name, &st)) - return error("%s: already exists in working directory", new_name); - if (errno != ENOENT) + struct stat nst; + if (!lstat(new_name, &nst)) { + if (S_ISDIR(nst.st_mode)) + ; /* ok */ + else + return error("%s: already exists in working directory", new_name); + } + else if ((errno != ENOENT) && (errno != ENOTDIR)) return error("%s: %s", new_name, strerror(errno)); } if (!patch->new_mode) { @@ -2010,6 +2015,16 @@ static void create_one_file(char *path, unsigned mode, const char *buf, unsigned return; } + if (errno == EEXIST) { + /* We may be trying to create a file where a directory + * used to be. + */ + struct stat st; + errno = 0; + if (!lstat(path, &st) && S_ISDIR(st.st_mode) && !rmdir(path)) + errno = EEXIST; + } + if (errno == EEXIST) { unsigned int nr = getpid(); -- cgit v1.2.3 From eed46644ca48ad08e1fd71e14afbe801399e8a67 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 16 Jul 2006 23:52:09 -0700 Subject: apply: split out removal and creation into different phases. This reworks write_out_result() loop so we first remove the paths that are to go away and then create them after finishing all the removal. This is necessary when a patch creates a file "foo" and removes a file "foo/bar". Signed-off-by: Junio C Hamano --- builtin-apply.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index 97274425d5..37404e2e6a 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -2059,32 +2059,42 @@ static void create_file(struct patch *patch) cache_tree_invalidate_path(active_cache_tree, path); } -static void write_out_one_result(struct patch *patch) +/* phase zero is to remove, phase one is to create */ +static void write_out_one_result(struct patch *patch, int phase) { if (patch->is_delete > 0) { - remove_file(patch); + if (phase == 0) + remove_file(patch); return; } if (patch->is_new > 0 || patch->is_copy) { - create_file(patch); + if (phase == 1) + create_file(patch); return; } /* * Rename or modification boils down to the same * thing: remove the old, write the new */ - remove_file(patch); + if (phase == 0) + remove_file(patch); + if (phase == 1) create_file(patch); } static void write_out_results(struct patch *list, int skipped_patch) { + int phase; + if (!list && !skipped_patch) die("No changes"); - while (list) { - write_out_one_result(list); - list = list->next; + for (phase = 0; phase < 2; phase++) { + struct patch *l = list; + while (l) { + write_out_one_result(l, phase); + l = l->next; + } } } -- cgit v1.2.3 From 7f95aef28fa1e2662aebb4556c71ad6912d395e5 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 17 Jul 2006 00:10:47 -0700 Subject: apply: handle type-changing patch correctly. A type-change diff is always split into a patch to delete old, immediately followed by a patch to create new. check_patch() routine noticed that the path to be created already exists in the working tree and/or in the index when looking at the creation patch and mistakenly thought it to be an error. Signed-off-by: Junio C Hamano --- builtin-apply.c | 29 ++++++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index 37404e2e6a..8f7cf44c69 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -1664,13 +1664,14 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry * return 0; } -static int check_patch(struct patch *patch) +static int check_patch(struct patch *patch, struct patch *prev_patch) { struct stat st; const char *old_name = patch->old_name; const char *new_name = patch->new_name; const char *name = old_name ? old_name : new_name; struct cache_entry *ce = NULL; + int ok_if_exists; if (old_name) { int changed = 0; @@ -1728,13 +1729,28 @@ static int check_patch(struct patch *patch) old_name, st_mode, patch->old_mode); } + if (new_name && prev_patch && prev_patch->is_delete && + !strcmp(prev_patch->old_name, new_name)) + /* A type-change diff is always split into a patch to + * delete old, immediately followed by a patch to + * create new (see diff.c::run_diff()); in such a case + * it is Ok that the entry to be deleted by the + * previous patch is still in the working tree and in + * the index. + */ + ok_if_exists = 1; + else + ok_if_exists = 0; + if (new_name && (patch->is_new | patch->is_rename | patch->is_copy)) { - if (check_index && cache_name_pos(new_name, strlen(new_name)) >= 0) + if (check_index && + cache_name_pos(new_name, strlen(new_name)) >= 0 && + !ok_if_exists) return error("%s: already exists in index", new_name); if (!cached) { struct stat nst; if (!lstat(new_name, &nst)) { - if (S_ISDIR(nst.st_mode)) + if (S_ISDIR(nst.st_mode) || ok_if_exists) ; /* ok */ else return error("%s: already exists in working directory", new_name); @@ -1767,10 +1783,13 @@ static int check_patch(struct patch *patch) static int check_patch_list(struct patch *patch) { + struct patch *prev_patch = NULL; int error = 0; - for (;patch ; patch = patch->next) - error |= check_patch(patch); + for (prev_patch = NULL; patch ; patch = patch->next) { + error |= check_patch(patch, prev_patch); + prev_patch = patch; + } return error; } -- cgit v1.2.3 From 56ac168f6f89bebf2846c4bafed01318fe3f25cd Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 18 Jul 2006 19:46:34 +0200 Subject: Fix t4114 on cygwin On cygwin, when you try to create a symlink over a directory, you do not get EEXIST, but EACCES. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin-apply.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin-apply.c b/builtin-apply.c index 8f7cf44c69..d924ac3d0a 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -2034,7 +2034,7 @@ static void create_one_file(char *path, unsigned mode, const char *buf, unsigned return; } - if (errno == EEXIST) { + if (errno == EEXIST || errno == EACCES) { /* We may be trying to create a file where a directory * used to be. */ -- cgit v1.2.3