From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 13 Sep 2018 21:06:47 GMT
Subject: Re: [PATCH v2 1/1] commit-reach: properly peel tags
Message-ID: <xmqqy3c5ds48.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <8d6061de-1654-577c-40c6-211dbd03aa36@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

>> +		if (!from_one || from_one->type != OBJ_COMMIT) {
>> +			/* no way to tell if this is reachable by
>> +			 * looking at the ancestry chain alone, so
>> +			 * leave a note to ourselves not to worry about
>> +			 * this object anymore.
>> +			 */
>> +			from->objects[i].item->flags |= assign_flag;
>> +			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? */
>
> Of course, after sending v2, I see this comment. This is a leak of
> 'list' and should be fixed.
>
> Not only is it a leak here, it is also a leak in the 'cleanup'
> section. I'll squash the following into v3, but I'll let v2 simmer for
> review before rerolling.
>
> diff --git a/commit-reach.c b/commit-reach.c
> index 4048a2132a..c457d8d85f 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -569,8 +569,11 @@ int can_all_from_reach_with_flag(struct
> object_array *from,
>
>                 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? */
> +                   list[nr_commits]->generation < min_generation) {
> +                       result = 0;
> +                       goto cleanup;
> +               }
> +
>                 nr_commits++;
>         }
>
> @@ -623,6 +626,7 @@ int can_all_from_reach_with_flag(struct
> object_array *from,
>                 clear_commit_marks(list[i], RESULT);
>                 clear_commit_marks(list[i], assign_flag);
>         }
> +       free(list);
>         return result;
>  }

With this, commit marks are cleared even when we do the "early
return", but only for the objects that appear in the resulting
list[].  Because the for() loop in the last hunk interates over
list[], those non-commit objects that are smudged with assign_flag
but never made to list[] will be left smudged when this function
returns (this is true when there is no early return).  

Is that intended?