public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: Junio C Hamano <gitster@pobox•com>
To: Eric Wong <normalperson@yhbt•net>
Cc: Jeff King <peff@peff•net>,
	git@vger•kernel.org, Jonathan Nieder <jrnieder@gmail•com>,
	Shawn Pearce <spearce@spearce•org>
Subject: Re: [PATCH v2] fast-import: implement unpack limit
Date: Sun, 24 Apr 2016 12:18:08 -0700	[thread overview]
Message-ID: <xmqqpoten7db.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160424043223.GA27609@dcvr.yhbt.net> (Eric Wong's message of "Sun, 24 Apr 2016 04:32:23 +0000")

Eric Wong <normalperson@yhbt•net> 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);
>  	}

next prev parent reply	other threads:[~2016-04-24 19:18 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-23  2:42 [PATCH] fast-import: implement --min-pack-size parameter Eric Wong
2016-04-23  3:13 ` Jeff King
2016-04-24  4:32   ` [PATCH v2] fast-import: implement unpack limit Eric Wong
2016-04-24 19:18     ` Junio C Hamano [this message]
2016-04-24 20:36       ` Eric Wong
2016-04-25 21:17         ` [PATCH v3] " Eric Wong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqpoten7db.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=jrnieder@gmail$(echo .)com \
    --cc=normalperson@yhbt$(echo .)net \
    --cc=peff@peff$(echo .)net \
    --cc=spearce@spearce$(echo .)org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox