{"thread":{"id":"14188","subject":"[PATCH] git-apply doesn't handle same name patches well [V4]","startedAt":"2008-06-27T18:39:12Z","lastAt":"2008-06-28T00:06:03Z","messageCount":2,"participants":["Don Zickus","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"81459","messageId":"1214591952-3763-1-git-send-email-dzickus@redhat.com","threadId":"14188","inReplyTo":null,"subject":"[PATCH] git-apply doesn't handle same name patches well [V4]","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2008-06-27T18:39:12Z","receivedAt":"2008-06-27T18:39:12Z","isPatch":true,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"When working with a lot of people who backport patches all day long, every\nonce in a while I get a patch that modifies the same file more than once\ninside the same patch.  git-apply either fails if the second change relies\non the first change or silently drops the first change if the second change\nis independent.\n\nThe silent part is the scary scenario for us.  Also this behaviour is\ndifferent from the patch-utils.\n\nI have modified git-apply to create a table of the filenames of files it\nmodifies such that if a later patch chunk modifies a file in the table it\nwill buffer the previously changed file instead of reading the original file\nfrom disk.\n\nLogic has been put in to handle creations/deletions/renames/copies.  All the\nrelevant tests of git-apply succeed.\n\nA new test has been added to cover the cases I addressed.\n\nThe fix is relatively straight-forward.\n\nSigned-off-by: Don Zickus <dzickus@redhat.com>\n\n---\n\nChanges since v3\n================\nvarious improvements based on suggestions from Junio\n*NOTE* I'm not entirely sure I got the mode bits right and the cache/index\nstuff correct.  It seems to work correctly but I didn't test every\npermutation.\n- simplified check_patch() using new mechanisms\n- re-used mode bits if lookup succeeds in check_preimage\n- add test to verify A->B, C->A, A->A works\n- cheap memset hack to deal with pointers to free'd memory\n- rename fn_cache to fn_table\n- hours lost chasing down weird behaviour from test results :)\n\nChanges since v2\n================\n- the updated patch not a v1 copy (doh!)\n\nChanges since v1\n================\n- converted to path-list structs\n- added testcases for renaming a patch and apply a new patch on top inside\nthe same patch file\n---\n builtin-apply.c          |   82 +++++++++++++++++++++++++++++++++++++++-----\n t/t4127-apply-same-fn.sh |   85 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 157 insertions(+), 10 deletions(-)\n create mode 100755 t/t4127-apply-same-fn.sh\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex c497889..34ab637 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -12,6 +12,7 @@\n #include \"blob.h\"\n #include \"delta.h\"\n #include \"builtin.h\"\n+#include \"path-list.h\"\n \n /*\n  *  --check turns on checking that the working tree matches the\n@@ -185,6 +186,13 @@ struct image {\n \tstruct line *line;\n };\n \n+/*\n+ * Caches patch filenames to handle the case where a\n+ * patch chunk reuses a filename\n+ */\n+\n+static struct path_list fn_table;\n+\n static uint32_t hash_line(const char *cp, size_t len)\n {\n \tsize_t i;\n@@ -2176,15 +2184,62 @@ static int read_file_or_gitlink(struct cache_entry *ce, struct strbuf *buf)\n \treturn 0;\n }\n \n+static struct patch *in_fn_table(const char *name)\n+{\n+\tstruct path_list_item *item;\n+\n+\tif (name == NULL)\n+\t\treturn NULL;\n+\n+\titem = path_list_lookup(name, &fn_table);\n+\tif (item != NULL)\n+\t\treturn (struct patch *)item->util;\n+\n+\treturn NULL;\n+}\n+\n+static void add_to_fn_table(struct patch *patch)\n+{\n+\tstruct path_list_item *item;\n+\n+\t/*\n+\t * Always add new_name unless patch is a deletion\n+\t * This should cover the cases for normal diffs,\n+\t * file creations and copies\n+\t */\n+\tif (patch->new_name != NULL) {\n+\t\titem = path_list_insert(patch->new_name, &fn_table);\n+\t\titem->util = patch;\n+\t}\n+\n+\t/*\n+\t * store a failure on rename/deletion cases because\n+\t * later chunks shouldn't patch old names\n+\t */\n+\tif ((patch->new_name == NULL) || (patch->is_rename)) {\n+\t\titem = path_list_insert(patch->old_name, &fn_table);\n+\t\titem->util = (struct patch *) -1;\n+\t}\n+}\n+\n static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)\n {\n \tstruct strbuf buf;\n \tstruct image image;\n \tsize_t len;\n \tchar *img;\n+\tstruct patch *tpatch;\n \n \tstrbuf_init(&buf, 0);\n-\tif (cached) {\n+\n+\tif ((tpatch = in_fn_table(patch->old_name)) != NULL) {\n+\t\tif (tpatch == (struct patch *) -1) {\n+\t\t\treturn error(\"patch %s has been renamed/deleted\",\n+\t\t\t\tpatch->old_name);\n+\t\t}\n+\t\t/* We have a patched copy in memory use that */\n+\t\tstrbuf_add(&buf, tpatch->result, tpatch->resultsize);\n+\t} else if (cached) {\n \t\tif (read_file_or_gitlink(ce, &buf))\n \t\t\treturn error(\"read of %s failed\", patch->old_name);\n \t} else if (patch->old_name) {\n@@ -2211,6 +2266,7 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *\n \t\treturn -1; /* note with --reject this succeeds. */\n \tpatch->result = image.buf;\n \tpatch->resultsize = image.len;\n+\tadd_to_fn_table(patch);\n \tfree(image.line_allocated);\n \n \tif (0 < patch->is_delete && patch->resultsize)\n@@ -2255,6 +2311,7 @@ static int verify_index_match(struct cache_entry *ce, struct stat *st)\n static int check_preimage(struct patch *patch, struct cache_entry **ce, struct stat *st)\n {\n \tconst char *old_name = patch->old_name;\n+\tstruct patch *tpatch;\n \tint stat_ret = 0;\n \tunsigned st_mode = 0;\n \n@@ -2268,12 +2325,17 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \t\treturn 0;\n \n \tassert(patch->is_new <= 0);\n-\tif (!cached) {\n+\tif ((tpatch = in_fn_table(old_name)) != NULL) {\n+\t\tif (tpatch == (struct patch *) -1) {\n+\t\t\treturn error(\"%s: has been deleted/renamed\", old_name);\n+\t\t}\n+\t\tst_mode = tpatch->new_mode;\n+\t} else if (!cached) {\n \t\tstat_ret = lstat(old_name, st);\n \t\tif (stat_ret && errno != ENOENT)\n \t\t\treturn error(\"%s: %s\", old_name, strerror(errno));\n \t}\n-\tif (check_index) {\n+\tif (check_index && !tpatch) {\n \t\tint pos = cache_name_pos(old_name, strlen(old_name));\n \t\tif (pos < 0) {\n \t\t\tif (patch->is_new < 0)\n@@ -2325,7 +2387,7 @@ static int check_preimage(struct patch *patch, struct cache_entry **ce, struct s\n \treturn 0;\n }\n \n-static int check_patch(struct patch *patch, struct patch *prev_patch)\n+static int check_patch(struct patch *patch)\n {\n \tstruct stat st;\n \tconst char *old_name = patch->old_name;\n@@ -2342,8 +2404,7 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)\n \t\treturn status;\n \told_name = patch->old_name;\n \n-\tif (new_name && prev_patch && 0 < prev_patch->is_delete &&\n-\t    !strcmp(prev_patch->old_name, new_name))\n+\tif (in_fn_table(new_name) == (struct patch *) -1)\n \t\t/*\n \t\t * A type-change diff is always split into a patch to\n \t\t * delete old, immediately followed by a patch to\n@@ -2393,15 +2454,14 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)\n \n static int check_patch_list(struct patch *patch)\n {\n-\tstruct patch *prev_patch = NULL;\n \tint err = 0;\n \n-\tfor (prev_patch = NULL; patch ; patch = patch->next) {\n+\twhile (patch) {\n \t\tif (apply_verbosely)\n \t\t\tsay_patch_name(stderr,\n \t\t\t\t       \"Checking patch \", patch, \"...\\n\");\n-\t\terr |= check_patch(patch, prev_patch);\n-\t\tprev_patch = patch;\n+\t\terr |= check_patch(patch);\n+\t\tpatch = patch->next;\n \t}\n \treturn err;\n }\n@@ -2919,6 +2979,8 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)\n \tstruct patch *list = NULL, **listp = &list;\n \tint skipped_patch = 0;\n \n+\t/* FIXME - memory leak when using multiple patch files as inputs */\n+\tmemset(&fn_table, 0, sizeof(struct path_list));\n \tstrbuf_init(&buf, 0);\n \tpatch_input_file = filename;\n \tread_patch_file(&buf, fd);\ndiff --git a/t/t4127-apply-same-fn.sh b/t/t4127-apply-same-fn.sh\nnew file mode 100755\nindex 0000000..2a6ed77\n--- /dev/null\n+++ b/t/t4127-apply-same-fn.sh\n@@ -0,0 +1,85 @@\n+#!/bin/sh\n+\n+test_description='apply same filename'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tfor i in a b c d e f g h i j k l m\n+\tdo\n+\t\techo $i\n+\tdone >same_fn &&\n+\tcp same_fn other_fn &&\n+\tgit add same_fn other_fn &&\n+\tgit commit -m initial\n+'\n+test_expect_success 'apply same filename with independent changes' '\n+\tsed -i -e \"s/^d/z/\" same_fn &&\n+\tgit diff > patch0 &&\n+\tgit add same_fn &&\n+\tsed -i -e \"s/^i/y/\" same_fn &&\n+\tgit diff >> patch0 &&\n+\tcp same_fn same_fn2 &&\n+\tgit reset --hard &&\n+\tgit-apply patch0 &&\n+\tdiff same_fn same_fn2\n+'\n+\n+test_expect_success 'apply same filename with overlapping changes' '\n+\tgit reset --hard\n+\tsed -i -e \"s/^d/z/\" same_fn &&\n+\tgit diff > patch0 &&\n+\tgit add same_fn &&\n+\tsed -i -e \"s/^e/y/\" same_fn &&\n+\tgit diff >> patch0 &&\n+\tcp same_fn same_fn2 &&\n+\tgit reset --hard &&\n+\tgit-apply patch0 &&\n+\tdiff same_fn same_fn2\n+'\n+\n+test_expect_success 'apply same new filename after rename' '\n+\tgit reset --hard\n+\tgit mv same_fn new_fn\n+\tsed -i -e \"s/^d/z/\" new_fn &&\n+\tgit add new_fn &&\n+\tgit diff -M --cached > patch1 &&\n+\tsed -i -e \"s/^e/y/\" new_fn &&\n+\tgit diff >> patch1 &&\n+\tcp new_fn new_fn2 &&\n+\tgit reset --hard &&\n+\tgit apply --index patch1 &&\n+\tdiff new_fn new_fn2\n+'\n+\n+test_expect_success 'apply same old filename after rename -- should fail.' '\n+\tgit reset --hard\n+\tgit mv same_fn new_fn\n+\tsed -i -e \"s/^d/z/\" new_fn &&\n+\tgit add new_fn &&\n+\tgit diff -M --cached > patch1 &&\n+\tgit mv new_fn same_fn\n+\tsed -i -e \"s/^e/y/\" same_fn &&\n+\tgit diff >> patch1 &&\n+\tgit reset --hard &&\n+\ttest_must_fail git apply patch1\n+'\n+\n+test_expect_success 'apply A->B (rename), C->A (rename), A->A -- should pass.' '\n+\tgit reset --hard\n+\tgit mv same_fn new_fn\n+\tsed -i -e \"s/^d/z/\" new_fn &&\n+\tgit add new_fn &&\n+\tgit diff -M --cached > patch1 &&\n+\tgit commit -m \"a rename\" &&\n+\tgit mv other_fn same_fn\n+\tsed -i -e \"s/^e/y/\" same_fn &&\n+\tgit add same_fn &&\n+\tgit diff -M --cached >> patch1 &&\n+\tsed -i -e \"s/^g/x/\" same_fn &&\n+\tgit diff >> patch1 &&\n+\tgit reset --hard HEAD^ &&\n+\tgit apply patch1\n+'\n+\n+test_done\n-- \n1.5.6.rc2.48.g13da\n"},{"id":"81521","messageId":"7v63rumnis.fsf@gitster.siamese.dyndns.org","threadId":"14188","inReplyTo":"1214591952-3763-1-git-send-email-dzickus@redhat.com","subject":"Re: [PATCH] git-apply doesn't handle same name patches well [V4]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-28T00:06:03Z","receivedAt":"2008-06-28T00:06:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.  Looks much better.  Will queue.\n"}]}