On Tue, Sep 4, 2018 at 5:57 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Jeff King <peff@xxxxxxxx> 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
