From: Jeff King <peff@peff.net>
Date: Mon, 24 Sep 2018 19:06:26 GMT
Subject: Re: [PATCH v3 2/2] commit-reach: fix memory and flag leaks
Message-ID: <20180924190625.GA27029@sigill.intra.peff.net>
In-Reply-To: <c8320650-230a-9a17-df79-9555d5214bd2@gmail.com>

On Mon, Sep 24, 2018 at 01:25:12PM -0400, Derrick Stolee wrote:

> On 9/21/2018 7:58 PM, Jeff King wrote:
> > 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.)

Yeah, that looks better still. Thanks!

-Peff