Re: [RFC 1/3] wt-status: Make conflict hint message more consistent with other hints

2014-02-26 Thread Jonathan Nieder 
Hi,

Andrew Wong wrote:

 [Subject: wt-status: Make conflict hint message more consistent with other 
 hints]

Thanks for working on this.

Could you include a little more detail?  What other hints is this
making the message more consistent with?

Ideally the commit message would include a quick sample interaction,
so the reviewer could see the user going Wha? and then look at the
patch to see how it resolves the confusion.

[...]
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -899,7 +899,7 @@ static void show_merge_in_progress(struct wt_status *s,
   status_printf_ln(s, color, _(You have unmerged paths.));
   if (s-hints)
   status_printf_ln(s, color,
 - _(  (fix conflicts and run \git commit\)));
 + _(  (fix conflicts, and use \git commit\ to 
 conclude the merge)));

Quick thoughts:

 - The comma just moves the message closer to the right margin.  I think
   it makes the message less readable.

 - What else would git commit do other than concluding the merge?
   What confusion is this meant to prevent?

 - Would introducing a new git merge --continue command help?

   Advantages: (1) the name of the command makes it obvious what
   it does; (2) the command could check that there is actually
   a merge in progress, helping the user when the state is not
   what they think; (3) consistency with git cherry-pick --abort /
   git cherry-pick --continue.

   Disadvantage: redundancy (but see (2) above).

Hope that helps,
Jonathan
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] wt-status: Make conflict hint message more consistent with other hints

2014-02-26 Thread Andrew Wong 
On Wed, Feb 26, 2014 at 3:37 PM, Junio C Hamano gits...@pobox.com wrote:
 I see that you are trying to match the phrasing used in the other
 side of this if/else (which is outside the context of the posted
 patch).  Over there we say ... to conclude merge while the new
 text says ... to conclude THE merge.  Don't we want to match them?

Ah, good catch. My mind just read it as conclude THE merge, even
though the word wasn't there. Let's add the the in. :)

 For those who did not look beyond the context of the patch text, as
 I had to look these up to convince myself that the proposed change
 is a good one.  This function is only called when we see MERGE_HEAD,
 so unmerged here can come only from a failed merge, not other
 mergy operations like am, cherry-pick, revert, etc. and telling the
 user that 'commit' will conclude the merge will not be misleading
 (unless you count 'git commit' will conclude a conflicted 'git pull'
 as misleading, and I of course do not).

I'll update the commit message to explain that I'm match the other
side of the if/else.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html