{"thread":{"id":"27226","subject":"Re: [PATCH 2/2] merge-one-file: fix broken merges with GIT_WORK_TREE","startedAt":"2011-04-29T18:52:28Z","lastAt":"2011-04-29T22:46:46Z","messageCount":5,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"166772","messageId":"20110429185228.GA27268@sigill.intra.peff.net","threadId":"27226","inReplyTo":"BANLkTimoehqY9ViF7AkQC3YU8e4Sq-OT_w@mail.gmail.com","subject":"Re: [PATCH 2/2] merge-one-file: fix broken merges with GIT_WORK_TREE","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-29T18:52:28Z","receivedAt":"2011-04-29T18:52:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 29, 2011 at 07:19:09PM +0200, Martin von Zweigbergk wrote:\n\n> On Apr 29, 2011 12:41 AM, \"Jeff King\" <peff@peff.net> wrote:\n> >\n> > -       git checkout-index -f --stage=2 -- \"$4\" && cat \"$src1\" >\"$4\"\n> > +       git checkout-index -f --stage=2 -- \"$4\" &&\n> > +               cat \"$src1\" >\"${GIT_WORK_TREE:+$GIT_WORK_TREE/}$4\" ||\n> > +               exit 1\n> \n> Would \"git rev-parse --show-toplevel\" be better here? Does it work with\n> core.worktree etc otherwise? (Maybe git-sh-setup sets the env var?)\n\nGood catch. I believe the git wrapper will convert \"git --work-tree\"\ninto an environment variable, but the config seems to be handled\nseparately.  and either way, I think we technically still support\ncalling \"git-merge-one-file\" directly without the git wrapper.\n\nThinking on it more, it probably makes more sense to \"cd_to_toplevel\"\nvia git-sh-setup. It seems pretty clear to me the original script was\nwritten with the expectation of being at the top of the working tree\n(and that is certainly where it ends up when called via\ngit-merge-octopus and git-merge-resolve).\n\nLet me extend the tests a bit and roll a new series.\n\n-Peff\n"},{"id":"166777","messageId":"20110429222354.GA3347@sigill.intra.peff.net","threadId":"27226","inReplyTo":"20110429185228.GA27268@sigill.intra.peff.net","subject":"[PATCHv2 1/2] add tests for merge-index / merge-one-file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-29T22:23:54Z","receivedAt":"2011-04-29T22:23:54Z","isPatch":false,"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 or core.worktree.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTwo new tests in this version:\n\n  - make sure we properly fail when there is no work tree; we do\n    already, although it is not entirely graceful. But mainly I wanted\n    to make sure I didn't regress on that behavior with the fix in 2/2.\n\n  - check both GIT_WORK_TREE and core.worktree; the latter was still\n    broken in the original series\n\n t/t6060-merge-index.sh |  100 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 100 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..895f079\n--- /dev/null\n+++ b/t/t6060-merge-index.sh\n@@ -0,0 +1,100 @@\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/ <file >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/ <file >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_success 'setup bare merge' '\n+\tgit clone --bare . bare.git &&\n+\t(cd bare.git &&\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)\n+'\n+\n+test_expect_success 'merge-one-file fails without a work tree' '\n+\t(cd bare.git &&\n+\t GIT_INDEX_FILE=$PWD/merge.index &&\n+\t export GIT_INDEX_FILE &&\n+\t test_must_fail git merge-index git-merge-one-file -a\n+\t)\n+'\n+\n+test_expect_failure 'merge-one-file respects GIT_WORK_TREE' '\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 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_expect_failure 'merge-one-file respects core.worktree' '\n+\tmkdir subdir &&\n+\tgit clone . subdir/child &&\n+\t(cd subdir &&\n+\t GIT_DIR=$PWD/child/.git &&\n+\t export GIT_DIR &&\n+\t git config core.worktree \"$PWD/child\" &&\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 >file-index\n+\t) &&\n+\ttest_cmp expect-merged subdir/child/file &&\n+\ttest_cmp expect-merged subdir/file-index\n+'\n+\n+test_done\n-- \n1.7.4.3.28.g10631\n"},{"id":"166778","messageId":"20110429222432.GB3347@sigill.intra.peff.net","threadId":"27226","inReplyTo":"20110429185228.GA27268@sigill.intra.peff.net","subject":"[PATCHv2 2/2] merge-one-file: fix broken merges with alternate work trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-29T22:24:32Z","receivedAt":"2011-04-29T22:24:32Z","isPatch":false,"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 makes merge-one-file chdir to the toplevel of the\nworking tree (and exit if we don't have one). This most\nclosely matches the assumption made by the original script\n(before separate work trees were invented), and matches what\nhappens when the script is called as part of a merge\nstrategy.\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 <peff@peff.net>\n---\nThis one takes a totally different strategy than v1, but I think it\nmakes more sense (and it fixes the core.worktree bug).\n\n git-merge-one-file.sh  |    7 ++++++-\n t/t6060-merge-index.sh |    4 ++--\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/git-merge-one-file.sh b/git-merge-one-file.sh\nindex b86402a..7aeb969 100755\n--- a/git-merge-one-file.sh\n+++ b/git-merge-one-file.sh\n@@ -22,6 +22,11 @@ LONG_USAGE=\"Usage: git merge-one-file $USAGE\n \n Blob ids and modes should be empty for missing files.\"\n \n+SUBDIRECTORY_OK=Yes\n+. git-sh-setup\n+cd_to_toplevel\n+require_work_tree\n+\n if ! test \"$#\" -eq 7\n then\n \techo \"$LONG_USAGE\"\n@@ -132,7 +137,7 @@ 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\" && cat \"$src1\" >\"$4\" || exit 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 895f079..debadbd 100755\n--- a/t/t6060-merge-index.sh\n+++ b/t/t6060-merge-index.sh\n@@ -68,7 +68,7 @@ test_expect_success 'merge-one-file fails without a work tree' '\n \t)\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 \t(cd bare.git &&\n \t mkdir work &&\n \t GIT_WORK_TREE=$PWD/work &&\n@@ -82,7 +82,7 @@ test_expect_failure 'merge-one-file respects GIT_WORK_TREE' '\n \ttest_cmp expect-merged bare.git/work/file-index\n '\n \n-test_expect_failure 'merge-one-file respects core.worktree' '\n+test_expect_success 'merge-one-file respects core.worktree' '\n \tmkdir subdir &&\n \tgit clone . subdir/child &&\n \t(cd subdir &&\n-- \n1.7.4.3.28.g10631\n"},{"id":"166783","messageId":"7vtydgvfx4.fsf@alter.siamese.dyndns.org","threadId":"27226","inReplyTo":"20110429222432.GB3347@sigill.intra.peff.net","subject":"Re: [PATCHv2 2/2] merge-one-file: fix broken merges with alternate work trees","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-04-29T22:41:43Z","receivedAt":"2011-04-29T22:41:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This patch makes merge-one-file chdir to the toplevel of the working\n> tree (and exit if we don't have one). This most closely matches the\n> assumption made by the original script (before separate work trees were\n> invented), and matches what happens when the script is called as part of\n> a merge strategy.\n>\n> While we're at it, we'll also error-check the call to cat.\n> Merging a file in a subdirectory could in fact fail, as the\n> redirection relies on the \"checkout-index\" call just prior\n> to create leading directories. But we never noticed, since\n> we ignored the error return from running cat.\n\nThis part is probably incorrect as we have && before cat that checks\nan error from checkout-index that fails to create such a subdirectory, no?\n\nAnd then \"exec git update-index -- $4\" at the last step would have failed.\n\nOther than that, the patch looks much nicer and more modern.\n\nWill queue.\n"},{"id":"166784","messageId":"20110429224646.GA4095@sigill.intra.peff.net","threadId":"27226","inReplyTo":"7vtydgvfx4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2 2/2] merge-one-file: fix broken merges with alternate work trees","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-29T22:46:46Z","receivedAt":"2011-04-29T22:46:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 29, 2011 at 03:41:43PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > This patch makes merge-one-file chdir to the toplevel of the working\n> > tree (and exit if we don't have one). This most closely matches the\n> > assumption made by the original script (before separate work trees were\n> > invented), and matches what happens when the script is called as part of\n> > a merge strategy.\n> >\n> > While we're at it, we'll also error-check the call to cat.\n> > Merging a file in a subdirectory could in fact fail, as the\n> > redirection relies on the \"checkout-index\" call just prior\n> > to create leading directories. But we never noticed, since\n> > we ignored the error return from running cat.\n> \n> This part is probably incorrect as we have && before cat that checks\n> an error from checkout-index that fails to create such a subdirectory, no?\n\nSort of. In the original code, checkout-index wrote to one spot, and cat\nwrote to another, so they could fail independently, masking the bug.\n\nNow that the code correctly respects GIT_WORK_TREE, that should never\nhappen, and the only reason for cat to fail is something like ENOSPC\n(checkout-index wrote the original version, but we are writing the merge\nresult, which could be larger).\n\n> And then \"exec git update-index -- $4\" at the last step would have failed.\n\nNo. In the old code, it succeeded because it respected GIT_WORK_TREE, so\nit marked the original stage-2 version as the merge result (hence the\nbogus merge results).\n\nIn the new code, if we got ENOSPC, then it would mark the truncated\nmerge result as good (except that we probably would also fail to write a\nnew index due to the same error...).\n\nIt's unlikely, I'll grant, but I don't see a reason not to check the\nerror.\n\n-Peff\n"}]}