Kong Lucien <Lucien.Kong@xxxxxxxxxxxxxxx> 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 (!advice_status_hints)
> return;
> if (s->whence != FROM_COMMIT)
> @@ -142,7 +160,16 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
> status_printf_ln(s, c, _(" (use \"git reset %s <file>...\" to unstage)"), s->reference);
> else
> status_printf_ln(s, c, _(" (use \"git rm --cached <file>...\" to unstage)"));
> - status_printf_ln(s, c, _(" (use \"git add/rm <file>...\" as appropriate to mark resolution)"));
> +
> + if (!both_deleted_flag)
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").
--
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