[PATCH] modify/delete conflict resolution overwrites untracked file

[PATCH] modify/delete conflict resolution overwrites untracked file

[Clemens Buchacher]

If a file was removed in HEAD, but modified in MERGE_HEAD, recursive merge
will result in a "CONFLICT (delete/modify)". If the (now untracked) file
already exists and was not added to the index, it is overwritten with the
conflict resolution contents.

In similar situations (cf. test 2), the merge would abort with

"error: Untracked working tree 'file' would be overwritten by merge."

The same should happen in this case.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

If it's a regression, it dates far back, since 1.5.0 fails as well.

I don't have time to look into this until Saturday. I won't complain if
someone beats me to it. ;-)

Clemens

 t/t7607-merge-overwrite.sh |   87 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)
 create mode 100755 t/t7607-merge-overwrite.sh

diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
new file mode 100755
index 0000000..513097c
--- /dev/null
+++ b/t/t7607-merge-overwrite.sh
@@ -0,0 +1,87 @@
+#!/bin/sh
+
+test_description='git-merge
+
+Do not overwrite changes.'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo c0 > c0.c &&
+	git add c0.c &&
+	git commit -m c0 &&
+	git tag c0 &&
+	echo c1 > c1.c &&
+	git add c1.c &&
+	git commit -m c1 &&
+	git tag c1 &&
+	git reset --hard c0 &&
+	echo c2 > c2.c &&
+	git add c2.c &&
+	git commit -m c2 &&
+	git tag c2 &&
+	git reset --hard c1 &&
+	echo "c1 a" > c1.c &&
+	git add c1.c &&
+	git commit -m "c1 a" &&
+	git tag c1a &&
+	echo "VERY IMPORTANT CHANGES" > important
+'
+
+test_expect_success 'will not overwrite untracked file' '
+	git reset --hard c1 &&
+	cat important > c2.c &&
+	! git merge c2 &&
+	test_cmp important c2.c
+'
+
+test_expect_success 'will not overwrite new file' '
+	git reset --hard c1 &&
+	cat important > c2.c &&
+	git add c2.c &&
+	! git merge c2 &&
+	test_cmp important c2.c
+'
+
+test_expect_success 'will not overwrite staged changes' '
+	git reset --hard c1 &&
+	cat important > c2.c &&
+	git add c2.c &&
+	rm c2.c &&
+	! git merge c2 &&
+	git checkout c2.c &&
+	test_cmp important c2.c
+'
+
+test_expect_failure 'will not overwrite removed file' '
+	git reset --hard c1 &&
+	git rm c1.c &&
+	git commit -m "rm c1.c" &&
+	cat important > c1.c &&
+	! git merge c1a &&
+	test_cmp important c1.c
+'
+
+test_expect_success 'will not overwrite re-added file' '
+	git reset --hard c1 &&
+	git rm c1.c &&
+	git commit -m "rm c1.c" &&
+	cat important > c1.c &&
+	git add c1.c &&
+	! git merge c1a &&
+	test_cmp important c1.c
+'
+
+test_expect_success 'will not overwrite removed file with staged changes' '
+	git reset --hard c1 &&
+	git rm c1.c &&
+	git commit -m "rm c1.c" &&
+	cat important > c1.c &&
+	git add c1.c &&
+	rm c1.c &&
+	! git merge c1a &&
+	git checkout c1.c &&
+	test_cmp important c1.c
+'
+
+test_done
-- 
1.6.0

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> If it's a regression, it dates far back, since 1.5.0 fails as well.

A good lit(h)mus test to see if it is a regression or just a plain bug in
the recursive strategy would be to see what 'resolve' strategy does
(replace "merge" with "merge -s resolve" in your test).

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Clemens Buchacher]

On Wed, Dec 10, 2008 at 12:51:59PM -0800, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > If it's a regression, it dates far back, since 1.5.0 fails as well.
> 
> A good lit(h)mus test to see if it is a regression or just a plain bug in
> the recursive strategy would be to see what 'resolve' strategy does
> (replace "merge" with "merge -s resolve" in your test).

"merge -s resolve" fails with

Trying really trivial in-index merge...
error: Merge requires file-level merging
Nope.
Trying simple merge.
Simple merge failed, trying Automatic merge.
ERROR: c1.c: Not handling case ae9304576a6ec3419b231b2b9c8e33a06f97f9fb ->
-> 8173b675dc61bb578b411c769c9fb654625a7c4e
fatal: merge program failed
Automatic merge failed; fix conflicts and then commit the result.

