[RFC PATCH v2 0/4] Improve error reporting to mention "why" a directory is not a repository

At the moment, there are a few scenarios where the error message for an invalid Git repository is a bit blunt. For instance, when we point GIT_OBJECT_DIRECTORY at a directory that does not exist, we get this:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'

Even though repo.git itself is a valid repository, we get this rather puzzling error saying that it is not. The actual problem is the invalid value given to GIT_OBJECT_DIRECTORY, and the user is left on their to figure that out. This series aims to make such issues easier to diagnose by saying why the specified repository was not considered valid.

With this series, the same command outputs:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git --git-dir repo.git rev-parse --is-bare-repository
  fatal: not a git repository: 'repo.git'
  reason: cannot access object directory '/does/not/exist' set via $GIT_OBJECT_DIRECTORY

The following scenarios are covered when --git-dir is given explicitly:

  - HEAD is missing, or its path is not traversable
  - HEAD is a symlink that cannot be read
  - HEAD is a symlink whose target lives outside refs/
  - HEAD cannot be opened, or cannot be read
  - HEAD contains neither a ref under refs/ nor an object ID
  - $GIT_OBJECT_DIRECTORY is set to something we cannot access
  - the object directory in the common directory is inaccessible
  - the refs directory in the common directory is inaccessible

This series does not yet report a reason in the following cases.

The discovery walk, where we iterate up to the ceiling or the mount point looking for a repository:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git rev-parse --is-bare-repository
  fatal: not a git repository (or any parent up to mount point /)
  Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)

The gitfile case, for instance when run from inside a submodule / worktree:

  $ GIT_OBJECT_DIRECTORY=/does/not/exist git rev-parse --is-bare-repository
  fatal: gitfile does not point to a valid repository: /path/to/super/sub/.git

GIT_ALTERNATE_OBJECT_DIRECTORIES is also left alone. Its diagnostic are misleading in their own way, but they are emitted much later, from the object database rather than from setup, so they need a separate treatment.

I would be very interested in hearing what others think about this direction. If it looks reasonable, I'm happy to cover the remaining cases too, either in this series or separately.

CI:

One symlink related test appears to be failing in Windows. I'll check the same. All others pass.

See here: https://github.com/sivaraam/git/actions/runs/36548531586

Changes since v2:

- Included 1/4 which fixes a style nit
- Dropped the sub-shell from the test that was unnecessary
- Fixed some style issue and tried to improve the code intentation corresponding
  to the strbuf construction code
- Replaced sentence lego with a single string even though that means
  additional work for translators. On the plus side, avoiding sentence
  lego provides them more context about the message.

Range-diff between v1 and v2

-:  ---------- > 1:  17b72331d0 setup: normalize an if-else to follow our convention
1:  b4d7a4efa2 ! 2:  962418c9f3 t0009: add tests to cover more error reporting scenarios
    @@ t/t0009-git-dir-validation.sh: test_expect_success 'setup: .git as an empty dire
     +test_expect_success 'setup: custom git directory with missing HEAD is rejected' '
     +	test_when_finished "rm -rf parent/empty-dir" &&
     +	mkdir -p parent/empty-dir &&
    -+	(
    -+		test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
    -+		test_grep "not a git repository" stderr
    -+	)
    ++	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
    ++	test_grep "not a git repository" stderr
     +'
     +
     +test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
