Re: [PATCH v10 7/7] bisect: allow any terms set by user

[Matthieu Moy]

Christian Couder  writes:

> On Fri, Jun 26, 2015 at 6:58 PM, Matthieu Moy  wrote:
>> From: Antoine Delaite 
>>
>> Introduction of the git bisect terms command. The user can set his own
>> terms. It will work exactly like before. The terms must be set before the
>> start.
>
> After looking a bit at the code, I think that for now existing
> predefined terms ("bad", "good", "new" and "old") as well as some
> other terms that look like bisect subcommands like "skip", "start" and
> "terms" should be disallowed as arguments to "git bisect terms",

More importantly, subcommands should be disallowed, or the user may
completely break bisect (e.g. running 'git bisect terms reset bar'
prevents you from running 'git bisect reset' later).

And they should be different, or some more funny situation will occur.

I've just squashed this into my last patch:

diff --git a/git-bisect.sh b/git-bisect.sh
index cf07a91..f6be218 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -607,9 +607,21 @@ bisect_terms () {
gettextln "Your current terms are $NAME_GOOD for the 
old state
 and $NAME_BAD for the new state."
else
-   die "$(gettext "No terms defined.")"
+   die "$(gettext "no terms defined")"
fi ;;
2)
+   for term in "$@"
+   do
+   case "$term" in
+   
help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run)
+   die "$(eval_gettext "can't use the builtin 
command '\$term' as a term")"
+   ;;
+   esac
+   done
+   if test "$1" = "$2"
+   then
+   die "$(gettext "please use two different terms")"
+   fi
if ! test -s "$GIT_DIR/BISECT_START"
then
write_terms "$1" "$2"
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index eb8cc80..5a7243b 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -852,7 +852,7 @@ test_expect_success 'bisect terms needs 0 or 2 arguments' '
test_must_fail git bisect terms only-one &&
test_must_fail git bisect terms 1 2 3 &&
test_must_fail git bisect terms 2>actual &&
-   echo "No terms defined." >expected &&
+   echo "no terms defined" >expected &&
test_cmp expected actual
 '
 
Updated my GitHub branch, but I'll stop spamming the list with git
send-email for a while ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 7/7] bisect: allow any terms set by user

[Christian Couder]

On Fri, Jun 26, 2015 at 6:58 PM, Matthieu Moy  wrote:
> From: Antoine Delaite 
>
> Introduction of the git bisect terms command. The user can set his own
> terms. It will work exactly like before. The terms must be set before the
> start.

After looking a bit at the code, I think that for now existing
predefined terms ("bad", "good", "new" and "old") as well as some
other terms that look like bisect subcommands like "skip", "start" and
"terms" should be disallowed as arguments to "git bisect terms", and
this should be stated in the commit message and in the documentation
as well as checked and tested.

For example a user might want to search for a fix by using "git bisect
terms good bad" (which should swap "good" and "bad"), but we should
not at least for now allow that.

> @@ -185,7 +197,12 @@ bisect_start() {
> eval "$eval true" &&
> if test $must_write_terms -eq 1
> then
> -   write_terms "$NAME_BAD" "$NAME_GOOD"
> +   write_terms "$NAME_BAD" "$NAME_GOOD" &&
> +   if test $must_log_terms -eq 1
> +   then
> +   echo "git bisect terms $NAME_BAD $NAME_GOOD" \
> +   >>"$GIT_DIR/BISECT_LOG"
> +   fi

Maybe you could move appending to the log into write_terms() though
you might need to pass an additional argument to enable or disable
logging.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 7/7] bisect: allow any terms set by user

[Junio C Hamano]

Matthieu Moy  writes:

> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index 24171a5..b1ef41c 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -19,6 +19,7 @@ on the subcommand:
>   git bisect start [--no-checkout] [ [...]] [--] [...]
>   git bisect (bad|new) []
>   git bisect (good|old) [...]
> + git bisect terms  

I think this is the other way around.

Other than that, this round looked OK to me.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html