and therefore passes the test.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Wed, Dec 10, 2008 at 12:51:59PM -0800, Junio C Hamano wrote:
>> Clemens Buchacher <drizzd@aon.at> writes:
>> 
>> > If it's a regression, it dates far back, since 1.5.0 fails as well.
>> 
>> A good lit(h)mus test to see if it is a regression or just a plain bug in
>> the recursive strategy would be to see what 'resolve' strategy does
>> (replace "merge" with "merge -s resolve" in your test).
>
> "merge -s resolve" fails with
>
> Trying really trivial in-index merge...
> error: Merge requires file-level merging
> Nope.
> Trying simple merge.
> Simple merge failed, trying Automatic merge.
> ERROR: c1.c: Not handling case ae9304576a6ec3419b231b2b9c8e33a06f97f9fb ->
> -> 8173b675dc61bb578b411c769c9fb654625a7c4e
> fatal: merge program failed
> Automatic merge failed; fix conflicts and then commit the result.
>
> and therefore passes the test.

Are you saying that:

 (1) the step should result in conflict and the merge should fail, but it
     should not clobber c1.c nevertheless; and

 (2) resolve fails to merge (as expected), and it does not clobber c1.c
     (as expected); therefore it passes the test.

If so, then you now established that it is a bug in merge-recursive,
right [implementors of recursive-in-C CC'ed]?

Or are you saying that the step should not fail to begin with?

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Clemens Buchacher]

On Wed, Dec 10, 2008 at 03:36:11PM -0800, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> >
> > "merge -s resolve" fails with
> >
> > Trying really trivial in-index merge...
> > error: Merge requires file-level merging
> > Nope.
> > Trying simple merge.
> > Simple merge failed, trying Automatic merge.
> > ERROR: c1.c: Not handling case ae9304576a6ec3419b231b2b9c8e33a06f97f9fb ->
> > -> 8173b675dc61bb578b411c769c9fb654625a7c4e
> > fatal: merge program failed
> > Automatic merge failed; fix conflicts and then commit the result.
> >
> > and therefore passes the test.
> 
> Are you saying that:
> 
>  (1) the step should result in conflict and the merge should fail, but it
>      should not clobber c1.c nevertheless; and
> 
>  (2) resolve fails to merge (as expected), and it does not clobber c1.c
>      (as expected); therefore it passes the test.

The latter.

> If so, then you now established that it is a bug in merge-recursive,
> right [implementors of recursive-in-C CC'ed]?

Correct.

> Or are you saying that the step should not fail to begin with?

No. IMO, merge should fail and abort. That is, it should not modify the
working tree at all and tell the user that an untracked file is in the way.

The tests check that merge returns an error code and c1.c is not modified.
Test number 5 fails, unless the merge strategy resolve is used. While this
indicates a bug in the recursive strategy, I am not satisfied with the error
output of the resolve strategy either. It should output

"error: Untracked working tree file '...' would be overwritten by merge."

just like test number 2 does.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Wed, Dec 10, 2008 at 03:36:11PM -0800, Junio C Hamano wrote:
>> Clemens Buchacher <drizzd@aon.at> writes:
>> >
>> > "merge -s resolve" fails with
>> >
>> > Trying really trivial in-index merge...
>> > error: Merge requires file-level merging
>> > Nope.
>> > Trying simple merge.
>> > Simple merge failed, trying Automatic merge.
>> > ERROR: c1.c: Not handling case ae9304576a6ec3419b231b2b9c8e33a06f97f9fb ->
>> > -> 8173b675dc61bb578b411c769c9fb654625a7c4e
>> > fatal: merge program failed
>> > Automatic merge failed; fix conflicts and then commit the result.
>> >
>> > and therefore passes the test.
>> 
>> Are you saying that:
>> 
>>  (1) the step should result in conflict and the merge should fail, but it
>>      should not clobber c1.c nevertheless; and
>> 
>>  (2) resolve fails to merge (as expected), and it does not clobber c1.c
>>      (as expected); therefore it passes the test.
>
> The latter.

I said "and" at the end of (1) so I do not understand your answer, but I
take it to mean that both (1) and (2) are true, judging from the rest of
your message.

>> If so, then you now established that it is a bug in merge-recursive,
>> right [implementors of recursive-in-C CC'ed]?
>
> Correct.
>
>> Or are you saying that the step should not fail to begin with?
>
> No...

Ok, thanks.  Unfortunately my git day for this week is over, so I'll try
to find time to take a look at what recursive strategy does wrong over the
weekend, if nobody gets to it before me.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Clemens Buchacher]

On Wed, Dec 10, 2008 at 09:12:59PM +0100, Clemens Buchacher wrote:
> If a file was removed in HEAD, but modified in MERGE_HEAD, recursive merge
> will result in a "CONFLICT (delete/modify)". If the (now untracked) file
> already exists and was not added to the index, it is overwritten with the
> conflict resolution contents.

The following patch fixes the problem described above, but it also breaks
t6023-merge-rename-nocruft.sh, which tries to merge "A" renamed to "B" in
HEAD and "A" modified in MERGE_HEAD, while ignoring an untracked file "A" in
the working tree. If we want to be able to do this, we have to handle the
other case after rename detection.

---
 unpack-trees.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 54f301d..de5b616 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -859,6 +859,9 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 	if (index) {
 		if (verify_uptodate(index, o))
 			return -1;
+	} else if (remote && !remote_match) {
+		if (verify_absent(remote, "overwritten", o))
+			return -1;
 	}
 
 	o->nontrivial_merge = 1;
-- 
1.6.1.rc2

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Wed, Dec 10, 2008 at 09:12:59PM +0100, Clemens Buchacher wrote:
>> If a file was removed in HEAD, but modified in MERGE_HEAD, recursive merge
>> will result in a "CONFLICT (delete/modify)". If the (now untracked) file
>> already exists and was not added to the index, it is overwritten with the
>> conflict resolution contents.
>
> The following patch fixes the problem described above, but it also breaks
> t6023-merge-rename-nocruft.sh, which tries to merge "A" renamed to "B" in
> HEAD and "A" modified in MERGE_HEAD, while ignoring an untracked file "A" in
> the working tree. If we want to be able to do this, we have to handle the
> other case after rename detection.

If the breakage is in merge-recursive but not in merge-resolve, my gut
feeling is that we should not be touching unpack-trees at all.  With luck
I may be able to find some time to take a look at this myself but right
now we are entertaining a guest, so....

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Clemens Buchacher <drizzd@aon.at> writes:
>
>> On Wed, Dec 10, 2008 at 09:12:59PM +0100, Clemens Buchacher wrote:
>>> If a file was removed in HEAD, but modified in MERGE_HEAD, recursive merge
>>> will result in a "CONFLICT (delete/modify)". If the (now untracked) file
>>> already exists and was not added to the index, it is overwritten with the
>>> conflict resolution contents.
>>
>> The following patch fixes the problem described above, but it also breaks
>> t6023-merge-rename-nocruft.sh, which tries to merge "A" renamed to "B" in
>> HEAD and "A" modified in MERGE_HEAD, while ignoring an untracked file "A" in
>> the working tree. If we want to be able to do this, we have to handle the
>> other case after rename detection.
>
> If the breakage is in merge-recursive but not in merge-resolve, my gut
> feeling is that we should not be touching unpack-trees at all.  With luck
> I may be able to find some time to take a look at this myself but right
> now we are entertaining a guest, so....

-- >8 --
merge-recursive: do not clobber untracked working tree garbage

When merge-recursive wanted to create a new file in the work tree (either
as the final result, or a hint for reference purposes while delete/modify
conflicts), it unconditionally overwrote an untracked file in the working
tree.  Be careful not to lose whatever the user has that is not tracked.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 merge-recursive.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git c/merge-recursive.c w/merge-recursive.c
index a0c804c..2da4333 100644
--- c/merge-recursive.c
+++ w/merge-recursive.c
@@ -447,6 +447,30 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
 	}
 }
 
