{"thread":{"id":"27213","subject":"[PATCH 0/2] merge-one-file and GIT_WORK_TREE","startedAt":"2011-04-28T22:38:22Z","lastAt":"2011-04-29T16:27:47Z","messageCount":6,"participants":["Jeff King","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"166694","messageId":"20110428223822.GA16004@sigill.intra.peff.net","threadId":"27213","inReplyTo":null,"subject":"[PATCH 0/2] merge-one-file and GIT_WORK_TREE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-28T22:38:22Z","receivedAt":"2011-04-28T22:38:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[argh, resend, actually remembering to cc the list this time]\n\nAt GitHub, we're using read-tree, merge-index, and merge-one-file to do\ntrivial content-level merges from bare repositories (without having to\ncheck out the entire repository contents each time).\n\nHowever, we noticed a bug in git-merge-one-file when used with\nGIT_WORK_TREE; it can silently create bogus results that ignore the\n\"theirs\" side of files needing content-level merging.\n\nThe problem is that merge-one-file simply assumes it is in the root of\nthe working tree without any checking. The only two places we use\nmerge-one-file in git itself are in the octopus and resolve strategies.\nI think normal use is fine, because \"git merge\" will have changed to the\ntoplevel of the worktree already.\n\nSo I doubt anybody else is being affected by this. But we do expose the\ncommands for general use, with no disclaimer or check on the working\ntree status or location. And the resulting bogus merge is a nasty\nsurprise.\n\n [1/2]: add tests for merge-index / merge-one-file\n [2/2]: merge-one-file: fix broken merges with GIT_WORK_TREE\n\n-Peff\n"},{"id":"166695","messageId":"20110428223949.GA16027@sigill.intra.peff.net","threadId":"27213","inReplyTo":"20110428223822.GA16004@sigill.intra.peff.net","subject":"[PATCH 1/2] add tests for merge-index / merge-one-file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-28T22:39:49Z","receivedAt":"2011-04-28T22:39:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There were no tests for either, except a brief use in\nt1200-tutorial.\n\nThese tools are not used much these days, as most people\nuse the merge-recursive strategy, which handles everything\ninternally. However, they are used by the \"octopus\" and\n\"resolve\" strategies, as well as any custom strategies\nor merge scripts people have built around them.\n\nFor example, together with read-tree, they are the simplest\nway to do a basic content-level merge without checking out\nthe entire repository contents beforehand.\n\nThis script adds a basic test of the tools to perform one\ncontent-level merge. It also shows a failure of the tools to\nwork properly in the face of GIT_WORK_TREE.\n\nSigned-off-by: Jeff King \n---\nWe could also exercise the more exotic \"both sides added\" code paths of\nmerge-one-file. But anybody who cares about that these days is almost\ncertainly using \"read-tree --aggressive\" anyway.\n\n t/t6060-merge-index.sh | 70 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 70 insertions(+), 0 deletions(-)\n create mode 100755 t/t6060-merge-index.sh\n\ndiff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh\nnew file mode 100755\nindex 0000000..8f8ec77\n--- /dev/null\n+++ b/t/t6060-merge-index.sh\n@@ -0,0 +1,70 @@\n+#!/bin/sh\n+\n+test_description='basic git merge-index / git-merge-one-file tests'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup diverging branches' '\n+\tfor i in 1 2 3 4 5 6 7 8 9 10; do\n+\t\techo $i\n+\tdone >file &&\n+\tgit add file &&\n+\tgit commit -m base &&\n+\tgit tag base &&\n+\tsed s/2/two/ tmp &&\n+\tmv tmp file &&\n+\tgit commit -a -m two &&\n+\tgit tag two &&\n+\tgit checkout -b other HEAD^ &&\n+\tsed s/10/ten/ tmp &&\n+\tmv tmp file &&\n+\tgit commit -a -m ten &&\n+\tgit tag ten\n+'\n+\n+cat >expect-merged <<'EOF'\n+1\n+two\n+3\n+4\n+5\n+6\n+7\n+8\n+9\n+ten\n+EOF\n+\n+test_expect_success 'read-tree does not resolve content merge' '\n+\tgit read-tree -i -m base ten two &&\n+\techo file >expect &&\n+\tgit diff-files --name-only --diff-filter=U >unmerged &&\n+\ttest_cmp expect unmerged\n+'\n+\n+test_expect_success 'git merge-index git-merge-one-file resolves' '\n+\tgit merge-index git-merge-one-file -a &&\n+\tgit diff-files --name-only --diff-filter=U >unmerged &&\n+\t>expect &&\n+\ttest_cmp expect unmerged &&\n+\ttest_cmp expect-merged file &&\n+\tgit cat-file blob :file >file-index &&\n+\ttest_cmp expect-merged file-index\n+'\n+\n+test_expect_failure 'merge-one-file respects GIT_WORK_TREE' '\n+\tgit clone . bare.git &&\n+\t(cd bare.git &&\n+\t mkdir work &&\n+\t GIT_WORK_TREE=$PWD/work &&\n+\t export GIT_WORK_TREE &&\n+\t GIT_INDEX_FILE=$PWD/merge.index &&\n+\t export GIT_INDEX_FILE &&\n+\t git read-tree -i -m base ten two &&\n+\t git merge-index git-merge-one-file -a &&\n+\t git cat-file blob :file >work/file-index\n+\t) &&\n+\ttest_cmp expect-merged bare.git/work/file &&\n+\ttest_cmp expect-merged bare.git/work/file-index\n+'\n+\n+test_done\n-- \n1.7.5.rc3.17.g5e09b\n"},{"id":"166696","messageId":"20110428224125.GB16027@sigill.intra.peff.net","threadId":"27213","inReplyTo":"20110428223822.GA16004@sigill.intra.peff.net","subject":"[PATCH 2/2] merge-one-file: fix broken merges with GIT_WORK_TREE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-28T22:41:25Z","receivedAt":"2011-04-28T22:41:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The merge-one-file tool predates the invention of\nGIT_WORK_TREE. By the time GIT_WORK_TREE was invented, most\npeople were using the merge-recursive strategy, which\nhandles resolving internally. Therefore these features have\nhad very little testing together.\n\nFor the most part, merge-one-file just works with\nGIT_WORK_TREE; most of its heavy lifting is done by plumbing\ncommands which do respect GIT_WORK_TREE properly. The one\nexception is a shell redirection which touches the worktree\ndirectly, writing results to the wrong place in the presence\nof a GIT_WORK_TREE variable.\n\nThis means that merges won't even fail; they will silently\nproduce incorrect results, throwing out the entire \"theirs\"\nside of files which need content-level merging!\n\nThis patch properly prefixes the path in the shell\nredirection. An alternative strategy would be to have\nmerge-one-file use git-sh-setup, and cd_to_toplevel\nbeforehand. I opted for the minimal fix here, as changing\ndirectories would impact where merge-one-file is unpacking\nand working with files.\n\nWhile we're at it, we'll also error-check the call to cat.\nMerging a file in a subdirectory could in fact fail, as the\nredirection relies on the \"checkout-index\" call just prior\nto create leading directories. But we never noticed, since\nwe ignored the error return from running cat.\n\nSigned-off-by: Jeff King \n---\n git-merge-one-file.sh | 4 +++-\n t/t6060-merge-index.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex b86402a..0b5e3f6 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -132,7 +132,9 @@ case \"${1:-.}${2:-.}${3:-.}\" in\n \n \t# Create the working tree file, using \"our tree\" version from the\n \t# index, and then store the result of the merge.\n-\tgit checkout-index -f --stage=2 -- \"$4\" && cat \"$src1\" >\"$4\"\n+\tgit checkout-index -f --stage=2 -- \"$4\" &&\n+\t\tcat \"$src1\" >\"${GIT_WORK_TREE:+$GIT_WORK_TREE/}$4\" ||\n+\t\texit 1\n \trm -f -- \"$orig\" \"$src1\" \"$src2\"\n \n \tif [ \"$6\" != \"$7\" ]; then\ndiff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh\nindex 8f8ec77..fd90e70 100755\n--- a/t/t6060-merge-index.sh\n+++ b/t/t6060-merge-index.sh\n@@ -51,7 +51,7 @@ test_expect_success 'git merge-index git-merge-one-file resolves' '\n \ttest_cmp expect-merged file-index\n '\n \n-test_expect_failure 'merge-one-file respects GIT_WORK_TREE' '\n+test_expect_success 'merge-one-file respects GIT_WORK_TREE' '\n \tgit clone . bare.git &&\n \t(cd bare.git &&\n \t mkdir work &&\n-- \n1.7.5.rc3.17.g5e09b\n"},{"id":"166707","messageId":"4DBA56DF.5070000@viscovery.net","threadId":"27213","inReplyTo":"20110428223949.GA16027@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] add tests for merge-index / merge-one-file","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-04-29T06:12:47Z","receivedAt":"2011-04-29T06:12:47Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/29/2011 0:39, schrieb Jeff King:\n> +\tsed s/2/two/ tmp &&\n\nYou are not using -e; will all seds out there grok this?\n\n-- Hannes\n"},{"id":"166709","messageId":"7vbozp376u.fsf@alter.siamese.dyndns.org","threadId":"27213","inReplyTo":"4DBA56DF.5070000@viscovery.net","subject":"Re: [PATCH 1/2] add tests for merge-index / merge-one-file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-29T06:25:29Z","receivedAt":"2011-04-29T06:25:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Am 4/29/2011 0:39, schrieb Jeff King:\n>> +\tsed s/2/two/ tmp &&\n>\n> You are not using -e; will all seds out there grok this?\n\nThat should be Ok, as long as the script is not ambiguous.\n"},{"id":"166746","messageId":"7v4o5hyqd8.fsf@alter.siamese.dyndns.org","threadId":"27213","inReplyTo":"20110428223822.GA16004@sigill.intra.peff.net","subject":"Re: [PATCH 0/2] merge-one-file and GIT_WORK_TREE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-29T16:27:47Z","receivedAt":"2011-04-29T16:27:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The problem is that merge-one-file simply assumes it is in the root of\n> the working tree without any checking.\n\nIt is kind of surprising that there was only one place that you needed to\ntouch ;-).\n\nThanks.\n"}]}