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

Junio C Hamano [off-list ref] writes:

Clemens Buchacher [off-list ref] writes:

quoted

On Wed, Dec 10, 2008 at 09:12:59PM +0100, Clemens Buchacher wrote:

quoted

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 <redacted>
---
 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;

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 <redacted>
---

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

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

Thanks,
Dscho

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.

Johannes Schindelin [off-list ref] writes:

quoted

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.

Hi,

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

Johannes Schindelin [off-list ref] writes:

quoted

quoted

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