Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Tiago Botelho <tiago@gitlab.com>]

On Thu, Apr 12, 2018 at 12:53 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Tiago,
>
> On Thu, 12 Apr 2018, Tiago Botelho wrote:
>
>> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren
>> > <haraldnordgren@gmail.com> wrote:
>> >> I think it looks similar. But if I'm reading that thread correctly
>> >> then there was never a patch created, right?
>> >
>> > (It is customary on this mailing list to reply after the sentences we
>> > reply to. We don't "top post".)
>> >
>> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we
>> > have been suggesting "Implement git bisect --first-parent" and there
>> > are the following related links:
>> >
>> > https://public-inbox.org/git/20150304053333.GA9584@peff.net/
>> > https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/
>> >
>> > Tiago in Cc also tried at a recent London hackathon to implement it
>> > and came up with the following:
>> >
>> > https://github.com/tiagonbotelho/git/pull/1/files
>> >
>> > I tried to help him by reworking his commit in the following branch:
>> >
>> > https://github.com/chriscool/git/commits/myfirstparent
>>
>> Thank you for the cc Christian, I’ve been quite busy and was not able
>> to work on the PR for quite some time.
>>
>> I intended to pick it back up again next week. If it is ok with Harald
>> I would love to finish the PR that I started,
>> since it is quite close to being finished (I think it was just specs
>> missing if I am not mistaken).
>
> That looks promising. Just like I suggested to Harald in another reply
> [*1*] on this thread, you probably want to use `int flags` instead, and
> turn `find_all` into BISECT_FIND_ALL in a preparatory commit.
>
> Also, you will definitely want to add a test. Again, my reply to Harald
> [*1*] should give you a head start there... You will want to imitate the
> test case I outlined there, maybe something like:
>
> # A - B - C - F
> #   \   \   /   \
> #     D - E - G - H
>
> [... 'setup' as in my mail to Harald ...]
>
> test_expect_success '--first-parent' '
>         write_script find-e.sh <<-\EOF &&
>         case "$(git show --format=%s -s)" in
>         B|C|F) ;; # first parent lineage: okay
>         *) git show -s --oneline HEAD >unexpected;;
>         esac
>         # detect whether we are "before" or "after" E
>         test ! -f E.t
>         EOF
>
>         git bisect start --first-parent HEAD A &&
>         git bisect run ./find-e.sh >actual &&
>         test_path_is_missing unexpected &&
>         grep "$(git rev-parse F) is the first bad commit" actual
> '
>
> Also, Tiago, reading through your patch (as on chriscool/git; do you have
> your own fork? That would make it much easier to collaborate with you by
> offering PRs), it looks more straight-forward than editing the commit_list
> after the fact and adding magic weights ;-)
>
> Except for one thing. I wonder why `bisect_next_all()` does not set
> revs.first_parent_only after calling `bisect_rev_setup()`? You would still
> need the changes in `count_distance()`, as it performs its own commit
> graph traversal, but there is no need to enumerate too many commits in the
> first place, right?
>
> Harald, maybe --merges-only can be implemented on top of --first-parent,
> with the `int flags` change I suggested?
>
> Ciao,
> Johannes
>
> Footnote *1*:
> https://public-inbox.org/git/nycvar.QRO.7.76.6.1804121143120.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/

Hi Johannes,

Thank you for the pointers!

I do have my own fork, you can see it here:
https://github.com/tiagonbotelho/git/pull/1/commits
which applies the changes Chris made to my first draft of the solution.

I still have to add him as co-author of that commit.

Cheers,
Tiago Botelho