Re: Merge bug causes changesets to be applied although this should not be the case

[Johan Corveleyn <jc...@gmail.com>]

On Thu, Apr 11, 2013 at 9:59 PM, Julian Foad <ju...@btopenworld.com> wrote:
> Hi Christoph.
>
> Christoph Schulz wrote:
>> [...] silently ignoring the deletion of a non-existing file part is not a
>> sound decision in my opinion.
>
>> [...] the problem is that [...] no conflict is _detected_ at all.
>
> I agree that this behaviour is not good when you want strict conflict detection.
>
> This particular behaviour is just one of several heuristics in Subversion that aim to help the more casual user by producing a 'fairly obvious' output instead of being strict and flagging a conflict.  These heuristics include:
>
>   * If the target file content is already equal to the right-hand side of the merge source, then do nothing.  (That's this issue.)
>
>
>  * When adding a text hunk, if the same hunk appears already at the same
>  place (or at a similar place, as determined by the context-matching
> rules) then do nothing with that hunk.
>
>   * When adding a whole
> file or directory, if that file already exists (even if its content is
> different) then there is an internal option to not flag a conflict.
> (The 'adds_as_modification' parameter to svn_client_update4(), which the
>  svn command-line client sets true during 'svn update'.)
>
>   * In some cases, deleting a file that is already deleted is not considered a conflict.
>
> And there are probably more.
>
> In my
> opinion, the user need to be able to specify either 'strict' mode or
> 'guess what I probably want' mode, since both modes are useful in
> different situations.  The 'strict' mode should disable *all* of those heuristics and cause a conflict to be raised in those cases.
>
>
>> Stefan Sperling wrote:
>>>  If we are currently giving our users less choice during conflict
>>>  resolution than they should have, we need to fix that. If however
>>>  there is only one practical resolution for the conflict in question,
>>>  then I don't see a need to change Subversion's behaviour.
>
> Users have different needs when merging.  Some people merge in a loosely controlled setting, where the same change may have been made on two different branches independently, or patches may have been applied that haven't been recorded as merges, and so on.  In that kind of situation it can be most useful if the tool automatically chooses a reasonably likely result for each conflict.
>
> On the other hand, sometimes people merge in a tightly controlled setting, where perhaps each change entered the version control system on exactly one branch, and the mergeinfo accurately reflects what has and has not been merged, and so on.  The user expects the merge to apply a predictable set of changes.  The user may know that none of the changes are going to conflict unless the user has made a mistake, or perhaps the user knows that the only changes that might conflict are the changes that touch a certain set of files.  In this kind of situation, it is helpful if the tool can flag any unexpected situation, because quite likely a mistake has been made.
>
> Of course in 'strict' mode it will be useful for Subversion to offer an easy way for the user to select  the 'obvious' resolution for the conflict, but the more important thing is to be able to detect such conflicts.
>
> So I would like us to implement such a mode.
>
> (I have been thinking of the 'strict mode' for a couple of years already, but I still don't know the complete list of heuristics that we have at the moment.  I only became aware of this "don't merge if the file already looks like the right-hand side of the incoming change" heuristic a few weeks ago when I was digging through that part of the code.)
>
> The simplest implementation would have a single mode flag that just selects 'strict' or 'not strict'.  A slightly more sophisticated implementation could have a bunch of flags that individually control the various heuristics, and probably a top-level control to easily switch them all on or all off.  Either way, this would be a significant improvement.
>
> - Julian

FYI, another user complained about this on the users list (Fredrik
Orderud, CC'd) [1]. He has filed issue #4405 [2]. Perhaps some of the
interested parties here can take a closer look? Or maybe Fredrik or
anyone can start / continue discussion to go towards a detailed
specification of the behavior of such an optional 'strict' flag or
something like that ...

(no specific interest myself, just trying to tie some loose ends together)

[1] http://mail-archives.apache.org/mod_mbox/subversion-users/201308.mbox/%3CCAHGmiisrngb1sogdLhKkY+9ePuNqVjZ8V7J_LyfAkxezZ54fJw@mail.gmail.com%3E

[2] http://subversion.tigris.org/issues/show_bug.cgi?id=4405 (Merge
order affects final result for repeated added & deleted changes)

-- 
Johan

Re: [PATCH] Re: Merge bug causes changesets to be applied although this should not be the case

[Stefan Sperling <st...@elego.de>]

On Fri, Aug 09, 2013 at 05:53:26PM +0300, Daniel Shahaf wrote:
> Johan Corveleyn wrote on Fri, Aug 09, 2013 at 16:35:36 +0200:
> > Thanks for the patch. Don't be put off by the lack of response, some
> > people are on holiday (I'm writing this response from the side of a
> > swimming pool myself :-)). So it might take a couple more days /
> > weeks.
> 
> It would also help to add the [PATCH] tag so that devs who don't follow
> the thread realise there's a patch pending.  (Done.  The actual patch is
> in Fredrik's most recent email.)

And a [SWIMMINGPOOL] tag for those who aren't on vacation right
now... so we know to better skip that post.

[PATCH] Re: Merge bug causes changesets to be applied although this should not be the case

[Daniel Shahaf <da...@elego.de>]

Johan Corveleyn wrote on Fri, Aug 09, 2013 at 16:35:36 +0200:
> Thanks for the patch. Don't be put off by the lack of response, some
> people are on holiday (I'm writing this response from the side of a
> swimming pool myself :-)). So it might take a couple more days /
> weeks.

