Re: svn commit: r34329 - in branches/tc_url_rev/subversion: include libsvn_wc svn

Stefan Sperling wrote:
> On Fri, Nov 21, 2008 at 07:37:54PM -0800, neels_at_tigris.org wrote:
>> Author: neels
>> Date: Fri Nov 21 19:37:54 2008
>> New Revision: 34329
>>
>> +const char *
>> +svn_wc_operation_str(svn_wc_operation_t operation, apr_pool_t *pool)
>> +{
>> + switch(operation){
>> + case svn_wc_operation_update:
>> + return SVN_WC__OPERATION_UPDATE;
>> + case svn_wc_operation_switch:
>> + return SVN_WC__OPERATION_SWITCH;
>> + case svn_wc_operation_merge:
>> + return SVN_WC__OPERATION_MERGE;
>
> You haven't marked the above strings for translation.

Say, do the svn commands "update", "switch", etc. get *translated*??

...
>> svn_cmdline_printf(pool,
>> _("Tree conflict: %s\n"
>> - " incoming version: %s\n"
>> - " local version: %s"),
>> - desc, incoming_version, local_version);
>> + " Older version: %s\n"
>> + " Their version: %s"),
>> + desc, older_version, their_version);
>
> I'm wondering why we use "older" and "their" for merge.
> It only makes sense for update.
>
> In case of merge, wouldn't it be more straightforward to say
> "merge-right" and "merge-left"?

True. I was thinking something like that but it wasn't as clear as you said
it. Thanks.

>
>> }
>>
>> /* Print extra newline separator. */
>>
>> Modified: branches/tc_url_rev/subversion/svn/tree-conflicts.c
>> URL: http://svn.collab.net/viewvc/svn/branches/tc_url_rev/subversion/svn/tree-conflicts.c?pathrev=34329&r1=34328&r2=34329
>> ==============================================================================
>
> This hunk below should have been done on trunk, because
> it's got nothing do to with the topic of the branch.
>
> Please revert this hunk on the branch.
> And I'm -1 on applying it to trunk anyway, see below.
>
>> --- branches/tc_url_rev/subversion/svn/tree-conflicts.c Fri Nov 21 15:55:47 2008 (r34328)
>> +++ branches/tc_url_rev/subversion/svn/tree-conflicts.c Fri Nov 21 19:37:54 2008 (r34329)
>> @@ -25,7 +25,7 @@
>> #include "svn_private_config.h"
>>
>> static const char *
>> -select_action(const svn_wc_conflict_description_t *conflict)
>> +action_str(const svn_wc_conflict_description_t *conflict)
>> {
>> switch (conflict->action)
>> {
>> @@ -35,13 +35,13 @@ select_action(const svn_wc_conflict_desc
>> case svn_wc_conflict_action_add:
>> return _("add");
>> case svn_wc_conflict_action_delete:
>> - return _("delete");
>> + return _("delete or move");
>
> I don't think this is a good idea.
>
> Part of what I did not like about the old long human readable
> descriptions was that they went out of their way to explain
> to users that a move may be involved.
>
> I am now thinking we should not pretend to be smarter than we
> really are. Let's just say "delete", because we really don't
> know any better. And people *know* that Subversion represents
> moves as delete+add. We don't hide this fact in the update/status
> output either. Just saying "delete" is more consistent.
> Mercurial also does the same, btw., it just says delete.
>
> Once the working copy has been made aware of moves, we can
> adjust the output without having to lie about what we know.

I was thinking of users that don't know much about nothing. "What? I didn't
*remove* that file, did I???"

Then again, I don't mind it all that much. I'll revert that.

> - *desc = apr_psprintf(pool, _("incoming %s, local %s"), action, reason);
> + *desc = apr_psprintf(pool, _("local %s, incoming %s (upon %s)."),
> + reason, action, operation);
> I liked it better without parentheses and full-stop, and I don't think

I like it better *with*. But whatever. I'll change it but quit bikeshedding. :)

> that printing the operation which produced the conflict really makes
> much sense.

I think it does. It tells people what they were trying to do, and what they
need to do to fix it. If we merge into an update-conflicted working copy,
and that merge creates more tree-conflicts... let's just leave it there.

>
> The typical user will still remember what they were doing just before
> the conflicts came about. It's not like people run an update/merge and
> resolve conflicts 2 weeks later.

And why prevent people from reading which conflict came from which
operation? It's not another line of output and we're carrying the OPERATION
information around everywhere already. What *is* your problem?

Thanks for the review!
~Neels