From: Christian Couder <chriscool@tuxfamily.org>
Date: Sat, 30 Jul 2011 13:49:36 GMT
Subject: Re: [PATCH 2/5] bisect: introduce support for --no-checkout=<ref> option.
Message-ID: <201107301549.36999.chriscool@tuxfamily.org>
In-Reply-To: <1312014511-7157-3-git-send-email-jon.seymour@gmail.com>

On Saturday 30 July 2011 10:28:28 Jon Seymour wrote:
> diff --git a/bisect.c b/bisect.c
> index dd7e8ed..e6c99a0 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -24,6 +24,8 @@ struct argv_array {
> 
>  static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
>  static const char *argv_show_branch[] = {"show-branch", NULL, NULL};
> +static const char *argv_update_ref[] = {"update-ref", "--no-deref",
>                                                           NULL, NULL, NULL};
> +static const char *cursor_ref = 0;

Please use "NULL" instead of "0" above.
And is this global really needed? I would prefer if you passed one more 
argument to some functions.

>  /* bits #0-15 in revision.h */
> 
> @@ -714,9 +716,19 @@ static int bisect_checkout(char *bisect_rev_hex)
>  	mark_expected_rev(bisect_rev_hex);
> 
>  	argv_checkout[2] = bisect_rev_hex;
> -	res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
> -	if (res)
> -		exit(res);
> +	if (!cursor_ref) {
> +	  res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
> +	  if (res) {
> +	    exit(res);
> +	  }

Style nit: braces are superfluous, please use:

if (res)
        exit(res); 

> +	} else {
> +	    argv_update_ref[2] = cursor_ref;
> +	    argv_update_ref[3] = bisect_rev_hex;
> +	    res = run_command_v_opt(argv_update_ref, RUN_GIT_CMD);
> +	    if (res) {
> +	      die("update-ref --no-deref %s failed on %s", cursor_ref,
> bisect_rev_hex);
> +	    }

Style nit: braces are superfluous.

> +	}

Thanks,
Christian.