Hey Stephan,
Extremely sorry I just forgot to reply to this email before. I was
preparing from the next iteration when I saw this.
On Mon, Nov 21, 2016 at 1:31 AM, Stephan Beyer <s-beyer@xxxxxxx> wrote:
> 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...
I had confusion about how to split the commits, but then I then
decided to dump it all together so that it compiles (I was finding it
difficult to split into meaningful parts which also compiled).
> 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:
>> 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()?
It should be return error(). Thanks for pointing it out! :)
>> @@ -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.
I will just remove that line.
>> 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.)
Yes. I will use xstrdup() and it does leak.
>> @@ -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.)
Yes I have addressed this issue.
> [This repeats several times below.]
>
>> 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?
This line is because of the following:
* TERM_BAD and TERM_GOOD are global but in the coming patch they
would be removed as global variables.
* To compensate for that, I will write out the state of TERM_BAD and
TERM_GOOD every time it is updated in the file BISECT_TERMS.
* So we will be reading it from there.
* It is quite possible that this is completely redundant as for now
but I really don't care to check for each case because I have removed
the shell function bisect_state() afterwards and then this line won't
create a problem there because we are using `struct bisect_terms`
there.