+static int would_lose_untracked(const char *path)
+{
+	int pos = cache_name_pos(path, strlen(path));
+
+	if (pos < 0)
+		pos = -1 - pos;
+	while (pos < active_nr &&
+	       !strcmp(path, active_cache[pos]->name)) {
+		/*
+		 * If stage #0, it is definitely tracked.
+		 * If it has stage #2 then it was tracked
+		 * before this merge started.  All other
+		 * cases the path was not tracked.
+		 */
+		switch (ce_stage(active_cache[pos])) {
+		case 0:
+		case 2:
+			return 0;
+		}
+		pos++;
+	}
+	return file_exists(path);
+}
+
 static int make_room_for_path(const char *path)
 {
 	int status;
@@ -462,6 +486,14 @@ static int make_room_for_path(const char *path)
 		die(msg, path, "");
 	}
 
+	/*
+	 * Do not unlink a file in the work tree if we are not
+	 * tracking it.
+	 */
+	if (would_lose_untracked(path))
+		return error("refusing to lose untracked file at '%s'",
+			     path);
+
 	/* Successful unlink is good.. */
 	if (!unlink(path))
 		return 0;

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Johannes Schindelin]

Hi,

On Sun, 14 Dec 2008, Junio C Hamano wrote:


> merge-recursive: do not clobber untracked working tree garbage
> 
> When merge-recursive wanted to create a new file in the work tree (either
> as the final result, or a hint for reference purposes while delete/modify
> conflicts), it unconditionally overwrote an untracked file in the working
> tree.  Be careful not to lose whatever the user has that is not tracked.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---

Thanks, I had no time at all to look into this issue.

