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

From: Jeff King <peff@peff.net>
Date: Oct 8, 2014, 07:33 UTC
Message-ID: <20141008073315.GB25250@peff.net>
In-Reply-To: <5433F3B8.2080505@alum.mit.edu>

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

Show 20 quoted lines

> On 10/03/2014 10:29 PM, Jeff King wrote:
> > 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.

Show 6 quoted lines

> > +	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).

Show 7 quoted lines

> > +		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).

Show 10 quoted lines

> > +			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.

Show 5 quoted lines

> > +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