Re: [PATCHv2 1/2] wt-status: better advices for git status

From: konglu@minatec.inpg.fr <konglu@minatec.inpg.fr>
Date: May 28, 2012, 10:54 UTC
Message-ID: <20120528125432.Horde.KyJxSnwdC4BPw1lo0-ZkrQA@webmail.minatec.grenoble-inp.fr>
In-Reply-To: <7v1um47vik.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> a écrit :

Show 8 quoted lines

>> +	int unmerged_state = 0;
>> +	int rebase_state = 0;
>> +	int rebase_interactive_state = 0;
>> +	int am_state = 0;
>> +	int bisect_state = 0;
>
> These are not independent (you cannot be in bisect and am at the
> same time).  Why five independent variables?

Some of these variables are independant. We can be in rebase and bisect at the same time and, as Matthieu said, we can do an am during a bisect. However, there are dependant cases such as you cannot do a rebase during an am.

Show 13 quoted lines

>> +	for (i = 0; i < s->change.nr; i++) {
>> +		struct wt_status_change_data *d;
>> +		struct string_list_item *it;
>> +		it = &(s->change.items[i]);
>> +		d = it->util;
>> +		if (d->stagemask) {
>> +			conflict = 1;
>> +			continue;
>> +		}
>> +	}
>
> That "continue" looks like a no-op.  Mental note: conflict seems to
> remember if there was any path that was unmerged.

You're right. It should be a "break" instead of "continue".

> Such a code structure invites bugs and missed cases (e.g. you do not
> seem to say anything after you detect that you are in "am").

In fact, the case of 'am' is not implemented in this patch. It's supposed to be added in the next patch.

Show 21 quoted lines

>
>> +	if(rebase_state || rebase_interactive_state) {
>> +		if (conflict) {
>> +			status_printf_ln(s, c, _("You are currently rebasing: fix  
>> conflicts and then run \"git rebase -- continue\"."));
>> +			status_printf_ln(s, c, _("If you would prefer to skip this  
>> patch, instead run \"git rebase --skip\"."));
>> +			status_printf_ln(s, c, _("To check out  the original branch and  
>> stop rebasing run \"git rebase --abort\"."));
>> +		}
>> +		else {
>
> 	if (...) {
> 		...
> 	} else {
> 		...
> 	}
>
>> +			if (rebase_state)
>
> Why extra level of nesting?

The 'conflict' variable stands for the case when we're in a conflict before resolving it (with 'git add'), as we distinguish the warning messages for when we're before resolving a conflict and after resolving it. So, the case with "conflict" can happen in both cases rebase_state and rebase_interactive_state but the warning messages are still specifical for the rebase cases. However, There are still specifical messages for when you are in rebase_state or in rebase_interactive_state.

>
> Oh, another thing.  Perhaps these (both detection logic and output)
> should be protected with a new advise.* configuration variable, no?

Yes, We think that it would be wise to add a new advice.* config variable to control the display of the new warning messages.