Re: [PATCH v6] Implement --first-parent for git rev-list --bisect
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 28, 2018, 18:39 UTC
- Message-ID
- <xmqqy3cqfi8c.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <nycvar.QRO.7.76.6.1808281512240.73@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 23 quoted lines
> Hi Tiago,
>
> On Tue, 28 Aug 2018, Tiago Botelho wrote:
>
>> This will enable users to implement bisecting on first parents
>> which can be useful for when the commits from a feature branch
>> that we want to merge are not always tested.
>
> This message is still lacking the explanation I asked for, namely for the
> lines:
>
> @@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
> if (0 <= weight(p))
> continue;
> for (q = p->item->parents; q; q = q->next) {
> + if ((bisect_flags & BISECT_FIRST_PARENT)) {
> + if (weight(q) < 0)
> + q = NULL;
> + break;
> + }
> if (q->item->object.flags & UNINTERESTING)
> continue;
> if (0 <= weight(q))I've just finished scanning the discussion thread on public-inbox for v5, v4, v3, v2 and the initial round of this series, but found your comments only on the tests. If you have a pointer that would be great; it also is OK to say what kind of explanation is needed for that addition again.
FWIW I too was puzzled about the correctness of the added logic above, especially the part that reads weight(q) before checking if it is not UNINTERESTING, but I covered it on a separate message.
Show 6 quoted lines
> I would have preferred to reuse the already existing commits generated in > the `setup` phase rather than generating yet another batch, and to not > introduce an inconsistent way to present a commit graph (compare your > diagram with the one in > https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90 > i.e. *in the same file*)
As I already said in the previous round, I do agree with these. That is, ...
Show 26 quoted lines
>> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh >> index a66140803..1bc297de5 100755 >> --- a/t/t6002-rev-list-bisect.sh >> +++ b/t/t6002-rev-list-bisect.sh >> @@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' ' >> test_cmp expect.sorted actual.sorted >> ' >> >> +# We generate the following commit graph: >> +# >> +# B ------ C >> +# / \ >> +# A FX >> +# \ / >> +# D - CC - EX >> + >> +test_expect_success 'setup' ' >> + test_commit A && >> + test_commit B && >> + test_commit C && >> + git reset --hard A && >> + test_commit D && >> + test_commit CC && >> + test_commit EX && >> + test_merge FX C >> +'
... the above graph construction should not be necessary. An earlier part of t6002 would have already created a history of suitable shape to use for writing the following tests.
Show 44 quoted lines
>> +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent FX ^A' <<EOF >> +$(git rev-parse CC) >> +EOF >> + >> +test_output_expect_success "--first-parent" 'git rev-list --first-parent FX ^A' <<EOF >> +$(git rev-parse FX) >> +$(git rev-parse EX) >> +$(git rev-parse CC) >> +$(git rev-parse D) >> +EOF >> + >> +test_output_expect_success "--bisect-vars --first-parent" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF >> +bisect_rev='$(git rev-parse CC)' >> +bisect_nr=1 >> +bisect_good=1 >> +bisect_bad=1 >> +bisect_all=4 >> +bisect_steps=1 >> +EOF >> + >> +test_expect_success "--bisect-all --first-parent" ' >> +cat >expect <<EOF && >> +$(git rev-parse CC) (dist=2) >> +$(git rev-parse EX) (dist=1) >> +$(git rev-parse D) (dist=1) >> +$(git rev-parse FX) (dist=0) >> +EOF >> + >> +# Make sure we have the same entries, nothing more, nothing less >> +git rev-list --bisect-all --first-parent FX ^A >actual && >> + sort actual >actual.sorted && >> + sort expect >expect.sorted && >> + test_cmp expect.sorted actual.sorted && >> + # Make sure the entries are sorted in the dist order >> + sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists && >> + sort -r actual.dists >actual.dists.sorted && >> + test_cmp actual.dists.sorted actual.dists >> +' >> + >> test_done >> -- >> 2.16.3 >> >>