Re: [PATCH v15 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C

Hey Stephan,

On Thu, Nov 17, 2016 at 5:17 AM, Stephan Beyer [off-list ref] wrote:

Hi,

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

quoted

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index d84ba86..c542e8b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c

@@ -123,13 +123,40 @@ static int bisect_reset(const char *commit)
      return bisect_clean_state();
 }

+static int is_expected_rev(const char *expected_hex)
+{
+     struct strbuf actual_hex = STRBUF_INIT;
+     int res = 0;
+     if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
+             strbuf_trim(&actual_hex);
+             res = !strcmp(actual_hex.buf, expected_hex);
+     }
+     strbuf_release(&actual_hex);
+     return res;
+}

I am not sure it does what it should.

I would expect the following behavior from this function:
 - file does not exist (or is "broken") => return 0
 - actual_hex != expected_hex => return 0
 - otherwise return 1

If I am not wrong, the code does the following instead:
 - file does not exist (or is "broken") => return 0
 - actual_hex != expected_hex => return 1
 - otherwise => return 0

Yeah, you are right. I should update this. Thanks for pointing it out.

quoted

+static int check_expected_revs(const char **revs, int rev_nr)
+{
+     int i;
+
+     for (i = 0; i < rev_nr; i++) {
+             if (!is_expected_rev(revs[i])) {
+                     unlink_or_warn(git_path_bisect_ancestors_ok());
+                     unlink_or_warn(git_path_bisect_expected_rev());
+                     return 0;
+             }
+     }
+     return 0;
+}

Here I am not sure what the function *should* do. However, I see that it
basically mimics the behavior of the shell function (assuming
is_expected_rev() is implemented correctly).

I don't understand why the return value is int and not void. To avoid a
"return 0;" line when calling this function?

Initially I thought I would be using the return value but now I
realize that it is meaningless to do so. Using void seems better. :)

quoted

@@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
              if (argc > 1)
                      die(_("--bisect-reset requires either zero or one arguments"));
              return bisect_reset(argc ? argv[0] : NULL);
+     case CHECK_EXPECTED_REVS:
+             return check_expected_revs(argv, argc);

I note that you check the correct number of arguments for some
subcommands and you do not check it for some other subcommands like this
one. (I don't care, I just want to mention it.)

Here we should be able to accept any number of arguments. I think it
would be good to add a non-zero check though just to maintain the
uniformity. Though this is something programmer needs to be careful
about rather than the user.

quoted

      default:
              die("BUG: unknown subcommand '%d'", cmdmode);
      }

~Stephan

Regards,
Pranit Bauva