{"thread":{"id":"13985","subject":"[PATCH] git-apply doesn't handle same name patches well [V3]","startedAt":"2008-06-16T20:04:46Z","lastAt":"2008-06-19T22:15:16Z","messageCount":6,"participants":["Don Zickus","Jakub Narebski","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"80063","messageId":"1213646686-31964-1-git-send-email-dzickus@redhat.com","threadId":"13985","inReplyTo":null,"subject":"[PATCH] git-apply doesn't handle same name patches well [V3]","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2008-06-16T20:04:46Z","receivedAt":"2008-06-16T20:04:46Z","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 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\nSigned-off-by: Don Zickus <dzickus@redhat.com>\n---\n builtin-apply.c          |   52 +++++++++++++++++++++++++++++++++++-\n t/t4127-apply-same-fn.sh |   67 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 118 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..9f76ce4 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+struct path_list fn_cache = {NULL, 0, 0, 0};\n+\n static uint32_t hash_line(const char *cp, size_t len)\n {\n \tsize_t i;\n@@ -2176,6 +2184,38 @@ 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 path_list_item *item;\n+\n+\titem = path_list_lookup(name, &fn_cache);\n+\tif (item != NULL)\n+\t\treturn (struct patch *)item->util;\n+\n+\treturn NULL;\n+}\n+\n+void add_to_fn_cache(char *name, struct patch *patch)\n+{\n+\tstruct path_list_item *item;\n+\n+\t/* Always add new_name unless patch is a deletion */\n+\tif (name != NULL) {\n+\t\titem = path_list_insert(name, &fn_cache);\n+\t\titem->util = patch;\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\titem = path_list_insert(patch->old_name, &fn_cache);\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@@ -2188,7 +2228,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 +2260,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..47b59d5\n--- /dev/null\n+++ b/t/t4127-apply-same-fn.sh\n@@ -0,0 +1,67 @@\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_expect_failure '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 patch1 &&\n+\tdiff new_fn new_fn2\n+'\n+\n+test_expect_success 'apply same old 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+\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_done\n-- \n1.5.6.rc2.48.g13da\n"},{"id":"80068","messageId":"m3prqhnn0b.fsf@localhost.localdomain","threadId":"13985","inReplyTo":"1213646686-31964-1-git-send-email-dzickus@redhat.com","subject":"Re: [PATCH] git-apply doesn't handle same name patches well [V3]","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2008-06-16T20:27:52Z","receivedAt":"2008-06-16T20:27:52Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Don Zickus <dzickus@redhat.com> writes:\n\n> When working with a lot of people who backport patches all day long, every\n> once in a while I get a patch that modifies the same file more than once\n> inside the same patch.  git-apply either fails if the second change relies\n> on the first change or silently drops the first change if the second change\n> is independent.\n> \n> The silent part is the scary scenario for us.  Also this behaviour is\n> different from the patch-utils.\n> \n> I have modified git-apply to cache the filenames of files it modifies such\n> that if a later patch chunk modifies a file in the cache it will buffer the\n> previously changed file instead of reading the original file from disk.\n> \n> Logic has been put in to handle creations/deletions/renames/copies.  All the\n> relevant tests of git-apply succeed.\n\nVery nice, although most probably I would never use this.\n\n+1\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"80120","messageId":"alpine.DEB.1.00.0806171039250.6439@racer","threadId":"13985","inReplyTo":"1213646686-31964-1-git-send-email-dzickus@redhat.com","subject":"Re: [PATCH] git-apply doesn't handle same name patches well [V3]","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-17T09:40:02Z","receivedAt":"2008-06-17T09:40:02Z","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 v2\n> ================\n> - the updated patch not a v1 copy (doh!)\n\nAh.  If you would have reused the same mail thread, I would not have \nmissed it when I responded to V2.\n\nCiao,\nDscho\n"},{"id":"80189","messageId":"7vbq1z375d.fsf@gitster.siamese.dyndns.org","threadId":"13985","inReplyTo":"1213646686-31964-1-git-send-email-dzickus@redhat.com","subject":"Re: [PATCH] git-apply doesn't handle same name patches well [V3]","fromName":"Junio C Hamano","fromEmail":"junio@pobox.com","sentAt":"2008-06-18T00:42:54Z","receivedAt":"2008-06-18T00:42:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Don Zickus <dzickus@redhat.com> writes:\n\n> When working with a lot of people who backport patches all day long, every\n> once in a while I get a patch that modifies the same file more than once\n> inside the same patch.  git-apply either fails if the second change relies\n> on the first change or silently drops the first change if the second change\n> is independent.\n\nGood issue to tackle.\n\n> A new test has been added to cover the cases I addressed.  However,\n> currently adding changes to renamed file inside the same patch doesn't work\n> correctly (it fails to find new file).  I didn't know how to fix this\n> correctly, so I have the test fail expectedly.\n\nDo you mean that the first patch rename-edits A to B, and then the second\npatch edits B in place?  Because your fn_cache is keyed by postimage\nfilename (in this case B), I would imagine that the later lookup of B\nshould successfully find the patch result from the previous one.  Unless\nthe in_fn_cache() is somehow wrong...\n\nor do you mean that the first patch rename-edits A to B, but the second\none still wants to edit A in place and you would want to pretend as if the\nlater one is for a patch to B?  I would think that is doable but asking\nfor too much magic, and a tool with too much magic is scary.\n\n> The fix is relatively straight-forward.  But I'm not sure if this new\n> behaviour is something the git community wants.\n\n> Changes since v2\n> ================\n> - the updated patch not a v1 copy (doh!)\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> Signed-off-by: Don Zickus <dzickus@redhat.com>\n> ---\n\nFirst, a minor style issue.  Because the final round after polishing on\nthe list is etched into the history without any of the earlier doh!\nrounds, we do not want to have the above \"Changes since...\" in the commit\nlog message.\n\n  Cf. http://article.gmane.org/gmane.comp.version-control.git/84308\n\n>  builtin-apply.c          |   52 +++++++++++++++++++++++++++++++++++-\n>  t/t4127-apply-same-fn.sh |   67 ++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 118 insertions(+), 1 deletions(-)\n>  create mode 100755 t/t4127-apply-same-fn.sh\n>\n> diff --git a/builtin-apply.c b/builtin-apply.c\n> index c497889..9f76ce4 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> +struct path_list fn_cache = {NULL, 0, 0, 0};\n\n\"Reuses a filename\"?  Do you mean touches the same file again?\n\nThis is not a \"cache\" in the sense that you can nuke it without changing\nthe behaviour except performance, but more about \"Record the postimage\npathnames (and contents, indirectly by pointing at the patch structure)\neach previous patch application would have created\".\n\nThere is a case where a normal git patch contains two separate patches to\nthe same file.  A typechange patch is always expressed as a deletion of\nthe old path immediately followed by a creation of the same path.  I have\nto wonder why that codepath for handing that particular special case is\nnot changed in this patch.  Surely the mechanism you are adding is a\ngeneralization that can cover such a case as well, isn't it?\n\n> @@ -2176,6 +2184,38 @@ 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 path_list_item *item;\n> +\n> +\titem = path_list_lookup(name, &fn_cache);\n> +\tif (item != NULL)\n> +\t\treturn (struct patch *)item->util;\n> +\n> +\treturn NULL;\n> +}\n> +\n> +void add_to_fn_cache(char *name, struct patch *patch)\n> +{\n> +\tstruct path_list_item *item;\n> +\n> +\t/* Always add new_name unless patch is a deletion */\n> +\tif (name != NULL) {\n> +\t\titem = path_list_insert(name, &fn_cache);\n> +\t\titem->util = patch;\n> +\t}\n> +\n> +\t/* skip normal diffs, creations and copies */\n\nThis comment is a \"Huh?\".\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 ((name == NULL) || (patch->is_rename)) {\n> +\t\titem = path_list_insert(patch->old_name, &fn_cache);\n> +\t\titem->util = (struct patch *) -1;\n\nIf you look at the patch->old_name _anyway_, why do you give a separate\nname parameter to this function?  The function would be much easier to\nread if you pass only patch, and use patch->new_name instead of the\nseparate name parameter.  Otherwise the reader needs to scroll down and\nfigure out that name is a new name by looking at the call site to\nunderstand what is going on.\n\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 +2228,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\nIsn't this wrong?  Why can't this new enhancement be used while operating\nonly on the index?\n\n>  \t\t\tif (ce) {\n>  \t\t\t\tread_file_or_gitlink(ce, &buf);\n>  \t\t\t} else {\n> @@ -2211,6 +2260,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\nSo after each patch application, the patch is remembered, keyed by the\npathname of the postimage, so by consulting fn_cache with a pathname, you\ncan know what the status of a path that was touched by previous patches\nis.  That sounds quite straightforward.\n"},{"id":"80376","messageId":"20080619213341.GP16941@redhat.com","threadId":"13985","inReplyTo":"7vbq1z375d.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] git-apply doesn't handle same name patches well [V3]","fromName":"Don Zickus","fromEmail":"dzickus@redhat.com","sentAt":"2008-06-19T21:33:41Z","receivedAt":"2008-06-19T21:33:41Z","isPatch":true,"sender":{"key":"dzickus@redhat.com","avatar":null},"body":"On Tue, Jun 17, 2008 at 05:42:54PM -0700, Junio C Hamano wrote:\n> Don Zickus <dzickus@redhat.com> writes:\n> \n> > When working with a lot of people who backport patches all day long, every\n> > once in a while I get a patch that modifies the same file more than once\n> > inside the same patch.  git-apply either fails if the second change relies\n> > on the first change or silently drops the first change if the second change\n> > is independent.\n> \n> Good issue to tackle.\n> \n> > A new test has been added to cover the cases I addressed.  However,\n> > currently adding changes to renamed file inside the same patch doesn't work\n> > correctly (it fails to find new file).  I didn't know how to fix this\n> > correctly, so I have the test fail expectedly.\n> \n> Do you mean that the first patch rename-edits A to B, and then the second\n> patch edits B in place?  Because your fn_cache is keyed by postimage\n> filename (in this case B), I would imagine that the later lookup of B\n> should successfully find the patch result from the previous one.  Unless\n> the in_fn_cache() is somehow wrong...\n\nYeah, I thought so too, but after debugging the problem, the 'lstat' in\ncheck_preimage() fails to find the file on disk and exits with that error.\n\nI was going to try to figure out a way to grab it from fn_cache but I\nwasn't sure how much of the 'lstat' info is needed later.\n\n> \n> or do you mean that the first patch rename-edits A to B, but the second\n> one still wants to edit A in place and you would want to pretend as if the\n> later one is for a patch to B?  I would think that is doable but asking\n> for too much magic, and a tool with too much magic is scary.\n\nPersonally I think this case should be a failure.  I even attached a\ntestcase in my patch to make sure this failed.  I wasn't comfortable doing\nthis magic either.\n\n> > +/*\n> > + * Caches patch filenames to handle the case where a\n> > + * patch chunk reuses a filename\n> > + */\n> > +\n> > +struct path_list fn_cache = {NULL, 0, 0, 0};\n> \n> \"Reuses a filename\"?  Do you mean touches the same file again?\n> \n> This is not a \"cache\" in the sense that you can nuke it without changing\n> the behaviour except performance, but more about \"Record the postimage\n> pathnames (and contents, indirectly by pointing at the patch structure)\n> each previous patch application would have created\".\n\nYes, poor choice of words.  I'll try to think of a something else.\n\n> \n> There is a case where a normal git patch contains two separate patches to\n> the same file.  A typechange patch is always expressed as a deletion of\n> the old path immediately followed by a creation of the same path.  I have\n> to wonder why that codepath for handing that particular special case is\n> not changed in this patch.  Surely the mechanism you are adding is a\n> generalization that can cover such a case as well, isn't it?\n\nHeh.  Maybe, but I didn't know the code well enough to do that.  Pointers?\nI'll try to poke around and see what I can cleanup, but I will have to\nrely on the mailing-list to make sure I did it correctly.\n\n> \n> > @@ -2176,6 +2184,38 @@ 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 path_list_item *item;\n> > +\n> > +\titem = path_list_lookup(name, &fn_cache);\n> > +\tif (item != NULL)\n> > +\t\treturn (struct patch *)item->util;\n> > +\n> > +\treturn NULL;\n> > +}\n> > +\n> > +void add_to_fn_cache(char *name, struct patch *patch)\n> > +{\n> > +\tstruct path_list_item *item;\n> > +\n> > +\t/* Always add new_name unless patch is a deletion */\n> > +\tif (name != NULL) {\n> > +\t\titem = path_list_insert(name, &fn_cache);\n> > +\t\titem->util = patch;\n> > +\t}\n> > +\n> > +\t/* skip normal diffs, creations and copies */\n> \n> This comment is a \"Huh?\".\nI was just making a note about which cases I wanted to skip and which ones\nI wanted to process.  I can expand on it.  For example, patches that\ncontain normal diffs, file creations and git copies or ignored as don't\ncares.  Only file deletions and git renames were interesting to me in the\ncode below.\n\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 ((name == NULL) || (patch->is_rename)) {\n> > +\t\titem = path_list_insert(patch->old_name, &fn_cache);\n> > +\t\titem->util = (struct patch *) -1;\n> \n> If you look at the patch->old_name _anyway_, why do you give a separate\n> name parameter to this function?  The function would be much easier to\n> read if you pass only patch, and use patch->new_name instead of the\n> separate name parameter.  Otherwise the reader needs to scroll down and\n> figure out that name is a new name by looking at the call site to\n> understand what is going on.\n\nYeah, leftover code that was added when I ran into rename and copy\nproblems.\n\n> \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 +2228,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> \n> Isn't this wrong?  Why can't this new enhancement be used while operating\n> only on the index?\n\nI don't know, can it?  You tell me.  I wasn't sure on the whole index\nthing worked.\n\nI'll respin the patch when I get some free time tomorrow or next week.\n\nCheers,\nDon\n"},{"id":"80388","messageId":"7vk5glrs0b.fsf@gitster.siamese.dyndns.org","threadId":"13985","inReplyTo":"20080619213341.GP16941@redhat.com","subject":"Re: [PATCH] git-apply doesn't handle same name patches well [V3]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-06-19T22:15:16Z","receivedAt":"2008-06-19T22:15:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Don Zickus <dzickus@redhat.com> writes:\n\n> On Tue, Jun 17, 2008 at 05:42:54PM -0700, Junio C Hamano wrote:\n>> Don Zickus <dzickus@redhat.com> writes:\n> ...\n> I was going to try to figure out a way to grab it from fn_cache but I\n> wasn't sure how much of the 'lstat' info is needed later.\n\nThe usual case of one-diff-one-path patch application wants to make sure\nthat there is no discrepancy between the index and the work tree for the\npath when working in --index mode.  When working in work-tree-only mode,\nlstat just makes sure that the path to be patched actually exists (or\ndoesn't, if it is a creation patch).\n\nWhen fn_cache is used, you pretend as if the resulting path exists and up\nto date when found in fn_cache and the previous round succeeded, so you\ncan substitute the lstat (you cannot just lose it, but need to make sure\nthe path exists after applying the earlier fn_cache contents when handling\na later patch that wants to touch an existing file).  As you pretend that\nthe previous round succeeded, you do not have to check the up-to-dateness\nbetween the index and work tree when dealing with a path that has previous\nresult in fn_cache, even when operating in --index mode.\n\n>> or do you mean that the first patch rename-edits A to B, but the second\n>> one still wants to edit A in place and you would want to pretend as if the\n>> later one is for a patch to B?  I would think that is doable but asking\n>> for too much magic, and a tool with too much magic is scary.\n>\n> Personally I think this case should be a failure.  I even attached a\n> testcase in my patch to make sure this failed.  I wasn't comfortable doing\n> this magic either.\n\nGood.\n\n>> There is a case where a normal git patch contains two separate patches to\n>> the same file.  A typechange patch is always expressed as a deletion of\n>> the old path immediately followed by a creation of the same path.  I have\n>> to wonder why that codepath for handing that particular special case is\n>> not changed in this patch.  Surely the mechanism you are adding is a\n>> generalization that can cover such a case as well, isn't it?\n>\n> Heh.  Maybe, but I didn't know the code well enough to do that.  Pointers?\n\nSee the way \"prev_patch\" is used in check_patch.\n\n>> > @@ -2176,6 +2184,38 @@ 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 path_list_item *item;\n>> > +\n>> > +\titem = path_list_lookup(name, &fn_cache);\n>> > +\tif (item != NULL)\n>> > +\t\treturn (struct patch *)item->util;\n>> > +\n>> > +\treturn NULL;\n>> > +}\n>> > +\n>> > +void add_to_fn_cache(char *name, struct patch *patch)\n>> > +{\n>> > +\tstruct path_list_item *item;\n>> > +\n>> > +\t/* Always add new_name unless patch is a deletion */\n>> > +\tif (name != NULL) {\n>> > +\t\titem = path_list_insert(name, &fn_cache);\n>> > +\t\titem->util = patch;\n>> > +\t}\n>> > +\n>> > +\t/* skip normal diffs, creations and copies */\n>> \n>> This comment is a \"Huh?\".\n> I was just making a note about which cases I wanted to skip and which ones\n> I wanted to process.  I can expand on it.  For example, patches that\n> contain normal diffs, file creations and git copies or ignored as don't\n> cares.  Only file deletions and git renames were interesting to me in the\n> code below.\n\nThe function's purpose is to record what the expected state of the path\nafter the current patch has applied successfully, and\n\n * If it is not a deletion, you will record the postimage, so that a later\n   patch can work from there, not from what is in the initial state;\n\n * If it is a deletion (or rename-away), you record that the path after\n   this patch no longer exists, so that you can catch a later broken patch\n   that tries to touch the path.\n\nYou have already handled \"normal diff, creation and copy\" in the first\npart \"if (name != NULL)\", but you talk about it again here, which was the\n\"Huh?\" inducing part.  It gave an impression that the part that follows\ndoes something other than the above two.\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 ((name == NULL) || (patch->is_rename)) {\n>> > +\t\titem = path_list_insert(patch->old_name, &fn_cache);\n>> > +\t\titem->util = (struct patch *) -1;\n\nIf you have a patch that does A->B (rename), C->A (rename), A->A (mod),\nwould your code handle that?\n\n>> If you look at the patch->old_name _anyway_, why do you give a separate\n>> name parameter to this function?  The function would be much easier to\n>> read if you pass only patch, and use patch->new_name instead of the\n>> separate name parameter.  Otherwise the reader needs to scroll down and\n>> figure out that name is a new name by looking at the call site to\n>> understand what is going on.\n>\n> Yeah, leftover code that was added when I ran into rename and copy\n> problems.\n>\n>> \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 +2228,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>> \n>> Isn't this wrong?  Why can't this new enhancement be used while operating\n>> only on the index?\n>\n> I don't know, can it?  You tell me.  I wasn't sure on the whole index\n> thing worked.\n\nPerhaps a \"git-apply\" primer might help.  The program can work in three\nmodes of operation.\n\n * normal mode: look at and operate only on work tree files.\n\n * --index mode: work on both the index and the work tree.  IOW, patch the\n   files and immediately do \"git add -u\" on that path, so that even\n   addition and deletion are recorded in the index.  In this case, the\n   paths involved must be up-to-date between the work tree and the index\n   when you start \"git apply\"; otherwise you will lose your local\n   changes.\n\n * --cached mode: work only on the index and never look at nor touch the\n   work tree.\n\nNow, if you have a patch that has A->A (mod), followed by another A->A(mod),\nis there a good reason why you allow it in the first two modes and not the\nlast one?  I do not think so.\n"}]}