Junio C Hamano <gitster@xxxxxxxxx> writes:
> And instead, hide the above new lines behind advice.statusHints,
> without introducing advice.statusHelp. As to the global code
> structure, it probably would make more sense to:
>
> - rename wt_status_evaluate_state() to wt_status_print_state();
>
> - rename the various "print help information for this state" functions
> that are called in the above if/else/... cascade to merge_in_progress_show()
> etc.
>
> - move the above if/else/... cascade to the end of
> wt_status_print_state(), which would make the above part more
> like:
>
> wt_status_print()
> {
> if (s->branch) {
> ...
> }
> + wt_status_print_state(s);
> if (s->is_initial) {
> ...
>
> - at the beginning of wt_status_print_state(), check advice.statusHints
> and return without doing anything if the user does not want hints.
>
> Otherwise, overall the patch is getting better looking.
>
> Thanks for a pleasant read.
After reading the current code, I changed my mind slightly.
The way the new code tries to separate the state information
(i.e. statement of the fact it found) and the advice messages
(i.e. what could be done next) is indeed in line with what the
advice.statusHints configuration is used in the existing code. We
unconditionally show what we find to the user, and we give help by
default but let the user decline.
So an updated suggestion would be to still move the whole thing to
wt_status_print_state(), but not check advice.statusHints at the
beginning of the function (as we will show what we find even when
the user declines help text) and do the discovery, and hide the help
messages (i.e. "do this to proceed") behind advice.statusHints,
without introducing advice.statusHelp.
As I pointed out in my review, there were some places in the patch
that gave help text unconditionally, which needs to be corrected.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html