Subject: Re: [PATCH v2] fast-import: implement unpack limit

From: Junio C Hamano <[email-masked]>
Date: Sun, 24 Apr 2016 12:18:08 -0700

Eric Wong <[email-masked]> writes:

> +static int loosen_small_pack(const struct packed_git *p)
> +{
> +	struct child_process unpack = CHILD_PROCESS_INIT;
> +
> +	if (lseek(p->pack_fd, 0, SEEK_SET) < 0)
> +		die_errno("Failed seeking to start of '%s'", p->pack_name);
> +
> +	unpack.in = p->pack_fd;
> +	unpack.git_cmd = 1;
> +	unpack.stdout_to_stderr = 1;
> +	argv_array_push(&unpack.args, "unpack-objects");
> +	argv_array_push(&unpack.args, "-q");
> +
> +	return run_command(&unpack);
> +}

So you have fd open to a *.pack file you have been writing, you are
going to close and discard it after you return from here, so you
just seek the fd to the beginning and hand it to unpack-objects.

And this works if you haven't finalized the *.pack file with the
corresponding *.idx (otherwise unpack-objects would silently ignore
objects found in the *.idx file); and obviously you haven't and you
are not going to create *.idx file for this.

Looks good.  I haven't thought if "-q" is appropriate or not though.

> @@ -972,6 +990,12 @@ static void end_packfile(void)
>  		fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,
>  				    pack_data->pack_name, object_count,
>  				    cur_pack_sha1, pack_size);
> +
> +		if (object_count <= unpack_limit) {
> +			if (loosen_small_pack(pack_data) == 0)
> +				goto discard_pack;
> +		}

"if (!loosen_small_pack(pack_data))" would be more idiomatic, but
the logic is very clear here.  We haven't created the idx, we skip
the part that creates the idx and instead jump directly to the part
that closes and unlinks it.

I like this change.  Thanks.

>  		close(pack_data->pack_fd);
>  		idx_name = keep_pack(create_index());
>  
> @@ -1002,6 +1026,7 @@ static void end_packfile(void)
>  		pack_id++;
>  	}
>  	else {
> +discard_pack:
>  		close(pack_data->pack_fd);
>  		unlink_or_warn(pack_data->pack_name);
>  	}