{"thread":{"id":"33345","subject":"[PATCH] rerere forget: grok files containing NUL","startedAt":"2013-04-01T21:36:36Z","lastAt":"2013-04-02T19:34:30Z","messageCount":5,"participants":["Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"212820","messageId":"5159FDE4.2090409@kdbg.org","threadId":"33345","inReplyTo":null,"subject":"[PATCH] rerere forget: grok files containing NUL","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-04-01T21:36:36Z","receivedAt":"2013-04-01T21:36:36Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Using 'git rerere forget .' after a merge that involved binary files\nruns into an infinite loop if the binary file contains a zero byte.\nReplace a strchrnul by memchr because the former does not make progress\nas soon as the NUL is encountered.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n The new test case runs into the infinite loop if you back out\n the code change.\n\n There's another bug where an uninitialized pointer is accessed\n in the second for-loop in handle_cache(), presumably for a file\n with ADD-ADD conflicts. Will look into that one later this week.\n\n -- Hannes\n\n rerere.c                  |  6 ++++--\n t/t2030-unresolve-info.sh | 12 ++++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex a6a5cd5..4d940cd 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -284,8 +284,10 @@ static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)\n \tstrbuf_release(sb);\n \tif (!io->input.len)\n \t\treturn -1;\n-\tep = strchrnul(io->input.buf, '\\n');\n-\tif (*ep == '\\n')\n+\tep = memchr(io->input.buf, '\\n', io->input.len);\n+\tif (!ep)\n+\t\tep = io->input.buf + io->input.len;\n+\telse if (*ep == '\\n')\n \t\tep++;\n \tlen = ep - io->input.buf;\n \tstrbuf_add(sb, io->input.buf, len);\ndiff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh\nindex f262065..0b699f5 100755\n--- a/t/t2030-unresolve-info.sh\n+++ b/t/t2030-unresolve-info.sh\n@@ -44,9 +44,13 @@ prime_resolve_undo () {\n \n test_expect_success setup '\n \tmkdir fi &&\n+\tprintf \"a\\0a\" >binary &&\n+\tgit add binary &&\n \ttest_commit initial fi/le first &&\n \tgit branch side &&\n \tgit branch another &&\n+\tprintf \"a\\0b\" >binary &&\n+\tgit add binary &&\n \ttest_commit second fi/le second &&\n \tgit checkout side &&\n \ttest_commit third fi/le third &&\n@@ -167,4 +171,12 @@ test_expect_success 'rerere and rerere forget (subdirectory)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rerere forget (binary)' '\n+\tgit checkout -f side &&\n+\tprintf \"a\\0c\" >binary &&\n+\tgit commit -a -m binary &&\n+\ttest_must_fail git merge second &&\n+\tgit rerere forget binary\n+'\n+\n test_done\n-- \n1.8.2.383.g5f2fd52\n"},{"id":"212831","messageId":"7vhajpj294.fsf@alter.siamese.dyndns.org","threadId":"33345","inReplyTo":"5159FDE4.2090409@kdbg.org","subject":"Re: [PATCH] rerere forget: grok files containing NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-01T22:48:55Z","receivedAt":"2013-04-01T22:48:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> Using 'git rerere forget .' after a merge that involved binary files\n> runs into an infinite loop if the binary file contains a zero byte.\n> Replace a strchrnul by memchr because the former does not make progress\n> as soon as the NUL is encountered.\n\nHmph, thanks.\n\nIs it the right behaviour for rerere to even attempt to interfere\nwith a merge that involves binary files in the first place?\n\nDoes the three-way merge machinery replay recorded resolution for\nsuch a binary file correctly (after your fix, that is)?\n\n> diff --git a/rerere.c b/rerere.c\n> index a6a5cd5..4d940cd 100644\n> --- a/rerere.c\n> +++ b/rerere.c\n> @@ -284,8 +284,10 @@ static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)\n>  \tstrbuf_release(sb);\n>  \tif (!io->input.len)\n>  \t\treturn -1;\n> -\tep = strchrnul(io->input.buf, '\\n');\n> -\tif (*ep == '\\n')\n> +\tep = memchr(io->input.buf, '\\n', io->input.len);\n> +\tif (!ep)\n> +\t\tep = io->input.buf + io->input.len;\n> +\telse if (*ep == '\\n')\n>  \t\tep++;\n>  \tlen = ep - io->input.buf;\n>  \tstrbuf_add(sb, io->input.buf, len);\n> diff --git a/t/t2030-unresolve-info.sh b/t/t2030-unresolve-info.sh\n> index f262065..0b699f5 100755\n> --- a/t/t2030-unresolve-info.sh\n> +++ b/t/t2030-unresolve-info.sh\n> @@ -44,9 +44,13 @@ prime_resolve_undo () {\n>  \n>  test_expect_success setup '\n>  \tmkdir fi &&\n> +\tprintf \"a\\0a\" >binary &&\n> +\tgit add binary &&\n>  \ttest_commit initial fi/le first &&\n>  \tgit branch side &&\n>  \tgit branch another &&\n> +\tprintf \"a\\0b\" >binary &&\n> +\tgit add binary &&\n>  \ttest_commit second fi/le second &&\n>  \tgit checkout side &&\n>  \ttest_commit third fi/le third &&\n> @@ -167,4 +171,12 @@ test_expect_success 'rerere and rerere forget (subdirectory)' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'rerere forget (binary)' '\n> +\tgit checkout -f side &&\n> +\tprintf \"a\\0c\" >binary &&\n> +\tgit commit -a -m binary &&\n> +\ttest_must_fail git merge second &&\n> +\tgit rerere forget binary\n> +'\n> +\n>  test_done\n"},{"id":"212937","messageId":"515B2B6D.4050801@kdbg.org","threadId":"33345","inReplyTo":"7vhajpj294.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rerere forget: grok files containing NUL","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-04-02T19:03:09Z","receivedAt":"2013-04-02T19:03:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.04.2013 00:48, schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n>> Using 'git rerere forget .' after a merge that involved binary files\n>> runs into an infinite loop if the binary file contains a zero byte.\n>> Replace a strchrnul by memchr because the former does not make progress\n>> as soon as the NUL is encountered.\n> \n> Hmph, thanks.\n> \n> Is it the right behaviour for rerere to even attempt to interfere\n> with a merge that involves binary files in the first place?\n\nWhy not? And how could rerere tell the difference? It would have to do\nthe same check for binary-ness as the merge driver before it even starts\nlooking closer at the files.\n\n> Does the three-way merge machinery replay recorded resolution for\n> such a binary file correctly (after your fix, that is)?\n\nYes, it does. It recognizes the binary-ness and picks 'our' side. Only\nthen comes rerere_mem_getline into play.\n\n>> diff --git a/rerere.c b/rerere.c\n>> index a6a5cd5..4d940cd 100644\n>> --- a/rerere.c\n>> +++ b/rerere.c\n>> @@ -284,8 +284,10 @@ static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)\n>>  \tstrbuf_release(sb);\n>>  \tif (!io->input.len)\n>>  \t\treturn -1;\n>> -\tep = strchrnul(io->input.buf, '\\n');\n>> -\tif (*ep == '\\n')\n>> +\tep = memchr(io->input.buf, '\\n', io->input.len);\n>> +\tif (!ep)\n>> +\t\tep = io->input.buf + io->input.len;\n>> +\telse if (*ep == '\\n')\n>>  \t\tep++;\n>>  \tlen = ep - io->input.buf;\n>>  \tstrbuf_add(sb, io->input.buf, len);\n"},{"id":"212962","messageId":"7vmwtgd9lq.fsf@alter.siamese.dyndns.org","threadId":"33345","inReplyTo":"515B2B6D.4050801@kdbg.org","subject":"Re: [PATCH] rerere forget: grok files containing NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-02T19:18:57Z","receivedAt":"2013-04-02T19:18:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n>> Does the three-way merge machinery replay recorded resolution for\n>> such a binary file correctly (after your fix, that is)?\n>\n> Yes, it does. It recognizes the binary-ness and picks 'our' side. Only\n> then comes rerere_mem_getline into play.\n\nSurely getline() needs to be fixed not to loop forever regardless of\nthe binary-ness, but I was more worried about our additions of lines\nthat satisfy is_cmarker(), counting of them in the callchain from\nhandle_file() to handle_path() to decide if a path has already been\nresolved by the user, and recording of an resolution based on the\nreturn value of that callchain, all of which relies on the merged\ncontents being textual and marked with the conflict marker.\n\n>>> diff --git a/rerere.c b/rerere.c\n>>> index a6a5cd5..4d940cd 100644\n>>> --- a/rerere.c\n>>> +++ b/rerere.c\n>>> @@ -284,8 +284,10 @@ static int rerere_mem_getline(struct strbuf *sb, struct rerere_io *io_)\n>>>  \tstrbuf_release(sb);\n>>>  \tif (!io->input.len)\n>>>  \t\treturn -1;\n>>> -\tep = strchrnul(io->input.buf, '\\n');\n>>> -\tif (*ep == '\\n')\n>>> +\tep = memchr(io->input.buf, '\\n', io->input.len);\n>>> +\tif (!ep)\n>>> +\t\tep = io->input.buf + io->input.len;\n>>> +\telse if (*ep == '\\n')\n>>>  \t\tep++;\n>>>  \tlen = ep - io->input.buf;\n>>>  \tstrbuf_add(sb, io->input.buf, len);\n"},{"id":"212968","messageId":"515B32C6.4060005@kdbg.org","threadId":"33345","inReplyTo":"7vmwtgd9lq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rerere forget: grok files containing NUL","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-04-02T19:34:30Z","receivedAt":"2013-04-02T19:34:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 02.04.2013 21:18, schrieb Junio C Hamano:\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n>>> Does the three-way merge machinery replay recorded resolution for\n>>> such a binary file correctly (after your fix, that is)?\n>>\n>> Yes, it does. It recognizes the binary-ness and picks 'our' side. Only\n>> then comes rerere_mem_getline into play.\n> \n> Surely getline() needs to be fixed not to loop forever regardless of\n> the binary-ness, but I was more worried about our additions of lines\n> that satisfy is_cmarker(), counting of them in the callchain from\n> handle_file() to handle_path() to decide if a path has already been\n> resolved by the user, and recording of an resolution based on the\n> return value of that callchain, all of which relies on the merged\n> contents being textual and marked with the conflict marker.\n\nOf course, it would make sense to exclude binary files from rerere's\noperation. But that's an independent issue, and it is not new.\n\n-- Hannes\n"}]}