[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] make diff against copy-source by default

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Wed, 05 Jan 2011 11:41:52 +0000

On Thu, 2010-12-30, Prabhu Gnana Sundar wrote:
> After a few discussions about the inconsistent behaviour of 'svn diff'
> in different scenarios, Kamesh suggested that let 'svn diff' be done
> against the copy-source by default, making use of the copyfrom info.
> Now this patch would do 'diff' against the copy-source by default,
> whereas the prevailing default behaviour(of showing a *modified copy* as
> all adds) can be achieved by using the '--show-copies-as-adds'
> switch(that already exists in the code-base!).

I like this change, in principle.

We will have to decide whether it is backwards-compatible enough or
whether we need to provide an additional compatibility mode. In order
to help us decide, it would be very useful if you could provide a
summary of the behavioural changes. For example, maybe some tables
something like this would be a good way to summarize the changes:

Type of diff shown BEFORE this patch, WITHOUT --show-copies-as-adds:
| | copied only | copied and modified | ... |
| | | | |
| WC-WC diff | | | |
| | | | |
| WC-repo diff | | | |
| | | | |
| repo-repo diff | | | |
| | | | |

Type of diff shown BEFORE this patch, WITH --show-copies-as-adds:

Type of diff shown AFTER this patch, WITHOUT --show-copies-as-adds:

Type of diff shown AFTER this patch, WITH --show-copies-as-adds:

Or whatever rows and columns best convey the information.

Are there any differences with different (old) versions of Subversion
server? (I seem to recall that copy-from data was not always supplied,
but I am not sure of the details.)

> I was quickly in to it and after spending some time in the process I
> came to know that several existing 'test cases' *failed* due to the
> change in the behaviour of the 'svn diff'. Later found that the 'wc
> editor' needs to be taught to handle this behaviour (handling the
> copyfrom info and retrieving it from the repository as against working
> copy text-base).
> This took quite some amount of time...
> See [1] below of the mailer.py usecase which tries to get the same
> output as my patch do.
> May be that's the behaviour of the 'diff' that is prefered ? :)
> I have attached the patch and the log message with this mail. Please
> review and respond.

I only had a very quick look at the patch. The first thing that catches
my eye is this:

> * subversion/libsvn_wc/diff.c
> (): included the svn_ra.h header file.
> (): persisted the ra_session, copyfrom file, copyfrom path and pristine properties
> in the edit_baton.
> (make_edit_baton) : introduced the ra_session.
> (get_file_from_ra): newly added to fetch file from repository.
> (add_file) : support copyfrom by making use of the copyfrom info.
> (apply_textdelta) : makes use of the copyfrom_temp file making use of the
> copyfrom info.
> (close_file) : makes use of the copyfrom info and tweaked the file_changed to
> display the copyfrom revision number.
> (svn_wc_get_diff_editor6) : introduced the ra_session.
> pass NULL for ra_session to make_edit_baton.

Isn't it a layering violation for libsvn_wc to know about libsvn_ra?
Maybe this needs to use callbacks or something, so that all the RA
knowledge remains in libsvn_client.

- Julian
Received on 2011-01-05 12:42:33 CET

This is an archived mail posted to the Subversion Dev mailing list.