rerere forget: grok files containing NUL

PATCH In review Johannes Sixt 2 participants 5 messages Jun 15, 2016

Share & tools

Johannes Sixt

author

2016-06-15 22:56 UTC #0 lore raw

PATCH 1/1 rerere forget: grok files containing NUL

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 <[email protected]>

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

rerere.c +4 -2

index a6a5cd5..4d940cd 100644
@@ -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);

t/t2030-unresolve-info.sh +12 -0

index f262065..0b699f5 100755
@@ -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

Signature · 1 line

1.8.2.383.g5f2fd52

Junio C Hamano

2016-06-15 22:56 UTC #1 lore raw

Quoted 4 lines · 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.

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

Quoted 48 lines

rerere.c +4 -2

index a6a5cd5..4d940cd 100644
@@ -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);

t/t2030-unresolve-info.sh +12 -0

index f262065..0b699f5 100755
@@ -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

author

2016-06-15 22:56 UTC #2 lore raw

Am 02.04.2013 00:48, schrieb Junio C Hamano:

Quoted 11 lines

Quoted 4 lines · 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.
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.

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

Quoted 17 lines

rerere.c +4 -2

index a6a5cd5..4d940cd 100644
@@ -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

2016-06-15 22:56 UTC #3 lore raw

Quoted 5 lines · Johannes Sixt

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

Quoted 17 lines

rerere.c +4 -2

index a6a5cd5..4d940cd 100644
@@ -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

author

2016-06-15 22:56 UTC #4 lore raw

Am 02.04.2013 21:18, schrieb Junio C Hamano:

Quoted 8 lines

Quoted 5 lines · Johannes Sixt
Quoted 2 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.

Quoted 7 lines

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