threads / patch / 33345

patchrerere forget: grok files containing NUL

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

## tl;dr

5 messages between Apr 1, 2013 and Apr 2, 2013. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Johannes Sixt· Apr 1, 2013, 21:36 UTC · lore

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(-)

Show changes to 2 files +16 −2

rerere.c, t/t2030-unresolve-info.sh

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

Junio C Hamano· Apr 1, 2013, 22:48 UTC · re: Johannes Sixt · lore

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

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)?

Show 48 quoted lines

> 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

Johannes Sixt· Apr 2, 2013, 19:03 UTC · re: Junio C Hamano · lore

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

Am 02.04.2013 00:48, schrieb Junio C Hamano:

Show 11 quoted lines

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

Show 17 quoted lines

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

Junio C Hamano· Apr 2, 2013, 19:18 UTC · re: Johannes Sixt · lore

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

Johannes Sixt <j6t@kdbg.org> writes:

Show 5 quoted lines

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

Show 17 quoted lines

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

Johannes Sixt· Apr 2, 2013, 19:34 UTC · re: Junio C Hamano · lore

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

Am 02.04.2013 21:18, schrieb Junio C Hamano:

Show 15 quoted lines

> 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