[PATCH] builtin-merge: missing structure bzero.

[PATCH] builtin-merge: missing structure bzero.

[Pierre Habouzit]

This cause segfaults when replacing a directory with a submodule in a
fast-forward.

Adds tests that revealed the issue, even if the second one isn't yet fixed for
another reason.

Signed-off-by: Pierre Habouzit <madcoder@debian.org>
---

    In the process of writing a test for a problem with submodules that quite
    bugs me so that I can fix it, I found a segfault that this patch fix.

    The problem I tried to fix (hence I first wrote a test) is what happens
    when you have in your repository a "dir/file.c" and that you replace
    "dir/" witha submodule that also has a "file.c". git-merge pretends the
    submodule checkout would clobber unversionned files, probably due to a too
    late removal. anyways, that's for a next patch when I will understand the
    root of this issue ;)

 builtin-merge.c            |    1 +
 t/t7403-submodule-merge.sh |   62 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 0 deletions(-)
 create mode 100755 t/t7403-submodule-merge.sh

diff --git a/builtin-merge.c b/builtin-merge.c
index e97c79e..0ed1acf 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -580,6 +580,7 @@ static int checkout_fast_forward(unsigned char *head, unsigned char *remote)
 	memset(&trees, 0, sizeof(trees));
 	memset(&opts, 0, sizeof(opts));
 	memset(&t, 0, sizeof(t));
+	memset(&dir, 0, sizeof(dir));
 	dir.show_ignored = 1;
 	dir.exclude_per_dir = ".gitignore";
 	opts.dir = &dir;
diff --git a/t/t7403-submodule-merge.sh b/t/t7403-submodule-merge.sh
new file mode 100755
index 0000000..a803829
--- /dev/null
+++ b/t/t7403-submodule-merge.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Pierre Habouzit
+#
+
+test_description='Test merging with submodules'
+
+. ./test-lib.sh
+
+test_expect_success 'Setup a repository used as a submodule for other tests' '
+	mkdir submodule &&
+	cd submodule &&
+	git init &&
+	echo a > a &&
+	git add a &&
+	git commit -asm"submodule init" &&
+	cd ..
+'
+
+test_expect_success 'Replace a directory with a submodule, no file conflict' '
+	mkdir test &&
+	cd test &&
+	: create our repository with a sub/b file &&
+	git init &&
+	mkdir sub && echo b > sub/b &&
+	git add sub && git commit -asm"initial repository" &&
+	: save this state in a new branch &&
+	git branch temp &&
+	: then replace sub with it &&
+	git rm -rf sub &&
+        git submodule add -- "$(pwd)/../submodule/.git/" sub &&
+	git commit -asm "replace sub/ with a submodule" &&
+	: then try to update the "temp" branch &&
+	git checkout temp &&
+	git merge master &&
+	: and finally cleanse the mess &&
+	cd .. &&
+	rm -rf test
+'
+
+test_expect_failure 'Replace a directory with a submodule, with a file conflict' '
+	mkdir test &&
+	cd test &&
+	: create our repository with a sub/a file &&
+	git init &&
+	mkdir sub && echo a > sub/a &&
+	git add sub && git commit -asm"initial repository" &&
+	: save this state in a new branch &&
+	git branch temp &&
+	: then replace sub with it &&
+	git rm -rf sub &&
+        git submodule add -- "$(pwd)/../submodule/.git/" sub &&
+	git commit -asm "replace sub/ with a submodule" &&
+	: then try to update the "temp" branch &&
+	git checkout temp &&
+	git merge master &&
+	: and finally cleanse the mess &&
+	cd .. &&
+	rm -rf test
+'
+
+test_done
-- 
1.5.6.3

Re: [PATCH] builtin-merge: missing structure bzero.

[Miklos Vajna]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

On Mon, Jul 21, 2008 at 07:03:50PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
> This cause segfaults when replacing a directory with a submodule in a
> fast-forward.

Thanks.

> +test_expect_failure 'Replace a directory with a submodule, with a file conflict' '
> +	mkdir test &&
> +	cd test &&
> +	: create our repository with a sub/a file &&
> +	git init &&
> +	mkdir sub && echo a > sub/a &&
> +	git add sub && git commit -asm"initial repository" &&
> +	: save this state in a new branch &&
> +	git branch temp &&
> +	: then replace sub with it &&
> +	git rm -rf sub &&
> +        git submodule add -- "$(pwd)/../submodule/.git/" sub &&
> +	git commit -asm "replace sub/ with a submodule" &&
> +	: then try to update the "temp" branch &&
> +	git checkout temp &&

It seems this one fails. I guess this will be a problem in the low-level
merge code (read-tree -m) and not in builtin-merge.

> +	git merge master &&
> +	: and finally cleanse the mess &&
> +	cd .. &&
> +	rm -rf test
> +'
> +
> +test_done

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] builtin-merge: missing structure bzero.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

