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

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

Junio C Hamano <gitster@pobox.com> writes:

> 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()?

Conceptually, what you want out of this new feature is:

    Please run "git bisect <anything>" normally.  Make sure you
    notice when the command completed and found the culprit, And
    when you notice it, run "git bisect reset" (or "git bisect reset
    <oid-of-bad-commit>").

Stepping back even further, shouldn't this be doable *a* *lot*
simpler, given that existing code does not locally exit(0)?
Wouldn't it be the matter of

 - Add --reset-when-finished option parsing to the main command and
   the subcommands that want to parse it.

 - Make sure all indivudual cmd_bisect__<subcommand> returns the
   result code, instead of calling exit(), and the result code
   includes those INTERNAL ones.

 - Tweak bisect_next_all() to report the found commit when it
   returns BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.  Perhaps
   bisect_next() can leave it in a file-scope static variable.

 - Instead of doing

	return is_bisect_success(res) ? 0 : -res;

   at the end of cmd_bisect(), if "--reset-when-finished" is in
   effect, also react to BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND and
   use the commit bisect_next() received from bisect_next_all() as
   needed and do what bisect_reset() does.  Presumably at that
   point, files that are problematic on Windows would all already
   been closed, right?

That way, almost all the changes needed to cmd_bisect__<subcommand>
and below will be pure clean-up changes.  

Hmm?