Hey Stephan,

On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer [off-list ref] wrote:

Hi,

On 10/14/2016 04:14 PM, Pranit Bauva wrote:

quoted

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" :)

Sure, will fix.

quoted

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;
}

quoted

+
+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.

Not really. After the whole conversion, the cmdmode will cease to
exists while bisect_state will be called directly, thus it is
important to check it here.

quoted

+
+     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;

quoted

+             unsigned char sha1[20];
+
+             if (get_sha1(bisected_head, sha1))
+                     die(_("Bad rev input: %s"), bisected_head);

And instead of...

quoted

+             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:

Yes I am planning to convert all places with hex rather than the sha1
but not yet, maybe in an another patch series because currently a lot
of things revolve around sha1 and changing its behaviour wouldn't
really be a part of a porting patch series.

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;
}

quoted

+             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:

quoted

+                     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;
                        }

quoted

+             }
+             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

Might have been a typo. Will fix.

quoted

@@ -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"

Okay, that would make it more specific.

quoted

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...]

quoted

+     eval git bisect--helper --bisect-state 'skip' $all

I think you don't need "eval" here any longer.

Yes, I wouldn't

quoted

@@ -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" )

True, but right now I didn't want to modify that part of the source
code to remove the comment. I will remove the comment all together
when I port bisect_run()

Regards,
Pranit Bauva