Re: [PATCH 09/16] prune: factor out loose-object directory traversal

On Tue, Oct 07, 2014 at 04:07:52PM +0200, Michael Haggerty wrote:

On 10/03/2014 10:29 PM, Jeff King wrote:

quoted

Prune has to walk $GIT_DIR/objects/?? in order to find the
set of loose objects to prune. Other parts of the code
(e.g., count-objects) want to do the same. Let's factor it
out into a reusable for_each-style function.

Note that this is not quite a straight code movement. There
are two differences:

  1. The original code iterated from 0 to 256, trying to
     opendir("$GIT_DIR/%02x"). The new code just does a
     readdir() on the object directory, and descends into
     any matching directories. This is faster on
     already-pruned repositories, and should not ever be
     slower (nobody ever creates other files in the object
     directory).

This would change the order that the objects are processed. I doubt that
matters to anybody, but it's probably worth mentioning in the commit
message.

Yeah, I don't think it matters, but I'll mention it.

quoted

+	if (!dir)
+		return opendir_error(path->buf);

OK, so if there is a non-directory named $GIT_DIR/objects/33, then we
emit an "unable to open" error rather than treating it as cruft. I think
this is reasonable.

Correct. The original "prune" silently ignored this case, but I think
it makes sense to complain about oddities. We must treat ENOENT
as a noop, even though the value comes from readdir() and therefore
should exist. A simultaneous prune might have deleted it (hopefully
nobody is insane enough to run two prunes at once, but ignoring
directories that went away seems like the only sane behavior to me).

quoted

+		if (cruft_cb) {
+			r = cruft_cb(de->d_name, path->buf, data);

So, files *and* directories at the $GIT_DIR/objects/XX/ level are
reported as cruft (as opposed to, say, descending into the directories
and reporting any files found deeper in the hierarchy). This seems fine,
too.

Yes, this matches the original prune behavior (and anyway, this is the
object database; anything that is not a loose object is cruft).

quoted

+			if (r)
+				break;
+		}
+	}
+	if (!r && subdir_cb)
+		r = subdir_cb(de->d_name, path->buf, data);

By my reading, path->buf still contains the name of the last file in the
directory at this point. I assume you want to pass it the original
"baselen"-length path here.

Ack, good catch. This was originally in the outer for_each_loose_file
loop, but I thought it was more clear to handle it in the subdir
function. Not only is path->buf wrong here, but de->d_name is totally
bogus here.

Will fix in the re-roll.

quoted

+int for_each_loose_file_in_objdir(const char *path,

[...]
So other files or directories at the $GIT_DIR/objects/ level are just
ignored; they are not considered cruft. This is worth clarifying in the
docstring.

I tried to clarify that by indicating that we iterated only over the
"loose-object parts of the object directory". I guess that needs a more
clear definition.

-Peff