Re: [PATCH v4 2/2] bisect: add --reset-when-found to leave when done

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -1211,6 +1212,7 @@ int bisect_clean_state(void)
>  	unlink_or_warn(git_path_bisect_run());
>  	unlink_or_warn(git_path_bisect_terms());
>  	unlink_or_warn(git_path_bisect_first_parent());
> +	unlink_or_warn(git_path_bisect_reset_when_found());
>  	/*
>  	 * Cleanup BISECT_START last to support the --no-checkout option
>  	 * introduced in the commit 4796e823a.

OK.  If we

> +static int bisect_reset_when_found(struct bisect_terms *terms)
> +{
> +	struct strbuf value = STRBUF_INIT;
> +	enum reset_when_found_mode mode;
> +	char *commit = NULL;
> +	int res;
> +
> +	if (strbuf_read_file(&value, git_path_bisect_reset_when_found(), 0) < 0) {
> +		res = error_errno(_("could not read '%s'"),
> +				  git_path_bisect_reset_when_found());
> +		goto cleanup;
> +	}

We expect that the caller calls this function only when we are doing
"--reset-when-found"; otherwise we would give an error message from
here even though we do not cause any damage otherwise.

We also expect that the callers refrain from calling this function
when bisect_next() that they eventually reach would not want to
immediately reset.

The defer_reset arrangement looks somewhat ugly even though what it
achieves may be a worthy thing to do.  Is the only code path that
passes defer_reset==true down the call chain the bisect_run()
codepath, to give that single caller a chance to close files that
bisect_reset() would remove by calling bisect_clean_state()?

I am wondering if the result of solving it slightly differently may
give us cleaner and easier to follow code, namely, we stop calling
bisect_clean_state() from bisect_reset().  Of course you would need
to find different place to call bisect_clean_state() to compensate,
if we go that route, but how many code paths do we have that depends
on bisect_reset() calling biesct_clean_state()?

Among existing callers of bisect_reset():

 - Does replay have to call reset?  Just like start does, isn't it
   sufficient to call clean_state?

 - cmd_bisect__reset() calls reset and returns, but it can call
   clean fater reset returns, if we need to make reset not to call
   clean.

> +	strbuf_trim(&value);
> +	if (parse_reset_when_found(value.buf, &mode)) {
> +		res = -1;
> +		goto cleanup;
> +	}
> +
> +	if (mode == RESET_WHEN_FOUND_TO_FOUND)
> +		commit = xstrfmt("refs/bisect/%s", terms->term_bad);
> +	res = bisect_reset(commit, 1);
> +
> +cleanup:
> +	free(commit);
> +	strbuf_release(&value);
> +	return res;
> +}

OK, "commit" is NULL unless mode specifies TO_FOUND in which case we
jump to the bad commit we found.  bisect_reset() knows that commit==NULL
means we go back to where we started.  OK.

> @@ -697,6 +760,9 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
>  
>  	if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
>  		res = bisect_successful(terms);
> +		if (!res && !defer_reset &&
> +		    !is_empty_or_missing_file(git_path_bisect_reset_when_found()))
> +			res = bisect_reset_when_found(terms);
>  		return res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;
>  	} else if (res == BISECT_ONLY_SKIPPED_LEFT) {
>  		res = bisect_skipped_commits(terms);

This is the first ugliness I mentioned earlier.

> @@ -1311,7 +1415,7 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
>  		saved_stdout = dup(1);
>  		dup2(temporary_stdout_fd, 1);
>  
> -		res = bisect_state(terms, 1, &new_state);
> +		res = bisect_state(terms, 1, &new_state, true);
>  
>  		fflush(stdout);
>  		dup2(saved_stdout, 1);
> @@ -1327,7 +1431,11 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
>  			res = BISECT_OK;
>  		} else if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
>  			printf(_("bisect found first '%s' commit\n"), terms->term_bad);
> -			res = BISECT_OK;
> +			if (!is_empty_or_missing_file(git_path_bisect_reset_when_found()) &&
> +			    bisect_reset_when_found(terms))
> +				res = BISECT_FAILED;
> +			else
> +				res = BISECT_OK;
>  		} else if (res) {
>  			error(_("bisect run failed: 'git bisect %s'"
>  				" exited with error code %d"), new_state, res);

And these are the second one, that made the first one needed.

Another thing that I find a bit iffy is that earlier we said:

    We expect that the caller calls this function only when we are doing
    "--reset-when-found"; otherwise we would give an error message from
    here even though we do not cause any damage otherwise.

    We also expect that the callers refrain from calling this function
    when bisect_next() that they eventually reach would not want to
    immediately reset.

but the way the callers see if "--reset-when-found" is in effect
looks quite ad-hoc.  Instead of sprinkling "do we have that file in
the filesystem and what does it say?" all over the place, I wonder
if it is simpler to reason about if we do these checks upfront and
store the parsed result in a variable, so that places that say
!is_empty_or_missing_file(...) etc. do not have to?  After all, we
do not keep calling get_terms() in the middle of operation, and
instead use values from "struct bisect_terms" that somebody else
prepared much earlier before terms->term_good and terms->term_bad
are used, right?  Shouldn't it be handled pretty much the same way?