[PATCH] rerere should not repeat the earlier hunks in later ones

[PATCH] rerere should not repeat the earlier hunks in later ones

[Junio C Hamano]

When a file has more then one conflicting hunks, it repeated the
contents of previous hunks in output for later ones.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

diff --git a/builtin-rerere.c b/builtin-rerere.c
index b8867ab..b463c07 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -78,6 +78,13 @@ static void append_line(struct buffer *buffer, const char *line)
 	buffer->nr += len;
 }
 
+static void clear_buffer(struct buffer *buffer)
+{
+	free(buffer->ptr);
+	buffer->ptr = NULL;
+	buffer->nr = buffer->alloc = 0;
+}
+
 static int handle_file(const char *path,
 	 unsigned char *sha1, const char *output)
 {
@@ -131,6 +138,8 @@ static int handle_file(const char *path,
 				SHA1_Update(&ctx, two->ptr, two->nr);
 				SHA1_Update(&ctx, "\0", 1);
 			}
+			clear_buffer(one);
+			clear_buffer(two);
 		} else if (hunk == 1)
 			append_line(one, buf);
 		else if (hunk == 2)
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index e081b32..8b611bb 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -34,7 +34,8 @@ EOF
 git commit -q -a -m first
 
 git checkout -b second master
-git show first:a1 | sed 's/To die, t/To die! T/' > a1
+git show first:a1 |
+sed -e 's/To die, t/To die! T/' -e 's/life;$/life./' > a1
 git commit -q -a -m second
 
 # activate rerere
@@ -42,19 +43,26 @@ mkdir .git/rr-cache
 
 test_expect_failure 'conflicting merge' 'git pull . first'
 
-sha1=4f58849a60b4f969a2848966b6d02893b783e8fb
+sha1=$(sed -e 's/\t.*//' .git/rr-cache/MERGE_RR)
 rr=.git/rr-cache/$sha1
 test_expect_success 'recorded preimage' "grep ======= $rr/preimage"
 
 test_expect_success 'no postimage or thisimage yet' \
 	"test ! -f $rr/postimage -a ! -f $rr/thisimage"
 
+test_expect_success 'preimage have right number of lines' '
+
+	cnt=$(sed -ne "/^<<<<<<</,/^>>>>>>>/p" $rr/preimage | wc -l) &&
+	test "$cnt" = 10
+
+'
+
 git show first:a1 > a1
 
 cat > expect << EOF
 --- a/a1
 +++ b/a1
-@@ -6,11 +6,7 @@
+@@ -6,17 +6,9 @@
  The heart-ache and the thousand natural shocks
  That flesh is heir to, 'tis a consummation
  Devoutly to be wish'd.
@@ -66,8 +74,13 @@ cat > expect << EOF
  To sleep: perchance to dream: ay, there's the rub;
  For in that sleep of death what dreams may come
  When we have shuffled off this mortal coil,
+ Must give us pause: there's the respect
+-<<<<<<<
+-That makes calamity of so long life.
+-=======
+ That makes calamity of so long life;
+->>>>>>>
 EOF
-
 git rerere diff > out
 
 test_expect_success 'rerere diff' 'git diff expect out'

Re: [PATCH] rerere should not repeat the earlier hunks in later ones

[Johannes Schindelin]

Hi,

On Tue, 3 Apr 2007, Junio C Hamano wrote:

> When a file has more then one conflicting hunks, it repeated the
> contents of previous hunks in output for later ones.

/me wonders why our spell-checker has not replied yet: isn't it "than" 
instead of "then"?

> @@ -131,6 +138,8 @@ static int handle_file(const char *path,
>  				SHA1_Update(&ctx, two->ptr, two->nr);
>  				SHA1_Update(&ctx, "\0", 1);
>  			}
> +			clear_buffer(one);
> +			clear_buffer(two);
>  		} else if (hunk == 1)
>  			append_line(one, buf);
>  		else if (hunk == 2)

Ouch. Well spotted, thanks.

I wonder if we should also do

