Re: [PATCH 2/2] merge-one-file: fix broken merges with GIT_WORK_TREE

Re: [PATCH 2/2] merge-one-file: fix broken merges with GIT_WORK_TREE

[Jeff King]

On Fri, Apr 29, 2011 at 07:19:09PM +0200, Martin von Zweigbergk wrote:

> On Apr 29, 2011 12:41 AM, "Jeff King" <peff@peff.net> wrote:
> >
> > -       git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4"
> > +       git checkout-index -f --stage=2 -- "$4" &&
> > +               cat "$src1" >"${GIT_WORK_TREE:+$GIT_WORK_TREE/}$4" ||
> > +               exit 1
> 
> Would "git rev-parse --show-toplevel" be better here? Does it work with
> core.worktree etc otherwise? (Maybe git-sh-setup sets the env var?)

Good catch. I believe the git wrapper will convert "git --work-tree"
into an environment variable, but the config seems to be handled
separately.  and either way, I think we technically still support
calling "git-merge-one-file" directly without the git wrapper.

Thinking on it more, it probably makes more sense to "cd_to_toplevel"
via git-sh-setup. It seems pretty clear to me the original script was
written with the expectation of being at the top of the working tree
(and that is certainly where it ends up when called via
git-merge-octopus and git-merge-resolve).

Let me extend the tests a bit and roll a new series.

-Peff

[PATCHv2 1/2] add tests for merge-index / merge-one-file

[Jeff King]

There were no tests for either, except a brief use in
t1200-tutorial.

These tools are not used much these days, as most people
use the merge-recursive strategy, which handles everything
internally. However, they are used by the "octopus" and
"resolve" strategies, as well as any custom strategies
or merge scripts people have built around them.

For example, together with read-tree, they are the simplest
way to do a basic content-level merge without checking out
the entire repository contents beforehand.

This script adds a basic test of the tools to perform one
content-level merge. It also shows a failure of the tools to
work properly in the face of GIT_WORK_TREE or core.worktree.

Signed-off-by: Jeff King <peff@peff.net>
---
Two new tests in this version:

  - make sure we properly fail when there is no work tree; we do
    already, although it is not entirely graceful. But mainly I wanted
    to make sure I didn't regress on that behavior with the fix in 2/2.

  - check both GIT_WORK_TREE and core.worktree; the latter was still
    broken in the original series

 t/t6060-merge-index.sh |  100 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 100 insertions(+), 0 deletions(-)
 create mode 100755 t/t6060-merge-index.sh

diff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh
new file mode 100755
index 0000000..895f079
--- /dev/null
+++ b/t/t6060-merge-index.sh
@@ -0,0 +1,100 @@
+#!/bin/sh
+
+test_description='basic git merge-index / git-merge-one-file tests'
+. ./test-lib.sh
+
+test_expect_success 'setup diverging branches' '
+	for i in 1 2 3 4 5 6 7 8 9 10; do
+		echo $i
+	done >file &&
+	git add file &&
+	git commit -m base &&
+	git tag base &&
+	sed s/2/two/ <file >tmp &&
+	mv tmp file &&
+	git commit -a -m two &&
+	git tag two &&
+	git checkout -b other HEAD^ &&
+	sed s/10/ten/ <file >tmp &&
+	mv tmp file &&
+	git commit -a -m ten &&
+	git tag ten
+'
+
+cat >expect-merged <<'EOF'
+1
+two
+3
+4
+5
+6
+7
+8
+9
+ten
+EOF
+
+test_expect_success 'read-tree does not resolve content merge' '
+	git read-tree -i -m base ten two &&
+	echo file >expect &&
+	git diff-files --name-only --diff-filter=U >unmerged &&
+	test_cmp expect unmerged
+'
+
+test_expect_success 'git merge-index git-merge-one-file resolves' '
+	git merge-index git-merge-one-file -a &&
+	git diff-files --name-only --diff-filter=U >unmerged &&
+	>expect &&
+	test_cmp expect unmerged &&
+	test_cmp expect-merged file &&
+	git cat-file blob :file >file-index &&
+	test_cmp expect-merged file-index
+'
+
+test_expect_success 'setup bare merge' '
+	git clone --bare . bare.git &&
+	(cd bare.git &&
+	 GIT_INDEX_FILE=$PWD/merge.index &&
+	 export GIT_INDEX_FILE &&
+	 git read-tree -i -m base ten two
+	)
+'
+
+test_expect_success 'merge-one-file fails without a work tree' '
+	(cd bare.git &&
+	 GIT_INDEX_FILE=$PWD/merge.index &&
+	 export GIT_INDEX_FILE &&
+	 test_must_fail git merge-index git-merge-one-file -a
+	)
+'
+
+test_expect_failure 'merge-one-file respects GIT_WORK_TREE' '
+	(cd bare.git &&
+	 mkdir work &&
+	 GIT_WORK_TREE=$PWD/work &&
+	 export GIT_WORK_TREE &&
+	 GIT_INDEX_FILE=$PWD/merge.index &&
+	 export GIT_INDEX_FILE &&
+	 git merge-index git-merge-one-file -a &&
+	 git cat-file blob :file >work/file-index
+	) &&
+	test_cmp expect-merged bare.git/work/file &&
+	test_cmp expect-merged bare.git/work/file-index
+'
+
+test_expect_failure 'merge-one-file respects core.worktree' '
+	mkdir subdir &&
+	git clone . subdir/child &&
+	(cd subdir &&
+	 GIT_DIR=$PWD/child/.git &&
+	 export GIT_DIR &&
+	 git config core.worktree "$PWD/child" &&
+	 git read-tree -i -m base ten two &&
+	 git merge-index git-merge-one-file -a &&
+	 git cat-file blob :file >file-index
+	) &&
+	test_cmp expect-merged subdir/child/file &&
+	test_cmp expect-merged subdir/file-index
+'
+
+test_done
-- 
1.7.4.3.28.g10631

