From: Felipe Contreras Date: Thu, 30 May 2013 15:20:48 GMT Subject: Re: [PATCH 7/7] unpack-trees: free cache_entry array members for merges Message-ID: In-Reply-To: <51A764CE.4000708@lsrfire.ath.cx> On Thu, May 30, 2013 at 9:40 AM, René Scharfe wrote: > Am 30.05.2013 14:04, schrieb Felipe Contreras: > >> On Thu, May 30, 2013 at 6:34 AM, René Scharfe >> wrote: >>> >>> The merge functions duplicate entries as needed and they don't free >>> them. Release them in unpack_nondirectories, the same function >>> where they were allocated, after we're done. >> >> >> Ah, you beat me to this change, but.. >> >>> @@ -600,9 +600,14 @@ static int unpack_nondirectories(int n, unsigned >>> long mask, >>> src[i + o->merge] = create_ce_entry(info, names + i, >>> stage); >>> } >>> >>> - if (o->merge) >>> - return call_unpack_fn((const struct cache_entry * const >>> *)src, >>> - o); >>> + if (o->merge) { >>> + int rc = call_unpack_fn((const struct cache_entry * const >>> *)src, >>> + o); >>> + for (i = 1; i <= n; i++) >>> + if (src[i] && src[i] != o->df_conflict_entry) >>> + free(src[i]); >> >> >> Doesn't it make more sense to follow the code above and do src[i + >> o->merge]? > > > Not sure I understand. Is the goal to avoid confusion for code readers by > using the same indexing method for allocation and release? Or are you > worried about o->merge having a different value than 1 in that loop? Both. In particular I'm eyeballing the code you can even see in this patch: src[i + o->merge] = create_ce_entry(info, names + i, stage); If you think it's better to use src[i], then I think the code above should do the same. -- Felipe Contreras