Re: Bug: `git init` with hook `reference-transaction` running `git rev-parse --git-dir` fails

[Gabriel Nützi <gnuetzi@xxxxxxxxx>]

Hi Patrick,

Thanks for clarifications! Could it work if for (2) -> call the reference-transaction hook after the HEAD has been initialized, meaning that Git would internally cache the different reference transactions and then call the hooks in one go at once after the creation of the repo, such that it is initialized properly? This might be probably a more elaborate change which introduces too many technicalities?

Gabriel

-----Original Message-----
From: Patrick Steinhardt <ps@xxxxxx>
To: Gabriel Nützi <gnuetzi@xxxxxxxxx>
Cc: git@xxxxxxxxxxxxxxx, Karthik Nayak <karthik.188@xxxxxxxxx>
Subject: Re: Bug: git init with hook reference-transaction running git rev-parse --git-dir fails
Date: 10/07/2024 10:03:30 AM

On Fri, Sep 20, 2024 at 12:07:53PM +0200, Gabriel Nützi wrote:

Thank you for filling out a Git bug report! Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)

I set git config --global core.hooksPath ~/myhooks and placed a reference-transaction hook in ~/myhooks/reference-transaction with the content:

#!/usr/bin/env bash

set -e
echo "$GIT_DIR"
git rev-parse --absolute-git-dir

then I ran

mkdir ~/test && cd test
git init

What did you expect to happen? (Expected behavior)

The Git repo ~/test should have been initialized (and the hook reference-transaction would have passed successfully.)

What happened instead? (Actual behavior)

The hook reference-transaction crashes since git rev-parse -- absolute-git-dir with

failed: not a git repository: ...

What's different between what you expected and what actually happened?

The documentation says that git rev-parse --absolute-git-dir inside the reference-transaction hooks read "$GIT_DIR" if defined (which is defined!) so the reference-transaction should have passed. I assume that hooks should be executed on properly initialized repositories, right? Therefore I do not understand why git rev-parse --absolute-git- dir fails -> Bug?

Anything else you want to add:

This came up with Githooks hooks manager https://github.com/gabyx/Githooks where we use this command to locate the current Git dir...

Please review the rest of the bug report below. You can delete any lines you don't wish to share.

Thanks for your bug report, and sorry for taking so long to respond.
Reproducing the observed behaviour is quite simple:

    test_expect_success 'git-init with global hook' '
        test_when_finished "rm -rf hooks repo" &&
        mkdir hooks &&
        write_script hooks/reference-transaction <<-EOF &&
        git rev-parse --absolute-git-dir >>"$(pwd)/reftx-logs"
        EOF
        test_config --global core.hooksPath "$(pwd)/hooks" &&
        git init repo
    '

This breakage is new in Git v2.46 and comes from the patch series that
introduces support for symbolic refs in the reftx hook via a8ae923f85
(refs: support symrefs in 'reference-transaction' hook, 2024-05-07) .
Before that change we didn't execute the hook for "HEAD" in the first
place, now we do.

Now the question is whether this is a bug or not. When the reftx hook
executes the first time it is when we are creating the "HEAD" ref in
the repo. Consequently, that file did not yet exist beforehand. And as
Git only considers something a repository when the "HEAD" file exists it
rightfully complains that this is not a valid Git repository when you
ask it to resolve the repo paths. So conceptually, the behaviour here is
correct.

There are two ways we could fix this that I can think of:

  - We can create a dummy "HEAD" file with invalid contents such that we
    do have a proper Git repository when creating "HEAD". It feels like
    a bit of a hack though, but we play similar games in git-clone(1).

  - We can skip execution of the "reference-transaction" hook during
    initialization of the repository. But this would make us miss some
    ref updates, which feels conceptually wrong.

I'd rule out (2), but (1) could be feasible if we label this a bug. I'm
not a 100% sure whether we should, as you could also argue that this is
reflecting the actual state of the repo. I'd be happy to hear arguments
in either direction.

Also Cc'd Karthik, the author of the menitoned change.

Patrick

Attachment: signature.asc
Description: This is a digitally signed message part