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

Reply via email to