Hi,
On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> Reimplement the `bisect_state` shell function in C and also add a
> subcommand `--bisect-state` to `git-bisect--helper` to call it from
> git-bisect.sh .
>
> Using `--bisect-state` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
>
> `bisect_head` is called from `bisect_state` thus its not required to
> introduce another subcommand.
Missing comma before "thus", and "it is" (or "it's") instead of "its" :)
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1767916..1481c6d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -784,6 +786,79 @@ static int bisect_autostart(struct bisect_terms *terms)
> return 0;
> }
>
> +static char *bisect_head(void)
> +{
> + if (is_empty_or_missing_file(git_path_bisect_head()))
> + return "HEAD";
> + else
> + return "BISECT_HEAD";
> +}
This is very shellish.
In C I'd expect something like
static int bisect_head_sha1(unsigned char *sha)
{
int res;
if (is_empty_or_missing_file(git_path_bisect_head()))
res = get_sha1("HEAD", sha);
else
res = get_sha1("BISECT_HEAD", sha);
if (res)
return error(_("Could not find BISECT_HEAD or HEAD."));
return 0;
}
> +
> +static int bisect_state(struct bisect_terms *terms, const char **argv,
> + int argc)
> +{
> + const char *state = argv[0];
> +
> + get_terms(terms);
> + if (check_and_set_terms(terms, state))
> + return -1;
> +
> + if (!argc)
> + die(_("Please call `--bisect-state` with at least one argument"));
I think this check should move to cmd_bisect__helper. There are also the
other argument number checks.
> +
> + if (argc == 1 && one_of(state, terms->term_good,
> + terms->term_bad, "skip", NULL)) {
> + const char *bisected_head = xstrdup(bisect_head());
> + const char *hex[1];
Maybe:
const char *hex;
> + unsigned char sha1[20];
> +
> + if (get_sha1(bisected_head, sha1))
> + die(_("Bad rev input: %s"), bisected_head);
And instead of...
> + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
> + return -1;
> +
> + *hex = xstrdup(sha1_to_hex(sha1));
> + if (check_expected_revs(hex, 1))
> + return -1;
... simply:
hex = xstrdup(sha1_to_hex(sha1));
if (set_state(terms, state, hex)) {
free(hex);
return -1;
}
free(hex);
where:
static int set_state(struct bisect_terms *terms, const char *state,
const char *hex)
{
if (bisect_write(state, hex, terms, 0))
return -1;
if (check_expected_revs(&hex, 1))
return -1;
return 0;
}
> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
> + one_of(state, terms->term_good, "skip", NULL)) {
> + int i;
> + struct string_list hex = STRING_LIST_INIT_DUP;
> +
> + for (i = 1; i < argc; i++) {
> + unsigned char sha1[20];
> +
> + if (get_sha1(argv[i], sha1)) {
> + string_list_clear(&hex, 0);
> + die(_("Bad rev input: %s"), argv[i]);
> + }
> + string_list_append(&hex, sha1_to_hex(sha1));
> + }
> + for (i = 0; i < hex.nr; i++) {
... And replace this:
> + const char **hex_string = (const char **) &hex.items[i].string;
> + if(bisect_write(state, *hex_string, terms, 0)) {
> + string_list_clear(&hex, 0);
> + return -1;
> + }
> + if (check_expected_revs(hex_string, 1)) {
> + string_list_clear(&hex, 0);
> + return -1;
> + }
by:
const char *hex_str = hex.items[i].string;
if (set_state(terms, state, hex_string)) {
string_list_clear(&hex, 0);
return -1;
}
> + }
> + string_list_clear(&hex, 0);
> + return bisect_auto_next(terms, NULL);
> + }
> +
> + if (!strcmp(state, terms->term_bad))
> + die(_("'git bisect %s' can take only one argument."),
> + terms->term_bad);
> +
> + return -1;
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> @@ -823,6 +899,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("verify the next bisection state then find the next bisection state"), BISECT_AUTO_NEXT),
> OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> N_("start the bisection if BISECT_START empty or missing"), BISECT_AUTOSTART),
> + OPT_CMDMODE(0, "bisect-state", &cmdmode,
> + N_("mark the state of ref (or refs)"), BISECT_STATE),
"mark the state of the given revs"
Note that rev != ref
> @@ -902,6 +980,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> terms.term_bad = "bad";
> res = bisect_autostart(&terms);
> break;
> + case BISECT_STATE:
> + if (argc == 0)
> + die(_("--bisect-state requires at least 1 argument"));
"at least one revision"
> diff --git a/git-bisect.sh b/git-bisect.sh
> index cd56551..a9eebbb 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -61,44 +51,7 @@ bisect_skip() {
> esac
> all="$all $revs"
> done
> - eval bisect_state 'skip' $all
[...deleted lines...]
> + eval git bisect--helper --bisect-state 'skip' $all
I think you don't need "eval" here any longer.
> @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> state="$TERM_GOOD"
> fi
>
> - # We have to use a subshell because "bisect_state" can exit.
> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> + # We have to use a subshell because "--bisect-state" can exit.
> + ( git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" )
The new comment is funny, but you don't need a subshell here any longer.
~Stephan