Re: [PATCH] wt-status.c: Modified status message shown for a parent-less branch

From: Jeff King <peff@peff.net>
Date: Jun 12, 2017, 21:20 UTC
Message-ID: <20170612212025.ytyukvmmthfcsejh@sigill.intra.peff.net>
In-Reply-To: <xmqqa85dnjpz.fsf@gitster.mtv.corp.google.com>

On Mon, Jun 12, 2017 at 11:28:56AM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:
> 
> >> Adding a bit to "struct wt_status" is a good first step to allow all
> >> three (i.e. in addition to "Initial commit" and "Changes to be
> >> committed", "Changes not staged for commit" is the other one that
> >> shares this potential confusion factor) to be phrased in a way that
> >> is more appropriate in an answer to the question "what is the status
> >> of my working area?", I would think.
> >> 
> >> Thanks.
> >> 
> > It seems that the current change has to be discarded altogether and
> > further the change required doesn't look trivial. This seems to warrant
> > some bit of research of the code base. As a first step I would like to
> > know which part of the code base creates the commit template. I guess
> > much can't be done without knowing how it's created.
> 
> Perhaps something along this line (warning: not even compile
> tested)?

So I think the addition of the bit here is obviously correct, and I'm not opposed to the idea of giving wt-status more information so that it can make better messages. But I'm not sure it's actually helping for some of these cases. E.g.:

Show 5 quoted lines

> -	status_printf_ln(s, c, _("Changes not staged for commit:"));
> +	if (s->commit_template)
> +		status_printf_ln(s, c, _("Changes not staged for commit:"));
> +	else
> +		status_printf_ln(s, c, _("Changes not yet in the index:"));

I think "staged for commit" still makes perfect sense even when we are just asking "what's the current status" and not "what would it look like if I were to commit".

And avoiding the word "index" is worth-while here, I think. I am not in general of the "let's hide the index" camp" but it is a technical term. If we can say the same thing in a way that is understood both by people who know what the index is and people who do not, that seems like a win.

Show 5 quoted lines

> -	status_printf_ln(s, c, _("Changes to be committed:"));
> +	if (s->commit_template)
> +		status_printf_ln(s, c, _("Changes to be committed:"));
> +	else
> +		status_printf_ln(s, c, _("Changes already in the index:"));

This one is less obvious, because "to be committed" more strongly implies making an actual commit. At the same time, I don't think it's unclear what it means in the context of status. It could be "Changes staged for commit" to match the other "not staged" message, though I think I prefer the existing wording.

Show 9 quoted lines

> @@ -1578,7 +1584,10 @@ static void wt_longstatus_print(struct wt_status *s)
>  
>  	if (s->is_initial) {
>  		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
> -		status_printf_ln(s, color(WT_STATUS_HEADER, s), _("Initial commit"));
> +		status_printf_ln(s, color(WT_STATUS_HEADER, s),
> +				 s->commit_template
> +				 ? _("Initial commit")
> +				 : _("No commit yet on the branch"));

This one I think is an improvement. :)

-Peff