On Mon, Jul 21, 2008 at 05:21:19PM +0000, Miklos Vajna wrote:
> On Mon, Jul 21, 2008 at 07:03:50PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
> > This cause segfaults when replacing a directory with a submodule in a
> > fast-forward.
> 
> Thanks.
> 
> > +test_expect_failure 'Replace a directory with a submodule, with a file conflict' '
> > +	mkdir test &&
> > +	cd test &&
> > +	: create our repository with a sub/a file &&
> > +	git init &&
> > +	mkdir sub && echo a > sub/a &&
> > +	git add sub && git commit -asm"initial repository" &&
> > +	: save this state in a new branch &&
> > +	git branch temp &&
> > +	: then replace sub with it &&
> > +	git rm -rf sub &&
> > +        git submodule add -- "$(pwd)/../submodule/.git/" sub &&
> > +	git commit -asm "replace sub/ with a submodule" &&
> > +	: then try to update the "temp" branch &&
> > +	git checkout temp &&
> 
> It seems this one fails. I guess this will be a problem in the low-level
> merge code (read-tree -m) and not in builtin-merge.

  Yeah, I saw that afterwards, the error was misleading (as it tells
about some "merge" issue), but when I tried to debug it, it was indeed
in git checkout. The easiest way to reproduce, is to have a submodule
that replace a file that was previously versionned (which is something
that will happen in real life when you take out a subdirectory of a
project to make it live into a submodule) and that you then git checkout
HEAD~1.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] builtin-merge: missing structure bzero.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 4378 bytes --]

On Mon, Jul 21, 2008 at 06:18:50PM +0000, Pierre Habouzit wrote:
> On Mon, Jul 21, 2008 at 05:21:19PM +0000, Miklos Vajna wrote:
> > On Mon, Jul 21, 2008 at 07:03:50PM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
> > > This cause segfaults when replacing a directory with a submodule in a
> > > fast-forward.
> > 
> > Thanks.
> > 
> > > +test_expect_failure 'Replace a directory with a submodule, with a file conflict' '
> > > +	mkdir test &&
> > > +	cd test &&
> > > +	: create our repository with a sub/a file &&
> > > +	git init &&
> > > +	mkdir sub && echo a > sub/a &&
> > > +	git add sub && git commit -asm"initial repository" &&
> > > +	: save this state in a new branch &&
> > > +	git branch temp &&
> > > +	: then replace sub with it &&
> > > +	git rm -rf sub &&
> > > +        git submodule add -- "$(pwd)/../submodule/.git/" sub &&
> > > +	git commit -asm "replace sub/ with a submodule" &&
> > > +	: then try to update the "temp" branch &&
> > > +	git checkout temp &&
> > 
> > It seems this one fails. I guess this will be a problem in the low-level
> > merge code (read-tree -m) and not in builtin-merge.
> 
>   Yeah, I saw that afterwards, the error was misleading (as it tells
> about some "merge" issue), but when I tried to debug it, it was indeed
> in git checkout. The easiest way to reproduce, is to have a submodule
> that replace a file that was previously versionned (which is something
> that will happen in real life when you take out a subdirectory of a
> project to make it live into a submodule) and that you then git checkout
> HEAD~1.

Actually, the issue is quite obvious when thinking. At git-checkout
time, git doesn't recurse (yet) into submodules, hence when we checkout
'temp' that is basically master~1 that basically does that:

"sub/" is a submodule, and gets replaced by sub/ a tree with "a" in it,
and the submodule _also_ has "a". As submodules are ignored silently, it
just:

  (1) forgets about the submodule at once, leaving all the submodules
      files into the wild, and sub/a (that was in the submodule) is now
      an untracked file.

  (2) tries to checkout sub/a and sees a file currently untracked and
      barfs.

So it boils down to the fact that git-checkout does nothing with
submodules. I really think we should address that, it's a major issue
wrt submodules and usability.


I believe we should for now require that all submodules are clean for
checkout to be able to work, and then we have those cases:

  * submodule -> directory: ignore any file that is either versionned or
    ignored in the submodule wrt checkout issues. If no issue arises
    with untracked non ignored files in the submodule, then basically:
    (1) in the submodule: git ls-files | xargs rm -f ; rm -rf .git 
    (2) proceed with the checkout.

  * submodule -> blob: refuse to proceed if there is any untracked non
    ignored file in the submodule, else just rm -rf it, and proceed.

  * submodule h1 -> h2: if "git checkout h2" will work in the submodule,
    then we have no problem with the checkout, and do proceed. Though,
    if the checkout isn't possible because "h2" is unknown, then instead
    of a "cannot read h2" error, one should suggest the user to update
    its submodules (he probably lacks some objects and needs to fetch).

    Special case: if the submodule isn't initialized, proceed and warn
    about the submodule being uninitialized.

  * {blob,directory} -> submodule already work properly, though
    suggesting the user to run "git submodule update -i" when we *know*
    we have uninitialized submodules because of a checkout is a good
    idea.

  Note that uninitialized submodules are not special cases for the two
first cases, because uninitialized submodules are not different from a
directory with possibly untracked files in it. An uninitialized
submodule probably need to be dealt with as if it's clean from many
places actually.


  The issue is that I tried to grok upack-trees.c, but the code is quite
dense, and how to hack this efficiently still eludes me, because I don't
really get the big picture enough yet.


-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]