> diff --git c/merge-recursive.c w/merge-recursive.c
> index a0c804c..2da4333 100644
> --- c/merge-recursive.c
> +++ w/merge-recursive.c
> @@ -447,6 +447,30 @@ static void flush_buffer(int fd, const char *buf, unsigned long size)
>  	}
>  }
>  
> +static int would_lose_untracked(const char *path)
> +{
> +	int pos = cache_name_pos(path, strlen(path));
> +
> +	if (pos < 0)
> +		pos = -1 - pos;
> +	while (pos < active_nr &&
> +	       !strcmp(path, active_cache[pos]->name)) {
> +		/*
> +		 * If stage #0, it is definitely tracked.
> +		 * If it has stage #2 then it was tracked
> +		 * before this merge started.  All other
> +		 * cases the path was not tracked.
> +		 */
> +		switch (ce_stage(active_cache[pos])) {
> +		case 0:
> +		case 2:
> +			return 0;
> +		}
> +		pos++;
> +	}
> +	return file_exists(path);

I wonder if it is cheaper to test file_exists() when the index contains a 
lot of files...

Thanks,
Dscho

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> merge-recursive: do not clobber untracked working tree garbage
>> ...
>> +static int would_lose_untracked(const char *path)
>> +{
>> +	int pos = cache_name_pos(path, strlen(path));
>> +
>> +	if (pos < 0)
>> +		pos = -1 - pos;
>> +	while (pos < active_nr &&
>> +	       !strcmp(path, active_cache[pos]->name)) {
>> +		/*
>> +		 * If stage #0, it is definitely tracked.
>> +		 * If it has stage #2 then it was tracked
>> +		 * before this merge started.  All other
>> +		 * cases the path was not tracked.
>> +		 */
>> +		switch (ce_stage(active_cache[pos])) {
>> +		case 0:
>> +		case 2:
>> +			return 0;
>> +		}
>> +		pos++;
>> +	}
>> +	return file_exists(path);
>
> I wonder if it is cheaper to test file_exists() when the index contains a 
> lot of files...

"cheaper" than what?

The test with the index is not about efficiency, but is all about
correctness.

If the file in the work tree came from the index that represents "our"
branch, we do not want to say "yes" from this function, even when
(actually, "especially when") the path exists in the work tree.
unpack-trees decided that the path matches the index (otherwise you are
already guaranteeing that we wouldn't have come this far, right?)  and we
are about to write out the (potentially partial) merge result to the
working tree file.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Johannes Schindelin]

Hi,

On Mon, 15 Dec 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> merge-recursive: do not clobber untracked working tree garbage
> >> ...
> >> +static int would_lose_untracked(const char *path)
> >> +{
> >> +	int pos = cache_name_pos(path, strlen(path));
> >> +
> >> +	if (pos < 0)
> >> +		pos = -1 - pos;
> >> +	while (pos < active_nr &&
> >> +	       !strcmp(path, active_cache[pos]->name)) {
> >> +		/*
> >> +		 * If stage #0, it is definitely tracked.
> >> +		 * If it has stage #2 then it was tracked
> >> +		 * before this merge started.  All other
> >> +		 * cases the path was not tracked.
> >> +		 */
> >> +		switch (ce_stage(active_cache[pos])) {
> >> +		case 0:
> >> +		case 2:
> >> +			return 0;
> >> +		}
> >> +		pos++;
> >> +	}
> >> +	return file_exists(path);
> >
> > I wonder if it is cheaper to test file_exists() when the index contains a 
> > lot of files...
> 
> "cheaper" than what?

Oops.  I meant "cheaper to test file_exists() _first_".  But thinking 
about it again, it is probably way more expensive, especially in the cold 
cache case.

Sorry for the noise,
Dscho

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Clemens Buchacher]

On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
> merge-recursive: do not clobber untracked working tree garbage
> 
> When merge-recursive wanted to create a new file in the work tree (either
> as the final result, or a hint for reference purposes while delete/modify
> conflicts), it unconditionally overwrote an untracked file in the working
> tree.  Be careful not to lose whatever the user has that is not tracked.

This leaves the index in an unmerged state, however, so that a subsequent
git reset --hard still kills the file. And I just realized that the same
goes for merge-resolve. So I'd prefer to abort the merge, leave everything
unchanged and tell the user to clean up first.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
>> merge-recursive: do not clobber untracked working tree garbage
>> 
>> When merge-recursive wanted to create a new file in the work tree (either
>> as the final result, or a hint for reference purposes while delete/modify
>> conflicts), it unconditionally overwrote an untracked file in the working
>> tree.  Be careful not to lose whatever the user has that is not tracked.
>
> This leaves the index in an unmerged state, however, so that a subsequent
> git reset --hard still kills the file. And I just realized that the same
> goes for merge-resolve. So I'd prefer to abort the merge, leave everything
> unchanged and tell the user to clean up first.

That is unfortunately asking for a moon, I am afraid.

It needs a major restructuring of the code so that the recursive works
more like the way resolve works, namely, changing the final "writeout"
into two phase thing (the first phase making sure nothing is clobbered in
the work tree, and then the second phase actually touching the work tree).

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Mike Ralphson]

2008/12/15 Junio C Hamano <gitster@pobox.com>:
> Clemens Buchacher <drizzd@aon.at> writes:
>
>> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
>>> merge-recursive: do not clobber untracked working tree garbage
>>>
>>> When merge-recursive wanted to create a new file in the work tree (either
>>> as the final result, or a hint for reference purposes while delete/modify
>>> conflicts), it unconditionally overwrote an untracked file in the working
>>> tree.  Be careful not to lose whatever the user has that is not tracked.
>>
>> This leaves the index in an unmerged state, however, so that a subsequent
>> git reset --hard still kills the file. And I just realized that the same
>> goes for merge-resolve. So I'd prefer to abort the merge, leave everything
>> unchanged and tell the user to clean up first.
>
> That is unfortunately asking for a moon, I am afraid.
>
> It needs a major restructuring of the code so that the recursive works
> more like the way resolve works, namely, changing the final "writeout"
> into two phase thing (the first phase making sure nothing is clobbered in
> the work tree, and then the second phase actually touching the work tree).

I wonder if another approach is workable... to read 'vulnerable'
untracked working tree files into a new (temporary, uncommittable)
stage in the index, perform whatever merging is required, then
reinstate all entries from the new stage.

Thus the merge should normally succeed under the covers, and the
previously untracked file(s) would now show up as modified against the
tracked content.

Two problems I foresee - potential loss of untracked metadata, and
ensuring we reinstate the untracked contents in all possible paths the
user can use to abort or resolve the merge.

Mike

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Johannes Schindelin]

