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@xxxxxxxxxxxxx>
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
