On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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 and 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.
>
> Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
> ---
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> @@ -118,12 +122,51 @@ int bisect_clean_state(void)
> +int bisect_reset(const char *commit)
s/^/static/
> +{
> + struct strbuf branch = STRBUF_INIT;
> + int status = 0;
> +
> + if (file_size(git_path_bisect_start()) < 1) {
This doesn't even care about the size of the file, only if it
encountered an error while stat()'ing it. Why not just use
file_exists() instead (which you already use elsewhere in this
function)? Alternately, if you're trying to be faithful to the shell
code, then you *do* need to check that the file has non-zero size
before issuing the "not bisecting" diagnostic, so:
if (file_size(git_path_bisect_start()) <= 0)
printf("... not bisecting ...");
A different approach would be to invoke strbuf_read_file()
unconditionally, rather than performing this separate check. If
strbuf_read_file() returns -1, then the file didn't exist or couldn't
be read; if it returns 0, then the file has no content:
if (strbuf_read_file(&branch, ..., 0) <= 0)
printf("... not bisecting ...");
> + printf("We are not bisecting.\n");
> + return 0;
> + }
> +
> + if (!commit) {
> + strbuf_read_file(&branch, git_path_bisect_start(), 0);
Shouldn't you be checking the return value of strbuf_read_file()?
> + strbuf_rtrim(&branch);
> + } else {
> + struct object_id oid;
> + if (get_oid(commit, &oid))
> + return error(_("'%s' is not a valid commit"), commit);
> + strbuf_addf(&branch, "%s", commit);
> + }
> +
> + if (!file_exists(git_path_bisect_head())) {
> + struct argv_array argv = ARGV_ARRAY_INIT;
> + argv_array_pushl(&argv, "checkout", branch.buf, "--", NULL);
> + status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
> + argv_array_clear(&argv);
> + }
> +
> + if (status) {
What's the purpose of having this 'status' conditional outside of the
above !file_exists() conditional, which is the only place that
'status' gets assigned. Likewise, 'status' itself could be declared
within the scope of that conditional block.
> + error(_("Could not check out original HEAD '%s'. "
> + "Try 'git bisect reset <commit>'."), branch.buf);
> + strbuf_release(&branch);
> + return -1;
> + }
> +
> + strbuf_release(&branch);
> + return bisect_clean_state();
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html