git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [BUG] index corruption with git commit -p

From
Duy Nguyen <pclouds@gmail.com>
Date
Sep 4, 2018, 16:13 UTC
Message-ID
<CACsJy8AuWKxAdETaqYmcSY2VeLeWFyjnSYrK4GJeyG5ecv3OcA@mail.gmail.com>
In-Reply-To
<xmqq36upcl1s.fsf@gitster-ct.c.googlers.com>
On Tue, Sep 4, 2018 at 5:57 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 31 quoted lines
>
> Jeff King <peff@peff.net> writes:
>
> >> diff --git a/builtin/commit.c b/builtin/commit.c
> >> index 2be7bdb331..60f30b3780 100644
> >> --- a/builtin/commit.c
> >> +++ b/builtin/commit.c
> >> @@ -432,6 +432,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
> >>              if (update_main_cache_tree(WRITE_TREE_SILENT) == 0) {
> >>                      if (reopen_lock_file(&index_lock) < 0)
> >>                              die(_("unable to write index file"));
> >> +                    ftruncate(index_lock.tempfile->fd, 0);
> >>                      if (write_locked_index(&the_index, &index_lock, 0))
> >>                              die(_("unable to update temporary index"));
> >>              } else
> >
> > Doh, of course. I even thought about this issue and dug all the way into
> > reopen_lock_file(), but for some reason temporarily forgot that O_WRONLY
> > does not imply O_TRUNC.
> >
> > Arguably this should be the default for reopen_lockfile(), as getting a
> > write pointer into an existing file is not ever going to be useful for
> > the way Git uses lockfiles. Opening with O_APPEND could conceivably be
> > useful, but it's pretty unlikely (and certainly not helpful here, and
> > this is the only caller). Alternatively, the function should just take
> > open(2) flags.
> >
> > At any rate, I think this perfectly explains the behavior we're seeing.
>
> Thanks all for digging this down (I am a bit jealous to see that I
> seem to have missed all this fun over the weekend X-<).

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.

-- 
Duy
Previous: Junio C HamanoNext: Jeff King
Message 9 of 19 in “[BUG] index corruption with git commit -p”
  1. Luc Van OostenryckSep 1, 2018
  2. Ævar Arnfjörð BjarmasonSep 1, 2018
  3. Jeff KingSep 2, 2018
  4. Duy NguyenSep 2, 2018
  5. Jeff KingSep 2, 2018
  6. Luc Van OostenryckSep 2, 2018
  7. Jeff KingSep 2, 2018
  8. Junio C HamanoSep 4, 2018
  9. Duy NguyenSep 4, 2018
  10. Jeff KingSep 4, 2018
  11. reopen_tempfile(): truncate opened fileJeff King, Sep 4, 2018
  12. Duy NguyenSep 5, 2018
  13. Jeff KingSep 5, 2018
  14. Duy NguyenSep 5, 2018
  15. Jeff KingSep 5, 2018
  16. Junio C HamanoSep 5, 2018
  17. Jeff KingSep 5, 2018
  18. Junio C HamanoSep 5, 2018
  19. Luc Van OostenryckSep 5, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.