Hi,

On Mon, 15 Dec 2008, Mike Ralphson wrote:

> 2008/12/15 Junio C Hamano <gitster@pobox.com>:
> > Clemens Buchacher <drizzd@aon.at> writes:
> >
> >> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
> >>> merge-recursive: do not clobber untracked working tree garbage
> >>>
> >>> When merge-recursive wanted to create a new file in the work tree 
> >>> (either as the final result, or a hint for reference purposes while 
> >>> delete/modify conflicts), it unconditionally overwrote an untracked 
> >>> file in the working tree.  Be careful not to lose whatever the user 
> >>> has that is not tracked.
> >>
> >> This leaves the index in an unmerged state, however, so that a 
> >> subsequent git reset --hard still kills the file. And I just realized 
> >> that the same goes for merge-resolve. So I'd prefer to abort the 
> >> merge, leave everything unchanged and tell the user to clean up 
> >> first.
> >
> > That is unfortunately asking for a moon, I am afraid.
> >
> > It needs a major restructuring of the code so that the recursive works 
> > more like the way resolve works, namely, changing the final "writeout" 
> > into two phase thing (the first phase making sure nothing is clobbered 
> > in the work tree, and then the second phase actually touching the work 
> > tree).
> 
> I wonder if another approach is workable... to read 'vulnerable' 
> untracked working tree files into a new (temporary, uncommittable) stage 
> in the index, perform whatever merging is required, then reinstate all 
> entries from the new stage.

I think the solution is not in making things more complicated, but 
simpler.  I agree with Junio that the recursive merge needs a major 
rewrite which respects d/f conflicts and renames in the _design_, not as 
an afterthought.

Besides, I really do not want untracked files to be inserted into a stage.  
Remember, adding something to the index means to hash it, and I do have 
half-a-gigabyte untracked data in some of my worktrees.

Ciao,
Dscho

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Mike Ralphson]

2008/12/15 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> On Mon, 15 Dec 2008, Mike Ralphson wrote:
>> I wonder if another approach is workable... to read 'vulnerable'
>> untracked working tree files into a new (temporary, uncommittable) stage
>> in the index, perform whatever merging is required, then reinstate all
>> entries from the new stage.
>
> I think the solution is not in making things more complicated, but
> simpler.  I agree with Junio that the recursive merge needs a major
> rewrite which respects d/f conflicts and renames in the _design_, not as
> an afterthought.

Yes, it might also score low on not surprising the user. I bow to more
experienced heads on matters of clean design and merge strategies.

> Besides, I really do not want untracked files to be inserted into a stage.
> Remember, adding something to the index means to hash it, and I do have
> half-a-gigabyte untracked data in some of my worktrees.

Granted, but here we are only talking about files (or clashing
file-names) which someone [else] has already added/removed/modified in
another branch etc.

Mike

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Clemens Buchacher <drizzd@aon.at> writes:
>
>> On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
>>> merge-recursive: do not clobber untracked working tree garbage
>>> 
>>> When merge-recursive wanted to create a new file in the work tree (either
>>> as the final result, or a hint for reference purposes while delete/modify
>>> conflicts), it unconditionally overwrote an untracked file in the working
>>> tree.  Be careful not to lose whatever the user has that is not tracked.
>>
>> This leaves the index in an unmerged state, however, so that a subsequent
>> git reset --hard still kills the file. And I just realized that the same
>> goes for merge-resolve. So I'd prefer to abort the merge, leave everything
>> unchanged and tell the user to clean up first.
>
> That is unfortunately asking for a moon, I am afraid.
>
> It needs a major restructuring of the code so that the recursive works
> more like the way resolve works, namely, changing the final "writeout"
> into two phase thing (the first phase making sure nothing is clobbered in
> the work tree, and then the second phase actually touching the work tree).

