From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 07 Apr 2026 02:01:18 GMT
Subject: Re: [GSoC PATCH v3 2/5] pack-write: add helper to fill promisor file after repack
Message-ID: <xmqqy0iz7clt.fsf@gitster.g>
In-Reply-To: <adP-MYYSmElK9wL3@lorenzo-VM>

Lorenzo Pegorari <lorenzo.pegorari2002@gmail.com> writes:

> On Tue, Apr 07, 2026 at 01:22:16AM +0800, Tian Yuchen wrote:
>> Hi,
>> 
>> On 4/6/26 08:24, LorenzoPegorari wrote:
>> 
>> > +		while (strbuf_getline(&line, source) != EOF) {
>> > +			struct strbuf **parts;
>> > +			struct object_id oid;
>> > +
>> > +			/* Split line into <oid>, <ref> and <time> (if <time> exists) */
>> > +			parts = strbuf_split_max(&line, ' ', 3);
>> > +
>> > +			/* Ignore the lines where <oid> doesn't appear in the dest_pack */
>> > +			strbuf_rtrim(parts[0]);
>> > +			get_oid_hex_algop(parts[0]->buf, &oid, repo->hash_algo);
>> > +			if (!find_pack_entry_one(&oid, dest_pack))
>> > +				continue;
>> 
>> Memory leak here;
>
> Yep, `strbuf_list_free(parts)` is missing here. Ack.
>
>> > +
>> > +			/* If <time> doesn't exist, retrieve it and add it to line */
>> > +			if (!parts[2]) {
>> > +				struct tm tm;
>> > +				localtime_r(&source_stat.st_mtim.tv_sec, &tm),
>> 
>> Typo.
>
> Ack.

Not just an unintended use of comma operator, this is not portable
and breaks OSX build

  https://github.com/git/git/actions/runs/24058681172/job/70170218891#step:4:213

>> > +				strbuf_addch(&line, ' ');
>> > +				strbuf_addftime(&line, "%Y/%m/%d-%H:%M:%S", &tm, 0, 0);

I suspect that storing seconds since epoch as a large integer would
be simpler and much less error prone than storing localtime in
textual form without even recording the timezone.