Re: [PATCH v15 15/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
- From
Stephan Beyer <s-beyer@gmx.net>
- Date
- Nov 20, 2016, 20:01 UTC
- Message-ID
- <8bd0682f-e35e-a50e-24a9-fd3a53454ed4@gmx.net>
- In-Reply-To
- <01020157c38b1af0-5d688c2e-868d-4d8c-a8fd-9a675f7f01da-000000@eu-west-1.amazonses.com>
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:
Show 27 quoted lines
> 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()?
Show 7 quoted lines
> @@ -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.
Show 14 quoted lines
> 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.)
Show 14 quoted lines
> @@ -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.]
Show 12 quoted lines
> 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" inI 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