Re: [PATCH] reopen_tempfile(): truncate opened file
On Tue, Sep 04, 2018 at 07:36:43PM -0400, Jeff King wrote: > On Tue, Sep 04, 2018 at 12:38:07PM -0400, Jeff King wrote: > > > > And just to be clear I'm looking forward to a patch from Jeff to fix > > > this since he clearly put more thoughts on this than me. With commit.c > > > being the only user of reopen_lock_file() I guess it's even ok to just > > > stick O_TRUNC in there and worry about O_APPEND when a new caller > > > needs that. > > > > That's the way I'm leaning to. The fix is obviously a one-liner, but I > > was hoping to construct a minimal test case. I just haven't gotten > > around to it yet. > > It turned out not to be too bad to write a test. It feels a little like > black magic, since I empirically determined a way in which the > cache-tree happens to shrink with the current code. But that assumption > is tested with a sanity check, so we'll at least know if it becomes a > noop. > > > The bug is ancient, so I don't think it's important for v2.19. > > The patch below should work on master or maint. We could do a fix > directly on top of the bug, but merging-up is weird (because the buggy > code became part of a reusable module). It's great that you were able to create a reproducer relatively easily. Thank you, guys. -- Luc
Re: [PATCH] reopen_tempfile(): truncate opened file
Jeff King writes: > On Wed, Sep 05, 2018 at 09:54:42AM -0700, Junio C Hamano wrote: > >> Jeff King writes: >> >> >> > So AFAIK this fsck catches everything and yields a non-zero exit in the >> >> > error case. And it should work for even a single byte of rubbish. >> >> >> >> Yes you're right. I forgot about the trailing hash. >> > >> > Thanks, I was worried that I was missing something. ;) >> > >> > Maybe it is worth making that final comment: >> > >> > # and that the trailing hash in the index was not corrupted, >> > # which should catch even a single byte of cruft >> > git fsck >> >> Perhaps. I do not mind seeing an additional comment to explain why >> this requires PERL (it wasn't immediately obvious as I never use >> 'commit -p' myself), either. > > I thought the PERL prereq in the existing "-p" test of the commit header > would explain it. ;) > > Do you prefer an in-code comment, or one in the commit message? Neither ;-) Just like I think 'our index was not corrupted' as an explanation for 'git fsck' is sufficient, PERL sprinkled all over this script and all of them tend to be near "commit -i/-p" should be a good enough clue, I'd think.
Re: [PATCH] reopen_tempfile(): truncate opened file
On Wed, Sep 05, 2018 at 09:54:42AM -0700, Junio C Hamano wrote: > Jeff King writes: > > >> > So AFAIK this fsck catches everything and yields a non-zero exit in the > >> > error case. And it should work for even a single byte of rubbish. > >> > >> Yes you're right. I forgot about the trailing hash. > > > > Thanks, I was worried that I was missing something. ;) > > > > Maybe it is worth making that final comment: > > > > # and that the trailing hash in the index was not corrupted, > > # which should catch even a single byte of cruft > > git fsck > > Perhaps. I do not mind seeing an additional comment to explain why > this requires PERL (it wasn't immediately obvious as I never use > 'commit -p' myself), either. I thought the PERL prereq in the existing "-p" test of the commit header would explain it. ;) Do you prefer an in-code comment, or one in the commit message? -Peff
Re: [PATCH] reopen_tempfile(): truncate opened file
Jeff King writes: >> > So AFAIK this fsck catches everything and yields a non-zero exit in the >> > error case. And it should work for even a single byte of rubbish. >> >> Yes you're right. I forgot about the trailing hash. > > Thanks, I was worried that I was missing something. ;) > > Maybe it is worth making that final comment: > > # and that the trailing hash in the index was not corrupted, > # which should catch even a single byte of cruft > git fsck Perhaps. I do not mind seeing an additional comment to explain why this requires PERL (it wasn't immediately obvious as I never use 'commit -p' myself), either.
Re: [PATCH] reopen_tempfile(): truncate opened file
On Wed, Sep 05, 2018 at 05:39:19PM +0200, Duy Nguyen wrote: > On Wed, Sep 5, 2018 at 5:35 PM Jeff King wrote: > > > > + after=$(wc -c <.git/index) && > > > > + > > > > + # double check that the index shrank > > > > + test $before -gt $after && > > > > + > > > > + # and that our index was not corrupted > > > > + git fsck > > > > > > If the index is not shrunk, we parse remaining rubbish as extensions. > > > If by chance the rubbish extension name is in uppercase, then we > > > ignore (and not flag it as error). But then the chances of the next 4 > > > bytes being the "right" extension size is so small that we would end > > > up flagging it as bad extension anyway. So it's good. But if you want > > > to be even stricter (not necessary in my opinion), make sure that > > > stderr is empty. > > > > In this case, the size difference is only a few bytes, so the rubbish > > actually ends up in the trailing sha1. The reason I use git-fsck here is > > that it actually verifies the whole sha1 (since normal index reads no > > longer do). In fact, a normal index read won't show any problem for this > > case (since it is _only_ the trailing sha1 which is junk, and we no > > longer verify it on every read). > > > > In the original sparse-dev case, the size of the rubbish is much larger > > (because we deleted a lot more entries), and we do interpret it as a > > bogus extension. But it also triggers here, because the trailing sha1 is > > _also_ wrong. > > > > So AFAIK this fsck catches everything and yields a non-zero exit in the > > error case. And it should work for even a single byte of rubbish. > > Yes you're right. I forgot about the trailing hash. Thanks, I was worried that I was missing something. ;) Maybe it is worth making that final comment: # and that the trailing hash in the index was not corrupted, # which should catch even a single byte of cruft git fsck -Peff
Re: [PATCH] reopen_tempfile(): truncate opened file
On Wed, Sep 5, 2018 at 5:35 PM Jeff King wrote: > > > + after=$(wc -c <.git/index) && > > > + > > > + # double check that the index shrank > > > + test $before -gt $after && > > > + > > > + # and that our index was not corrupted > > > + git fsck > > > > If the index is not shrunk, we parse remaining rubbish as extensions. > > If by chance the rubbish extension name is in uppercase, then we > > ignore (and not flag it as error). But then the chances of the next 4 > > bytes being the "right" extension size is so small that we would end > > up flagging it as bad extension anyway. So it's good. But if you want > > to be even stricter (not necessary in my opinion), make sure that > > stderr is empty. > > In this case, the size difference is only a few bytes, so the rubbish > actually ends up in the trailing sha1. The reason I use git-fsck here is > that it actually verifies the whole sha1 (since normal index reads no > longer do). In fact, a normal index read won't show any problem for this > case (since it is _only_ the trailing sha1 which is junk, and we no > longer verify it on every read). > > In the original sparse-dev case, the size of the rubbish is much larger > (because we deleted a lot more entries), and we do interpret it as a > bogus extension. But it also triggers here, because the trailing sha1 is > _also_ wrong. > > So AFAIK this fsck catches everything and yields a non-zero exit in the > error case. And it should work for even a single byte of rubbish. Yes you're right. I forgot about the trailing hash. -- Duy
Re: [PATCH] reopen_tempfile(): truncate opened file
On Wed, Sep 05, 2018 at 05:27:11PM +0200, Duy Nguyen wrote: > > +test_expect_success PERL 'commit -p with shrinking cache-tree' ' > > + mkdir -p deep/subdir && > > + echo content >deep/subdir/file && > > + git add deep && > > + git commit -m add && > > + git rm -r deep && > > OK so I guess at this step, we invalidate some cache-tree blocks, but > we write the same blocks down (with "invalid" flag), so pretty much > the same size as before. I didn't verify exactly what was in the index, but that was my understanding, too (well, it's a little smaller because we drop the actual index entries, but keep the invalidated cache-tree). I worry a little that "rm" might eventually learn to drop those invalidated bits. But hopefully finding this commit would lead that person to figure out another way to accomplish the same thing, or to decide that carrying the test forward isn't worth it. > > + after=$(wc -c <.git/index) && > > + > > + # double check that the index shrank > > + test $before -gt $after && > > + > > + # and that our index was not corrupted > > + git fsck > > If the index is not shrunk, we parse remaining rubbish as extensions. > If by chance the rubbish extension name is in uppercase, then we > ignore (and not flag it as error). But then the chances of the next 4 > bytes being the "right" extension size is so small that we would end > up flagging it as bad extension anyway. So it's good. But if you want > to be even stricter (not necessary in my opinion), make sure that > stderr is empty. In this case, the size difference is only a few bytes, so the rubbish actually ends up in the trailing sha1. The reason I use git-fsck here is that it actually verifies the whole sha1 (since normal index reads no longer do). In fact, a normal index read won't show any problem for this case (since it is _only_ the trailing sha1 which is junk, and we no longer verify it on every read). In the original sparse-dev case, the size of the rubbish is much larger (because we deleted a lot more entries), and we do interpret it as a bogus extension. But it also triggers here, because the trailing sha1 is _also_ wrong. So AFAIK this fsck catches everything and yields a non-zero exit in the error case. And it should work for even a single byte of rubbish. -Peff
Re: [PATCH] reopen_tempfile(): truncate opened file
On Wed, Sep 5, 2018 at 1:36 AM Jeff King wrote: > It turned out not to be too bad to write a test. It feels a little like > black magic, since I empirically determined a way in which the > cache-tree happens to shrink with the current code. Aha! I attempted to reproduce with a verylongpathname and failed, but then I had the main index portion in mind, not cache-tree. > diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh > index 7de40141ca..94fcb4a78e 100755 > --- a/t/t0090-cache-tree.sh > +++ b/t/t0090-cache-tree.sh > @@ -161,6 +161,24 @@ test_expect_success PERL 'commit --interactive gives > cache-tree on partial commi > test_cache_tree > ' > > +test_expect_success PERL 'commit -p with shrinking cache-tree' ' > + mkdir -p deep/subdir && > + echo content >deep/subdir/file && > + git add deep && > + git commit -m add && > + git rm -r deep && OK so I guess at this step, we invalidate some cache-tree blocks, but we write the same blocks down (with "invalid" flag), so pretty much the same size as before. > + > + before=$(wc -c <.git/index) && > + git commit -m delete -p && Then inside this command we recompute cache-tree and throw away the invalidated cache-tree blocks for deep and deep/subdir, which shrinks the index. Makes sense. > + after=$(wc -c <.git/index) && > + > + # double check that the index shrank > + test $before -gt $after && > + > + # and that our index was not corrupted > + git fsck If the index is not shrunk, we parse remaining rubbish as extensions. If by chance the rubbish extension name is in uppercase, then we ignore (and not flag it as error). But then the chances of the next 4 bytes being the "right" extension size is so small that we would end up flagging it as bad extension anyway. So it's good. But if you want to be even stricter (not necessary in my opinion), make sure that stderr is empty. The rest of the patch is obviously correct. -- Duy
