mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Christian Couder <>
To: Yasushi SHOJI <>
Cc: "Martin Ågren" <>,
	"Ævar Arnfjörð Bjarmason" <>,
	"Git Mailing List" <>,
	"Christian Couder" <>,
	"Junio C Hamano" <>
Subject: Re: [BUG] v2.16.0-rc0 seg faults when git bisect skip
Date: Sat, 6 Jan 2018 16:05:37 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

Hi Yasushi,

On Sat, Jan 6, 2018 at 3:27 PM, Yasushi SHOJI <> wrote:

> best_bisection_sorted() seems to do
>  - get the commit list along with the number of elements in the list
>  - walk the list one by one to check whether a element have TREESAME or not
>  - if TREESAME, skip
>  - if not, add it to array
>  - sort the array by distance
>  - put elements back to the list


> so, if you find TREESAME, you get less elements than given, right?


> Also, if you sort, the last commit, which has NULL in the ->next,
> might get in the middle of the array??

Well, first the array is just necessary to sort the items pointed to
by the list. It is freed at the end of the function. We are mostly
interested in getting a sorted list from this function, not a sorted

Also the array contains only items (commits) and distances, not list
elements, so the elements in the array don't have a ->next pointer.

About items in the list, when we put them back into the list we only
change p->item, so p->next still points to the next one. Only for the
last one we set p->next to NULL (if p is not NULL). So we don't
actually sort the list elements, we sort the items pointed to by the

> # BTW, is it really fast to use QSORT even if you have to convert to
> # an array from list?

It is easier and probably faster to just use qsort, which we get from
#include <stdlib.h>, as it is hopefully optimized, rather than
reimplementing our own list sorting.

>>>> Since nobody noticed it since 7c117184d7, it must be a rare case, right?
>> Right, you marked a commit both good and bad. That's probably not very
>> common. But it obviously happens. :-)

Yeah, mistakes unfortunately happens :-)

>> Thank you for providing a script for reproducing this. It helped me come
>> up with the attached patch. The patch is based on ma/bisect-leakfix,
>> which includes Ævar's patch.
>> I think this patch could be useful, either as a final "let's test
>> something previously non-tested; this would have caught the segfault",
>> or simply squashed into Ævar's patch as a "let's add a test that would
>> have caught this, and which also tests a previously non-tested code
>> path."
> Do we really need that?  What is a practical use of a commit having
> both good and bad?

It is not practical, but it is a user mistake that happens.

Thanks for your reports and test cases,

  parent reply	other threads:[~2018-01-06 15:05 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-03 12:36 [BUG] v2.16.0-rc0 seg faults when git bisect skip Yasushi SHOJI
2018-01-03 14:21 ` Ævar Arnfjörð Bjarmason
2018-01-03 18:26   ` Martin Ågren
2018-01-03 18:48     ` [PATCH] bisect: fix a regression causing a segfault Ævar Arnfjörð Bjarmason
2018-01-05  2:45     ` [BUG] v2.16.0-rc0 seg faults when git bisect skip Yasushi SHOJI
2018-01-05  5:28       ` Yasushi SHOJI
2018-01-06  8:21         ` Martin Ågren
2018-01-06 14:27           ` Yasushi SHOJI
2018-01-06 15:02             ` Martin Ågren
2018-01-06 15:05             ` Christian Couder [this message]
2018-01-08 14:45               ` Yasushi SHOJI
2018-01-08 15:09                 ` Christian Couder
2018-01-09 11:09                   ` Yasushi SHOJI
2018-01-05 22:20       ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).