Re: [PATCH v3 2/2] commit-reach: fix memory and flag leaks

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 24, 2018, 17:25 UTC
Message-ID: <c8320650-230a-9a17-df79-9555d5214bd2@gmail.com>
In-Reply-To: <20180921235803.GB3437@sigill.intra.peff.net>

On 9/21/2018 7:58 PM, Jeff King wrote:

Show 19 quoted lines

> On Fri, Sep 21, 2018 at 08:05:27AM -0700, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The can_all_from_reach_with_flag() method uses 'assign_flag' as a
>> value we can use to mark objects temporarily during our commit walk.
>> The intent is that these flags are removed from all objects before
>> returning. However, this is not the case.
>>
>> The 'from' array could also contain objects that are not commits, and
>> we mark those objects with 'assign_flag'. Add a loop to the 'cleanup'
>> section that removes these markers.
>>
>> Also, we forgot to free() the memory for 'list', so add that to the
>> 'cleanup' section.
> Urgh, ignore most of my response to patch 1, then. I saw there was a
> patch 2, but thought it was just handling the free().
>
> The flag-clearing here makes perfect sense.

In my local branch, I ended up squashing this commit into the previous, because I discovered clear_commit_marks_many() making the cleanup section have this diff:

@@ -600,10 +622,12 @@ int can_all_from_reach_with_flag(struct 
object_array *from,
         }

  cleanup:
-       for (i = 0; i < from->nr; i++) {
-               clear_commit_marks(list[i], RESULT);
-               clear_commit_marks(list[i], assign_flag);
-       }
+       clear_commit_marks_many(nr_commits, list, RESULT | assign_flag);
+       free(list);
+
+       for (i = 0; i < from->nr; i++)
+               from->objects[i].item->flags &= ~assign_flag;
+
         return result;
  }

With the bigger change in the larger set, there is less reason to do 
separate commits. (Plus, it confused you during review.)

Thanks,

-Stolee