It would also help to add the [PATCH] tag so that devs who don't follow
the thread realise there's a patch pending.  (Done.  The actual patch is
in Fredrik's most recent email.)

Stylistically, the patch looks good... I haven't followed the thread or
tried to apply the patch, though.

Re: Merge bug causes changesets to be applied although this should not be the case

[Johan Corveleyn <jc...@gmail.com>]

On Tue, Aug 6, 2013 at 10:40 PM, Fredrik Orderud <fo...@gmail.com> wrote:
> On Tue, Aug 6, 2013 at 3:19 PM, Johan Corveleyn <jc...@gmail.com> wrote:
>>
>> FYI, another user complained about this on the users list (Fredrik
>> Orderud, CC'd) [1]. He has filed issue #4405 [2]. Perhaps some of the
>> interested parties here can take a closer look? Or maybe Fredrik or
>> anyone can start / continue discussion to go towards a detailed
>> specification of the behavior of such an optional 'strict' flag or
>> something like that ...
>>
>> (no specific interest myself, just trying to tie some loose ends together)
>>
>> [1]
>> http://mail-archives.apache.org/mod_mbox/subversion-users/201308.mbox/%3CCAHGmiisrngb1sogdLhKkY+9ePuNqVjZ8V7J_LyfAkxezZ54fJw@mail.gmail.com%3E
>>
>> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=4405 (Merge
>> order affects final result for repeated added & deleted changes)
>
>
> I've now written an XFAIL test for merging the same change change twice.
> Patch attached. The test fails as expected due to the lack of conflict. This
> is the first test I've ever written for subversion, so there are probably
> some improvement opportunities. I suspect that one weak spot is that a
> "greek-tree" structure is generated without being used in the test. Also,
> comparison of console output for merge results feels a little fragile.
>
> Please let me know if there are any comments to the patch, and I'll do my
> best to improve it. Otherwise, it would be great it the test could be
> integrated, so that issue #4405 can receive some test coverage.
>

Hi Fredrik,

Thanks for the patch. Don't be put off by the lack of response, some
people are on holiday (I'm writing this response from the side of a
swimming pool myself :-)). So it might take a couple more days /
weeks.

I've just taken a quick look at your patch (I have not tested it (away
from my development computer) to see if it fails for the right
reason). Overall it seems pretty good, but I think I'd make the test
expectations a little less fragile, by only using the minimal part of
output that needs to be verified (e.g. only 'C    file\n' or something
like that).

Have you ran the test yourself, and verified that it (x)fails for the
right reason (by examining the test output)?

Apart from that, I don't have much feedback, but I'm only tangentially
involved in this feature / discussion anyway. Perhaps some of the
other developers have more to add ...

-- 
Johan

Re: Merge bug causes changesets to be applied although this should not be the case

