Re: [PATCH v16 Part II 1/8] bisect--helper: `bisect_reset` shell function in C

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 27/10/17 16:06, Pranit Bauva wrote:
> Reimplement `bisect_reset` shell function in C and add a `--bisect-reset`
> subcommand to `git bisect--helper` to call it from git-bisect.sh .
> 
> Using `bisect_reset` subcommand is a temporary measure to port shell
> functions to C so as to use the existing test suite. As more functions
> are ported, this subcommand would be retired but its implementation will
> be called by some other method.
> 
> Note: --bisect-clean-state subcommand has not been retired as there are
> still a function namely `bisect_start()` which still uses this
> subcommand.
> 
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> 
> ---
[snip]

Sorry for not responding sooner, I've been a bit busy.

Unfortunately, I have only had time to skim the patches, but
I haven't noticed anything too serious.

>  builtin/bisect--helper.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            | 28 ++-------------------------
>  2 files changed, 50 insertions(+), 27 deletions(-)
> 
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 35d2105f941c6..12754448f7b6a 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -3,15 +3,21 @@
>  #include "parse-options.h"
>  #include "bisect.h"
>  #include "refs.h"
> +#include "dir.h"
> +#include "argv-array.h"
> +#include "run-command.h"
>  
>  static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
>  static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
>  static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
> +static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
> +static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
>  
>  static const char * const git_bisect_helper_usage[] = {
>  	N_("git bisect--helper --next-all [--no-checkout]"),
>  	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
>  	N_("git bisect--helper --bisect-clean-state"),
> +	N_("git bisect--helper --bisect-reset [<commit>]"),
>  	NULL
>  };
>  
> @@ -106,13 +112,48 @@ static void check_expected_revs(const char **revs, int rev_nr)
>  	}
>  }
>  
> +static int bisect_reset(const char *commit)
> +{
> +	struct strbuf branch = STRBUF_INIT;
> +
> +	if (!commit) {
> +		if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1)
> +			return !printf(_("We are not bisecting.\n"));

I've no idea what this is about! If printf encounters an error, then
this will be equivalent to !-1. If printf does not encounter an error,
then this will be !<length of output> (whatever that may be, given that
the string is marked for translation).

I would suggest that you don't want to do that. ;-)

ATB,
Ramsay Jones