Hey Stephan,

Extremely sorry I just forgot to reply to this email before. I was
preparing from the next iteration when I saw this.

On Mon, Nov 21, 2016 at 1:31 AM, Stephan Beyer [off-list ref] wrote:

Hi Pranit,

this one is hard to review because you do two or three commits in one here.
I think the first commit should be the exit()->return conversion, the
second commit is next and autonext, and the third commit is the pretty
trivial bisect_start commit ;) However, you did it this way and it's
always a hassle to split commit, so I don't really care...

I had confusion about how to split the commits, but then I then
decided to dump it all together so that it compiles (I was finding it
difficult to split into meaningful parts which also compiled).

However, I was reviewing this superficially, to be honest. This mail
skips the next and autonext part.

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

quoted

diff --git a/bisect.c b/bisect.c
index 45d598d..7c97e85 100644
--- a/bisect.c
+++ b/bisect.c

@@ -843,16 +878,21 @@ static int check_ancestors(const char *prefix)
  *
  * If that's not the case, we need to check the merge bases.
  * If a merge base must be tested by the user, its source code will be
- * checked out to be tested by the user and we will exit.
+ * checked out to be tested by the user and we will return.
  */
-static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
+static int check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
 {
      char *filename = git_pathdup("BISECT_ANCESTORS_OK");
      struct stat st;
-     int fd;
+     int fd, res = 0;

+     /*
+      * We don't want to clean the bisection state
+      * as we need to get back to where we started
+      * by using `git bisect reset`.
+      */
      if (!current_bad_oid)
-             die(_("a %s revision is needed"), term_bad);
+             error(_("a %s revision is needed"), term_bad);

Only error() or return error()?

It should be return error(). Thanks for pointing it out! :)

quoted

@@ -873,8 +916,11 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
                            filename);
      else
              close(fd);
+
+     goto done;
  done:

I never understand why you do this. In case of adding a "fail" label
(and fail code like "res = -1;") between "goto done" and "done:", it's
fine... but without one this is just a nop.

I will just remove that line.

quoted

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1d3e17f..fcd7574 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c

@@ -427,15 +560,24 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
              no_checkout = 1;

      for (i = 0; i < argc; i++) {
-             if (!strcmp(argv[i], "--")) {
+             const char *arg;
+             if (starts_with(argv[i], "'"))
+                     arg = sq_dequote(xstrdup(argv[i]));
+             else
+                     arg = argv[i];

One is xstrdup'ed, one is not, so there'll be a leak somewhere, and it's
an inconsistent leak... I guess it's a bad idea to do it this way ;)
(Also below.)

Yes. I will use xstrdup() and it does leak.

quoted

@@ -443,24 +585,31 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
                      no_checkout = 1;
              } else if (!strcmp(arg, "--term-good") ||
                       !strcmp(arg, "--term-old")) {
+                     if (starts_with(argv[++i], "'"))
+                             terms->term_good = sq_dequote(xstrdup(argv[i]));
+                     else
+                             terms->term_good = xstrdup(argv[i]);
                      must_write_terms = 1;
-                     terms->term_good = xstrdup(argv[++i]);
              } else if (skip_prefix(arg, "--term-good=", &arg)) {
                      must_write_terms = 1;
-                     terms->term_good = xstrdup(arg);
+                     terms->term_good = arg;

No ;) (See my other comments (to other patches) for the "terms" leaks.)

Yes I have addressed this issue.

[This repeats several times below.]

quoted

diff --git a/git-bisect.sh b/git-bisect.sh
index f0896b3..d574c44 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh

@@ -109,6 +88,7 @@ bisect_skip() {
 bisect_state() {
      bisect_autostart
      state=$1
+     get_terms
      git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
      get_terms
      case "$#,$state" in

I can't say if this change is right or wrong. It looks right, but: How
does this relate to the other changes? Is this the right patch for it?

This line is because of the following:

 * TERM_BAD and TERM_GOOD are global but in the coming patch they
would be removed as global variables.

 * To compensate for that, I will write out the state of TERM_BAD and
TERM_GOOD every time it is updated in the file BISECT_TERMS.

 * So we will be reading it from there.

 * It is quite possible that this is completely redundant as for now
but I really don't care to check for each case because I have removed
the shell function bisect_state() afterwards and then this line won't
create a problem there because we are using `struct bisect_terms`
there.