Re: [PATCHv6 1/4] wt-status.*: better advices for git status added

[konglu@minatec.inpg.fr]

Junio C Hamano <gitster@pobox.com> a écrit :

>> +		if (advice_status_hints)
>> +			status_printf_ln(s, color,
>> +				_("  (use \"git am --abort\" to restore the original branch)"));
>> +	} else if (advice_status_hints) {
>> +		status_printf_ln(s, color,
>> +			_("  (when you have fixed this problem run \"git am --resolved\")"));
>> +		status_printf_ln(s, color,
>> +			_("  (use \"git am --skip\" to skip this patch)"));
>> +		status_printf_ln(s, color,
>> +			_("  (use \"git am --abort\" to restore the original branch)"));
>> +	}
>
> I think the structure can simply be:
>
> 	if (am_empty_patch)
> 		"The current patch is empty.";
> 	if (advice_status_hints) {
> 		"  (use --abort to restore)";
> 		"  (use --skip to skip)";
>                 if (!am_empty_patch)
> 			"  (use --resolved after you are done)";
> 	}
>
> Note that I am not suggesting to change the wording of the message
> in the above; just showing the flow-structure.
>
> Also when you are in the middle of "git am -3", there may be
> unmerged entries in the index; do we want to suggest "add/rm" to
> resolve to fill in the missing detail of "fix" in your "when you
> have fixed this problem" message?
>
> We may want to decide how detailed we want to make the help texts;
> the same fuzziness exists in "fix conflicts and then" at the
> beginning of show_rebase_in_progress().

In fact, to detail more "fix" in this case by suggesting "add/rm"
would be repetitive, as this advice is already given under the block
"unmerged paths":

# Not currently on any branch.
# You are currently rebasing.
#   (fix conflicts and then run "git rebase --continue")
#   (use "git rebase --skip" to skip this patch)
#   (use "git rebase --abort" to check out the original branch)
#
# Unmerged paths:
#   (use "git reset HEAD <file>..." to unstage)
#   (use "git add <file>..." to mark resolution)
#
#	both modified:      main.txt
#
no changes added to commit (use "git add" and/or "git commit -a")

>> +static void show_rebase_in_progress(struct wt_status *s,
>> +				struct wt_status_state *state,
>> +				const char *color)
>> +{
>> +	if (has_unmerged(s)) {
>> +		status_printf_ln(s, color, _("You are currently rebasing."));
>> +		if (advice_status_hints) {
>> +			status_printf_ln(s, color,
>> +				_("  (fix conflicts and then run \"git rebase --continue\")"));
>
>> +			status_printf_ln(s, color,
>> +				_("  (use \"git rebase --skip\" to skip this patch)"));
>> +			status_printf_ln(s, color,
>> +				_("  (use \"git rebase --abort\" to check out the original branch)"));
>> +		}
>> +	} else if (state->rebase_in_progress) {
>> +		status_printf_ln(s, color, _("You are currently rebasing."));
>> +		if (advice_status_hints)
>> +			status_printf_ln(s, color,
>> +				_("  (all conflicts fixed: run \"git rebase --continue\")"));
>> +	} else {
>> +		status_printf_ln(s, color, _("You are currently editing a commit  
>> during a rebase."));
>> +		if (advice_status_hints && !s->amend) {
>> +			status_printf_ln(s, color,
>> +				_("  (use \"git commit --amend\" to amend the current commit)"));
>> +			status_printf_ln(s, color,
>> +				_("  (use \"git rebase --continue\" once you are satisfied  
>> with your changes)"));
>> +		}
>
> This last "else" block is taken when running "rebase -i" and there
> is no longer any unmerged index entry.  I wonder if the message from
> the first printf_ln needs to be clarified further depending on the
> context.
>
> Specifically, in this sequence:
>
> 	- the user marked a commit as "pick";
>         - replaying of that commit resulted in conflicts;
>         - the user edited files and used add/rm to resolve conflicts;
>         - the user did one of these:
> 	  1. "git status"
>           2. "git commit" without "--amend"
>           3. "git commit --amend"
>
> can this message come up?
>
> In such a case, "You are currently editing a commit" is actively
> wrong.  The user has finished resolving the conflict and are about
> to record the result.  Also, "git status" and "git commit" without
> "--amend" are both sensible commands in this situation, but running
> "git commit --amend" is likely to be a mistake, no?

True, the last "else" block is not supposed to be taken. The correct
message that should be displayed is the first "else if" block, saying
just that all conflicts are fixed and to run rebase --continue.