Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
From: Stephan Beyer <hidden>
Date: 2016-11-20 20:01:42
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... 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 hunk ↗ jump to hunk
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()?
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
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.)
quoted hunk ↗ jump to hunk
@@ -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.) [This repeats several times below.]
quoted hunk ↗ jump to hunk
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? ~Stephan