[PATCH] rerere forget: grok files containing NUL

[PATCH] rerere forget: grok files containing NUL

[Johannes Sixt]

Using 'git rerere forget .' after a merge that involved binary files
runs into an infinite loop if the binary file contains a zero byte.
Replace a strchrnul by memchr because the former does not make progress
as soon as the NUL is encountered.

Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 The new test case runs into the infinite loop if you back out
 the code change.

 There's another bug where an uninitialized pointer is accessed
 in the second for-loop in handle_cache(), presumably for a file
 with ADD-ADD conflicts. Will look into that one later this week.

 -- Hannes

 rerere.c                  |  6 ++++--
 t/t2030-unresolve-info.sh | 12 ++++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/rerere.c b/rerere.c
index a6a5cd5..4d940cd 100644
--- a/rerere.c
+++ b/rerere.c
@@ -284,8 +284,10 @@ static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)
 	strbuf_release(sb);
 	if (!io->input.len)
 		return -1;
-	ep = strchrnul(io->input.buf, '\n');
-	if (*ep == '\n')
+	ep = memchr(io->input.buf, '\n', io->input.len);
+	if (!ep)
+		ep = io->input.buf + io->input.len;
+	else if (*ep == '\n')
 		ep++;
 	len = ep - io->input.buf;
 	strbuf_add(sb, io->input.buf, len);
diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
index f262065..0b699f5 100755
--- a/t/t2030-unresolve-info.sh
+++ b/t/t2030-unresolve-info.sh
@@ -44,9 +44,13 @@ prime_resolve_undo () {
 
 test_expect_success setup '
 	mkdir fi &&
+	printf "a\0a" >binary &&
+	git add binary &&
 	test_commit initial fi/le first &&
 	git branch side &&
 	git branch another &&
+	printf "a\0b" >binary &&
+	git add binary &&
 	test_commit second fi/le second &&
 	git checkout side &&
 	test_commit third fi/le third &&
@@ -167,4 +171,12 @@ test_expect_success 'rerere and rerere forget (subdirectory)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rerere forget (binary)' '
+	git checkout -f side &&
+	printf "a\0c" >binary &&
+	git commit -a -m binary &&
+	test_must_fail git merge second &&
+	git rerere forget binary
+'
+
 test_done
-- 
1.8.2.383.g5f2fd52

Re: [PATCH] rerere forget: grok files containing NUL

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Using 'git rerere forget .' after a merge that involved binary files
> runs into an infinite loop if the binary file contains a zero byte.
> Replace a strchrnul by memchr because the former does not make progress
> as soon as the NUL is encountered.

Hmph, thanks.

Is it the right behaviour for rerere to even attempt to interfere
with a merge that involves binary files in the first place?

Does the three-way merge machinery replay recorded resolution for
such a binary file correctly (after your fix, that is)?

> diff --git a/rerere.c b/rerere.c
> index a6a5cd5..4d940cd 100644
> --- a/rerere.c
> +++ b/rerere.c
> @@ -284,8 +284,10 @@ static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)
>  	strbuf_release(sb);
>  	if (!io->input.len)
>  		return -1;
> -	ep = strchrnul(io->input.buf, '\n');
> -	if (*ep == '\n')
> +	ep = memchr(io->input.buf, '\n', io->input.len);
> +	if (!ep)
> +		ep = io->input.buf + io->input.len;
> +	else if (*ep == '\n')
>  		ep++;
>  	len = ep - io->input.buf;
>  	strbuf_add(sb, io->input.buf, len);
> diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh
> index f262065..0b699f5 100755
> --- a/t/t2030-unresolve-info.sh
> +++ b/t/t2030-unresolve-info.sh
> @@ -44,9 +44,13 @@ prime_resolve_undo () {
>  
>  test_expect_success setup '
>  	mkdir fi &&
> +	printf "a\0a" >binary &&
> +	git add binary &&
>  	test_commit initial fi/le first &&
>  	git branch side &&
>  	git branch another &&
> +	printf "a\0b" >binary &&
> +	git add binary &&
>  	test_commit second fi/le second &&
>  	git checkout side &&
>  	test_commit third fi/le third &&
> @@ -167,4 +171,12 @@ test_expect_success 'rerere and rerere forget (subdirectory)' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'rerere forget (binary)' '
> +	git checkout -f side &&
> +	printf "a\0c" >binary &&
> +	git commit -a -m binary &&
> +	test_must_fail git merge second &&
> +	git rerere forget binary
> +'
> +
>  test_done

Re: [PATCH] rerere forget: grok files containing NUL

[Johannes Sixt]

Am 02.04.2013 00:48, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>> Using 'git rerere forget .' after a merge that involved binary files
>> runs into an infinite loop if the binary file contains a zero byte.
>> Replace a strchrnul by memchr because the former does not make progress
>> as soon as the NUL is encountered.
> 
> Hmph, thanks.
> 
> Is it the right behaviour for rerere to even attempt to interfere
> with a merge that involves binary files in the first place?

Why not? And how could rerere tell the difference? It would have to do
the same check for binary-ness as the merge driver before it even starts
looking closer at the files.

> Does the three-way merge machinery replay recorded resolution for
> such a binary file correctly (after your fix, that is)?

Yes, it does. It recognizes the binary-ness and picks 'our' side. Only
then comes rerere_mem_getline into play.

>> diff --git a/rerere.c b/rerere.c
>> index a6a5cd5..4d940cd 100644
>> --- a/rerere.c
>> +++ b/rerere.c
>> @@ -284,8 +284,10 @@ static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)
>>  	strbuf_release(sb);
>>  	if (!io->input.len)
>>  		return -1;
>> -	ep = strchrnul(io->input.buf, '\n');
>> -	if (*ep == '\n')
>> +	ep = memchr(io->input.buf, '\n', io->input.len);
>> +	if (!ep)
>> +		ep = io->input.buf + io->input.len;
>> +	else if (*ep == '\n')
>>  		ep++;
>>  	len = ep - io->input.buf;
>>  	strbuf_add(sb, io->input.buf, len);

