Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

Hi Pranit,

On 12/06/2016 11:40 PM, Pranit Bauva wrote:

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

quoted

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

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.

Okay, that's a point.
In that case, you should probably use "return error()" again and the
message (mentioning "--bisect-state") does not make sense when
--bisect-state ceases to exist.

quoted

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.

I was not suggesting a change of behavior, I was suggesting a simple
helper function set_state() to get rid of code duplication above and
some lines below:

quoted

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

See?

quoted

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

The new comment is funny, but you don't need a subshell here any
longer.

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()

For most of the patches, I was commenting on the current state, not on
the big picture.

Still I think that it is better to remove the comment and the subshell
instead of making the comment weird ("yes the builtin can exit, but why
do we need a subshell?" vs "yes the shell function calls exit, so we
need a subshell because we do not want to exit this shell script")

~Stephan