Actually, the more I think about it, I do not think this is not something
we would even want to do.

By this, I do not mean the restructuring to bring some sanity to
merge-recursive.  That is necessary.  What I do not think is unnecessary
is the issue you raise about "git reset --hard".

You can do a merge inside a dirty work tree, and the merge will fail
without clobbering your work tree files that are dirty when it needs to be
able to overwrite to do its job.  The set of "dirty files" in this
sentence of course includes paths that are modified since HEAD, but it
also includes also paths that do not exist in HEAD (i.e. "new files").

But we already caution users that you need to know what you are doing when
working in such a dirty work tree.  Namely, after a failed merge, your
next "git reset --hard" will blow away your local modifications.  And
local modifications in this context includes the files you could have
added to the index but you haven't.

By the way, I think the patch I sent earlier is too complex and
suboptimal for an entirely different reason.

The only reason the codepath for delete/modify in process_entry() wants to
leave the modified side in the result is because the internal merge done
when the algorithm is coming up with a merged merge bases _must_ be fully
resolved.  There is no such requirement for the final round of the merge
whose result is written out to the work tree.  Whether the path that was
involved in delete/modify conflict was originally in the index or not, we
should just leave it alone in the work tree.  The logic I implemented as
the would_lose_untracked() function is just overkill.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Clemens Buchacher]

On Mon, Dec 15, 2008 at 02:13:13PM -0800, Junio C Hamano wrote:
> You can do a merge inside a dirty work tree, and the merge will fail
> without clobbering your work tree files that are dirty when it needs to be
> able to overwrite to do its job.  The set of "dirty files" in this
> sentence of course includes paths that are modified since HEAD, but it
> also includes also paths that do not exist in HEAD (i.e. "new files").
> 
> But we already caution users that you need to know what you are doing when
> working in such a dirty work tree.  Namely, after a failed merge, your
> next "git reset --hard" will blow away your local modifications.  And
> local modifications in this context includes the files you could have
> added to the index but you haven't.

I strongly disagree. With the suggested behavior I would have to
double-check every single untracked file in my tree for conflicts before
trying a throw-away merge followed by git reset --hard, for example. The
file could even be ignored! Whatever happened to git reset doesn't touch
untracked files?

I would even prefer breaking t6023 (until we can properly implement this
feature) in order to avoid that.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I strongly disagree. With the suggested behavior I would have to
> double-check every single untracked file in my tree for conflicts before
> trying a throw-away merge followed by git reset --hard, for example.

AFAICS, that's not "suggested behaviour" for merge, but it is the
behaviour that has been true for eternity.

You can suggest to fix "reset --hard", though.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:

> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > I strongly disagree. With the suggested behavior I would have to
> > double-check every single untracked file in my tree for conflicts before
> > trying a throw-away merge followed by git reset --hard, for example.
> 
> AFAICS, that's not "suggested behaviour" for merge, but it is the
> behaviour that has been true for eternity.
> 
> You can suggest to fix "reset --hard", though.

Errr... what about using "reset --merge" instead?
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Clemens Buchacher]

On Mon, Dec 15, 2008 at 02:22:28AM -0800, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> > On Sun, Dec 14, 2008 at 07:34:46PM -0800, Junio C Hamano wrote:
> >> merge-recursive: do not clobber untracked working tree garbage
> >> 
> >> When merge-recursive wanted to create a new file in the work tree (either
> >> as the final result, or a hint for reference purposes while delete/modify
> >> conflicts), it unconditionally overwrote an untracked file in the working
> >> tree.  Be careful not to lose whatever the user has that is not tracked.
> >
> > This leaves the index in an unmerged state, however, so that a subsequent
> > git reset --hard still kills the file. And I just realized that the same
> > goes for merge-resolve. So I'd prefer to abort the merge, leave everything
> > unchanged and tell the user to clean up first.
> 
> That is unfortunately asking for a moon, I am afraid.
> 
> It needs a major restructuring of the code so that the recursive works
> more like the way resolve works, namely, changing the final "writeout"
> into two phase thing (the first phase making sure nothing is clobbered in
> the work tree, and then the second phase actually touching the work tree).

I've been giving this some thought and I would like to propose the following
solution:

1. Save index state.
2. Merge using whichever strategy, in index only, ignoring work tree.
   This step includes rename detection.
3. Check that work tree files match original index, if index does not match
   HEAD. Otherwise abort, restore index and leave everything unchanged.
4. Checkout index, overwriting work tree files, and removing files which are
   in HEAD, but not in the index.
5. If the merge was clean, commit.

