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

From: Eric Wong <normalperson@yhbt.net>
Date: Apr 24, 2016, 20:36 UTC
Message-ID: <20160424203650.GA32458@dcvr.yhbt.net>
In-Reply-To: <xmqqpoten7db.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

> Eric Wong <normalperson@yhbt.net> writes:
> > +	argv_array_push(&unpack.args, "unpack-objects");
> > +	argv_array_push(&unpack.args, "-q");
> > +
> > +	return run_command(&unpack);

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

Oops, I think tying it to the existing --quiet option in PATCH v3 would be good.

Show 14 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 was on the fence about "!" vs "== 0" vs something else, too; and I get thrown off by things like "!strcmp" in C all the time. I can change it to "if (!loosen_small_pack(pack_data))" in v3 (probably in a day or so in case there's further comments)