{"thread":{"id":"49266","subject":"[BUG] index corruption with git commit -p","startedAt":"2018-09-01T21:42:03Z","lastAt":"2018-09-05T18:48:39Z","messageCount":19,"participants":["Luc Van Oostenryck","Ævar Arnfjörð Bjarmason","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"357164","messageId":"20180901214157.hxlqmbz3fds7hsdl@ltop.local","threadId":"49266","inReplyTo":null,"subject":"[BUG] index corruption with git commit -p","fromName":"Luc Van Oostenryck","fromEmail":"luc.vanoostenryck@gmail.com","sentAt":"2018-09-01T21:41:58Z","receivedAt":"2018-09-01T21:42:03Z","isPatch":false,"sender":{"key":"luc.vanoostenryck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16168334?v=4"},"body":"Hi,\n\nI've just had a scary error:\n\terror: index uses $?+? extension, which we do not understand\n\tfatal: index file corrupt\n\nThings were quickly recovered by deleting the index but it clearly\nlooks to a but to me.\n\nHere are the steps to reproduce it:\n  $ git clone git://github.com/lucvoo/sparse-dev.git <somedir>\n  $ cd <somedir>\n  $ git co index-corruption\n  $ git rm -r validation/ Documentation/\n  $ git commit -m <some message> -p\n  $ git status\nerror: index uses $?+? extension, which we do not understand\nfatal: index file corrupt\n\n\nThe 'extension' pattern '$?+?', can vary a bit, sometimes\nit's just '????', but always seems 4 chars.\nIf the commit command doesn't use the '-p' flag, there is no\nproblem. The repository itself is not corrupted, it's only\nthe index. It happends with git 2.18.0 and 2.17.0\n\n\n-- Luc Van Oostenryck\n"},{"id":"357165","messageId":"87tvn8c166.fsf@evledraar.gmail.com","threadId":"49266","inReplyTo":"20180901214157.hxlqmbz3fds7hsdl@ltop.local","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-01T22:17:53Z","receivedAt":"2018-09-01T22:18:13Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Sep 01 2018, Luc Van Oostenryck wrote:\n\n> Hi,\n>\n> I've just had a scary error:\n> \terror: index uses $?+? extension, which we do not understand\n> \tfatal: index file corrupt\n>\n> Things were quickly recovered by deleting the index but it clearly\n> looks to a but to me.\n>\n> Here are the steps to reproduce it:\n>   $ git clone git://github.com/lucvoo/sparse-dev.git <somedir>\n>   $ cd <somedir>\n>   $ git co index-corruption\n>   $ git rm -r validation/ Documentation/\n>   $ git commit -m <some message> -p\n>   $ git status\n> error: index uses $?+? extension, which we do not understand\n> fatal: index file corrupt\n>\n>\n> The 'extension' pattern '$?+?', can vary a bit, sometimes\n> it's just '????', but always seems 4 chars.\n> If the commit command doesn't use the '-p' flag, there is no\n> problem. The repository itself is not corrupted, it's only\n> the index. It happends with git 2.18.0 and 2.17.0\n\nYeah this is a bug, I didn't dig much but testing with this script down\nto 2.8.0:\n\n    #!/bin/sh\n\n    cd ~/g/git\n    make -j $(parallel --number-of-cores) USE_LIBPCRE2=YesPlease CFLAGS=\"-O0 -g -ggdb3\" DEVELOPER=1 DEVOPTS=no-error NO_OPENSSL=Y all\n\n    (\n        rm -rf /tmp/x;\n        ~/g/git/git --exec-path=/home/avar/g/git clone git://github.com/lucvoo/sparse-dev.git /tmp/x &&\n        cd /tmp/x &&\n        ~/g/git/git --exec-path=/home/avar/g/git checkout index-corruption &&\n        ~/g/git/git --exec-path=/home/avar/g/git rm -r validation/ Documentation/ &&\n        ~/g/git/git --exec-path=/home/avar/g/git commit -p\n    )\n\n    ~/g/git/git --exec-path=/home/avar/g/git -C /tmp/x status\n\n    if ~/g/git/git --exec-path=/home/avar/g/git -C /tmp/x status\n    then\n        exit 0\n    else\n        exit 1\n    fi\n\nI found that the first bad commit was: 680ee550d7 (\"commit: skip\ndiscarding the index if there is no pre-commit hook\", 2017-08-14)\n\nNow, note the two invocations of \"status\" in my script. Before we'd\nalready been complaining about a bad index, but after that commit is the\nfirst time we started getting a persistent error, and indeed even\nreverting it now on top of master makes the error non-persistent.\n\nSo not a fix, but a strong signal to see where we should start\nlooking. I.e. the index file handling around discard_cache() &\n\"interactive\" in commit.c is likely what's broken.\n"},{"id":"357174","messageId":"20180902050803.GA21324@sigill.intra.peff.net","threadId":"49266","inReplyTo":"87tvn8c166.fsf@evledraar.gmail.com","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-02T05:08:03Z","receivedAt":"2018-09-02T05:08:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 02, 2018 at 12:17:53AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Here are the steps to reproduce it:\n> >   $ git clone git://github.com/lucvoo/sparse-dev.git <somedir>\n> >   $ cd <somedir>\n> >   $ git co index-corruption\n> >   $ git rm -r validation/ Documentation/\n> >   $ git commit -m <some message> -p\n> >   $ git status\n> > error: index uses $?+? extension, which we do not understand\n> > fatal: index file corrupt\n> >\n> > The 'extension' pattern '$?+?', can vary a bit, sometimes\n> > it's just '????', but always seems 4 chars.\n> > If the commit command doesn't use the '-p' flag, there is no\n> > problem. The repository itself is not corrupted, it's only\n> > the index. It happends with git 2.18.0 and 2.17.0\n> \n> Yeah this is a bug, I didn't dig much but testing with this script down\n> to 2.8.0:\n> [...]\n> I found that the first bad commit was: 680ee550d7 (\"commit: skip\n> discarding the index if there is no pre-commit hook\", 2017-08-14)\n\nI think it's much older than that. I set up my test repo like this:\n\n  git clone git://github.com/lucvoo/sparse-dev.git\n  cd sparse-dev\n  git checkout --detach\n\nand then bisected with this script:\n\n  cd /path/to/sparse-dev\n  rm .git/index\n  git reset --hard index-corruption &&\n  git rm -q -r validation/ Documentation/ &&\n  git commit -qm foo -p &&\n  git status\n\nSince a33fc72fe9 (read-cache: force_verify_index_checksum, 2017-04-14),\nthat produces the corrupt extension error. But before that, I\nconsistently get:\n\n  error: bad index file sha1 signature\n  fatal: index file corrupt\n\nfrom git-commit. And that bisects back to 9c4d6c0297 (cache-tree: Write\nupdated cache-tree after commit, 2014-07-13).\n\nIf I revert that commit (which takes some untangling, see below), then\nthe problem seems to go away. Here's the patch I tried on top of the\ncurrent master, though I think it is actually the first hunk that is\nmaking the difference.\n\n---\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0d9828e29e..779c5e2cb5 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -359,13 +359,6 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \n \t\tdiscard_cache();\n \t\tread_cache_from(get_lock_file_path(&index_lock));\n-\t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n-\t\t\tif (reopen_lock_file(&index_lock) < 0)\n-\t\t\t\tdie(_(\"unable to write index file\"));\n-\t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n-\t\t\t\tdie(_(\"unable to update temporary index\"));\n-\t\t} else\n-\t\t\twarning(_(\"Failed to update main cache tree\"));\n \n \t\tcommit_style = COMMIT_NORMAL;\n \t\tret = get_lock_file_path(&index_lock);\n@@ -408,9 +401,6 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tif (!only && !pathspec.nr) {\n \t\thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \t\trefresh_cache_or_die(refresh_flags);\n-\t\tif (active_cache_changed\n-\t\t    || !cache_tree_fully_valid(active_cache_tree))\n-\t\t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock,\n \t\t\t\t       COMMIT_LOCK | SKIP_IF_UNCHANGED))\n \t\t\tdie(_(\"unable to write new_index file\"));\n@@ -457,7 +447,6 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \thold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n-\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n \tif (write_locked_index(&the_index, &index_lock, 0))\n \t\tdie(_(\"unable to write new_index file\"));\n \n\n-Peff\n"},{"id":"357177","messageId":"20180902071204.GA2868@duynguyen.home","threadId":"49266","inReplyTo":"20180902050803.GA21324@sigill.intra.peff.net","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-02T07:12:04Z","receivedAt":"2018-09-02T07:18:31Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Sep 02, 2018 at 01:08:03AM -0400, Jeff King wrote:\n> On Sun, Sep 02, 2018 at 12:17:53AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> > > Here are the steps to reproduce it:\n> > >   $ git clone git://github.com/lucvoo/sparse-dev.git <somedir>\n> > >   $ cd <somedir>\n> > >   $ git co index-corruption\n> > >   $ git rm -r validation/ Documentation/\n> > >   $ git commit -m <some message> -p\n> > >   $ git status\n> > > error: index uses $?+? extension, which we do not understand\n> > > fatal: index file corrupt\n> > >\n> > > The 'extension' pattern '$?+?', can vary a bit, sometimes\n> > > it's just '????', but always seems 4 chars.\n> > > If the commit command doesn't use the '-p' flag, there is no\n> > > problem. The repository itself is not corrupted, it's only\n> > > the index. It happends with git 2.18.0 and 2.17.0\n> > \n> > Yeah this is a bug, I didn't dig much but testing with this script down\n> > to 2.8.0:\n> > [...]\n> > I found that the first bad commit was: 680ee550d7 (\"commit: skip\n> > discarding the index if there is no pre-commit hook\", 2017-08-14)\n> \n> I think it's much older than that. I set up my test repo like this:\n> \n>   git clone git://github.com/lucvoo/sparse-dev.git\n>   cd sparse-dev\n>   git checkout --detach\n> \n> and then bisected with this script:\n> \n>   cd /path/to/sparse-dev\n>   rm .git/index\n>   git reset --hard index-corruption &&\n>   git rm -q -r validation/ Documentation/ &&\n>   git commit -qm foo -p &&\n>   git status\n> \n> Since a33fc72fe9 (read-cache: force_verify_index_checksum, 2017-04-14),\n> that produces the corrupt extension error. But before that, I\n> consistently get:\n> \n>   error: bad index file sha1 signature\n>   fatal: index file corrupt\n> \n> from git-commit. And that bisects back to 9c4d6c0297 (cache-tree: Write\n> updated cache-tree after commit, 2014-07-13).\n> \n> If I revert that commit (which takes some untangling, see below), then\n> the problem seems to go away. Here's the patch I tried on top of the\n> current master, though I think it is actually the first hunk that is\n> making the difference.\n> \n> ---\n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 0d9828e29e..779c5e2cb5 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -359,13 +359,6 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n>  \n>  \t\tdiscard_cache();\n>  \t\tread_cache_from(get_lock_file_path(&index_lock));\n> -\t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n> -\t\t\tif (reopen_lock_file(&index_lock) < 0)\n> -\t\t\t\tdie(_(\"unable to write index file\"));\n> -\t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n> -\t\t\t\tdie(_(\"unable to update temporary index\"));\n> -\t\t} else\n> -\t\t\twarning(_(\"Failed to update main cache tree\"));\n>\n\nNarrowing down to this does help. This patch seems to fix it to me. I\nguess we have some leftover from the interactive add that should not\nbe there after we have written the new index.\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 2be7bdb331..60f30b3780 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -432,6 +432,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n \t\t\tif (reopen_lock_file(&index_lock) < 0)\n \t\t\t\tdie(_(\"unable to write index file\"));\n+\t\t\tftruncate(index_lock.tempfile->fd, 0);\n \t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n \t\t\t\tdie(_(\"unable to update temporary index\"));\n \t\t} else\n\n\n--\nDuy\n"},{"id":"357178","messageId":"20180902072408.GA18787@sigill.intra.peff.net","threadId":"49266","inReplyTo":"20180902071204.GA2868@duynguyen.home","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-02T07:24:09Z","receivedAt":"2018-09-02T07:24:14Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 02, 2018 at 09:12:04AM +0200, Duy Nguyen wrote:\n\n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 0d9828e29e..779c5e2cb5 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -359,13 +359,6 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> >  \n> >  \t\tdiscard_cache();\n> >  \t\tread_cache_from(get_lock_file_path(&index_lock));\n> > -\t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n> > -\t\t\tif (reopen_lock_file(&index_lock) < 0)\n> > -\t\t\t\tdie(_(\"unable to write index file\"));\n> > -\t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n> > -\t\t\t\tdie(_(\"unable to update temporary index\"));\n> > -\t\t} else\n> > -\t\t\twarning(_(\"Failed to update main cache tree\"));\n> >\n> \n> Narrowing down to this does help. This patch seems to fix it to me. I\n> guess we have some leftover from the interactive add that should not\n> be there after we have written the new index.\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index 2be7bdb331..60f30b3780 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -432,6 +432,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n>  \t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n>  \t\t\tif (reopen_lock_file(&index_lock) < 0)\n>  \t\t\t\tdie(_(\"unable to write index file\"));\n> +\t\t\tftruncate(index_lock.tempfile->fd, 0);\n>  \t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n>  \t\t\t\tdie(_(\"unable to update temporary index\"));\n>  \t\t} else\n\nDoh, of course. I even thought about this issue and dug all the way into\nreopen_lock_file(), but for some reason temporarily forgot that O_WRONLY\ndoes not imply O_TRUNC.\n\nArguably this should be the default for reopen_lockfile(), as getting a\nwrite pointer into an existing file is not ever going to be useful for\nthe way Git uses lockfiles. Opening with O_APPEND could conceivably be\nuseful, but it's pretty unlikely (and certainly not helpful here, and\nthis is the only caller). Alternatively, the function should just take\nopen(2) flags.\n\nAt any rate, I think this perfectly explains the behavior we're seeing.\n\n-Peff\n"},{"id":"357183","messageId":"20180902075351.lkfnekklnwot7lzf@ltop.local","threadId":"49266","inReplyTo":"20180902072408.GA18787@sigill.intra.peff.net","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Luc Van Oostenryck","fromEmail":"luc.vanoostenryck@gmail.com","sentAt":"2018-09-02T07:53:53Z","receivedAt":"2018-09-02T07:53:58Z","isPatch":false,"sender":{"key":"luc.vanoostenryck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16168334?v=4"},"body":"On Sun, Sep 02, 2018 at 03:24:09AM -0400, Jeff King wrote:\n> On Sun, Sep 02, 2018 at 09:12:04AM +0200, Duy Nguyen wrote:\n> > diff --git a/builtin/commit.c b/builtin/commit.c\n> > index 2be7bdb331..60f30b3780 100644\n> > --- a/builtin/commit.c\n> > +++ b/builtin/commit.c\n> > @@ -432,6 +432,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> >  \t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n> >  \t\t\tif (reopen_lock_file(&index_lock) < 0)\n> >  \t\t\t\tdie(_(\"unable to write index file\"));\n> > +\t\t\tftruncate(index_lock.tempfile->fd, 0);\n> >  \t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n> >  \t\t\t\tdie(_(\"unable to update temporary index\"));\n> >  \t\t} else\n> \n> Doh, of course. I even thought about this issue and dug all the way into\n> reopen_lock_file(), but for some reason temporarily forgot that O_WRONLY\n> does not imply O_TRUNC.\n> \n> Arguably this should be the default for reopen_lockfile(), as getting a\n> write pointer into an existing file is not ever going to be useful for\n> the way Git uses lockfiles. Opening with O_APPEND could conceivably be\n> useful, but it's pretty unlikely (and certainly not helpful here, and\n> this is the only caller). Alternatively, the function should just take\n> open(2) flags.\n> \n> At any rate, I think this perfectly explains the behavior we're seeing.\n\nYes, this certainly make sense.\n\nFor fun (and testing) I tried to take the problem in the other direction:\n* why hasn't this be detected earlier (I'm reasonably be sure I did the\n  same operation a few times already without seeing the corruption)?\n* how easy it is to reproduce the problem?\n* Is it enough to do an interactive commit with nothing needing interactive?\n\nSo I tried the following and some variants:\n  > for i in $(seq -w 0 100); do echo $i > f$i; done\n  > git add f* && git commit -m add f* && git rm -q f* && git commit -m rm -p\n\nbut I didn't succeed to recreate the problem. So I'm still wondering\nwhat is special in my repository & tree that trigger the corruption.\n\nAnyway, thanks to al of you for looking at this so quickly.\n\n-- Luc\n"},{"id":"357186","messageId":"20180902080205.GD18787@sigill.intra.peff.net","threadId":"49266","inReplyTo":"20180902075351.lkfnekklnwot7lzf@ltop.local","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-02T08:02:06Z","receivedAt":"2018-09-02T08:02:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 02, 2018 at 09:53:53AM +0200, Luc Van Oostenryck wrote:\n\n> > At any rate, I think this perfectly explains the behavior we're seeing.\n> \n> Yes, this certainly make sense.\n> \n> For fun (and testing) I tried to take the problem in the other direction:\n> * why hasn't this be detected earlier (I'm reasonably be sure I did the\n>   same operation a few times already without seeing the corruption)?\n> * how easy it is to reproduce the problem?\n> * Is it enough to do an interactive commit with nothing needing interactive?\n> \n> So I tried the following and some variants:\n>   > for i in $(seq -w 0 100); do echo $i > f$i; done\n>   > git add f* && git commit -m add f* && git rm -q f* && git commit -m rm -p\n> \n> but I didn't succeed to recreate the problem. So I'm still wondering\n> what is special in my repository & tree that trigger the corruption.\n\nI think the partial deletion is important, because it means that the\nresulting index is going to be smaller. And the truncation problem only\nmatters when we go from a larger file to a smaller one (since otherwise\noverwrite the old content completely).\n\nAnd it doesn't seem to trigger without the interactive \"-p\". I think\nthat's not directly related, but it somehow triggers the case where we\nactually need to update the cache tree in this particular order.\n\nThat's pretty hand-wavy, but I think it gives a sense of why most people\ndidn't run into this. I do wish I understood better what it would take\nto create a minimal example for the test suite.\n\n-Peff\n"},{"id":"357299","messageId":"xmqq36upcl1s.fsf@gitster-ct.c.googlers.com","threadId":"49266","inReplyTo":"20180902072408.GA18787@sigill.intra.peff.net","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-04T15:57:35Z","receivedAt":"2018-09-04T15:57:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> diff --git a/builtin/commit.c b/builtin/commit.c\n>> index 2be7bdb331..60f30b3780 100644\n>> --- a/builtin/commit.c\n>> +++ b/builtin/commit.c\n>> @@ -432,6 +432,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n>>  \t\tif (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n>>  \t\t\tif (reopen_lock_file(&index_lock) < 0)\n>>  \t\t\t\tdie(_(\"unable to write index file\"));\n>> +\t\t\tftruncate(index_lock.tempfile->fd, 0);\n>>  \t\t\tif (write_locked_index(&the_index, &index_lock, 0))\n>>  \t\t\t\tdie(_(\"unable to update temporary index\"));\n>>  \t\t} else\n>\n> Doh, of course. I even thought about this issue and dug all the way into\n> reopen_lock_file(), but for some reason temporarily forgot that O_WRONLY\n> does not imply O_TRUNC.\n>\n> Arguably this should be the default for reopen_lockfile(), as getting a\n> write pointer into an existing file is not ever going to be useful for\n> the way Git uses lockfiles. Opening with O_APPEND could conceivably be\n> useful, but it's pretty unlikely (and certainly not helpful here, and\n> this is the only caller). Alternatively, the function should just take\n> open(2) flags.\n>\n> At any rate, I think this perfectly explains the behavior we're seeing.\n\nThanks all for digging this down (I am a bit jealous to see that I\nseem to have missed all this fun over the weekend X-<).\n"},{"id":"357301","messageId":"CACsJy8AuWKxAdETaqYmcSY2VeLeWFyjnSYrK4GJeyG5ecv3OcA@mail.gmail.com","threadId":"49266","inReplyTo":"xmqq36upcl1s.fsf@gitster-ct.c.googlers.com","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-04T16:13:36Z","receivedAt":"2018-09-04T16:14:05Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Sep 4, 2018 at 5:57 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeff King <peff@peff.net> writes:\n>\n> >> diff --git a/builtin/commit.c b/builtin/commit.c\n> >> index 2be7bdb331..60f30b3780 100644\n> >> --- a/builtin/commit.c\n> >> +++ b/builtin/commit.c\n> >> @@ -432,6 +432,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n> >>              if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {\n> >>                      if (reopen_lock_file(&index_lock) < 0)\n> >>                              die(_(\"unable to write index file\"));\n> >> +                    ftruncate(index_lock.tempfile->fd, 0);\n> >>                      if (write_locked_index(&the_index, &index_lock, 0))\n> >>                              die(_(\"unable to update temporary index\"));\n> >>              } else\n> >\n> > Doh, of course. I even thought about this issue and dug all the way into\n> > reopen_lock_file(), but for some reason temporarily forgot that O_WRONLY\n> > does not imply O_TRUNC.\n> >\n> > Arguably this should be the default for reopen_lockfile(), as getting a\n> > write pointer into an existing file is not ever going to be useful for\n> > the way Git uses lockfiles. Opening with O_APPEND could conceivably be\n> > useful, but it's pretty unlikely (and certainly not helpful here, and\n> > this is the only caller). Alternatively, the function should just take\n> > open(2) flags.\n> >\n> > At any rate, I think this perfectly explains the behavior we're seeing.\n>\n> Thanks all for digging this down (I am a bit jealous to see that I\n> seem to have missed all this fun over the weekend X-<).\n\nAnd just to be clear I'm looking forward to a patch from Jeff to fix\nthis since he clearly put more thoughts on this than me. With commit.c\nbeing the only user of reopen_lock_file() I guess it's even ok to just\nstick O_TRUNC in there and worry about O_APPEND when a new caller\nneeds that.\n-- \nDuy\n"},{"id":"357303","messageId":"20180904163807.GA23572@sigill.intra.peff.net","threadId":"49266","inReplyTo":"CACsJy8AuWKxAdETaqYmcSY2VeLeWFyjnSYrK4GJeyG5ecv3OcA@mail.gmail.com","subject":"Re: [BUG] index corruption with git commit -p","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-04T16:38:08Z","receivedAt":"2018-09-04T16:38:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 04, 2018 at 06:13:36PM +0200, Duy Nguyen wrote:\n\n> > > Doh, of course. I even thought about this issue and dug all the way into\n> > > reopen_lock_file(), but for some reason temporarily forgot that O_WRONLY\n> > > does not imply O_TRUNC.\n> > >\n> > > Arguably this should be the default for reopen_lockfile(), as getting a\n> > > write pointer into an existing file is not ever going to be useful for\n> > > the way Git uses lockfiles. Opening with O_APPEND could conceivably be\n> > > useful, but it's pretty unlikely (and certainly not helpful here, and\n> > > this is the only caller). Alternatively, the function should just take\n> > > open(2) flags.\n> > >\n> > > At any rate, I think this perfectly explains the behavior we're seeing.\n> >\n> > Thanks all for digging this down (I am a bit jealous to see that I\n> > seem to have missed all this fun over the weekend X-<).\n> \n> And just to be clear I'm looking forward to a patch from Jeff to fix\n> this since he clearly put more thoughts on this than me. With commit.c\n> being the only user of reopen_lock_file() I guess it's even ok to just\n> stick O_TRUNC in there and worry about O_APPEND when a new caller\n> needs that.\n\nThat's the way I'm leaning to. The fix is obviously a one-liner, but I\nwas hoping to construct a minimal test case. I just haven't gotten\naround to it yet.\n\nThe bug is ancient, so I don't think it's important for v2.19.\n\n-Peff\n"},{"id":"357404","messageId":"20180904233643.GA9156@sigill.intra.peff.net","threadId":"49266","inReplyTo":"20180904163807.GA23572@sigill.intra.peff.net","subject":"[PATCH] reopen_tempfile(): truncate opened file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-04T23:36:43Z","receivedAt":"2018-09-04T23:36:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 04, 2018 at 12:38:07PM -0400, Jeff King wrote:\n\n> > And just to be clear I'm looking forward to a patch from Jeff to fix\n> > this since he clearly put more thoughts on this than me. With commit.c\n> > being the only user of reopen_lock_file() I guess it's even ok to just\n> > stick O_TRUNC in there and worry about O_APPEND when a new caller\n> > needs that.\n> \n> That's the way I'm leaning to. The fix is obviously a one-liner, but I\n> was hoping to construct a minimal test case. I just haven't gotten\n> around to it yet.\n\nIt turned out not to be too bad to write a test. It feels a little like\nblack magic, since I empirically determined a way in which the\ncache-tree happens to shrink with the current code. But that assumption\nis tested with a sanity check, so we'll at least know if it becomes a\nnoop.\n\n> The bug is ancient, so I don't think it's important for v2.19.\n\nThe patch below should work on master or maint. We could do a fix\ndirectly on top of the bug, but merging-up is weird (because the buggy\ncode became part of a reusable module).\n\n-- >8 --\nSubject: [PATCH] reopen_tempfile(): truncate opened file\n\nWe provide a reopen_tempfile() function, which is in turn\nused by reopen_lockfile().  The idea is that a caller may\nwant to rewrite the tempfile without letting go of the lock.\nAnd that's what our one caller does: after running\nadd--interactive, \"commit -p\" will update the cache-tree\nextension of the index and write out the result, all while\nholding the lock.\n\nHowever, because we open the file with only the O_WRONLY\nflag, the existing index content is left in place, and we\noverwrite it starting at position 0. If the new index after\nupdating the cache-tree is smaller than the original, those\nfinal bytes are not overwritten and remain in the file. This\nresults in a corrupt index, since those cruft bytes are\ninterpreted as part of the trailing hash (or even as an\nextension, if there are enough bytes).\n\nThis bug actually pre-dates reopen_tempfile(); the original\ncode from 9c4d6c0297 (cache-tree: Write updated cache-tree\nafter commit, 2014-07-13) has the same bug, and those lines\nwere eventually refactored into the tempfile module. Nobody\nnoticed until now for two reasons:\n\n - the bug can only be triggered in interactive mode\n   (\"commit -p\" or \"commit -i\")\n\n - the size of the index must shrink after updating the\n   cache-tree, which implies a non-trivial deletion. Notice\n   that the included test actually has to create a 2-deep\n   hierarchy. A single level is not enough to actually cause\n   shrinkage.\n\nThe fix is to truncate the file before writing out the\nsecond index. We can do that at the caller by using\nftruncate(). But we shouldn't have to do that. There is no\nother place in Git where we want to open a file and\noverwrite bytes, making reopen_tempfile() a confusing and\nerror-prone interface. Let's pass O_TRUNC there, which gives\ncallers the same state they had after initially opening the\nfile or lock.\n\nIt's possible that we could later add a caller that wants\nsomething else (e.g., to open with O_APPEND). But this is\nthe only caller we've had in the history of the codebase.\nLet's punt on doing anything more clever until another one\ncomes along.\n\nReported-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n lockfile.h            |  4 ++--\n t/t0090-cache-tree.sh | 18 ++++++++++++++++++\n tempfile.c            |  2 +-\n tempfile.h            |  4 ++--\n 4 files changed, 23 insertions(+), 5 deletions(-)\n\ndiff --git a/lockfile.h b/lockfile.h\nindex f401c979f0..35403ccc0d 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -263,8 +263,8 @@ static inline int close_lock_file_gently(struct lock_file *lk)\n  *   nobody else) to inspect the contents you wrote, while still\n  *   holding the lock yourself.\n  *\n- * * `reopen_lock_file()` to reopen the lockfile. Make further updates\n- *   to the contents.\n+ * * `reopen_lock_file()` to reopen the lockfile, truncating the existing\n+ *   contents. Write out the new contents.\n  *\n  * * `commit_lock_file()` to make the final version permanent.\n  */\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 7de40141ca..94fcb4a78e 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -161,6 +161,24 @@ test_expect_success PERL 'commit --interactive gives cache-tree on partial commi\n \ttest_cache_tree\n '\n \n+test_expect_success PERL 'commit -p with shrinking cache-tree' '\n+\tmkdir -p deep/subdir &&\n+\techo content >deep/subdir/file &&\n+\tgit add deep &&\n+\tgit commit -m add &&\n+\tgit rm -r deep &&\n+\n+\tbefore=$(wc -c <.git/index) &&\n+\tgit commit -m delete -p &&\n+\tafter=$(wc -c <.git/index) &&\n+\n+\t# double check that the index shrank\n+\ttest $before -gt $after &&\n+\n+\t# and that our index was not corrupted\n+\tgit fsck\n+'\n+\n test_expect_success 'commit in child dir has cache-tree' '\n \tmkdir dir &&\n \t>dir/child.t &&\ndiff --git a/tempfile.c b/tempfile.c\nindex 139ecd97f8..d43ad8c191 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -279,7 +279,7 @@ int reopen_tempfile(struct tempfile *tempfile)\n \t\tBUG(\"reopen_tempfile called for an inactive object\");\n \tif (0 <= tempfile->fd)\n \t\tBUG(\"reopen_tempfile called for an open object\");\n-\ttempfile->fd = open(tempfile->filename.buf, O_WRONLY);\n+\ttempfile->fd = open(tempfile->filename.buf, O_WRONLY|O_TRUNC);\n \treturn tempfile->fd;\n }\n \ndiff --git a/tempfile.h b/tempfile.h\nindex 36434eb6fa..61d8dc4d1b 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -236,8 +236,8 @@ extern int close_tempfile_gently(struct tempfile *tempfile);\n  *   it (and nobody else) to inspect or even modify the file's\n  *   contents.\n  *\n- * * `reopen_tempfile()` to reopen the temporary file. Make further\n- *   updates to the contents.\n+ * * `reopen_tempfile()` to reopen the temporary file, truncating the existing\n+ *   contents. Write out the new contents.\n  *\n  * * `rename_tempfile()` to move the file to its permanent location.\n  */\n-- \n2.19.0.rc1.605.g83416793fa\n\n"},{"id":"357435","messageId":"CACsJy8Ax4S9Sms6TY1dMV8M9-=hakEW8TCqn8yxb73Vbrpy_MQ@mail.gmail.com","threadId":"49266","inReplyTo":"20180904233643.GA9156@sigill.intra.peff.net","subject":"Re: [PATCH] reopen_tempfile(): truncate opened file","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-05T15:27:11Z","receivedAt":"2018-09-05T15:27:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 5, 2018 at 1:36 AM Jeff King <peff@peff.net> wrote:\n> It turned out not to be too bad to write a test. It feels a little like\n> black magic, since I empirically determined a way in which the\n> cache-tree happens to shrink with the current code.\n\nAha! I attempted to reproduce with a verylongpathname and failed, but\nthen I had the main index portion in mind, not cache-tree.\n\n> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\n> index 7de40141ca..94fcb4a78e 100755\n> --- a/t/t0090-cache-tree.sh\n> +++ b/t/t0090-cache-tree.sh\n> @@ -161,6 +161,24 @@ test_expect_success PERL 'commit --interactive gives cache-tree on partial commi\n>         test_cache_tree\n>  '\n>\n> +test_expect_success PERL 'commit -p with shrinking cache-tree' '\n> +       mkdir -p deep/subdir &&\n> +       echo content >deep/subdir/file &&\n> +       git add deep &&\n> +       git commit -m add &&\n> +       git rm -r deep &&\n\nOK so I guess at this step, we invalidate some cache-tree blocks, but\nwe write the same blocks down (with \"invalid\" flag), so pretty much\nthe same size as before.\n\n> +\n> +       before=$(wc -c <.git/index) &&\n> +       git commit -m delete -p &&\n\nThen inside this command we recompute cache-tree and throw away the\ninvalidated cache-tree blocks for deep and deep/subdir, which shrinks\nthe index. Makes sense.\n\n> +       after=$(wc -c <.git/index) &&\n> +\n> +       # double check that the index shrank\n> +       test $before -gt $after &&\n> +\n> +       # and that our index was not corrupted\n> +       git fsck\n\nIf the index is not shrunk, we parse remaining rubbish as extensions.\nIf by chance the rubbish extension name is in uppercase, then we\nignore (and not flag it as error). But then the chances of the next 4\nbytes being the \"right\" extension size is so small that we would end\nup flagging it as bad extension anyway. So it's good. But if you want\nto be even stricter (not necessary in my opinion), make sure that\nstderr is empty.\n\nThe rest of the patch is obviously correct.\n-- \nDuy\n"},{"id":"357436","messageId":"20180905153551.GB24660@sigill.intra.peff.net","threadId":"49266","inReplyTo":"CACsJy8Ax4S9Sms6TY1dMV8M9-=hakEW8TCqn8yxb73Vbrpy_MQ@mail.gmail.com","subject":"Re: [PATCH] reopen_tempfile(): truncate opened file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-05T15:35:52Z","receivedAt":"2018-09-05T15:35:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 05, 2018 at 05:27:11PM +0200, Duy Nguyen wrote:\n\n> > +test_expect_success PERL 'commit -p with shrinking cache-tree' '\n> > +       mkdir -p deep/subdir &&\n> > +       echo content >deep/subdir/file &&\n> > +       git add deep &&\n> > +       git commit -m add &&\n> > +       git rm -r deep &&\n> \n> OK so I guess at this step, we invalidate some cache-tree blocks, but\n> we write the same blocks down (with \"invalid\" flag), so pretty much\n> the same size as before.\n\nI didn't verify exactly what was in the index, but that was my\nunderstanding, too (well, it's a little smaller because we drop the\nactual index entries, but keep the invalidated cache-tree). I worry a\nlittle that \"rm\" might eventually learn to drop those invalidated bits.\nBut hopefully finding this commit would lead that person to figure out\nanother way to accomplish the same thing, or to decide that carrying the\ntest forward isn't worth it.\n\n> > +       after=$(wc -c <.git/index) &&\n> > +\n> > +       # double check that the index shrank\n> > +       test $before -gt $after &&\n> > +\n> > +       # and that our index was not corrupted\n> > +       git fsck\n> \n> If the index is not shrunk, we parse remaining rubbish as extensions.\n> If by chance the rubbish extension name is in uppercase, then we\n> ignore (and not flag it as error). But then the chances of the next 4\n> bytes being the \"right\" extension size is so small that we would end\n> up flagging it as bad extension anyway. So it's good. But if you want\n> to be even stricter (not necessary in my opinion), make sure that\n> stderr is empty.\n\nIn this case, the size difference is only a few bytes, so the rubbish\nactually ends up in the trailing sha1. The reason I use git-fsck here is\nthat it actually verifies the whole sha1 (since normal index reads no\nlonger do). In fact, a normal index read won't show any problem for this\ncase (since it is _only_ the trailing sha1 which is junk, and we no\nlonger verify it on every read).\n\nIn the original sparse-dev case, the size of the rubbish is much larger\n(because we deleted a lot more entries), and we do interpret it as a\nbogus extension. But it also triggers here, because the trailing sha1 is\n_also_ wrong.\n\nSo AFAIK this fsck catches everything and yields a non-zero exit in the\nerror case. And it should work for even a single byte of rubbish.\n\n-Peff\n"},{"id":"357437","messageId":"CACsJy8BGxqzjXUprnhSU7jQDjzgDnY4x+SMsnOVb4Uho4dJt0g@mail.gmail.com","threadId":"49266","inReplyTo":"20180905153551.GB24660@sigill.intra.peff.net","subject":"Re: [PATCH] reopen_tempfile(): truncate opened file","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-05T15:39:19Z","receivedAt":"2018-09-05T15:39:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Sep 5, 2018 at 5:35 PM Jeff King <peff@peff.net> wrote:\n> > > +       after=$(wc -c <.git/index) &&\n> > > +\n> > > +       # double check that the index shrank\n> > > +       test $before -gt $after &&\n> > > +\n> > > +       # and that our index was not corrupted\n> > > +       git fsck\n> >\n> > If the index is not shrunk, we parse remaining rubbish as extensions.\n> > If by chance the rubbish extension name is in uppercase, then we\n> > ignore (and not flag it as error). But then the chances of the next 4\n> > bytes being the \"right\" extension size is so small that we would end\n> > up flagging it as bad extension anyway. So it's good. But if you want\n> > to be even stricter (not necessary in my opinion), make sure that\n> > stderr is empty.\n>\n> In this case, the size difference is only a few bytes, so the rubbish\n> actually ends up in the trailing sha1. The reason I use git-fsck here is\n> that it actually verifies the whole sha1 (since normal index reads no\n> longer do). In fact, a normal index read won't show any problem for this\n> case (since it is _only_ the trailing sha1 which is junk, and we no\n> longer verify it on every read).\n>\n> In the original sparse-dev case, the size of the rubbish is much larger\n> (because we deleted a lot more entries), and we do interpret it as a\n> bogus extension. But it also triggers here, because the trailing sha1 is\n> _also_ wrong.\n>\n> So AFAIK this fsck catches everything and yields a non-zero exit in the\n> error case. And it should work for even a single byte of rubbish.\n\nYes you're right. I forgot about the trailing hash.\n-- \nDuy\n"},{"id":"357438","messageId":"20180905154827.GC24660@sigill.intra.peff.net","threadId":"49266","inReplyTo":"CACsJy8BGxqzjXUprnhSU7jQDjzgDnY4x+SMsnOVb4Uho4dJt0g@mail.gmail.com","subject":"Re: [PATCH] reopen_tempfile(): truncate opened file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-05T15:48:27Z","receivedAt":"2018-09-05T15:48:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 05, 2018 at 05:39:19PM +0200, Duy Nguyen wrote:\n\n> On Wed, Sep 5, 2018 at 5:35 PM Jeff King <peff@peff.net> wrote:\n> > > > +       after=$(wc -c <.git/index) &&\n> > > > +\n> > > > +       # double check that the index shrank\n> > > > +       test $before -gt $after &&\n> > > > +\n> > > > +       # and that our index was not corrupted\n> > > > +       git fsck\n> > >\n> > > If the index is not shrunk, we parse remaining rubbish as extensions.\n> > > If by chance the rubbish extension name is in uppercase, then we\n> > > ignore (and not flag it as error). But then the chances of the next 4\n> > > bytes being the \"right\" extension size is so small that we would end\n> > > up flagging it as bad extension anyway. So it's good. But if you want\n> > > to be even stricter (not necessary in my opinion), make sure that\n> > > stderr is empty.\n> >\n> > In this case, the size difference is only a few bytes, so the rubbish\n> > actually ends up in the trailing sha1. The reason I use git-fsck here is\n> > that it actually verifies the whole sha1 (since normal index reads no\n> > longer do). In fact, a normal index read won't show any problem for this\n> > case (since it is _only_ the trailing sha1 which is junk, and we no\n> > longer verify it on every read).\n> >\n> > In the original sparse-dev case, the size of the rubbish is much larger\n> > (because we deleted a lot more entries), and we do interpret it as a\n> > bogus extension. But it also triggers here, because the trailing sha1 is\n> > _also_ wrong.\n> >\n> > So AFAIK this fsck catches everything and yields a non-zero exit in the\n> > error case. And it should work for even a single byte of rubbish.\n> \n> Yes you're right. I forgot about the trailing hash.\n\nThanks, I was worried that I was missing something. ;)\n\nMaybe it is worth making that final comment:\n\n  # and that the trailing hash in the index was not corrupted,\n  # which should catch even a single byte of cruft\n  git fsck\n\n-Peff\n"},{"id":"357446","messageId":"xmqqva7j51gt.fsf@gitster-ct.c.googlers.com","threadId":"49266","inReplyTo":"20180905154827.GC24660@sigill.intra.peff.net","subject":"Re: [PATCH] reopen_tempfile(): truncate opened file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-05T16:54:42Z","receivedAt":"2018-09-05T16:54:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > So AFAIK this fsck catches everything and yields a non-zero exit in the\n>> > error case. And it should work for even a single byte of rubbish.\n>> \n>> Yes you're right. I forgot about the trailing hash.\n>\n> Thanks, I was worried that I was missing something. ;)\n>\n> Maybe it is worth making that final comment:\n>\n>   # and that the trailing hash in the index was not corrupted,\n>   # which should catch even a single byte of cruft\n>   git fsck\n\nPerhaps.  I do not mind seeing an additional comment to explain why\nthis requires PERL (it wasn't immediately obvious as I never use\n'commit -p' myself), either.\n"},{"id":"357448","messageId":"20180905165625.GA31930@sigill.intra.peff.net","threadId":"49266","inReplyTo":"xmqqva7j51gt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] reopen_tempfile(): truncate opened file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-09-05T16:56:26Z","receivedAt":"2018-09-05T16:56:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 05, 2018 at 09:54:42AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> > So AFAIK this fsck catches everything and yields a non-zero exit in the\n> >> > error case. And it should work for even a single byte of rubbish.\n> >> \n> >> Yes you're right. I forgot about the trailing hash.\n> >\n> > Thanks, I was worried that I was missing something. ;)\n> >\n> > Maybe it is worth making that final comment:\n> >\n> >   # and that the trailing hash in the index was not corrupted,\n> >   # which should catch even a single byte of cruft\n> >   git fsck\n> \n> Perhaps.  I do not mind seeing an additional comment to explain why\n> this requires PERL (it wasn't immediately obvious as I never use\n> 'commit -p' myself), either.\n\nI thought the PERL prereq in the existing \"-p\" test of the commit header\nwould explain it. ;)\n\nDo you prefer an in-code comment, or one in the commit message?\n\n-Peff\n"},{"id":"357449","messageId":"xmqqr2i7515r.fsf@gitster-ct.c.googlers.com","threadId":"49266","inReplyTo":"20180905165625.GA31930@sigill.intra.peff.net","subject":"Re: [PATCH] reopen_tempfile(): truncate opened file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-05T17:01:20Z","receivedAt":"2018-09-05T17:01:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Sep 05, 2018 at 09:54:42AM -0700, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> >> > So AFAIK this fsck catches everything and yields a non-zero exit in the\n>> >> > error case. And it should work for even a single byte of rubbish.\n>> >> \n>> >> Yes you're right. I forgot about the trailing hash.\n>> >\n>> > Thanks, I was worried that I was missing something. ;)\n>> >\n>> > Maybe it is worth making that final comment:\n>> >\n>> >   # and that the trailing hash in the index was not corrupted,\n>> >   # which should catch even a single byte of cruft\n>> >   git fsck\n>> \n>> Perhaps.  I do not mind seeing an additional comment to explain why\n>> this requires PERL (it wasn't immediately obvious as I never use\n>> 'commit -p' myself), either.\n>\n> I thought the PERL prereq in the existing \"-p\" test of the commit header\n> would explain it. ;)\n>\n> Do you prefer an in-code comment, or one in the commit message?\n\nNeither ;-)  \n\nJust like I think 'our index was not corrupted' as an explanation\nfor 'git fsck' is sufficient, PERL sprinkled all over this script\nand all of them tend to be near \"commit -i/-p\" should be a good\nenough clue, I'd think.\n"},{"id":"357465","messageId":"20180905184834.ihpjardxnny6d54p@ltop.local","threadId":"49266","inReplyTo":"20180904233643.GA9156@sigill.intra.peff.net","subject":"Re: [PATCH] reopen_tempfile(): truncate opened file","fromName":"Luc Van Oostenryck","fromEmail":"luc.vanoostenryck@gmail.com","sentAt":"2018-09-05T18:48:34Z","receivedAt":"2018-09-05T18:48:39Z","isPatch":true,"sender":{"key":"luc.vanoostenryck@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16168334?v=4"},"body":"On Tue, Sep 04, 2018 at 07:36:43PM -0400, Jeff King wrote:\n> On Tue, Sep 04, 2018 at 12:38:07PM -0400, Jeff King wrote:\n> \n> > > And just to be clear I'm looking forward to a patch from Jeff to fix\n> > > this since he clearly put more thoughts on this than me. With commit.c\n> > > being the only user of reopen_lock_file() I guess it's even ok to just\n> > > stick O_TRUNC in there and worry about O_APPEND when a new caller\n> > > needs that.\n> > \n> > That's the way I'm leaning to. The fix is obviously a one-liner, but I\n> > was hoping to construct a minimal test case. I just haven't gotten\n> > around to it yet.\n> \n> It turned out not to be too bad to write a test. It feels a little like\n> black magic, since I empirically determined a way in which the\n> cache-tree happens to shrink with the current code. But that assumption\n> is tested with a sanity check, so we'll at least know if it becomes a\n> noop.\n> \n> > The bug is ancient, so I don't think it's important for v2.19.\n> \n> The patch below should work on master or maint. We could do a fix\n> directly on top of the bug, but merging-up is weird (because the buggy\n> code became part of a reusable module).\n\nIt's great that you were able to create a reproducer relatively easily.\n\nThank you, guys.\n\n-- Luc \n"}]}