On Thu, Jul 17, 2014 at 01:22:19PM +0200, Matthias Gerstner wrote:
> Hello,
>
> attached is a patch against the current subversion trunk state (revision
> 1611327) that fixes a problem when using external diff programs that's
> also been described years ago here:
>
> http://svn.haxx.se/users/archive-2008-10/0664.shtml
>
> ----
> When passing the new diff command line option --no-normalization then
> svn diff won't create and pass on a temporary, normalized version of a
> file for local working copy files.
>
> This normalization is the default behaviour when svn diff encounters
> files that have the svn:keywords or svn:eol-style properties set, such
> that the base version and the working copy version of the file have the
> same format.
>
> This makes it impossible to edit diffed files when using an external
> --diff-cmd that supports editing, because the file passed to the
> external tool is a temporary file that will be deleted afterwards,
> instead of the original working copy file.
>
> When passing --no-normalization the original file is passed to the diff
> tool instead. External tools that can ignore whitespace differences
> (present due to svn:eol-style) can still display decent diffs and the
> benefit of editing the diffed files in place is helpful.
> ----
>
> Best regards,
>
> Matthias Gerstner
I'd hope we could address this without public API changes and
without adding yet another command line option.
How about we make this the default if a third party diff tool is used?
This way, third party diff tools will always display differences in
keywords, and possibly EOLs. I don't think this is much of a problem,
except for cases where all lines are considered different because of
end-of-line differences. I would hope most 3rd party tools have
options to control this behaviour. But perhaps my idea is controversial,
in which case we won't get by without adding a new option for this.
In terms of coding style, I'd use a boolean that switches normal
form on, rather than off. I find it easier to keep track of this way.
The interaction between use_text_base and your new no_normalization
flag isn't made clear. You can't have both!
Also, your change only addresses BASE->WORKING diffs, as far as I can tell.
What about REPOS->WORKING or WORKING->REPOS diffs?
The diff below (compile-tested but otherwise untested) shows how
I would try to address this without any public API changes.
Does this do what you want?
Index: subversion/include/private/svn_wc_private.h
===================================================================
--- subversion/include/private/svn_wc_private.h (revision 1611309)
+++ subversion/include/private/svn_wc_private.h (working copy)
@@ -1578,6 +1578,10 @@ svn_wc__get_switch_editor(const svn_delta_editor_t
* Normally, the difference from repository->working_copy is shown.
* If @a reverse_order is TRUE, then show working_copy->repository diffs.
*
+ * If @a use_normal_form is TRUE, normalize file content to repository-normal
+ * form before comparison. If @a use_text_base is TRUE then @use_normal_form
+ * has no effect since pristine content is always in normal form.
+ *
* If @a cancel_func is non-NULL, it will be used along with @a cancel_baton
* to periodically check if the client has canceled the operation.
*
@@ -1623,6 +1627,7 @@ svn_wc__get_diff_editor(const svn_delta_editor_t *
svn_boolean_t use_text_base,
svn_boolean_t reverse_order,
svn_boolean_t server_performs_filtering,
+ svn_boolean_t use_normal_form,
const apr_array_header_t *changelist_filter,
const svn_diff_tree_processor_t *diff_processor,
svn_cancel_func_t cancel_func,
@@ -1845,6 +1850,7 @@ svn_wc__diff7(const char **root_relpath,
const char *local_abspath,
svn_depth_t depth,
svn_boolean_t ignore_ancestry,
+ svn_boolean_t use_normal_form,
const apr_array_header_t *changelist_filter,
const svn_diff_tree_processor_t *diff_processor,
svn_cancel_func_t cancel_func,
Index: subversion/libsvn_client/diff.c
===================================================================
--- subversion/libsvn_client/diff.c (revision 1611309)
+++ subversion/libsvn_client/diff.c (working copy)
@@ -1638,6 +1638,7 @@ diff_wc_wc(const char **root_relpath,
apr_pool_t *scratch_pool)
{
const char *abspath1;
+ diff_writer_info_t *dwi = diff_processor->baton;
SVN_ERR_ASSERT(! svn_path_is_url(path1));
SVN_ERR_ASSERT(! svn_path_is_url(path2));
@@ -1670,7 +1671,9 @@ diff_wc_wc(const char **root_relpath,
SVN_ERR(svn_wc__diff7(root_relpath, root_is_dir,
ctx->wc_ctx, abspath1, depth,
- ignore_ancestry, changelists,
+ ignore_ancestry,
+ dwi->diff_cmd != NULL, /* use_normal_form */
+ changelists,
diff_processor,
ctx->cancel_func, ctx->cancel_baton,
result_pool, scratch_pool));
@@ -1890,6 +1893,7 @@ diff_repos_wc(const char **root_relpath,
const char *copy_root_abspath;
const char *target_url;
svn_client__pathrev_t *loc1;
+ diff_writer_info_t *dwi = diff_processor->baton;
SVN_ERR_ASSERT(! svn_path_is_url(path2));
@@ -2052,6 +2056,7 @@ diff_repos_wc(const char **root_relpath,
rev2_is_base,
reverse,
server_supports_depth,
+ dwi->diff_cmd != NULL, /* use_normal_form */
changelists,
diff_processor,
ctx->cancel_func, ctx->cancel_baton,
Index: subversion/libsvn_wc/deprecated.c
===================================================================
--- subversion/libsvn_wc/deprecated.c (revision 1611309)
+++ subversion/libsvn_wc/deprecated.c (working copy)
@@ -2031,7 +2031,7 @@ svn_wc_get_diff_editor6(const svn_delta_editor_t *
depth,
use_git_diff_format, use_text_base,
reverse_order, server_performs_filtering,
- changelist_filter,
+ TRUE, changelist_filter,
diff_processor,
cancel_func, cancel_baton,
result_pool, scratch_pool));
Index: subversion/libsvn_wc/diff.h
===================================================================
--- subversion/libsvn_wc/diff.h (revision 1611309)
+++ subversion/libsvn_wc/diff.h (working copy)
@@ -136,6 +136,7 @@ svn_wc__diff_base_working_diff(svn_wc__db_t *db,
const svn_diff_tree_processor_t *processor,
void *processor_dir_baton,
svn_boolean_t diff_pristine,
+ svn_boolean_t use_normal_form,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *scratch_pool);
Index: subversion/libsvn_wc/diff_editor.c
===================================================================
--- subversion/libsvn_wc/diff_editor.c (revision 1611309)
+++ subversion/libsvn_wc/diff_editor.c (working copy)
@@ -115,6 +115,9 @@ struct edit_baton_t
/* Possibly diff repos against text-bases instead of working files. */
svn_boolean_t diff_pristine;
+ /* Use repository-normal form of working files. */
+ svn_boolean_t use_normal_form;
+
/* Hash whose keys are const char * changelist names. */
apr_hash_t *changelist_hash;
@@ -238,7 +241,9 @@ struct file_baton_t
* IGNORE_ANCESTRY defines whether to utilize node ancestry when
* calculating diffs. USE_TEXT_BASE defines whether to compare
* against working files or text-bases. REVERSE_ORDER defines which
- * direction to perform the diff.
+ * direction to perform the diff. USE_NORMAL_FORM says whether to
+ * normalize WORKING files to repository-normal form for diffing
+ * (i.e. it only matters if USE_TEXT_BASE is FALSE).
*
* CHANGELIST_FILTER is a list of const char * changelist names, used to
* filter diff output responses to only those items in one of the
@@ -255,6 +260,7 @@ make_edit_baton(struct edit_baton_t **edit_baton,
svn_boolean_t ignore_ancestry,
svn_boolean_t use_text_base,
svn_boolean_t reverse_order,
+ svn_boolean_t use_normal_form,
const apr_array_header_t *changelist_filter,
svn_cancel_func_t cancel_func,
void *cancel_baton,
@@ -278,6 +284,7 @@ make_edit_baton(struct edit_baton_t **edit_baton,
eb->ignore_ancestry = ignore_ancestry;
eb->local_before_remote = reverse_order;
eb->diff_pristine = use_text_base;
+ eb->use_normal_form = use_normal_form;
eb->changelist_hash = changelist_hash;
eb->cancel_func = cancel_func;
eb->cancel_baton = cancel_baton;
@@ -396,6 +403,7 @@ svn_wc__diff_base_working_diff(svn_wc__db_t *db,
const svn_diff_tree_processor_t *processor,
void *processor_dir_baton,
svn_boolean_t diff_pristine,
+ svn_boolean_t use_normal_form,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *scratch_pool)
@@ -505,7 +513,7 @@ svn_wc__diff_base_working_diff(svn_wc__db_t *db,
db, local_abspath,
working_checksum,
scratch_pool, scratch_pool));
- else if (! (had_props || props_mod))
+ else if (! (had_props || props_mod) || ! (files_same || use_normal_form))
local_file = local_abspath;
else if (files_same)
local_file = pristine_file;
@@ -817,6 +825,7 @@ walk_local_nodes_diff(struct edit_baton_t *eb,
eb->changelist_hash,
eb->processor, dir_baton,
eb->diff_pristine,
+ eb->use_normal_form,
eb->cancel_func,
eb->cancel_baton,
scratch_pool));
@@ -2243,6 +2252,7 @@ svn_wc__get_diff_editor(const svn_delta_editor_t *
svn_boolean_t use_text_base,
svn_boolean_t reverse_order,
svn_boolean_t server_performs_filtering,
+ svn_boolean_t use_normal_form,
const apr_array_header_t *changelist_filter,
const svn_diff_tree_processor_t *diff_processor,
svn_cancel_func_t cancel_func,
@@ -2265,7 +2275,8 @@ svn_wc__get_diff_editor(const svn_delta_editor_t *
anchor_abspath, target,
diff_processor,
depth, ignore_ancestry,
- use_text_base, reverse_order, changelist_filter,
+ use_text_base, reverse_order, use_normal_form,
+ changelist_filter,
cancel_func, cancel_baton,
result_pool));
Index: subversion/libsvn_wc/diff_local.c
===================================================================
--- subversion/libsvn_wc/diff_local.c (revision 1611309)
+++ subversion/libsvn_wc/diff_local.c (working copy)
@@ -89,6 +89,9 @@ struct diff_baton
/* Should this diff ignore node ancestry? */
svn_boolean_t ignore_ancestry;
+ /* Should this diff use repos-normal form for working files? */
+ svn_boolean_t use_normal_form;
+
/* Hash whose keys are const char * changelist names. */
apr_hash_t *changelist_hash;
@@ -364,6 +367,7 @@ diff_status_callback(void *baton,
? eb->cur->baton
: NULL,
FALSE,
+ eb->use_normal_form,
eb->cancel_func,
eb->cancel_baton,
scratch_pool));
@@ -435,6 +439,7 @@ svn_wc__diff7(const char **root_relpath,
const char *local_abspath,
svn_depth_t depth,
svn_boolean_t ignore_ancestry,
+ svn_boolean_t use_normal_form,
const apr_array_header_t *changelist_filter,
const svn_diff_tree_processor_t *diff_processor,
svn_cancel_func_t cancel_func,
@@ -478,6 +483,7 @@ svn_wc__diff7(const char **root_relpath,
eb.db = wc_ctx->db;
eb.processor = diff_processor;
eb.ignore_ancestry = ignore_ancestry;
+ eb.use_normal_form = use_normal_form;
eb.pool = scratch_pool;
if (changelist_filter && changelist_filter->nelts)
@@ -564,6 +570,7 @@ svn_wc_diff6(svn_wc_context_t *wc_ctx,
wc_ctx, local_abspath,
depth,
ignore_ancestry,
+ TRUE, /* use_normal_form */
changelist_filter,
processor,
cancel_func, cancel_baton,