[PATCHv2 2/2] merge-one-file: fix broken merges with alternate work trees

[Jeff King]

The merge-one-file tool predates the invention of
GIT_WORK_TREE. By the time GIT_WORK_TREE was invented, most
people were using the merge-recursive strategy, which
handles resolving internally. Therefore these features have
had very little testing together.

For the most part, merge-one-file just works with
GIT_WORK_TREE; most of its heavy lifting is done by plumbing
commands which do respect GIT_WORK_TREE properly. The one
exception is a shell redirection which touches the worktree
directly, writing results to the wrong place in the presence
of a GIT_WORK_TREE variable.

This means that merges won't even fail; they will silently
produce incorrect results, throwing out the entire "theirs"
side of files which need content-level merging!

This patch makes merge-one-file chdir to the toplevel of the
working tree (and exit if we don't have one). This most
closely matches the assumption made by the original script
(before separate work trees were invented), and matches what
happens when the script is called as part of a merge
strategy.

While we're at it, we'll also error-check the call to cat.
Merging a file in a subdirectory could in fact fail, as the
redirection relies on the "checkout-index" call just prior
to create leading directories. But we never noticed, since
we ignored the error return from running cat.

Signed-off-by: Jeff King <peff@peff.net>
---
This one takes a totally different strategy than v1, but I think it
makes more sense (and it fixes the core.worktree bug).

 git-merge-one-file.sh  |    7 ++++++-
 t/t6060-merge-index.sh |    4 ++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index b86402a..7aeb969 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -22,6 +22,11 @@ LONG_USAGE="Usage: git merge-one-file $USAGE
 
 Blob ids and modes should be empty for missing files."
 
+SUBDIRECTORY_OK=Yes
+. git-sh-setup
+cd_to_toplevel
+require_work_tree
+
 if ! test "$#" -eq 7
 then
 	echo "$LONG_USAGE"
@@ -132,7 +137,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 
 	# Create the working tree file, using "our tree" version from the
 	# index, and then store the result of the merge.
-	git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4"
+	git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4" || exit 1
 	rm -f -- "$orig" "$src1" "$src2"
 
 	if [ "$6" != "$7" ]; then
diff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh
index 895f079..debadbd 100755
--- a/t/t6060-merge-index.sh
+++ b/t/t6060-merge-index.sh
@@ -68,7 +68,7 @@ test_expect_success 'merge-one-file fails without a work tree' '
 	)
 '
 
