Re: Bug report: Strange behavior with `git gc` and `reference-transaction` hook

From: Jeff King <peff@peff.net>
Date: Nov 18, 2021, 18:08 UTC
Message-ID: <YZaWqTwPOyQz0/mu@coredump.intra.peff.net>
In-Reply-To: <CAGyf7-FoRyVtQHa2ETQtRA6fD7x0GDhKVPg+eAajhgPNrsw_OQ@mail.gmail.com>

[+cc pks]

On Wed, Nov 17, 2021 at 04:52:46PM -0800, Bryan Turner wrote:

Show 14 quoted lines

> > The expected behavior would be that the latest reference transaction
> > hook refers to the state of the references on disk. That is, either
> > `master` should point to 0 (be deleted), or it should have said that
> > `master` pointed to `e197d1`.
> >
> > But if we actually examine `master`, it's set to `e197d1`, just as you
> > would expect. The GC should have been a no-op overall.
> 
> One of the subtasks of "git gc" is "git pack-refs". If you inspect in
> more detail, I suspect you'll find that "refs/heads/master" was loose
> before "git gc" ran (as in, there was a file
> "$GIT_DIR/refs/heads/master") and "packed-refs" either didn't have a
> "refs/heads/master" entry or had a different hash. (Loose refs always
> "win" over packed, since ref updates only write loose refs.)

It seems totally broken to me that we'd trigger the reference-transaction hook for ref packing. The point of the hook is to track logical updates to the refs. But during ref packing that does not change at all; the value remains the same. So I don't think we should be triggering the hook at all, let alone with confusing values.

This snippet shows a simple case that I think is wrong:

-- >8 -- git init -q repo cd repo

cat >.git/hooks/reference-transaction <<\EOF #!/bin/sh echo >&2 "==> reference-transaction $*" sed 's/^/ /' EOF chmod +x .git/hooks/reference-transaction

echo >&2 "running commit..." git commit --allow-empty -qm foo echo >&2 "running pack-refs..." git pack-refs --all --prune -- >8 --

It produces:

  running commit...
  ==> reference-transaction prepared
    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b HEAD
    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main
  ==> reference-transaction committed
    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b HEAD
    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main
  running pack-refs...
  ==> reference-transaction prepared
    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main
  ==> reference-transaction committed
    0000000000000000000000000000000000000000 77bcab0d950aee3021e8aa13a15d40e7a9a5f71b refs/heads/main
  ==> reference-transaction prepared
    77bcab0d950aee3021e8aa13a15d40e7a9a5f71b 0000000000000000000000000000000000000000 refs/heads/main
  ==> reference-transaction committed
    77bcab0d950aee3021e8aa13a15d40e7a9a5f71b 0000000000000000000000000000000000000000 refs/heads/main

I think the final four invocations should be skipped entirely. They're pointless at best (nothing actually changed), and extremely misleading at worst (they look like the ref ended up deleted!).

-Peff