From: Felipe Contreras Date: Sun, 02 Jun 2013 22:38:38 GMT Subject: Re: [PATCH v2 7/7] unpack-trees: free cache_entry array members for merges Message-ID: In-Reply-To: <51ABAA84.8090301@lsrfire.ath.cx> On Sun, Jun 2, 2013 at 3:26 PM, René Scharfe wrote: > Am 02.06.2013 19:59, schrieb Felipe Contreras: > >> On Sun, Jun 2, 2013 at 12:54 PM, René Scharfe >> wrote: >>> >>> Am 02.06.2013 19:25, schrieb Felipe Contreras: >>>> >>>> >>>> On Sun, Jun 2, 2013 at 10:46 AM, René Scharfe >>>> wrote: >>>>> >>>>> >>>>> + for (i = 0; i < n; i++) { >>>>> + struct cache_entry *ce = src[i + o->merge]; >>>>> + if (ce != o->df_conflict_entry) >>>> >>>> >>>> >>>> It's possible that ce is NULL, but you didn't add that check because >>>> free(NULL) still works? Or because ce cannot be NULL? >>>> >>>> If it's the former, I think it's clearer if we check that ce is not >>>> NULL either way. >>> >>> >>> >>> It is NULL if one tree misses an entry (e.g. a new or removed file). free >>> handles NULL and we generally avoid duplicating its NULL-check. >> >> >> Yeah, but I can see somebody adding code inside that 'if' clause to >> print the cache entry, and see a crash only to wonder what's going on. >> And to save what? 5 characters? > > > The person adding code that depends on ce not being NULL needs to add that > check as well. Let's not worry too much about future changes that may or > (more likely IMHO) may not be done. The test suite covers this case > multiple times, so such a mistake doesn't have a realistic chance to hit > master. What do we gain by not doing this? 5 less characters? -- Felipe Contreras