{"thread":{"id":"40111","subject":"\"git am --abort\" screwing up index?","startedAt":"2015-08-16T19:46:15Z","lastAt":"2015-08-19T17:55:20Z","messageCount":9,"participants":["Linus Torvalds","Johannes Schindelin","Paul Tan","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"268155","messageId":"CA+55aFwDkQAS8ULiLt9N5NVOYJ242Nd2MOWeiRX8HrVHXf2zog@mail.gmail.com","threadId":"40111","inReplyTo":null,"subject":"\"git am --abort\" screwing up index?","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-08-16T19:46:15Z","receivedAt":"2015-08-16T19:46:15Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"So I just noticed while applying a patch with \"git am\" when I had a\ndirty tree, and I ended up getting a failure and starting over:\n\n [torvalds@i7 linux]$ git am --abort\n [torvalds@i7 linux]$ git reset --hard\n Checking out files: 100% (50794/50794), done.0794)\n HEAD is now at 1efdb5f0a924 Merge tag 'scsi-fixes' of\ngit://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi\n\nand the thing I reacted to is that the \"git reset --hard\" re-checked\nout all the files.\n\nThat implies that \"git am --abort\" ended up leaving the index in a bad\nstate, presumably it re-did the index entirely from HEAD, without\nfilling it in with the stat() details from the old index.\n\nMaybe it has always done this, and I just haven't noticed (I usually\n_just_ do the \"git reset --hard\" thing, don't ask me why I wanted to\nbe doubly sure this time). But maybe it's an effect of the new\nbuilt-in \"am\".\n\nI'm about to go out and don't have time to debug this any further\nright now, but I'll try to get back to it later. I thought I'd send\nout this email in case it makes Paul goes \"ahh, yes.. obvious\"\n\nNot a big deal - things *work* fine. But forcing checking out every\nfile obviously also means that subsequent builds end up being slowed\ndown etc.,.\n\n Linus\n"},{"id":"268157","messageId":"CA+55aFwwD=K-i9d40N5FtnTLT-ApZOzmgnXhnuA=C0zw2eBt3Q@mail.gmail.com","threadId":"40111","inReplyTo":"CA+55aFwDkQAS8ULiLt9N5NVOYJ242Nd2MOWeiRX8HrVHXf2zog@mail.gmail.com","subject":"Re: \"git am --abort\" screwing up index?","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-08-16T23:33:33Z","receivedAt":"2015-08-16T23:33:33Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Sun, Aug 16, 2015 at 12:46 PM, Linus Torvalds\n wrote:\n>\n> Maybe it has always done this, and I just haven't noticed (I usually\n> _just_ do the \"git reset --hard\" thing, don't ask me why I wanted to\n> be doubly sure this time). But maybe it's an effect of the new\n> built-in \"am\".\n\nI bisected this. It's definitely used to work, and the regression is\nfrom the new built-in am. But I cannot bisect into that branch\n'pt/am-builtin', because \"git am\" doesn't actually work in the middle\nof that branch.\n\nSo I've verified that commit c1e5ca90dba8 (\"Merge branch\n'es/worktree-add'\") is good, and that commit 7aa2da616208 (\"Merge\nbranch 'pt/am-builtin'\") is bad, but I cannot pinpoint the exact\ncommit where \"git am --abort\" starts breaking the index.\n\nBut I assume it's simply that initial implementation of \"--abort\" in\ncommit 33388a71d23e (\"builtin-am: implement --abort\") that already\nends up rewriting the index from scratch without applying the old stat\ndata.\n\nThe test-case is pretty simple: just force a \"git am\" failure, then do\n\"git am --abort\", and then you can check whether the index stat()\ninformation is valid in various ways. For the kernel, doing a \"git\nreset --hard\" makes it obvious because the reset will force all files\nto be written out (since the index stat information doesn't match the\ncurrent tree). But you can do it by just counting system calls for a\n\"git diff\" too. On the git tree, for example, when the index has\nmatching stat information, you get something like\n\n [torvalds@i7 git]$ strace -cf git diff\n ..\n 0.04 0.000025 1 26 4 open\n ..\n\nie you only actually ended up with 26 open() system calls. When the\nindex is not in sync with the stat information, \"git diff\" will have\nto open each file to see what the actual contents are, and you get\n\n [torvalds@i7 git]$ strace -cf git diff\n ...\n 0.30 0.000070 0 5987 302 open\n ...\n\nso now it opened about 6k files instead (and for the kernel, that\nnumber will be much larger, of course).\n\nI _think_ it's because git-am (in \"clean_index()\") uses read_tree(),\nwhile it probably should use \"unpack_trees\" with opts.update and\nopts.reset set (like reset_index() does in builtin/reset.h).\n\nI have to go off do my weekly -rc now, and probably won't get to\ndebugging this much further. Adding Stefan to the cc, since he helped\nwith that \"--abort\" implementation.\n\n Linus\n"},{"id":"268167","messageId":"03631611149f05dbcd862b4c1e8e9d6b@www.dscho.org","threadId":"40111","inReplyTo":"CA+55aFwwD=K-i9d40N5FtnTLT-ApZOzmgnXhnuA=C0zw2eBt3Q@mail.gmail.com","subject":"Re: \"git am --abort\" screwing up index?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-17T08:01:29Z","receivedAt":"2015-08-17T08:01:29Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Linus,\n\nOn 2015-08-17 01:33, Linus Torvalds wrote:\n> On Sun, Aug 16, 2015 at 12:46 PM, Linus Torvalds\n> wrote:\n>>\n>> Maybe it has always done this, and I just haven't noticed (I usually\n>> _just_ do the \"git reset --hard\" thing, don't ask me why I wanted to\n>> be doubly sure this time). But maybe it's an effect of the new\n>> built-in \"am\".\n> \n> I bisected this. It's definitely used to work, and the regression is\n> from the new built-in am.\n\nThis patch is a reproducer:\n\n-- snipsnap --\nFrom 5323f1c309ad40721e2e19fa9c6ce5ad52d98271 Mon Sep 17 00:00:00 2001\nFrom: Johannes Schindelin \nDate: Mon, 17 Aug 2015 09:37:39 +0200\nSubject: [PATCH] t4151: demonstrate that builtin am corrupts index' stat data\n\nReported by Linus Torvalds.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t4151-am-abort.sh | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 05bdc3e..bf2e6f4 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -168,4 +168,16 @@ test_expect_success 'am --abort on unborn branch will keep local commits intact'\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'am --abort leaves index stat info alone' '\n+\tgit checkout -f --orphan stat-info &&\n+\tgit reset &&\n+\ttest_commit should-be-untouched &&\n+\ttest-chmtime =0 should-be-untouched.t &&\n+\tgit update-index --refresh &&\n+\tgit diff-files --exit-code --quiet &&\n+\ttest_must_fail git am 0001-*.patch &&\n+\tgit am --abort &&\n+\tgit diff-files --exit-code --quiet\n+'\n+\n test_done\n-- \n2.3.1.windows.1.9.g8c01ab4\n"},{"id":"268177","messageId":"20150817094819.GA10375@yoshi.chippynet.com","threadId":"40111","inReplyTo":"03631611149f05dbcd862b4c1e8e9d6b@www.dscho.org","subject":"[PATCH] am --abort: merge ORIG_HEAD tree into index","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-17T09:48:19Z","receivedAt":"2015-08-17T09:48:19Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Mon, Aug 17, 2015 at 10:01:29AM +0200, Johannes Schindelin wrote:\n> Hi Linus,\n> \n> On 2015-08-17 01:33, Linus Torvalds wrote:\n> > On Sun, Aug 16, 2015 at 12:46 PM, Linus Torvalds\n> > wrote:\n> >>\n> >> Maybe it has always done this, and I just haven't noticed (I usually\n> >> _just_ do the \"git reset --hard\" thing, don't ask me why I wanted to\n> >> be doubly sure this time). But maybe it's an effect of the new\n> >> built-in \"am\".\n> > \n> > I bisected this. It's definitely used to work, and the regression is\n> > from the new built-in am.\n> \n> This patch is a reproducer:\n\nThanks Johannes for the test, and sorry all for the oversight. ><\n\nIt's true that we need to merge the ORIG_HEAD tree into the index\ninstead of overwriting it. Patch below.\n\nRegards,\nPaul\n\n-- >8 --\nSubject: [PATCH] am --abort: merge ORIG_HEAD tree into index\n\nAfter running \"git am --abort\", and then running \"git reset --hard\",\nfiles that were not modified would still be re-checked out.\n\nThis is because clean_index() in builtin/am.c mistakenly called the\nread_tree() function, which overwrites all entries in the index,\nincluding the stat info.\n\nFix this by using unpack_trees() instead to merge the tree into the\nindex, so that the stat info from the index is kept.\n\nReported-by: Linus Torvalds \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Paul Tan \n---\n builtin/am.c | 49 ++++++++++++++++++++++++++++++++++++-------------\n t/t4151-am-abort.sh | 12 ++++++++++++\n 2 files changed, 48 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1399c8d..6aaa85d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1940,15 +1940,48 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n }\n \n /**\n+ * Merges a tree into the index. The index's stat info will take precedence\n+ * over the merged tree's. Returns 0 on success, -1 on failure.\n+ */\n+static int merge_tree(struct tree *tree)\n+{\n+\tstruct lock_file *lock_file;\n+\tstruct unpack_trees_options opts;\n+\tstruct tree_desc t[2];\n+\n+\tif (parse_tree(tree))\n+\t\treturn -1;\n+\n+\tlock_file = xcalloc(1, sizeof(struct lock_file));\n+\thold_locked_index(lock_file, 1);\n+\n+\tmemset(&opts, 0, sizeof(opts));\n+\topts.head_idx = 1;\n+\topts.src_index = &the_index;\n+\topts.dst_index = &the_index;\n+\topts.merge = 1;\n+\topts.fn = oneway_merge;\n+\tinit_tree_desc(&t[0], tree->buffer, tree->size);\n+\n+\tif (unpack_trees(1, t, &opts)) {\n+\t\trollback_lock_file(lock_file);\n+\t\treturn -1;\n+\t}\n+\n+\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\t\tdie(_(\"unable to write new index file\"));\n+\n+\treturn 0;\n+}\n+\n+/**\n * Clean the index without touching entries that are not modified between\n * `head` and `remote`.\n */\n static int clean_index(const unsigned char *head, const unsigned char *remote)\n {\n-\tstruct lock_file *lock_file;\n \tstruct tree *head_tree, *remote_tree, *index_tree;\n \tunsigned char index[GIT_SHA1_RAWSZ];\n-\tstruct pathspec pathspec;\n \n \thead_tree = parse_tree_indirect(head);\n \tif (!head_tree)\n@@ -1973,18 +2006,8 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)\n \tif (fast_forward_to(index_tree, remote_tree, 0))\n \t\treturn -1;\n \n-\tmemset(&pathspec, 0, sizeof(pathspec));\n-\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, 1);\n-\n-\tif (read_tree(remote_tree, 0, &pathspec)) {\n-\t\trollback_lock_file(lock_file);\n+\tif (merge_tree(remote_tree))\n \t\treturn -1;\n-\t}\n-\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n-\t\tdie(_(\"unable to write new index file\"));\n \n \tremove_branch_state();\n \ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 05bdc3e..9c3bbd1 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -168,4 +168,16 @@ test_expect_success 'am --abort on unborn branch will keep local commits intact'\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'am --abort leaves index stat info alone' '\n+\tgit checkout -f --orphan stat-info &&\n+\tgit reset &&\n+\ttest_commit should-be-untouched &&\n+\ttest-chmtime =0 should-be-untouched.t &&\n+\tgit update-index --refresh &&\n+\tgit diff-files --exit-code --quiet &&\n+\ttest_must_fail git am 0001-*.patch &&\n+\tgit am --abort &&\n+\tgit diff-files --exit-code --quiet\n+'\n+\n test_done\n-- \n2.5.0.331.g11c07ce\n"},{"id":"268178","messageId":"33846154f448fa217389d91583ff081e@www.dscho.org","threadId":"40111","inReplyTo":"20150817094819.GA10375@yoshi.chippynet.com","subject":"Re: [PATCH] am --abort: merge ORIG_HEAD tree into index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-08-17T10:09:58Z","receivedAt":"2015-08-17T10:09:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-08-17 11:48, Paul Tan wrote:\n\n> It's true that we need to merge the ORIG_HEAD tree into the index\n> instead of overwriting it. Patch below.\n\nThanks for your impressive, very responsive work!\nDscho\n"},{"id":"268186","messageId":"CA+55aFzvw4ogQCUpEQXtOrpbvehXiVHTfQsPDk0-ZhEaEchm2Q@mail.gmail.com","threadId":"40111","inReplyTo":"20150817094819.GA10375@yoshi.chippynet.com","subject":"Re: [PATCH] am --abort: merge ORIG_HEAD tree into index","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-08-17T14:54:46Z","receivedAt":"2015-08-17T14:54:46Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Aug 17, 2015 at 2:48 AM, Paul Tan wrote:\n>\n> It's true that we need to merge the ORIG_HEAD tree into the index\n> instead of overwriting it. Patch below.\n\nSeems to work for me. Thanks,\n\n Linus\n"},{"id":"268212","messageId":"xmqqsi7hd817.fsf@gitster.dls.corp.google.com","threadId":"40111","inReplyTo":"20150817094819.GA10375@yoshi.chippynet.com","subject":"Re: [PATCH] am --abort: merge ORIG_HEAD tree into index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-17T19:33:40Z","receivedAt":"2015-08-17T19:33:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\nThe codepath in the original looks like this:\n\n head_tree=$(git rev-parse --verify -q HEAD || echo $empty_tree) &&\n==> git read-tree --reset -u $head_tree $head_tree &&\n index_tree=$(git write-tree) &&\n orig_head=$(git rev-parse --verify -q ORIG_HEAD || echo $empty_tree) &&\n==> git read-tree -m -u $index_tree $orig_head\n if git rev-parse --verify -q ORIG_HEAD >/dev/null 2>&1\n then\n git reset ORIG_HEAD\n else\n git read-tree $empty_tree\n curr_branch=$(git symbolic-ref HEAD 2>/dev/null) &&\n git update-ref -d $curr_branch\n fi\n\nYour am_abort() implements the above fairly faithfully up to the\npoint where it computes orig_head. Your clean_index() function that\nis called from there roughly corresponds to the \"read-tree --reset -u\"\nto reset the index to the HEAD's tree and then \"read-tree -m -u\" to\ngo to ORIG_HEAD from $index_tree.\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 1399c8d..6aaa85d 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1940,15 +1940,48 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n> }\n> \n> /**\n> + * Merges a tree into the index. The index's stat info will take precedence\n> + * over the merged tree's. Returns 0 on success, -1 on failure.\n> + */\n> +static int merge_tree(struct tree *tree)\n> +{\n> +...\n> +}\n\nThis looks more like \"git reset ORIG_HEAD\" in the original above ;-)\n\n> +\n> +/**\n> * Clean the index without touching entries that are not modified between\n> * `head` and `remote`.\n> */\n> static int clean_index(const unsigned char *head, const unsigned char *remote)\n> {\n> -\tstruct lock_file *lock_file;\n> \tstruct tree *head_tree, *remote_tree, *index_tree;\n> \tunsigned char index[GIT_SHA1_RAWSZ];\n> -\tstruct pathspec pathspec;\n> \n> \thead_tree = parse_tree_indirect(head);\n> \tif (!head_tree)\n> @@ -1973,18 +2006,8 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)\n> \tif (fast_forward_to(index_tree, remote_tree, 0))\n> \t\treturn -1;\n> \n> -\tmemset(&pathspec, 0, sizeof(pathspec));\n> -\n> -\tlock_file = xcalloc(1, sizeof(struct lock_file));\n> -\thold_locked_index(lock_file, 1);\n> -\n> -\tif (read_tree(remote_tree, 0, &pathspec)) {\n> -\t\trollback_lock_file(lock_file);\n> +\tif (merge_tree(remote_tree))\n> \t\treturn -1;\n> -\t}\n> -\n> -\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n> -\t\tdie(_(\"unable to write new index file\"));\n\nAnd by getting rid of the call to \"one-tree from scratch\" form or\nread_tree(), we can lose quite a lot of code from here. Good ;-)\n\nNote that \"am skip\" codepath also calls clean_index(), so this patch\nwould affect it.\n\nHave you checked how this change affects that codepath? To put it\ndifferently, does \"am skip\" have the same issue without this fix?\nIf so, I wonder if we can have a test for that, too?\n\nThanks.\n\n> diff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\n> index 05bdc3e..9c3bbd1 100755\n> --- a/t/t4151-am-abort.sh\n> +++ b/t/t4151-am-abort.sh\n> @@ -168,4 +168,16 @@ test_expect_success 'am --abort on unborn branch will keep local commits intact'\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'am --abort leaves index stat info alone' '\n> +\tgit checkout -f --orphan stat-info &&\n> +\tgit reset &&\n> +\ttest_commit should-be-untouched &&\n> +\ttest-chmtime =0 should-be-untouched.t &&\n> +\tgit update-index --refresh &&\n> +\tgit diff-files --exit-code --quiet &&\n> +\ttest_must_fail git am 0001-*.patch &&\n> +\tgit am --abort &&\n> +\tgit diff-files --exit-code --quiet\n> +'\n> +\n> test_done\n"},{"id":"268292","messageId":"20150819082222.GA27685@potato.chippynet.com","threadId":"40111","inReplyTo":"xmqqsi7hd817.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] am --skip/--abort: merge HEAD/ORIG_HEAD tree into index","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-08-19T08:22:22Z","receivedAt":"2015-08-19T08:22:22Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Mon, Aug 17, 2015 at 12:33:40PM -0700, Junio C Hamano wrote:\n> Have you checked how this change affects that codepath? To put it\n> differently, does \"am skip\" have the same issue without this fix?\n\nHmm, I adopted Dscho's test to run \"git am --skip\" and it did not fail.\nI think it's because am_skip() calls am_run(), which calls\nrefresh_cache(), so the resulting index will have the updated stat info.\nHowever, there should still be a performance penalty because\nrefresh_cache() would have to scan all files for changes.\n\n> If so, I wonder if we can have a test for that, too?\n\nSo yeah, we should have a test for that too.\n\n(In addition, I fixed a small mistake with the \"struct tree_desc\" array\nsize.)\n\nThanks,\nPaul\n\n-- >8 --\nSubject: [PATCH v2] am --skip/--abort: merge HEAD/ORIG_HEAD tree into index\n\nAfter running \"git am --abort\", and then running \"git reset --hard\",\nfiles that were not modified would still be re-checked out.\n\nThis is because clean_index() in builtin/am.c mistakenly called the\nread_tree() function, which overwrites all entries in the index,\nincluding the stat info.\n\n\"git am --skip\" did not seem to have this issue because am_skip() called\nam_run(), which called refresh_cache() to update the stat info. However,\nthere's still a performance penalty as the lack of stat info meant that\nrefresh_cache() would have to scan all files for changes.\n\nFix this by using unpack_trees() instead to merge the tree into the\nindex, so that the stat info from the index is kept.\n\nReported-by: Linus Torvalds \nHelped-by: Junio C Hamano \nSigned-off-by: Johannes Schindelin \nSigned-off-by: Paul Tan \n---\n builtin/am.c | 49 ++++++++++++++++++++++++++++++++++++-------------\n t/t4151-am-abort.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 60 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1399c8d..3e7e66f 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1940,15 +1940,48 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n }\n \n /**\n+ * Merges a tree into the index. The index's stat info will take precedence\n+ * over the merged tree's. Returns 0 on success, -1 on failure.\n+ */\n+static int merge_tree(struct tree *tree)\n+{\n+\tstruct lock_file *lock_file;\n+\tstruct unpack_trees_options opts;\n+\tstruct tree_desc t[1];\n+\n+\tif (parse_tree(tree))\n+\t\treturn -1;\n+\n+\tlock_file = xcalloc(1, sizeof(struct lock_file));\n+\thold_locked_index(lock_file, 1);\n+\n+\tmemset(&opts, 0, sizeof(opts));\n+\topts.head_idx = 1;\n+\topts.src_index = &the_index;\n+\topts.dst_index = &the_index;\n+\topts.merge = 1;\n+\topts.fn = oneway_merge;\n+\tinit_tree_desc(&t[0], tree->buffer, tree->size);\n+\n+\tif (unpack_trees(1, t, &opts)) {\n+\t\trollback_lock_file(lock_file);\n+\t\treturn -1;\n+\t}\n+\n+\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\t\tdie(_(\"unable to write new index file\"));\n+\n+\treturn 0;\n+}\n+\n+/**\n * Clean the index without touching entries that are not modified between\n * `head` and `remote`.\n */\n static int clean_index(const unsigned char *head, const unsigned char *remote)\n {\n-\tstruct lock_file *lock_file;\n \tstruct tree *head_tree, *remote_tree, *index_tree;\n \tunsigned char index[GIT_SHA1_RAWSZ];\n-\tstruct pathspec pathspec;\n \n \thead_tree = parse_tree_indirect(head);\n \tif (!head_tree)\n@@ -1973,18 +2006,8 @@ static int clean_index(const unsigned char *head, const unsigned char *remote)\n \tif (fast_forward_to(index_tree, remote_tree, 0))\n \t\treturn -1;\n \n-\tmemset(&pathspec, 0, sizeof(pathspec));\n-\n-\tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, 1);\n-\n-\tif (read_tree(remote_tree, 0, &pathspec)) {\n-\t\trollback_lock_file(lock_file);\n+\tif (merge_tree(remote_tree))\n \t\treturn -1;\n-\t}\n-\n-\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n-\t\tdie(_(\"unable to write new index file\"));\n \n \tremove_branch_state();\n \ndiff --git a/t/t4151-am-abort.sh b/t/t4151-am-abort.sh\nindex 05bdc3e..ea5ace9 100755\n--- a/t/t4151-am-abort.sh\n+++ b/t/t4151-am-abort.sh\n@@ -168,4 +168,28 @@ test_expect_success 'am --abort on unborn branch will keep local commits intact'\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'am --skip leaves index stat info alone' '\n+\tgit checkout -f --orphan skip-stat-info &&\n+\tgit reset &&\n+\ttest_commit skip-should-be-untouched &&\n+\ttest-chmtime =0 skip-should-be-untouched.t &&\n+\tgit update-index --refresh &&\n+\tgit diff-files --exit-code --quiet &&\n+\ttest_must_fail git am 0001-*.patch &&\n+\tgit am --skip &&\n+\tgit diff-files --exit-code --quiet\n+'\n+\n+test_expect_success 'am --abort leaves index stat info alone' '\n+\tgit checkout -f --orphan abort-stat-info &&\n+\tgit reset &&\n+\ttest_commit abort-should-be-untouched &&\n+\ttest-chmtime =0 abort-should-be-untouched.t &&\n+\tgit update-index --refresh &&\n+\tgit diff-files --exit-code --quiet &&\n+\ttest_must_fail git am 0001-*.patch &&\n+\tgit am --abort &&\n+\tgit diff-files --exit-code --quiet\n+'\n+\n test_done\n-- \n2.5.0\n"},{"id":"268331","messageId":"xmqqy4h79n93.fsf@gitster.dls.corp.google.com","threadId":"40111","inReplyTo":"20150819082222.GA27685@potato.chippynet.com","subject":"Re: [PATCH v2] am --skip/--abort: merge HEAD/ORIG_HEAD tree into index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-19T17:55:20Z","receivedAt":"2015-08-19T17:55:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> (In addition, I fixed a small mistake with the \"struct tree_desc\" array\n> size.)\n\nYeah, one thing I forgot to mention was that I suspect this kind of\n\"oneway merge\" already in other places in the code, and a future\nmini-project might be to inspect them all and try to see if their\ncommonalities can be taken advantage of. If we had a small helper\nfunction to do the \"do the equivalent of 'git reset $TREE'\", this\nmiscounting wouldn't have happened.\n\nThe change looks sensible. Thanks.\n"}]}