Re: [PATCH] rerere forget: grok files containing NUL

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> Does the three-way merge machinery replay recorded resolution for
>> such a binary file correctly (after your fix, that is)?
>
> Yes, it does. It recognizes the binary-ness and picks 'our' side. Only
> then comes rerere_mem_getline into play.

Surely getline() needs to be fixed not to loop forever regardless of
the binary-ness, but I was more worried about our additions of lines
that satisfy is_cmarker(), counting of them in the callchain from
handle_file() to handle_path() to decide if a path has already been
resolved by the user, and recording of an resolution based on the
return value of that callchain, all of which relies on the merged
contents being textual and marked with the conflict marker.

>>> diff --git a/rerere.c b/rerere.c
>>> index a6a5cd5..4d940cd 100644
>>> --- a/rerere.c
>>> +++ b/rerere.c
>>> @@ -284,8 +284,10 @@ static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)
>>>  	strbuf_release(sb);
>>>  	if (!io->input.len)
>>>  		return -1;
>>> -	ep = strchrnul(io->input.buf, '\n');
>>> -	if (*ep == '\n')
>>> +	ep = memchr(io->input.buf, '\n', io->input.len);
>>> +	if (!ep)
>>> +		ep = io->input.buf + io->input.len;
>>> +	else if (*ep == '\n')
>>>  		ep++;
>>>  	len = ep - io->input.buf;
>>>  	strbuf_add(sb, io->input.buf, len);

Re: [PATCH] rerere forget: grok files containing NUL

[Johannes Sixt]

Am 02.04.2013 21:18, schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
> 
>>> Does the three-way merge machinery replay recorded resolution for
>>> such a binary file correctly (after your fix, that is)?
>>
>> Yes, it does. It recognizes the binary-ness and picks 'our' side. Only
>> then comes rerere_mem_getline into play.
> 
> Surely getline() needs to be fixed not to loop forever regardless of
> the binary-ness, but I was more worried about our additions of lines
> that satisfy is_cmarker(), counting of them in the callchain from
> handle_file() to handle_path() to decide if a path has already been
> resolved by the user, and recording of an resolution based on the
> return value of that callchain, all of which relies on the merged
> contents being textual and marked with the conflict marker.

Of course, it would make sense to exclude binary files from rerere's
operation. But that's an independent issue, and it is not new.

-- Hannes