AFAICS, this is largely the behavior right now, except that steps 3 and 4
are intermingled with step 2, which makes it impossible to abort the merge
if an untracked file is in the way after rename detection.

The idea at step 3 is that we can decide whether or not to proceed, based
only on the merge result, irrespective of the strategy used, possible rename
detection, or conflict resolution.

Apart from the fact that this seems like the sane thing to do, I want this
behavior because it allows me to do

git merge <branch>
# Conflicts? I don't have time for that now.
git reset --hard HEAD

under all circumstances, without touching any untracked files.

Do you agree that this is a desireable goal?

I have not looked into d/f conflicts, but I am under the impression that
this could also be handled at step 3, as far as the work tree is concerned.
Is the above proposal a workable approach, which I can pursue independently
of the major rewrite wrt. d/f conflicts Johannes indicated?

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I've been giving this some thought and I would like to propose the following
> solution:
>
> 1. Save index state.
> 2. Merge using whichever strategy, in index only, ignoring work tree.
>    This step includes rename detection.
> 3. Check that work tree files match original index, if index does not match
>    HEAD. Otherwise abort, restore index and leave everything unchanged.
> 4. Checkout index, overwriting work tree files, and removing files which are
>    in HEAD, but not in the index.
> 5. If the merge was clean, commit.
>
> AFAICS, this is largely the behavior right now, except that steps 3 and 4
> are intermingled with step 2, which makes it impossible to abort the merge
> if an untracked file is in the way after rename detection.

