Re: [PATCH v2 1/2] extract setting of wt_status.commitable flag out of wt_status_print_updated()

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 4, 2015, 21:34 UTC
Message-ID: <xmqq8ubzupi8.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433440591-30917-2-git-send-email-rctay89@gmail.com>

Tay Ray Chuan <rctay89@gmail.com> writes:

> It may not be obvious from its name that wt_status_print_updated() that
> it also sets wt_status.commitable, which affects commit functionality.
> Extract this out into a separate function for improved clarity, though
> at the expense of executing another loop.

Makes sense.

Show 8 quoted lines

> @@ -1360,6 +1374,7 @@ void wt_status_print(struct wt_status *s)
>  
>  	wt_status_print_updated(s);
>  	wt_status_print_unmerged(s);
> +	wt_status_mark_commitable(s);
>  	wt_status_print_changed(s);
>  	if (s->submodule_summary &&
>  	    (!s->ignore_submodule_arg ||

As this is the only callsite of _updated(), we can be assured that the conversion would not change the behaviour.

But I am not sure the placement of the new call is sensible. The standard pattern used in the wt-status infrastructure is to first collect the information and then make output based on what was collected. Because the value of this patch is to separte the "is it committable?" information gathering step out of the output step, shouldn't the call be made a lot earlier than these sequence of wt_status_print_blah() calls?

I am wondering if the flipping of the "is it committable?" bit belongs to wt_status_collect(). It could be that some other crufty checks that wt_status_print() have accumulated over time might be better moved to the "collect" phase, but that is a separate topic.