Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
That is very different from ENOENT, which is an expected error when you are not using a customized terms. But in the current state, we are going to create bisect_terms even if the bisection is in bad/good mode. Should we differentiate the erors then ? and should we abort the bisection instead of doing it in bad/good mode by default ? -- 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 3/4] bisect: simplify the add of new bisect terms
But ENOENT is not a normal case at all. Should we not treat it the same way as other fopen() errors ? (either going on with default case or returning an error) Would : if (!fp) { die(could not read file '%s': %s, filename, strerror(errno)); } else { be 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 3/4] bisect: simplify the add of new bisect terms
Louis-Alexandre Stuber stub...@ensimag.grenoble-inp.fr writes: That is very different from ENOENT, which is an expected error when you are not using a customized terms. But in the current state, we are going to create bisect_terms even if the bisection is in bad/good mode. Which means that in normal cases, you'll either succeed to open it, or get ENOENT. We're talking about unexcepted cases (you don't have permission to read it because it's not your file, because you messed up with a chmod, or whatever reason). I don't expect a tool to consider that an unreadable file is equivalent to no file at all. If something's wrong, as a user, I expect a helpful message telling me what's wrong, to give me a clue on how to solve 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 3/4] bisect: simplify the add of new bisect terms
Junio C Hamano gits...@pobox.com writes: Matthieu Moy matthieu@grenoble-inp.fr writes: Louis-Alexandre Stuber stub...@ensimag.grenoble-inp.fr writes: That is very different from ENOENT, which is an expected error when you are not using a customized terms. But in the current state, we are going to create bisect_terms even if the bisection is in bad/good mode. Which means that in normal cases, you'll either succeed to open it, or get ENOENT. We're talking about unexcepted cases (you don't have permission to read it because it's not your file, because you messed up with a chmod, or whatever reason). I think both I and you misunderstood what they wanted to do, which is to write out good and bad into terms file even though these are not customized, and then always read from terms file to learn what words are used for good and bad. Yes, indeed. But I do not think it is a good idea to penalize the normal case by writing the terms file and reading them back from it when the user is bisecting with good/bad in the first place, so No strong opinion on that, but creating one file doesn't cost much, and one advantage of writing it unconditionally is that it unifies bad/good and old/new more in the code. Just the creation of BISECT_TERMS becomes a special-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 3/4] bisect: simplify the add of new bisect terms
Louis-Alexandre Stuber stub...@ensimag.grenoble-inp.fr writes: But ENOENT is not a normal case at all. Should we not treat it the same way as other fopen() errors ? (either going on with default case or returning an error) Would : if (!fp) { die(could not read file '%s': %s, filename, strerror(errno)); } else { be ok ? That would be much better than what we had in the patch, which did not look like an error at all: + FILE *fp = fopen(filename, r); + + if (!fp) { + name_bad = bad; + name_good = good; -- 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 3/4] bisect: simplify the add of new bisect terms
Matthieu Moy matthieu@grenoble-inp.fr writes: Junio C Hamano gits...@pobox.com writes: But I do not think it is a good idea to penalize the normal case by writing the terms file and reading them back from it when the user is bisecting with good/bad in the first place, so No strong opinion on that, but creating one file doesn't cost much, and one advantage of writing it unconditionally is that it unifies bad/good and old/new more in the code. Just the creation of BISECT_TERMS becomes a special-case. You have to handle that case anyway when I start bisection, which is going to take very long and I had to leave for the day to come back the next day to continue, and during the night the sysadmin updates the installed version of Git. -- 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 3/4] bisect: simplify the add of new bisect terms
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: +get_terms () { +if test -s $GIT_DIR/BISECT_TERMS +then +NAME_BAD=$(sed -n 1p $GIT_DIR/BISECT_TERMS) +NAME_GOOD=$(sed -n 2p $GIT_DIR/BISECT_TERMS) It is sad that we need to open the file twice. Can't we do something using read perhaps? The cost of it is quite low and we see directly what we meant. We didn't found a pretty way to read two lines with read. Should be stg like: { read good read bad } $GIT_DIR/BISECT_TERMS -- 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 3/4] bisect: simplify the add of new bisect terms
Matthieu Moy matthieu@grenoble-inp.fr writes: Louis-Alexandre Stuber stub...@ensimag.grenoble-inp.fr writes: That is very different from ENOENT, which is an expected error when you are not using a customized terms. But in the current state, we are going to create bisect_terms even if the bisection is in bad/good mode. Which means that in normal cases, you'll either succeed to open it, or get ENOENT. We're talking about unexcepted cases (you don't have permission to read it because it's not your file, because you messed up with a chmod, or whatever reason). I think both I and you misunderstood what they wanted to do, which is to write out good and bad into terms file even though these are not customized, and then always read from terms file to learn what words are used for good and bad. So from _that_ point of view, ENOENT is an error just like others. But I do not think it is a good idea to penalize the normal case by writing the terms file and reading them back from it when the user is bisecting with good/bad in the first place, so -- 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 3/4] bisect: simplify the add of new bisect terms
On Tue, Jun 9, 2015 at 9:01 AM, Matthieu Moy matthieu@grenoble-inp.fr wrote: Subject: Re: [PATCH 3/4] bisect: simplify the add of new bisect terms s/add/addition/ Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: +static const char *name_bad; +static const char *name_good; Same remark as PATCH 2. As for patch 2, I think name_bad and name_good are better than name_new and name_old. [...] + name_bad = bad; + name_good = good; + } else { + strbuf_getline(str, fp, '\n'); + name_bad = strbuf_detach(str, NULL); + strbuf_getline(str, fp, '\n'); + name_good = strbuf_detach(str, NULL); + } I would have kept just name_bad = bad; name_good = good; in this patch, and introduce BISECT_TERMS in a separate one. Yeah I agree that it is more logical to have first a patch that does on bisect.c the same thing as patch 2 does on git-bisect.sh. For example the patch series could be for now: 1) bisect: typo fix 2) bisect: replace hardcoded bad|good with variables 3) git-bisect: replace hardcoded bad|good with variables 4) bisect: simplify adding new terms 5) bisect: add old/new terms -- 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 3/4] bisect: simplify the add of new bisect terms
Subject: Re: [PATCH 3/4] bisect: simplify the add of new bisect terms s/add/addition/ Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: +static const char *name_bad; +static const char *name_good; Same remark as PATCH 2. } else if (starts_with(refname, good-)) { Did you forget this one? - - fprintf(stderr, The merge base %s is bad.\n - This means the bug has been fixed - between %s and [%s].\n, - bad_hex, bad_hex, good_hex); - + if (!strcmp(name_bad, bad)) { + fprintf(stderr, The merge base %s is bad.\n + This means the bug has been fixed + between %s and [%s].\n, + bad_hex, bad_hex, good_hex); + } You need an else here. Maybe it comes later, but as a reviewer, I want to check that you did not forget it now (because I don't trust myself to remember that it must be added later). @@ -890,6 +894,31 @@ static void show_diff_tree(const char *prefix, struct commit *commit) } /* + * The terms used for this bisect session are stocked in + * BISECT_TERMS: it can be bad/good or new/old. + * We read them and stock them to adapt the messages s/stock/store/ (two instances) + * accordingly. Default is bad/good. + */ +void read_bisect_terms(void) +{ + struct strbuf str = STRBUF_INIT; + const char *filename = git_path(BISECT_TERMS); + FILE *fp = fopen(filename, r); + + if (!fp) { I think you would want to error out if errno is not ENOENT. + name_bad = bad; + name_good = good; + } else { + strbuf_getline(str, fp, '\n'); + name_bad = strbuf_detach(str, NULL); + strbuf_getline(str, fp, '\n'); + name_good = strbuf_detach(str, NULL); + } I would have kept just name_bad = bad; name_good = good; in this patch, and introduce BISECT_TERMS in a separate one. --- a/git-bisect.sh +++ b/git-bisect.sh @@ -77,6 +77,7 @@ bisect_start() { orig_args=$(git rev-parse --sq-quote $@) bad_seen=0 eval='' + start_bad_good=0 if test z$(git rev-parse --is-bare-repository) != zfalse then mode=--no-checkout @@ -101,6 +102,9 @@ bisect_start() { die $(eval_gettext '\$arg' does not appear to be a valid revision) break } + + start_bad_good=1 + Why do you need this variable? It seems to me that you are hardcoding once more that terms can be either good/bad or old/new, which you tried to eliminate from the previous round. + if test $start_bad_good -eq 1 -a ! -s $GIT_DIR/BISECT_TERMS Avoid test -a (not strictly POSIX, and sometimes ambiguous). Use test ... test ... instead. + then + echo $NAME_BAD $GIT_DIR/BISECT_TERMS + echo $NAME_GOOD $GIT_DIR/BISECT_TERMS + fi Why not do this unconditionnally? Whether terms are good/bad or old/new, you can write them to BISECT_TERMS. - gettextln You need to give me at least one good and one bad revision. -(You can use \git bisect bad\ and \git bisect good\ for that.) 2 + gettextln You need to give me at least one $(bisect_voc bad) and one $(bisect_voc good) revision. +(You can use \git bisect $(bisect_voc bad)\ and \git bisect $(bisect_voc good)\ for that.) 2 I suspect you broke the i18n here too. +get_terms () { + if test -s $GIT_DIR/BISECT_TERMS + then + NAME_BAD=$(sed -n 1p $GIT_DIR/BISECT_TERMS) + NAME_GOOD=$(sed -n 2p $GIT_DIR/BISECT_TERMS) + fi +} + +check_and_set_terms () { + cmd=$1 + case $cmd in + bad|good) + if test -s $GIT_DIR/BISECT_TERMS -a $cmd != $NAME_BAD -a $cmd != $NAME_GOOD + then + die $(eval_gettext Invalid command : you're currently in a \$NAME_BAD/\$NAME_GOOD bisect.) No space before : + fi + case $cmd in + bad|good) + if test ! -s $GIT_DIR/BISECT_TERMS + then + echo bad $GIT_DIR/BISECT_TERMS + echo good $GIT_DIR/BISECT_TERMS + fi + NAME_BAD=bad + NAME_GOOD=good ;; + esac ;; + esac +} + +bisect_voc () { + case $1 in + bad) echo bad ;; + good) echo good ;; + esac +} It's weird to have these hardcoded bad/good when you already have BISECT_TERMS in the same patch. -- 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
Re: [PATCH 3/4] bisect: simplify the add of new bisect terms
Hi, Thanks for the review, Junio C Hamano gits...@pobox.com writes: /* + * The terms used for this bisect session are stocked in + * BISECT_TERMS: it can be bad/good or new/old. + * We read them and stock them to adapt the messages + * accordingly. Default is bad/good. + */ s/stock/store/ perhaps? I think the idea is not to have this file in the default case so that absence of it would mean you would be looking for a transition from (older) good to (more recent) bad. Yes it is nice but a bisect_terms file is a very useful tool for verifications at a little cost. For instance, if the user type this: git bisect start git bisect bad HEAD git bisect old HEAD~10 We created the bisect_terms file after the bad then the error is directly detected when the user type git bisect old. And I think having we should limit the impact of this good/bad default case as we would prefer old/new. +void read_bisect_terms(void) +{ +struct strbuf str = STRBUF_INIT; +const char *filename = git_path(BISECT_TERMS); +FILE *fp = fopen(filename, r); + +if (!fp) { We might want to see why fopen() failed here. If it is because the file did not exist, great. But otherwise? Should we display a specific message and cancel the last command? diff --git a/git-bisect.sh b/git-bisect.sh index 1f16aaf..529bb43 100644 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -77,6 +77,7 @@ bisect_start() { orig_args=$(git rev-parse --sq-quote $@) bad_seen=0 eval='' +start_bad_good=0 if test z$(git rev-parse --is-bare-repository) != zfalse then mode=--no-checkout @@ -101,6 +102,9 @@ bisect_start() { die $(eval_gettext '\$arg' does not appear to be a valid revision) break } + +start_bad_good=1 + It is unclear what this variable means, or what it means to have zero or one as its value. This has been done by our elders and we kept because it was useful. We are however trying to remove it. It is in case of a 'git bisect start bad_rev good_rev', our function that creates the bisect_terms is not called. Thus we need to do it manually in the code. +get_terms () { +if test -s $GIT_DIR/BISECT_TERMS +then +NAME_BAD=$(sed -n 1p $GIT_DIR/BISECT_TERMS) +NAME_GOOD=$(sed -n 2p $GIT_DIR/BISECT_TERMS) It is sad that we need to open the file twice. Can't we do something using read perhaps? The cost of it is quite low and we see directly what we meant. We didn't found a pretty way to read two lines with read. Don't we want to make sure these two names are sensible? We do not want an empty-string, for example. I suspect you do not want to take anything that check-ref-format does not like. Same comment applies to the C code. Yes, for now only bad/good/old/new are allowed. But in the future it will be necessary. +bisect_voc () { +case $1 in +bad) echo bad ;; +good) echo good ;; +esac +} What is voc? What if $1 is neither bad/good? Did you mean to translate 'bad' to $NAME_BAD and 'good' to $NAME_GOOD? voc stands for vocabulary. This fonction is mainly used after to display all the builtins possibility. It is only called internally and if the argument is bad it returns the synonyms of bad (bad|new in the next patch). -- 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 3/4] bisect: simplify the add of new bisect terms
Matthieu Moy matthieu@grenoble-inp.fr wrote: I think you would want to error out if errno is not ENOENT. Junio C Hamano jch2...@gmail.com wrote: We might want to see why fopen() failed here. If it is because the file did not exist, great. But otherwise? This file is always supposed to exist when the function is called unless the user has manually deleted it (or a bug in our programs). Maybe we should just return an error if the file cannot be opened, regardless the reason. We kept it like it is, with the default case, because it was what our elders did, and that aborting because of BISECT_TERMS is not good for backward compatibility. But then, in both cases, there is no reason to differenciate ENOENT. Is that right ? -- 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 3/4] bisect: simplify the add of new bisect terms
Louis-Alexandre Stuber stub...@ensimag.grenoble-inp.fr writes: Matthieu Moy matthieu@grenoble-inp.fr wrote: I think you would want to error out if errno is not ENOENT. Junio C Hamano jch2...@gmail.com wrote: We might want to see why fopen() failed here. If it is because the file did not exist, great. But otherwise? This file is always supposed to exist when the function is called unless the user has manually deleted it (or a bug in our programs). Maybe we should just return an error if the file cannot be opened, regardless the reason. We kept it like it is, with the default case, because it was what our elders did, and that aborting because of BISECT_TERMS is not good for backward compatibility. But then, in both cases, there is no reason to differenciate ENOENT. Is that right ? One thing that immediately comes to mind is when you (perhaps accidentally, or perhaps you were asked to) cd into somebody else's repository and take a look or help, the file exists but cannot be read by you and you get EPERM. That is very different from ENOENT, which is an expected error when you are not using a customized terms. -- 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 3/4] bisect: simplify the add of new bisect terms
Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr writes: We create a file BISECT_TERMS in the repository .git to be read during a bisection. The fonctions to be changed if we add new terms are quite few. In git-bisect.sh : check_and_set_terms bisect_voc In bisect.c : handle_bad_merge_base Signed-off-by: Antoine Delaite antoine.dela...@ensimag.grenoble-inp.fr Signed-off-by: Louis Stuber stub...@ensimag.grenoble-inp.fr Signed-off-by: Valentin Duperray valentin.duper...@ensimag.imag.fr Signed-off-by: Franck Jonas franck.jo...@ensimag.imag.fr Signed-off-by: Lucien Kong lucien.k...@ensimag.imag.fr Signed-off-by: Thomas Nguy thomas.n...@ensimag.imag.fr Signed-off-by: Huynh Khoi Nguyen Nguyen huynh-khoi-nguyen.ngu...@ensimag.imag.fr Signed-off-by: Matthieu Moy matthieu@grenoble-inp.fr --- This step seems very straight-forward and makes sense from a cursory look. /* + * The terms used for this bisect session are stocked in + * BISECT_TERMS: it can be bad/good or new/old. + * We read them and stock them to adapt the messages + * accordingly. Default is bad/good. + */ s/stock/store/ perhaps? I think the idea is not to have this file in the default case so that absence of it would mean you would be looking for a transition from (older) good to (more recent) bad. +void read_bisect_terms(void) +{ + struct strbuf str = STRBUF_INIT; + const char *filename = git_path(BISECT_TERMS); + FILE *fp = fopen(filename, r); + + if (!fp) { We might want to see why fopen() failed here. If it is because the file did not exist, great. But otherwise? diff --git a/git-bisect.sh b/git-bisect.sh index 1f16aaf..529bb43 100644 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -77,6 +77,7 @@ bisect_start() { orig_args=$(git rev-parse --sq-quote $@) bad_seen=0 eval='' + start_bad_good=0 if test z$(git rev-parse --is-bare-repository) != zfalse then mode=--no-checkout @@ -101,6 +102,9 @@ bisect_start() { die $(eval_gettext '\$arg' does not appear to be a valid revision) break } + + start_bad_good=1 + It is unclear what this variable means, or what it means to have zero or one as its value. case $bad_seen in 0) state='bad' ; bad_seen=1 ;; *) state='good' ;; @@ -172,6 +176,11 @@ bisect_start() { } git rev-parse --sq-quote $@ $GIT_DIR/BISECT_NAMES eval $eval true + if test $start_bad_good -eq 1 -a ! -s $GIT_DIR/BISECT_TERMS Avoid test condition1 -a condition2 (or -o). +get_terms () { + if test -s $GIT_DIR/BISECT_TERMS + then + NAME_BAD=$(sed -n 1p $GIT_DIR/BISECT_TERMS) + NAME_GOOD=$(sed -n 2p $GIT_DIR/BISECT_TERMS) It is sad that we need to open the file twice. Can't we do something using read perhaps? Don't we want to make sure these two names are sensible? We do not want an empty-string, for example. I suspect you do not want to take anything that check-ref-format does not like. Same comment applies to the C code. +bisect_voc () { + case $1 in + bad) echo bad ;; + good) echo good ;; + esac +} What is voc? What if $1 is neither bad/good? Did you mean to translate 'bad' to $NAME_BAD and 'good' to $NAME_GOOD? -- 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