You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2013/05/09 16:51:35 UTC
svn commit: r1480669 - /subversion/trunk/subversion/svn/conflict-callbacks.c
Author: stsp
Date: Thu May 9 14:51:35 2013
New Revision: 1480669
URL: http://svn.apache.org/r1480669
Log:
If a merged property value exists, make the 'dc' conflict prompt option use
the merged value as 'mine' for display. Else, users might get confused if
they edit a property, run 'dc', and see the value from before the edit.
* subversion/svn/conflict-callbacks.c
(show_prop_conflict, merge_prop_conflict): Add merged_abspath parameter.
Use it as 'my' version if non-NULL.
(edit_prop_conflict): For now, pass NULL for the merged file to
merge_prop_conflict(), so that repeated edits always start with
the same original values. This behaviour can be reconsidered later.
(handle_prop_conflict): Pass the path to the merged file, which might be
NULL, to show_prop_conflict().
Modified:
subversion/trunk/subversion/svn/conflict-callbacks.c
Modified: subversion/trunk/subversion/svn/conflict-callbacks.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/svn/conflict-callbacks.c?rev=1480669&r1=1480668&r2=1480669&view=diff
==============================================================================
--- subversion/trunk/subversion/svn/conflict-callbacks.c (original)
+++ subversion/trunk/subversion/svn/conflict-callbacks.c Thu May 9 14:51:35 2013
@@ -230,11 +230,15 @@ show_conflicts(const svn_wc_conflict_des
/* Perform a 3-way merge of the conflicting values of a property,
* and write the result to the OUTPUT stream.
*
+ * If MERGED_ABSPATH is non-NULL, use it as 'my' version instead of
+ * DESC->MY_ABSPATH.
+ *
* Assume the values are printable UTF-8 text.
*/
static svn_error_t *
merge_prop_conflict(svn_stream_t *output,
const svn_wc_conflict_description2_t *desc,
+ const char *merged_abspath,
apr_pool_t *pool)
{
const char *base_abspath = desc->base_abspath;
@@ -262,11 +266,14 @@ merge_prop_conflict(svn_stream_t *output
options->ignore_eol_style = TRUE;
SVN_ERR(svn_diff_file_diff3_2(&diff,
- base_abspath, my_abspath, their_abspath,
+ base_abspath,
+ merged_abspath ? merged_abspath : my_abspath,
+ their_abspath,
options, pool));
SVN_ERR(svn_diff_file_output_merge2(output, diff,
base_abspath,
- my_abspath,
+ merged_abspath ? merged_abspath
+ : my_abspath,
their_abspath,
_("||||||| ORIGINAL"),
_("<<<<<<< MINE"),
@@ -280,16 +287,20 @@ merge_prop_conflict(svn_stream_t *output
/* Display the conflicting values of a property as a 3-way diff.
*
+ * If MERGED_ABSPATH is non-NULL, show it as 'my' version instead of
+ * DESC->MY_ABSPATH.
+ *
* Assume the values are printable UTF-8 text.
*/
static svn_error_t *
show_prop_conflict(const svn_wc_conflict_description2_t *desc,
+ const char *merged_abspath,
apr_pool_t *pool)
{
svn_stream_t *output;
SVN_ERR(svn_stream_for_stdout(&output, pool));
- SVN_ERR(merge_prop_conflict(output, desc, pool));
+ SVN_ERR(merge_prop_conflict(output, desc, merged_abspath, pool));
return SVN_NO_ERROR;
}
@@ -366,7 +377,7 @@ edit_prop_conflict(const char **merged_f
result_pool, scratch_pool));
merged_prop = svn_stream_from_aprfile2(file, TRUE /* disown */,
scratch_pool);
- SVN_ERR(merge_prop_conflict(merged_prop, desc, scratch_pool));
+ SVN_ERR(merge_prop_conflict(merged_prop, desc, NULL, scratch_pool));
SVN_ERR(svn_stream_close(merged_prop));
SVN_ERR(svn_io_file_flush_to_disk(file, scratch_pool));
SVN_ERR(open_editor(&performed_edit, file_path, b, scratch_pool));
@@ -1001,7 +1012,7 @@ handle_prop_conflict(svn_wc_conflict_res
}
else if (strcmp(opt->code, "dc") == 0)
{
- SVN_ERR(show_prop_conflict(desc, scratch_pool));
+ SVN_ERR(show_prop_conflict(desc, merged_file_path, scratch_pool));
}
else if (strcmp(opt->code, "e") == 0)
{
Re: svn commit: r1480669 - interactive 'dc' and 'edit' for prop conflicts
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> On Mon, May 13, 2013 at 10:10:58PM +0100, Julian Foad wrote:
>>> Author: stsp
>>
>>> URL: http://svn.apache.org/r1480669
>>> Log:
>>> If a merged property value exists, make the 'dc' conflict prompt
>>> option use the merged value as 'mine' for display. Else, users might
>>> get confused if they edit a property, run 'dc', and see the value from
>>> before the edit.
>>
>> Hi Stefan. I see how it could be confusing, but now it is inconsistent
>> with text conflict handling:
>>
>> | text conflict | prop conflict
>> -------------+----------------+------------------------
>> 'dc' shows | original text | edited val
>> 'e' | edited text | conflict hunk with original val
>
> For text conflicts, 'e' shows edited text + conflict markers,
> doesn't it?
> Does your 'edited text' imply the possible presence of conflict markers?
After a previous edit, 'e' simply opens the same file again, so it has whatever text and/or markings you left in it. (The first time you 'edit', the text is whatever was produced by the failed merge attempt, so it has conflict markers in it if your merge tool configuration is the default or similar.) This is different from properties, where each time you choose 'e' it recreates the original conflict, discarding any previous edit.
> In the log message of r1480669, I've hinted at the possibility to show
> the same for prop conflicts in the future, because I'm not 100% happy
> with the current behaviour either:
> (edit_prop_conflict): For now, pass NULL for the merged file to
> merge_prop_conflict(), so that repeated edits always start with
> the same original values. This behaviour can be reconsidered later.
>
>> and also inconsistent with the 'select my version' option:
>>
>> [[[
>> Conflict for property 'p1' discovered on 'foo'.
>> local edit, incoming edit upon update
>> Select: (p) postpone, (mf) my version, (tf) their version,
>> (dc) display conflict, (e) edit property, (r) resolved,
>> (q) quit resolution, (h) help: dc
>> <<<<<<< MINE
>> edited prop val
>> ||||||| ORIGINAL
>> v=======
>> v2>>>>>>> THEIRS
>> Select: (p) postpone, (mf) my version, (tf) their version,
>> (dc) display conflict, (e) edit property, (r) resolved,
>> (q) quit resolution, (h) help: mf
>> [...]
>> $ svn pl -v foo
>> Properties on 'foo':
>> p1
>> me
>> ]]]
>
> Ugh, that's a big ugly alright.
>
>> I'm going to vote +1 on the r1477294 group of back-ports that you
>> proposed, where this change is listed as a 'small follow-up fix',
>> because basically the changes are good and this is just a small
>> inconsistency by comparison, and because the proposal involves a UI
>> change which needs to get into 1.8.0 if it's going to get anywhere
>> soon.
>>
>> But I think we should do something to make this consistent with text
> conflicts, one way or the other.
>
> I agree. We're not going to be able to fix many of the property conflict
> handling quirks for 1.8. I intend to make further improvements for 1.9
> and later, and will keep your points in mind.
>
> Thanks!
Thanks.
- Julian
Re: svn commit: r1480669 - interactive 'dc' and 'edit' for prop
conflicts
Posted by Stefan Sperling <st...@elego.de>.
On Mon, May 13, 2013 at 10:10:58PM +0100, Julian Foad wrote:
> > Author: stsp
>
> > URL: http://svn.apache.org/r1480669
> > Log:
> > If a merged property value exists, make the 'dc' conflict prompt option
> > use the merged value as 'mine' for display. Else, users might get confused
> > if they edit a property, run 'dc', and see the value from before the edit.
>
> Hi Stefan. I see how it could be confusing, but now it is inconsistent with text conflict handling:
>
> | text conflict | prop conflict
> -------------+----------------+------------------------
> 'dc' shows | original text | edited val
> 'e' | edited text | conflict hunk with original val
For text conflicts, 'e' shows edited text + conflict markers, doesn't it?
Does your 'edited text' imply the possible presence of conflict markers?
In the log message of r1480669, I've hinted at the possibility to show
the same for prop conflicts in the future, because I'm not 100% happy
with the current behaviour either:
(edit_prop_conflict): For now, pass NULL for the merged file to
merge_prop_conflict(), so that repeated edits always start with
the same original values. This behaviour can be reconsidered later.
> and also inconsistent with the 'select my version' option:
>
> [[[
> Conflict for property 'p1' discovered on 'foo'.
> local edit, incoming edit upon update
> Select: (p) postpone, (mf) my version, (tf) their version,
> (dc) display conflict, (e) edit property, (r) resolved,
> (q) quit resolution, (h) help: dc
> <<<<<<< MINE
> edited prop val
> ||||||| ORIGINAL
> v=======
> v2>>>>>>> THEIRS
> Select: (p) postpone, (mf) my version, (tf) their version,
> (dc) display conflict, (e) edit property, (r) resolved,
> (q) quit resolution, (h) help: mf
> [...]
> $ svn pl -v foo
> Properties on 'foo':
> p1
> me
> ]]]
Ugh, that's a big ugly alright.
> I'm going to vote +1 on the r1477294 group of back-ports that you
> proposed, where this change is listed as a 'small follow-up fix',
> because basically the changes are good and this is just a small
> inconsistency by comparison, and because the proposal involves a UI
> change which needs to get into 1.8.0 if it's going to get anywhere
> soon.
>
> But I think we should do something to make this consistent with text conflicts, one way or the other.
I agree. We're not going to be able to fix many of the property conflict
handling quirks for 1.8. I intend to make further improvements for 1.9
and later, and will keep your points in mind.
Thanks!
Re: svn commit: r1480669 - interactive 'dc' and 'edit' for prop conflicts
Posted by Julian Foad <ju...@btopenworld.com>.
> Author: stsp
> URL: http://svn.apache.org/r1480669
> Log:
> If a merged property value exists, make the 'dc' conflict prompt option
> use the merged value as 'mine' for display. Else, users might get confused
> if they edit a property, run 'dc', and see the value from before the edit.
Hi Stefan. I see how it could be confusing, but now it is inconsistent with text conflict handling:
| text conflict | prop conflict
-------------+----------------+------------------------
'dc' shows | original text | edited val
'e' | edited text | conflict hunk with original val
and also inconsistent with the 'select my version' option:
[[[
Conflict for property 'p1' discovered on 'foo'.
local edit, incoming edit upon update
Select: (p) postpone, (mf) my version, (tf) their version,
(dc) display conflict, (e) edit property, (r) resolved,
(q) quit resolution, (h) help: dc
<<<<<<< MINE
edited prop val
||||||| ORIGINAL
v=======
v2>>>>>>> THEIRS
Select: (p) postpone, (mf) my version, (tf) their version,
(dc) display conflict, (e) edit property, (r) resolved,
(q) quit resolution, (h) help: mf
[...]
$ svn pl -v foo
Properties on 'foo':
p1
me
]]]
I'm going to vote +1 on the r1477294 group of back-ports that you proposed, where this change is listed as a 'small follow-up fix', because basically the changes are good and this is just a small inconsistency by comparison, and because the proposal involves a UI change which needs to get into 1.8.0 if it's going to get anywhere soon.
But I think we should do something to make this consistent with text conflicts, one way or the other.
- Julian
>
> * subversion/svn/conflict-callbacks.c
> (show_prop_conflict, merge_prop_conflict): Add merged_abspath parameter.
> Use it as 'my' version if non-NULL.
> (edit_prop_conflict): For now, pass NULL for the merged file to
> merge_prop_conflict(), so that repeated edits always start with
> the same original values. This behaviour can be reconsidered later.
> (handle_prop_conflict): Pass the path to the merged file, which might be
> NULL, to show_prop_conflict().