2:  8591dc4162 ! 3:  4a1503a6dc setup: introduce new helper 'is_git_directory_verbose'
    @@ setup.c: static int validate_headref(const char *path)
     -	if (lstat(path, &st) < 0)
     +	if (lstat(path, &st) < 0) {
     +		if (err)
    -+			strbuf_addf(
    -+				err, _("could not stat HEAD at '%s'"), path
    -+			);
    ++			strbuf_addf(err, _("could not stat HEAD at '%s'"), path);
      		return -1;
     +	}
      
    @@ setup.c: static int validate_headref(const char *path)
      		if (len >= 5 && !memcmp("refs/", buffer, 5))
      			return 0;
     +		if (len == -1 && err)
    -+			strbuf_addf(
    -+				err,
    -+				_("could not read the symlink HEAD at '%s'"),
    -+				path
    -+			);
    ++			strbuf_addf(err, _("could not read the symlink HEAD at '%s'"),
    ++				    path);
     +		else if (err)
    -+			strbuf_addf(
    -+				err,
    -+				_("HEAD is a symlink ('%s') but target"
    -+				  " lives outside refs/"),
    -+				path
    -+			);
    ++			strbuf_addf(err, _("HEAD is a symlink ('%s') but target lives"
    ++					   " outside refs/"), path);
      		return -1;
      	}
      
    @@ setup.c: static int validate_headref(const char *path)
     -	if (fd < 0)
     +	if (fd < 0) {
     +		if (err)
    -+			strbuf_addf(
    -+				err, _("could not open HEAD at '%s'"), path
    -+			);
    ++			strbuf_addf(err, _("could not open HEAD at '%s'"), path);
      		return -1;
     +	}
      	len = read_in_full(fd, buffer, sizeof(buffer)-1);
    @@ setup.c: static int validate_headref(const char *path)
     -	if (len < 0)
     +	if (len < 0) {
     +		if (err)
    -+			strbuf_addf(
    -+				err, _("could not read HEAD at '%s'"), path
    -+			);
    ++			strbuf_addf(err, _("could not read HEAD at '%s'"), path);
      		return -1;
     +	}
      	buffer[len] = '\0';
    @@ setup.c: static int validate_headref(const char *path)
      		return 0;
      
     +	if (err)
    -+		strbuf_addf(
    -+			err,
    -+			_("HEAD at '%s' does not point to a valid"
    -+			  " symbolic link or an object ID"),
    -+			path
    -+		);
    ++		strbuf_addf(err, _("HEAD at '%s' does not point to a valid symbolic"
    ++				   " link or an object ID"), path);
     +
      	return -1;
      }
    @@ setup.c: static int validate_headref(const char *path)
     +	if (objdir) {
     +		if (access(objdir, X_OK)) {
     +			if (err)
    -+				strbuf_addf(
    -+					err,
    -+					_("cannot access object directory '%s'"
    -+					  " set via $%s\n"),
    -+				objdir,
    -+				DB_ENVIRONMENT
    -+			);
    ++				strbuf_addf(err, _("cannot access object directory '%s'"
    ++						   "set via $%s\n"), objdir, DB_ENVIRONMENT);
     +			goto done;
     +		}
    -+	}
    -+	else {
    ++	} else {
     +		strbuf_setlen(&path, len);
     +		strbuf_addstr(&path, "/objects");
     +		if (access(path.buf, X_OK)) {
     +			if (err)
    -+				strbuf_addf(
    -+					err,
    -+					_("cannot access object directory '%s'"),
    -+					path.buf
    -+				);
    ++				strbuf_addf(err, _("cannot access object directory '%s'"),
    ++					    path.buf);
     +			goto done;
     +		}
     +	}
    @@ setup.c: static int validate_headref(const char *path)
     +	strbuf_addstr(&path, "/refs");
     +	if (access(path.buf, X_OK)) {
     +		if (err)
    -+			strbuf_addf(
    -+				err,
    -+				_("cannot access refs directory '%s'"),
    -+				path.buf
    -+			);
    ++			strbuf_addf(err, _("cannot access refs directory '%s'"), path.buf);
     +		goto done;
     +	}
     +
3:  2c9c8d64bb ! 4:  27a9f668ab setup: communicate why a directory is not a valid git directory
    @@ setup.c: static void repo_discover_explicit_gitdir(struct repo_discovery *discov
      		}
     -		die(_("not a git repository: '%s'"), gitdirenv);
     +
    -+		strbuf_addf(&die_msg, _("not a git repository: '%s'"), gitdirenv);
    -+		strbuf_addch(&die_msg, '\n');
    -+		strbuf_addf(&die_msg, _("reason: %s"), invalid_gitdir_reason.buf);
    ++		strbuf_addf(&die_msg, _("not a git repository: '%s'\nreason: %s"),
    ++			    gitdirenv, invalid_gitdir_reason.buf);
     +		die("%s", die_msg.buf);
    -+
    -+		strbuf_release(&die_msg);
      	}
      
      	if (read_and_verify_repository_format(&discovery->format, gitdirenv, nongit_ok))
    @@ setup.c: static void repo_discover_explicit_gitdir(struct repo_discovery *discov
     
      ## t/t0009-git-dir-validation.sh ##
     @@ t/t0009-git-dir-validation.sh: test_expect_success 'setup: custom git directory with missing HEAD is rejected'
    + 	test_when_finished "rm -rf parent/empty-dir" &&
      	mkdir -p parent/empty-dir &&
    - 	(
    - 		test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
    --		test_grep "not a git repository" stderr
    -+		test_grep "not a git repository" stderr &&
    -+		test_grep "reason: could not stat HEAD at" stderr
    - 	)
    + 	test_must_fail git --git-dir parent/empty-dir rev-parse --is-bare-repository 2>stderr &&
    +-	test_grep "not a git repository" stderr
    ++	test_grep "not a git repository" stderr &&
    ++	test_grep "reason: could not stat HEAD at" stderr
      '
      
    + test_expect_success 'setup: custom git directory with HEAD as a symlink outside refs/ is rejected' '
     @@ t/t0009-git-dir-validation.sh: test_expect_success 'setup: custom git directory with HEAD as a symlink outside
      		rm real-repo/HEAD &&
      		ln -s ../garbage real-repo/HEAD &&

Kaartic Sivaraam (4):
  setup: normalize an if-else to follow our convention
  t0009: add tests to cover more error reporting scenarios
  setup: introduce new helper 'is_git_directory_verbose'
  setup: communicate why a directory is not a valid git directory

 setup.c                       | 135 +++++++++++++++++++++++-----------
 t/t0009-git-dir-validation.sh |  36 +++++++++
 2 files changed, 127 insertions(+), 44 deletions(-)