mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Martin Ågren" <>
To: Yasushi SHOJI <>
Cc: "Æ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:02:34 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 6 January 2018 at 15:27, 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?

All of this matches my understanding.

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

This is a bit tricky. The elements of the linked list are not sorted.
Instead, the items of the linked list are copied into an array and
sorted. The result is then put into a linked list.

Or actually, into the *same* list. That's an optimization to avoid
deallocating all objects in the list, then allocate (roughly) the same
number of objects again. It means all the `next`-pointers are already
set up, and we just need to set the final one to NULL. (It will already
be NULL if and only if the new list has the same length as the old,
i.e., if we didn't skip any TREESAME-commit.)

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

You'll find some QSORT/qsort-stuff in git-compat-util.h. We may or may
not end up doing an actual "quick sort". That would depend on, e.g., how
the C-library implements `qsort()`. Also, I'd guess that even if we have
room for an improvement, those cases (small `cnt`!) are already fast
enough that no-one really cares. That is, maybe we could measure a
speed-up, but would anyone really *notice* it?

>> 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?

There is not much practical use for *having* it. But there might be some
use in *detecting* it. Linus wrote in 670f5fe34f: "Also, add a safety
net: if somebody marks as good something that includes the known-bad
point, we now notice and complain, instead of writing an empty revision
to the new bisection branch."

So there might be some value in verifying that the safety-net works and
tells the user something useful (as opposed to Git segfaulting ;-) ).


  reply	other threads:[~2018-01-06 15:02 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 [this message]
2018-01-06 15:05             ` Christian Couder
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).