Re: [PATCH v6 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:

> @@ -269,7 +276,79 @@ static int bisect_reset(const char *commit, bool quiet)
>  	}
>  
>  	strbuf_release(&branch);
> -	return bisect_clean_state();
> +	return 0;
> +}

Let's make a mental note that the intenral "bisect_reset()" no
longer calls bisect_clean_state(), so those that call this function
would eventually need to call it to compensate.

> @@ -682,7 +761,8 @@ static int bisect_successful(struct bisect_terms *terms)
>  	return res;
>  }
>  
> -static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> +static enum bisect_error bisect_next(struct bisect_terms *terms,
> +				     const char *prefix)
>  {
>  	enum bisect_error res;
>  
> @@ -705,7 +785,8 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
>  	return res;
>  }
>  
> -static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
> +static enum bisect_error bisect_auto_next(struct bisect_terms *terms,
> +					  const char *prefix)
>  {
>  	if (bisect_next_check(terms, NULL)) {
>  		bisect_print_status(terms);

The above two hunks are pure style clean-ups.  When having others to
review a 500+ line patch, you would want to omit them or move them
to a separate preliminary clean-up step, to avoid distracting them.

> @@ -1246,13 +1344,36 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
>  {
>  	int res = BISECT_OK;
>  	struct strbuf command = STRBUF_INIT;
> +	const char *reset_when_found_arg;
>  	const char *new_state;
>  	int temporary_stdout_fd, saved_stdout;
>  	int is_first_run = 1;
> +	enum reset_when_found_mode reset_when_found = RESET_WHEN_FOUND_NONE;
>  
>  	if (bisect_next_check(terms, NULL))
>  		return BISECT_FAILED;
>  
> +	if (argc && !strcmp(argv[0], "--reset-when-found")) {
> +		reset_when_found = RESET_WHEN_FOUND_TO_ORIGINAL;
> +	} else if (argc && skip_prefix(argv[0], "--reset-when-found=",
> +				    &reset_when_found_arg)) {
> +		if (parse_reset_when_found(reset_when_found_arg,
> +					   &reset_when_found))
> +			return BISECT_FAILED;
> +	}
> +
> +	if (reset_when_found != RESET_WHEN_FOUND_NONE &&
> +	    refs_ref_exists(get_main_ref_store(the_repository), "BISECT_HEAD"))
> +		return error(_("options '%s' and '%s' cannot be used together"),
> +			     "--reset-when-found", "--no-checkout");
> +
> +	if (reset_when_found != RESET_WHEN_FOUND_NONE) {
> +		write_file(git_path_bisect_reset_when_found(), "%s\n",
> +			   reset_when_found_mode_name(reset_when_found));
> +		argc--;
> +		argv++;
> +	}
> +
>  	if (!argc) {
>  		error(_("bisect run failed: no command provided."));
>  		return BISECT_FAILED;
> @@ -1327,7 +1448,6 @@ 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;

Now whoever called bisect_run() can react to 1st-bad-found but it is
their responsibility to report that overall bisect was OK to their
callers.

> @@ -1344,10 +1464,15 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
>  static int cmd_bisect__reset(int argc, const char **argv, const char *prefix UNUSED,
>  			     struct repository *repo UNUSED)
>  {
> +	int res;
> +
>  	if (argc > 1)
>  		return error(_("'%s' requires either no argument or a commit"),
>  			     "git bisect reset");
> -	return bisect_reset(argc ? argv[0] : NULL, false);
> +	res = bisect_reset(argc ? argv[0] : NULL, false);
> +	if (res)
> +		return res;
> +	return bisect_clean_state();
>  }

Everything contained in this patch to enable --reset-when-finished
are exactly as expected and very understandable to me, but this bit
was a bit hard to grok.  Let me think aloud to see if I can explain
it.

 * Lower level bisect_reset() used to almost always called
   clean_state(), but except when it returned error().

 * Now bisect_reset() never calls clean_state().

Hence, somebody has to call it in the new code.  The above change is
an example of doing exactly that.  If bisect_reset() returns an
error, we refrain from cleaning the state.  If it succeeds, we clean
the state.

Earlier we saw that bisect_reset_when_found() does the same thing.
If bisect_reset() did not fail, it called clean_state().

Both make sense.

>  static int cmd_bisect__terms(int argc, const char **argv, const char *prefix UNUSED,
> @@ -1489,7 +1614,8 @@ int cmd_bisect(int argc,
>  		    !one_of(argv[0], terms.term_good, terms.term_bad, NULL))
>  			usage_msg_optf(_("unknown command: '%s'"), git_bisect_usage,
>  				       options, argv[0]);
> -		res = bisect_state(&terms, argc, argv);
> +		else
> +			res = bisect_state(&terms, argc, argv);
>  		free_terms(&terms);
>  	} else {
>  		argc--;

What is this change about?  We used to see if the given terms
(bad/good) are sensible and otherwise barfed with usage_msg_optf()
that never returns, so we did without "else".  With "else" you are
making it more explicit.  The value of such a change is debatable.
Some would say that, just like 'if ... die()', it is already
explicit enough that 'if ... usage()' never returns and does not
require an "else".  Some would say new readers may not know die()
and usage() do not return, so "else" makes it more explicit.  My
stance is that we should not optimize our code for total newbies
[*], so I may have a mild preference for the original over the
updated version, but it is minor.  In other words, I would not mind
if an author wrote this either way in new code.

However.

If an author is adding a new feature, I would recommend against
making such a change that would only force reviewers to read more
and think more about the change.  Do not waste reviewers' attention,
which is a precious resource, to something much less relevant for
the goal of your topic.

> @@ -1497,5 +1623,15 @@ int cmd_bisect(int argc,
>  		res = fn(argc, argv, prefix, repo);
>  	}
>  
> +	if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> +		enum reset_when_found_mode mode;
> +
> +		if (read_reset_when_found(&mode))
> +			res = BISECT_FAILED;
> +		else if (mode != RESET_WHEN_FOUND_NONE &&
> +			 bisect_reset_when_found(mode))
> +			res = BISECT_FAILED;
> +	}

Are there "dead end" states, other than '1st-bad-found', in which we
can no longer make any progress?  One thing that comes to mind is
"you said this one is good, but that contradicts what you said about
its ancestor that you said is bad".  I wonder if we want to do
anything special here, just as this part of the code handles the
'1st-bad-found' state, for such "dead end" states.

This is just for my own education, as I am pondering possible future
extensions.

>  	return is_bisect_success(res) ? 0 : -res;
>  }

Overall, the patch looks very nicely done, except for a few minor
nits that made my reading hiccup while I was reviewing this round.

Thanks.