You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Paul Burba <pa...@softlanding.com> on 2006/01/12 21:12:01 UTC
[PATCH] svn diff -r1:BASE URL asserts
kfogel@newton.ch.collab.net wrote on 12/23/2005 12:36:16 PM:
> Paul Burba <pa...@softlanding.com> writes:
> > I'd be glad to look into it, with one caveat: I'm on vacation next
week so
> > I probably won't get to it until the first week of January. If this
is
> > minor enough to wait until then I can do it.
>
> I think that's fine -- a mis-handled error condition, however
> egregious, is not in the same class as, say, a dataloss bug :-).
>
> -Karl
>
> --
> www.collab.net <> CollabNet | Distributed Development On Demand
>
>
> > > -Karl
> > >
> > > > C:\home\BURBA\WCS\SIG5>C:\SVN\src-trunk.collabnet.
> > > trunk\Release\subversion\svn\svn.exe
> > > > diff -r1:BASE file:///home/BURBA/REPOS/SIG5
> > > > Assertion failed: ! svn_path_is_url (path2), file
> > > > C:\SVN\src-trunk.collabnet.trunk\subversion\libsvn_client\diff.c,
line
> >
> > > > 2117
> > > >
> > > > This application has requested the Runtime to terminate it in an
> > unusual
> > > > way.
> > > > Please contact the application's support team for more
information.
> > > >
> > > > I didn't find any mention of this in the issue tracker.
> > > >
> > > > If it matters, this build is of r17918 on XP Pro 5.1 SP2
> > > >
> > > > Paul B.
Karl,
Sorry I didn't have time to get at this last week, I've been busy hunting
down an iSeries optimization bug in our 1.3.0 port.
Anyway, here is my proposed fix. The "opt_state->start_revision.kind ==
svn_opt_revision_base" isn't absolutely necessary since this case is
caught in svn_client__get_revision_number(), but it seems more appropriate
to check it here.
Paul B.
[[[
Stop svn diff 1:BASE URL from asserting and provide a more useful error
msg.
* subversion/svn/diff-cmd.c
(svn_cl__diff): If the starting or ending revision against a URL is
BASE
return an error.
]]]
Index: subversion/svn/diff-cmd.c
===================================================================
--- subversion/svn/diff-cmd.c (revision 18079)
+++ subversion/svn/diff-cmd.c (working copy)
@@ -155,6 +155,13 @@
return svn_error_createf (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
_("Target lists to diff may not contain
"
"both working copy paths and URLs"));
+
+ /* Diff -rBASE:xxxx | rxxxx:BASE URL is invalid. */
+ if (url_present
+ && (opt_state->start_revision.kind == svn_opt_revision_base
+ || opt_state->end_revision.kind == svn_opt_revision_base))
+ return svn_error_create
+ (SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED, NULL, NULL);
if (opt_state->start_revision.kind == svn_opt_revision_unspecified
&& working_copy_present)
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Paul Burba <pa...@softlanding.com>.
Julian Foad <ju...@btopenworld.com> wrote on 01/13/2006 09:06:24 PM:
> Paul Burba wrote:
> > Julian Foad <ju...@btopenworld.com> wrote on 01/12/2006 07:39:00
PM:
> >
> >>My first reaction is: "What is special about BASE?" Why isn't
> handled the same
> >>as PREV and COMMITTED which are the other two revision keywords
> that require a WC?
> >
> > What's special about it is that svn diff x:BASE asserts and the other
> > cases are handled, albeit inconsistently. My thought was just to
prevent
> > the assert - that's my rather lame excuse anyway since I *did* include
the
> > svn diff BASE:x case which the current code doesn't assert on.
>
> Yes, I understood that; sorry I wasn't clear, but what I meant was, "Why
> doesn't our existing code behave consistently? Is there some problem
with it
> that we should fix, or is there a logical reason for the disparity?"In
other
> words, is this extra check necessary, or is there a
> broken/incomplete check in
> place that needs to be corrected?
Hi Julian,
I don't have a definitive answer, but debugging a similar diff that
doesn't assert, side-by-side with the problem case, there are a couple
points where things seem a bit odd.
--------------------------------------------------------------------------------
(Working Scenario) (Asserting Scenario)
svn diff BASE:1 URL svn diff 1:BASE URL
--------------------------------------------------------------------------------
svn/diff-cmd.c svn/diff-cmd.c
svn__cl_diff() svn__cl_diff()
pegged_diff is determined only by The first potential problem,
looking at the start revision pegged_diff == TRUE, so we call:
kind. It is TRUE iff start
revision kind is not
svn_opt_revision_base or
svn_opt_revision_working.
So it's FALSE in this case which
results in call to:
--------------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
svn_client_diff3() svn_client_diff_peg3()
--------------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
do_diff() do_diff_peg()
Call check_paths() and get Our second potential problem, the
call
the struct diff_paths to check_paths() results in the
struct
*diff_paths: diff_paths* diff_paths:
diff_paths->is_local_rev1 1 diff_paths->is_local_rev1 0
diff_paths->is_local_rev2 0 diff_paths->is_local_rev2 1
diff_paths->is_repos_path1 1 diff_paths->is_repos_path1 1
diff_paths->is_repos_path2 1 diff_paths->is_repos_path2 0
Here the paths struct accurately But path 2 is a URL at revision
BASE,
represents the invalid request invalid yes, but the paths struct
for the BASE rev of an URL. doesn't reflect this and causes a
call
to:
--------------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
diff_repos_repos() diff_repos_wc()
And here we run into the assert:
assert (! svn_path_is_url
(path2));
--------------------------------------------------------------------------------
libsvn_client/diff.c
diff_prepare_repos_repos()
Here we call
svn_client__get_revision_number()
on a NULL path.
--------------------------------------------------------------------------------
libsvn_client/revisions.c
svn_client__get_revision_number()
Returns SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED
svn: A path under version control is needed for this operation
----------------------------------------------------------------
----------------
I'm afraid I don't have a thorough enough grasp of the implications of
changing svn__cl_diff(), do_diff_peg(), and/or check_paths() to say that
any of these could be "fixed" in a way that makes my patch unnecessary. I
can start digging into this some more, but I'm hoping that the above might
turn on the light bulb for someone who is more familiar with this code.
> >>svn diff -r1:PREV file:///home/julianfoad/tmp/vcs/sandbox/trunk
> >>svn: 'file:///home/julianfoad/tmp/vcs/sandbox' is not a working copy
>
> > [[[
> > Return consistent error on svn diff of special revisions BASE,
COMMITTED,
> > or PREV when operating on a URL.
> >
> > The original impetus for this patch was to stop svn diff 1:BASE URL
from
> > asserting and provide a more useful error message.
> >
> > * subversion/svn/diff-cmd.c
> > (svn_cl__diff): If the starting or ending revision against a URL is
> > BASE,
> > COMMITTED, or PREV return an error.
> > ]]]
> >
> > Index: subversion/svn/diff-cmd.c
> > ===================================================================
> > --- subversion/svn/diff-cmd.c (revision 18091)
> > +++ subversion/svn/diff-cmd.c (working copy)
> > @@ -155,6 +155,19 @@
> > return svn_error_createf (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> > _("Target lists to diff may not
contain
> > "
> > "both working copy paths and
URLs"));
> > +
> > + /* Diffing special revisions BASE, COMMITTED, or PREV
> > + against a URL is invalid. */
> > + if (url_present
> > + && (opt_state->start_revision.kind == svn_opt_revision_base
> > + || opt_state->end_revision.kind ==
svn_opt_revision_base
> > + || opt_state->start_revision.kind ==
> > svn_opt_revision_committed
> > + || opt_state->end_revision.kind ==
> > svn_opt_revision_committed
> > + || opt_state->start_revision.kind ==
> > svn_opt_revision_previous
> > + || opt_state->end_revision.kind ==
> > svn_opt_revision_previous)
> > +)
> > + return svn_error_create
> > + (SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED, NULL, NULL);
>
> My only other comment is that it would be nice to include the path
> in the error
> message, so that the cause is clear if this occurs in a command that was
> handling multiple paths. That requires writing out most of the error
message
> long-hand, which is a bit ugly but worth it.
I'm not sure this matters; If the svn diff command is handling multiple
targets there are three scenarios:
1) Targets are all WCS
2) Targets are mixed WCS and URLS
3) Targets are all URLS
In the first case the check in my patch is never made because url_present
== FALSE.
In the second case the existing test of (url_present &&
working_copy_present) returns the "Target lists to diff may not contain
both working copy paths and URLs" error.
And in the last case *all* the target URLS are invalid and always will be,
since they are URLS :-)
...unless I am missing something.
Paul B.
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> Julian Foad <ju...@btopenworld.com> wrote on 01/12/2006 07:39:00 PM:
>
>>My first reaction is: "What is special about BASE?" Why isn't handled the same
>>as PREV and COMMITTED which are the other two revision keywords that require a WC?
>
> What's special about it is that svn diff x:BASE asserts and the other
> cases are handled, albeit inconsistently. My thought was just to prevent
> the assert - that's my rather lame excuse anyway since I *did* include the
> svn diff BASE:x case which the current code doesn't assert on.
Yes, I understood that; sorry I wasn't clear, but what I meant was, "Why
doesn't our existing code behave consistently? Is there some problem with it
that we should fix, or is there a logical reason for the disparity?" In other
words, is this extra check necessary, or is there a broken/incomplete check in
place that needs to be corrected?
>>svn diff -r1:PREV file:///home/julianfoad/tmp/vcs/sandbox/trunk
>>svn: 'file:///home/julianfoad/tmp/vcs/sandbox' is not a working copy
> [[[
> Return consistent error on svn diff of special revisions BASE, COMMITTED,
> or PREV when operating on a URL.
>
> The original impetus for this patch was to stop svn diff 1:BASE URL from
> asserting and provide a more useful error message.
>
> * subversion/svn/diff-cmd.c
> (svn_cl__diff): If the starting or ending revision against a URL is
> BASE,
> COMMITTED, or PREV return an error.
> ]]]
>
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 18091)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -155,6 +155,19 @@
> return svn_error_createf (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> _("Target lists to diff may not contain
> "
> "both working copy paths and URLs"));
> +
> + /* Diffing special revisions BASE, COMMITTED, or PREV
> + against a URL is invalid. */
> + if (url_present
> + && (opt_state->start_revision.kind == svn_opt_revision_base
> + || opt_state->end_revision.kind == svn_opt_revision_base
> + || opt_state->start_revision.kind ==
> svn_opt_revision_committed
> + || opt_state->end_revision.kind ==
> svn_opt_revision_committed
> + || opt_state->start_revision.kind ==
> svn_opt_revision_previous
> + || opt_state->end_revision.kind ==
> svn_opt_revision_previous)
> +)
> + return svn_error_create
> + (SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED, NULL, NULL);
My only other comment is that it would be nice to include the path in the error
message, so that the cause is clear if this occurs in a command that was
handling multiple paths. That requires writing out most of the error message
long-hand, which is a bit ugly but worth it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by kf...@collab.net.
Michael W Thelen <mi...@pietdepsi.com> writes:
> I have to apologize for letting this patch sit so long without being
> addressed. Would any developer be able to review Paul's patch? If not,
> I'll file an issue for in within a few days. Here is a link to Paul's
> patch in the archive:
>
> http://svn.haxx.se/dev/archive-2006-01/0350.shtml
Yes, I will take this and DTRT. Thanks for the ping, Michael!
-Karl
--
www.collab.net <> CollabNet | Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Pegged diffs [was: [PATCH] svn diff -r1:BASE URL asserts]
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote (earlier):
> Okay, perhaps I have the definition of a 'pegged diff' wrong - what
> exactly is it? I assumed, I guess, that it was one in which we needed
> to follow history to get the name of the item at the operative revisions.
Gosh, this thread has really made me think hard about what we mean. After
several hours and several revisions of my reply, here's what I think, starting
from first principles and working up towards higher-level concepts.
THE MEANING OF PEG REVISIONS
An OBJECT under version control (I mean a file or a directory tree; would
"node" be more consistent with other docs?) is identified by one of:
* a WC-path (under version control)
* a URL in a certain revision of the repository
An object has a LINE OF HISTORY which records its life from creation,
potentially through modifications, renames, and/or deletion. (Looking forward
in history, there is the complication that there are potentially multiple lines
of history because of copying, but that's not relevant to the present
discussion.) Identifying an object at any stage of its life implicitly
identifies its line of history.
When a revision is used in conjunction with a path-in-repository to identify an
object, and the object's line of history is followed in order to find a later
or earlier revision in that history, the initial revision is known as the PEG
REVISION.
If we just want to locate a particular object as it exists now or existed in
history, and we know what its path was at that time and do not need to follow
its history, we will specify the object in the same way (WC path, or URL in a
particular revision), and we might still call this particular revision the
"peg" revision, particularly when referring to the syntax "@REV" with which it
is specified.
Any target to an 'svn' command is therefore specified as:
WCPATH (implicit @WC)
URL (implicit @HEAD)
URL@HEAD
URL@N
URL@{DATE}
When we specify a plain WC path, we are specifying an object by the path it has
"now" in this working copy, not at some previous time when its WC path might
have been different. After "svn move" or "svn rm", the base path will no
longer exist on disk. The following is not currently supported, but would be
logical:
WCPATH@BASE (to specify the WCPATH that existed before mv/rm)
The following do not make sense:
URL@WC/BASE/COMMITTED/PREV (these keywords have no meaning without a WC)
WCPATH@HEAD/N/{DATE} (a WC path has no intrinsic meaning in repos *)
WCPATH@COMMITTED/PREV (these keywords have no meaning until a WC is
consulted, but the peg syntax requires them to
identify a revision in which the WC path can
be found)
Then, in many commands, a separate OPERATIVE REVISION (or a pair of them) can
be specified with "--revision" and Subversion will trace the identified object
through history to that revision, where its name might be different.
"-rX URL@REV" means:
From the object found at URL in revision REV,
trace back or forward to revision X.
"-rX wc-path" means:
From the object found at wc-path (in the WC),
trace back or forward to revision X.
Note that a peg revision applied to a local path doesn't make sense because a
local path (assumed to refer to a versioned object) already locates the object
uniquely; the peg is implicitly "WC".
<bug>
The syntax described in "svn help" for some command implies that we currently
allow "wc-path@REV" for arbitrary REVs.
"-rX wc-path@REV" means... what?
Trace from WC to REV and then further to X? REV is redundant if so.
Trace from WC to REV and ignore X? "diff --old" does, but that's bad.
(* In the future we might want to give some meaning to "path@REV", where "path"
looks like a WC path, but perhaps means a relative URL based on the current
directory's URL: "foo@100" -> (url(".") + "/foo")@100. This is not a proposal,
just an example of a reason why we should disallow "wc-path@REV" now.)
</bug>
So "wc-path" always implies a "peg" revision of "WC", in the sense that the WC
is where "wc-path" will be found. (Let's assume for now that we won't allow
the explicit syntax "wc-path@REV".)
For commands that operate on two revisions of a single object, primarily diff
and merge, it is necessary to know in which revision the specified path is to
be found, and a separate peg revision specifier is a convenient and flexible
solution. Commands that operate on a single object (at a time) support a
separately specified peg revision only for convenience and could instead
require the object's path to be specified exactly as it exists in the operative
revision.
Does this all make sense so far?
The two TYPES OF DIFF INVOCATION that we support are:
(a) Difference between one revision and another revision of an object.
(b) Difference between one object and another which may be unrelated. This
is more general, and potentially covers the first type.
In both types, our user interface allows multiple targets, but those are
handled by high-level iteration in svn_cl__diff(). Although that's
inefficient, it means we don't presently have to think about multiple targets
in this discussion which pertains to libsvn_client and lower layers.
In type (a), there is one object's line of history that must be identified, and
also a "start" revision and an "end" revision within that line. This is what I
think we have been calling a "pegged diff". It corresponds to type (1)
invocation syntax in "svn help diff" and is handled by svn_client_diff_peg3().
I would say now that "pegged" is not the clearest name for it; something like
"diff of an object across revisions" would be better.
In type (b), there are two objects that must be identified. Each can be
identified by means of a local path, or a URL at a certain revision (which we
might be tempted to call a "peg" revision even if we are not going to trace
history from it to another revision). This corresponds to type (2) syntax in
"svn help diff" and is handled by svn_client_diff3(). If the two objects
happen to be linked by history, then there may be scope for behavioural options
which I won't discuss yet.
I get the impression that there is some confusion about the "real" meaning of
these concepts, commands and functions, as it is easy to think that a "pegged
diff" is any of:
* a diff where "TARGET@REV" syntax was used
* a diff where at least one object has to be traced through history
* a diff comparing two revisions of a single object
These are all slightly different.
Are we happy to accept the third variant, the concept of comparing two
revisions of a single object (necessarily within its own line of history), as
the primary concept that differentiates svn_client_diff3() and
svn_client_diff_peg3() and the like?
Malcolm Rowe wrote:
>>Okay, perhaps that wasn't too clear. In summary:
>>
>>* I don't think a BASE:WORKING diff is pegged, because by definition
>> theres no history-tracing to do. However...
Has the above discussion changed your mind, at least if by "pegged" we mean my
type (a), thus should call svn_client_diff_peg3()?
>>* I don't mind if we just call the pegged version of diff unconditionally,
>
> where possible (i.e., only in the single-path case)
I don't get that. If you mean single-path as opposed to multi-path, I
certainly don't want a special case for a single path; I thought this would
work for any number of paths. If you mean single-path as opposed to two-path,
as in the "--old/--new" form, then yes, the two-path form is necessarily my
type (b), which we may colloquially refer to as "non-pegged".
>> as long as we pass a peg-revision of svn_opt_revision_unspecified for
>> the BASE:WORKING case.
I'd say the peg should be _revision_working in that case.
>>The latter currently isn't well-defined, though I'm just about to go
>>off and fix that, making the diff (and possibly merge; I've not checked)
>>functions more like the rest of the functions in libsvn_client.
>
> though we will still need different functions, as the _peg versions only
> take a single path.
As opposed to two paths, I assume you mean. I realise this conversation may be
obsolete by now, but if it's still relevant, why is it a problem that the _peg
functions only take a single path?
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Malcolm Rowe <ma...@farside.org.uk>.
I'm not properly awake yet :) Let me try that again:
On Fri, Mar 10, 2006 at 09:39:38AM +0000, Malcolm Rowe wrote:
> On Thu, Mar 09, 2006 at 10:02:09PM +0000, Malcolm Rowe wrote:
> > [long rambling email]
>
> Okay, perhaps that wasn't too clear. In summary:
>
> * I don't think a BASE:WORKING diff is pegged, because by definition
> theres no history-tracing to do. However...
> * I don't mind if we just call the pegged version of diff unconditionally,
where possible (i.e., only in the single-path case)
> as long as we pass a peg-revision of svn_opt_revision_unspecified for
> the BASE:WORKING case.
>
> The latter currently isn't well-defined, though I'm just about to go
> off and fix that, making the diff (and possibly merge; I've not checked)
> functions more like the rest of the functions in libsvn_client.
though we will still need different functions, as the _peg versions only
take a single path.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Mar 09, 2006 at 10:02:09PM +0000, Malcolm Rowe wrote:
> [long rambling email]
Okay, perhaps that wasn't too clear. In summary:
* I don't think a BASE:WORKING diff is pegged, because by definition
theres no history-tracing to do. However...
* I don't mind if we just call the pegged version of diff unconditionally,
as long as we pass a peg-revision of svn_opt_revision_unspecified for
the BASE:WORKING case.
The latter currently isn't well-defined, though I'm just about to go
off and fix that, making the diff (and possibly merge; I've not checked)
functions more like the rest of the functions in libsvn_client.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Mar 09, 2006 at 01:19:03PM +0000, Julian Foad wrote:
> Malcolm Rowe wrote:
> >>That's wrong. In this section, according to the comment, we should
> >>assign pegged_diff=TRUE unconditionally.
> >
> >Nope; 'svn diff foo' is handled by that case too, and that's not a pegged
> >diff (though it looks like later code may may it pegged @WORKING, which
> >may work anyway). I think it only wants to be pegged if either revision
> >is 'non-local'.
>
> I disagree with you. Yes, "svn diff foo" is handled by that case. That
> means, "show me the difference between the base revision and the working
> pseudo-revision of this object". To me, that's a "pegged diff" because I
> think of "the working files" as being a revision of a sort, rather than
> thinking of this as a difference between one arbitrary tree and another
> arbitrary tree.
>
Okay, perhaps I have the definition of a 'pegged diff' wrong - what
exactly is it? I assumed, I guess, that it was one in which we needed
to follow history to get the name of the item at the operative revisions.
> different. I don't see the logic in saying that a diff is "pegged" only if
> at least one revision is non-local:
>
> diff foo@100 foo@200
> diff foo@BASE foo@200
> diff foo@100 foo@WC
> diff foo@BASE foo@WC
>
> Why should we consider just the last one to be non-pegged?
Perhaps because we don't have to determine what foo's name at the
operative revisions is? (assuming an implicit -rBASE:WORKING) But I
must admit that I'm reaching - I don't know the answer, and it may not
really matter because it...
> will go through to the same diff_wc_wc function in the end, whether we call
> it pegged or non-pegged at this stage.)
> A different way to ask the question is: given that the distinction between
> pegged and non-pegged diffs seems to affect only the user interface, is
> there any reason why we want a conceptual definition of it that excludes
> the BASE<->WC case?
>
I'm now struggling to understand what the difference is at the API level.
Hmm, lets see, svn_client_diff3() can answer 'diff -rX:Y path1@X path2@Y',
where 'path1' and 'path2' are URLs or wc paths, and X and Y are almost
any combination of revisions ('local': BASE, WORKING; 'remote': COMMITTED,
PREV, date, rN).
Hmm, the API documentation says "(Currently, @a path1 and @a path2 must
be the exact same path)", but that's not true, is it, because they're
different in the 'diff -rX:Y --old=path1 --new=path2' case.
On the other hand, svn_client_diff3_peg() answers 'diff -rX:Y path@PEG'
(note: one path only), and one of X or Y must be non-local simply because
diff_wc_wc() doesn't accept a peg revision (and what would it mean if
it did?).
[It does seem odd that we've got a separate set of interfaces for
'non-pegged diffs' rather than just passing svn_opt_revision_unspecified -
in fact, the non-pegged versions can be trivially implemented using the
pegged versions.]
Anyway, back to the question: yes, I think that there is a difference:
a 'pegged' diff implies that we need to go through some kind of
history-tracing to find the name of the items at the operative revisions,
and that isn't something we ever need to do for BASE:WORKING diffs.
> There are more bugs with pegged diffs. For example, the previous section
> of svn_cl__diff() handles the "[-rN:M] --old=TGT@PEG [--new=TGT@PEG]" case,
> but overwrites the "-r" revisions with the peg revisions rather than
> correctly looking the targets up at their peg revisions.
I _think_ that's correct, albeit bizarre. The help says:
2. diff [-r N[:M]] --old=OLD-TGT[@OLDREV] [--new=NEW-TGT[@NEWREV]] \
[PATH...]
2. Display the differences between OLD-TGT as it was seen in OLDREV and
NEW-TGT as it was seen in NEWREV. PATHs, if given, are relative to
OLD-TGT and NEW-TGT and restrict the output to differences for those
paths. OLD-TGT and NEW-TGT may be working copy paths or URL[@REV].
NEW-TGT defaults to OLD-TGT if not specified. -r N makes OLDREV default
to N, -r N:M makes OLDREV default to N and NEWREV default to M.
Notice that 'N' and 'M' don't actually have any part in the diff activity
other than providing defaults for OLDREV and NEWREV. Syntactical sugar,
nothing more.
(and while it might be nice to be able to do 'diff -rX:Y path1@A path2@B',
that's not something that our API offers).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
> On Thu, Mar 09, 2006 at 01:58:33AM +0000, Julian Foad wrote:
>
>>> {
>>> /* The 'svn diff [-r N[:M]] [TARGET[@REV]...]' case matches. */
>>>
>>> /* Here each target is a pegged object. Find out the starting
>>> and ending paths for each target. */
>>[...]
>>> /* Determine if we need to do pegged diffs. */
>>> if (opt_state->start_revision.kind != svn_opt_revision_base
>>> && opt_state->start_revision.kind != svn_opt_revision_working)
>>> pegged_diff = TRUE;
>>
>>That's wrong. In this section, according to the comment, we should assign
>>pegged_diff=TRUE unconditionally.
>
> Nope; 'svn diff foo' is handled by that case too, and that's not a pegged
> diff (though it looks like later code may may it pegged @WORKING, which
> may work anyway). I think it only wants to be pegged if either revision
> is 'non-local'.
I disagree with you. Yes, "svn diff foo" is handled by that case. That means,
"show me the difference between the base revision and the working
pseudo-revision of this object". To me, that's a "pegged diff" because I think
of "the working files" as being a revision of a sort, rather than thinking of
this as a difference between one arbitrary tree and another arbitrary tree.
I can accept that either definition would work in this case, but that's because
pegged and non-pegged diffs are the same thing at this level, it's just a
question of whether the two paths being compared are identical or different. I
don't see the logic in saying that a diff is "pegged" only if at least one
revision is non-local:
diff foo@100 foo@200
diff foo@BASE foo@200
diff foo@100 foo@WC
diff foo@BASE foo@WC
Why should we consider just the last one to be non-pegged? (Note that it will
go through to the same diff_wc_wc function in the end, whether we call it
pegged or non-pegged at this stage.)
A different way to ask the question is: given that the distinction between
pegged and non-pegged diffs seems to affect only the user interface, is there
any reason why we want a conceptual definition of it that excludes the
BASE<->WC case?
There are more bugs with pegged diffs. For example, the previous section of
svn_cl__diff() handles the "[-rN:M] --old=TGT@PEG [--new=TGT@PEG]" case, but
overwrites the "-r" revisions with the peg revisions rather than correctly
looking the targets up at their peg revisions. I'll try to have a look at that
before committing this, in case it throws more light on the best way to do
this, but I'f fairly confident that this current patch is good.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Mar 09, 2006 at 01:58:33AM +0000, Julian Foad wrote:
> Julian Foad wrote:
> >Paul (earlier in the thread) compared the running of diff "BASE:1" with
> >"1:BASE" and found where the latter went wrong:
> >
> If I understand the meaning of "a pegged diff" correctly, it is the
> left-hand side that is wrong here. Both of these should be pegged diffs.
>
Agreed.
> > {
> > /* The 'svn diff [-r N[:M]] [TARGET[@REV]...]' case matches. */
> >
> > /* Here each target is a pegged object. Find out the starting
> > and ending paths for each target. */
> [...]
> > /* Determine if we need to do pegged diffs. */
> > if (opt_state->start_revision.kind != svn_opt_revision_base
> > && opt_state->start_revision.kind != svn_opt_revision_working)
> > pegged_diff = TRUE;
>
> That's wrong. In this section, according to the comment, we should assign
> pegged_diff=TRUE unconditionally.
>
Nope; 'svn diff foo' is handled by that case too, and that's not a pegged
diff (though it looks like later code may may it pegged @WORKING, which
may work anyway). I think it only wants to be pegged if either revision
is 'non-local'.
> I've just checked in a refactoring that removes a fair bit of duplicated
> code from libsvn_client/diff.c.
(Nice work btw! Looks good to me, though I didn't spend too long on it).
> It also fixes another bug in which "svn diff -rBASE:1 WC_PATH" generates
> the wrong output, but my test for that is not right (it passes with and
> without the patch). I have a sandbox in which I can see the effect, but am
> not sure yet exactly how to reproduce it from scratch.
>
I'm quite surprised that the existing diff_tests don't trigger whatever
you're seeing here - they're fairly comprehensive, but I guess we're
missing a particular case. I wouldn't mind knowing what the case is.
> Therefore this is not yet ready for commit, but I thought I ought to share
> it, especially as it might affect Malcolm who seems to be looking at diff
> bugs at the moment.
>
Thanks; I'm mostly working in libsvn_wc, but it's always good to know
what else is going on.
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 18780)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -218,11 +218,7 @@ svn_cl__diff(apr_getopt_t *os,
> opt_state->end_revision.kind = working_copy_present
> ? svn_opt_revision_working : svn_opt_revision_head;
>
> - /* Determine if we need to do pegged diffs. */
> - if (opt_state->start_revision.kind != svn_opt_revision_base
> - && opt_state->start_revision.kind != svn_opt_revision_working)
> - pegged_diff = TRUE;
> -
> + pegged_diff = TRUE;
> }
>
> svn_opt_push_implicit_dot_target(targets, pool);
Like I said above, I think that this is wrong for the (implicit)
BASE:WORKING case.
How about something along the lines of:
/* Determine if we need to do pegged diffs. */
if (!((opt_state->start_revision.kind == svn_opt_revision_base
|| opt_state->start_revision.kind == svn_opt_revision_working)
&& (opt_state->end_revision.kind == svn_opt_revision_base
|| opt_state->end_revision.kind == svn_opt_revision_working)))
pegged_diff = TRUE;
> +#----------------------------------------------------------------------
> +# A base->repos diff used to output an all-lines-deleted diff
> +def diff_base_repos(sbox):
I'm not sure how the conditions being tested here are materially different
to that in e.g. diff_mime_type_changes (the BASE:1 test).
> + # Check for correct output of diff BASE:1
> + out, err = svntest.actions.run_and_verify_svn(None, SVNAnyOutput, [],
> + 'diff', '-rBASE:1')
(I prefer checking the actual expected output rather than programmatically
checking parts of it, though it does make the tests a lot longer).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Julian Foad <ju...@btopenworld.com>.
Julian Foad wrote:
>
> Paul (earlier in the thread) compared the running of diff "BASE:1" with
> "1:BASE" and found where the latter went wrong:
>
> --------------------------------------------------------------------------
>
> (Working Scenario) (Asserting Scenario)
> svn diff BASE:1 URL svn diff 1:BASE URL
> --------------------------------------------------------------------------
> svn/diff-cmd.c svn/diff-cmd.c
> svn__cl_diff() svn__cl_diff()
> pegged_diff is determined only by The first potential problem,
> looking at the start revision pegged_diff == TRUE, so we call:
> kind. It is TRUE iff start
> revision kind is not
> svn_opt_revision_base or
> svn_opt_revision_working.
> So it's FALSE in this case which
> results in call to:
> --------------------------------------------------------------------------
> libsvn_client/diff.c libsvn_client/diff.c
> svn_client_diff3() svn_client_diff_peg3()
If I understand the meaning of "a pegged diff" correctly, it is the left-hand
side that is wrong here. Both of these should be pegged diffs. In svn_cl__diff():
> else
> {
> /* The 'svn diff [-r N[:M]] [TARGET[@REV]...]' case matches. */
>
> /* Here each target is a pegged object. Find out the starting
> and ending paths for each target. */
[...]
> /* Determine if we need to do pegged diffs. */
> if (opt_state->start_revision.kind != svn_opt_revision_base
> && opt_state->start_revision.kind != svn_opt_revision_working)
> pegged_diff = TRUE;
That's wrong. In this section, according to the comment, we should assign
pegged_diff=TRUE unconditionally.
Continuing,
> --------------------------------------------------------------------------
> libsvn_client/diff.c libsvn_client/diff.c
> do_diff() do_diff_peg()
> Call check_paths() and get Our second potential problem, the
> call to check_paths() results in
> the struct diff_paths *diff_paths: the struct diff_paths* diff_paths:
> diff_paths->is_local_rev1 1 diff_paths->is_local_rev1 0
> diff_paths->is_local_rev2 0 diff_paths->is_local_rev2 1
> diff_paths->is_repos_path1 1 diff_paths->is_repos_path1 1
> diff_paths->is_repos_path2 1 diff_paths->is_repos_path2 0
> Here the paths struct accurately But path 2 is a URL at revision
> represents the invalid request BASE, invalid yes, but the paths
> for the BASE rev of an URL. struct doesn't reflect this and
> causes a call to:
OK, here, on the right-hand side, the result is being used wrongly. "...path2"
is inapplicable to a pegged diff, and supposed to be ignored.
I've just checked in a refactoring that removes a fair bit of duplicated code
from libsvn_client/diff.c. I've prepared the attached patch which I believe
fixes these two errors, and thus fixes this bug. It also improves the help
message, which is an unrelated change so ought to go in a separate patch.
It also fixes another bug in which "svn diff -rBASE:1 WC_PATH" generates the
wrong output, but my test for that is not right (it passes with and without the
patch). I have a sandbox in which I can see the effect, but am not sure yet
exactly how to reproduce it from scratch.
Therefore this is not yet ready for commit, but I thought I ought to share it,
especially as it might affect Malcolm who seems to be looking at diff bugs at
the moment.
- Julian
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
>>
>>http://svn.haxx.se/dev/archive-2006-01/0350.shtml
>
> Paul, +1 to commit. Here's a new patch, exactly the same as yours
> except with a custom error message, [...]
-1. This isn't the right solution. It solves the immediate problem, avoiding
the assertion failure, but this explicit check should not be necessary: the
problem (of the user having specified a revision keyword that doesn't make
sense with a URL) should be caught later when the code tries to resolve the
revision specifier to a revision number.
Paul (earlier in the thread) compared the running of diff "BASE:1" with
"1:BASE" and found where the latter went wrong:
--------------------------------------------------------------------------
(Working Scenario) (Asserting Scenario)
svn diff BASE:1 URL svn diff 1:BASE URL
--------------------------------------------------------------------------
svn/diff-cmd.c svn/diff-cmd.c
svn__cl_diff() svn__cl_diff()
pegged_diff is determined only by The first potential problem,
looking at the start revision pegged_diff == TRUE, so we call:
kind. It is TRUE iff start
revision kind is not
svn_opt_revision_base or
svn_opt_revision_working.
So it's FALSE in this case which
results in call to:
--------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
svn_client_diff3() svn_client_diff_peg3()
--------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
do_diff() do_diff_peg()
Call check_paths() and get Our second potential problem, the
call to check_paths() results in
the struct diff_paths *diff_paths: the struct diff_paths* diff_paths:
diff_paths->is_local_rev1 1 diff_paths->is_local_rev1 0
diff_paths->is_local_rev2 0 diff_paths->is_local_rev2 1
diff_paths->is_repos_path1 1 diff_paths->is_repos_path1 1
diff_paths->is_repos_path2 1 diff_paths->is_repos_path2 0
Here the paths struct accurately But path 2 is a URL at revision
represents the invalid request BASE, invalid yes, but the paths
for the BASE rev of an URL. struct doesn't reflect this and
causes a call to:
--------------------------------------------------------------------------
libsvn_client/diff.c libsvn_client/diff.c
diff_repos_repos() diff_repos_wc()
And here we run into the assert:
assert(! svn_path_is_url(path2));
--------------------------------------------------------------------------
libsvn_client/diff.c
diff_prepare_repos_repos()
Here we call
svn_client__get_revision_number()
on a NULL path.
--------------------------------------------------------------------------
libsvn_client/revisions.c
svn_client__get_revision_number()
Returns SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED
svn: A path under version control is needed for this operation
--------------------------------------------------------------------------
Paul wrote about that analysis:
> I'm afraid I don't have a thorough enough grasp of the implications of
> changing svn__cl_diff(), do_diff_peg(), and/or check_paths() to say that
> any of these could be "fixed" in a way that makes my patch unnecessary. I
> can start digging into this some more, but I'm hoping that the above might
> turn on the light bulb for someone who is more familiar with this code.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by kf...@collab.net.
Michael W Thelen <mi...@pietdepsi.com> writes:
> I have to apologize for letting this patch sit so long without being
> addressed. Would any developer be able to review Paul's patch? If not,
> I'll file an issue for in within a few days. Here is a link to Paul's
> patch in the archive:
>
> http://svn.haxx.se/dev/archive-2006-01/0350.shtml
Paul, +1 to commit. Here's a new patch, exactly the same as yours
except with a custom error message, because the generic one wouldn't
be clear in this scenario. The generic one just said "A path under
version control is needed for this operation", to which the creative
user might reply "But of course it's under version control, it's in
the repository!" :-)
Feel free to change the custom error message if you think of a better
phrasing, of course.
-Karl
[[[
Return consistent error on svn diff of special revisions BASE, COMMITTED,
or PREV when operating on a URL.
The original impetus for this patch was to stop svn diff 1:BASE URL from
asserting, and to provide a more useful error message.
Approved by: kfogel
* subversion/svn/diff-cmd.c
(svn_cl__diff): If the starting or ending revision against a URL is
BASE, COMMITTED, or PREV return an error.
]]]
Index: subversion/svn/diff-cmd.c
===================================================================
--- subversion/svn/diff-cmd.c (revision 18760)
+++ subversion/svn/diff-cmd.c (working copy)
@@ -206,6 +206,21 @@
_("Target lists to diff may not contain "
"both working copy paths and URLs"));
+
+ /* Diffing special revisions BASE, COMMITTED, or PREV
+ against a URL is invalid. */
+ if (url_present
+ && (opt_state->start_revision.kind == svn_opt_revision_base
+ || opt_state->end_revision.kind == svn_opt_revision_base
+ || opt_state->start_revision.kind == svn_opt_revision_committed
+ || opt_state->end_revision.kind == svn_opt_revision_committed
+ || opt_state->start_revision.kind == svn_opt_revision_previous
+ || opt_state->end_revision.kind == svn_opt_revision_previous))
+ return svn_error_create
+ (SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED, NULL,
+ _("That symbolic revision name(s) is only for "
+ "working copy paths, not URLs"));
+
if (opt_state->start_revision.kind == svn_opt_revision_unspecified
&& working_copy_present)
opt_state->start_revision.kind = svn_opt_revision_base;
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Michael W Thelen <mi...@pietdepsi.com>.
Paul Burba wrote:
> Julian Foad <ju...@btopenworld.com> wrote on 01/12/2006 07:39:00 PM:
>
>
>>>>>>>diff -r1:BASE file:///home/BURBA/REPOS/SIG5
>>>>>>>Assertion failed: ! svn_path_is_url (path2), [...]
>>
>>Paul Burba wrote:
>>
>>>Anyway, here is my proposed fix. The "opt_state->start_revision.kind
>
> ==
>
>>>svn_opt_revision_base" isn't absolutely necessary since this case is
>>>caught in svn_client__get_revision_number(), but it seems more
>
> appropriate
>
>>>to check it here.
>>
>>My first reaction is: "What is special about BASE?" Why isn't
>>handled the same
>>as PREV and COMMITTED which are the other two revision keywords that
>>require a WC?
>
>
> Hi Julian,
>
> What's special about it is that svn diff x:BASE asserts and the other
> cases are handled, albeit inconsistently. My thought was just to prevent
> the assert - that's my rather lame excuse anyway since I *did* include the
> svn diff BASE:x case which the current code doesn't assert on. As you
> say, it does makes more sense to handle all these invalid diffs in a
> consistent manner...
>
>
>>It would be good to make BASE and PREV and COMMITTED all produce the
>
> same
>
>>error, preferably by all being handled at the same point. At
>>present, PREV and
>>COMMITTED produce:
>>
>>svn diff -r1:PREV file:///home/julianfoad/tmp/vcs/sandbox/trunk
>>svn: 'file:///home/julianfoad/tmp/vcs/sandbox' is not a working copy
>
> ...so to that end here is a new log and patch.
I have to apologize for letting this patch sit so long without being
addressed. Would any developer be able to review Paul's patch? If not,
I'll file an issue for in within a few days. Here is a link to Paul's
patch in the archive:
http://svn.haxx.se/dev/archive-2006-01/0350.shtml
--
Michael W Thelen
It is a mistake to think you can solve any major problems just with
potatoes. -- Douglas Adams
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Paul Burba <pa...@softlanding.com>.
Julian Foad <ju...@btopenworld.com> wrote on 01/12/2006 07:39:00 PM:
> >>>>>diff -r1:BASE file:///home/BURBA/REPOS/SIG5
> >>>>>Assertion failed: ! svn_path_is_url (path2), [...]
>
> Paul Burba wrote:
> > Anyway, here is my proposed fix. The "opt_state->start_revision.kind
==
> > svn_opt_revision_base" isn't absolutely necessary since this case is
> > caught in svn_client__get_revision_number(), but it seems more
appropriate
> > to check it here.
>
> My first reaction is: "What is special about BASE?" Why isn't
> handled the same
> as PREV and COMMITTED which are the other two revision keywords that
> require a WC?
Hi Julian,
What's special about it is that svn diff x:BASE asserts and the other
cases are handled, albeit inconsistently. My thought was just to prevent
the assert - that's my rather lame excuse anyway since I *did* include the
svn diff BASE:x case which the current code doesn't assert on. As you
say, it does makes more sense to handle all these invalid diffs in a
consistent manner...
> It would be good to make BASE and PREV and COMMITTED all produce the
same
> error, preferably by all being handled at the same point. At
> present, PREV and
> COMMITTED produce:
>
> svn diff -r1:PREV file:///home/julianfoad/tmp/vcs/sandbox/trunk
> svn: 'file:///home/julianfoad/tmp/vcs/sandbox' is not a working copy
...so to that end here is a new log and patch.
Thanks for checking this out,
Paul B.
[[[
Return consistent error on svn diff of special revisions BASE, COMMITTED,
or PREV when operating on a URL.
The original impetus for this patch was to stop svn diff 1:BASE URL from
asserting and provide a more useful error message.
* subversion/svn/diff-cmd.c
(svn_cl__diff): If the starting or ending revision against a URL is
BASE,
COMMITTED, or PREV return an error.
]]]
Index: subversion/svn/diff-cmd.c
===================================================================
--- subversion/svn/diff-cmd.c (revision 18091)
+++ subversion/svn/diff-cmd.c (working copy)
@@ -155,6 +155,19 @@
return svn_error_createf (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
_("Target lists to diff may not contain
"
"both working copy paths and URLs"));
+
+ /* Diffing special revisions BASE, COMMITTED, or PREV
+ against a URL is invalid. */
+ if (url_present
+ && (opt_state->start_revision.kind == svn_opt_revision_base
+ || opt_state->end_revision.kind == svn_opt_revision_base
+ || opt_state->start_revision.kind ==
svn_opt_revision_committed
+ || opt_state->end_revision.kind ==
svn_opt_revision_committed
+ || opt_state->start_revision.kind ==
svn_opt_revision_previous
+ || opt_state->end_revision.kind ==
svn_opt_revision_previous)
+)
+ return svn_error_create
+ (SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED, NULL, NULL);
if (opt_state->start_revision.kind == svn_opt_revision_unspecified
&& working_copy_present)
_____________________________________________________________________________
Scanned for SoftLanding Systems, Inc. and SoftLanding Europe Plc by IBM Email Security Management Services powered by MessageLabs.
_____________________________________________________________________________
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] svn diff -r1:BASE URL asserts
Posted by Julian Foad <ju...@btopenworld.com>.
>>>>> diff -r1:BASE file:///home/BURBA/REPOS/SIG5
>>>>>Assertion failed: ! svn_path_is_url (path2), [...]
Paul Burba wrote:
> Anyway, here is my proposed fix. The "opt_state->start_revision.kind ==
> svn_opt_revision_base" isn't absolutely necessary since this case is
> caught in svn_client__get_revision_number(), but it seems more appropriate
> to check it here.
My first reaction is: "What is special about BASE?" Why isn't handled the same
as PREV and COMMITTED which are the other two revision keywords that require a WC?
It would be good to make BASE and PREV and COMMITTED all produce the same
error, preferably by all being handled at the same point. At present, PREV and
COMMITTED produce:
svn diff -r1:PREV file:///home/julianfoad/tmp/vcs/sandbox/trunk
svn: 'file:///home/julianfoad/tmp/vcs/sandbox' is not a working copy
- Julian
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 18079)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -155,6 +155,13 @@
> return svn_error_createf (SVN_ERR_UNSUPPORTED_FEATURE, NULL,
> _("Target lists to diff may not contain
> "
> "both working copy paths and URLs"));
> +
> + /* Diff -rBASE:xxxx | rxxxx:BASE URL is invalid. */
> + if (url_present
> + && (opt_state->start_revision.kind == svn_opt_revision_base
> + || opt_state->end_revision.kind == svn_opt_revision_base))
> + return svn_error_create
> + (SVN_ERR_CLIENT_VERSIONED_PATH_REQUIRED, NULL, NULL);
>
> if (opt_state->start_revision.kind == svn_opt_revision_unspecified
> && working_copy_present)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org