Re: [PATCH 4/4] bisect--helper: `bisect_reset` shell function in C
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jun 8, 2016, 07:59 UTC
- Message-ID
- <CAPig+cSsMg5HznWGiUsngpHskSDMNhauvVw5jvaJTtEtJBw+ew@mail.gmail.com>
- In-Reply-To
- <20160607205454.22576-4-pranit.bauva@gmail.com>
On Tue, Jun 7, 2016 at 4:54 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
Show 17 quoted lines
> 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@gmail.com> > --- > 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/
Show 5 quoted lines
> +{
> + 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 ...");Show 6 quoted lines
> + 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()?
Show 16 quoted lines
> + 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.
Show 9 quoted lines
> + 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();
> +}