"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> diff --git a/commit-reach.c b/commit-reach.c
> index 86715c103c..6de72c6e03 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -544,20 +544,31 @@ int can_all_from_reach_with_flag(struct object_array *from,
> {
> struct commit **list = NULL;
> int i;
> + int nr_commits;
> int result = 1;
>
> ALLOC_ARRAY(list, from->nr);
> + nr_commits = 0;
> for (i = 0; i < from->nr; i++) {
> - list[i] = (struct commit *)from->objects[i].item;
> + struct object *from_one = from->objects[i].item;
>
> - if (parse_commit(list[i]) ||
> - list[i]->generation < min_generation)
> - return 0;
> + from_one = deref_tag(the_repository, from_one,
> + "a from object", 0);
> + if (!from_one || from_one->type != OBJ_COMMIT) {
> + from->objects[i].item->flags |= assign_flag;
I wondered why this is not futzing with "from_one->flags"; by going
back to the original from->objects[] array, the code is setting the
flags on the original tag object and not the non-commit object that
was pointed at by the tag.
> + continue;
> + }
> +
> + list[nr_commits] = (struct commit *)from_one;
> + if (parse_commit(list[nr_commits]) ||
> + list[nr_commits]->generation < min_generation)
> + return 0; /* is this a leak? */
> + nr_commits++;
> }
In the original code, the flags bits were left unchanged if the loop
terminated by hitting a commit whose generation is too young (or
parse_commit() returns non-zero). With this updated code, flags bit
can be modified before the code notices the situation and leave the
function, bypassing the "cleanup" we see below that clears the
"assign_flag" bits.
Would it be a problem that we return early without cleaning up?
Even if we do not call this early return, the assign_flag bits added
to the original tag in from->objects[i].item won't be cleaned in
this new code, as "cleanup:" section now loops over the list[] that
omits the object whose flags was smudged above before the "continue".
Would it be a problem that we leave the assign_flags without
cleaning up?
> - QSORT(list, from->nr, compare_commits_by_gen);
> + QSORT(list, nr_commits, compare_commits_by_gen);
>
> - for (i = 0; i < from->nr; i++) {
> + for (i = 0; i < nr_commits; i++) {
> /* DFS from list[i] */
> struct commit_list *stack = NULL;
>
> @@ -600,7 +611,7 @@ int can_all_from_reach_with_flag(struct object_array *from,
> }
>
> cleanup:
> - for (i = 0; i < from->nr; i++) {
> + for (i = 0; i < nr_commits; i++) {
> clear_commit_marks(list[i], RESULT);
> clear_commit_marks(list[i], assign_flag);
> }
