{"thread":{"id":"48927","subject":"[BUG] merge-recursive overly aggressive when skipping updating the working tree","startedAt":"2018-07-20T19:53:11Z","lastAt":"2018-07-31T16:11:04Z","messageCount":22,"participants":["Ben Peart","Elijah Newren","Junio C Hamano","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"353179","messageId":"5a8d1098-b4c5-64e1-da98-dac13521e7ba@gmail.com","threadId":"48927","inReplyTo":null,"subject":"[BUG] merge-recursive overly aggressive when skipping updating the working tree","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-20T19:53:07Z","receivedAt":"2018-07-20T19:53:11Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"As we were attempting to migrate to 2.18 some of our internal functional \ntests failed. The tests that failed were testing merge and cherry-pick \nwhen there was a merge conflict. Our tests run with sparse-checkout \nenabled which is what exposed the bug.\n\nWhat is happening is that in merge_recursive, the skip-worktree bit is \ncleared on the cache entry but then the working directory isn't updated. \n The end result is that git reports that the merged file has actually \nbeen deleted.\n\nWe've identified the patch that introduced the regression as:\n\ncommit 1de70dbd1ada0069d1b6cd6345323906cc9a9ed3\nAuthor: Elijah Newren \nDate: Thu Apr 19 10:58:23 2018 -0700\n\n merge-recursive: fix check for skipability of working tree updates\n\n The can-working-tree-updates-be-skipped check has had a long and \nblemished\n history. The update can be skipped iff:\n a) The merge is clean\n b) The merge matches what was in HEAD (content, mode, pathname)\n c) The target path is usable (i.e. not involved in D/F conflict)\n\n\nI've written a test that can be used to reproduce the issue:\n\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh \nb/t/t3507-cherry-pick-conflict.sh\nindex 7c5ad08626..de0bdc8634 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the \nsign-off at the right place' '\n\n test_cmp expect actual\n '\n\n+test_expect_success 'failed cherry-pick with sparse-checkout' '\n+ pristine_detach initial &&\n+ git config core.sparseCheckout true &&\n+ echo /unrelated >.git/info/sparse-checkout &&\n+ git read-tree --reset -u HEAD &&\n+ test_must_fail git cherry-pick -Xours picked>actual &&\n+ test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n+ echo \"/*\" >.git/info/sparse-checkout &&\n+ git read-tree --reset -u HEAD &&\n+ git config core.sparseCheckout false &&\n+ rm .git/info/sparse-checkout\n+'\n+\n test_done\n\nThanks,\n\nBen\n"},{"id":"353183","messageId":"CABPp-BF+Vx8YT2KAJQ+szbkYExv-_o5E-ZkywgvzsHWR0QvVEg@mail.gmail.com","threadId":"48927","inReplyTo":"5a8d1098-b4c5-64e1-da98-dac13521e7ba@gmail.com","subject":"Re: [BUG] merge-recursive overly aggressive when skipping updating the working tree","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-20T20:48:37Z","receivedAt":"2018-07-20T20:48:40Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 20, 2018 at 12:53 PM, Ben Peart wrote:\n> As we were attempting to migrate to 2.18 some of our internal functional\n> tests failed. The tests that failed were testing merge and cherry-pick when\n> there was a merge conflict. Our tests run with sparse-checkout enabled which\n> is what exposed the bug.\n\nIndeed, I've never used sparse checkout before. And I've got\nquestions related to it below...\n\n> What is happening is that in merge_recursive, the skip-worktree bit is\n> cleared on the cache entry but then the working directory isn't updated.\n> The end result is that git reports that the merged file has actually been\n> deleted.\n>\n> We've identified the patch that introduced the regression as:\n>\n> commit 1de70dbd1ada0069d1b6cd6345323906cc9a9ed3\n> Author: Elijah Newren \n> Date: Thu Apr 19 10:58:23 2018 -0700\n>\n> merge-recursive: fix check for skipability of working tree updates\n>\n> The can-working-tree-updates-be-skipped check has had a long and\n> blemished\n> history. The update can be skipped iff:\n> a) The merge is clean\n> b) The merge matches what was in HEAD (content, mode, pathname)\n> c) The target path is usable (i.e. not involved in D/F conflict)\n>\n>\n> I've written a test that can be used to reproduce the issue:\n>\n>\n> diff --git a/t/t3507-cherry-pick-conflict.sh\n> b/t/t3507-cherry-pick-conflict.sh\n> index 7c5ad08626..de0bdc8634 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the\n> sign-off at the right place' '\n>\n> test_cmp expect actual\n> '\n>\n> +test_expect_success 'failed cherry-pick with sparse-checkout' '\n> + pristine_detach initial &&\n> + git config core.sparseCheckout true &&\n> + echo /unrelated >.git/info/sparse-checkout &&\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git cherry-pick -Xours picked>actual &&\n> + test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n> + echo \"/*\" >.git/info/sparse-checkout &&\n> + git read-tree --reset -u HEAD &&\n> + git config core.sparseCheckout false &&\n> + rm .git/info/sparse-checkout\n> +'\n> +\n> test_done\n\nThanks for cooking up a testcase. In short, you've got:\n - a one-line file that was modified on both sides\n - you explicitly set the skip-worktree bit for this file (by the\ncore.sparseCheckout and read-tree steps),\n suggesting that you DONT want it being written to your working tree\n - you're using -Xours to avoid what would otherwise be a conflict\n\nSo, I actually think merge-recursive is behaving correctly with\nrespect to the working tree here, because:\n - You said you didn't want foo in your working copy with your\nsparse-checkout pattern\n - You manually nuked foo from your working copy when you called 'git\nread-tree --reset -u HEAD'\n - You setup this merge such that the merge result was the same as\nbefore the merge started.\nIn short, the file didn't change AND you don't want it in your working\ntree, so why write it out?\n\nTo me, the bug is that merge-recursive clears the skip-worktree bit\nfor foo when that should be left intact -- at least in this case.\n\n\nBut that brings up another interesting question. What if a merge\n*does* modify a file for which you have skip-worktree set?\nPreviously, it'd clear the bit and write the file to the working tree,\nbut that was by no means an explicit decision; that was just a side\neffect of the commands it uses. Isn't that choice wrong -- shouldn't\nit just update the index and continue on? Or, if there are conflicts,\nis that a case that is considered special where you do want the\nskip-worktree bit cleared and have the file written out?\n\nI'm worried that getting skip-worktree right in merge-recursive, when\nit had never been considered before with that codebase, might be a\nlittle messy...\n"},{"id":"353185","messageId":"xmqqsh4da9wd.fsf@gitster-ct.c.googlers.com","threadId":"48927","inReplyTo":"CABPp-BF+Vx8YT2KAJQ+szbkYExv-_o5E-ZkywgvzsHWR0QvVEg@mail.gmail.com","subject":"Re: [BUG] merge-recursive overly aggressive when skipping updating the working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-20T21:13:06Z","receivedAt":"2018-07-20T21:13:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> But that brings up another interesting question. What if a merge\n> *does* modify a file for which you have skip-worktree set?\n> Previously, it'd clear the bit and write the file to the working tree,\n> but that was by no means an explicit decision;\n\nAt least in my mind, the \"skip worktree\" aka sparse checkout has\nalways been \"best effort\" in that if Git needs to materialize a\nworking tree file in order to carry out some operation (e.g. a merge\nneeds conflict resolution, hence we need to give a working tree file\nwith conflict markers to the end user) Git is free to do so.\n\nIsn't that what happens currently?\n"},{"id":"353192","messageId":"CABPp-BE_OHxMqmhJ8BisG6HFpGQjsPJ1AE4G9m5bpJWznkq-1Q@mail.gmail.com","threadId":"48927","inReplyTo":"xmqqsh4da9wd.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] merge-recursive overly aggressive when skipping updating the working tree","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-20T21:42:35Z","receivedAt":"2018-07-20T21:42:38Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 20, 2018 at 2:13 PM, Junio C Hamano wrote:\n> Elijah Newren writes:\n>\n>> But that brings up another interesting question. What if a merge\n>> *does* modify a file for which you have skip-worktree set?\n>> Previously, it'd clear the bit and write the file to the working tree,\n>> but that was by no means an explicit decision;\n>\n> At least in my mind, the \"skip worktree\" aka sparse checkout has\n> always been \"best effort\" in that if Git needs to materialize a\n> working tree file in order to carry out some operation (e.g. a merge\n> needs conflict resolution, hence we need to give a working tree file\n> with conflict markers to the end user) Git is free to do so.\n>\n> Isn't that what happens currently?\n\nAh, okay, that's helpful. So, if there are conflicts, it should be\nfree to clear the skip_worktree flag. Since merge-recursive calls\nadd_cacheinfo() for all entries it needs to update, which deletes the\nold cache entry and just makes new ones, we get that for free.\n\nAnd conversely, if a file-level merge succeeds without conflicts then\nit clearly doesn't \"need to materialize a working tree file\", so it\nshould NOT clear the skip_worktree flag for that path.\n(Unfortunately, that means we have to work around add_cacheinfo() for\nthese cases.)\n"},{"id":"353197","messageId":"xmqqbmb1a7ga.fsf@gitster-ct.c.googlers.com","threadId":"48927","inReplyTo":"CABPp-BE_OHxMqmhJ8BisG6HFpGQjsPJ1AE4G9m5bpJWznkq-1Q@mail.gmail.com","subject":"Re: [BUG] merge-recursive overly aggressive when skipping updating the working tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-20T22:05:57Z","receivedAt":"2018-07-20T22:06:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> Ah, okay, that's helpful. So, if there are conflicts, it should be\n> free to clear the skip_worktree flag. Since merge-recursive calls\n> add_cacheinfo() for all entries it needs to update, which deletes the\n> old cache entry and just makes new ones, we get that for free.\n\nCorrect.\n\n> And conversely, if a file-level merge succeeds without conflicts then\n> it clearly doesn't \"need to materialize a working tree file\", so it\n> should NOT clear the skip_worktree flag for that path.\n\nThat is not at all implied by what I wrote, though.\n\nIf it can be done without too much effort, then it certainly is\nnicer to keep the sparseness when we do not have to materialize the\nworking tree file. But at least in my mind, if it needs too many\nspecial cases, hacks, and conditionals, then it is not worth the\ncomplexity---if it is easier to write a correct code by allowing Git\nto populate working tree files, it is perfectly fine to do so. \n\nIn a sense, the sparse checkout \"feature\" itself is a hack by\nitself, and that is why I think this part should be \"best effort\" as\nwell.\n"},{"id":"353206","messageId":"CABPp-BGVtP4-_LYh7SjSSoTwcm+ZiKbCRSuK0MCo_wvUWBj3vg@mail.gmail.com","threadId":"48927","inReplyTo":"xmqqbmb1a7ga.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] merge-recursive overly aggressive when skipping updating the working tree","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-20T23:02:01Z","receivedAt":"2018-07-20T23:02:04Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 20, 2018 at 3:05 PM, Junio C Hamano wrote:\n> Elijah Newren writes:\n>\n>> Ah, okay, that's helpful. So, if there are conflicts, it should be\n>> free to clear the skip_worktree flag. Since merge-recursive calls\n>> add_cacheinfo() for all entries it needs to update, which deletes the\n>> old cache entry and just makes new ones, we get that for free.\n>\n> Correct.\n>\n>> And conversely, if a file-level merge succeeds without conflicts then\n>> it clearly doesn't \"need to materialize a working tree file\", so it\n>> should NOT clear the skip_worktree flag for that path.\n>\n> That is not at all implied by what I wrote, though.\n>\n> If it can be done without too much effort, then it certainly is\n> nicer to keep the sparseness when we do not have to materialize the\n> working tree file. But at least in my mind, if it needs too many\n> special cases, hacks, and conditionals, then it is not worth the\n> complexity---if it is easier to write a correct code by allowing Git\n> to populate working tree files, it is perfectly fine to do so.\n>\n> In a sense, the sparse checkout \"feature\" itself is a hack by\n> itself, and that is why I think this part should be \"best effort\" as\n> well.\n\nThat's good to know, but I don't think we can back out easily:\n - Clearing the skip_worktree bit: no big deal, as you mention above\n - Avoiding working tree updates when merge doesn't change them: very\ndesirable[1]\n - Doing both: whoops\n\n[1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/\n\n\nI don't want to regress the bug Linus reported, so to fix Ben's issue,\nwhen we detect that a path's contents/mode won't be modified by the\nmerge, we can either:\n - Update the working tree file if the original cache entry had the\nskip_worktree flag set\n - Mark the new cache entry as skip_worktree if the original cache\nentry had the skip_worktree flag set\n\nBoth should be about the same amount of work; the first seems weird\nand confusing for future readers of the code. The second makes sense,\nbut probably should be accompanied with a note in the code about how\nthere are other codepaths that could consider skip_worktree too.\n\nI'll see if I can put something together, but I have family flying in\ntomorrow, and then am out on vacation Mon-Sat next week, so...\n"},{"id":"353215","messageId":"20180721063428.20518-1-newren@gmail.com","threadId":"48927","inReplyTo":"5a8d1098-b4c5-64e1-da98-dac13521e7ba@gmail.com","subject":"[PATCH 0/2] Preserve skip_worktree bit in merges when necessary","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-21T06:34:26Z","receivedAt":"2018-07-21T06:34:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"merge-recursive used to update files in the working tree\nunnecessarily. This was reported by Linus in the 2.18.0 cycle and\nfixed in commit 1de70dbd1 (\"merge-recursive: fix check for skipability\nof working tree updates\", 2018-04-19). Unfortunately, this bug masked\nanother one: that merge-recursive cleared the skip_worktree bit for any\nfiles marked as unmerged by unpack_trees(), even if a file-level merge\nwas clean.\n\nThis series fixes the clearing of the skip_worktree bit for files that\nmerge cleanly and match HEAD. A future possible improvement exists to\nalso avoid clearing the skip_worktree bit for files that merge cleanly\nbut do not match HEAD (for such cases we'd still want to write those\nfiles to the index, but stop updating them in the working tree).\n\nThis series applies cleanly to either maint or master (or next or pu).\n\nTwo important notes:\n - Need a sign-off from Ben for the first patch\n - I'm out on vacation next week, so I won't be able to respond to\n feedback or handle any necessary re-rolls until I return.\n\nBen Peart (1):\n t3507: add a testcase showing failure with sparse checkout\n\nElijah Newren (1):\n merge-recursive: preserve skip_worktree bit when necessary\n\n merge-recursive.c | 16 ++++++++++++++++\n t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++\n 2 files changed, 29 insertions(+)\n\n-- \n2.18.0.234.g2d1e6cefb\n"},{"id":"353216","messageId":"20180721063428.20518-2-newren@gmail.com","threadId":"48927","inReplyTo":"20180721063428.20518-1-newren@gmail.com","subject":"[PATCH 1/2] t3507: add a testcase showing failure with sparse checkout","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-21T06:34:27Z","receivedAt":"2018-07-21T06:34:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Ben Peart \n\nSigned-off-by: Elijah Newren \n---\nTestcase provided by Ben, so committing with him as the author. Just need\na sign off from him.\n\n t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 7c5ad0862..25fac490d 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'failed cherry-pick with sparse-checkout' '\n+ pristine_detach initial &&\n+ git config core.sparseCheckout true &&\n+ echo /unrelated >.git/info/sparse-checkout &&\n+ git read-tree --reset -u HEAD &&\n+ test_must_fail git cherry-pick -Xours picked>actual &&\n+ test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n+ echo \"/*\" >.git/info/sparse-checkout &&\n+ git read-tree --reset -u HEAD &&\n+ git config core.sparseCheckout false &&\n+ rm .git/info/sparse-checkout\n+'\n+\n test_done\n-- \n2.18.0.234.g2d1e6cefb\n\n"},{"id":"353217","messageId":"20180721063428.20518-3-newren@gmail.com","threadId":"48927","inReplyTo":"20180721063428.20518-1-newren@gmail.com","subject":"[PATCH 2/2] merge-recursive: preserve skip_worktree bit when necessary","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-21T06:34:28Z","receivedAt":"2018-07-21T06:34:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"merge-recursive takes any files marked as unmerged by unpack_trees,\ntries to figure out whether they can be resolved (e.g. using renames\nor a file-level merge), and then if they can be it will delete the old\ncache entries and writes new ones. This means that any ce_flags for\nthose cache entries are essentially cleared when merging.\n\nUnfortunately, if a file was marked as skip_worktree and it needs a\nfile-level merge but the merge results in the same version of the file\nthat was found in HEAD, we skip updating the worktree (because the\nfile was unchanged) but clear the skip_worktree bit (because of the\ndelete-cache-entry-and-write-new-one). This makes git treat the file\nas having a local change in the working copy, namely a delete, when it\nshould appear as unchanged despite not being present. Avoid this\nproblem by copying the skip_worktree flag in this case.\n\nSigned-off-by: Elijah Newren \n---\nNo need to check whether pos >= 0 in this patch because the fact that\nwe got to this point in the code meant the entry was definitely in\nboth the new and old indexes (and with the same oid and mode).\n\nWe could optimize this a bit; the call to was_tracked_and_matches()\nalready does the lookup in o->orig_index. So we could cache that and\nre-use it. Likewise, if we instead set ce_flags just after calling\nmake_cache_entry() within add_cacheinfo(), we could avoid looking up\nthe cache entry in the_index as well. Setting ce_flags there would\njust require plumbing an extra flag option through add_cacheinfo() and\nmodifying all other callsites to pass 0 for that flag. But doing all\nthis felt a little messy, and I really wanted to keep the logic for\nthis case all in one little place. Especially for a fixup that might\nbe wanted for maint.\n\nThere is also another callsite in update_file_flags() that could be\nupdated to preserve the skip_worktree flag, which would be technically\nbetter. But I really don't want to tackle that right now, I just want\na small simple fix for Ben's issue.\n\nBesides, as Junio said:\n\n \"If it can be done without too much effort, then it certainly is\n nicer to keep the sparseness when we do not have to materialize the\n working tree file. But at least in my mind, if it needs too many\n special cases, hacks, and conditionals, then it is not worth the\n complexity\"\n\n merge-recursive.c | 16 ++++++++++++++++\n t/t3507-cherry-pick-conflict.sh | 2 +-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 113c1d696..fd74bca17 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3069,10 +3069,26 @@ static int merge_content(struct merge_options *o,\n \tif (mfi.clean &&\n \t was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) &&\n \t !df_conflict_remains) {\n+\t\tint pos;\n+\t\tstruct cache_entry *ce;\n+\n \t\toutput(o, 3, _(\"Skipped %s (merged same as existing)\"), path);\n \t\tif (add_cacheinfo(o, mfi.mode, &mfi.oid, path,\n \t\t\t\t 0, (!o->call_depth && !is_dirty), 0))\n \t\t\treturn -1;\n+\t\t/*\n+\t\t * However, add_cacheinfo() will delete the old cache entry\n+\t\t * and add a new one. We need to copy over any skip_worktree\n+\t\t * flag to avoid making the file appear as if it were\n+\t\t * deleted by the user.\n+\t\t */\n+\t\tpos = index_name_pos(&o->orig_index, path, strlen(path));\n+\t\tce = o->orig_index.cache[pos];\n+\t\tif (ce_skip_worktree(ce)) {\n+\t\t\tpos = index_name_pos(&the_index, path, strlen(path));\n+\t\t\tce = the_index.cache[pos];\n+\t\t\tce->ce_flags |= CE_SKIP_WORKTREE;\n+\t\t}\n \t\treturn mfi.clean;\n \t}\n \ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 25fac490d..9b1456a7c 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -392,7 +392,7 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'failed cherry-pick with sparse-checkout' '\n+test_expect_success 'failed cherry-pick with sparse-checkout' '\n pristine_detach initial &&\n git config core.sparseCheckout true &&\n echo /unrelated >.git/info/sparse-checkout &&\n-- \n2.18.0.234.g2d1e6cefb\n\n"},{"id":"353221","messageId":"CAPig+cTjQ74gOt8gJbYUBaTRsHBvqtcmucoVdiudFt2TrhCn+g@mail.gmail.com","threadId":"48927","inReplyTo":"20180721063428.20518-2-newren@gmail.com","subject":"Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-21T07:21:34Z","receivedAt":"2018-07-21T07:21:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren wrote:\n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> @@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n> +test_expect_failure 'failed cherry-pick with sparse-checkout' '\n> + pristine_detach initial &&\n> + git config core.sparseCheckout true &&\n\nShould this be test_config()?\n\n> + echo /unrelated >.git/info/sparse-checkout &&\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git cherry-pick -Xours picked>actual &&\n> + test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n> + echo \"/*\" >.git/info/sparse-checkout &&\n> + git read-tree --reset -u HEAD &&\n> + git config core.sparseCheckout false &&\n\nSee question above.\n\n> + rm .git/info/sparse-checkout\n\nShould this cleanup be done by test_when_finished()?\n\n> +'\n"},{"id":"353249","messageId":"5250481c-f6a1-0784-28e7-f34a9d6d3c93@gmail.com","threadId":"48927","inReplyTo":"20180721063428.20518-2-newren@gmail.com","subject":"Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-21T13:02:47Z","receivedAt":"2018-07-21T13:02:52Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/21/2018 2:34 AM, Elijah Newren wrote:\n> From: Ben Peart \n> \n> Signed-off-by: Elijah Newren \n> ---\n> Testcase provided by Ben, so committing with him as the author. Just need\n> a sign off from him.\n\nThanks Elijah, consider it\n\nSigned-off-by: Ben Peart \n\n\n> \n> t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++\n> 1 file changed, 13 insertions(+)\n> \n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 7c5ad0862..25fac490d 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_failure 'failed cherry-pick with sparse-checkout' '\n> + pristine_detach initial &&\n> + git config core.sparseCheckout true &&\n> + echo /unrelated >.git/info/sparse-checkout &&\n> + git read-tree --reset -u HEAD &&\n> + test_must_fail git cherry-pick -Xours picked>actual &&\n> + test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n> + echo \"/*\" >.git/info/sparse-checkout &&\n> + git read-tree --reset -u HEAD &&\n> + git config core.sparseCheckout false &&\n> + rm .git/info/sparse-checkout\n> +'\n> +\n> test_done\n> \n\n"},{"id":"353334","messageId":"906db47d-e7ad-56ab-286d-094b0eaf9f01@gmail.com","threadId":"48927","inReplyTo":"CABPp-BGVtP4-_LYh7SjSSoTwcm+ZiKbCRSuK0MCo_wvUWBj3vg@mail.gmail.com","subject":"Re: [BUG] merge-recursive overly aggressive when skipping updating the working tree","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-23T12:49:08Z","receivedAt":"2018-07-23T12:49:12Z","isPatch":false,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/20/2018 7:02 PM, Elijah Newren wrote:\n> On Fri, Jul 20, 2018 at 3:05 PM, Junio C Hamano wrote:\n>> Elijah Newren writes:\n>>\n>>> Ah, okay, that's helpful. So, if there are conflicts, it should be\n>>> free to clear the skip_worktree flag. Since merge-recursive calls\n>>> add_cacheinfo() for all entries it needs to update, which deletes the\n>>> old cache entry and just makes new ones, we get that for free.\n>>\n>> Correct.\n>>\n>>> And conversely, if a file-level merge succeeds without conflicts then\n>>> it clearly doesn't \"need to materialize a working tree file\", so it\n>>> should NOT clear the skip_worktree flag for that path.\n>>\n>> That is not at all implied by what I wrote, though.\n>>\n>> If it can be done without too much effort, then it certainly is\n>> nicer to keep the sparseness when we do not have to materialize the\n>> working tree file. But at least in my mind, if it needs too many\n>> special cases, hacks, and conditionals, then it is not worth the\n>> complexity---if it is easier to write a correct code by allowing Git\n>> to populate working tree files, it is perfectly fine to do so.\n>>\n>> In a sense, the sparse checkout \"feature\" itself is a hack by\n>> itself, and that is why I think this part should be \"best effort\" as\n>> well.\n> \n> That's good to know, but I don't think we can back out easily:\n> - Clearing the skip_worktree bit: no big deal, as you mention above\n> - Avoiding working tree updates when merge doesn't change them: very\n> desirable[1]\n> - Doing both: whoops\n> \n> [1] https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fXJRkvU1m_RHBG54NOoaZPA@mail.gmail.com/\n> \n> \n> I don't want to regress the bug Linus reported, so to fix Ben's issue,\n> when we detect that a path's contents/mode won't be modified by the\n> merge, we can either:\n> - Update the working tree file if the original cache entry had the\n> skip_worktree flag set\n> - Mark the new cache entry as skip_worktree if the original cache\n> entry had the skip_worktree flag set\n> \n> Both should be about the same amount of work; the first seems weird\n> and confusing for future readers of the code. The second makes sense,\n> but probably should be accompanied with a note in the code about how\n> there are other codepaths that could consider skip_worktree too.\n> \n> I'll see if I can put something together, but I have family flying in\n> tomorrow, and then am out on vacation Mon-Sat next week, so...\n> \n\nI agree with the priorities around proposed behavior with this scenario.\n\nIt would be preferred that the skip worktree bit be preserved but the \nbehavior in 2.17 of clearing it and writing the file to the working \ndirectory is much better than the current 2.18 behavior that makes the \nuser think they had somehow deleted the file just by doing the merge.\n\nAt this point, it isn't clear to the user what they should do to recover \nwithout causing harm to the repo.\n\n$ git status\nHEAD detached at df2a63d\nYou are currently cherry-picking commit 7e6d412.\n (all conflicts fixed: run \"git cherry-pick --continue\")\n (use \"git cherry-pick --abort\" to cancel the cherry-pick operation)\n\nChanges not staged for commit:\n (use \"git add/rm ...\" to update what will be committed)\n (use \"git checkout -- ...\" to discard changes in working directory)\n\n deleted: foo\n\n"},{"id":"353341","messageId":"b56598a1-e543-500a-a39b-cee07fe1e533@gmail.com","threadId":"48927","inReplyTo":"CAPig+cTjQ74gOt8gJbYUBaTRsHBvqtcmucoVdiudFt2TrhCn+g@mail.gmail.com","subject":"Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-23T13:12:51Z","receivedAt":"2018-07-23T13:12:55Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/21/2018 3:21 AM, Eric Sunshine wrote:\n> On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren wrote:\n>> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n>> @@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n>> +test_expect_failure 'failed cherry-pick with sparse-checkout' '\n>> + pristine_detach initial &&\n>> + git config core.sparseCheckout true &&\n> \n> Should this be test_config()?\n> \n\nI think using test_config() here is fine but...\n\n>> + echo /unrelated >.git/info/sparse-checkout &&\n>> + git read-tree --reset -u HEAD &&\n>> + test_must_fail git cherry-pick -Xours picked>actual &&\n>> + test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n>> + echo \"/*\" >.git/info/sparse-checkout &&\n>> + git read-tree --reset -u HEAD &&\n>> + git config core.sparseCheckout false &&\n> \n> See question above.\n> \n>> + rm .git/info/sparse-checkout\n> \n> Should this cleanup be done by test_when_finished()?\n> \n\nI think trying to use test_when_finished() for this really degrades the \nreadability of the test. See below:\n\ntest_expect_success 'failed cherry-pick with sparse-checkout' '\n\tpristine_detach initial &&\n\ttest_config core.sparsecheckout true &&\n\techo /unrelated >.git/info/sparse-checkout &&\n\tgit read-tree --reset -u HEAD &&\n\ttest_when_finished \"echo \\\"/*\\\" >.git/info/sparse-checkout && git \nread-tree --reset -u HEAD && rm .git/info/sparse-checkout\" &&\n\ttest_must_fail git cherry-pick -Xours picked>actual &&\n\ttest_i18ngrep ! \"Changes not staged for commit:\" actual\n'\n\nGiven it takes multiple commands, I'd prefer to keep the setup and \ncleanup of the sparse checkout settings symmetrical.\n"},{"id":"353363","messageId":"75aa297e-4857-d92a-7041-618ff3b0b77a@gmail.com","threadId":"48927","inReplyTo":"20180721063428.20518-3-newren@gmail.com","subject":"Re: [PATCH 2/2] merge-recursive: preserve skip_worktree bit when necessary","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-23T14:14:40Z","receivedAt":"2018-07-23T14:14:44Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\n\n> merge-recursive.c | 16 ++++++++++++++++\n> t/t3507-cherry-pick-conflict.sh | 2 +-\n> 2 files changed, 17 insertions(+), 1 deletion(-)\n> \n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 113c1d696..fd74bca17 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -3069,10 +3069,26 @@ static int merge_content(struct merge_options *o,\n> \tif (mfi.clean &&\n> \t was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) &&\n> \t !df_conflict_remains) {\n> +\t\tint pos;\n> +\t\tstruct cache_entry *ce;\n> +\n> \t\toutput(o, 3, _(\"Skipped %s (merged same as existing)\"), path);\n> \t\tif (add_cacheinfo(o, mfi.mode, &mfi.oid, path,\n> \t\t\t\t 0, (!o->call_depth && !is_dirty), 0))\n> \t\t\treturn -1;\n> +\t\t/*\n> +\t\t * However, add_cacheinfo() will delete the old cache entry\n> +\t\t * and add a new one. We need to copy over any skip_worktree\n> +\t\t * flag to avoid making the file appear as if it were\n> +\t\t * deleted by the user.\n> +\t\t */\n\nnit - I find it a little odd to start a comment with \"However\" as if you \nare continuing a conversation.\n\n> +\t\tpos = index_name_pos(&o->orig_index, path, strlen(path));\n> +\t\tce = o->orig_index.cache[pos];\n> +\t\tif (ce_skip_worktree(ce)) {\n> +\t\t\tpos = index_name_pos(&the_index, path, strlen(path));\n> +\t\t\tce = the_index.cache[pos];\n> +\t\t\tce->ce_flags |= CE_SKIP_WORKTREE;\n> +\t\t}\n> \t\treturn mfi.clean;\n> \t}\n> \n> diff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\n> index 25fac490d..9b1456a7c 100755\n> --- a/t/t3507-cherry-pick-conflict.sh\n> +++ b/t/t3507-cherry-pick-conflict.sh\n> @@ -392,7 +392,7 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n> \ttest_cmp expect actual\n> '\n> \n> -test_expect_failure 'failed cherry-pick with sparse-checkout' '\n> +test_expect_success 'failed cherry-pick with sparse-checkout' '\n> pristine_detach initial &&\n> git config core.sparseCheckout true &&\n> echo /unrelated >.git/info/sparse-checkout &&\n> \n\nThanks Elijah, I can verify this fixes the problem with the preferred \nsolution (ie it preserves the skip-worktree bit). As such, this is:\n\nReviewed-by: Ben Peart \n\nThat said, I would propose the test be updated to include a specific \ntest for the skip-worktree bit so that if a future patch reverts to the \nold behavior of clearing the skip-worktree bit and writing out the file \nto the working directory, we do it explicitly instead of by accident.\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh \nb/t/t3507-cherry-pick-conflict.sh\nindex 9b1456a7c3..bc8863ff36 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -392,17 +392,17 @@ test_expect_success 'commit --amend -s places the \nsign-off at the right place' '\n\n test_cmp expect actual\n '\n\n-test_expect_success 'failed cherry-pick with sparse-checkout' '\n+test_expect_success 'cherry-pick preserves sparse-checkout' '\n pristine_detach initial &&\n git config core.sparseCheckout true &&\n echo /unrelated >.git/info/sparse-checkout &&\n git read-tree --reset -u HEAD &&\n test_must_fail git cherry-pick -Xours picked>actual &&\n+ test \"$(git ls-files -t foo)\" = \"S foo\" &&\n test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n echo \"/*\" >.git/info/sparse-checkout &&\n git read-tree --reset -u HEAD &&\n git config core.sparseCheckout false &&\n rm .git/info/sparse-checkout\n '\n\n test_done\n\n"},{"id":"353385","messageId":"CAPig+cR3ic5OAAhNNaSu3YXMnpm=zw6XjspYSU4jouH6po05gg@mail.gmail.com","threadId":"48927","inReplyTo":"b56598a1-e543-500a-a39b-cee07fe1e533@gmail.com","subject":"Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-07-23T18:09:36Z","receivedAt":"2018-07-23T18:09:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jul 23, 2018 at 9:12 AM Ben Peart wrote:\n> On 7/21/2018 3:21 AM, Eric Sunshine wrote:\n> > On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren wrote:\n> >> + rm .git/info/sparse-checkout\n> >\n> > Should this cleanup be done by test_when_finished()?\n>\n> I think trying to use test_when_finished() for this really degrades the\n> readability of the test. See below:\n>\n> test_expect_success 'failed cherry-pick with sparse-checkout' '\n> pristine_detach initial &&\n> test_config core.sparsecheckout true &&\n> echo /unrelated >.git/info/sparse-checkout &&\n> git read-tree --reset -u HEAD &&\n> test_when_finished \"echo \\\"/*\\\" >.git/info/sparse-checkout && git\n> read-tree --reset -u HEAD && rm .git/info/sparse-checkout\" &&\n> test_must_fail git cherry-pick -Xours picked>actual &&\n> test_i18ngrep ! \"Changes not staged for commit:\" actual\n> '\n>\n> Given it takes multiple commands, I'd prefer to keep the setup and\n> cleanup of the sparse checkout settings symmetrical.\n\nSome observations:\n\nThe test_when_finished() ought to be called before the initial\ngit-read-tree, otherwise you risk leaving a .git/info/sparse-checkout\nsitting around if git-read-tree fails.\n\nThe tear-down code could be moved to a function, in which case,\ntest_when_finished() would simply call that function.\n\nMulti-line quoted strings are valid, so you don't need to string out\nall the tear-down steps on a single line like that, and instead spread\nthem across multiple lines to improve readability.\n\ntest_when_finished() doesn't expect just a single quoted string as\nargument. In fact, it can take many (unquoted) arguments, which also\nallows you to spread the tear-down steps over multiple lines to\nimprove readability.\n\nMultiple test_when_finished() invocations are allowed, so you could\nspread out the tear-down commands that way (though they'd have to be\nin reverse order, which would be bad for readability in this case,\nthus not recommended).\n\nCorrectness ought to trump readability, not the other way around.\n\nSo, one possibility, which seems pretty readable to me:\n\n test_expect_failure 'failed cherry-pick with sparse-checkout' '\n pristine_detach initial &&\n test_config core.sparseCheckout true &&\n test_when_finished \"\n echo \\\"/*\\\" >.git/info/sparse-checkout\n git read-tree --reset -u HEAD\n rm .git/info/sparse-checkout\" &&\n echo /unrelated >.git/info/sparse-checkout &&\n git read-tree --reset -u HEAD &&\n test_must_fail git cherry-pick -Xours picked>actual &&\n test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n '\n\nNotice that I dropped the internal &&-chain in test_when_finish() to\nensure that the final 'rm' is invoked even if the cleanup\ngit-read-tree fails (though all bets are probably off, anyhow, if it\ndoes fail).\n"},{"id":"353386","messageId":"xmqq4lgp7res.fsf@gitster-ct.c.googlers.com","threadId":"48927","inReplyTo":"5250481c-f6a1-0784-28e7-f34a9d6d3c93@gmail.com","subject":"Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-23T18:12:11Z","receivedAt":"2018-07-23T18:12:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> On 7/21/2018 2:34 AM, Elijah Newren wrote:\n>> From: Ben Peart \n>>\n>> Signed-off-by: Elijah Newren \n>> ---\n>> Testcase provided by Ben, so committing with him as the author. Just need\n>> a sign off from him.\n>\n> Thanks Elijah, consider it\n>\n> Signed-off-by: Ben Peart \n>\n\nThanks; I'll also tweak the in-body From: line while applying to\nmatch the Sign-off.\n\n"},{"id":"353390","messageId":"795b0bbc-c9d3-f627-4798-80d0e25bfade@gmail.com","threadId":"48927","inReplyTo":"CAPig+cR3ic5OAAhNNaSu3YXMnpm=zw6XjspYSU4jouH6po05gg@mail.gmail.com","subject":"Re: [PATCH 1/2] t3507: add a testcase showing failure with sparse checkout","fromName":"Ben Peart","fromEmail":"peartben@gmail.com","sentAt":"2018-07-23T18:22:07Z","receivedAt":"2018-07-23T18:22:10Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"\n\nOn 7/23/2018 2:09 PM, Eric Sunshine wrote:\n> On Mon, Jul 23, 2018 at 9:12 AM Ben Peart wrote:\n>> On 7/21/2018 3:21 AM, Eric Sunshine wrote:\n>>> On Sat, Jul 21, 2018 at 2:34 AM Elijah Newren wrote:\n>>>> + rm .git/info/sparse-checkout\n>>>\n>>> Should this cleanup be done by test_when_finished()?\n>>\n>> I think trying to use test_when_finished() for this really degrades the\n>> readability of the test. See below:\n>>\n>> test_expect_success 'failed cherry-pick with sparse-checkout' '\n>> pristine_detach initial &&\n>> test_config core.sparsecheckout true &&\n>> echo /unrelated >.git/info/sparse-checkout &&\n>> git read-tree --reset -u HEAD &&\n>> test_when_finished \"echo \\\"/*\\\" >.git/info/sparse-checkout && git\n>> read-tree --reset -u HEAD && rm .git/info/sparse-checkout\" &&\n>> test_must_fail git cherry-pick -Xours picked>actual &&\n>> test_i18ngrep ! \"Changes not staged for commit:\" actual\n>> '\n>>\n>> Given it takes multiple commands, I'd prefer to keep the setup and\n>> cleanup of the sparse checkout settings symmetrical.\n> \n> Some observations:\n> \n> The test_when_finished() ought to be called before the initial\n> git-read-tree, otherwise you risk leaving a .git/info/sparse-checkout\n> sitting around if git-read-tree fails.\n> \n> The tear-down code could be moved to a function, in which case,\n> test_when_finished() would simply call that function.\n> \n> Multi-line quoted strings are valid, so you don't need to string out\n> all the tear-down steps on a single line like that, and instead spread\n> them across multiple lines to improve readability.\n> \n> test_when_finished() doesn't expect just a single quoted string as\n> argument. In fact, it can take many (unquoted) arguments, which also\n> allows you to spread the tear-down steps over multiple lines to\n> improve readability.\n> \n> Multiple test_when_finished() invocations are allowed, so you could\n> spread out the tear-down commands that way (though they'd have to be\n> in reverse order, which would be bad for readability in this case,\n> thus not recommended).\n> \n> Correctness ought to trump readability, not the other way around.\n> \n> So, one possibility, which seems pretty readable to me:\n> \n> test_expect_failure 'failed cherry-pick with sparse-checkout' '\n> pristine_detach initial &&\n> test_config core.sparseCheckout true &&\n> test_when_finished \"\n> echo \\\"/*\\\" >.git/info/sparse-checkout\n> git read-tree --reset -u HEAD\n> rm .git/info/sparse-checkout\" &&\n> echo /unrelated >.git/info/sparse-checkout &&\n> git read-tree --reset -u HEAD &&\n> test_must_fail git cherry-pick -Xours picked>actual &&\n> test_i18ngrep ! \"Changes not staged for commit:\" actual &&\n> '\n> \n\nMinus the trailing && on the last line, that works for me. Thank you - \nreadability and correctness.\n\n> Notice that I dropped the internal &&-chain in test_when_finish() to\n> ensure that the final 'rm' is invoked even if the cleanup\n> git-read-tree fails (though all bets are probably off, anyhow, if it\n> does fail).\n> \n"},{"id":"353701","messageId":"20180727125923.11744-1-benpeart@microsoft.com","threadId":"48927","inReplyTo":"5a8d1098-b4c5-64e1-da98-dac13521e7ba@gmail.com","subject":"[PATCH v2 0/2] Preserve skip_worktree bit in merges when necessary","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-07-27T12:59:41Z","receivedAt":"2018-07-27T12:59:46Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"Sending this update as Elijah is on vacation. This only updates the test\ncase based on feedback from the list.\n\nBase Ref: master\nWeb-Diff: https://github.com/benpeart/git/commit/0ab3816d61\nCheckout: git fetch https://github.com/benpeart/git merge-recursive-v2 && git checkout 0ab3816d61\n\n### Patches\n\nBen Peart (1):\n t3507: add a testcase showing failure with sparse checkout\n\nElijah Newren (1):\n merge-recursive: preserve skip_worktree bit when necessary\n\n merge-recursive.c | 16 ++++++++++++++++\n t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++\n 2 files changed, 29 insertions(+)\n\n\nbase-commit: ffc6fa0e396238de3a30623912980263b4f283ab\n-- \n2.17.0.gvfs.1.123.g449c066\n\n\n"},{"id":"353702","messageId":"20180727125923.11744-2-benpeart@microsoft.com","threadId":"48927","inReplyTo":"20180727125923.11744-1-benpeart@microsoft.com","subject":"[PATCH v2 1/2] t3507: add a testcase showing failure with sparse checkout","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-07-27T12:59:42Z","receivedAt":"2018-07-27T12:59:48Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Ben Peart \n\nRecent changes in merge_content() induced a bug when merging files that are\nnot present in the local working directory due to sparse-checkout. Add a\ntest case to demonstrate the bug so that we can ensure the fix resolves\nit and to prevent future regressions.\n\nSigned-off-by: Ben Peart \nSigned-off-by: Elijah Newren \n---\n t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 7c5ad08626..45ddd81bfa 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -392,4 +392,17 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'cherry-pick preserves sparse-checkout' '\n+\tpristine_detach initial &&\n+\ttest_config core.sparseCheckout true &&\n+\ttest_when_finished \"\n+\t\techo \\\"/*\\\" >.git/info/sparse-checkout\n+\t\tgit read-tree --reset -u HEAD\n+\t\trm .git/info/sparse-checkout\" &&\n+\techo /unrelated >.git/info/sparse-checkout &&\n+\tgit read-tree --reset -u HEAD &&\n+\ttest_must_fail git cherry-pick -Xours picked>actual &&\n+\ttest_i18ngrep ! \"Changes not staged for commit:\" actual\n+'\n+\n test_done\n-- \n2.17.0.gvfs.1.123.g449c066\n\n"},{"id":"353703","messageId":"20180727125923.11744-3-benpeart@microsoft.com","threadId":"48927","inReplyTo":"20180727125923.11744-1-benpeart@microsoft.com","subject":"[PATCH v2 2/2] merge-recursive: preserve skip_worktree bit when necessary","fromName":"Ben Peart","fromEmail":"ben.peart@microsoft.com","sentAt":"2018-07-27T12:59:44Z","receivedAt":"2018-07-27T12:59:50Z","isPatch":true,"sender":{"key":"benpeart@microsoft.com","avatar":"https://avatars.githubusercontent.com/u/15252029?v=4"},"body":"From: Elijah Newren \n\nmerge-recursive takes any files marked as unmerged by unpack_trees,\ntries to figure out whether they can be resolved (e.g. using renames\nor a file-level merge), and then if they can be it will delete the old\ncache entries and writes new ones. This means that any ce_flags for\nthose cache entries are essentially cleared when merging.\n\nUnfortunately, if a file was marked as skip_worktree and it needs a\nfile-level merge but the merge results in the same version of the file\nthat was found in HEAD, we skip updating the worktree (because the\nfile was unchanged) but clear the skip_worktree bit (because of the\ndelete-cache-entry-and-write-new-one). This makes git treat the file\nas having a local change in the working copy, namely a delete, when it\nshould appear as unchanged despite not being present. Avoid this\nproblem by copying the skip_worktree flag in this case.\n\nSigned-off-by: Elijah Newren \n---\n merge-recursive.c | 16 ++++++++++++++++\n t/t3507-cherry-pick-conflict.sh | 2 +-\n 2 files changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 113c1d6962..fd74bca173 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3069,10 +3069,26 @@ static int merge_content(struct merge_options *o,\n \tif (mfi.clean &&\n \t was_tracked_and_matches(o, path, &mfi.oid, mfi.mode) &&\n \t !df_conflict_remains) {\n+\t\tint pos;\n+\t\tstruct cache_entry *ce;\n+\n \t\toutput(o, 3, _(\"Skipped %s (merged same as existing)\"), path);\n \t\tif (add_cacheinfo(o, mfi.mode, &mfi.oid, path,\n \t\t\t\t 0, (!o->call_depth && !is_dirty), 0))\n \t\t\treturn -1;\n+\t\t/*\n+\t\t * However, add_cacheinfo() will delete the old cache entry\n+\t\t * and add a new one. We need to copy over any skip_worktree\n+\t\t * flag to avoid making the file appear as if it were\n+\t\t * deleted by the user.\n+\t\t */\n+\t\tpos = index_name_pos(&o->orig_index, path, strlen(path));\n+\t\tce = o->orig_index.cache[pos];\n+\t\tif (ce_skip_worktree(ce)) {\n+\t\t\tpos = index_name_pos(&the_index, path, strlen(path));\n+\t\t\tce = the_index.cache[pos];\n+\t\t\tce->ce_flags |= CE_SKIP_WORKTREE;\n+\t\t}\n \t\treturn mfi.clean;\n \t}\n \ndiff --git a/t/t3507-cherry-pick-conflict.sh b/t/t3507-cherry-pick-conflict.sh\nindex 45ddd81bfa..0db166152a 100755\n--- a/t/t3507-cherry-pick-conflict.sh\n+++ b/t/t3507-cherry-pick-conflict.sh\n@@ -392,7 +392,7 @@ test_expect_success 'commit --amend -s places the sign-off at the right place' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'cherry-pick preserves sparse-checkout' '\n+test_expect_success 'cherry-pick preserves sparse-checkout' '\n \tpristine_detach initial &&\n \ttest_config core.sparseCheckout true &&\n \ttest_when_finished \"\n-- \n2.17.0.gvfs.1.123.g449c066\n\n"},{"id":"353740","messageId":"xmqqzhycmtq1.fsf@gitster-ct.c.googlers.com","threadId":"48927","inReplyTo":"20180727125923.11744-1-benpeart@microsoft.com","subject":"Re: [PATCH v2 0/2] Preserve skip_worktree bit in merges when necessary","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T18:14:30Z","receivedAt":"2018-07-27T18:14:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Peart writes:\n\n> Sending this update as Elijah is on vacation. This only updates the test\n> case based on feedback from the list.\n>\n> Base Ref: master\n> Web-Diff: https://github.com/benpeart/git/commit/0ab3816d61\n> Checkout: git fetch https://github.com/benpeart/git merge-recursive-v2 && git checkout 0ab3816d61\n\nVery much appreciated. Thanks.\n\n>\n> ### Patches\n>\n> Ben Peart (1):\n> t3507: add a testcase showing failure with sparse checkout\n>\n> Elijah Newren (1):\n> merge-recursive: preserve skip_worktree bit when necessary\n>\n> merge-recursive.c | 16 ++++++++++++++++\n> t/t3507-cherry-pick-conflict.sh | 13 +++++++++++++\n> 2 files changed, 29 insertions(+)\n>\n>\n> base-commit: ffc6fa0e396238de3a30623912980263b4f283ab\n"},{"id":"354054","messageId":"CABPp-BEKboEvz7Yoq+OJnCJJ3JH=XZB=NepVHowCZMa9DogDiA@mail.gmail.com","threadId":"48927","inReplyTo":"20180727125923.11744-1-benpeart@microsoft.com","subject":"Re: [PATCH v2 0/2] Preserve skip_worktree bit in merges when necessary","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-07-31T16:11:01Z","receivedAt":"2018-07-31T16:11:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 27, 2018 at 5:59 AM, Ben Peart wrote:\n> Sending this update as Elijah is on vacation. This only updates the test\n> case based on feedback from the list.\n\nThanks! One less thing for me to catch up on. :-)\n"}]}