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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2016, 19:18 UTC
Message-ID: <xmqqpoten7db.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160424043223.GA27609@dcvr.yhbt.net>

Eric Wong <normalperson@yhbt.net> writes:

Show 15 quoted lines

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

Show 9 quoted lines

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

Show 11 quoted lines

>  		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);
>  	}