Re: [PATCH 2/3] prune_object_dir(): verify that path fits in the temporary buffer

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> 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...

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

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