git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] diffcore-rename: don't consider unmerged path as source
@ 2011-03-18  1:42 Martin von Zweigbergk
  2011-03-18  6:49 ` Junio C Hamano
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Martin von Zweigbergk @ 2011-03-18  1:42 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Martin von Zweigbergk

The output from 'git status' and 'git diff-index --cached -M' is
broken when there are unmerged files as well as new files similar to
the unmerged file (in stage 1).

When two copies have been created the output is:

$ git status -s
C  original -> exact-copy
R  original -> modified-copy
 U original

There are several errors here: 1) "original" has invalid status " U",
2) "modified-copy" is listed as a rename, even though its source
("original") still exists, and 3) "exact-copy" is detected as a copy,
even though 'git status' is only supposed to show renames

In the verbose 'git status' output, the unmerged path is completely
missing. 'git diff-index --cached -M' show similar output.

Fix these problems by making diffcore-rename not consider unmerged
paths as source for rename detection. For simplicty, don't consider
the path independent of the type of merge conflict, even if the path
is deleted on at least one side. Still consider unmerged paths as
source for copy detection.

While at it, update the users of diff_options to use
DIFF_DETECT_RENAME instead of directly using the value 1 to enable
rename detection.

Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---

This is the first time I look at the diff code, so please review
carefully. I think the changes make sense, but I really don't know the
code enough to be sure.

Also not sure about the "while at it" stuff...

The test cases assume that the paths will be printed in a certain
order. Can I rely on that?

 builtin/commit.c                    |    2 +-
 diffcore-rename.c                   |    7 +++-
 t/t7510-status-merge-rename-copy.sh |   68 +++++++++++++++++++++++++++++++++++
 wt-status.c                         |    6 ++--
 4 files changed, 77 insertions(+), 6 deletions(-)
 create mode 100755 t/t7510-status-merge-rename-copy.sh

diff --git a/builtin/commit.c b/builtin/commit.c
index 82092e5..1f9f314 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1284,7 +1284,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	rev.show_root_diff = 1;
 	get_commit_format(format.buf, &rev);
 	rev.always_show_header = 0;
-	rev.diffopt.detect_rename = 1;
+	rev.diffopt.detect_rename = DIFF_DETECT_RENAME;
 	rev.diffopt.rename_limit = 100;
 	rev.diffopt.break_opt = 0;
 	diff_setup_done(&rev.diffopt);
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 0cd4c13..c53ca36 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -464,7 +464,7 @@ void diffcore_rename(struct diff_options *options)
 			else
 				locate_rename_dst(p->two, 1);
 		}
-		else if (!DIFF_FILE_VALID(p->two)) {
+		else if (!DIFF_PAIR_UNMERGED(p) && !DIFF_FILE_VALID(p->two)) {
 			/*
 			 * If the source is a broken "delete", and
 			 * they did not really want to get broken,
@@ -575,7 +575,10 @@ void diffcore_rename(struct diff_options *options)
 		struct diff_filepair *p = q->queue[i];
 		struct diff_filepair *pair_to_free = NULL;
 
-		if (!DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {
+		if (DIFF_PAIR_UNMERGED(p)) {
+			diff_q(&outq, p);
+		}
+		else if (!DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {
 			/*
 			 * Creation
 			 *
diff --git a/t/t7510-status-merge-rename-copy.sh b/t/t7510-status-merge-rename-copy.sh
new file mode 100755
index 0000000..8c7a4d2
--- /dev/null
+++ b/t/t7510-status-merge-rename-copy.sh
@@ -0,0 +1,68 @@
+#!/bin/sh
+#
+# Copyright (c) 2011 Martin von Zweigbergk
+#
+
+test_description='git status during merge with copies'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo a b c | xargs -n1 >original &&
+	git add original &&
+	git commit -m initial &&
+
+	cp original exact-copy &&
+	cp original modified-copy &&
+	echo x >>original &&
+	echo y >>modified-copy &&
+	git add original exact-copy modified-copy &&
+	git commit -m "create copies and modify original on master" &&
+
+	git checkout -b topic HEAD^ &&
+	echo z >>original &&
+	git add original &&
+	git commit -m "modify original on topic" &&
+	test_must_fail git merge master
+'
+
+cat >expected <<\EOF
+A  exact-copy
+A  modified-copy
+UU original
+EOF
+
+test_expect_success 'git status -s shows 2 added + 1 unmerged' '
+	git status -s -uno >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+A	exact-copy
+A	modified-copy
+U	original
+EOF
+
+test_expect_success 'git diff-index --cached shows 2 added + 1 unmerged' '
+	git diff-index --cached --name-status HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'git diff-index --cached -M shows 2 added + 1 unmerged' '
+	git diff-index --cached --name-status HEAD >actual &&
+	test_cmp expected actual
+'
+
+cat >expected <<\EOF
+C	original	exact-copy
+C	original	modified-copy
+U	original
+EOF
+
+test_expect_success 'git diff-index --cached -C shows 2 copies + 1 unmerged' '
+	git diff-index --cached -C --name-status HEAD |
+		sed "s/^C[0-9]*/C/g" >actual &&
+	test_cmp expected actual
+'
+
+test_done
diff --git a/wt-status.c b/wt-status.c
index 4daa8bb..532bbcc 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -320,7 +320,7 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 	if (s->ignore_submodule_arg) {
 		DIFF_OPT_SET(&rev.diffopt, OVERRIDE_SUBMODULE_CONFIG);
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
-    }
+	}
 	rev.diffopt.format_callback = wt_status_collect_changed_cb;
 	rev.diffopt.format_callback_data = s;
 	init_pathspec(&rev.prune_data, s->pathspec);
@@ -345,7 +345,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
 	rev.diffopt.format_callback = wt_status_collect_updated_cb;
 	rev.diffopt.format_callback_data = s;
-	rev.diffopt.detect_rename = 1;
+	rev.diffopt.detect_rename = DIFF_DETECT_RENAME;
 	rev.diffopt.rename_limit = 200;
 	rev.diffopt.break_opt = 0;
 	init_pathspec(&rev.prune_data, s->pathspec);
@@ -597,7 +597,7 @@ static void wt_status_print_verbose(struct wt_status *s)
 	setup_revisions(0, NULL, &rev, &opt);
 
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
-	rev.diffopt.detect_rename = 1;
+	rev.diffopt.detect_rename = DIFF_DETECT_RENAME;
 	rev.diffopt.file = s->fp;
 	rev.diffopt.close_file = 0;
 	/*
-- 
1.7.4.79.gcbe20

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-03-24  2:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-18  1:42 [PATCH] diffcore-rename: don't consider unmerged path as source Martin von Zweigbergk
2011-03-18  6:49 ` Junio C Hamano
2011-03-18 14:00   ` Martin von Zweigbergk
2011-03-21  8:06 ` Junio C Hamano
2011-03-24  0:52   ` Martin von Zweigbergk
2011-03-24  2:41 ` [PATCH v2] " Martin von Zweigbergk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).