Re: [PATCH v14 14/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Hey Junio, On Wed, Aug 31, 2016 at 1:03 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> This is a very tricky one. I have purposely not included this after a >> lot of testing. I have hand tested with the original git and with this >> branch. The reason why anyone wouldn't be able to catch this is >> because its not covered in the test suite. I am including a patch with >> this as an attachment (because I am behind a proxy right now but don't >> worry I will include this as a commit in the next series). The >> original behaviour of git does not clean the bisect state when this >> situation occurs. > > "We sometimes clean and we sometimes don't and this follows the > original" may be a valid justification but it is not a very useful > explanation. The real issue is if not cleaning is intended (and if > so why; otherwise, if it is clear that it is simply forgotten, we > can just fix it in the series as a follow-up step). I think we do want to recover from this "bad merge base" state and for that not cleaning up is essential. The original behaviour of git seems natural to me. > If not cleaning in some cases (but not others) is the right thing, > at least there needs an in-code comment to warn others against > "fixing" the lack of cleanups (e.g. "don't clean state here, because > the caller still wants to see what state we were for this and that > reason"). I will mention it in the comments. + if (bisect_next_check(terms, terms->term_good.buf)) + return -1; >>> >>> Mental note. The "autostart" in the original is gone. Perhaps it >>> is done by next_check in this code, but we haven't seen that yet. >> >> This will be added back again in a coming patch[1]. > > In other words, this series is broken at this step and the breakage > stay with the codebase until a later step? Broken though it passes the test suite. > I do not know if reordering the patches in the series is enough to > fix that, or if it is even worth to avoid such a temporary breakage. > It depends on how serious the circularity is, but I would understand > if it is too hard and not worth the effort (I think in a very early > review round some people advised against the bottom-up rewrite > because they anticipated this exact reason). At least the omission > (and later resurrection) needs to be mentioned in the log so that > people understand what is going on when they later need to bisect. bisect_autostart() call from bisect_next() was introduced in the earliest version of git-bisect in the commit 8cc6a0831 but it isn't really a very big deal and I think it would be OK to skip it for a very little while as any which ways the series in the end would fix it. It is important to mention this in the commit message and I will do it. Regards, Pranit Bauva
Re: [PATCH v14 14/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Pranit Bauva writes: > This is a very tricky one. I have purposely not included this after a > lot of testing. I have hand tested with the original git and with this > branch. The reason why anyone wouldn't be able to catch this is > because its not covered in the test suite. I am including a patch with > this as an attachment (because I am behind a proxy right now but don't > worry I will include this as a commit in the next series). The > original behaviour of git does not clean the bisect state when this > situation occurs. "We sometimes clean and we sometimes don't and this follows the original" may be a valid justification but it is not a very useful explanation. The real issue is if not cleaning is intended (and if so why; otherwise, if it is clear that it is simply forgotten, we can just fix it in the series as a follow-up step). If not cleaning in some cases (but not others) is the right thing, at least there needs an in-code comment to warn others against "fixing" the lack of cleanups (e.g. "don't clean state here, because the caller still wants to see what state we were for this and that reason"). >>> + if (bisect_next_check(terms, terms->term_good.buf)) >>> + return -1; >> >> Mental note. The "autostart" in the original is gone. Perhaps it >> is done by next_check in this code, but we haven't seen that yet. > > This will be added back again in a coming patch[1]. In other words, this series is broken at this step and the breakage stay with the codebase until a later step? I do not know if reordering the patches in the series is enough to fix that, or if it is even worth to avoid such a temporary breakage. It depends on how serious the circularity is, but I would understand if it is too hard and not worth the effort (I think in a very early review round some people advised against the bottom-up rewrite because they anticipated this exact reason). At least the omission (and later resurrection) needs to be mentioned in the log so that people understand what is going on when they later need to bisect. Thanks.
Re: [PATCH v14 14/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Hey Junio,
On Tue, Aug 30, 2016 at 11:55 PM, Pranit Bauva wrote:
>
>>> @@ -729,7 +735,7 @@ static struct commit **get_bad_and_good_commits(int
>>> *rev_nr)
>>> return rev;
>>> }
>>>
>>> -static void handle_bad_merge_base(void)
>>> +static int handle_bad_merge_base(void)
>>> {
>>> if (is_expected_rev(current_bad_oid)) {
>>> char *bad_hex = oid_to_hex(current_bad_oid);
>>> @@ -750,17 +756,18 @@ static void handle_bad_merge_base(void)
>>> "between %s and [%s].\n"),
>>> bad_hex, term_bad, term_good, bad_hex,
>>> good_hex);
>>> }
>>> - exit(3);
>>> + return 3;
>>> }
>>>
>>> fprintf(stderr, _("Some %s revs are not ancestor of the %s rev.\n"
>>> "git bisect cannot work properly in this case.\n"
>>> "Maybe you mistook %s and %s revs?\n"),
>>> term_good, term_bad, term_good, term_bad);
>>> - exit(1);
>>> + bisect_clean_state();
>>> + return 1;
>>
>> What is the logic behind this function sometimes clean the state,
>> and some other times do not, when it makes an error-return? We see
>> above that "return 3" codepath leaves the state behind.
>>
>> Either you forgot a necessary clean_state in "return 3" codepath,
>> or you forgot to document why the distinction exists in the in-code
>> comment for the function. I cannot tell which, but I am leaning
>> towards guessing that it is the former.
>
> This is a very tricky one. I have purposely not included this after a
> lot of testing. I have hand tested with the original git and with this
> branch. The reason why anyone wouldn't be able to catch this is
> because its not covered in the test suite. I am including a patch with
> this as an attachment (because I am behind a proxy right now but don't
> worry I will include this as a commit in the next series). The
> original behaviour of git does not clean the bisect state when this
> situation occurs. On another note which you might have missed that
> bisect_clean_state() is purposely put before return 1 which is covered
> by the test suite. You can try removing it and see that there is a
> broken tes. tI was thinking of including the tests after the whole
> conversion but now I think including this before will make the
> conversion more easier for review.
The patch which I included as an attachment before will fail for
different reasons if you apply it on master branch. To test it on
master branch use the current attachment. Again sorry I couldn't
include this in the email itself.
Regards,
Pranit Bauva
out3
Description: Binary data
Re: [PATCH v14 14/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Hey Junio,
Sorry for a late replay.
On Fri, Aug 26, 2016 at 2:00 AM, Junio C Hamano wrote:
> Pranit Bauva writes:
>
>> A lot of parts of bisect.c uses exit() and these signals are then
>> trapped in the `bisect_start` function. Since the shell script ceases
>> its existence it would be necessary to convert those exit() calls to
>> return statements so that errors can be reported efficiently in C code.
>
> Is efficiency really an issue? I think the real reason is that it
> would make it impossible for the callers to handle errors, if you do
> not convert and let the error codepaths exit().
I think I put the word "efficiently" wrongly over here. Will omit it.
>> @@ -729,7 +735,7 @@ static struct commit **get_bad_and_good_commits(int
>> *rev_nr)
>> return rev;
>> }
>>
>> -static void handle_bad_merge_base(void)
>> +static int handle_bad_merge_base(void)
>> {
>> if (is_expected_rev(current_bad_oid)) {
>> char *bad_hex = oid_to_hex(current_bad_oid);
>> @@ -750,17 +756,18 @@ static void handle_bad_merge_base(void)
>> "between %s and [%s].\n"),
>> bad_hex, term_bad, term_good, bad_hex,
>> good_hex);
>> }
>> - exit(3);
>> + return 3;
>> }
>>
>> fprintf(stderr, _("Some %s revs are not ancestor of the %s rev.\n"
>> "git bisect cannot work properly in this case.\n"
>> "Maybe you mistook %s and %s revs?\n"),
>> term_good, term_bad, term_good, term_bad);
>> - exit(1);
>> + bisect_clean_state();
>> + return 1;
>
> What is the logic behind this function sometimes clean the state,
> and some other times do not, when it makes an error-return? We see
> above that "return 3" codepath leaves the state behind.
>
> Either you forgot a necessary clean_state in "return 3" codepath,
> or you forgot to document why the distinction exists in the in-code
> comment for the function. I cannot tell which, but I am leaning
> towards guessing that it is the former.
This is a very tricky one. I have purposely not included this after a
lot of testing. I have hand tested with the original git and with this
branch. The reason why anyone wouldn't be able to catch this is
because its not covered in the test suite. I am including a patch with
this as an attachment (because I am behind a proxy right now but don't
worry I will include this as a commit in the next series). The
original behaviour of git does not clean the bisect state when this
situation occurs. On another note which you might have missed that
bisect_clean_state() is purposely put before return 1 which is covered
by the test suite. You can try removing it and see that there is a
broken tes. tI was thinking of including the tests after the whole
conversion but now I think including this before will make the
conversion more easier for review.
>> -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;
>>
>> if (!current_bad_oid)
>> die(_("a %s revision is needed"), term_bad);
>
> Can you let it die yere?
Not really. I should change it to return error().
>> @@ -873,8 +890,11 @@ static void check_good_are_ancestors_of_bad(const char
>> *prefix, int no_checkout)
>> filename);
>> else
>> close(fd);
>> +
>> + return 0;
>> done:
>> free(filename);
>> + return 0;
>> }
>
> Who owns "filename"? The first "return 0" leaves it unfreed, and
> when "goto done" is done, it is freed.
>
> The above two may indicate that "perhaps 'retval + goto finish'
> pattern?" is a really relevant suggestion for the earlier steps in
> this series.
Yes.
>> if (!all) {
>> fprintf(stderr, _("No testable commit found.\n"
>> "Maybe you started with bad path parameters?\n"));
>> - exit(4);
>> + return 4;
>> }
>>
>> bisect_rev = revs.commits->item->object.oid.hash;
>>
>> if (!hashcmp(bisect_rev, current_bad_oid->hash)) {
>> - exit_if_skipped_commits(tried, current_bad_oid);
>> + res = exit_if_skipped_commits(tried, current_bad_oid);
>> + if (res)
>> + return res;
>> +
>> printf("%s is the first %s commit\n", sha1_to_hex(bisect_rev),
>> term_bad);
>> show_diff_tree(prefix, revs.commits->item);
>> /* This means the bisection process succeeded. */
>> - exit(10);
>> + return 10;
>> }
>>
>> nr = all - reaches - 1;
>> @@ -1005,7 +1033,11 @@ int bisect_next_all(const char *prefix, int
>> no_checkout)
>> "Bisecting: %d revisions left to t
Re: [PATCH v14 14/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Pranit Bauva writes:
> A lot of parts of bisect.c uses exit() and these signals are then
> trapped in the `bisect_start` function. Since the shell script ceases
> its existence it would be necessary to convert those exit() calls to
> return statements so that errors can be reported efficiently in C code.
Is efficiency really an issue? I think the real reason is that it
would make it impossible for the callers to handle errors, if you do
not convert and let the error codepaths exit().
> @@ -729,7 +735,7 @@ static struct commit **get_bad_and_good_commits(int
> *rev_nr)
> return rev;
> }
>
> -static void handle_bad_merge_base(void)
> +static int handle_bad_merge_base(void)
> {
> if (is_expected_rev(current_bad_oid)) {
> char *bad_hex = oid_to_hex(current_bad_oid);
> @@ -750,17 +756,18 @@ static void handle_bad_merge_base(void)
> "between %s and [%s].\n"),
> bad_hex, term_bad, term_good, bad_hex,
> good_hex);
> }
> - exit(3);
> + return 3;
> }
>
> fprintf(stderr, _("Some %s revs are not ancestor of the %s rev.\n"
> "git bisect cannot work properly in this case.\n"
> "Maybe you mistook %s and %s revs?\n"),
> term_good, term_bad, term_good, term_bad);
> - exit(1);
> + bisect_clean_state();
> + return 1;
What is the logic behind this function sometimes clean the state,
and some other times do not, when it makes an error-return? We see
above that "return 3" codepath leaves the state behind.
Either you forgot a necessary clean_state in "return 3" codepath,
or you forgot to document why the distinction exists in the in-code
comment for the function. I cannot tell which, but I am leaning
towards guessing that it is the former.
> -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;
>
> if (!current_bad_oid)
> die(_("a %s revision is needed"), term_bad);
Can you let it die yere?
> @@ -873,8 +890,11 @@ static void check_good_are_ancestors_of_bad(const char
> *prefix, int no_checkout)
> filename);
> else
> close(fd);
> +
> + return 0;
> done:
> free(filename);
> + return 0;
> }
Who owns "filename"? The first "return 0" leaves it unfreed, and
when "goto done" is done, it is freed.
The above two may indicate that "perhaps 'retval + goto finish'
pattern?" is a really relevant suggestion for the earlier steps in
this series.
> if (!all) {
> fprintf(stderr, _("No testable commit found.\n"
> "Maybe you started with bad path parameters?\n"));
> - exit(4);
> + return 4;
> }
>
> bisect_rev = revs.commits->item->object.oid.hash;
>
> if (!hashcmp(bisect_rev, current_bad_oid->hash)) {
> - exit_if_skipped_commits(tried, current_bad_oid);
> + res = exit_if_skipped_commits(tried, current_bad_oid);
> + if (res)
> + return res;
> +
> printf("%s is the first %s commit\n", sha1_to_hex(bisect_rev),
> term_bad);
> show_diff_tree(prefix, revs.commits->item);
> /* This means the bisection process succeeded. */
> - exit(10);
> + return 10;
> }
>
> nr = all - reaches - 1;
> @@ -1005,7 +1033,11 @@ int bisect_next_all(const char *prefix, int
> no_checkout)
> "Bisecting: %d revisions left to test after this %s\n",
> nr), nr, steps_msg);
>
> - return bisect_checkout(bisect_rev, no_checkout);
> + res = bisect_checkout(bisect_rev, no_checkout);
> + if (res)
> + bisect_clean_state();
> +
> + return res;
> }
There were tons of "exit_if" that was converted to "if (res) return
res" above, instead of jumping here to cause clean_state to be
called. I cannot tell if this new call to clean_state() is wrong,
or all the earlier "return res" should come here. I am guessing the
latter.
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c64996a..ef7b3a1 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -8,6 +8,7 @@
> #include "run-command.h"
> #include "prompt.h"
> #include "quote.h"
> +#include "revision.h"
>
> static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
> static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
> @@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-terms [--term-good | --term-old |
> --term-bad | --term-new]"),
> N_("git bisect--helper --bi
[PATCH v14 14/27] bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
Reimplement the `bisect_next` and the `bisect_auto_next` shell function
in C and add the subcommands to `git bisect--helper` to call it from
git-bisect.sh .
Along with this conversion of `bisect_start` has also finished and thus
it has been fully ported to C.
A lot of parts of bisect.c uses exit() and these signals are then
trapped in the `bisect_start` function. Since the shell script ceases
its existence it would be necessary to convert those exit() calls to
return statements so that errors can be reported efficiently in C code.
As more and more calls are happening to the subcommands in `git
bisect--helper`, more specifically when `bisect_start $rev` is converted to
`git bisect--helper --bisect-start $rev` it is necessary to dequote the
arguments because of shell to C conversion.
Using `--bisect-next` and `--bisect-auto-start` subcommands is a
temporary measure to port shell function to C so as to use the existing
test suite. As more functions are ported, this subcommand will be
retired and will be called by some other methods.
Mentored-by: Lars Schneider
Mentored-by: Christian Couder
Signed-off-by: Pranit Bauva
---
bisect.c | 80 --
builtin/bisect--helper.c | 206 ++-
git-bisect.sh| 74 ++---
3 files changed, 249 insertions(+), 111 deletions(-)
diff --git a/bisect.c b/bisect.c
index 45d598d..68c583b 100644
--- a/bisect.c
+++ b/bisect.c
@@ -618,6 +618,12 @@ static void bisect_rev_setup(struct rev_info *revs, const
char *prefix,
struct argv_array rev_argv = ARGV_ARRAY_INIT;
int i;
+ /*
+* Since the code is slowly being converted to C, there might be
+* instances where the revisions were initialized before. Thus
+* we first need to reset it.
+*/
+ reset_revision_walk();
init_revisions(revs, prefix);
revs->abbrev = 0;
revs->commit_format = CMIT_FMT_UNSPECIFIED;
@@ -644,11 +650,11 @@ static void bisect_common(struct rev_info *revs)
mark_edges_uninteresting(revs, NULL);
}
-static void exit_if_skipped_commits(struct commit_list *tried,
+static int exit_if_skipped_commits(struct commit_list *tried,
const struct object_id *bad)
{
if (!tried)
- return;
+ return 0;
printf("There are only 'skip'ped commits left to test.\n"
"The first %s commit could be any of:\n", term_bad);
@@ -659,7 +665,7 @@ static void exit_if_skipped_commits(struct commit_list
*tried,
if (bad)
printf("%s\n", oid_to_hex(bad));
printf(_("We cannot bisect more!\n"));
- exit(2);
+ return 2;
}
static int is_expected_rev(const struct object_id *oid)
@@ -700,7 +706,7 @@ static int bisect_checkout(const unsigned char *bisect_rev,
int no_checkout)
int res;
res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
if (res)
- exit(res);
+ return res;
}
argv_show_branch[1] = bisect_rev_hex;
@@ -729,7 +735,7 @@ static struct commit **get_bad_and_good_commits(int *rev_nr)
return rev;
}
-static void handle_bad_merge_base(void)
+static int handle_bad_merge_base(void)
{
if (is_expected_rev(current_bad_oid)) {
char *bad_hex = oid_to_hex(current_bad_oid);
@@ -750,17 +756,18 @@ static void handle_bad_merge_base(void)
"between %s and [%s].\n"),
bad_hex, term_bad, term_good, bad_hex,
good_hex);
}
- exit(3);
+ return 3;
}
fprintf(stderr, _("Some %s revs are not ancestor of the %s rev.\n"
"git bisect cannot work properly in this case.\n"
"Maybe you mistook %s and %s revs?\n"),
term_good, term_bad, term_good, term_bad);
- exit(1);
+ bisect_clean_state();
+ return 1;
}
-static void handle_skipped_merge_base(const unsigned char *mb)
+static int handle_skipped_merge_base(const unsigned char *mb)
{
char *mb_hex = sha1_to_hex(mb);
char *bad_hex = oid_to_hex(current_bad_oid);
@@ -773,6 +780,7 @@ static void handle_skipped_merge_base(const unsigned char
*mb)
"We continue anyway."),
bad_hex, good_hex, term_bad, mb_hex, bad_hex);
free(good_hex);
+ return 0;
}
/*
@@ -784,10 +792,10 @@ static void handle_skipped_merge_base(const unsigned char
*mb)
* - 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.
*/
-static void check_merge_bases(int no_checkout)
+static int check_merge_bases(int no_checkout)
{
struct commit_list *result;
- int rev_nr;
+ int rev_nr, res = 0;
struct commit **rev = get_bad_and_good_commits(&rev
