[PATCH 0/6] avoid touching ref code in non-repositories

From: Jeff King <peff@peff.net>
Date: Mar 5, 2016, 22:08 UTC
Message-ID: <20160305220829.GA31316@sigill.intra.peff.net>

Here's the series I mentioned earlier, to avoid avoid accessing the ref code if we know we aren't in a repository. For those just joining us, the end goal is to be able to detect and complain when code tries to open a ref without having run check_repository_format() or similar. Once we have pluggable ref backends, doing so would be wrong, since we won't know which backend we're supposed to be using.

It's already _kind of_ wrong, in the sense that we should not be looking for file-backend refs if we know we don't have a repository. But nobody really noticed in practice, because you do not generally have ".git/HEAD" lying around in non-repository directories. So it was a de facto noop, though you could construct pathological cases where it did the wrong thing.

The patches are:

  [1/6]: setup: make startup_info available everywhere
  [2/6]: setup: set startup_info->have_repository more reliably
  [3/6]: remote: don't resolve HEAD in non-repository
  [4/6]: mailmap: do not resolve blobs in a non-repository
  [5/6]: grep: turn off gitlink detection for --no-index
  [6/6]: use setup_git_directory() in test-* programs

There's a sort-of "7/6" I used to find these spots:

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 81f68f8..22c1b6d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -947,6 +947,8 @@ static struct ref_cache *lookup_ref_cache(const char *submodule)
 {
 	struct ref_cache *refs;
 
+	assert(startup_info->have_repository);
+
 	if (!submodule || !*submodule)
 		return &ref_cache;
 
@@ -1414,6 +1416,8 @@ static const char *resolve_ref_1(const char *refname,
 	int depth = MAXDEPTH;
 	int bad_name = 0;
 
+	assert(startup_info->have_repository);
+
 	if (flags)
 		*flags = 0;
 

Running the test suite with that patch and 1/6 turned up the cases I've
fixed here. But I'm not including it here for a few reasons:

  1. I'm not sure that asserting is the best behavior. It's great for
     finding problems (and that's why I used assert() -- to get a
     coredump for the backtrace), but in practice it might be friendlier
     to turn it into a silent noop ("no, I don't even have a ref
     backend, so your ref definitely does not exist").

  2. In a pluggable ref-backend world, this isn't where we'd put the
     asserts, anyway. We'd do it when accessing the ref_backend vtable
     (or just starting with a dummy vtable that asserts, or returns
     "nope, no refs", or whatever).

So it was useful for debugging/analysis now, and we may want something
like it later, but I think that should come on top the ref-backend
series.

-Peff