You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Alexey Neyman <st...@att.net> on 2013/02/06 19:02:22 UTC

[PATCH] Weird 'svn diff' operation with --old/--new (was: Re: FreeBSD project and subversion.)

[Cross-posting to dev@ for patch review]

Hi,

I noticed a weird behavior of 'svn diff --old=... --new=...' where reversing 
the old/new targets does not produce the reverse diff, as one would expect.

Here is the script for reproduction:

[[[
#!/bin/bash
 
REPO=/tmp/foo
WC=/tmp/foo.wc
 
rm -rf $REPO $WC
svnadmin create $REPO
svn co -q file://$REPO $WC
cd $WC
 
echo r1 > a
svn add -q a
svn ci -q -m R1
echo r2 > a
svn ci -q -m R2
svn up -q -r 1
echo new > a
echo "Issue: --old=A --new=B is not opposite of --old=B --new=A"
echo "  Running: svn di --old=^/ --new=."
svn di --old=^/ --new=.
echo "  Running: svn di --old=. --new=^/"
svn di --old=. --new=^/
]]]
 
And here is the output (svn 1.7.6):
[[[
Issue: --old=A --new=B is not opposite of --old=B --new=A
  Running: svn di --old=^/ --new=.
Index: a
===================================================================
--- a   (.../file:///tmp/foo)   (revision 2)
+++ a   (working copy)
@@ -1 +1 @@
-r2
+new
  Running: svn di --old=. --new=^/
Index: a
===================================================================
--- a   (working copy)
+++ a   (.../file:///tmp/foo)   (revision 2)
@@ -1 +1 @@
-r1
+r2
]]]

The reason for this behavior is that the code in diff-cmd.c makes the 
following defaults for the old/new revisions:

      if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
        opt_state->start_revision.kind = svn_path_is_url(old_target)
          ? svn_opt_revision_head : svn_opt_revision_base;

      if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
        opt_state->end_revision.kind = svn_path_is_url(new_target)
          ? svn_opt_revision_head : svn_opt_revision_working;

These defaults make some sense, if the new target is not specified (it 
defaults to old target in that case). If the new target is specified, and is 
different from from old target, I am not so sure if it makes sense. Note that 
there is a special case if both old/new targets are WC paths - in that case, 
both old and new targets default to svn_opt_revision_working. So,

$ svn diff --old=foo --new=bar

will compare MODIFIED version of 'foo' against modified version of 'bar' [*], 
but

$ svn diff --old=foo --new=^/bar

would compare BASE version of 'foo' against head version of 'bar. Does it make 
sense? I would say, if both old and new are specified, old target's revision 
should default to 'working' as well.

Problem is further aggravated since there is no commandline alias to request 
svn_opt_revision_working setting from the command line: only 'head', 'prev', 
'base' and 'committed' are currently available. Hence, it is not possible to 
request the exact opposite of the patch using -rN:M syntax if one of the --
old/--new targets is a working copy path. Is there a reason why 'working' is 
not parsed by revision_from_word()?

Also note that 'svn help diff' does not describe revision defaults for 
synopsis 2 (and the default is different from synopsis 1, which requires old 
revision to be specified for URLs).

PS. Stephan apparently made 'working' revision as default in his check-in 
(r1442640) at least for the short-hand form, but Julian Foad, "optimizing the 
logic" in r1442659 and r1442676, switched it back to be the same as --old/--
new logic.

PPS. If agreed to suggested change of default, the patch is attached.

[[[
Make svn diff --old=.. --new=.. default to WORKING revision for old target
if new target is explicitly specified.

* subversion/svn/diff-cmd.c
  (svn_cl__diff) Change defaults as described and simplify the logic.
]]]

Regards,
Alexey.

Re: [PATCH] Weird 'svn diff' operation with --old/--new (was: Re: FreeBSD project and subversion.)

Posted by Julian Foad <ju...@btopenworld.com>.
Alexey Neyman wrote:
> Julian Foad wrote:
>> Alexey Neyman wrote:
>> > I noticed a weird behavior of 'svn diff --old=... --new=...' where
>> > reversing the old/new targets does not produce the reverse diff, as
>> > one would expect.
> [[[
> Make 'svn diff --old=FOO --new=BAR default to WORKING revision for old target
> if new target is explicitly specified, so that it is the reverse diff from 'svn diff --old=BAR --new=FOO'.
> 
> * subversion/svn/diff-cmd.c
> (svn_cl__diff) Change defaults as described and simplify the logic.
> ]]]

Thanks -- I think this all looks good.

Committed in 1443821, with just some minor editing of the comment.

(Incidentally, applying your patch using 'svn patch' led me to discover a bug in 'svn patch' which I have just emailed about.)

- Julian

Re: [PATCH] Weird 'svn diff' operation with --old/--new (was: Re: FreeBSD project and subversion.)

Posted by Alexey Neyman <st...@att.net>.
Hi Julian,

On Thursday, February 07, 2013 04:00:49 PM Julian Foad wrote:
> Alexey Neyman wrote:
> > [Cross-posting to dev@ for patch review]
> > I noticed a weird behavior of 'svn diff --old=... --new=...' where
> > reversing the old/new targets does not produce the reverse diff, as
> > one would expect.
> 
> Hi Alexey.  Thanks for the report.  As you pointed out, I changed something
> in this area recently, so I'll take a closer look and fix it.  I haven't
> done so yet but should get to it in the next day or two.
> 
> [...]

Thanks!

[... snip ...]

> > [[[
> > Make svn diff --old=.. --new=.. default to WORKING revision for old target
> > if new target is explicitly specified.
> 
> A log message should always indicate *why* the change was made.

Hope this is better:

[[[
Make 'svn diff --old=FOO --new=BAR default to WORKING revision for old target
if new target is explicitly specified, so that it is the reverse diff from 
'svn diff --old=BAR --new=FOO'.

* subversion/svn/diff-cmd.c
 (svn_cl__diff) Change defaults as described and simplify the logic.
]]]

Regards,
Alexey.

Re: [PATCH] Weird 'svn diff' operation with --old/--new (was: Re: FreeBSD project and subversion.)

Posted by Julian Foad <ju...@btopenworld.com>.
Alexey Neyman wrote:
> [Cross-posting to dev@ for patch review]
> I noticed a weird behavior of 'svn diff --old=... --new=...' where
> reversing the old/new targets does not produce the reverse diff, as
> one would expect.

Hi Alexey.  Thanks for the report.  As you pointed out, I changed something in this area recently, so I'll take a closer look and fix it.  I haven't done so yet but should get to it in the next day or two.

[...]
> The reason for this behavior is that the code in diff-cmd.c makes the
> following defaults for the old/new revisions:
> 
> if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
>   opt_state->start_revision.kind = svn_path_is_url(old_target)
>     ? svn_opt_revision_head : svn_opt_revision_base;
> 
> if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
>   opt_state->end_revision.kind = svn_path_is_url(new_target)
>     ? svn_opt_revision_head : svn_opt_revision_working;
> 
> These defaults make some sense, if the new target is not specified (it
> defaults to old target in that case). If the new target is specified,
> and is different from from old target, I am not so sure if it makes
> sense. Note that there is a special case if both old/new targets are
> WC paths - in that case, both old and new targets default to
> svn_opt_revision_working. So,
> 
> $ svn diff --old=foo --new=bar
> 
> will compare MODIFIED version of 'foo' against modified version of
> 'bar' [*], but
> 
> $ svn diff --old=foo --new=^/bar
> 
> would compare BASE version of 'foo' against head version of 'bar.
> Does it make sense? I would say, if both old and new are specified,
> old target's revision should default to 'working' as well.
> 
> Problem is further aggravated since there is no commandline alias
> to request svn_opt_revision_working setting from the command line:
> only 'head', 'prev', 'base' and 'committed' are currently available.
> Hence, it is not possible to request the exact opposite of the patch
> using -rN:M syntax if one of the --old/--new targets is a working
> copy path. Is there a reason why 'working' is not parsed by
> revision_from_word()?
> 
> Also note that 'svn help diff' does not describe revision defaults
> for synopsis 2 (and the default is different from synopsis 1, which
> requires old revision to be specified for URLs).
> 
> PS. Stephan apparently made 'working' revision as default in his
> check-in (r1442640) at least for the short-hand form, but Julian
> Foad, "optimizing the logic" in r1442659 and r1442676, switched it
> back to be the same as --old/--new logic.

Oops, I didn't notice I was changing the logic.  As my log message said, I thought I was just simplifying it.

> PPS. If agreed to suggested change of default, the patch is attached.
> 
> [[[
> Make svn diff --old=.. --new=.. default to WORKING revision for old target
> if new target is explicitly specified.

A log message should always indicate *why* the change was made.

> 
> * subversion/svn/diff-cmd.c
> (svn_cl__diff) Change defaults as described and simplify the logic.
> ]]]

- Julian