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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Junio C Hamano <gitster@xxxxxxxxx> 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.




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


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]