Re: [PATCH v2 7/7] bisect: allows any terms set by user
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: Matthieu Moy matthieu@grenoble-inp.fr writes: # terms_defined is 0 when the user did not define the terms explicitely # yet. This is the case when running 'git bisect start bad_rev good_rev' # before we see an explicit reference to a term. terms_defined=0 The thing is: 'git bisect reset git bisect new HEAD git bisect new does not exist. Did you mean git bisect start HEAD? No I meant new but it can be 'git bisect bad' aswell OK, answering emails before coffee doesn't suit me well, I did not remember that the series was about new ;-). (Actually your use-case is not possible yet as of PATCH 3 which introduces start_bad_good. It is possible after PATCH 4) So ' git bisect reset git bisect bad answer yes to autostart ? ' In the case I rewrited, we saw a 'bad' but terms_defined value in bisect_start (called by the autostart) is 0. As you said, it is really equivalent to git bisect start git bisect bad the autostart is just a convenience piece of code to run git bisect start for the user, but does not change the logic of the code. Write good code for the normal case (start, and then bad), and it will just work without having to worry about in in the autostart case. -- 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 v2 7/7] bisect: allows any terms set by user
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: Matthieu Moy matthieu@grenoble-inp.fr writes: # terms_defined is 0 when the user did not define the terms explicitely # yet. This is the case when running 'git bisect start bad_rev good_rev' # before we see an explicit reference to a term. terms_defined=0 The thing is: 'git bisect reset git bisect new HEAD git bisect new does not exist. Did you mean git bisect start HEAD? (autostart ?) y' is strictly equivalent to 'git bisect reset git bisect start git bisect new HEAD' In both case terms_defined value would be 0 while we kinda know that a term has been used. I don't understand. The user didn't say either bad or good, so in both cases we haven't seen a term yet. Or I misunderstood what you meant by define a term. -- 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 v2 7/7] bisect: allows any terms set by user
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: To submit a v3 I would need answer about how we rebase the commit history and what do we do to simplify the life of the user with the terms (see my last mails). I was thinking of: -a config variable that would say :as long as I don't reset keep the terms of the previous bisection or we could decided that this is the default behaviour of bisect. -two config variables to choose default terms I'd say: keep it simple, try to close this series ASAP. It'll still be time to send another series once this one has settled. -- 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 v2 7/7] bisect: allows any terms set by user
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: Matthieu Moy matthieu@grenoble-inp.fr writes: +if test -s $GIT_DIR/TERMS_DEFINED +then +terms_defined=1 +get_terms +rm -rf $GIT_DIR/TERMS_DEFINED I don't understand why you need to delete this file. I did not review thoroughly so there may be a reason, but you can help the reader with a comment here. I will just complete Louis' answer. We delete it with backward compatibility with old/new in mind (even if old/new is not merged yet). For instance, after a old/new mode, if you do a 'bisect start rev1 rev2' the mode would be bad/good ie the default mode. So if you defined your terms, we decided it would only be for the following bisection. I would say for the current bisection, i.e. $ git bisect start $ git bisect terms foo bar # Scope starts here ... The first 'bar' commit is deadbeef. # Scope ends here $ git bisect terms foo bar You need to start by git bisect start Do you want me to do it for you [Y/n]? The next 'bisect start rev1 rev2' would be in bad/good mode. But this have to be discuted, do the user have to type 'git bisect terms' each bisection if he wants to use special terms ? To me, yes. If we allow arbitrary terms, then they will most likely be specific to one bisect session (e.g. if I bisect present/absent once, it tells me nothing about what I'll want for my next bisection). -- 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 v2 7/7] bisect: allows any terms set by user
Matthieu Moy matthieu@grenoble-inp.fr writes: I would say terms_defined is OK even if only the first patches get merged. The reason why you need this variable is because you need to know whether the terms have been defined or not, and to me that's the most important. Well said. Thanks. I'd suggest something like this: # terms_defined is 0 when the user did not define the terms explicitely s/citely/citly/; # yet. This is the case when running 'git bisect start bad_rev good_rev' # before we see an explicit reference to a term. terms_defined=0 Then PATCH 7/7 can add a mention of 'git bisect terms' just in the comment. -- 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 v2 7/7] bisect: allows any terms set by user
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: Louis Stuber louis--alexandre.stu...@ensimag.grenoble-inp.fr writes: Matthieu Moy matthieu@grenoble-inp.fr writes: Modifying in PATCH 7 some code that you introduced in PATCH 3 is suspicious. Is there any reason you did not name the variable terms_defined in the first place? (i.e. squash this hunk and the other instance of start_bad_good into PATCH 3) (Whether this is a rethorical question is up to you ;-) ) In the previouses versions where we only want to introduce old/new, the terms can only be defined in bisect_start if the user typed start bad good. The name start_bad_good is not very explicit indeed, but isn't it more appropriate in this case than terms_defined ? I agree with Louis, but maybe a consistant commit history is more important. But if only the first patches (which implement old/new ) would come to be accepted the name of the variable would sounds strange. I would say terms_defined is OK even if only the first patches get merged. The reason why you need this variable is because you need to know whether the terms have been defined or not, and to me that's the most important. I'd suggest something like this: # terms_defined is 0 when the user did not define the terms explicitely # yet. This is the case when running 'git bisect start bad_rev good_rev' # before we see an explicit reference to a term. terms_defined=0 Then PATCH 7/7 can add a mention of 'git bisect terms' just in the comment. -- 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 v2 7/7] bisect: allows any terms set by user
Matthieu Moy matthieu@grenoble-inp.fr writes: Modifying in PATCH 7 some code that you introduced in PATCH 3 is suspicious. Is there any reason you did not name the variable terms_defined in the first place? (i.e. squash this hunk and the other instance of start_bad_good into PATCH 3) (Whether this is a rethorical question is up to you ;-) ) In the previouses versions where we only want to introduce old/new, the terms can only be defined in bisect_start if the user typed start bad good. The name start_bad_good is not very explicit indeed, but isn't it more appropriate in this case than terms_defined ? I don't understand why you need to delete this file. I did not review thoroughly so there may be a reason, but you can help the reader with a comment here. I think it's a mistake. I'd say we should put this test just before the bisect_clean_state || exit line, but that would deserve more attention indeed. The idea is to delete the file at the right moment because we don't want it to exist again when the user starts an other bisection, but also have an intelligent behaviour if the start command fails. -- 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 v2 7/7] bisect: allows any terms set by user
Junio C Hamano gits...@pobox.com writes:
Junio C Hamano gits...@pobox.com writes:
+bisect_terms () {
+test $# -eq 2 ||
+die You need to give me at least two arguments
+
+if ! test -s $GIT_DIR/BISECT_START
+then
+echo $1 $GIT_DIR/BISECT_TERMS
+echo $2 $GIT_DIR/BISECT_TERMS
+echo 1 $GIT_DIR/TERMS_DEFINED
+else
+die A bisection has already started, please use \
+'git bisect reset' to restart and change the terms
+fi
+}
+
I think git bisect terms is a good way to help a user to recall
what two names s/he decided to use for the current session. So
dying 'already started' with suggestion for 'reset' is OK, but at
the same time, helping the user to continue the current bisection by
giving a message along the lines of You are hunting for a commit
that is at the boundary of the old state (you are calling it
'$NAME_OLD') and the new state ('$NAME_NEW') would be a good idea.
I'd put a very verbose message explaining the situation and the way out
(use 'git bisect') for the second die, and I would consider git
bisect terms without arguments as a valid command to ask please tell
me what the terms are.
--
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 v2 7/7] bisect: allows any terms set by user
Junio C Hamano gits...@pobox.com writes: Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: -USAGE='[help|start|bad|good|new|old|skip|next|reset|visualize|replay|log|run]' +USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]' I think this patch makes the whole series go in the right direction. I wonder if you can skip the we only support new/old if you are not doing bog-standard bad/good step and start from this bisect terms one, though. While I think we should not hardcode too much in the code, I also think it makes sense to have hardcoded old/new in the user-interface. If you give me just git bisect terms first-term second-term then half of the times, I'll use old/new in the wrong order. And if I hadn't looked to bisect close enough, I'd even complain that the terms are not usable interchangeably. At least, a in a flow like git bisect start git bisect new git checkout old-sha git bisect old there's no ambiguity. So, to me, having both hardcoded old/new (unambiguous) and git bisect terms (for flexibility) makes sense. Another important parameter: the students are finishing their project tomorrow. They may continue on their personnal free time, but at best their time budget is considerably reduced, and from my past experience, the time budget usually reaches 0. That's not a reason to merge bad code in git.git, but an incentive to close the series with something going in the right direction. Typically: - preliminary clean-up steps (e.g. correct 'mistook'); This is straightforward - use $name_new and $name_old throughout the code, giving them 'bad' and 'good' as hardcoded values; finally This is relatively uncontroversial, and should be easy to finish. I would consider it a good thing anyway. - add 'bisect terms' support. I'd put this on the low-priority whishlist. If the codebase is ready and a preliminary patch exists, it will be relatively easy for someone else to finish the job. -- 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 v2 7/7] bisect: allows any terms set by user
Matthieu Moy matthieu@grenoble-inp.fr writes:
Junio C Hamano gits...@pobox.com writes:
Junio C Hamano gits...@pobox.com writes:
+bisect_terms () {
+ test $# -eq 2 ||
+ die You need to give me at least two arguments
+
+ if ! test -s $GIT_DIR/BISECT_START
+ then
+ echo $1 $GIT_DIR/BISECT_TERMS
+ echo $2 $GIT_DIR/BISECT_TERMS
+ echo 1 $GIT_DIR/TERMS_DEFINED
+ else
+ die A bisection has already started, please use \
+ 'git bisect reset' to restart and change the terms
+ fi
+}
+
I think git bisect terms is a good way to help a user to recall
what two names s/he decided to use for the current session. So
dying 'already started' with suggestion for 'reset' is OK, but at
the same time, helping the user to continue the current bisection by
giving a message along the lines of You are hunting for a commit
that is at the boundary of the old state (you are calling it
'$NAME_OLD') and the new state ('$NAME_NEW') would be a good idea.
I'd put a very verbose message explaining the situation and the way out
(use 'git bisect') for the second die, and I would consider git
bisect terms without arguments as a valid command to ask please tell
me what the terms are.
Of course you are right. The remind me what I was doing help
should be given when the user asks git bisect terms without any
parameters, not in else die part.
Thanks for a correction.
--
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 v2 7/7] bisect: allows any terms set by user
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: -USAGE='[help|start|bad|good|new|old|skip|next|reset|visualize|replay|log|run]' +USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]' I think this patch makes the whole series go in the right direction. I wonder if you can skip the we only support new/old if you are not doing bog-standard bad/good step and start from this bisect terms one, though. While I think we should not hardcode too much in the code, I also think it makes sense to have hardcoded old/new in the user-interface. OK. -- 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 v2 7/7] bisect: allows any terms set by user
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes:
- # start_bad_good is used to detect if we did a
- # 'git bisect start bad_rev good_rev'
- start_bad_good=0
+ # terms_defined is used to detect if we did a
+ # 'git bisect start bad_rev good_rev' or if the user
+ # defined his own terms with git bisect terms
+ terms_defined=0
Modifying in PATCH 7 some code that you introduced in PATCH 3 is
suspicious. Is there any reason you did not name the variable
terms_defined in the first place? (i.e. squash this hunk and the other
instance of start_bad_good into PATCH 3)
(Whether this is a rethorical question is up to you ;-) )
+ if test -s $GIT_DIR/TERMS_DEFINED
+ then
+ terms_defined=1
+ get_terms
+ rm -rf $GIT_DIR/TERMS_DEFINED
I don't understand why you need to delete this file. I did not review
thoroughly so there may be a reason, but you can help the reader with a
comment here.
+bisect_terms () {
+ test $# -eq 2 ||
+ die You need to give me at least two arguments
+
+ if ! test -s $GIT_DIR/BISECT_START
+ then
+ echo $1 $GIT_DIR/BISECT_TERMS
+ echo $2 $GIT_DIR/BISECT_TERMS
+ echo 1 $GIT_DIR/TERMS_DEFINED
Space after .
@@ -574,7 +600,7 @@ case $# in
git bisect -h ;;
start)
bisect_start $@ ;;
- bad|good|new|old)
+ bad|good|new|old|$NAME_BAD|$NAME_GOOD)
See my other message about quoting.
--
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 v2 7/7] bisect: allows any terms set by user
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes:
-USAGE='[help|start|bad|good|new|old|skip|next|reset|visualize|replay|log|run]'
+USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|replay|log|run]'
I think this patch makes the whole series go in the right direction.
I wonder if you can skip the we only support new/old if you are not
doing bog-standard bad/good step and start from this bisect terms
one, though.
Then you do not even have to treat new/old any specially, and do not
even have to list them in the above list.
@@ -79,9 +81,16 @@ bisect_start() {
orig_args=$(git rev-parse --sq-quote $@)
bad_seen=0
eval=''
- # start_bad_good is used to detect if we did a
- # 'git bisect start bad_rev good_rev'
- start_bad_good=0
+ # terms_defined is used to detect if we did a
+ # 'git bisect start bad_rev good_rev' or if the user
+ # defined his own terms with git bisect terms
+ terms_defined=0
I like this change very much; it removes the mysteriously misnamed
start-bad-good variable (because you do not really _care_ that
'start' was what implicitly decided that good/bad pair is the term
we use in this session; what you care is that the terms are already
known or not).
That is another reason why I think it would be a better organization
for the patch series to do without the intermediate we now add new/old
as another hardcoded values on top of the traditional bad/good.
That is, I would think a reasonable progression of the series would
look more like these three steps:
- preliminary clean-up steps (e.g. correct 'mistook');
- use $name_new and $name_old throughout the code, giving them
'bad' and 'good' as hardcoded values; finally
- add 'bisect terms' support.
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 v2 7/7] bisect: allows any terms set by user
Junio C Hamano gits...@pobox.com writes:
I like this change very much; it removes the mysteriously misnamed
start-bad-good variable (because you do not really _care_ that
'start' was what implicitly decided that good/bad pair is the term
we use in this session; what you care is that the terms are already
known or not).
That is another reason why I think it would be a better organization
for the patch series to do without the intermediate we now add new/old
as another hardcoded values on top of the traditional bad/good.
That is, I would think a reasonable progression of the series would
look more like these three steps:
- preliminary clean-up steps (e.g. correct 'mistook');
- use $name_new and $name_old throughout the code, giving them
'bad' and 'good' as hardcoded values; finally
- add 'bisect terms' support.
Just in case I confused readers with a message that apparently
conflicts with what I said in the ancient thread:
http://thread.gmane.org/gmane.comp.version-control.git/199758/focus=200025
I am admitting that I was wrong. Or perhaps I was right back then,
but the world has changed ;-).
We have been hearing bisect bad/good is hard to use for the last 3
years since that thread discussed this topic, and that made me
realize that addition of single new/old may not be good enough, and
we should bite the bullet to support 'bisect terms' properly, making
bad/good and new/old even less special (contrary to what I said back
then in that thread we only need these two pairs), following the
suggestion by Phil Hord in that thread.
And the suggested three-step approach above reflects that new world
order in my mind. We admit that the machinery should have been
built around a value-agnostic old vs new in the first place, but
unfortunately it wasn't. So we belatedly update the system to use
these two terms internally to express the logic by naming the
variables name_old and name_new after these two value-agnostic
concepts, and then support the traditional good vs bad as a mere
default values for the names of old and new states.
One thing I forgot to say in the review of this patch:
+bisect_terms () {
+ test $# -eq 2 ||
+ die You need to give me at least two arguments
+
+ if ! test -s $GIT_DIR/BISECT_START
+ then
+ echo $1 $GIT_DIR/BISECT_TERMS
+ echo $2 $GIT_DIR/BISECT_TERMS
+ echo 1 $GIT_DIR/TERMS_DEFINED
+ else
+ die A bisection has already started, please use \
+ 'git bisect reset' to restart and change the terms
+ fi
+}
+
I think git bisect terms is a good way to help a user to recall
what two names s/he decided to use for the current session. So
dying 'already started' with suggestion for 'reset' is OK, but at
the same time, helping the user to continue the current bisection by
giving a message along the lines of You are hunting for a commit
that is at the boundary of the old state (you are calling it
'$NAME_OLD') and the new state ('$NAME_NEW') would be a good idea.
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