The description of step 3 above may be a bit too sketchy and simplistic,
but I agree, provided if you are talking about what merge-recursive
backend does, that is how it ought to do things.  The strategy should
instead:

 - have trivial merge done inside the index, likely leveraging
   unpack_trees() three-way merge mechanism we already have, and it will
   allow you to notice and abort upon seeing local modifications in the
   work tree at this stage, except for renamed paths;

 - notice renames for the unmerged ones and deal with them inside the
   index, still without resolving.  E.g. in a merge between A and B, if
   side A deleted "path1" and created a "path2" that is similar to deleted
   "path1", have the ancestor version at stage #1 of "path2", hoist
   "path2" of side A to stage #2, and move the version of "path1" from
   side B and have it at stage #3 of "path2", which would result in an
   index without "path1" in any stage, and "path2" in three stages.

   If the other side's rename causes a path to be moved, you can/should
   check and notice local modifications to it at this stage.

   You can also notice D/F conflict at this phase (e.g. side B in the
   above example may have created a new path "path2/file1" and the index
   may have stage #3 entry without any other stages for "path2/file1") and
   abort.

 - try to resolve unmerged paths inside the index.

   Because you matched up possible renames and and adjusted the entries,
   you can resolve aggressively, e.g. a path with identical stage #1 and
   #3 but missing stage #2 results in delete, a path with missing stage #1
   and #2 but with stage #3 results in create, etc.

 - notice the paths that need to be written out to the work tree.  Paths
   with local changes should have been noticed already in the above, but
   while debugging your new algorithm, you may want to have a logic to
   double check so that you won't miss any.  Abort if you find any local
   change that can be clobbered.

 - Then, finally, you commit to writing the results out, both the index
   and the work tree.  The changes to the work tree may be cleanly merged,
   or an unmerged results with three-way conflict markers.

This way, you will never have "merge stops in halfway because it notices
it will clobber a local change too late after it has already written a
partial merge results to the work tree" problem, I think.

> git merge <branch>
> # Conflicts? I don't have time for that now.
> git reset --hard HEAD
>
> under all circumstances, without touching any untracked files.

Even though I agree the above without the second line is a good thing to
have, I think your description is a wrong way to present the desirable
goal.

"Conflicts?" is an indication that you had a lot of other things going on
inside your work tree as set of local changes that you "don't have time
for that now", and "reset --hard" is a sure way to lose them.  You
shouldn't train your users to say "--hard" lightly.

The issue of unreliable D/F conflict detection and local change detection
the current merge-recursive has at its corner case is not about
"Conflicts? I don't have time for that now" at all.  It is not about " I
had something precious in my work tree, but the merge turned out to be
unmanageable for me", either.

The issue is about "Crap, if merge has to abort in the middle because it
has to refrain from writing out a half-merged state out, not to overwrite
my local changes, it shouldn't have written out anything else to the work
tree at all.  Don't abort in the middle but abort upfront without touching
my work tree!".

I think the correct statement of the goal is:

	$ git merge ..
        # Ah, I have "path2" locally modified so it cannot proceed?
        # I did not lose anything, nor it did anything to my index nor
        # work tree.  Good.

By this, you realize that you should wrap up what you have been doing
first, because the above is an indication that other end has made some
overlapping changes.  You can either (1) decide not to merge at this point
because you are not ready, keep working on what you were doing first and
later merge, or (2) stash away what you have been doing and do the merge
first, and then unstash.

Re: [PATCH] modify/delete conflict resolution overwrites untracked file

[Clemens Buchacher]

On Sun, Dec 28, 2008 at 02:21:40PM -0800, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > I've been giving this some thought and I would like to propose the following
> > solution:
> >
> > 1. Save index state.
> > 2. Merge using whichever strategy, in index only, ignoring work tree.
> >    This step includes rename detection.
> > 3. Check that work tree files match original index, if index does not match
> >    HEAD. Otherwise abort, restore index and leave everything unchanged.
> > 4. Checkout index, overwriting work tree files, and removing files which are
> >    in HEAD, but not in the index.
> > 5. If the merge was clean, commit.
> >
> > AFAICS, this is largely the behavior right now, except that steps 3 and 4
> > are intermingled with step 2, which makes it impossible to abort the merge
> > if an untracked file is in the way after rename detection.
> 
> The description of step 3 above may be a bit too sketchy and simplistic,
> but I agree, provided if you are talking about what merge-recursive
> backend does, that is how it ought to do things.  The strategy should
> instead:

Actually, I was hoping to find a solution that's independent of the merge
strategy. I think the merge strategy should not have to concern itself with
the work tree at all.

Regarding my description of step 3, I worded it carefully to cover all
cases. My assumption at this stage is that the merge strategy took care not
to overwrite any staged changes, unless the merge is trivial. It is
therefore safe to overwrite the work tree file if it matches either the
original index, or HEAD.

>  - have trivial merge done inside the index, likely leveraging
>    unpack_trees() three-way merge mechanism we already have, and it will
>    allow you to notice and abort upon seeing local modifications in the
>    work tree at this stage, except for renamed paths;

Right, but why would we want to do it this early? Is there a performance
issue? If not, I argue that we don't care about the work tree at this stage.
Only the end result matters.

>  - notice renames for the unmerged ones and deal with them inside the
>    index, still without resolving.  E.g. in a merge between A and B, if
>    side A deleted "path1" and created a "path2" that is similar to deleted
>    "path1", have the ancestor version at stage #1 of "path2", hoist
>    "path2" of side A to stage #2, and move the version of "path1" from
>    side B and have it at stage #3 of "path2", which would result in an
>    index without "path1" in any stage, and "path2" in three stages.

Right. So in this case we do not abort if "path1" had unstaged
changes, because "path1" of B was moved to "path2" anyways. Conversely, if
"path2" had unstaged changes, we do abort, because it would be overwritten
by the merge (or conflict resolution). All this can be decided without any
knowledge about detected renames or the merge strategy used.

>    If the other side's rename causes a path to be moved, you can/should
>    check and notice local modifications to it at this stage.
> 
>    You can also notice D/F conflict at this phase (e.g. side B in the
>    above example may have created a new path "path2/file1" and the index
>    may have stage #3 entry without any other stages for "path2/file1") and
>    abort.
> 
>  - try to resolve unmerged paths inside the index.
> 
>    Because you matched up possible renames and and adjusted the entries,
>    you can resolve aggressively, e.g. a path with identical stage #1 and
>    #3 but missing stage #2 results in delete, a path with missing stage #1
>    and #2 but with stage #3 results in create, etc.

This should be done before checking for D/F conflicts. If a file ends up
getting removed, the D/F conflict is avoided.

>  - notice the paths that need to be written out to the work tree.  Paths
>    with local changes should have been noticed already in the above, but
>    while debugging your new algorithm, you may want to have a logic to
>    double check so that you won't miss any.  Abort if you find any local
>    change that can be clobbered.
> 
>  - Then, finally, you commit to writing the results out, both the index
>    and the work tree.  The changes to the work tree may be cleanly merged,
>    or an unmerged results with three-way conflict markers.
>
> This way, you will never have "merge stops in halfway because it notices
> it will clobber a local change too late after it has already written a
> partial merge results to the work tree" problem, I think.

Right. So all we have to do is keep unpack_trees() and process_entry() in
merge_trees() from writing to the work tree and delay until after
try_merge_strategy() has completed. Then we check for D/F conflicts and
dirty work tree files before finally checking out the merge result.

Thinking about this now, I wonder what happens if recursive merge has a D/F
conflict in one of its recursions. There is nothing smart it can do, so it
could continue with a D/F conflict in the index, hoping that it goes away in
the final result. So again, D/F conflicts should not matter for the merge
strategy.

> > git merge <branch>
> > # Conflicts? I don't have time for that now.
> > git reset --hard HEAD
> >
> > under all circumstances, without touching any untracked files.
> 
> Even though I agree the above without the second line is a good thing to
> have, I think your description is a wrong way to present the desirable
> goal.
[...]
> The issue is about "Crap, if merge has to abort in the middle because it
> has to refrain from writing out a half-merged state out, not to overwrite
> my local changes, it shouldn't have written out anything else to the work
> tree at all.  Don't abort in the middle but abort upfront without touching
> my work tree!".

Point taken.