Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:29
Jeff King [off-list ref] writes:
Converting it to use strbuf looks like it will actually let us drop a bunch of copying, too, as we just end up in mkpath at the very lowest level. I.e., something like below.
Thanks; I may have a few minor comments, but overall, I like the placement of mkpath() in the resulting callchain a lot better than the original.
As an aside, I have noticed us using this "push/pop" approach to treating a strbuf as a stack of paths elsewhere, too. I.e: size_t baselen = base->len; strbuf_addf(base, "/%s", some_thing); some_call(base); base->len = baselen; I wondered if there was any kind of helper we could add to make it look nicer. But I don't think so; the hairy part is that you must remember to reset base->len after the call, and there is no easy way around that in C. If you had object destructors that ran as the stack unwound, or dynamic scoping, it would be easy to manipulate the object. Wrapping won't work because strbuf isn't just a length wrapping an immutable buffer; it actually has to move the NUL in the buffer. Anyway, not important, but perhaps somebody is more clever than I am.
Hmph... interesting but we would need a lot more thought than the time necessary to respond to one piece of e-mail for this ;-) Perhaps later...
quoted hunk
diff --git a/builtin/prune.c b/builtin/prune.c index 6366917..4ca8ec1 100644 --- a/builtin/prune.c +++ b/builtin/prune.c@@ -17,9 +17,8 @@ static int verbose; static unsigned long expire; static int show_progress = -1; -static int prune_tmp_object(const char *path, const char *filename) +static int prune_tmp_object(const char *fullpath) { - const char *fullpath = mkpath("%s/%s", path, filename); struct stat st; if (lstat(fullpath, &st)) return error("Could not stat '%s'", fullpath);
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 "<sha-1> <typename>").
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.
quoted hunk
-static int prune_dir(int i, char *path) +static int prune_dir(int i, struct strbuf *path) { - DIR *dir = opendir(path); + size_t baselen = path->len; + DIR *dir = opendir(path->buf); struct dirent *de; if (!dir)@@ -77,28 +76,39 @@ static int prune_dir(int i, char *path) if (lookup_object(sha1)) continue; - 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. Thanks.