patch, 2 partsmerge-one-file and GIT_WORK_TREE

[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

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.

Signed-off-by: Jeff King <peff@peff.net>
---
We could also exercise the more exotic "both sides added" code paths of
merge-one-file. But anybody who cares about that these days is almost
certainly using "read-tree --aggressive" anyway.

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

Am 4/29/2011 0:39, schrieb Jeff King:

You are not using -e; will all seds out there grok this?

-- Hannes

Johannes Sixt <j.sixt@viscovery.net> writes:

That should be Ok, as long as the script is not ambiguous.

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(-)

Jeff King <peff@peff.net> writes:

It is kind of surprising that there was only one place that you needed to touch ;-).

Thanks.