On Tuesday, February 05, 2013 01:55:20 PM Lathan Bidwell wrote:
> On 02/05/2013 01:14 PM, Stefan Sperling wrote:
> > On Tue, Feb 05, 2013 at 09:19:19AM -0800, Alexey Neyman wrote:
> >> There is one more weird issue with svn diff, see the script below. The
> >> issue is that "--old=A --new=B" is not opposite of "--old=B --new=A". I
> >> don't know if it is a bug or another ambuguity I am not aware of :)
> >>
> >> Here is the script:
> >> [[[
> >> #!/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
> >> ]]]
> >>
> >> Regards,
> >> Alexey.
> >
> > I can reproduce this with a trunk build. Can you please file an issue
> > for this? I'm not going to get to this right away. Thanks!
>
> Here is the issue that I see:
>
> The --old=. get's the workspace version of ., but --new get's the
> non-changed version of /.
>
> So, I believe it is comparing "r1" with the r2 contents of "r2" and not
> comparing both workspace versions.
>
> Rev Contents
> 1 r1
> 2 r2
> 2* new
>
> 2* only get's referenced when it is in the --old position,
> 2 get's used when its in the --new position.
>
> Please correct me if I'm wrong about this, but that is what seems to be
> a logical reasoning behind that behavior.
First, it is just the opposite: 2* gets referenced when it is in the --new,
not --old position. Second, it is not the reasoning (why it behaves so) but
rather a description of current behavior, which I think was rather obvious
from the example script.
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, given that new target is optional and defaults
to old target if not specified. If 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.Index: subversion/svn/diff-cmd.c
===================================================================
--- subversion/svn/diff-cmd.c (revision 1442686)
+++ subversion/svn/diff-cmd.c (working copy)
@@ -238,10 +238,11 @@ svn_cl__diff(apr_getopt_t *os,
targets->nelts = 0;
/* Set default start/end revisions based on target types, in the same
- * manner as done for the corresponding '--old X --new Y' cases. */
+ * manner as done for the corresponding '--old X --new Y' cases,
+ * (note that we have an explicit --new target) */
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;
+ ? svn_opt_revision_head : svn_opt_revision_working;
if (opt_state->end_revision.kind == svn_opt_revision_unspecified)
opt_state->end_revision.kind = svn_path_is_url(new_target)
@@ -280,20 +281,14 @@ svn_cl__diff(apr_getopt_t *os,
if (new_rev.kind != svn_opt_revision_unspecified)
opt_state->end_revision = new_rev;
- if (opt_state->new_target
- && opt_state->start_revision.kind == svn_opt_revision_unspecified
- && opt_state->end_revision.kind == svn_opt_revision_unspecified
- && ! svn_path_is_url(old_target)
- && ! svn_path_is_url(new_target))
- {
- /* We want the arbitrary_nodes_diff instead of just working nodes */
- opt_state->start_revision.kind = svn_opt_revision_working;
- opt_state->end_revision.kind = svn_opt_revision_working;
- }
-
+ /* For URLs, default to HEAD. For WC paths, default to WORKING
+ * if new target is explicit or to BASE if new target is implicitly
+ * set to the same as old target. */
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;
+ ? svn_opt_revision_head
+ : opt_state->new_target
+ ? svn_opt_revision_working : 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)
