| Message ID | 20200120143800.900-10-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect to C part 1 | expand |
Hi, On Mon, 20 Jan 2020, Miriam Rubio wrote: > From: Pranit Bauva <pranit.bauva@gmail.com> > > Since we want to get rid of git-bisect.sh it would be necessary to > convert those exit() calls to return statements so that errors can be > reported. > > Emulate try catch in C by converting `exit(<positive-value>)` to > `return <negative-value>`. Follow POSIX conventions to return > <negative-value> to indicate error. > > Turn `exit()` to `return` calls in `check_good_are_ancestors_of_bad()`. > > Code that turns -11(early success code) to 0 from > `check_good_are_ancestors_of_bad()` has been moved to > `cmd_bisect__helper()`. > > Handle the return value in dependent function `bisect_next_all()`. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> > Signed-off-by: Miriam Rubio <mirucam@gmail.com> > --- > bisect.c | 42 ++++++++++++++++++++++++++-------------- > builtin/bisect--helper.c | 12 ++++++++++-- > 2 files changed, 37 insertions(+), 17 deletions(-) > > diff --git a/bisect.c b/bisect.c > index 367258b0dd..2b80597a1d 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -865,9 +865,10 @@ static int check_ancestors(struct repository *r, int rev_nr, > * > * 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(struct repository *r, > + > +static int check_good_are_ancestors_of_bad(struct repository *r, > const char *prefix, > int no_checkout) > { > @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > int fd, rev_nr, res = 0; > struct commit **rev; > > - if (!current_bad_oid) > - die(_("a %s revision is needed"), term_bad); > + /* > + * 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) { > + res = error(_("a %s revision is needed"), term_bad); > + goto done; > + } Why not just return here? Ah, there is a `filename` that was allocated... it is too bad that we have a mailing-list based review, as the hunk context simply cannot be extended in a mail. Personally, I think it would be nicer to split the definition of `filename` from its declaration and move it _after_ this conditional code, so that we can `return` right away. However, there is a more pressing issue than that: `die()` exits with exit code 128, so in keeping with the idea to hand down negative exit codes as return values, should we not assign `res = -128` here? > > /* Check if file BISECT_ANCESTORS_OK exists. */ > if (!stat(filename, &st) && S_ISREG(st.st_mode)) > @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > if (check_ancestors(r, rev_nr, rev, prefix)) > res = check_merge_bases(rev_nr, rev, no_checkout); > free(rev); > - if(res) > - exit(res == -11 ? 0 : -res); > - > - /* Create file BISECT_ANCESTORS_OK. */ > - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); > - if (fd < 0) > - warning_errno(_("could not create file '%s'"), > - filename); > - else > - close(fd); > + > + if (!res) > + { We usually put the `{` on the same line as the `if` condition (like you did in the `if (!current_bad_oid)` line above. The rest looks reasonable. Thank you, Johannes > + /* Create file BISECT_ANCESTORS_OK. */ > + fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); > + if (fd < 0) > + warning_errno(_("could not create file '%s'"), > + filename); > + else > + close(fd); > + } > done: > free(filename); > + return res; > } > > /* > @@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout) > if (read_bisect_refs()) > die(_("reading bisect refs failed")); > > - check_good_are_ancestors_of_bad(r, prefix, no_checkout); > + res = check_good_are_ancestors_of_bad(r, prefix, no_checkout); > + if (res) > + return res; > > bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1); > revs.limited = 1; > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 826fcba2ed..5e0f759d50 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > switch (cmdmode) { > case NEXT_ALL: > - return bisect_next_all(the_repository, prefix, no_checkout); > + res = bisect_next_all(the_repository, prefix, no_checkout); > + break; > case WRITE_TERMS: > if (argc != 2) > return error(_("--write-terms requires two arguments")); > @@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > return error("BUG: unknown subcommand '%d'", cmdmode); > } > free_terms(&terms); > - return !!res; > + /* > + * Handle early success > + * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all > + */ > + if (res == -11) > + res = 0; > + > + return res < 0 ? -res : res; > } > -- > 2.21.1 (Apple Git-122.3) > >
On Mon, Jan 20, 2020 at 11:20 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > On Mon, 20 Jan 2020, Miriam Rubio wrote: > > @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > > int fd, rev_nr, res = 0; > > struct commit **rev; > > > > - if (!current_bad_oid) > > - die(_("a %s revision is needed"), term_bad); > > + /* > > + * 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) { > > + res = error(_("a %s revision is needed"), term_bad); > > + goto done; > > + } > > Why not just return here? Ah, there is a `filename` that was allocated... > it is too bad that we have a mailing-list based review, as the hunk > context simply cannot be extended in a mail. > > Personally, I think it would be nicer to split the definition of > `filename` from its declaration and move it _after_ this conditional code, > so that we can `return` right away. Yeah, I agree. > However, there is a more pressing issue than that: `die()` exits with exit > code 128, so in keeping with the idea to hand down negative exit codes as > return values, should we not assign `res = -128` here? I think it has been ok when converting git-bisect.sh to C to just convert `die(...)` into `return error(...)`. > > /* Check if file BISECT_ANCESTORS_OK exists. */ > > if (!stat(filename, &st) && S_ISREG(st.st_mode)) > > @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > > if (check_ancestors(r, rev_nr, rev, prefix)) > > res = check_merge_bases(rev_nr, rev, no_checkout); > > free(rev); > > - if(res) > > - exit(res == -11 ? 0 : -res); > > - > > - /* Create file BISECT_ANCESTORS_OK. */ > > - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); > > - if (fd < 0) > > - warning_errno(_("could not create file '%s'"), > > - filename); > > - else > > - close(fd); > > + > > + if (!res) > > + { > > We usually put the `{` on the same line as the `if` condition (like you > did in the `if (!current_bad_oid)` line above. > > The rest looks reasonable. Thank you, Thank you for your review, Christian.
Hi, El mar., 21 ene. 2020 a las 7:59, Christian Couder (<christian.couder@gmail.com>) escribió: > > On Mon, Jan 20, 2020 at 11:20 PM Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: > > > > On Mon, 20 Jan 2020, Miriam Rubio wrote: > > > > @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > > > int fd, rev_nr, res = 0; > > > struct commit **rev; > > > > > > - if (!current_bad_oid) > > > - die(_("a %s revision is needed"), term_bad); > > > + /* > > > + * 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) { > > > + res = error(_("a %s revision is needed"), term_bad); > > > + goto done; > > > + } > > > > Why not just return here? Ah, there is a `filename` that was allocated... > > it is too bad that we have a mailing-list based review, as the hunk > > context simply cannot be extended in a mail. > > > > Personally, I think it would be nicer to split the definition of > > `filename` from its declaration and move it _after_ this conditional code, > > so that we can `return` right away. > > Yeah, I agree. Ok. Noted. > > > However, there is a more pressing issue than that: `die()` exits with exit > > code 128, so in keeping with the idea to hand down negative exit codes as > > return values, should we not assign `res = -128` here? > > I think it has been ok when converting git-bisect.sh to C to just > convert `die(...)` into `return error(...)`. > > > > /* Check if file BISECT_ANCESTORS_OK exists. */ > > > if (!stat(filename, &st) && S_ISREG(st.st_mode)) > > > @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > > > if (check_ancestors(r, rev_nr, rev, prefix)) > > > res = check_merge_bases(rev_nr, rev, no_checkout); > > > free(rev); > > > - if(res) > > > - exit(res == -11 ? 0 : -res); > > > - > > > - /* Create file BISECT_ANCESTORS_OK. */ > > > - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); > > > - if (fd < 0) > > > - warning_errno(_("could not create file '%s'"), > > > - filename); > > > - else > > > - close(fd); > > > + > > > + if (!res) > > > + { > > > > We usually put the `{` on the same line as the `if` condition (like you > > did in the `if (!current_bad_oid)` line above. Ok. I will change that. > > > > The rest looks reasonable. Thank you, Great! Thank you for your review! > > Thank you for your review, > Christian.
diff --git a/bisect.c b/bisect.c index 367258b0dd..2b80597a1d 100644 --- a/bisect.c +++ b/bisect.c @@ -865,9 +865,10 @@ static int check_ancestors(struct repository *r, int rev_nr, * * 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(struct repository *r, + +static int check_good_are_ancestors_of_bad(struct repository *r, const char *prefix, int no_checkout) { @@ -876,8 +877,15 @@ static void check_good_are_ancestors_of_bad(struct repository *r, int fd, rev_nr, res = 0; struct commit **rev; - if (!current_bad_oid) - die(_("a %s revision is needed"), term_bad); + /* + * 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) { + res = error(_("a %s revision is needed"), term_bad); + goto done; + } /* Check if file BISECT_ANCESTORS_OK exists. */ if (!stat(filename, &st) && S_ISREG(st.st_mode)) @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r, if (check_ancestors(r, rev_nr, rev, prefix)) res = check_merge_bases(rev_nr, rev, no_checkout); free(rev); - if(res) - exit(res == -11 ? 0 : -res); - - /* Create file BISECT_ANCESTORS_OK. */ - fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); - if (fd < 0) - warning_errno(_("could not create file '%s'"), - filename); - else - close(fd); + + if (!res) + { + /* Create file BISECT_ANCESTORS_OK. */ + fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); + if (fd < 0) + warning_errno(_("could not create file '%s'"), + filename); + else + close(fd); + } done: free(filename); + return res; } /* @@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout) if (read_bisect_refs()) die(_("reading bisect refs failed")); - check_good_are_ancestors_of_bad(r, prefix, no_checkout); + res = check_good_are_ancestors_of_bad(r, prefix, no_checkout); + if (res) + return res; bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1); revs.limited = 1; diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 826fcba2ed..5e0f759d50 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) switch (cmdmode) { case NEXT_ALL: - return bisect_next_all(the_repository, prefix, no_checkout); + res = bisect_next_all(the_repository, prefix, no_checkout); + break; case WRITE_TERMS: if (argc != 2) return error(_("--write-terms requires two arguments")); @@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) return error("BUG: unknown subcommand '%d'", cmdmode); } free_terms(&terms); - return !!res; + /* + * Handle early success + * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all + */ + if (res == -11) + res = 0; + + return res < 0 ? -res : res; }