| Message ID | 20200120143800.900-9-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_merge_bases()`. > > In `check_merge_bases()` there is an early success special case, > so we have introduced special error code `-11` which indicates early > success. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`. > > Handle the return value in dependent function `check_good_are_ancestors_of_bad()`. This is a lot of repeated text from earlier commit messages. It might make sense to condense it a bit, and while at it, to remove sentences that essentially repeat what the diff says. > > 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 | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/bisect.c b/bisect.c > index 385afaf875..367258b0dd 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb) > * "check_merge_bases" checks that merge bases are not "bad" (or "new"). > * > * - If one is "bad" (or "new"), it means the user assumed something wrong > - * and we must exit with a non 0 error code. > + * and we must return error with a non 0 error code. > * - If one is "good" (or "old"), that's good, we have nothing to do. > * - If one is "skipped", we can't know but we should warn. > * - If we don't know, we should check it out and ask the user to test. > + * - If a merge base must be tested, on success return -11 a special condition > + * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad(). > */ > -static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) > +static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) > { > + int res = 0; > struct commit_list *result; > > result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1); > @@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) > handle_skipped_merge_base(mb); > } else { > printf(_("Bisecting: a merge base must be tested\n")); > - exit(bisect_checkout(mb, no_checkout)); > + res = bisect_checkout(mb, no_checkout); > + if (!res) > + /* indicate early success */ > + res = -11; This is yet another good candidate for an `enum`. > + break; > } > } > > free_commit_list(result); > + return res; > } > > static int check_ancestors(struct repository *r, int rev_nr, > @@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > { > char *filename = git_pathdup("BISECT_ANCESTORS_OK"); > struct stat st; > - int fd, rev_nr; > + int fd, rev_nr, res = 0; > struct commit **rev; > > if (!current_bad_oid) > @@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > goto done; > > /* Check if all good revs are ancestor of the bad rev. */ > + > rev = get_bad_and_good_commits(r, &rev_nr); > if (check_ancestors(r, rev_nr, rev, prefix)) > - check_merge_bases(rev_nr, rev, no_checkout); > + res = check_merge_bases(rev_nr, rev, no_checkout); > free(rev); > + if(res) Please put a space between the `if` keyword and the `(` following it. Thanks, Johannes > + exit(res == -11 ? 0 : -res); > > /* Create file BISECT_ANCESTORS_OK. */ > fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); > -- > 2.21.1 (Apple Git-122.3) > >
Hi, El lun., 20 ene. 2020 a las 23:09, Johannes Schindelin (<Johannes.Schindelin@gmx.de>) escribió: > > 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_merge_bases()`. > > > > In `check_merge_bases()` there is an early success special case, > > so we have introduced special error code `-11` which indicates early > > success. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`. > > > > Handle the return value in dependent function `check_good_are_ancestors_of_bad()`. > > This is a lot of repeated text from earlier commit messages. It might make > sense to condense it a bit, and while at it, to remove sentences that > essentially repeat what the diff says. > Ok. Noted! > > > > 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 | 21 ++++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/bisect.c b/bisect.c > > index 385afaf875..367258b0dd 100644 > > --- a/bisect.c > > +++ b/bisect.c > > @@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb) > > * "check_merge_bases" checks that merge bases are not "bad" (or "new"). > > * > > * - If one is "bad" (or "new"), it means the user assumed something wrong > > - * and we must exit with a non 0 error code. > > + * and we must return error with a non 0 error code. > > * - If one is "good" (or "old"), that's good, we have nothing to do. > > * - If one is "skipped", we can't know but we should warn. > > * - If we don't know, we should check it out and ask the user to test. > > + * - If a merge base must be tested, on success return -11 a special condition > > + * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad(). > > */ > > -static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) > > +static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) > > { > > + int res = 0; > > struct commit_list *result; > > > > result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1); > > @@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) > > handle_skipped_merge_base(mb); > > } else { > > printf(_("Bisecting: a merge base must be tested\n")); > > - exit(bisect_checkout(mb, no_checkout)); > > + res = bisect_checkout(mb, no_checkout); > > + if (!res) > > + /* indicate early success */ > > + res = -11; > > This is yet another good candidate for an `enum`. > > > + break; > > } > > } > > > > free_commit_list(result); > > + return res; > > } > > > > static int check_ancestors(struct repository *r, int rev_nr, > > @@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > > { > > char *filename = git_pathdup("BISECT_ANCESTORS_OK"); > > struct stat st; > > - int fd, rev_nr; > > + int fd, rev_nr, res = 0; > > struct commit **rev; > > > > if (!current_bad_oid) > > @@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r, > > goto done; > > > > /* Check if all good revs are ancestor of the bad rev. */ > > + > > rev = get_bad_and_good_commits(r, &rev_nr); > > if (check_ancestors(r, rev_nr, rev, prefix)) > > - check_merge_bases(rev_nr, rev, no_checkout); > > + res = check_merge_bases(rev_nr, rev, no_checkout); > > free(rev); > > + if(res) > > Please put a space between the `if` keyword and the `(` following it. Ok. > > Thanks, > Johannes > > > + exit(res == -11 ? 0 : -res); > > > > /* Create file BISECT_ANCESTORS_OK. */ > > fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); > > -- > > 2.21.1 (Apple Git-122.3) > > > >
diff --git a/bisect.c b/bisect.c index 385afaf875..367258b0dd 100644 --- a/bisect.c +++ b/bisect.c @@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb) * "check_merge_bases" checks that merge bases are not "bad" (or "new"). * * - If one is "bad" (or "new"), it means the user assumed something wrong - * and we must exit with a non 0 error code. + * and we must return error with a non 0 error code. * - If one is "good" (or "old"), that's good, we have nothing to do. * - If one is "skipped", we can't know but we should warn. * - If we don't know, we should check it out and ask the user to test. + * - If a merge base must be tested, on success return -11 a special condition + * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad(). */ -static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) +static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) { + int res = 0; struct commit_list *result; result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1); @@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) handle_skipped_merge_base(mb); } else { printf(_("Bisecting: a merge base must be tested\n")); - exit(bisect_checkout(mb, no_checkout)); + res = bisect_checkout(mb, no_checkout); + if (!res) + /* indicate early success */ + res = -11; + break; } } free_commit_list(result); + return res; } static int check_ancestors(struct repository *r, int rev_nr, @@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r, { char *filename = git_pathdup("BISECT_ANCESTORS_OK"); struct stat st; - int fd, rev_nr; + int fd, rev_nr, res = 0; struct commit **rev; if (!current_bad_oid) @@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r, goto done; /* Check if all good revs are ancestor of the bad rev. */ + rev = get_bad_and_good_commits(r, &rev_nr); if (check_ancestors(r, rev_nr, rev, prefix)) - check_merge_bases(rev_nr, rev, no_checkout); + 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);