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

From: Stefan Beller <stefanbeller@googlemail.com>
Date: Aug 21, 2013, 22:57 UTC
Message-ID: <521545DF.2040004@googlemail.com>
In-Reply-To: <xmqqsiy2slo3.fsf@gitster.dls.corp.google.com>

On 08/22/2013 12:50 AM, Junio C Hamano wrote:

Show 11 quoted lines

> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
>>>> +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.

No need to be sorry! I am sleepy, and may missunderstand even clear messages. I'll change it to static of course.

Show 10 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?

What about one of: get_non_kept_pack_filenames get_prunable_pack_filenames get_remove_candidate_pack_filenames

Show 15 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?
>

I should read the strbuf documentation again. Thanks for pointing it out. I'll remove the strbuf_addstr(&line, "");

Thanks for your patience in the reviews, Stefan