diff --git a/builtin-rerere.c b/builtin-rerere.c
index b8867ab..4eae27b 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -112,7 +112,8 @@ static int handle_file(const char *path,
 		else if (!prefixcmp(buf, ">>>>>>> ")) {
 			hunk_no++;
 			hunk = 0;
-			if (memcmp(one->ptr, two->ptr, one->nr < two->nr ?
+			if (one->nr > two->nr || memcmp(one->ptr, two->ptr,
+						one->nr < two->nr ?
 						one->nr : two->nr) > 0) {
 				struct buffer *swap = one;
 				one = two;

in case that one conflicting region is prefix of the other one.

Ciao,
Dscho

Re: [PATCH] rerere should not repeat the earlier hunks in later ones

[Junio C Hamano]

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

> I wonder if we should also do
>
> diff --git a/builtin-rerere.c b/builtin-rerere.c
> index b8867ab..4eae27b 100644
> --- a/builtin-rerere.c
> +++ b/builtin-rerere.c
> @@ -112,7 +112,8 @@ static int handle_file(const char *path,
>  		else if (!prefixcmp(buf, ">>>>>>> ")) {
>  			hunk_no++;
>  			hunk = 0;
> -			if (memcmp(one->ptr, two->ptr, one->nr < two->nr ?
> +			if (one->nr > two->nr || memcmp(one->ptr, two->ptr,
> +						one->nr < two->nr ?
>  						one->nr : two->nr) > 0) {
>  				struct buffer *swap = one;
>  				one = two;
>
> in case that one conflicting region is prefix of the other one.

If one is not a prefix of two but simply longer what does that
code do?

[PATCH] rerere: make sorting really stable.

[Junio C Hamano]

The earlier code does not swap hunks when the beginning of the
first side is identical to the whole of the second side.  In
such a case, the first one should sort later.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---
 builtin-rerere.c  |    7 +++++--
 t/t4200-rerere.sh |    9 +++++----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin-rerere.c b/builtin-rerere.c
index b463c07..8c2c8bd 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -117,10 +117,13 @@ static int handle_file(const char *path,
 		else if (!prefixcmp(buf, "======="))
 			hunk = 2;
 		else if (!prefixcmp(buf, ">>>>>>> ")) {
+			int one_is_longer = (one->nr > two->nr);
+			int common_len = one_is_longer ? two->nr : one->nr;
+			int cmp = memcmp(one->ptr, two->ptr, common_len);
+
 			hunk_no++;
 			hunk = 0;
-			if (memcmp(one->ptr, two->ptr, one->nr < two->nr ?
-						one->nr : two->nr) > 0) {
+			if ((cmp > 0) || ((cmp == 0) && one_is_longer)) {
 				struct buffer *swap = one;
 				one = two;
 				two = swap;
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index bc878d7..6ba63d7 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -35,7 +35,8 @@ git commit -q -a -m first
 
 git checkout -b second master
 git show first:a1 |
-sed -e 's/To die, t/To die! T/' -e 's/life;$/life./' > a1
+sed -e 's/To die, t/To die! T/' > a1
+echo "* END *" >>a1
 git commit -q -a -m second
 
 # activate rerere
@@ -53,7 +54,7 @@ test_expect_success 'no postimage or thisimage yet' \
 test_expect_success 'preimage has right number of lines' '
 
 	cnt=$(sed -ne "/^<<<<<<</,/^>>>>>>>/p" $rr/preimage | wc -l) &&
-	test $cnt = 10
+	test $cnt = 9
 
 '
 
@@ -75,10 +76,10 @@ cat > expect << EOF
  For in that sleep of death what dreams may come
  When we have shuffled off this mortal coil,
  Must give us pause: there's the respect
+ That makes calamity of so long life;
 -<<<<<<<
--That makes calamity of so long life.
 -=======
- That makes calamity of so long life;
+-* END *
 ->>>>>>>
 EOF
 git rerere diff > out
-- 
1.5.1.31.ge421f

Re: [PATCH] rerere should not repeat the earlier hunks in later ones

[Johannes Schindelin]

Hi,

On Wed, 4 Apr 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > I wonder if we should also do
> >
> > diff --git a/builtin-rerere.c b/builtin-rerere.c
> > index b8867ab..4eae27b 100644
> > --- a/builtin-rerere.c
> > +++ b/builtin-rerere.c
> > @@ -112,7 +112,8 @@ static int handle_file(const char *path,
> >  		else if (!prefixcmp(buf, ">>>>>>> ")) {
> >  			hunk_no++;
> >  			hunk = 0;
> > -			if (memcmp(one->ptr, two->ptr, one->nr < two->nr ?
> > +			if (one->nr > two->nr || memcmp(one->ptr, two->ptr,
> > +						one->nr < two->nr ?
> >  						one->nr : two->nr) > 0) {
> >  				struct buffer *swap = one;
> >  				one = two;
> >
> > in case that one conflicting region is prefix of the other one.
> 
> If one is not a prefix of two but simply longer what does that
> code do?

You're right. With the eager merging algorithm, it is no longer possible 
that one side is a strict prefix of the other.

So please forget about my comment.

Ciao,
Dscho

Re: [PATCH] rerere should not repeat the earlier hunks in later ones

[Junio C Hamano]

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

>> > @@ -112,7 +112,8 @@ static int handle_file(const char *path,
>> >  		else if (!prefixcmp(buf, ">>>>>>> ")) {
>> >  			hunk_no++;
>> >  			hunk = 0;
>> > -			if (memcmp(one->ptr, two->ptr, one->nr < two->nr ?
>> > +			if (one->nr > two->nr || memcmp(one->ptr, two->ptr,
>> > +						one->nr < two->nr ?
>> >  						one->nr : two->nr) > 0) {
>> >  				struct buffer *swap = one;
>> >  				one = two;
>> >
>> > in case that one conflicting region is prefix of the other one.
>> 
>> If one is not a prefix of two but simply longer what does that
>> code do?
>
> You're right. With the eager merging algorithm, it is no longer possible 
> that one side is a strict prefix of the other.
>
> So please forget about my comment.

Now you confused me even more...

Isn't there a case where one is full of text and two is empty?

Re: [PATCH] rerere should not repeat the earlier hunks in later ones

[Johannes Schindelin]

Hi,

On Wed, 4 Apr 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> > @@ -112,7 +112,8 @@ static int handle_file(const char *path,
> >> >  		else if (!prefixcmp(buf, ">>>>>>> ")) {
> >> >  			hunk_no++;
> >> >  			hunk = 0;
> >> > -			if (memcmp(one->ptr, two->ptr, one->nr < two->nr ?
> >> > +			if (one->nr > two->nr || memcmp(one->ptr, two->ptr,
> >> > +						one->nr < two->nr ?
> >> >  						one->nr : two->nr) > 0) {
> >> >  				struct buffer *swap = one;
> >> >  				one = two;
> >> >
> >> > in case that one conflicting region is prefix of the other one.
> >> 
> >> If one is not a prefix of two but simply longer what does that
> >> code do?
> >
> > You're right. With the eager merging algorithm, it is no longer possible 
> > that one side is a strict prefix of the other.
> >
> > So please forget about my comment.
> 
> Now you confused me even more...
> 
> Isn't there a case where one is full of text and two is empty?

If one is empty, yes, there can be a conflict. I only did not think of the 
empty string as a prefix to everything, but you are right.

Ciao,
Dscho