-test_expect_failure 'merge-one-file respects GIT_WORK_TREE' '
+test_expect_success 'merge-one-file respects GIT_WORK_TREE' '
 	(cd bare.git &&
 	 mkdir work &&
 	 GIT_WORK_TREE=$PWD/work &&
@@ -82,7 +82,7 @@ test_expect_failure 'merge-one-file respects GIT_WORK_TREE' '
 	test_cmp expect-merged bare.git/work/file-index
 '
 
-test_expect_failure 'merge-one-file respects core.worktree' '
+test_expect_success 'merge-one-file respects core.worktree' '
 	mkdir subdir &&
 	git clone . subdir/child &&
 	(cd subdir &&
-- 
1.7.4.3.28.g10631

Re: [PATCHv2 2/2] merge-one-file: fix broken merges with alternate work trees

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This patch makes merge-one-file chdir to the toplevel of the working
> tree (and exit if we don't have one). This most closely matches the
> assumption made by the original script (before separate work trees were
> invented), and matches what happens when the script is called as part of
> a merge strategy.
>
> While we're at it, we'll also error-check the call to cat.
> Merging a file in a subdirectory could in fact fail, as the
> redirection relies on the "checkout-index" call just prior
> to create leading directories. But we never noticed, since
> we ignored the error return from running cat.

This part is probably incorrect as we have && before cat that checks
an error from checkout-index that fails to create such a subdirectory, no?

And then "exec git update-index -- $4" at the last step would have failed.

Other than that, the patch looks much nicer and more modern.

Will queue.

Re: [PATCHv2 2/2] merge-one-file: fix broken merges with alternate work trees

[Jeff King]

On Fri, Apr 29, 2011 at 03:41:43PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This patch makes merge-one-file chdir to the toplevel of the working
> > tree (and exit if we don't have one). This most closely matches the
> > assumption made by the original script (before separate work trees were
> > invented), and matches what happens when the script is called as part of
> > a merge strategy.
> >
> > While we're at it, we'll also error-check the call to cat.
> > Merging a file in a subdirectory could in fact fail, as the
> > redirection relies on the "checkout-index" call just prior
> > to create leading directories. But we never noticed, since
> > we ignored the error return from running cat.
> 
> This part is probably incorrect as we have && before cat that checks
> an error from checkout-index that fails to create such a subdirectory, no?

Sort of. In the original code, checkout-index wrote to one spot, and cat
wrote to another, so they could fail independently, masking the bug.

Now that the code correctly respects GIT_WORK_TREE, that should never
happen, and the only reason for cat to fail is something like ENOSPC
(checkout-index wrote the original version, but we are writing the merge
result, which could be larger).

> And then "exec git update-index -- $4" at the last step would have failed.

No. In the old code, it succeeded because it respected GIT_WORK_TREE, so
it marked the original stage-2 version as the merge result (hence the
bogus merge results).

In the new code, if we got ENOSPC, then it would mark the truncated
merge result as good (except that we probably would also fail to write a
new index due to the same error...).

It's unlikely, I'll grant, but I don't see a reason not to check the
error.

-Peff

[PATCH 0/2] merge-one-file and GIT_WORK_TREE

[Jeff King]

[argh, resend, actually remembering to cc the list this time]

At GitHub, we're using read-tree, merge-index, and merge-one-file to do
trivial content-level merges from bare repositories (without having to
check out the entire repository contents each time).

However, we noticed a bug in git-merge-one-file when used with
GIT_WORK_TREE; it can silently create bogus results that ignore the
"theirs" side of files needing content-level merging.

The problem is that merge-one-file simply assumes it is in the root of
the working tree without any checking.  The only two places we use
merge-one-file in git itself are in the octopus and resolve strategies.
I think normal use is fine, because "git merge" will have changed to the
toplevel of the worktree already.

So I doubt anybody else is being affected by this. But we do expose the
commands for general use, with no disclaimer or check on the working
tree status or location. And the resulting bogus merge is a nasty
surprise.

  [1/2]: add tests for merge-index / merge-one-file
  [2/2]: merge-one-file: fix broken merges with GIT_WORK_TREE

-Peff

[PATCH 2/2] merge-one-file: fix broken merges with GIT_WORK_TREE

[Jeff King]

The merge-one-file tool predates the invention of
GIT_WORK_TREE. By the time GIT_WORK_TREE was invented, most
people were using the merge-recursive strategy, which
handles resolving internally. Therefore these features have
had very little testing together.

For the most part, merge-one-file just works with
GIT_WORK_TREE; most of its heavy lifting is done by plumbing
commands which do respect GIT_WORK_TREE properly. The one
exception is a shell redirection which touches the worktree
directly, writing results to the wrong place in the presence
of a GIT_WORK_TREE variable.

This means that merges won't even fail; they will silently
produce incorrect results, throwing out the entire "theirs"
side of files which need content-level merging!

This patch properly prefixes the path in the shell
redirection. An alternative strategy would be to have
merge-one-file use git-sh-setup, and cd_to_toplevel
beforehand. I opted for the minimal fix here, as changing
directories would impact where merge-one-file is unpacking
and working with files.

While we're at it, we'll also error-check the call to cat.
Merging a file in a subdirectory could in fact fail, as the
redirection relies on the "checkout-index" call just prior
to create leading directories. But we never noticed, since
we ignored the error return from running cat.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-merge-one-file.sh  |    4 +++-
 t/t6060-merge-index.sh |    2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index b86402a..0b5e3f6 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -132,7 +132,9 @@ case "${1:-.}${2:-.}${3:-.}" in
 
 	# Create the working tree file, using "our tree" version from the
 	# index, and then store the result of the merge.
-	git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4"
+	git checkout-index -f --stage=2 -- "$4" &&
+		cat "$src1" >"${GIT_WORK_TREE:+$GIT_WORK_TREE/}$4" ||
+		exit 1
 	rm -f -- "$orig" "$src1" "$src2"
 
 	if [ "$6" != "$7" ]; then
diff --git a/t/t6060-merge-index.sh b/t/t6060-merge-index.sh
index 8f8ec77..fd90e70 100755
--- a/t/t6060-merge-index.sh
+++ b/t/t6060-merge-index.sh
@@ -51,7 +51,7 @@ test_expect_success 'git merge-index git-merge-one-file resolves' '
 	test_cmp expect-merged file-index
 '
 
-test_expect_failure 'merge-one-file respects GIT_WORK_TREE' '
+test_expect_success 'merge-one-file respects GIT_WORK_TREE' '
 	git clone . bare.git &&
 	(cd bare.git &&
 	 mkdir work &&
-- 
1.7.5.rc3.17.g5e09b