* [PATCH] mergetool: don't skip modify/remove conflicts
@ 2011-02-08 3:08 Martin von Zweigbergk
2011-02-09 21:45 ` Junio C Hamano
2011-02-13 4:09 ` [PATCH v2 0/2] " Martin von Zweigbergk
0 siblings, 2 replies; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-08 3:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Martin von Zweigbergk
Since bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17),
mergetool uses different ways of figuring out the list of files with
merge conflicts depending on whether rerere is active. If rerere is
active, mergetool will use 'git rerere status' to list the files with
remaining conflicts. However, the output from that command does not
list conflicts of types that rerere does not handle, such as
modify/remove conflicts.
Another problem with solely relying on the output from 'git rerere
status' is that, for new conflicts that are not yet known to rerere,
the output from the command will list the files even after adding them
to the index. This means that if the conflicts in some files have been
resolved and 'git mergetool' is run again, it will ask the user
something like the following for each of those files.
file1: file does not need merging
Continue merging other unresolved paths (y/n) ?
Solve the first of these problems by replacing the call to 'git rerere
status' with a call to the new 'git rerere remaining' that was
introduced in the previous commit. Solve the second problem by
modifying 'git rerere remaining' not to output files that have already
been staged.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
This applies on top of jc/rerere-remaining.
This is my first patch that touches any C code and I haven't written
anything in C for about 10 years. I hope it doesn't look too bad.
I also have another version of this patch that doesn't modify 'git
rerere remaining', but instead combines the output with the output
from 'git ls-files'. Tell me if you think that is better.
Is it correct to call "yes" with multiple arguments in the test script
the way I did?
builtin/rerere.c | 10 +++++++---
git-mergetool.sh | 2 +-
rerere.c | 31 ++++++++++++++++++++-----------
rerere.h | 3 +++
t/t7610-mergetool.sh | 40 ++++++++++++++++++++++++++++++++++------
5 files changed, 65 insertions(+), 21 deletions(-)
diff --git a/builtin/rerere.c b/builtin/rerere.c
index 081fccc..9cbab27 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -147,7 +147,7 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
if (!strcmp(argv[0], "clear")) {
for (i = 0; i < merge_rr.nr; i++) {
const char *name = (const char *)merge_rr.items[i].util;
- if (!name)
+ if (name == RERERE_UTIL_PUNTED || name == RERERE_UTIL_STAGED)
continue;
if (!has_rerere_resolution(name))
unlink_rr_item(name);
@@ -162,13 +162,17 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
printf("%s\n", merge_rr.items[i].string);
}
else if (!strcmp(argv[0], "remaining"))
- for (i = 0; i < merge_rr.nr; i++)
+ for (i = 0; i < merge_rr.nr; i++) {
+ const char *name = (const char *)merge_rr.items[i].util;
+ if (name == RERERE_UTIL_STAGED)
+ continue;
printf("%s\n", merge_rr.items[i].string);
+ }
else if (!strcmp(argv[0], "diff"))
for (i = 0; i < merge_rr.nr; i++) {
const char *path = merge_rr.items[i].string;
const char *name = (const char *)merge_rr.items[i].util;
- if (!name)
+ if (name == RERERE_UTIL_PUNTED || name == RERERE_UTIL_STAGED)
continue;
diff_two(rerere_path(name, "preimage"), path, path, path);
}
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 2f8dc44..bacbda2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -269,7 +269,7 @@ rerere=false
files_to_merge() {
if test "$rerere" = true
then
- git rerere status
+ git rerere remaining
else
git ls-files -u | sed -e 's/^[^ ]* //' | sort -u
fi
diff --git a/rerere.c b/rerere.c
index eb47f97..61cac54 100644
--- a/rerere.c
+++ b/rerere.c
@@ -7,6 +7,10 @@
#include "ll-merge.h"
#include "attr.h"
+#define RERERE_UTIL_ELIGIBLE NULL
+void *RERERE_UTIL_PUNTED = &RERERE_UTIL_PUNTED;
+void *RERERE_UTIL_STAGED = &RERERE_UTIL_STAGED;
+
/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
static int rerere_enabled = -1;
@@ -352,18 +356,20 @@ static int find_conflict(struct string_list *conflict)
return error("Could not read index");
/*
- * Collect paths with conflict, mark them with NULL (punted) or
- * !NULL (eligible) in their ->util field.
+ * Collect paths with conflict, mark them according to type in
+ * their ->util field.
*/
for (i = 0; i < active_nr; i++) {
struct cache_entry *e = active_cache[i];
struct string_list_item *it;
- if (!ce_stage(e))
+ if (!ce_stage(e)) {
continue;
+ }
it = string_list_insert(conflict, (const char *)e->name);
- it->util = NULL;
+ it->util = RERERE_UTIL_PUNTED;
if (ce_stage(e) == 1) {
+ it->util = RERERE_UTIL_STAGED;
if (active_nr <= ++i)
break;
}
@@ -377,7 +383,7 @@ static int find_conflict(struct string_list *conflict)
ce_same_name(e, e3) &&
S_ISREG(e2->ce_mode) &&
S_ISREG(e3->ce_mode))
- it->util = (void *) 1;
+ it->util = RERERE_UTIL_ELIGIBLE;
}
/* Skip the entries with the same name */
@@ -395,9 +401,10 @@ static void add_punted(struct string_list *merge_rr)
find_conflict(&conflict);
for (i = 0; i < conflict.nr; i++) {
- if (conflict.items[i].util)
+ if (conflict.items[i].util == RERERE_UTIL_ELIGIBLE)
continue;
- string_list_insert(merge_rr, conflict.items[i].string);
+ string_list_insert(merge_rr, conflict.items[i].string)->util =
+ conflict.items[i].util;
}
}
@@ -487,8 +494,9 @@ static int do_plain_rerere(struct string_list *rr, int fd)
for (i = 0; i < conflict.nr; i++) {
const char *path = conflict.items[i].string;
- if (!conflict.items[i].util)
- continue; /* punted */
+ if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
+ conflict.items[i].util == RERERE_UTIL_STAGED)
+ continue;
if (!string_list_has_string(rr, path)) {
unsigned char sha1[20];
char *hex;
@@ -648,8 +656,9 @@ int rerere_forget(const char **pathspec)
find_conflict(&conflict);
for (i = 0; i < conflict.nr; i++) {
struct string_list_item *it = &conflict.items[i];
- if (!conflict.items[i].util)
- continue; /* punted */
+ if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
+ conflict.items[i].util == RERERE_UTIL_STAGED)
+ continue;
if (!match_pathspec(pathspec, it->string, strlen(it->string),
0, NULL))
continue;
diff --git a/rerere.h b/rerere.h
index eaa9004..107a2bc 100644
--- a/rerere.h
+++ b/rerere.h
@@ -6,6 +6,9 @@
#define RERERE_AUTOUPDATE 01
#define RERERE_NOAUTOUPDATE 02
+extern void *RERERE_UTIL_PUNTED;
+extern void *RERERE_UTIL_STAGED;
+
extern int setup_rerere(struct string_list *, int);
extern int rerere(int);
extern const char *rerere_path(const char *hex, const char *file);
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index d78bdec..dc838c9 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -16,23 +16,33 @@ Testing basic merge tool invocation'
test_expect_success 'setup' '
git config rerere.enabled true &&
echo master >file1 &&
+ echo master file11 >file11 &&
+ echo master file12 >file12 &&
+ echo master file13 >file13 &&
+ echo master file14 >file14 &&
mkdir subdir &&
echo master sub >subdir/file3 &&
- git add file1 subdir/file3 &&
- git commit -m "added file1" &&
+ git add file1 file1[1-4] subdir/file3 &&
+ git commit -m "add initial versions" &&
git checkout -b branch1 master &&
echo branch1 change >file1 &&
echo branch1 newfile >file2 &&
+ echo branch1 change file11 >file11 &&
+ echo branch1 change file13 >file13 &&
echo branch1 sub >subdir/file3 &&
- git add file1 file2 subdir/file3 &&
+ git add file1 file11 file13 file2 subdir/file3 &&
+ git rm file12 &&
git commit -m "branch1 changes" &&
git checkout master &&
echo master updated >file1 &&
echo master new >file2 &&
+ echo master updated file12 >file12 &&
+ echo master updated file14 >file14 &&
echo master new sub >subdir/file3 &&
- git add file1 file2 subdir/file3 &&
+ git add file1 file12 file14 file2 subdir/file3 &&
+ git rm file11 &&
git commit -m "master updates" &&
git config merge.tool mytool &&
@@ -46,6 +56,8 @@ test_expect_success 'custom mergetool' '
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
test "$(cat file1)" = "master updated" &&
test "$(cat file2)" = "master new" &&
test "$(cat subdir/file3)" = "master new sub" &&
@@ -59,6 +71,8 @@ test_expect_success 'mergetool crlf' '
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -82,6 +96,8 @@ test_expect_success 'mergetool on file in parent dir' '
cd subdir &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
test "$(cat ../file1)" = "master updated" &&
test "$(cat ../file2)" = "master new" &&
git commit -m "branch1 resolved with mergetool - subdir"
@@ -92,6 +108,8 @@ test_expect_success 'mergetool skips autoresolved' '
git checkout -b test4 branch1 &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
output="$(git mergetool --no-prompt)" &&
test "$output" = "No files need merging" &&
git reset --hard
@@ -102,13 +120,23 @@ test_expect_success 'mergetool merges all from subdir' '
cd subdir &&
git config rerere.enabled false &&
test_must_fail git merge master &&
- git mergetool --no-prompt &&
+ ( yes "d" "d" | git mergetool --no-prompt ) &&
test "$(cat ../file1)" = "master updated" &&
test "$(cat ../file2)" = "master new" &&
test "$(cat file3)" = "master new sub" &&
- git add ../file1 ../file2 file3 &&
git commit -m "branch2 resolved by mergetool from subdir"
)
'
+test_expect_success 'mergetool skips resolved paths when rerere is active' '
+ git config rerere.enabled true &&
+ rm -rf .git/rr-cache &&
+ git checkout -b test5 branch1
+ test_must_fail git merge master >/dev/null 2>&1 &&
+ ( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+ output="$(yes "n" | git mergetool --no-prompt)" &&
+ test "$output" = "No files need merging" &&
+ git reset --hard
+'
+
test_done
--
1.7.4.rc2.33.g8a14f
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] mergetool: don't skip modify/remove conflicts
2011-02-08 3:08 [PATCH] mergetool: don't skip modify/remove conflicts Martin von Zweigbergk
@ 2011-02-09 21:45 ` Junio C Hamano
2011-02-11 2:46 ` Martin von Zweigbergk
2011-02-13 4:09 ` [PATCH v2 0/2] " Martin von Zweigbergk
1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2011-02-09 21:45 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git
Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> diff --git a/rerere.h b/rerere.h
> index eaa9004..107a2bc 100644
> --- a/rerere.h
> +++ b/rerere.h
> @@ -6,6 +6,9 @@
> #define RERERE_AUTOUPDATE 01
> #define RERERE_NOAUTOUPDATE 02
>
> +extern void *RERERE_UTIL_PUNTED;
This is for paths without three-stages, i.e. ones rerere won't help.
Needs some comment for these two symbols.
Leading "RERERE_" is necessary for clarifying the namespace, PUNTED is
necessary because it defines what the symbols means, but why do we need
UTIL in the name? Drop it.
> +extern void *RERERE_UTIL_STAGED;
This is for what kind? The contents on the filesystem is ready to go,
added to the index, but still in MERGE_RR (i.e. "git rerere" not run yet)?
Is the real problem that git-mergetool is not running rerere when it
should, I wonder...
> diff --git a/rerere.c b/rerere.c
> index eb47f97..61cac54 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -7,6 +7,10 @@
> #include "ll-merge.h"
> #include "attr.h"
>
> +#define RERERE_UTIL_ELIGIBLE NULL
> +void *RERERE_UTIL_PUNTED = &RERERE_UTIL_PUNTED;
> +void *RERERE_UTIL_STAGED = &RERERE_UTIL_STAGED;
> +
Same for "ELIGIBLE"; describe what it means.
> for (i = 0; i < active_nr; i++) {
> struct cache_entry *e = active_cache[i];
> struct string_list_item *it;
>
> - if (!ce_stage(e))
> + if (!ce_stage(e)) {
> continue;
> + }
Unnecessary change.
> it = string_list_insert(conflict, (const char *)e->name);
> - it->util = NULL;
> + it->util = RERERE_UTIL_PUNTED;
> if (ce_stage(e) == 1) {
> + it->util = RERERE_UTIL_STAGED;
Hmm, I thought that you were taling about paths that the user
hand-resolved and then ran "git add" on. Why is this marked "STAGED"?
Either your logic is wrong, or the name of the symbol is.
In any case, the original code is letting rerere handle both two-way
merge (stage #2 and #3 exist without state #1) and three-way merge (all
three stages exist) case, and changing only the body of this if statement
smells there is extremely fishy going on.
> @@ -487,8 +494,9 @@ static int do_plain_rerere(struct string_list *rr, int fd)
>
> for (i = 0; i < conflict.nr; i++) {
> const char *path = conflict.items[i].string;
> - if (!conflict.items[i].util)
> - continue; /* punted */
> + if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
> + conflict.items[i].util == RERERE_UTIL_STAGED)
> + continue;
> if (!string_list_has_string(rr, path)) {
> unsigned char sha1[20];
> char *hex;
> @@ -648,8 +656,9 @@ int rerere_forget(const char **pathspec)
> find_conflict(&conflict);
> for (i = 0; i < conflict.nr; i++) {
> struct string_list_item *it = &conflict.items[i];
> - if (!conflict.items[i].util)
> - continue; /* punted */
> + if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
> + conflict.items[i].util == RERERE_UTIL_STAGED)
> + continue;
There are a few repetition of "if it is marked with PUNTED or STAGED"; can
you make it into a small helper function and give it a _meaningful_ name?
What does it mean for an entry to be marked with either of these marks?
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] mergetool: don't skip modify/remove conflicts
2011-02-09 21:45 ` Junio C Hamano
@ 2011-02-11 2:46 ` Martin von Zweigbergk
0 siblings, 0 replies; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-11 2:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Martin von Zweigbergk, git
On Wed, 9 Feb 2011, Junio C Hamano wrote:
> Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
>
> > +extern void *RERERE_UTIL_STAGED;
>
> This is for what kind? The contents on the filesystem is ready to go,
> added to the index, but still in MERGE_RR (i.e. "git rerere" not run yet)?
Correct.
> Is the real problem that git-mergetool is not running rerere when it
> should, I wonder...
You mean that maybe it should call rerere for files that are added to
the index?
> > it = string_list_insert(conflict, (const char *)e->name);
> > - it->util = NULL;
> > + it->util = RERERE_UTIL_PUNTED;
> > if (ce_stage(e) == 1) {
> > + it->util = RERERE_UTIL_STAGED;
>
> Hmm, I thought that you were taling about paths that the user
> hand-resolved and then ran "git add" on. Why is this marked "STAGED"?
Very good question. It shouldn't.
> Either your logic is wrong, or the name of the symbol is.
The former. What I really wanted to check is if the file has been
added to the index. I am currently struggling with how to do this
correctly. My inexperience with the git internals makes it a very slow
process with a lot of guesses and trial and error.
> > - if (!conflict.items[i].util)
> > - continue; /* punted */
> > + if (conflict.items[i].util == RERERE_UTIL_PUNTED ||
> > + conflict.items[i].util == RERERE_UTIL_STAGED)
> > + continue;
>
> There are a few repetition of "if it is marked with PUNTED or STAGED"; can
> you make it into a small helper function and give it a _meaningful_ name?
> What does it mean for an entry to be marked with either of these marks?
There is another big problem here. I had not realized that the util
field would be freed later (being spoilt by GCing languages, I guess)
and that will of course fail if it is one of the two constants I
defined. I stupidly did not run the commands manually, but relied only
on the test cases, so I didn't notice the segfault at the end. Somehow
all the test cases still pass.
I think this means that anyone intending to run rerere should not
build pu at the moment, right? I'm really sorry about the
inconvenience :-(.
/Martin
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 0/2] mergetool: don't skip modify/remove conflicts
2011-02-08 3:08 [PATCH] mergetool: don't skip modify/remove conflicts Martin von Zweigbergk
2011-02-09 21:45 ` Junio C Hamano
@ 2011-02-13 4:09 ` Martin von Zweigbergk
2011-02-13 4:09 ` [PATCH v2 1/2] " Martin von Zweigbergk
2011-02-13 4:09 ` [PATCH v2 2/2] rerere: factor out common conflict search code Martin von Zweigbergk
1 sibling, 2 replies; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-13 4:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Thomas Rast, Martin von Zweigbergk
Trying again. I hope it looks better this time. The second patch could
be squashed with the first one.
This applies on top of jc/rerere-remaining.
Martin von Zweigbergk (2):
mergetool: don't skip modify/remove conflicts
rerere: factor out common conflict search code
builtin/rerere.c | 21 +++++-----
git-mergetool.sh | 2 +-
rerere.c | 109 +++++++++++++++++++++++++++++---------------------
rerere.h | 8 ++++
t/t7610-mergetool.sh | 40 +++++++++++++++---
5 files changed, 117 insertions(+), 63 deletions(-)
--
1.7.4.rc2.33.g8a14f
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] mergetool: don't skip modify/remove conflicts
2011-02-13 4:09 ` [PATCH v2 0/2] " Martin von Zweigbergk
@ 2011-02-13 4:09 ` Martin von Zweigbergk
2011-02-15 0:54 ` Junio C Hamano
2011-02-13 4:09 ` [PATCH v2 2/2] rerere: factor out common conflict search code Martin von Zweigbergk
1 sibling, 1 reply; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-13 4:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Thomas Rast, Martin von Zweigbergk
Since bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17),
mergetool uses different ways of figuring out the list of files with
merge conflicts depending on whether rerere is active. If rerere is
active, mergetool will use 'git rerere status' to list the files with
remaining conflicts. However, the output from that command does not
list conflicts of types that rerere does not handle, such as
modify/remove conflicts.
Another problem with solely relying on the output from 'git rerere
status' is that, for new conflicts that are not yet known to rerere,
the output from the command will list the files even after adding them
to the index. This means that if the conflicts in some files have been
resolved and 'git mergetool' is run again, it will ask the user
something like the following for each of those files.
file1: file does not need merging
Continue merging other unresolved paths (y/n) ?
Solve the first of these problems by replacing the call to 'git rerere
status' with a call to the new 'git rerere remaining' that was
introduced in the previous commit. Solve the second problem by
modifying 'git rerere remaining' not to output files that have already
been staged.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
Ideally, I would have liked to just remove the resolved elements from
the string_list, but since there is no function that drops an element,
I marked it by pointing its util field to a special address. Is this a
good way of doing it or can/should I manually unlink the element from
the list instead?
builtin/rerere.c | 21 +++++++++++----------
git-mergetool.sh | 2 +-
rerere.c | 24 ++++++++++++++++++++----
rerere.h | 8 ++++++++
t/t7610-mergetool.sh | 40 ++++++++++++++++++++++++++++++++++------
5 files changed, 74 insertions(+), 21 deletions(-)
diff --git a/builtin/rerere.c b/builtin/rerere.c
index 081fccc..7b9fe18 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -147,8 +147,6 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
if (!strcmp(argv[0], "clear")) {
for (i = 0; i < merge_rr.nr; i++) {
const char *name = (const char *)merge_rr.items[i].util;
- if (!name)
- continue;
if (!has_rerere_resolution(name))
unlink_rr_item(name);
}
@@ -157,19 +155,22 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
garbage_collect(&merge_rr);
else if (!strcmp(argv[0], "status"))
for (i = 0; i < merge_rr.nr; i++) {
- if (!merge_rr.items[i].util)
- continue;
printf("%s\n", merge_rr.items[i].string);
}
- else if (!strcmp(argv[0], "remaining"))
- for (i = 0; i < merge_rr.nr; i++)
- printf("%s\n", merge_rr.items[i].string);
- else if (!strcmp(argv[0], "diff"))
+ else if (!strcmp(argv[0], "remaining")) {
+ rerere_remaining(&merge_rr);
+ for (i = 0; i < merge_rr.nr; i++) {
+ if (merge_rr.items[i].util != RERERE_RESOLVED)
+ printf("%s\n", merge_rr.items[i].string);
+ else
+ /* prepare for later call to
+ * string_list_clear() */
+ merge_rr.items[i].util = NULL;
+ }
+ } else if (!strcmp(argv[0], "diff"))
for (i = 0; i < merge_rr.nr; i++) {
const char *path = merge_rr.items[i].string;
const char *name = (const char *)merge_rr.items[i].util;
- if (!name)
- continue;
diff_two(rerere_path(name, "preimage"), path, path, path);
}
else
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 2f8dc44..bacbda2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -269,7 +269,7 @@ rerere=false
files_to_merge() {
if test "$rerere" = true
then
- git rerere status
+ git rerere remaining
else
git ls-files -u | sed -e 's/^[^ ]* //' | sort -u
fi
diff --git a/rerere.c b/rerere.c
index eb47f97..e5bccd5 100644
--- a/rerere.c
+++ b/rerere.c
@@ -7,6 +7,8 @@
#include "ll-merge.h"
#include "attr.h"
+void *RERERE_RESOLVED = &RERERE_RESOLVED;
+
/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
static int rerere_enabled = -1;
@@ -388,16 +390,31 @@ static int find_conflict(struct string_list *conflict)
return 0;
}
-static void add_punted(struct string_list *merge_rr)
+void rerere_remaining(struct string_list *merge_rr)
{
int i;
struct string_list conflict = STRING_LIST_INIT_DUP;
+ /* Add punted paths */
find_conflict(&conflict);
for (i = 0; i < conflict.nr; i++) {
- if (conflict.items[i].util)
+ if (!conflict.items[i].util)
+ string_list_insert(merge_rr, conflict.items[i].string);
+ }
+
+ /* Mark already resolved paths */
+ for (i = 0; i < active_nr; i++) {
+ struct cache_entry *e = active_cache[i];
+
+ if (!ce_stage(e)) {
+ struct string_list_item *it;
+ it = string_list_lookup(merge_rr, (const char *)e->name);
+ if (it != NULL) {
+ free(it->util);
+ it->util = RERERE_RESOLVED;
+ }
continue;
- string_list_insert(merge_rr, conflict.items[i].string);
+ }
}
}
@@ -592,7 +609,6 @@ int setup_rerere(struct string_list *merge_rr, int flags)
fd = hold_lock_file_for_update(&write_lock, merge_rr_path,
LOCK_DIE_ON_ERROR);
read_rr(merge_rr);
- add_punted(merge_rr);
return fd;
}
diff --git a/rerere.h b/rerere.h
index eaa9004..ea2618b 100644
--- a/rerere.h
+++ b/rerere.h
@@ -6,11 +6,19 @@
#define RERERE_AUTOUPDATE 01
#define RERERE_NOAUTOUPDATE 02
+/*
+ * Marks paths that have been hand-resolved and added to the
+ * index. Set in the util field of such paths after calling
+ * rerere_remaining.
+ */
+extern void *RERERE_RESOLVED;
+
extern int setup_rerere(struct string_list *, int);
extern int rerere(int);
extern const char *rerere_path(const char *hex, const char *file);
extern int has_rerere_resolution(const char *hex);
extern int rerere_forget(const char **);
+extern void rerere_remaining(struct string_list *);
#define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
"update the index with reused conflict resolution if possible")
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index d78bdec..dc838c9 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -16,23 +16,33 @@ Testing basic merge tool invocation'
test_expect_success 'setup' '
git config rerere.enabled true &&
echo master >file1 &&
+ echo master file11 >file11 &&
+ echo master file12 >file12 &&
+ echo master file13 >file13 &&
+ echo master file14 >file14 &&
mkdir subdir &&
echo master sub >subdir/file3 &&
- git add file1 subdir/file3 &&
- git commit -m "added file1" &&
+ git add file1 file1[1-4] subdir/file3 &&
+ git commit -m "add initial versions" &&
git checkout -b branch1 master &&
echo branch1 change >file1 &&
echo branch1 newfile >file2 &&
+ echo branch1 change file11 >file11 &&
+ echo branch1 change file13 >file13 &&
echo branch1 sub >subdir/file3 &&
- git add file1 file2 subdir/file3 &&
+ git add file1 file11 file13 file2 subdir/file3 &&
+ git rm file12 &&
git commit -m "branch1 changes" &&
git checkout master &&
echo master updated >file1 &&
echo master new >file2 &&
+ echo master updated file12 >file12 &&
+ echo master updated file14 >file14 &&
echo master new sub >subdir/file3 &&
- git add file1 file2 subdir/file3 &&
+ git add file1 file12 file14 file2 subdir/file3 &&
+ git rm file11 &&
git commit -m "master updates" &&
git config merge.tool mytool &&
@@ -46,6 +56,8 @@ test_expect_success 'custom mergetool' '
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
test "$(cat file1)" = "master updated" &&
test "$(cat file2)" = "master new" &&
test "$(cat subdir/file3)" = "master new sub" &&
@@ -59,6 +71,8 @@ test_expect_success 'mergetool crlf' '
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -82,6 +96,8 @@ test_expect_success 'mergetool on file in parent dir' '
cd subdir &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
test "$(cat ../file1)" = "master updated" &&
test "$(cat ../file2)" = "master new" &&
git commit -m "branch1 resolved with mergetool - subdir"
@@ -92,6 +108,8 @@ test_expect_success 'mergetool skips autoresolved' '
git checkout -b test4 branch1 &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
output="$(git mergetool --no-prompt)" &&
test "$output" = "No files need merging" &&
git reset --hard
@@ -102,13 +120,23 @@ test_expect_success 'mergetool merges all from subdir' '
cd subdir &&
git config rerere.enabled false &&
test_must_fail git merge master &&
- git mergetool --no-prompt &&
+ ( yes "d" "d" | git mergetool --no-prompt ) &&
test "$(cat ../file1)" = "master updated" &&
test "$(cat ../file2)" = "master new" &&
test "$(cat file3)" = "master new sub" &&
- git add ../file1 ../file2 file3 &&
git commit -m "branch2 resolved by mergetool from subdir"
)
'
+test_expect_success 'mergetool skips resolved paths when rerere is active' '
+ git config rerere.enabled true &&
+ rm -rf .git/rr-cache &&
+ git checkout -b test5 branch1
+ test_must_fail git merge master >/dev/null 2>&1 &&
+ ( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+ output="$(yes "n" | git mergetool --no-prompt)" &&
+ test "$output" = "No files need merging" &&
+ git reset --hard
+'
+
test_done
--
1.7.4.rc2.33.g8a14f
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] rerere: factor out common conflict search code
2011-02-13 4:09 ` [PATCH v2 0/2] " Martin von Zweigbergk
2011-02-13 4:09 ` [PATCH v2 1/2] " Martin von Zweigbergk
@ 2011-02-13 4:09 ` Martin von Zweigbergk
1 sibling, 0 replies; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-13 4:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Thomas Rast, Martin von Zweigbergk
A little bit of the code in the new rerere_remaining function is
shared with find_conflicts. Factor this out into a new function that
is called from the two mentioned functions. Apart from reducing code
duplication, it also means that the callers of find_conflict do not
need to handle the case when the util pointer is NULL, because that is
only set in rerere_remaining now.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
rerere.c | 105 +++++++++++++++++++++++++++++++------------------------------
rerere.h | 2 +-
2 files changed, 54 insertions(+), 53 deletions(-)
diff --git a/rerere.c b/rerere.c
index e5bccd5..d14fd9a 100644
--- a/rerere.c
+++ b/rerere.c
@@ -7,6 +7,9 @@
#include "ll-merge.h"
#include "attr.h"
+#define RESOLVED 0
+#define PUNTED 1
+#define THREE_STAGED 2
void *RERERE_RESOLVED = &RERERE_RESOLVED;
/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
@@ -347,75 +350,79 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
return hunk_no;
}
+
+static int check_one_conflict(int i, int *type)
+{
+ struct cache_entry *e = active_cache[i];
+ struct string_list_item *it;
+
+ if (!ce_stage(e)) {
+ *type = RESOLVED;
+ return i + 1;
+ }
+
+ *type = PUNTED;
+ if (ce_stage(e) == 1) {
+ if (active_nr <= ++i)
+ return i + 1;
+ }
+
+ /* Only handle regular files with both stages #2 and #3 */
+ if (i + 1 < active_nr) {
+ struct cache_entry *e2 = active_cache[i];
+ struct cache_entry *e3 = active_cache[i + 1];
+ if (ce_stage(e2) == 2 &&
+ ce_stage(e3) == 3 &&
+ ce_same_name(e, e3) &&
+ S_ISREG(e2->ce_mode) &&
+ S_ISREG(e3->ce_mode))
+ *type = THREE_STAGED;
+ }
+
+ /* Skip the entries with the same name */
+ while (i < active_nr && ce_same_name(e, active_cache[i]))
+ i++;
+ return i;
+}
+
static int find_conflict(struct string_list *conflict)
{
int i;
if (read_cache() < 0)
return error("Could not read index");
- /*
- * Collect paths with conflict, mark them with NULL (punted) or
- * !NULL (eligible) in their ->util field.
- */
- for (i = 0; i < active_nr; i++) {
+ for (i = 0; i < active_nr;) {
+ int conflict_type;
struct cache_entry *e = active_cache[i];
- struct string_list_item *it;
-
- if (!ce_stage(e))
- continue;
- it = string_list_insert(conflict, (const char *)e->name);
- it->util = NULL;
- if (ce_stage(e) == 1) {
- if (active_nr <= ++i)
- break;
- }
-
- /* Only handle regular files with both stages #2 and #3 */
- if (i + 1 < active_nr) {
- struct cache_entry *e2 = active_cache[i];
- struct cache_entry *e3 = active_cache[i + 1];
- if (ce_stage(e2) == 2 &&
- ce_stage(e3) == 3 &&
- ce_same_name(e, e3) &&
- S_ISREG(e2->ce_mode) &&
- S_ISREG(e3->ce_mode))
- it->util = (void *) 1;
- }
-
- /* Skip the entries with the same name */
- while (i < active_nr && ce_same_name(e, active_cache[i]))
- i++;
- i--; /* compensate for the outer loop */
+ i = check_one_conflict(i, &conflict_type);
+ if (conflict_type == THREE_STAGED)
+ string_list_insert(conflict, (const char *)e->name);
}
return 0;
}
-void rerere_remaining(struct string_list *merge_rr)
+int rerere_remaining(struct string_list *merge_rr)
{
int i;
- struct string_list conflict = STRING_LIST_INIT_DUP;
-
- /* Add punted paths */
- find_conflict(&conflict);
- for (i = 0; i < conflict.nr; i++) {
- if (!conflict.items[i].util)
- string_list_insert(merge_rr, conflict.items[i].string);
- }
+ if (read_cache() < 0)
+ return error("Could not read index");
- /* Mark already resolved paths */
- for (i = 0; i < active_nr; i++) {
+ for (i = 0; i < active_nr;) {
+ int conflict_type;
struct cache_entry *e = active_cache[i];
-
- if (!ce_stage(e)) {
+ i = check_one_conflict(i, &conflict_type);
+ if (conflict_type == PUNTED)
+ string_list_insert(merge_rr, (const char *)e->name);
+ else if (conflict_type == RESOLVED) {
struct string_list_item *it;
it = string_list_lookup(merge_rr, (const char *)e->name);
if (it != NULL) {
free(it->util);
it->util = RERERE_RESOLVED;
}
- continue;
}
}
+ return 0;
}
static int merge(const char *name, const char *path)
@@ -504,8 +511,6 @@ static int do_plain_rerere(struct string_list *rr, int fd)
for (i = 0; i < conflict.nr; i++) {
const char *path = conflict.items[i].string;
- if (!conflict.items[i].util)
- continue; /* punted */
if (!string_list_has_string(rr, path)) {
unsigned char sha1[20];
char *hex;
@@ -533,8 +538,6 @@ static int do_plain_rerere(struct string_list *rr, int fd)
const char *path = rr->items[i].string;
const char *name = (const char *)rr->items[i].util;
- if (!name)
- continue;
if (has_rerere_resolution(name)) {
if (!merge(name, path)) {
if (rerere_autoupdate)
@@ -664,8 +667,6 @@ int rerere_forget(const char **pathspec)
find_conflict(&conflict);
for (i = 0; i < conflict.nr; i++) {
struct string_list_item *it = &conflict.items[i];
- if (!conflict.items[i].util)
- continue; /* punted */
if (!match_pathspec(pathspec, it->string, strlen(it->string),
0, NULL))
continue;
diff --git a/rerere.h b/rerere.h
index ea2618b..595f49f 100644
--- a/rerere.h
+++ b/rerere.h
@@ -18,7 +18,7 @@ extern int rerere(int);
extern const char *rerere_path(const char *hex, const char *file);
extern int has_rerere_resolution(const char *hex);
extern int rerere_forget(const char **);
-extern void rerere_remaining(struct string_list *);
+extern int rerere_remaining(struct string_list *);
#define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
"update the index with reused conflict resolution if possible")
--
1.7.4.rc2.33.g8a14f
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mergetool: don't skip modify/remove conflicts
2011-02-13 4:09 ` [PATCH v2 1/2] " Martin von Zweigbergk
@ 2011-02-15 0:54 ` Junio C Hamano
2011-02-15 3:30 ` Martin von Zweigbergk
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2011-02-15 0:54 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git, Thomas Rast
Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> diff --git a/builtin/rerere.c b/builtin/rerere.c
> index 081fccc..7b9fe18 100644
> --- a/builtin/rerere.c
> +++ b/builtin/rerere.c
> @@ -147,8 +147,6 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
> if (!strcmp(argv[0], "clear")) {
> for (i = 0; i < merge_rr.nr; i++) {
> const char *name = (const char *)merge_rr.items[i].util;
> - if (!name)
> - continue;
> if (!has_rerere_resolution(name))
> unlink_rr_item(name);
> }
> @@ -157,19 +155,22 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
> garbage_collect(&merge_rr);
> else if (!strcmp(argv[0], "status"))
> for (i = 0; i < merge_rr.nr; i++) {
> - if (!merge_rr.items[i].util)
> - continue;
> printf("%s\n", merge_rr.items[i].string);
> }
> - else if (!strcmp(argv[0], "remaining"))
> - for (i = 0; i < merge_rr.nr; i++)
> - printf("%s\n", merge_rr.items[i].string);
> - else if (!strcmp(argv[0], "diff"))
> + else if (!strcmp(argv[0], "remaining")) {
> + rerere_remaining(&merge_rr);
> + for (i = 0; i < merge_rr.nr; i++) {
> + if (merge_rr.items[i].util != RERERE_RESOLVED)
> + printf("%s\n", merge_rr.items[i].string);
> + else
> + /* prepare for later call to
> + * string_list_clear() */
> + merge_rr.items[i].util = NULL;
> + }
> + } else if (!strcmp(argv[0], "diff"))
> for (i = 0; i < merge_rr.nr; i++) {
> const char *path = merge_rr.items[i].string;
> const char *name = (const char *)merge_rr.items[i].util;
> - if (!name)
> - continue;
> diff_two(rerere_path(name, "preimage"), path, path, path);
> }
> else
By looking at the diff between the parent of f322b35 (my earlier "rerere
remaining" patch) and the result of applying this patch on top of the
patch, I think the basic idea of this is to correct my stupid mistake of
contaminating merge_rr unconditionally, and instead adding the "remaining"
entries only when handling the "rerere remaining" command (hence many
removal of special cases "if (!name) continue" introduced by my patch.
The result looks _much_ cleaner than the above patch shows and I am happy
with it.
But shouldn't you also revert the parts of my patch to do_plain_rerere()
and rerere_forget() that have similar special cases?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mergetool: don't skip modify/remove conflicts
2011-02-15 0:54 ` Junio C Hamano
@ 2011-02-15 3:30 ` Martin von Zweigbergk
2011-02-15 20:12 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-15 3:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Martin von Zweigbergk, git, Thomas Rast
On Mon, 14 Feb 2011, Junio C Hamano wrote:
> But shouldn't you also revert the parts of my patch to do_plain_rerere()
> and rerere_forget() that have similar special cases?
Yep, done in patch 2/2.
Your changes in find_conflicts() are still there after patch 1/2 and
since do_plain_rerere() and rerere_forget() call find_conflicts(), I
think they need to check for the punted paths. Only the callers of
setup_rerere() no longer need to care.
In patch 2/2, I refactored a bit more, so that find_conflicts()
behaves the way it did before your first patch, and then the checks
for punted paths are removed from the callers.
If you think the second one is good, it could easily be squashed with
the first one. I left it separate because I wasn't quite sure. For
example, could the interface to check_one_conflict() be improved? As
you said, part of the changes from your patch are now undone, so I
guess all three patches could even be squashed.
/Martin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] mergetool: don't skip modify/remove conflicts
2011-02-15 3:30 ` Martin von Zweigbergk
@ 2011-02-15 20:12 ` Junio C Hamano
2011-02-16 10:47 ` [PATCH v3 0/2] " Martin von Zweigbergk
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2011-02-15 20:12 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git, Thomas Rast
Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> On Mon, 14 Feb 2011, Junio C Hamano wrote:
>
>> But shouldn't you also revert the parts of my patch to do_plain_rerere()
>> and rerere_forget() that have similar special cases?
>
> Yep, done in patch 2/2.
>
> Your changes in find_conflicts() are still there after patch 1/2 and
> since do_plain_rerere() and rerere_forget() call find_conflicts(), I
> think they need to check for the punted paths. Only the callers of
> setup_rerere() no longer need to care.
>
> In patch 2/2, I refactored a bit more, so that find_conflicts()
> behaves the way it did before your first patch, and then the checks
> for punted paths are removed from the callers.
>
> If you think the second one is good, it could easily be squashed with
> the first one. I left it separate because I wasn't quite sure.
Probably the changes to rerere proper from all three patches should be
squashed into one, and then the patch to the script to use the new feature
can be applied on top of it.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 0/2] mergetool: don't skip modify/remove conflicts
2011-02-15 20:12 ` Junio C Hamano
@ 2011-02-16 10:47 ` Martin von Zweigbergk
2011-02-16 10:47 ` [PATCH v3 1/2] rerere "remaining" Martin von Zweigbergk
2011-02-16 10:47 ` [PATCH v3 2/2] mergetool: don't skip modify/remove conflicts Martin von Zweigbergk
0 siblings, 2 replies; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-16 10:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Thomas Rast, Martin von Zweigbergk
Third round. Reorganized the patches a bit as suggested by Junio. The
first patch now contains all the changes to rerere and the second one
only makes mergetool call 'rerere remaining'.
The only tiny change to the end result is the removal of a blank line.
The series applies on top of master.
Junio C Hamano (1):
rerere "remaining"
Martin von Zweigbergk (1):
mergetool: don't skip modify/remove conflicts
builtin/rerere.c | 17 +++++++++--
git-mergetool.sh | 2 +-
rerere.c | 79 +++++++++++++++++++++++++++++++++++++++++++------
rerere.h | 8 +++++
t/t7610-mergetool.sh | 40 +++++++++++++++++++++----
5 files changed, 126 insertions(+), 20 deletions(-)
--
1.7.4.rc2.33.g8a14f
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3 1/2] rerere "remaining"
2011-02-16 10:47 ` [PATCH v3 0/2] " Martin von Zweigbergk
@ 2011-02-16 10:47 ` Martin von Zweigbergk
2011-02-16 21:14 ` Junio C Hamano
2011-02-16 10:47 ` [PATCH v3 2/2] mergetool: don't skip modify/remove conflicts Martin von Zweigbergk
1 sibling, 1 reply; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-16 10:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Thomas Rast, Martin von Zweigbergk
From: Junio C Hamano <gitster@pobox.com>
After "rerere" resolves conflicts by reusing old resolution, there would
be three kinds of paths with conflict in the index:
* paths that have been resolved in the working tree by rerere;
* paths that need further work whose resolution could be recorded;
* paths that need resolving that rerere won't help.
When the user wants a list of paths that need hand-resolving, output from
"rerere status" does not help, as it shows only the second category, but
the paths in the third category still needs work (rerere only makes sense
for regular files that have both our side and their side, and does not
help other kinds of conflicts, e.g. "we modified, they deleted").
The new subcommand "rerere remaining" can be used to show both. As
opposed to "rerere status", this subcommand also skips printing paths
that have been added to the index, since these paths are already
resolved and are no longer "remaining".
Initial patch provided by Junio. Refactored and modified to skip
resolved paths by Martin. Commit message mostly by Junio.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
Junio, I wasn't sure how to handle the sign-off etc, so feel free to
modify those as appropriate.
builtin/rerere.c | 17 +++++++++--
rerere.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++-------
rerere.h | 8 +++++
3 files changed, 91 insertions(+), 13 deletions(-)
diff --git a/builtin/rerere.c b/builtin/rerere.c
index 642bf35..7b9fe18 100644
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -8,7 +8,7 @@
#include "xdiff-interface.h"
static const char * const rerere_usage[] = {
- "git rerere [clear | status | diff | gc]",
+ "git rerere [clear | status | remaining | diff | gc]",
NULL,
};
@@ -154,9 +154,20 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
} else if (!strcmp(argv[0], "gc"))
garbage_collect(&merge_rr);
else if (!strcmp(argv[0], "status"))
- for (i = 0; i < merge_rr.nr; i++)
+ for (i = 0; i < merge_rr.nr; i++) {
printf("%s\n", merge_rr.items[i].string);
- else if (!strcmp(argv[0], "diff"))
+ }
+ else if (!strcmp(argv[0], "remaining")) {
+ rerere_remaining(&merge_rr);
+ for (i = 0; i < merge_rr.nr; i++) {
+ if (merge_rr.items[i].util != RERERE_RESOLVED)
+ printf("%s\n", merge_rr.items[i].string);
+ else
+ /* prepare for later call to
+ * string_list_clear() */
+ merge_rr.items[i].util = NULL;
+ }
+ } else if (!strcmp(argv[0], "diff"))
for (i = 0; i < merge_rr.nr; i++) {
const char *path = merge_rr.items[i].string;
const char *name = (const char *)merge_rr.items[i].util;
diff --git a/rerere.c b/rerere.c
index d260843..1240d06 100644
--- a/rerere.c
+++ b/rerere.c
@@ -7,6 +7,11 @@
#include "ll-merge.h"
#include "attr.h"
+#define RESOLVED 0
+#define PUNTED 1
+#define THREE_STAGED 2
+void *RERERE_RESOLVED = &RERERE_RESOLVED;
+
/* if rerere_enabled == -1, fall back to detection of .git/rr-cache */
static int rerere_enabled = -1;
@@ -345,21 +350,75 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpu
return hunk_no;
}
-static int find_conflict(struct string_list *conflict)
+static int check_one_conflict(int i, int *type)
{
- int i;
- if (read_cache() < 0)
- return error("Could not read index");
- for (i = 0; i+1 < active_nr; i++) {
+ struct cache_entry *e = active_cache[i];
+ struct string_list_item *it;
+
+ if (!ce_stage(e)) {
+ *type = RESOLVED;
+ return i + 1;
+ }
+
+ *type = PUNTED;
+ if (ce_stage(e) == 1) {
+ if (active_nr <= ++i)
+ return i + 1;
+ }
+
+ /* Only handle regular files with both stages #2 and #3 */
+ if (i + 1 < active_nr) {
struct cache_entry *e2 = active_cache[i];
- struct cache_entry *e3 = active_cache[i+1];
+ struct cache_entry *e3 = active_cache[i + 1];
if (ce_stage(e2) == 2 &&
ce_stage(e3) == 3 &&
- ce_same_name(e2, e3) &&
+ ce_same_name(e, e3) &&
S_ISREG(e2->ce_mode) &&
- S_ISREG(e3->ce_mode)) {
- string_list_insert(conflict, (const char *)e2->name);
- i++; /* skip over both #2 and #3 */
+ S_ISREG(e3->ce_mode))
+ *type = THREE_STAGED;
+ }
+
+ /* Skip the entries with the same name */
+ while (i < active_nr && ce_same_name(e, active_cache[i]))
+ i++;
+ return i;
+}
+
+static int find_conflict(struct string_list *conflict)
+{
+ int i;
+ if (read_cache() < 0)
+ return error("Could not read index");
+
+ for (i = 0; i < active_nr;) {
+ int conflict_type;
+ struct cache_entry *e = active_cache[i];
+ i = check_one_conflict(i, &conflict_type);
+ if (conflict_type == THREE_STAGED)
+ string_list_insert(conflict, (const char *)e->name);
+ }
+ return 0;
+}
+
+int rerere_remaining(struct string_list *merge_rr)
+{
+ int i;
+ if (read_cache() < 0)
+ return error("Could not read index");
+
+ for (i = 0; i < active_nr;) {
+ int conflict_type;
+ struct cache_entry *e = active_cache[i];
+ i = check_one_conflict(i, &conflict_type);
+ if (conflict_type == PUNTED)
+ string_list_insert(merge_rr, (const char *)e->name);
+ else if (conflict_type == RESOLVED) {
+ struct string_list_item *it;
+ it = string_list_lookup(merge_rr, (const char *)e->name);
+ if (it != NULL) {
+ free(it->util);
+ it->util = RERERE_RESOLVED;
+ }
}
}
return 0;
diff --git a/rerere.h b/rerere.h
index eaa9004..595f49f 100644
--- a/rerere.h
+++ b/rerere.h
@@ -6,11 +6,19 @@
#define RERERE_AUTOUPDATE 01
#define RERERE_NOAUTOUPDATE 02
+/*
+ * Marks paths that have been hand-resolved and added to the
+ * index. Set in the util field of such paths after calling
+ * rerere_remaining.
+ */
+extern void *RERERE_RESOLVED;
+
extern int setup_rerere(struct string_list *, int);
extern int rerere(int);
extern const char *rerere_path(const char *hex, const char *file);
extern int has_rerere_resolution(const char *hex);
extern int rerere_forget(const char **);
+extern int rerere_remaining(struct string_list *);
#define OPT_RERERE_AUTOUPDATE(v) OPT_UYN(0, "rerere-autoupdate", (v), \
"update the index with reused conflict resolution if possible")
--
1.7.4.rc2.33.g8a14f
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 2/2] mergetool: don't skip modify/remove conflicts
2011-02-16 10:47 ` [PATCH v3 0/2] " Martin von Zweigbergk
2011-02-16 10:47 ` [PATCH v3 1/2] rerere "remaining" Martin von Zweigbergk
@ 2011-02-16 10:47 ` Martin von Zweigbergk
1 sibling, 0 replies; 13+ messages in thread
From: Martin von Zweigbergk @ 2011-02-16 10:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Thomas Rast, Martin von Zweigbergk
Since bb0a484 (mergetool: Skip autoresolved paths, 2010-08-17),
mergetool uses different ways of figuring out the list of files with
merge conflicts depending on whether rerere is active. If rerere is
active, mergetool will use 'git rerere status' to list the files with
remaining conflicts. However, the output from that command does not
list conflicts of types that rerere does not handle, such as
modify/remove conflicts.
Another problem with solely relying on the output from 'git rerere
status' is that, for new conflicts that are not yet known to rerere,
the output from the command will list the files even after adding them
to the index. This means that if the conflicts in some files have been
resolved and 'git mergetool' is run again, it will ask the user
something like the following for each of those files.
file1: file does not need merging
Continue merging other unresolved paths (y/n) ?
Solve both of these problems by replacing the call to 'git rerere
status' with a call to the new 'git rerere remaining' that was
introduced in the previous commit.
Signed-off-by: Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>
---
git-mergetool.sh | 2 +-
t/t7610-mergetool.sh | 40 ++++++++++++++++++++++++++++++++++------
2 files changed, 35 insertions(+), 7 deletions(-)
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 2f8dc44..bacbda2 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -269,7 +269,7 @@ rerere=false
files_to_merge() {
if test "$rerere" = true
then
- git rerere status
+ git rerere remaining
else
git ls-files -u | sed -e 's/^[^ ]* //' | sort -u
fi
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index d78bdec..dc838c9 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -16,23 +16,33 @@ Testing basic merge tool invocation'
test_expect_success 'setup' '
git config rerere.enabled true &&
echo master >file1 &&
+ echo master file11 >file11 &&
+ echo master file12 >file12 &&
+ echo master file13 >file13 &&
+ echo master file14 >file14 &&
mkdir subdir &&
echo master sub >subdir/file3 &&
- git add file1 subdir/file3 &&
- git commit -m "added file1" &&
+ git add file1 file1[1-4] subdir/file3 &&
+ git commit -m "add initial versions" &&
git checkout -b branch1 master &&
echo branch1 change >file1 &&
echo branch1 newfile >file2 &&
+ echo branch1 change file11 >file11 &&
+ echo branch1 change file13 >file13 &&
echo branch1 sub >subdir/file3 &&
- git add file1 file2 subdir/file3 &&
+ git add file1 file11 file13 file2 subdir/file3 &&
+ git rm file12 &&
git commit -m "branch1 changes" &&
git checkout master &&
echo master updated >file1 &&
echo master new >file2 &&
+ echo master updated file12 >file12 &&
+ echo master updated file14 >file14 &&
echo master new sub >subdir/file3 &&
- git add file1 file2 subdir/file3 &&
+ git add file1 file12 file14 file2 subdir/file3 &&
+ git rm file11 &&
git commit -m "master updates" &&
git config merge.tool mytool &&
@@ -46,6 +56,8 @@ test_expect_success 'custom mergetool' '
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
test "$(cat file1)" = "master updated" &&
test "$(cat file2)" = "master new" &&
test "$(cat subdir/file3)" = "master new sub" &&
@@ -59,6 +71,8 @@ test_expect_success 'mergetool crlf' '
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
test "$(printf x | cat file1 -)" = "$(printf "master updated\r\nx")" &&
test "$(printf x | cat file2 -)" = "$(printf "master new\r\nx")" &&
test "$(printf x | cat subdir/file3 -)" = "$(printf "master new sub\r\nx")" &&
@@ -82,6 +96,8 @@ test_expect_success 'mergetool on file in parent dir' '
cd subdir &&
( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) &&
test "$(cat ../file1)" = "master updated" &&
test "$(cat ../file2)" = "master new" &&
git commit -m "branch1 resolved with mergetool - subdir"
@@ -92,6 +108,8 @@ test_expect_success 'mergetool skips autoresolved' '
git checkout -b test4 branch1 &&
test_must_fail git merge master &&
test -n "$(git ls-files -u)" &&
+ ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
+ ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
output="$(git mergetool --no-prompt)" &&
test "$output" = "No files need merging" &&
git reset --hard
@@ -102,13 +120,23 @@ test_expect_success 'mergetool merges all from subdir' '
cd subdir &&
git config rerere.enabled false &&
test_must_fail git merge master &&
- git mergetool --no-prompt &&
+ ( yes "d" "d" | git mergetool --no-prompt ) &&
test "$(cat ../file1)" = "master updated" &&
test "$(cat ../file2)" = "master new" &&
test "$(cat file3)" = "master new sub" &&
- git add ../file1 ../file2 file3 &&
git commit -m "branch2 resolved by mergetool from subdir"
)
'
+test_expect_success 'mergetool skips resolved paths when rerere is active' '
+ git config rerere.enabled true &&
+ rm -rf .git/rr-cache &&
+ git checkout -b test5 branch1
+ test_must_fail git merge master >/dev/null 2>&1 &&
+ ( yes "d" "d" | git mergetool --no-prompt >/dev/null 2>&1 ) &&
+ output="$(yes "n" | git mergetool --no-prompt)" &&
+ test "$output" = "No files need merging" &&
+ git reset --hard
+'
+
test_done
--
1.7.4.rc2.33.g8a14f
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 1/2] rerere "remaining"
2011-02-16 10:47 ` [PATCH v3 1/2] rerere "remaining" Martin von Zweigbergk
@ 2011-02-16 21:14 ` Junio C Hamano
0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2011-02-16 21:14 UTC (permalink / raw)
To: Martin von Zweigbergk; +Cc: git, Thomas Rast
Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:
> Junio, I wasn't sure how to handle the sign-off etc, so feel free to
> modify those as appropriate.
At this point I think the primary value of the change is your adding
"rerere_remaining()" to only where it matters (i.e. the isolation of the
change) and you should get the credit as the primary author. Mine was
impossible to read because I contaminated merge_rr in a wrong place and
had to work it around in unrelated codepaths all over the place.
This was much cleaner and easier to read.
> builtin/rerere.c | 17 +++++++++--
> rerere.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++-------
> rerere.h | 8 +++++
> 3 files changed, 91 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/rerere.c b/builtin/rerere.c
> index 642bf35..7b9fe18 100644
> --- a/builtin/rerere.c
> +++ b/builtin/rerere.c
> @@ -8,7 +8,7 @@
> #include "xdiff-interface.h"
>
> static const char * const rerere_usage[] = {
> - "git rerere [clear | status | diff | gc]",
> + "git rerere [clear | status | remaining | diff | gc]",
> NULL,
> };
>
> @@ -154,9 +154,20 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)
> } else if (!strcmp(argv[0], "gc"))
> garbage_collect(&merge_rr);
> else if (!strcmp(argv[0], "status"))
> - for (i = 0; i < merge_rr.nr; i++)
> + for (i = 0; i < merge_rr.nr; i++) {
> printf("%s\n", merge_rr.items[i].string);
> - else if (!strcmp(argv[0], "diff"))
> + }
Unnecessary {} around a single printf(). Will clean-up.
Thanks.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-02-16 21:14 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-02-08 3:08 [PATCH] mergetool: don't skip modify/remove conflicts Martin von Zweigbergk
2011-02-09 21:45 ` Junio C Hamano
2011-02-11 2:46 ` Martin von Zweigbergk
2011-02-13 4:09 ` [PATCH v2 0/2] " Martin von Zweigbergk
2011-02-13 4:09 ` [PATCH v2 1/2] " Martin von Zweigbergk
2011-02-15 0:54 ` Junio C Hamano
2011-02-15 3:30 ` Martin von Zweigbergk
2011-02-15 20:12 ` Junio C Hamano
2011-02-16 10:47 ` [PATCH v3 0/2] " Martin von Zweigbergk
2011-02-16 10:47 ` [PATCH v3 1/2] rerere "remaining" Martin von Zweigbergk
2011-02-16 21:14 ` Junio C Hamano
2011-02-16 10:47 ` [PATCH v3 2/2] mergetool: don't skip modify/remove conflicts Martin von Zweigbergk
2011-02-13 4:09 ` [PATCH v2 2/2] rerere: factor out common conflict search code Martin von Zweigbergk
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).