Re: [PATCH v2 7/7] bisect: allows any terms set by user
Matthieu Moy 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 . 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 4/7] bisect: add the terms old/new
I agree this makes no sense hardcoding old/new once bisect terms is considerred. It would have been a lot better if we had started implementing bisect terms immediatly (but we thought old/new would already be an appreciable step for most of users). -- 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
> 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
Matthieu Moy wrote: > I think you would want to error out if errno is not ENOENT. Junio C Hamano 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 1/2] git-bisect.sh : create a file if the bisection is in old/new mode, named "BISECT_OLDNEWMODE", so it can easily be seen outside the program without having to read BISECT_TERMS. This wil
Thank you for the feedback. We are trying to apply all of your suggestions, but we would prefer to rebase the history before doing some of them (like renaming variables). About the BISECT_OLDNEWMODE file: The current implementation changes almost nothing to revision.c. We thought it was better, even if it needs a new file. The code for bisect uses BISECT_TERMS because 3 states are possible: 'bad/good mode', 'old/new mode', or 'no bisection started' (if BISECT_TERMS doesn't exist). But the other files (like revision.c) don't need all these informations, so we thought it would be good to check if a file exists instead of reusing BISECT_TERMS, which would require reading its content. -- 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
