Hi Pranit,
this one is hard to review because you do two or three commits in one here.
I think the first commit should be the exit()->return conversion, the
second commit is next and autonext, and the third commit is the pretty
trivial bisect_start commit ;) However, you did it this way and it's
always a hassle to split commit, so I don't really care...
However, I was reviewing this superficially, to be honest. This mail
skips the next and autonext part.
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> diff --git a/bisect.c b/bisect.c
> index 45d598d..7c97e85 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -843,16 +878,21 @@ static int check_ancestors(const char *prefix)
> *
> * If that's not the case, we need to check the merge bases.
> * If a merge base must be tested by the user, its source code will be
> - * checked out to be tested by the user and we will exit.
> + * checked out to be tested by the user and we will return.
> */
> -static void check_good_are_ancestors_of_bad(const char *prefix, int
> no_checkout)
> +static int check_good_are_ancestors_of_bad(const char *prefix, int
> no_checkout)
> {
> char *filename = git_pathdup("BISECT_ANCESTORS_OK");
> struct stat st;
> - int fd;
> + int fd, res = 0;
>
> + /*
> + * We don't want to clean the bisection state
> + * as we need to get back to where we started
> + * by using `git bisect reset`.
> + */
> if (!current_bad_oid)
> - die(_("a %s revision is needed"), term_bad);
> + error(_("a %s revision is needed"), term_bad);
Only error() or return error()?
> @@ -873,8 +916,11 @@ static void check_good_are_ancestors_of_bad(const char
> *prefix, int no_checkout)
> filename);
> else
> close(fd);
> +
> + goto done;
> done:
I never understand why you do this. In case of adding a "fail" label
(and fail code like "res = -1;") between "goto done" and "done:", it's
fine... but without one this is just a nop.
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1d3e17f..fcd7574 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -427,15 +560,24 @@ static int bisect_start(struct bisect_terms *terms, int
> no_checkout,
> no_checkout = 1;
>
> for (i = 0; i < argc; i++) {
> - if (!strcmp(argv[i], "--")) {
> + const char *arg;
> + if (starts_with(argv[i], "'"))
> + arg = sq_dequote(xstrdup(argv[i]));
> + else
> + arg = argv[i];
One is xstrdup'ed, one is not, so there'll be a leak somewhere, and it's
an inconsistent leak... I guess it's a bad idea to do it this way ;)
(Also below.)
> @@ -443,24 +585,31 @@ static int bisect_start(struct bisect_terms *terms, int
> no_checkout,
> no_checkout = 1;
> } else if (!strcmp(arg, "--term-good") ||
> !strcmp(arg, "--term-old")) {
> + if (starts_with(argv[++i], "'"))
> + terms->term_good = sq_dequote(xstrdup(argv[i]));
> + else
> + terms->term_good = xstrdup(argv[i]);
> must_write_terms = 1;
> - terms->term_good = xstrdup(argv[++i]);
> } else if (skip_prefix(arg, "--term-good=", &arg)) {
> must_write_terms = 1;
> - terms->term_good = xstrdup(arg);
> + terms->term_good = arg;
No ;) (See my other comments (to other patches) for the "terms" leaks.)
[This repeats several times below.]
> diff --git a/git-bisect.sh b/git-bisect.sh
> index f0896b3..d574c44 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -109,6 +88,7 @@ bisect_skip() {
> bisect_state() {
> bisect_autostart
> state=$1
> + get_terms
> git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD ||
> exit
> get_terms
> case "$#,$state" in
I can't say if this change is right or wrong. It looks right, but: How
does this relate to the other changes? Is this the right patch for it?
~Stephan
