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

From: Junio C Hamano <gitster@pobox.com>
Date: May 30, 2012, 18:44 UTC
Message-ID: <7vipfdzeyu.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1338384216-18782-3-git-send-email-Lucien.Kong@ensimag.imag.fr>

Kong Lucien <Lucien.Kong@ensimag.imag.fr> writes:

Show 22 quoted lines

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

Show 10 quoted lines

>  	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").