Re: [PATCH v2] bisect--helper: use BISECT_TERMS in 'bisect skip' command

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2021, 00:56 UTC
Message-ID: <xmqqo8dwegk0.fsf@gitster.g>
In-Reply-To: <fa098676-a56c-cbb9-28cb-e57d4a8a5d77@ramsayjones.plus.com>

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

Show 8 quoted lines

> This v2 patch was also created directly on top of commit e4c7b33747 and,
> once again, applied to current 'master' (@311531c9de). (I was a little
> surprised that it applied clean, given the changes to t6030-*.sh in the
> meantime! ;).
>
> I didn't bother with the range-diff[*], since it wasn't very illuminating
> (just the addition of a few trailing lines in the commit message and the
> addition of the test).

Thanks. Let's hear from Bagas that this version looks good and merge it down.

> I don't know if I did the 'Co-authorship' bit correctly; I nearly used an
> 'Co-authored-by:' rather than an 'Signed-off-by:' for Bagas, but I seem to
> remember that caused some push-back recently. Anyway, if I messed that up,
> please just modify however you see fit.

I am not in principle opposed to the idea of co-authored-by; the case you remember was where we would have used Helped-by (i.e. a reviewer offers "writing it this way is cleaner" suggestions on the list and then gets credited on the next version), and it wasn't clear to me if the reviewer consented to be a co-author of that change. If the party who were named as a co-author responded that it is OK, I would have taken it.

Show 41 quoted lines

> [*] Once I got it to work, that is; for some reason, I was trying smaller
> creation-factor values rather than larger! ;-)
>
>  builtin/bisect--helper.c    |  1 +
>  t/t6030-bisect-porcelain.sh | 11 +++++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 7ad9b4d55b..49c07f0710 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -1129,6 +1129,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		break;
>  	case BISECT_SKIP:
>  		set_terms(&terms, "bad", "good");
> +		get_terms(&terms);
>  		res = bisect_skip(&terms, argv, argc);
>  		break;
>  	default:
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index aa226381be..b707296ae2 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -919,6 +919,17 @@ test_expect_success 'bisect start takes options and revs in any order' '
>  	test_cmp expected actual
>  '
>  
> +# Bisect is started with --term-new and --term-old arguments,
> +# then skip. The HEAD should be changed.
> +test_expect_success 'bisect skip works with --term*' '
> +	git bisect reset &&
> +	git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&
> +	hash_skipped_from=$(git rev-parse --verify HEAD) &&
> +	git bisect skip &&
> +	hash_skipped_to=$(git rev-parse --verify HEAD) &&
> +	test "$hash_skipped_from" != "$hash_skipped_to"
> +'
> +
>  test_expect_success 'git bisect reset cleans bisection state properly' '
>  	git bisect reset &&
>  	git bisect start &&