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

[Junio C Hamano <gitster@pobox.com>]

Kong Lucien <Lucien.Kong@ensimag.imag.fr> 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").