Re: What's cooking in git.git (Sep 2021, #08; Mon, 27)

From: Jeff King <peff@peff.net>
Date: Sep 28, 2021, 23:40 UTC
Message-ID: <YVOn3hDsb5pnxR53@coredump.intra.peff.net>
In-Reply-To: <CABPp-BGuzd_TH57-1RvwJQD5r3S3ZkJcuiPnU8aWee8pgzUBEw@mail.gmail.com>

On Mon, Sep 27, 2021 at 11:46:40PM -0700, Elijah Newren wrote:

Show 24 quoted lines

> > * en/remerge-diff (2021-08-31) 7 commits
> >  - doc/diff-options: explain the new --remerge-diff option
> >  - show, log: provide a --remerge-diff capability
> >  - tmp-objdir: new API for creating and removing primary object dirs
> >  - merge-ort: capture and print ll-merge warnings in our preferred fashion
> >  - ll-merge: add API for capturing warnings in a strbuf instead of stderr
> >  - merge-ort: add ability to record conflict messages in a file
> >  - merge-ort: mark a few more conflict messages as omittable
> >
> >  A new presentation for two-parent merge "--remerge-diff" can be
> >  used to show the difference between mechanical (and possibly
> >  conflicted) merge results and the recorded resolution.
> >
> >  Will merge to 'next'?
> 
> It has been a month that it's been cooking with no issues brought up,
> and it's been in production for nearly a year...
> 
> But just this morning I pinged peff and jrnieder if they might have
> time to respectively look at the tmp-objdir stuff (patch 5, plus its
> integration into log-tree.c in patch 7) and the ll-merge.[ch] changes
> (patch 3).  I don't know if either will have time to do it, but
> perhaps wait half a week or so to see if they'll mention they have
> time?  Otherwise, yeah, it's probably time to merge this down.

Sorry to take so long. I think this is a very exciting topic, and I appreciate being called into to look at tmp-objdir stuff, because it can be quite subtle.

I just left a rather long-ish mail in the thread, but the gist of it is that I'm worried that there's some possibility of corruption there if the diff code writes objects. I didn't do a proof-of-concept there, but I worked one up just now. Try this:

  # make a repo
  git init repo
  cd repo
  echo base >file
  git add file
  git commit -m base

  # two diverging branches
  echo main >file
  git commit -am main
  git checkout -b side HEAD^
  echo side >file
  git commit -am side

  # we get a conflict, and we resolve
  git merge main
  echo resolved >file
  git commit -am merged

  # now imagine we had a file with a diff driver. I stuffed it
  # in here after the fact, but it could have been here all along,
  # or come as part of the merge, etc.
  echo whatever >unrelated
  echo "unrelated diff=foo" >.gitattributes
  git add .
  git commit --amend --no-edit

  # set up the diff driver not just to do a textconv, but to cache the
  # result. This will require writing out new objects for the cache
  # as part of the diff operation.
  git config diff.foo.textconv "$PWD/upcase"
  git config diff.foo.cachetextconv true
  cat >upcase <<\EOF &&
  #!/bin/sh
  tr a-z A-Z <$1
  EOF
  chmod +x upcase

  # now show the diff
  git log -1 --remerge-diff

  # and make sure the repo is still OK
  git fsck

The remerge diff will correctly show the textconv'd content (because it's not in either parent, and hence an evil merge):

  diff --git a/unrelated b/unrelated
  new file mode 100644
  index 0000000..982793c
  --- /dev/null
  +++ b/unrelated
  @@ -0,0 +1 @@
  +WHATEVER

but then fsck shows that our cache is corrupted:

  Checking object directories: 100% (256/256), done.
  error: refs/notes/textconv/foo: invalid sha1 pointer 1e9b3ecffca73411001043fe914a7ec0e7291043
  error: refs/notes/textconv/foo: invalid reflog entry 1e9b3ecffca73411001043fe914a7ec0e7291043
  dangling tree 569b6e414203d2f50bdaafc1585eae11e28afc37

Now I'll admit the textconv-cache is a pretty seldom-used feature. And that there _probably_ aren't a lot of other ways that the diff code would try to write objects or references. But I think it speaks to the kind of subtle interactions we should worry about (and certainly corrupting the repository is a pretty bad outcome, though at least in this case it's "just" a cache and we could blow away that ref manually).

-Peff