From: Jeff King Date: Wed, 18 Dec 2013 20:00:43 GMT Subject: Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer Message-ID: <20131218200043.GA10244@sigill.intra.peff.net> In-Reply-To: On Wed, Dec 18, 2013 at 11:35:34AM -0800, Junio C Hamano wrote: > This function is called to remove > > * Any tmp_* found directly in .git/objects/ > * Any tmp_* found directly in .git/objects/pack/ > * Any tmp_obj_* found directly in .git/objects/??/ > > and shares the same expiration logic with prune_object(). The only > difference from the other function is what the file is called in > dry-run or verbose report ("stale temporary file" vs " "). > > We may want to rename it to prune_tmp_file(); its usage may have > been limited to an unborn loose object file at some point in the > history, but it does not look that way in today's code. Yes, certainly the rename makes sense (and I think the history is as you mentioned). I noticed the similarity between the two, as well. It might be simple to refactor into a single function. > > - prune_object(path, de->d_name, sha1); > > + strbuf_addf(path, "/%s", de->d_name); > > + prune_object(path->buf, sha1); > > + path->len = baselen; > > This is minor, but I prefer using strbuf_setlen() for this. Good catch. I do not think it is minor at all; my version is buggy. After the loop ends, path->len does not match the NUL in path->buf. That is OK if the next caller is strbuf-aware, but if it were to pass path->buf straight to a system call, that would be rather...confusing. -Peff