{"thread":{"id":"13980","subject":"[PATCH] git-apply doesn't handle same name patches well [V2]","startedAt":"2008-06-16T16:24:01Z","lastAt":"2008-06-17T14:14:52Z","messageCount":3,"participants":["Don Zickus","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"80045","messageId":"1213633441-29185-1-git-send-email-dzickus@redhat.com","threadId":"13980","inReplyTo":null,"subject":"[PATCH] git-apply doesn't handle same name patches well [V2]","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2008-06-16T16:24:01Z","receivedAt":"2008-06-16T16:24:01Z","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 cache the filenames of files it modifies such\nthat if a later patch chunk modifies a file in the cache it will buffer the\npreviously changed file instead of reading the original file from 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.  However,\ncurrently adding changes to renamed file inside the same patch doesn't work\ncorrectly (it fails to find new file).  I didn't know how to fix this\ncorrectly, so I have the test fail expectedly.\n\nThe fix is relatively straight-forward.  But I'm not sure if this new\nbehaviour is something the git community wants.\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\nSigned-off-by: Don Zickus <dzickus@redhat.com>\n---\n builtin-apply.c          |   69 +++++++++++++++++++++++++++++++++++++++++++++-\n t/t4127-apply-same-fn.sh |   40 ++++++++++++++++++++++++++\n 2 files changed, 108 insertions(+), 1 deletions(-)\n create mode 100755 t/t4127-apply-same-fn.sh\n\ndiff --git a/builtin-apply.c b/builtin-apply.c\nindex c497889..8330517 100644\n--- a/builtin-apply.c\n+++ b/builtin-apply.c\n@@ -185,6 +185,18 @@ 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+struct fn_cache {\n+\tchar *name;\n+\tstruct patch *patch;\n+\tstruct fn_cache *next;\n+};\n+\n+struct fn_cache *fn_cache_top = NULL;\n+\n static uint32_t hash_line(const char *cp, size_t len)\n {\n \tsize_t i;\n@@ -2176,6 +2188,51 @@ static int read_file_or_gitlink(struct cache_entry *ce, struct strbuf *buf)\n \treturn 0;\n }\n \n+struct patch *in_fn_cache(char *name)\n+{\n+\tstruct fn_cache *p;\n+\n+\tfor (p=fn_cache_top; p; p=p->next) {\n+\t\tif (!strcmp(name, p->name))\n+\t\t\treturn p->patch;\n+\t}\n+\n+\treturn NULL;\n+}\n+\n+void add_to_fn_cache(char *name, struct patch *patch)\n+{\n+\tstruct fn_cache *fn_cache;\n+\n+\t/* Always add new_name unless patch is a deletion */\n+\tif (name != NULL) {\n+\t\tfn_cache = xmalloc(sizeof(*fn_cache));\n+\n+\t\t/* assuming the pointer to filename won't disappear */\n+\t\tfn_cache->name = name;\n+\t\tfn_cache->patch = patch;\n+\t\tfn_cache->next = fn_cache_top;\n+\n+\t\tfn_cache_top = fn_cache;\n+\t}\n+\n+\t/* skip normal diffs, creations and copies */\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 ((name == NULL) || (patch->is_rename)) {\n+\t\tfn_cache = xmalloc(sizeof(*fn_cache));\n+\n+\t\t/* assuming the pointer to filename won't disappear */\n+\t\tfn_cache->name = patch->old_name;\n+\t\tfn_cache->patch = (struct patch *) -1;\n+\t\tfn_cache->next = fn_cache_top;\n+\n+\t\tfn_cache_top = fn_cache;\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@@ -2188,7 +2245,16 @@ static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *\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-\t\tif (S_ISGITLINK(patch->old_mode)) {\n+\t\tstruct patch *tpatch = in_fn_cache(patch->old_name);\n+\n+\t\tif (tpatch != NULL) {\n+\t\t\tif (tpatch == (struct patch *) -1) {\n+\t\t\t\treturn error(\"patch %s has been renamed/deleted\",\n+\t\t\t\t\tpatch->old_name);\n+\t\t\t}\n+\t\t\t/* We have a patched copy in memory use that */\n+\t\t\tstrbuf_add(&buf, tpatch->result, tpatch->resultsize);\n+\t\t} else if (S_ISGITLINK(patch->old_mode)) {\n \t\t\tif (ce) {\n \t\t\t\tread_file_or_gitlink(ce, &buf);\n \t\t\t} else {\n@@ -2211,6 +2277,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_cache(patch->new_name, patch);\n \tfree(image.line_allocated);\n \n \tif (0 < patch->is_delete && patch->resultsize)\ndiff --git a/t/t4127-apply-same-fn.sh b/t/t4127-apply-same-fn.sh\nnew file mode 100755\nindex 0000000..a20bd8e\n--- /dev/null\n+++ b/t/t4127-apply-same-fn.sh\n@@ -0,0 +1,40 @@\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+\tgit add same_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_done\n-- \n1.5.6.rc2.48.g13da\n"},{"id":"80119","messageId":"alpine.DEB.1.00.0806171027200.6439@racer","threadId":"13980","inReplyTo":"1213633441-29185-1-git-send-email-dzickus@redhat.com","subject":"Re: [PATCH] git-apply doesn't handle same name patches well [V2]","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-17T09:30:35Z","receivedAt":"2008-06-17T09:30:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 16 Jun 2008, Don Zickus wrote:\n\n> Changes since v1\n> ================\n> - converted to path-list structs\n> - added testcases for renaming a patch and apply a new patch on top inside\n> the same patch file\n\nJust for future reference: commonly, this is not put into the commit \nmessage, but after the \"---\" separator into the mail comments.\n\n> +/*\n> + * Caches patch filenames to handle the case where a\n> + * patch chunk reuses a filename\n> + */\n> +struct fn_cache {\n> +\tchar *name;\n> +\tstruct patch *patch;\n> +\tstruct fn_cache *next;\n> +};\n\nIt is still not a path_list.  Even if you said so in the \"Changes since \nV1\".\n\nSee \nhttp://repo.or.cz/w/git/vmiklos.git?a=blob;f=Documentation/technical/api-path-list.txt;h=9dbedd0a67ce6c1cecd157b0d89f9b1333e180e3;hb=340d6344cfe13bb93740f40d3268ca39b8c7c15d#l36 \nfor a nice example how to use path_lists.\n\nYou want to use path_list_has_path().\n\nHth,\nDscho\n"},{"id":"80140","messageId":"20080617141452.GJ16941@redhat.com","threadId":"13980","inReplyTo":"alpine.DEB.1.00.0806171027200.6439@racer","subject":"Re: [PATCH] git-apply doesn't handle same name patches well [V2]","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2008-06-17T14:14:52Z","receivedAt":"2008-06-17T14:14:52Z","isPatch":true,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"On Tue, Jun 17, 2008 at 10:30:35AM +0100, Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 16 Jun 2008, Don Zickus wrote:\n> \n> > Changes since v1\n> > ================\n> > - converted to path-list structs\n> > - added testcases for renaming a patch and apply a new patch on top inside\n> > the same patch file\n> \n> Just for future reference: commonly, this is not put into the commit \n> message, but after the \"---\" separator into the mail comments.\n\nOk. Thanks.\n\n> \n> > +/*\n> > + * Caches patch filenames to handle the case where a\n> > + * patch chunk reuses a filename\n> > + */\n> > +struct fn_cache {\n> > +\tchar *name;\n> > +\tstruct patch *patch;\n> > +\tstruct fn_cache *next;\n> > +};\n> \n> It is still not a path_list.  Even if you said so in the \"Changes since \n> V1\".\n\nYeah, I attached the wrong patch, see V3. :-)\n\nCheers,\nDon\n"}]}