Re: [RFC PATCHv6 1/2] repack: rewrite the shell script in C

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2013, 22:50 UTC
Message-ID: <xmqqsiy2slo3.fsf@gitster.dls.corp.google.com>
In-Reply-To: <52153C01.6040101@googlemail.com>

Stefan Beller <stefanbeller@googlemail.com> writes:

Show 6 quoted lines

>>> +static int delta_base_offset = 1;
>>> +char *packdir;
>> 
>> Does this have to be global?
>
> We could pass it to all the functions, making it not global.

Sorry for being unclear; I meant "not static". It is perfectly fine for this to be a file-scope static.

Show 7 quoted lines

>>> +
>>> +		if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
>>> +			string_list_append_nodup(fname_list, fname);
>> 
>> mental note: this is getting names of non-kept packs, not all packs.
>
> I should document that. ;)

Rather, consider giving the function a better name, perhaps?

Show 9 quoted lines

>>> +	while (strbuf_getline(&line, out, '\n') != EOF) {
>>> +		if (line.len != 40)
>>> +			die("repack: Expecting 40 character sha1 lines only from pack-objects.");
>>> +		strbuf_addstr(&line, "");
>> 
>> What is this addstr() about?
>
> According to the documentation of strbufs, we cannot assume to have sane 
> strings, but anything.

Sorry, I do not get this. What is a sane string and what is an insane string? sb->buf[sb-len] is always terminated with a NUL when strbuf_getline() returns success, isn't it?