Re: [PATCHv4 3/3] Advices about 'git rm' during conflicts (unmerged paths) more relevant

Kong Lucien [off-list ref] writes:

 static void wt_status_print_unmerged_header(struct wt_status *s)
 {
+	int i;
+	int simple_deleted_flag = 0;
+	int both_deleted_flag = 0;
+	int not_deleted_flag = 0;
 	const char *c = color(WT_STATUS_HEADER, s);
 
 	status_printf_ln(s, c, _("Unmerged paths:"));
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct string_list_item *it = &(s->change.items[i]);
+		struct wt_status_change_data *d = it->util;
+
+		if (d->stagemask == 1 && !both_deleted_flag)
+			both_deleted_flag = 1;
+		else if ((d->stagemask == 3 || d->stagemask == 5) && !simple_deleted_flag)
+			simple_deleted_flag = 1;
+		else if ((d->stagemask == 2 || d->stagemask == 4 || d->stagemask == 6 ||
+				d->stagemask == 7) && !not_deleted_flag)
+			not_deleted_flag = 1;
+	}

Yuck.  Do you need to "&& !flag" in any of these?

	switch (d->stagemask) {
        case 1:
        	both_deleted = 1;
                break;
        case 3:
        case 5:
        	simple_deleted = 1;
                break;
        default:
        	not_deleted = 1;
                break;
	}

Also, I think "simple deleted" is grossly misnamed.  It is "one side
deleted, other side possibly modified", isn't it?  Because it is
almost always safe to resolve "both sides deleted" to a deletion,
but "one deleted, the other modified" needs a lot more thought to
resolve correctly, it is a much more complex case.  I'd call it
del_mod_conflict or something if I were writing this code.

In any case, drop "_flag" from the names.  They are annoying and do
not add much value.

If you meant the control flow to follow your indentation, I think
you are missing opening "{" at the end of the above line.

+		if (!simple_deleted_flag)
+			status_printf_ln(s, c, _("  (use \"git add <file>...\" as appropriate to mark resolution)"));

Nobody deleted, so we do not suggest "rm" as resolution, which makes
sense.

As far as I know, in the current message we say "as appropriate"
because we leave the choice of add/rm to the end user.  Do you still
need "as appropriate" after deciding that "add" is the only choice
for the user?

+		else
+			status_printf_ln(s, c, _("  (use \"git add/rm <file>...\" as appropriate to mark resolution)"));

There is del/mod conflict and nothing else; the user needs to choose
between add/rm.  OK.

+	else if (!simple_deleted_flag && !not_deleted_flag)
+		status_printf_ln(s, c, _("  (use \"git rm <file>...\" as appropriate to mark resolution)"));

Assuming that a closing "}" for the "if (!both_deleted)" above is
missing at the beginning of this line, there is no del/mod conflict,
some del/del and no addition, so "rm" is the only choice, which
makes sense.  The same comment on "as appropriate" applies here.

+	else
+		status_printf_ln(s, c, _("  (use \"git add/rm <file>...\" as appropriate to mark resolution)"));
 	status_printf_ln(s, c, "");
 }

I like the way it tries to be helpful, but I doubt this patch would
make much difference in the real life.  As you follow the logic like
I demonstrated above, you show specific help only in a very narrow
case (i.e. there is no possible removal or there are nothing but
"both sides removed").

Junio C Hamano [off-list ref] a écrit :

quoted

+		if (d->stagemask == 1 && !both_deleted_flag)
+			both_deleted_flag = 1;
+		else if ((d->stagemask == 3 || d->stagemask == 5) &&  
!simple_deleted_flag)
+			simple_deleted_flag = 1;
+		else if ((d->stagemask == 2 || d->stagemask == 4 || d->stagemask == 6 ||
+				d->stagemask == 7) && !not_deleted_flag)
+			not_deleted_flag = 1;
+	}

	switch (d->stagemask) {
        case 1:
        	both_deleted = 1;
                break;
        case 3:
        case 5:
        	simple_deleted = 1;
                break;
        default:
        	not_deleted = 1;
                break;
	}

Yes, using switch makes the code easier to read. In fact, I put explicitly the
condition when there are no delete (cases 2, 4, 6 and 7) because there  
are cases
when d->stagemask can take other number than 1..7. For example:

git init git &&
cd git &&
test_commit foo main.txt foo &&
git branch second_branch &&
git mv main.txt sub_master.txt &&
git commit -m "main.txt renamed in sub_master.txt" &&
git checkout second_branch &&
git mv main.txt sub_second.txt &&
git commit -m "main.txt renamed in sub_second.txt" &&
git merge master &&
git add sub_master.txt &&
git add sub_second.txt

At this point, the output of git status shows in unmerged paths the  
file main.txt
that is marked as 'both deleted' so the number of elements in  
s->change.items should
be 1. However, in this case, there are 2 elements. One is about the  
file main.txt and
its stagemask is '1' (as expected) but the stagemask of the other  
element (I don't
know its origin) is '0'.
Anyway, the new code becomes something like:

switch (d->stagemask) {
case 0:
	break;
case 1:
	both_deleted = 1;
	break;
case 3:
case 5:
	del_mod_conflict = 1;
	break;
default:
	not_deleted = 1;
	break;
}

Though I don't know if d->stagemask can take values such as 8, 9, etc.