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

2015-06-26 Thread Junio C Hamano
Matthieu Moy matthieu@imag.fr 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] [bad [good...]] [--] [paths...]
   git bisect (bad|new) [rev]
   git bisect (good|old) [rev...]
 + git bisect terms term-old term-new

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


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

2015-06-26 Thread Matthieu Moy
Christian Couder christian.cou...@gmail.com writes:

 On Fri, Jun 26, 2015 at 6:58 PM, Matthieu Moy matthieu@imag.fr wrote:
 From: Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr

 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 2actual 
-   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

2015-06-26 Thread Christian Couder
On Fri, Jun 26, 2015 at 6:58 PM, Matthieu Moy matthieu@imag.fr wrote:
 From: Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr

 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