Re: [External] Re: [PATCH v1] negotiator/default.c: avoid stack overflow

From: Derrick Stolee <derrickstolee@github.com>
Date: Apr 25, 2023, 13:34 UTC
Message-ID: <4ddb4030-0789-5ed4-448d-96f6afb48334@github.com>
In-Reply-To: <CAKgqsWUEnbmhLL3p9+_P4yH_=A+hz+bBPqmfb8FyRUeW-u7_gw@mail.gmail.com>

On 4/24/2023 11:02 PM, Han Xin wrote:

> On Mon, Apr 24, 2023 at 10:44 PM Derrick Stolee
> <derrickstolee@github.com> wrote:

Show 36 quoted lines

>>> @@ -70,15 +76,17 @@ static void mark_common(struct negotiation_state *ns, struct commit *commit,
>>>                               ns->non_common_revs--;
>>>                       if (!o->parsed && !dont_parse)
>>>                               if (repo_parse_commit(the_repository, commit))
>>> -                                     return;
>>> +                                     continue;
>>>
>>> +                     ancestors_only = 0;
>>
>> This caught me off guard, but this flag essentially says "should
>> I mark the first commit as common or not?". It would probably be
>> clearer if this was done before the loop, and then was ignored
>> within the loop, setting the flag on each parent in this loop:
>>
>>>                       for (parents = commit->parents;
>>>                                       parents;
>>>                                       parents = parents->next)
>>> -                             mark_common(ns, parents->item, 0,
>>> -                                         dont_parse);
>>> +                             prio_queue_put(&queue, parents->item);
>>
> 
> I'll think about how to optimize this again.
> 
> ancestors_only is used multiple times in the original logic:
> 1.
>               if (!ancestors_only)
>                      o->flags |= COMMON;
> 2.
>              if (!(o->flags & SEEN))
>                      rev_list_push(ns, commit, SEEN);
>              else {
>                      struct commit_list *parents;
> 
>                      if (!ancestors_only && !(o->flags & POPPED))
>                              ns->non_common_revs--;

Good point. Thanks for checking.

Show 9 quoted lines

> Should we use this ?
> 
>              if (!ancestors_only) {
>                     commit->object.flags |= COMMON;
> 
>                     if ((commit->object.flags & SEEN) &&
> !(commit->object.flags & POPPED))
>                              ns->non_common_revs--;
>              }

This seems correct, although your email seems to have done a strange line wrap that I'm sure you'll fix in the actual patch.

Show 9 quoted lines

> and
> 
>                    for (parents = commit->parents;
>                              parents;
>                              parents = parents->next) {
>                              if (parents->item->object.flags & COMMON)
>                                       continue;
> 
>                             parents->item->object.flags |= COMMON;

Thanks, this part avoids duplicate additions to the queue.

>                             if ((parents->item->object.flags & SEEN)
>                                      && !(parents->item->object.flags & POPPED))
>                                       ns->non_common_revs--;

And this matches the non_common_revs part.

If you want this code to be a little cleaner, you could add

	struct commit *p = parents->item;

at the start of the loop and then s/parents->item/p/ for the rest of the uses in the loop.

>                             prio_queue_put(&queue, parents->item);
>                    }

>> In addition, the mark_common() method there seems to have a few
>> problems:

...

>> Consider fixing these issues while you are here.
>>
> 
> Make sense.

I'm looking forward to your v2!

Thanks, -Stolee