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

[konglu@minatec.inpg.fr]

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

>> +	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.


>> +	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.

>
>> +	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.