Re: [PATCH 1/1] commit-reach: properly peel tags
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 12, 2018, 21:23 UTC
- Message-ID
- <xmqqpnxijtpd.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <948e222228d2f2868b85a425142382e63a17773a.1536762173.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 24 quoted lines
> 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.
Show 9 quoted lines
> + 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?
Show 17 quoted lines
> - 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);
> }