[Daniel Shahaf <da...@elego.de>]

Fredrik Orderud wrote on Sun, Aug 11, 2013 at 00:26:54 +0200:
> I've managed to rewrite the test script to use A/mu in the auto-generated
> greek tree, but I did not understand how to use svntest/actions.py to parse
> the output. Still, I took the liberty of attaching a 2nd revision of the
> failing XFAIL test to
> http://subversion.tigris.org/issues/show_bug.cgi?id=4405
> 
> Hope that this is ok for you.

Looks good to me, thanks.

Re: Merge bug causes changesets to be applied although this should not be the case

[Fredrik Orderud <fo...@gmail.com>]

On Sat, Aug 10, 2013 at 9:30 PM, Fredrik Orderud <fo...@gmail.com> wrote:

> On Fri, Aug 9, 2013 at 5:18 PM, Daniel Shahaf <da...@elego.de> wrote:
>
>> Fredrik Orderud wrote on Tue, Aug 06, 2013 at 22:40:02 +0200:
>>  > I've now written an XFAIL test for merging the same change change
>> twice.
>> > Patch attached. The test fails as expected due to the lack of conflict.
>> > This is the first test I've ever written for subversion, so there are
>> > probably some improvement opportunities. I suspect that one weak spot is
>> > that a "greek-tree" structure is generated without being used in the
>> test.
>> > Also, comparison of console output for merge results feels a little
>> fragile.
>>
>> Yes, agreed on both points.  You could use A/mu instead of creating a new
>> file, and use one of the svntest/actions.py helpers that parse the
>> output of merge/status instead of depending on the exact byte-by-byte
>> expected output.  More below.
>>
>> > Please let me know if there are any comments to the patch, and I'll do
>> my
>> > best to improve it. Otherwise, it would be great it the test could be
>> > integrated, so that issue #4405 can receive some test coverage.
>>
>> Please use text/plain MIME type.  This makes review easier.  Usually
>> *.txt extesion achieves this.
>>
>> I think the patch is correct *if* we agree that "Merge the same change
>> twice" should raise a conflict.  Prior discussion in this thread
>> indicates that in present svn that scenario is explicitly decided not to
>> be a conflict.  Therefore, I am not going to commit this patch.
>>
>> I think the best thing to do is to attach it to the issue tracker on the
>> issue tracking Julian's suggestion of a "strict conflicts" mode.  That
>> way, we can apply the patch once we start implementing the "strict"
>> mode.  (We tend not to change our svntest/*.py interfaces that much, so
>> I wouldn't be concerned about bitrot.)
>>
>> So, in summary: thanks for the patch, I think it's correct, but I'm not
>> going to apply it for the reasons above.
>>
>> Does that make sense?
>>
>
> Thank you for the response Daniel. I agree with your reasoning and will be
> satisfied with a "strict conflicts" mode in subversion. That's fine for
> me. :-)
>
> I will attempt to improve the patch based on Johan's and your feedback, and
> attach it to the issue-tracker sometime the next few days.
>

I've managed to rewrite the test script to use A/mu in the auto-generated
greek tree, but I did not understand how to use svntest/actions.py to parse
the output. Still, I took the liberty of attaching a 2nd revision of the
failing XFAIL test to
http://subversion.tigris.org/issues/show_bug.cgi?id=4405

Hope that this is ok for you.

Fredrik

Re: Merge bug causes changesets to be applied although this should not be the case

[Fredrik Orderud <fo...@gmail.com>]

On Sat, Aug 10, 2013 at 9:30 PM, Fredrik Orderud <fo...@gmail.com> wrote:

> On Fri, Aug 9, 2013 at 5:18 PM, Daniel Shahaf <da...@elego.de> wrote:
>
>> Fredrik Orderud wrote on Tue, Aug 06, 2013 at 22:40:02 +0200:
>>  > I've now written an XFAIL test for merging the same change change
>> twice.
>> > Patch attached. The test fails as expected due to the lack of conflict.
>> > This is the first test I've ever written for subversion, so there are
>> > probably some improvement opportunities. I suspect that one weak spot is
>> > that a "greek-tree" structure is generated without being used in the
>> test.
>> > Also, comparison of console output for merge results feels a little
>> fragile.
>>
>> Yes, agreed on both points.  You could use A/mu instead of creating a new
>> file, and use one of the svntest/actions.py helpers that parse the
>> output of merge/status instead of depending on the exact byte-by-byte
>> expected output.  More below.
>>
>> > Please let me know if there are any comments to the patch, and I'll do
>> my
>> > best to improve it. Otherwise, it would be great it the test could be
>> > integrated, so that issue #4405 can receive some test coverage.
>>
>> Please use text/plain MIME type.  This makes review easier.  Usually
>> *.txt extesion achieves this.
>>
>> I think the patch is correct *if* we agree that "Merge the same change
>> twice" should raise a conflict.  Prior discussion in this thread
>> indicates that in present svn that scenario is explicitly decided not to
>> be a conflict.  Therefore, I am not going to commit this patch.
>>
>> I think the best thing to do is to attach it to the issue tracker on the
>> issue tracking Julian's suggestion of a "strict conflicts" mode.  That
>> way, we can apply the patch once we start implementing the "strict"
>> mode.  (We tend not to change our svntest/*.py interfaces that much, so
>> I wouldn't be concerned about bitrot.)
>>
>> So, in summary: thanks for the patch, I think it's correct, but I'm not
>> going to apply it for the reasons above.
>>
>> Does that make sense?
>>
>
> Thank you for the response Daniel. I agree with your reasoning and will be
> satisfied with a "strict conflicts" mode in subversion. That's fine for
> me. :-)
>
> I will attempt to improve the patch based on Johan's and your feedback, and
> attach it to the issue-tracker sometime the next few days.
>

Re: Merge bug causes changesets to be applied although this should not be the case

[Fredrik Orderud <fo...@gmail.com>]

On Fri, Aug 9, 2013 at 5:18 PM, Daniel Shahaf <da...@elego.de> wrote:

> Fredrik Orderud wrote on Tue, Aug 06, 2013 at 22:40:02 +0200:
>  > I've now written an XFAIL test for merging the same change change twice.
> > Patch attached. The test fails as expected due to the lack of conflict.
> > This is the first test I've ever written for subversion, so there are
> > probably some improvement opportunities. I suspect that one weak spot is
> > that a "greek-tree" structure is generated without being used in the
> test.
> > Also, comparison of console output for merge results feels a little
> fragile.
>
> Yes, agreed on both points.  You could use A/mu instead of creating a new
> file, and use one of the svntest/actions.py helpers that parse the
> output of merge/status instead of depending on the exact byte-by-byte
> expected output.  More below.
>
> > Please let me know if there are any comments to the patch, and I'll do my
> > best to improve it. Otherwise, it would be great it the test could be
> > integrated, so that issue #4405 can receive some test coverage.
>
> Please use text/plain MIME type.  This makes review easier.  Usually
> *.txt extesion achieves this.
>
> I think the patch is correct *if* we agree that "Merge the same change
> twice" should raise a conflict.  Prior discussion in this thread
> indicates that in present svn that scenario is explicitly decided not to
> be a conflict.  Therefore, I am not going to commit this patch.
>
> I think the best thing to do is to attach it to the issue tracker on the
> issue tracking Julian's suggestion of a "strict conflicts" mode.  That
> way, we can apply the patch once we start implementing the "strict"
> mode.  (We tend not to change our svntest/*.py interfaces that much, so
> I wouldn't be concerned about bitrot.)
>
> So, in summary: thanks for the patch, I think it's correct, but I'm not
> going to apply it for the reasons above.
>
> Does that make sense?
>

Thank you for the response Daniel. I agree with your reasoning and will be
satisfied with a "strict conflicts" mode in subversion. That's fine for me.
:-)

I will attempt to improve the patch based on Johan's and your feedback, and
attach it to the issue-tracker sometime the next few days.

Best regards,
Fredrik

Re: Merge bug causes changesets to be applied although this should not be the case

[Daniel Shahaf <da...@elego.de>]

Fredrik Orderud wrote on Tue, Aug 06, 2013 at 22:40:02 +0200:
> On Tue, Aug 6, 2013 at 3:19 PM, Johan Corveleyn <jc...@gmail.com> wrote:
> 
> > FYI, another user complained about this on the users list (Fredrik
> > Orderud, CC'd) [1]. He has filed issue #4405 [2]. Perhaps some of the
> > interested parties here can take a closer look? Or maybe Fredrik or
> > anyone can start / continue discussion to go towards a detailed
> > specification of the behavior of such an optional 'strict' flag or
> > something like that ...
> >
> > (no specific interest myself, just trying to tie some loose ends together)
> >
> > [1]
> > http://mail-archives.apache.org/mod_mbox/subversion-users/201308.mbox/%3CCAHGmiisrngb1sogdLhKkY+9ePuNqVjZ8V7J_LyfAkxezZ54fJw@mail.gmail.com%3E
> >
> > [2] http://subversion.tigris.org/issues/show_bug.cgi?id=4405 (Merge
> > order affects final result for repeated added & deleted changes)
> >
> 
> I've now written an XFAIL test for merging the same change change twice.
> Patch attached. The test fails as expected due to the lack of conflict.
> This is the first test I've ever written for subversion, so there are
> probably some improvement opportunities. I suspect that one weak spot is
> that a "greek-tree" structure is generated without being used in the test.
> Also, comparison of console output for merge results feels a little fragile.
> 

Yes, agreed on both points.  You could use A/mu instead of creating a new
file, and use one of the svntest/actions.py helpers that parse the
output of merge/status instead of depending on the exact byte-by-byte
expected output.  More below.

> Please let me know if there are any comments to the patch, and I'll do my
> best to improve it. Otherwise, it would be great it the test could be
> integrated, so that issue #4405 can receive some test coverage.
> 
> Thanks in advance,
> Fredrik

Please use text/plain MIME type.  This makes review easier.  Usually
*.txt extesion achieves this.

I think the patch is correct *if* we agree that "Merge the same change
twice" should raise a conflict.  Prior discussion in this thread
indicates that in present svn that scenario is explicitly decided not to
be a conflict.  Therefore, I am not going to commit this patch.

I think the best thing to do is to attach it to the issue tracker on the
issue tracking Julian's suggestion of a "strict conflicts" mode.  That
way, we can apply the patch once we start implementing the "strict"
mode.  (We tend not to change our svntest/*.py interfaces that much, so
I wouldn't be concerned about bitrot.)

So, in summary: thanks for the patch, I think it's correct, but I'm not
going to apply it for the reasons above.

Does that make sense?

Thanks again,

Daniel

Re: Merge bug causes changesets to be applied although this should not be the case

[Fredrik Orderud <fo...@gmail.com>]

On Tue, Aug 6, 2013 at 3:19 PM, Johan Corveleyn <jc...@gmail.com> wrote:

> FYI, another user complained about this on the users list (Fredrik
> Orderud, CC'd) [1]. He has filed issue #4405 [2]. Perhaps some of the
> interested parties here can take a closer look? Or maybe Fredrik or
> anyone can start / continue discussion to go towards a detailed
> specification of the behavior of such an optional 'strict' flag or
> something like that ...
>
> (no specific interest myself, just trying to tie some loose ends together)
>
> [1]
> http://mail-archives.apache.org/mod_mbox/subversion-users/201308.mbox/%3CCAHGmiisrngb1sogdLhKkY+9ePuNqVjZ8V7J_LyfAkxezZ54fJw@mail.gmail.com%3E
>
> [2] http://subversion.tigris.org/issues/show_bug.cgi?id=4405 (Merge
> order affects final result for repeated added & deleted changes)
>

I've now written an XFAIL test for merging the same change change twice.
Patch attached. The test fails as expected due to the lack of conflict.
This is the first test I've ever written for subversion, so there are
probably some improvement opportunities. I suspect that one weak spot is
that a "greek-tree" structure is generated without being used in the test.
Also, comparison of console output for merge results feels a little fragile.

Please let me know if there are any comments to the patch, and I'll do my
best to improve it. Otherwise, it would be great it the test could be
integrated, so that issue #4405 can receive some test coverage.

Thanks in advance,
Fredrik