Re: [PATCH v10.1 7/7] bisect: allow any terms set by user
On Mon, Jun 29, 2015 at 11:32 AM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
Christian Couder christian.cou...@gmail.com writes:
[...]
I find it particularly frustrating that we issue the message:
The merge base %s is bad.\n
This means the bug has been fixed
between %s and [%s].\n
I find it a good safety feature.
bisect is all about finding the commit where a property has changed,
That is your interpretation of this command. On the man page there is:
git-bisect - Find by binary search the change that introduced a bug
So its stated purpose is to find the first bad commit. Not to find a fix.
This doesn't mean that we cannot or should not make it possible to
find a fix, but we shoudn't try to find a fix when there is doubt
about whether the user wants to find a fix or the first bad commit.
and
here it stops, saying I know it's between A and B, but I won't go
further.
You can interpret it that way, but my interpretation of this is that
the real reason why we stop is because we don't know what the users
really wants to do, find the fix, or really find the first bad commit.
In particular when bisecting from two branches, the user knows that
branch A is good, and branch B is bad, but does not necessarily know
whether it's a regression in B or a
fix in A. The fact that bisect can find out should be just normal from
the user point of view. There's no mistake involved, nothing to fix, and
nothing that went wrong.
Well in this case, it's possible that the merge base is bad and what
the user is interested in is the first bad commit that was commited
before the merge base. We just don't know, in the case the merge base
is bad, what is more interesting for the user.
The question asked by the user is X is good, Y is bad, tell me where
exactly it changed.
That's your interpretation of the question asked by the user.
It can be interpreted otherwise.
We can't know for sure what is interesting for
the user, but we can answer his question anyway.
You can answer your interpretation of the question, yes, but this
means taking risks that we don't need to take in my opinion.
If people really want it, we can still have an option called for
example --find-fix that could automatically try to find the fix when
the merge base is bad without displaying the message that annoys
you. It would make it explicit that it is ok to find a fix.
Similarly, there can be several commits that introduce the same
regression (this may happen if you cherry picked the guilty commit from
branch A to branch B, and then merged A and B, or if you broke, fixed,
and then broke again). bisect finds one transition from good to bad, but
not all. It may or may not be the one the user wanted, but we can't
know.
Yeah, but this is different as we would still find a first bad
commit, not a fix.
I think I prefer term to name.
Ok with that. I agree that it would be more consistent to have a git
bisect terms and --term-{old,new,bad,good}.
OK, I've applied it.
Great!
--
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.1 7/7] bisect: allow any terms set by user
Christian Couder christian.cou...@gmail.com writes:
On Mon, Jun 29, 2015 at 9:34 AM, Matthieu Moy
As a user, when I
discovered git bisect, I was actually surprised that it expected one
particular order between good and bad. I would have expected to be able
to say this is good, this is bad, tell me where it changed without
having an idea of who's good and who's bad.
Maybe, but it's not how it has been developed.
... but it's how it would behave if we had this guessing feature. The
user does not have to care whether we internally need good is an
ancestor of bad if we can provide a user-interface which does not need
that.
I find it particularly frustrating that we issue the message:
The merge base %s is bad.\n
This means the bug has been fixed
between %s and [%s].\n
bisect is all about finding the commit where a property has changed, and
here it stops, saying I know it's between A and B, but I won't go
further.
In particular when bisecting from two branches, the user knows that
branch A is good, and branch B is bad, but does not necessarily know
whether it's a regression in B or a
fix in A. The fact that bisect can find out should be just normal from
the user point of view. There's no mistake involved, nothing to fix, and
nothing that went wrong.
Well in this case, it's possible that the merge base is bad and what
the user is interested in is the first bad commit that was commited
before the merge base. We just don't know, in the case the merge base
is bad, what is more interesting for the user.
The question asked by the user is X is good, Y is bad, tell me where
exactly it changed. We can't know for sure what is interesting for
the user, but we can answer his question anyway.
Similarly, there can be several commits that introduce the same
regression (this may happen if you cherry picked the guilty commit from
branch A to branch B, and then merged A and B, or if you broke, fixed,
and then broke again). bisect finds one transition from good to bad, but
not all. It may or may not be the one the user wanted, but we can't
know.
I think I prefer term to name.
Ok with that. I agree that it would be more consistent to have a git
bisect terms and --term-{old,new,bad,good}.
OK, I've applied it.
--
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.1 7/7] bisect: allow any terms set by user
Junio C Hamano gits...@pobox.com writes: I moderately hate to see both from aesthetics point of view, but can we at least lose --name- prefix? I changed it to --term- prefix, but I'd rather not drop it. When reading --old=foo, it is not clear to me whether the meaning should be the term used for old is foo or mark foo as old. The longer version does not have this problem. -- 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.1 7/7] bisect: allow any terms set by user
Christian Couder christian.cou...@gmail.com writes: On Mon, Jun 29, 2015 at 11:32 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: bisect is all about finding the commit where a property has changed, That is your interpretation of this command. On the man page there is: git-bisect - Find by binary search the change that introduced a bug So its stated purpose is to find the first bad commit. Not to find a fix. This is a limitation of the current bisect, but the discussion is precisely about removing this limitation. I still don't understand what risk we are taking by doing the bisection anyway. I can't imagine a case where we would harm the user by doing so. I just tested with Mercurial, and looking for a fix instead of a regression just works: $ hg bisect --good 4 $ hg bisect --bad 1 Testing changeset 2:d75a2d042c99 (3 changesets remaining, ~1 tests) 1 files updated, 0 files merged, 0 files removed, 0 files unresolved $ hg bisect --bad Testing changeset 3:9d27d9c02e28 (2 changesets remaining, ~1 tests) 1 files updated, 0 files merged, 0 files removed, 0 files unresolved $ hg bisect --bad The first good revision is: changeset: 4:1dd9bb959eb6 tag: tip user:Matthieu Moy matthieu@imag.fr date:Mon Jun 29 17:07:51 2015 +0200 summary: foo I don't see anything wrong with this. (OTOH, hg bisect does not accept revisions which aren't parent of each other) -- 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.1 7/7] bisect: allow any terms set by user
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: I moderately hate to see both from aesthetics point of view, but can we at least lose --name- prefix? I changed it to --term- prefix, but I'd rather not drop it. When reading --old=foo, it is not clear to me whether the meaning should be the term used for old is foo or mark foo as old. The longer version does not have this problem. Yeah, my suggestion was based on one assumption I did not mention, which is that we do not need to crowd bisect start with this option when we have bisect terms, as long as bisect start does not have to take both what are the terms and which commits are painted using which one of the two terms on its command line, the --name- prefix was unnecessary. But if you are dropping bisect terms and allowing the terms specified only from bisect start as you mentioned in the cover letter of v11, that changes the equation. And I think start is the place that sets up a clean slate, and that is the only place where you can optionally declare your custom terms is a very sensible design. I do not have a problem with --terms prefix in that case. 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.1 7/7] bisect: allow any terms set by user
Matthieu Moy matthieu@grenoble-inp.fr writes: So, my proposal would be to remove the old/new patch from the series, and keep the other patches. What do you think? Let me answer after reading v11 through. but now it would be more clear that $name_good and $name_bad is a bad way to name internal variables and files in $GIT_DIR. The inferred 'ah you are hunting for regression' mode would call old ones 'bad' and new ones 'good', they have to be given value neutral names, e.g. $name_old and $name_new. Ideally, the whole code would be ported to use old/new, but the more I read the code the more I agree with Christian actually: we still have many more instances of good/bad in the code (e.g. functions check_good_are_ancestors_of_bad, for_each_good_bisect_ref, ...), so having just name_new/name_old would make the code very inconsistant. Oh, no question about that. I was hoping that we would at least get the concensus that we should move all to old/new and these good/bad in code no longer make sense. It just was that introducing new variables and functions whose names follow the convention that reflects the world view that is no longer valid (i.e. good is always old and bad is always new) in a series that introduces the new world view somehow felt wrong. -- 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.1 7/7] bisect: allow any terms set by user
Junio C Hamano gits...@pobox.com writes: I _think_ bulk of Antoine and Matthieu's work can be salvaged/reused to implement the proposal, I'm obviously biaised since I already spent time on the bisect terms idea, and I would hate to see my work and Antoine Louis' thrown away. But I have to admit that I do like the idea of git bisect figuring out which commit is the old and which commit is the new one. It's much easier to implement after the series. I'm currently forbidding redefining good as bad and vice versa, but that's just to avoid confusion and because I didn't test this case, but it should just work. So, essentially an implementation of this guess who's old and who's new could be: if we find a good commit that is an ancestor of an old commit, swap the terms in BISECT_TERMS. When this happens before we started to set any refs, this should do the trick. In general, we should rename the good-$sha1 reference to good and the bad to bad-$sha1 (there are corner-cases where the user already specified several good-$sha1 refs, in which case we would need to discard some of them). I'm getting out of Git time budget, so I can't be the one doing this, at least not soon. So, one option is to take the series as-is, and wait for someone to implement the guess who's old and who's new later on top of it. One drawback would be that we'd end up having the not-so-useful feature bisect terms in the user-interface. At least, I am now convinced that hardcoding the pair old/new is not needed. In the short term, we can have git bisect start --name-old foo --name-new bar which avoids the One needs to remember in which order to give terms issue we used to have, so we don't need to clutter the user-interface with many ways to do the same thing. OTOH, the bisect terms feature wouldn't be completely useless: not everything is good or bad, so leaving the option to the user to tag slow/fast, present/absent, ... still makes sense. So, my proposal would be to remove the old/new patch from the series, and keep the other patches. What do you think? but now it would be more clear that $name_good and $name_bad is a bad way to name internal variables and files in $GIT_DIR. The inferred 'ah you are hunting for regression' mode would call old ones 'bad' and new ones 'good', they have to be given value neutral names, e.g. $name_old and $name_new. Ideally, the whole code would be ported to use old/new, but the more I read the code the more I agree with Christian actually: we still have many more instances of good/bad in the code (e.g. functions check_good_are_ancestors_of_bad, for_each_good_bisect_ref, ...), so having just name_new/name_old would make the code very inconsistant. It's easier to read the code thinking good revs are old, bad revs are recent; maybe some magic swapped the terms but I don't need to worry about this for now. -- 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.1 7/7] bisect: allow any terms set by user
Christian Couder christian.cou...@gmail.com writes: On Sun, Jun 28, 2015 at 8:46 AM, Michael Haggerty mhag...@alum.mit.edu wrote: I understand that the user might make a mistake when marking the initial commits, but as soon as bisect says Commit sha1-abbrev is an ancestor of sha1-abbrev, so I will look for the commit that caused the transition from xyzzy to plugh. then I hope the user will notice and correct her/his mistake. This looks fragile to me. Unfortunately many users will probably not read it and continue, and then spend a lot of time later trying to understand what went wrong, I don't understand what you mean by went wrong. As a user, when I discovered git bisect, I was actually surprised that it expected one particular order between good and bad. I would have expected to be able to say this is good, this is bad, tell me where it changed without having an idea of who's good and who's bad. In particular when bisecting from two branches, the user knows that branch A is good, and branch B is bad, but does not necessarily know whether it's a regression in B or a fix in A. The fact that bisect can find out should be just normal from the user point of view. There's no mistake involved, nothing to fix, and nothing that went wrong. By the way we could use mark or term instead of name in the option name (like --mark-old or --term-old) and in the code too if it looks clearer. I prefer term to mark because mark is both a verb and a noun, so --mark-old=foo could mean both mark foo as old or the name of the marks for old commits is foo. I think I prefer term to name. -- 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.1 7/7] bisect: allow any terms set by user
On Mon, Jun 29, 2015 at 9:34 AM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
Christian Couder christian.cou...@gmail.com writes:
On Sun, Jun 28, 2015 at 8:46 AM, Michael Haggerty mhag...@alum.mit.edu
wrote:
I understand that the user might make a mistake when marking the initial
commits, but as soon as bisect says
Commit sha1-abbrev is an ancestor of sha1-abbrev, so I
will look for the commit that caused the transition from
xyzzy to plugh.
then I hope the user will notice and correct her/his mistake.
This looks fragile to me. Unfortunately many users will probably not
read it and continue, and then spend a lot of time later trying to
understand what went wrong,
I don't understand what you mean by went wrong.
It happens that users mistake the good and the bad commits when
giving them to git bisect.
Right now in the most common case, we can error out because we know
that a bad commit cannot be an ancestor of a good commit.
As a user, when I
discovered git bisect, I was actually surprised that it expected one
particular order between good and bad. I would have expected to be able
to say this is good, this is bad, tell me where it changed without
having an idea of who's good and who's bad.
Maybe, but it's not how it has been developed.
In particular when bisecting
from two branches, the user knows that branch A is good, and branch B is
bad, but does not necessarily know whether it's a regression in B or a
fix in A. The fact that bisect can find out should be just normal from
the user point of view. There's no mistake involved, nothing to fix, and
nothing that went wrong.
Well in this case, it's possible that the merge base is bad and what
the user is interested in is the first bad commit that was commited
before the merge base. We just don't know, in the case the merge base
is bad, what is more interesting for the user.
So I disagree with you and Michael that we should decide that the user
is interested by the fix in this case. It's better to error out like
we do now and let the user decide what he/she wants rather than decide
for him/her that he/she is interested by the fix.
By the way we could use mark or term instead of name in the
option name (like --mark-old or --term-old) and in the code too if it
looks clearer.
I prefer term to mark because mark is both a verb and a noun, so
--mark-old=foo could mean both mark foo as old or the name of the
marks for old commits is foo.
I think I prefer term to name.
Ok with that. I agree that it would be more consistent to have a git
bisect terms and --term-{old,new,bad,good}.
Thanks,
Christian.
--
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.1 7/7] bisect: allow any terms set by user
On 06/28/2015 09:32 AM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: I understand that the user might make a mistake when marking the initial commits, but as soon as bisect says Commit sha1-abbrev is an ancestor of sha1-abbrev, so I will look for the commit that caused the transition from xyzzy to plugh. then I hope the user will notice and correct her/his mistake. For example, a session could be started with git bisect start --mark=broken committish --mark=fixed committish Interesting. If we extend that line of thought further, maybe we do not even need to add new/old, fixed/broken, or slow/fast. You just _always_ say good or bad. If something is slow, you say bad and if something is fast, you say good. Yes, I think good and bad would usually be perfectly intuitive and would almost always be usable. [...] No need for bisect new, bisect old, or bisect terms, let alone bisect terms --new=fast --old=slow. The tool just does the right thing because it already has the information necessary to infer what the user means by 'good' and 'bad', and the initial topology determines which transition, either from 'good' to 'bad' or from 'bad' to 'good', the user is hunting for. Correct. The only caveat is if the initial good and bad commits are not ancestrally related to each other. But in this case, I think bisect asks the user to test a merge base anyway, and once that one has been tested it will be clear which of the labels comes before the other. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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.1 7/7] bisect: allow any terms set by user
On Sat, Jun 27, 2015 at 10:51 PM, Michael Haggerty mhag...@alum.mit.edu wrote: I would like to remind everybody of my old claim that it would be possible to teach `git bisect` to infer by itself which term means older and which term means newer: http://article.gmane.org/gmane.comp.version-control.git/244036 But then one mistake at the beginning and the user will be on a wrong track during the whole bisect session, no? Unless you make absolutely clear when making the intelligent decision what Git inferred, that is. For something complex like bisect, I highly suspect that a tool that is more intelligent than the end users (more precisely, a tool that it thinks it is more intelligent) would hurt them more than it helps them. Of course, that is only my claim ;-) -- 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.1 7/7] bisect: allow any terms set by user
On 06/28/2015 08:15 AM, Junio C Hamano wrote: On Sat, Jun 27, 2015 at 10:51 PM, Michael Haggerty mhag...@alum.mit.edu wrote: I would like to remind everybody of my old claim that it would be possible to teach `git bisect` to infer by itself which term means older and which term means newer: http://article.gmane.org/gmane.comp.version-control.git/244036 But then one mistake at the beginning and the user will be on a wrong track during the whole bisect session, no? Unless you make absolutely clear when making the intelligent decision what Git inferred, that is. Definitely, `git bisect` should tell the user what it inferred. For something complex like bisect, I highly suspect that a tool that is more intelligent than the end users (more precisely, a tool that it thinks it is more intelligent) would hurt them more than it helps them. This isn't about making bisect more intelligent than the end users. It is about not forcing the user cumbersomely to spell out redundant information because the tool is too stupid. If I mark one commit broken and another commit fixed, and the broken commit is an ancestor of the fixed commit, then it is pretty obvious that I am looking for the commit that caused the transition broken - fixed. The same if I mark one commit xyzzy and the other one plugh. I understand that the user might make a mistake when marking the initial commits, but as soon as bisect says Commit sha1-abbrev is an ancestor of sha1-abbrev, so I will look for the commit that caused the transition from xyzzy to plugh. then I hope the user will notice and correct her/his mistake. For example, a session could be started with git bisect start --mark=broken committish --mark=fixed committish and from then on git bisect broken git bisect fixed Or, if the user doesn't want to specify both endpoints on the `start` line, git bisect start git bisect --mark=broken [committish] git bisect --mark=fixed [committish] Essentially, specifying `--mark=name` once would make `name` a shorthand for `--mark=name`. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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.1 7/7] bisect: allow any terms set by user
Michael Haggerty mhag...@alum.mit.edu writes: I understand that the user might make a mistake when marking the initial commits, but as soon as bisect says Commit sha1-abbrev is an ancestor of sha1-abbrev, so I will look for the commit that caused the transition from xyzzy to plugh. then I hope the user will notice and correct her/his mistake. For example, a session could be started with git bisect start --mark=broken committish --mark=fixed committish Interesting. If we extend that line of thought further, maybe we do not even need to add new/old, fixed/broken, or slow/fast. You just _always_ say good or bad. If something is slow, you say bad and if something is fast, you say good. If you start git bisect start maint master (and as always, bad comes before good) because some operation is very slow in maint but somehow is usably fast in master, you are automatically hunting for a performance fix that you can cherry-pick to the maintenance track. No need for bisect new, bisect old, or bisect terms, let alone bisect terms --new=fast --old=slow. The tool just does the right thing because it already has the information necessary to infer what the user means by 'good' and 'bad', and the initial topology determines which transition, either from 'good' to 'bad' or from 'bad' to 'good', the user is hunting for. I really like that simplicity. -- 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.1 7/7] bisect: allow any terms set by user
Michael Haggerty mhag...@alum.mit.edu writes: On 06/28/2015 09:32 AM, Junio C Hamano wrote: You just _always_ say good or bad. If something is slow, you say bad and if something is fast, you say good. Yes, I think good and bad would usually be perfectly intuitive and would almost always be usable. [...] No need for bisect new, bisect old, or bisect terms, let alone bisect terms --new=fast --old=slow. The tool just does the right thing because it already has the information necessary to infer what the user means by 'good' and 'bad', and the initial topology determines which transition, either from 'good' to 'bad' or from 'bad' to 'good', the user is hunting for. Correct. The only caveat is if the initial good and bad commits are not ancestrally related to each other. But in this case, I think bisect asks the user to test a merge base anyway, and once that one has been tested it will be clear which of the labels comes before the other. The more I look at the proposal, the more I like it. The old way of thinking is that we need to keep 'bad' for newer one and 'good' for older one, that required us to invent 'broken' vs 'fixed', or value neutral 'old' vs 'new'. Then we extend it to a random pair of 'terms', but we reserve 'good', 'bad', etc. and do not allow the user to say old was bad, new is now good. With your proposal, the user can just say oh this is good, vs oh this is bad. The mental model becomes much simpler. I _think_ bulk of Antoine and Matthieu's work can be salvaged/reused to implement the proposal, but now it would be more clear that $name_good and $name_bad is a bad way to name internal variables and files in $GIT_DIR. The inferred 'ah you are hunting for regression' mode would call old ones 'bad' and new ones 'good', they have to be given value neutral names, e.g. $name_old and $name_new. -- 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.1 7/7] bisect: allow any terms set by user
On Sun, Jun 28, 2015 at 8:46 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 06/28/2015 08:15 AM, Junio C Hamano wrote: On Sat, Jun 27, 2015 at 10:51 PM, Michael Haggerty mhag...@alum.mit.edu wrote: I would like to remind everybody of my old claim that it would be possible to teach `git bisect` to infer by itself which term means older and which term means newer: http://article.gmane.org/gmane.comp.version-control.git/244036 But then one mistake at the beginning and the user will be on a wrong track during the whole bisect session, no? Unless you make absolutely clear when making the intelligent decision what Git inferred, that is. Definitely, `git bisect` should tell the user what it inferred. For something complex like bisect, I highly suspect that a tool that is more intelligent than the end users (more precisely, a tool that it thinks it is more intelligent) would hurt them more than it helps them. This isn't about making bisect more intelligent than the end users. It is about not forcing the user cumbersomely to spell out redundant information because the tool is too stupid. If I mark one commit broken and another commit fixed, and the broken commit is an ancestor of the fixed commit, then it is pretty obvious that I am looking for the commit that caused the transition broken - fixed. The same if I mark one commit xyzzy and the other one plugh. I understand that the user might make a mistake when marking the initial commits, but as soon as bisect says Commit sha1-abbrev is an ancestor of sha1-abbrev, so I will look for the commit that caused the transition from xyzzy to plugh. then I hope the user will notice and correct her/his mistake. This looks fragile to me. Unfortunately many users will probably not read it and continue, and then spend a lot of time later trying to understand what went wrong, not remembering about the message at all. The message looks like an informative message. At least we should add something like Please check that it is what you want to do and abort with 'git bisect reset' if it is not. For example, a session could be started with git bisect start --mark=broken committish --mark=fixed committish This look nearly the same as: git bisect start --name-old=broken --broken=committish --name-new=fixed --fixed=committish except that it looks safer and more backward compatible to me with --name-old and --name-new. By the way we could use mark or term instead of name in the option name (like --mark-old or --term-old) and in the code too if it looks clearer. and from then on git bisect broken git bisect fixed Or, if the user doesn't want to specify both endpoints on the `start` line, git bisect start git bisect --mark=broken [committish] git bisect --mark=fixed [committish] We could do that too with: git bisect start git bisect --name-old=broken broken [committish] git bisect --name-new=fixed fixed [committish] and/or: git bisect start git bisect --name-old=broken --broken=[committish] git bisect --name-new=fixed --fixed=[committish] -- 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.1 7/7] bisect: allow any terms set by user
Christian Couder christian.cou...@gmail.com writes:
On Sat, Jun 27, 2015 at 6:25 AM, Junio C Hamano gits...@pobox.com wrote:
On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder
christian.cou...@gmail.com wrote:
If we don't want to support positional arguments, then I would suggest
supporting first the following instead:
git bisect terms --name-good=fast --name-bad=slow
git bisect terms --name-bad=slow --name-good=fast
This would make the interface consistent with the code.
Which somewhat defeats the point of introducing old and new, though.
The terms support is for people who feel that good/bad would be too
confusing
for the particular bisect session (e.g. because they are hunting for a fix).
Well if --name-old and --name-new are also available as synonyms, it
would not be too bad I think.
People could use the option names that fit their mental model or their
use case better.
OK, I'll add both.
We may want to start supporting
git bisect start --new=master --old=maint
Maybe we could also support:
git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master
The same comment for the token after --name-, but allowing the terms to be
set
at start could be a type-saver. With need for added --name-
prefix (worse, twice),
I am not sure if it would be seen as a useful type-saver, though.
At least people don't need to remember if they have to use git bisect
term before or after starting :-)
OK, first lesson learnt: the UI is controversial. So, I'm working on a
series that splits the last patch into several preparation steps, and a
very simple patch to implement the UI.
I have a draft here https://github.com/moy/git/commits/bisect-terms
I'll go through it once more and send it later.
As a teaser, the patch implementing the UI is just:
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -70,6 +70,26 @@ bisect_autostart() {
}
}
+parse_name_args() {
+ while [ $# -gt 0 ]; do
+ arg=$1
+ case $arg in
+ --name-good|--name-old)
+ shift
+ must_write_terms=1
+ NAME_GOOD=$1
+ shift ;;
+ --name-bad|--name-new)
+ shift
+ must_write_terms=1
+ NAME_BAD=$1
+ shift ;;
+ *)
+ shift ;;
+ esac
+ done
+}
+
bisect_start() {
#
# Check for one bad and then some good revisions.
@@ -88,6 +108,9 @@ bisect_start() {
else
mode=''
fi
+ # Parse --name-* options before the other to allow any mix of
+ # --name-* and revisions on the command-line.
+ parse_name_args $@
while [ $# -gt 0 ]; do
arg=$1
case $arg in
@@ -98,6 +121,8 @@ bisect_start() {
--no-checkout)
mode=--no-checkout
shift ;;
+ --name-good|--name-old|--name-bad|--name-new)
+ shift 2 ;;
--*)
die $(eval_gettext unrecognised option: '\$arg') ;;
*)
--
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.1 7/7] bisect: allow any terms set by user
Matthieu Moy matthieu@grenoble-inp.fr writes: Christian Couder christian.cou...@gmail.com writes: On Sat, Jun 27, 2015 at 6:25 AM, Junio C Hamano gits...@pobox.com wrote: On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder christian.cou...@gmail.com wrote: If we don't want to support positional arguments, then I would suggest supporting first the following instead: git bisect terms --name-good=fast --name-bad=slow git bisect terms --name-bad=slow --name-good=fast This would make the interface consistent with the code. Which somewhat defeats the point of introducing old and new, though. The terms support is for people who feel that good/bad would be too confusing for the particular bisect session (e.g. because they are hunting for a fix). Well if --name-old and --name-new are also available as synonyms, it would not be too bad I think. People could use the option names that fit their mental model or their use case better. OK, I'll add both. I moderately hate to see both from aesthetics point of view, but can we at least lose --name- prefix? Having to repeat that twice does not add any value to the subcommand. -- 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.1 7/7] bisect: allow any terms set by user
On 06/27/2015 06:25 AM, Junio C Hamano wrote: On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder christian.cou...@gmail.com wrote: If we don't want to support positional arguments, then I would suggest supporting first the following instead: git bisect terms --name-good=fast --name-bad=slow git bisect terms --name-bad=slow --name-good=fast This would make the interface consistent with the code. Which somewhat defeats the point of introducing old and new, though. The terms support is for people who feel that good/bad would be too confusing for the particular bisect session (e.g. because they are hunting for a fix). We may want to start supporting git bisect start --new=master --old=maint Maybe we could also support: git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master The same comment for the token after --name-, but allowing the terms to be set at start could be a type-saver. With need for added --name- prefix (worse, twice), I am not sure if it would be seen as a useful type-saver, though. I would like to remind everybody of my old claim that it would be possible to teach `git bisect` to infer by itself which term means older and which term means newer: http://article.gmane.org/gmane.comp.version-control.git/244036 I think that making `bisect` smarter could make the UI simpler, though admittedly it would be more work than the current proposal. Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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
[PATCH v10.1 7/7] bisect: allow any terms set by user
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.
Signed-off-by: Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr
Signed-off-by: Louis Stuber stub...@ensimag.grenoble-inp.fr
Signed-off-by: Matthieu Moy matthieu@imag.fr
---
Junio C Hamano gits...@pobox.com writes:
Matthieu Moy matthieu@imag.fr writes:
+ git bisect terms term-old term-new
I think this is the other way around.
Indeed.
The test scripts were messed up, I sent too quickly after refactoring.
There was also a small bug: git bisect reset was not reseting the
terms when ran before git bisect start.
My branch https://github.com/moy/git/commits/bisect-terms is up to
date in case the individual resend are too messy. I can also resend
later, but I was too ashamed of my bugs with tests to leave it as
is ;-).
Documentation/git-bisect.txt | 36 +++-
git-bisect.sh| 67 +---
t/t6030-bisect-porcelain.sh | 81
3 files changed, 179 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index abaf462..28f6104 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-new term-old
git bisect skip [(rev|range)...]
git bisect reset [commit]
git bisect visualize
@@ -40,7 +41,7 @@ In fact, `git bisect` can be used to find the commit that
changed
*any* property of your project; e.g., the commit that fixed a bug, or
the commit that caused a benchmark's performance to improve. To
support this more general usage, the terms old and new can be used
-in place of good and bad. See
+in place of good and bad, or you can choose your own terms. See
section Alternate terms below for more information.
Basic bisect commands: start, bad, good
@@ -157,6 +158,31 @@ git bisect new [rev...]
to indicate that it was after.
+If you would like to use your own terms instead of bad/good or
+new/old, you can choose any names you like by typing
+
+
+git bisect terms term-new term-old
+
+
+before starting a bisection session. For example, if you are looking
+for a commit that introduced a performance regression, you might use
+
+
+git bisect terms slow fast
+
+
+Or if you are looking for the commit that fixed a bug, you might use
+
+
+git bisect terms fixed broken
+
+
+Only the bisection following the `git bisect terms` will use the
+terms. If you mistyped one of the terms you can do again `git bisect
+terms term-new term-old`, but that is possible only before you
+start the bisection.
+
Bisect visualize
@@ -440,6 +466,14 @@ $ git bisect start
$ git bisect new HEAD# current commit is marked as new
$ git bisect old HEAD~10 # the tenth commit from now is marked as old
++
+or:
+
+$ git bisect terms fixed broken
+$ git bisect start
+$ git bisect fixed
+$ git bisect broken HEAD~10
+
Getting help
diff --git a/git-bisect.sh b/git-bisect.sh
index 5769eaf..cf07a91 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-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]'
LONG_USAGE='git bisect help
print this long help message.
git bisect start [--no-checkout] [bad [good...]] [--] [pathspec...]
@@ -11,6 +11,8 @@ git bisect (bad|new) [rev]
git bisect (good|old) [rev...]
mark rev... known-good revisions/
revisions before change in a given property.
+git bisect terms term-new term-old
+ set up term-new and term-old as terms (default: bad, good)
git bisect skip [(rev|range)...]
mark rev... untestable revisions.
git bisect next
@@ -80,6 +82,16 @@ bisect_start() {
bad_seen=0
eval=''
must_write_terms=0
+ must_log_terms=0
+ if test -s $GIT_DIR/BISECT_TERMS
+ then
+ # We're going to restart from a clean state and the
+ # file will be deleted. Record the old state in
+ # variables and restore it below.
+ must_write_terms=1
+ must_log_terms=1
+ get_terms
+ fi
if test z$(git rev-parse --is-bare-repository) != zfalse
then
mode=--no-checkout
@@
Re: [PATCH v10.1 7/7] bisect: allow any terms set by user
Matthieu Moy matthieu@imag.fr writes: Matthieu Moy matthieu@imag.fr writes: + git bisect terms term-old term-new I think this is the other way around. Indeed. I hate to be saying this, but this is a strong indication that consistency with start $bad $good... must be broken. If the person who has been working on this topic for a few iterations in the past few days cannot get it right, no ordinary user can. With or without a mnemonic hint N comes before O, so does B before G. Of course we cannot just say git bisect terms old new. That would only invite eh, I do not remember, which between terms and start take the old one first? without helping people. The best I can come up with is to forbid positional arguments to this subcommand and always require them to be given like so: git bisect terms --old=fast --new=slow git bisect terms --new=slow --old=fast We may want to start supporting git bisect start --new=master --old=maint while at it and then gently nudging people to stop using git bisect start master maint by showing depreation notice. -- 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.1 7/7] bisect: allow any terms set by user
On Sat, Jun 27, 2015 at 12:25 AM, Junio C Hamano gits...@pobox.com wrote: Matthieu Moy matthieu@imag.fr writes: Matthieu Moy matthieu@imag.fr writes: + git bisect terms term-old term-new I think this is the other way around. Indeed. I hate to be saying this, but this is a strong indication that consistency with start $bad $good... must be broken. If the person who has been working on this topic for a few iterations in the past few days cannot get it right, no ordinary user can. With or without a mnemonic hint N comes before O, so does B before G. Of course we cannot just say git bisect terms old new. That would only invite eh, I do not remember, which between terms and start take the old one first? without helping people. The best I can come up with is to forbid positional arguments to this subcommand and always require them to be given like so: git bisect terms --old=fast --new=slow git bisect terms --new=slow --old=fast If we don't want to support positional arguments, then I would suggest supporting first the following instead: git bisect terms --name-good=fast --name-bad=slow git bisect terms --name-bad=slow --name-good=fast This would make the interface consistent with the code. Of course we could also accept --name-old and --name-new as synonyms for --name-good and --name-bad. We may want to start supporting git bisect start --new=master --old=maint Maybe we could also support: git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master -- 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.1 7/7] bisect: allow any terms set by user
On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder christian.cou...@gmail.com wrote: If we don't want to support positional arguments, then I would suggest supporting first the following instead: git bisect terms --name-good=fast --name-bad=slow git bisect terms --name-bad=slow --name-good=fast This would make the interface consistent with the code. Which somewhat defeats the point of introducing old and new, though. The terms support is for people who feel that good/bad would be too confusing for the particular bisect session (e.g. because they are hunting for a fix). We may want to start supporting git bisect start --new=master --old=maint Maybe we could also support: git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master The same comment for the token after --name-, but allowing the terms to be set at start could be a type-saver. With need for added --name- prefix (worse, twice), I am not sure if it would be seen as a useful type-saver, though. 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.1 7/7] bisect: allow any terms set by user
On Sat, Jun 27, 2015 at 6:25 AM, Junio C Hamano gits...@pobox.com wrote: On Fri, Jun 26, 2015 at 9:10 PM, Christian Couder christian.cou...@gmail.com wrote: If we don't want to support positional arguments, then I would suggest supporting first the following instead: git bisect terms --name-good=fast --name-bad=slow git bisect terms --name-bad=slow --name-good=fast This would make the interface consistent with the code. Which somewhat defeats the point of introducing old and new, though. The terms support is for people who feel that good/bad would be too confusing for the particular bisect session (e.g. because they are hunting for a fix). Well if --name-old and --name-new are also available as synonyms, it would not be too bad I think. People could use the option names that fit their mental model or their use case better. We may want to start supporting git bisect start --new=master --old=maint Maybe we could also support: git bisect start --name-good=fast --name-bad=slow --fast=maint --slow=master The same comment for the token after --name-, but allowing the terms to be set at start could be a type-saver. With need for added --name- prefix (worse, twice), I am not sure if it would be seen as a useful type-saver, though. At least people don't need to remember if they have to use git bisect term before or after starting :-) -- 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
