r18810: svn_client_diff_peg3() with unspecified peg revision

[Julian Foad <ju...@btopenworld.com>]

Malcolm,

This change included documenting that

   svn_client_diff_peg3(path, peg=unspecified, rev1, rev2)

will mean

   svn_client_diff3(path, rev1, path, rev2)

However, I think that's confusing two different semantics.  Please read my 
"Pegged diffs" reply that I have just sent to an earlier thread, first, and see 
if you agree with my analysis of what these concepts and terms mean.

What we call a "pegged" diff means comparing the two states (at rev1 and rev2) 
of the OBJECT found at path@peg.  The non-pegged diff compares two paths 
(identical in this case) that may not represent the same object, and so will 
give different results if the object has been moved.

Why do you want that new meaning?  What was the circumstance that caused it to 
be called in this way?

I believe it should be an error to request a pegged diff with an unspecified peg.


> ------------------------------------------------------------------------
> r18810 | malcolm | 2006-03-10 12:41:18 +0000 (Fri, 10 Mar 2006) | 21 lines
> 
> Fix a bug whereby svn_client_diff3_peg() and
> svn_client_diff_summarize_peg() would return a bogus error when called
> with an unspecified peg-revision and two 'local' revisions (e.g. BASE
> and WORKING), matching the behaviour of our other client functions in
> allowing the peg-revision to be optional.

As for matching other functions, I don't think they behave like this; instead, 
if the peg is unspecified, they choose a default peg and use it.


> * subversion/libsvn_client/diff.c
>   (check_paths): Determine if the diff is pegged by looking at the
>     diff parameters' peg-revision field rather than by requiring the
>     caller to tell us.  Document the behaviour of the function for
>     pegged diffs.
>   (do_diff, do_diff_summarize): Remove the pegged argument (only used
>     in the calls to check_paths()), and update calls to check_paths().
>   (svn_client_diff3, svn_client_diff_peg3): Update calls to do_diff().
>   (svn_client_diff_summarize, svn_client_diff_summarize_peg): Update
>     calls to do_diff_summarize().
> 
> * subversion/include/svn_client.h
>   (svn_client_diff_peg3, svn_client_diff_summarize_peg): Document the
>     behaviour of these functions with an unspecified peg-revision.
> 

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: r18810: svn_client_diff_peg3() with unspecified peg revision

[Malcolm Rowe <ma...@farside.org.uk>]

Hi Julian,

I hope to have some time to reply to your larger email later on tonight,
but for the moment...

On Mon, Mar 13, 2006 at 11:20:21AM +0000, Julian Foad wrote:
> Why do you want that new meaning?
> 
> I believe it should be an error to request a pegged diff with an 
> unspecified peg.
> 

Well, I think you're right in pointing out that 'pegged diff' is a
particularly bad term; it's really 'single-target (with perhaps-optional
peg revision)' vs 'two-target'.  So we're talking about the first case.

Our other client functions allow a peg-revision to be optional; I had
assumed that the lack of documentation on this for the single-target diff
case was an oversight rather than a deliberate omission.  Though now
I actually double-check, I see that in the 'svn diff' case, the caller
provides a default.

> >Fix a bug whereby svn_client_diff3_peg() and
> >svn_client_diff_summarize_peg() would return a bogus error when called
> >with an unspecified peg-revision and two 'local' revisions (e.g. BASE
> >and WORKING), matching the behaviour of our other client functions in
> >allowing the peg-revision to be optional.
> 
> As for matching other functions, I don't think they behave like this; 
> instead, if the peg is unspecified, they choose a default peg and use it.
> 

You're right (typically svn_opt_revision_head for URLs and
svn_opt_revision_working for WC targets).  So what this change should
possibly have been described as is "allow svn_opt_revision_unspecified
to be used to imply a peg revision of svn_opt_revision_working for wc-wc
diffs", although that's not the way it's implemented.

> What was the circumstance that caused it to be called in this way?

Ah, none, from our codebase anyway.  Bear in mind that providing an
unspecified peg-revision has always worked (except in the BASE:WORKING
case) and so I don't think we can necessarily define it as invalid ex
post facto, though I'm no longer sure that my [documentation] changes
were the right thing to do.

Regards,
Malcolm

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org