Return-Path: X-Original-To: apmail-subversion-users-archive@minotaur.apache.org Delivered-To: apmail-subversion-users-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 0161AEABA for ; Tue, 5 Feb 2013 22:23:27 +0000 (UTC) Received: (qmail 44073 invoked by uid 500); 5 Feb 2013 22:23:26 -0000 Delivered-To: apmail-subversion-users-archive@subversion.apache.org Received: (qmail 44054 invoked by uid 500); 5 Feb 2013 22:23:26 -0000 Mailing-List: contact users-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list users@subversion.apache.org Received: (qmail 44047 invoked by uid 99); 5 Feb 2013 22:23:26 -0000 Received: from athena.apache.org (HELO athena.apache.org) (140.211.11.136) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 05 Feb 2013 22:23:26 +0000 X-ASF-Spam-Status: No, hits=2.9 required=5.0 tests=HTML_MESSAGE,RCVD_IN_DNSWL_NONE,SPF_NEUTRAL,UNPARSEABLE_RELAY X-Spam-Check-By: apache.org Received-SPF: neutral (athena.apache.org: local policy) Received: from [66.94.236.31] (HELO nm21-vm0.access.bullet.mail.mud.yahoo.com) (66.94.236.31) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 05 Feb 2013 22:23:17 +0000 Received: from [66.94.237.198] by nm21.access.bullet.mail.mud.yahoo.com with NNFMP; 05 Feb 2013 22:22:54 -0000 Received: from [98.138.84.214] by tm9.access.bullet.mail.mud.yahoo.com with NNFMP; 05 Feb 2013 22:22:53 -0000 Received: from [127.0.0.1] by smtp103.sbc.mail.ne1.yahoo.com with NNFMP; 05 Feb 2013 22:22:53 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1360102973; bh=Y+K9JvtufbXAjU7gGM6yBep2NvZxuO1Ee8DOpdA92Lc=; h=X-Yahoo-Newman-Id:X-Yahoo-Newman-Property:X-YMail-OSG:X-Yahoo-SMTP:Received:From:To:Cc:Subject:Date:Message-ID:User-Agent:In-Reply-To:References:MIME-Version:Content-Type:Content-Transfer-Encoding; b=LB93o8KLYsAkPFlTPJr8sLVBEnsSYu5VV/6qm9kGmFcIxyWOJQgRNeeNgG6CBkxCEneYJkQp6019cjLzrUnftJ4ZuUlzAt7+9zOgO//kLks2/zwsxsx3nKzINtGHLNyNtZAfWHGLQBQDrUTX3GJsArXwHbDBdVQ8lSzh7HJVgyc= X-Yahoo-Newman-Id: 751746.88117.bm@smtp103.sbc.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: jyVJ9n4VM1mUA.Ab6ntAJ2UAz7xrcMlWLmX7SCFNLfGFeEk RLHw9l4SY3OIbjSj0WQwVv3kVNShYXbSk3TRgSsqpVM6lkJWZe58rcbhl4mW MfAQrQXLGfa_VahfqJdg2tA29V..PrMSAHlbujx1ZLtmXOTjzR8ZCEzVAMLG _srLSdGncSyTySiZBgpnYVeLnTpuHNtWoTKjCtVV0Nv5fzqrw_M3SlwK7Hp1 EilICL_81ukQyKstffKIR3T2xIVlBUAIBJJYirezggRLnwvxrFOfWI5UxtZd .Ko5sndIzh.22u4QkWPKkj21Fbs.D3JVpIDcQ2j7qyfg9YCT23INpV.tDwPT koeO3jdcefJphm0bZIlvE4xWjy1_aSrLzTeyXT.hFqswZktVlL7WHhVs7T9A Jcyn_jMG.28WlFshdSSnBzRkYZaYH_AcBRkWzupMqtSXO2scQDiIVXQb304T OGfdlIYNjq3xfLPqBWQ-- X-Yahoo-SMTP: 0h0Q7euswBD_g.kcEqbzJWRFfrba801gq1M1 Received: from etoile.localnet (stilor@216.100.252.242 with plain) by smtp103.sbc.mail.ne1.yahoo.com with SMTP; 05 Feb 2013 22:22:53 +0000 UTC From: Alexey Neyman To: users@subversion.apache.org Cc: Lathan Bidwell , Stefan Sperling , Johan Corveleyn , Alfred Perlstein Subject: Re: FreeBSD project and subversion. Date: Tue, 05 Feb 2013 14:22:51 -0800 Message-ID: <59137366.pMA19V6gGN@etoile> User-Agent: KMail/4.9.4 (Linux/3.5.0-22-generic; KDE/4.9.4; i686; ; ) In-Reply-To: <51115598.9010305@andrews.edu> References: <510A8FAA.2020903@mu.org> <20130205181421.GD32341@byrne.stsp.name> <51115598.9010305@andrews.edu> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nextPart1565791.GMCBfR3VSs" Content-Transfer-Encoding: 7Bit X-Virus-Checked: Checked by ClamAV on apache.org This is a multi-part message in MIME format. --nextPart1565791.GMCBfR3VSs Content-Type: multipart/alternative; boundary="nextPart1737280.dhMWqs1bc2" Content-Transfer-Encoding: 7Bit This is a multi-part message in MIME format. --nextPart1737280.dhMWqs1bc2 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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. --nextPart1737280.dhMWqs1bc2 Content-Transfer-Encoding: 7Bit Content-Type: text/html; charset="us-ascii"

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.

--nextPart1737280.dhMWqs1bc2-- --nextPart1565791.GMCBfR3VSs Content-Disposition: attachment; filename="svn-diff-default-revision.diff" Content-Transfer-Encoding: 7Bit Content-Type: text/x-patch; charset="UTF-8"; name="svn-diff-default-revision.diff" 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) --nextPart1565791.GMCBfR3VSs--