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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 18, 2013, 19:35 UTC
Message-ID: <xmqqa9fyhrzt.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20131217232231.GA14807@sigill.intra.peff.net>

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.

Show 17 quoted lines

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

Show 15 quoted lines

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

Show 17 quoted lines

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