Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack

From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 04/12] packfile: check midx coverage with .idx rather than .pack
Date: Fri, 5 Apr 2019 09:30:44 -0400	[thread overview]
Message-ID: <20190405133044.GB1460@sigill.intra.peff.net> (raw)
In-Reply-To: <20190405132125.GA1460@sigill.intra.peff.net>

On Fri, Apr 05, 2019 at 09:21:25AM -0400, Jeff King wrote:

> > ... which also means this open_pack_index() call isn't done anymore if
> > there's no .midx file at all.  You don't mention this change in the
> > commit message; is it intended?
> 
> Doh, thank you for catching that. I made that switch at the last minute
> because I didn't want to pay the malloc/free cost when we had no list to
> compare to. I'm surprised it works at all. :-/
> 
> I guess it doesn't, from the other message in the thread.

Hmm, no, that's a different problem. I think we'd generally already have
the pack open unless there _is_ a midx, so this conditional probably
never kicks in unless we have a midx anyway. Meaning my malloc-avoidance
was over-thought. So we could just get rid of it. Or do it like this:

diff --git a/packfile.c b/packfile.c
index e7ca135ed5..ef0f959311 100644
--- a/packfile.c
+++ b/packfile.c
@@ -496,17 +496,22 @@ static int open_packed_git_1(struct packed_git *p)
 	ssize_t read_result;
 	const unsigned hashsz = the_hash_algo->rawsz;
 
-	if (!p->index_data && the_repository->objects->multi_pack_index) {
-		struct multi_pack_index *m;
-		char *idx_name = pack_name_to_idx(pack_basename(p));
+	if (!p->index_data) {
+		struct multi_pack_index *m =
+			the_repository->objects->multi_pack_index;
 
-		for (m = the_repository->objects->multi_pack_index;
-		     m; m = m->next) {
-			if (midx_contains_pack(m, idx_name))
-				break;
+		if (m) {
+			char *idx_name = pack_name_to_idx(pack_basename(p));
+
+			for (; m; m = m->next) {
+				if (midx_contains_pack(m, idx_name))
+					break;
+			}
+			free(idx_name);
 		}
-		free(idx_name);
 
+		if (!m)
+			warning("opening anyway");
 		if (!m && open_pack_index(p))
 			return error("packfile %s index unavailable", p->pack_name);
 	}

I'll have to re-roll to address the other problem, though, so I'll give
it some thought.

-Peff
