[PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[William Duclot]

The git UI can be improved by addressing the error messages to those
they help: inexperienced and casual git users. To this intent, it is
helpful to make sure the terms used in those messages can be understood
by this segment of users, and that they guide them to resolve the
problem.

In particular, failure to apply a patch during a git rebase is a common
problem that can be very destabilizing for the inexperienced user. It is
important to lead them toward the resolution of the conflict (which is a
3-steps process, thus complex) and reassure them that they can escape a
situation they can't handle with "--abort". This commit answer those two
points by detailling the resolution process and by avoiding cryptic git
linguo.

Signed-off-by: William Duclot <william.duclot@gmail.com>
---
While I do not expect that this V1 wording will be to the liking of
everyone, I think (know?) that the heart of this patch isn't something
that I'm the only one bothered with :) I'd very much like to hear your
opinions about it

 git-rebase.sh | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 2cf73b88e..50457f687 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -55,9 +55,10 @@ LF='
 '
 ok_to_skip_pre_rebase=
 resolvemsg="
-$(gettext 'When you have resolved this problem, run "git rebase --continue".
-If you prefer to skip this patch, run "git rebase --skip" instead.
-To check out the original branch and stop rebasing, run "git rebase --abort".')
+$(gettext 'Resolve this conflict manually, mark it as resolved with "git add <conflicted_file>",
+then run "git rebase --continue".
+You can instead skip this commit: run "git rebase --skip".
+To stop the whole rebasing and get back to your pre-rebase state, run "git rebase --abort".')
 "
 unset onto
 unset restrict_revision
-- 
2.13.0

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[Junio C Hamano]

William Duclot <william.duclot@gmail.com> writes:

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 2cf73b88e..50457f687 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -55,9 +55,10 @@ LF='
>  '
>  ok_to_skip_pre_rebase=
>  resolvemsg="
> -$(gettext 'When you have resolved this problem, run "git rebase --continue".
> -If you prefer to skip this patch, run "git rebase --skip" instead.
> -To check out the original branch and stop rebasing, run "git rebase --abort".')
> +$(gettext 'Resolve this conflict manually, mark it as resolved with "git add <conflicted_file>",
> +then run "git rebase --continue".
> +You can instead skip this commit: run "git rebase --skip".
> +To stop the whole rebasing and get back to your pre-rebase state, run "git rebase --abort".')
>  "

I find the updated one easier to follow in general.
Disecting the phrases in the above:

 - The original said "When you have resolved this problem", without
   giving a guidance how to resolve, and without saying what the
   problem is.  The updated one says "conflict" to clarify the
   "problem", and suggests "git add" as the tool to use after a
   manual resolition.  

   Modulo that there are cases where "git rm" is the right tool, the
   updated one is strict improvement.

 - The original said "If you prefer to skip" and the updated one
   says "You can instead skip".  Neither gives any guidance to
   decide when it is the right thing to skip, but probably that is
   not needed.  The updated one is shorter, which is a plus ;-)

 - The original said "to check out the original branch and stop
   rebasing", and the updated one says "to stop and get back to",
   which is in a more logical order.  

   "the whole rebasing" used as a noun feels something is missing
   there, though.  I wonder if "To get back to the state before you
   started 'rebase -i', run 'git rebase --abort'" is sufficient,
   without saying anything further about abandoning the rebase in
   progress (i.e. "and stop rebasing" or "stop the whole rebasing").

Thanks.

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[William Duclot]

Junio C Hamano writes:
> William Duclot <william.duclot@gmail.com> writes:
> 
> > diff --git a/git-rebase.sh b/git-rebase.sh
> > index 2cf73b88e..50457f687 100755
> > --- a/git-rebase.sh
> > +++ b/git-rebase.sh
> > @@ -55,9 +55,10 @@ LF='
> >  '
> >  ok_to_skip_pre_rebase=
> >  resolvemsg="
> > -$(gettext 'When you have resolved this problem, run "git rebase --continue".
> > -If you prefer to skip this patch, run "git rebase --skip" instead.
> > -To check out the original branch and stop rebasing, run "git rebase --abort".')
> > +$(gettext 'Resolve this conflict manually, mark it as resolved with "git add <conflicted_file>",
> > +then run "git rebase --continue".
> > +You can instead skip this commit: run "git rebase --skip".
> > +To stop the whole rebasing and get back to your pre-rebase state, run "git rebase --abort".')
> >  "
> 
> I find the updated one easier to follow in general.
> Disecting the phrases in the above:
> 
>  - The original said "When you have resolved this problem", without
>    giving a guidance how to resolve, and without saying what the
>    problem is.  The updated one says "conflict" to clarify the
>    "problem", and suggests "git add" as the tool to use after a
>    manual resolition.  
> 
>    Modulo that there are cases where "git rm" is the right tool, the
>    updated one is strict improvement.

I also wrote "<conflicted_file>" when there could be several. Maybe
'mark it as resolved with "git add/rm"' would be a better (and shorter)
formulation?

>  - The original said "to check out the original branch and stop
>    rebasing", and the updated one says "to stop and get back to",
>    which is in a more logical order.  
> 
>    "the whole rebasing" used as a noun feels something is missing
>    there, though.  I wonder if "To get back to the state before you
>    started 'rebase -i', run 'git rebase --abort'" is sufficient,
>    without saying anything further about abandoning the rebase in
>    progress (i.e. "and stop rebasing" or "stop the whole rebasing").

Definitely seems clearer to me: straight to the point.

> Thanks.

Happy to see this patch seems interesting to you. I feel like a lot of
git messages could be improved this way to offer a UI more welcoming to
inexperienced user (which is a *broad* segment of users). But I am not
aware of the cost of translation of this kind of patch: would several
patches like this one be welcomed?

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[Junio C Hamano]

William Duclot <william.duclot@gmail.com> writes:

>>  - The original said "When you have resolved this problem", without
>>    giving a guidance how to resolve, and without saying what the
>>    problem is.  The updated one says "conflict" to clarify the
>>    "problem", and suggests "git add" as the tool to use after a
>>    manual resolition.  
>> 
>>    Modulo that there are cases where "git rm" is the right tool, the
>>    updated one is strict improvement.
>
> I also wrote "<conflicted_file>" when there could be several. Maybe
> 'mark it as resolved with "git add/rm"' would be a better (and shorter)
> formulation?

Another potential source of confusion is if we are seeing "a"
conflict, or multiple ones.  I'd say it is OK to treat the whole
thing as "a conflict" that Git needs help with by the user editing
multiple files and using multiple "git add" or "git rm".  So "mark
it as resolved with 'git add/rm'" is fine, I would think, but
anything that I say about UI's understandability to new people needs
to be taken with a large grain of salt ;-).

> ... I feel like a lot of git messages could be improved this way
> to offer a UI more welcoming to inexperienced user (which is a
> *broad* segment of users). But I am not aware of the cost of
> translation of this kind of patch: would several patches like this
> one be welcomed?

Surely, as long as I can depend on other reviewers who are more
passionate about end-user experience than I am, I'll take such
patches with their help.

Thanks.

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[Philip Oakley]

From: "Junio C Hamano" <gitster@pobox.com>
Sent: Wednesday, July 12, 2017 10:29 PM
> William Duclot <william.duclot@gmail.com> writes:
>
>>>  - The original said "When you have resolved this problem", without
>>>    giving a guidance how to resolve, and without saying what the
>>>    problem is.  The updated one says "conflict" to clarify the
>>>    "problem", and suggests "git add" as the tool to use after a
>>>    manual resolition.
>>>
>>>    Modulo that there are cases where "git rm" is the right tool, the
>>>    updated one is strict improvement.
>>
>> I also wrote "<conflicted_file>" when there could be several. Maybe
>> 'mark it as resolved with "git add/rm"' would be a better (and shorter)
>> formulation?
>
> Another potential source of confusion is if we are seeing "a"
> conflict, or multiple ones.  I'd say it is OK to treat the whole
> thing as "a conflict" that Git needs help with by the user editing
> multiple files and using multiple "git add" or "git rm".  So "mark
> it as resolved with 'git add/rm'" is fine, I would think, but
> anything that I say about UI's understandability to new people needs
> to be taken with a large grain of salt ;-).
>
>> ... I feel like a lot of git messages could be improved this way
>> to offer a UI more welcoming to inexperienced user (which is a
>> *broad* segment of users). But I am not aware of the cost of
>> translation of this kind of patch: would several patches like this
>> one be welcomed?
>
> Surely, as long as I can depend on other reviewers who are more
> passionate about end-user experience than I am, I'll take such
> patches with their help.
>
> Thanks.

One of the other confusions I had / have (and I have a saved note to remind 
me) is when rebase stops with a conflict, and asks the user to "fix" it, 
then ues "--continue".

I always expect that Git will do the 'add' of the resolved conflict because 
that is what it would do normally as the next step after the merge.

I also had a similar issue with the --allow-empty case of 'nothing added to 
commit but untracked files present' where I had been expecting the commit to 
be simply omitted. You have to go through a reset dance before continuing.

Philip
[I'll be off line till Friday] 

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[Phillip Wood]

On 16/07/17 12:39, Philip Oakley wrote:
> 
> From: "Junio C Hamano" <gitster@pobox.com>
> Sent: Wednesday, July 12, 2017 10:29 PM
>> William Duclot <william.duclot@gmail.com> writes:
>>
>>>>  - The original said "When you have resolved this problem", without
>>>>    giving a guidance how to resolve, and without saying what the
>>>>    problem is.  The updated one says "conflict" to clarify the
>>>>    "problem", and suggests "git add" as the tool to use after a
>>>>    manual resolition.
>>>>
>>>>    Modulo that there are cases where "git rm" is the right tool, the
>>>>    updated one is strict improvement.
>>>
>>> I also wrote "<conflicted_file>" when there could be several. Maybe
>>> 'mark it as resolved with "git add/rm"' would be a better (and shorter)
>>> formulation?
>>
>> Another potential source of confusion is if we are seeing "a"
>> conflict, or multiple ones.  I'd say it is OK to treat the whole
>> thing as "a conflict" that Git needs help with by the user editing
>> multiple files and using multiple "git add" or "git rm".  So "mark
>> it as resolved with 'git add/rm'" is fine, I would think, but
>> anything that I say about UI's understandability to new people needs
>> to be taken with a large grain of salt ;-).
>>
>>> ... I feel like a lot of git messages could be improved this way
>>> to offer a UI more welcoming to inexperienced user (which is a
>>> *broad* segment of users). But I am not aware of the cost of
>>> translation of this kind of patch: would several patches like this
>>> one be welcomed?
>>
>> Surely, as long as I can depend on other reviewers who are more
>> passionate about end-user experience than I am, I'll take such
>> patches with their help.
>>
>> Thanks.
> 
> One of the other confusions I had / have (and I have a saved note to
> remind me) is when rebase stops with a conflict, and asks the user to
> "fix" it, then ues "--continue".
> 
> I always expect that Git will do the 'add' of the resolved conflict
> because that is what it would do normally as the next step after the merge.
> 
> I also had a similar issue with the --allow-empty case of 'nothing added
> to commit but untracked files present' where I had been expecting the
> commit to be simply omitted. You have to go through a reset dance before
> continuing.
> 
> Philip
> [I'll be off line till Friday]

git rebase --continue requiring one to git add first confuses/annoys me
too. I started a patch to autostage unstaged changes if they don't
contain conflict markers a couple of weeks ago, I'll clean it up and
post it later this week.

I also find it confusing that it asks me to edit the commit message for
picks, fixups and non-final squashes after conflicts. I can see that
perhaps one might want to amend the message to reflect any changes that
were made while resolving the conflicts but I've never had too. I'd
rather be able to pass --edit to rebase --continue if I needed to edit
the message in those cases. Looking through the code I think it would
require saving some extra state when rebase bails out on conflicts so
rebase --continue could tell if it should be asking the user to amend
the message.

Best Wishes

Phillip

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[Junio C Hamano]

Phillip Wood <phillip.wood@talktalk.net> writes:

> git rebase --continue requiring one to git add first confuses/annoys me
> too. I started a patch to autostage unstaged changes if they don't
> contain conflict markers a couple of weeks ago, I'll clean it up and
> post it later this week.

As long as "git rebase" will keep refusing to start in a working
tree with dirty files and/or index, this could be a good change.

But people _may_ be annoyed because they expect "--continue" to
remind them that some conflicts are not concluded with an explicit
"git add", and they would even feel that you made the command unsafe
if "--continue" just goes ahead by auto-adding their change that is
still work-in-progress.  Lack of conflict markers is not a sign that
a file is fully resolved (which they are used to signal by "git
add", and they do so per set of paths).

> I also find it confusing that it asks me to edit the commit message for
> picks, fixups and non-final squashes after conflicts. I can see that
> perhaps one might want to amend the message to reflect any changes that
> were made while resolving the conflicts but I've never had too. I'd
> rather be able to pass --edit to rebase --continue if I needed to edit
> the message in those cases. Looking through the code I think it would
> require saving some extra state when rebase bails out on conflicts so
> rebase --continue could tell if it should be asking the user to amend
> the message.

This is disruptive if done without a careful transition plan and
you'll annoy existing users who expect to be able to edit by
default.  Especially since "rebase" keeps going and potentially
rebuild many commits on top, by the time they realize the mistake of
not passing "--edit", it is too late and they will hate you for
forcing them rebase many commits again.

If these suggestions above were given while "rebase -i" was
developed, it might have made the end-user experience a better one
than what it currently is, but transitioning after the current
behaviour has long been established makes it much harder.

Re: [PATCH/RFC] rebase: make resolve message clearer for inexperienced users

[Phillip Wood]

On 24/07/17 21:53, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> git rebase --continue requiring one to git add first confuses/annoys me
>> too. I started a patch to autostage unstaged changes if they don't
>> contain conflict markers a couple of weeks ago, I'll clean it up and
>> post it later this week.
> 
> As long as "git rebase" will keep refusing to start in a working
> tree with dirty files and/or index, this could be a good change.
> 
> But people _may_ be annoyed because they expect "--continue" to
> remind them that some conflicts are not concluded with an explicit
> "git add", and they would even feel that you made the command unsafe
> if "--continue" just goes ahead by auto-adding their change that is
> still work-in-progress.  Lack of conflict markers is not a sign that
> a file is fully resolved (which they are used to signal by "git
> add", and they do so per set of paths).

Thanks for your comments, I've tried to address them in the message with 
the patches I sent earlier today [1]. In summary autostaging is opt-in 
and the conflict marker check isn't perfect but it's better than nothing 
and covers an important case where the user has simply overlooked a 
conflict.

>> I also find it confusing that it asks me to edit the commit message for
>> picks, fixups and non-final squashes after conflicts. I can see that
>> perhaps one might want to amend the message to reflect any changes that
>> were made while resolving the conflicts but I've never had too. I'd
>> rather be able to pass --edit to rebase --continue if I needed to edit
>> the message in those cases. Looking through the code I think it would
>> require saving some extra state when rebase bails out on conflicts so
>> rebase --continue could tell if it should be asking the user to amend
>> the message.
> 
> This is disruptive if done without a careful transition plan and
> you'll annoy existing users who expect to be able to edit by
> default.  Especially since "rebase" keeps going and potentially
> rebuild many commits on top, by the time they realize the mistake of
> not passing "--edit", it is too late and they will hate you for
> forcing them rebase many commits again.

I agree, I was imagining the new behaviour would be opt in via a config 
variable. Then if in the future there is a consensus to enable the new 
behaviour by default there would be a transition phase where users of 
the old behaviour would get a message telling that the behaviour is 
going to change in the future and what value to set the config variable 
to in order to keep the old behaviour if that's what they want.

> If these suggestions above were given while "rebase -i" was
> developed, it might have made the end-user experience a better one
> than what it currently is, but transitioning after the current
> behaviour has long been established makes it much harder.

Sadly I didn't even know git existed at the time rebase was first added.

Best Wishes

Phillip


[1] 
https://public-inbox.org/git/20170726102720.15274-2-phillip.wood@talktalk.net/T/#u