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

Commit Message

Derrick Stolee via GitGitGadget Sept. 21, 2018, 3:05 p.m. UTC

From: Derrick Stolee <[e-mail]>

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.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 commit-reach.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Jeff King Sept. 21, 2018, 11:58 p.m. UTC | #1

On Fri, Sep 21, 2018 at 08:05:27AM -0700, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <[e-mail]>
> 
> 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.

-Peff

Derrick Stolee Sept. 24, 2018, 5:25 p.m. UTC | #2

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 <[e-mail]>
>>
>> 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

Jeff King Sept. 24, 2018, 7:06 p.m. UTC | #3

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 <[e-mail]>
> > > 
> > > 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

Patch

diff --git a/commit-reach.c b/commit-reach.c
index e748414d04..5a845440a9 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -626,6 +626,11 @@  cleanup:
 		clear_commit_marks(list[i], RESULT);
 		clear_commit_marks(list[i], assign_flag);
 	}
+	free(list);
+
+	for (i = 0; i < from->nr; i++)
+		from->objects[i].item->flags &= ~assign_flag;
+
 	return result;
 }