You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Matthias Gerstner <Ma...@ncp-e.com> on 2014/07/17 13:22:19 UTC
[PATCH] optionally disable normalization of working copy files in
diff invocations
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
--
Matthias Gerstner, Dipl.-Wirtsch.-Inf. (FH)
Entwicklung
NCP engineering GmbH
Domb�hler Stra�e 2, D-90449, N�rnberg
Gesch�ftsf�hrer Peter S�ll, HRB-Nr: 77 86 N�rnberg
Telefon: +49 911 9968-153, Fax: +49 911 9968-229
E-Mail: Matthias.Gerstner@ncp-e.com
Internet: http://www.ncp-e.com
Re: [PATCH] optionally disable normalization of working copy files in diff invocations
Posted by Julian Foad <ju...@btopenworld.com>.
> Matthias Gerstner wrote:
>> 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.
>> ----
Hi Matthias, thanks for the patch. I understand why this is useful, and basically I agree.
The main point I want to make is that the user interface you chose is an example of the "X Y problem" <http://mywiki.wooledge.org/XyProblem>. What does the user really want? To have their actual working file loaded in the external diff program. What does the user tell Subversion?
svn diff --don't-normalize-the-content
# in fact I don't care about normalization but I learnt that
# this option makes Subversion do what I really want
A better interface, the user would tell Subversion what they really want:
svn diff --load-my-actual-file-not-a-copy-of-it
# and I learnt that I won't have normalization in this case
See the difference?
(It's possible Subversion may, today or tomorrow, pass a temporary file for reasons other than normalizing the content. It's also possible that Subversion may, tomorrow, be able to normalize the keyword expansions in the working file, run the diff, and then put them back again; that's not necessarily a good idea for various reasons, but in principle things like that are possible. So disabling normalization does not logically imply getting the original file, and vice-versa.)
I'd like the interface (option name, help text, C symbols, API docs) to say what it means.
Stefan Sperling wrote:
> 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.
I agree that yet another command-line option is somewhat unwelcome, but, as you know, we're generally careful not to change the user interface unnecessarily, because we don't want to break people's existing scripts and work flows. This does seem to me like a case where that would potentially cause unnecessary disruption to people who are already using an external diff tool and have adapted to the way it currently behaves. For example, I once wrote an external diff tool plug-in script which took Subversion's diff parameters and then found and opened the actual WC working file instead, for this very reason.
It would be better if we could make it easy to get the new behaviour without forcing users of the old behaviour to change their diff set-ups.
Since you point out that this is only relevant when using a third party diff tool, it would be a good idea to make it a configuration file option. It would be silly to be able to configure an external diff tool in the config file (as we already can) but then have to add the '--whatever' option manually on every 'svn diff' command.
Then the question is, is it useful to have a dedicated command-line option as well? I think perhaps not. (There's always the ability to specify a config option on the command line using "--config-option=...".)
> 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 rest of Stefan's comments, above, I agree with (and I haven't looked further into those or into the patch itself).
- Julian
Re: [PATCH] optionally disable normalization of working copy files
in diff invocations
Posted by Stefan Sperling <st...@elego.de>.
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,