# [PATCH] rerere forget: grok files containing NUL 5 messages from 2013-04-01 to 2013-04-02. Participants: Johannes Sixt, Junio C Hamano. Thread: https://gitlist.dev/t/33345 ## Johannes Sixt, 2013-04-01 21:36 Subject: [PATCH] rerere forget: grok files containing NUL Message-ID: <5159FDE4.2090409@kdbg.org> URL: https://gitlist.dev/e/5159FDE4.2090409%40kdbg.org ``` 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 --- 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 ``` ## Junio C Hamano, 2013-04-01 22:48 Subject: Re: [PATCH] rerere forget: grok files containing NUL Message-ID: <7vhajpj294.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhajpj294.fsf%40alter.siamese.dyndns.org In-Reply-To: <5159FDE4.2090409@kdbg.org> ``` Johannes Sixt 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 ``` ## Johannes Sixt, 2013-04-02 19:03 Subject: Re: [PATCH] rerere forget: grok files containing NUL Message-ID: <515B2B6D.4050801@kdbg.org> URL: https://gitlist.dev/e/515B2B6D.4050801%40kdbg.org In-Reply-To: <7vhajpj294.fsf@alter.siamese.dyndns.org> ``` Am 02.04.2013 00:48, schrieb Junio C Hamano: > Johannes Sixt 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); ``` ## Junio C Hamano, 2013-04-02 19:18 Subject: Re: [PATCH] rerere forget: grok files containing NUL Message-ID: <7vmwtgd9lq.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmwtgd9lq.fsf%40alter.siamese.dyndns.org In-Reply-To: <515B2B6D.4050801@kdbg.org> ``` Johannes Sixt 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); ``` ## Johannes Sixt, 2013-04-02 19:34 Subject: Re: [PATCH] rerere forget: grok files containing NUL Message-ID: <515B32C6.4060005@kdbg.org> URL: https://gitlist.dev/e/515B32C6.4060005%40kdbg.org In-Reply-To: <7vmwtgd9lq.fsf@alter.siamese.dyndns.org> ``` Am 02.04.2013 21:18, schrieb Junio C Hamano: > Johannes Sixt 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 ```