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
