On Mon, Sep 24, 2018 at 01:25:12PM -0400, Derrick Stolee wrote:
Show 45 quoted lines
> 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.)Jeff King <peff@peff.net>