On Wed, Nov 16, 2011 at 04:09:51PM -0800, Alexey Neyman wrote:
> On Sunday, November 06, 2011 10:27:01 am Daniel Shahaf wrote:
> > Perhaps the solution is to make 'svn diff' use --diff-cmd for propdiffs
> > too? It seems that currently --diff-cmd is only used for file content
> > diffs.
>
> I guess this is sort of a feature. As Julian pointed out, the property diffs
> are output using a different hunk format that starts with ## instead of @@.
> Thus invoking an external diff utility (which does not know whether it is
> diffing, content or property) would actually make the problem worse.
>
> FWIW, I implemented the --patch option for 'svn diff' as suggested - implying
> no property diffs and copies-as-additions. Patch against trunk, r1202879;
> survives 'make check'.
I'd prefer keeping --no-diff-properties and add a --patch option later
which implies --no-diff-properties and maybe other options.
I'm not sure I like the name --patch.
Maybe call it --patch-compatible or something?
Can you send patches generated with 'svn diff -xp' please? Thanks.
Some review of your current patch inline below.
> [[[
> * subversion/svn/cl.h
> * subversion/svn/main.c
> New option, --patch for svn diff.
>
> * subversion/include/svn_client.h
> (svn_client_diff6,svn_client_diff_peg6): New argument, ignore_prop_diff.
> (svn_client_diff5,svn_client_diff_peg5): Update comments.
>
> * subversion/libsvn_client/deprecated.c
> (svn_client_diff5,svn_client_diff_peg5): Pass FALSE as ignore_prop_diff.
>
> * subversion/libsvn_client/diff.c
> (diff_cmd_baton): New field, ignore_prop_diff
> (diff_props_changed): Do nothing if diff_cmd_baton->ignore_prop_diff is set.
> (svn_client_diff6,svn_client_diff_peg6): Pass ignore_prop_diff downstream
> via diff_cmd_baton.
>
> * subversion/svn/diff-cmd.c
> (svn_cl__diff): Handle --patch: ignore property diff, force
> --show-copies-as-adds.
>
> * subversion/svn/log-cmd.c
> (log_entry_receiver): Request property changes from svn_client_diff6.
> ]]]
> Index: subversion/svn/cl.h
> ===================================================================
> --- subversion/svn/cl.h (revision 1202879)
> +++ subversion/svn/cl.h (working copy)
> @@ -229,6 +229,7 @@
> svn_boolean_t show_diff; /* produce diff output (maps to --diff) */
> svn_boolean_t internal_diff; /* override diff_cmd in config file */
> svn_boolean_t use_git_diff_format; /* Use git's extended diff format */
> + svn_boolean_t use_patch_diff_format; /* Output compatible with GNU patch */
> svn_boolean_t allow_mixed_rev; /* Allow operation on mixed-revision WC */
> svn_boolean_t include_externals; /* Recurses (in)to file & dir externals */
> } svn_cl__opt_state_t;
> Index: subversion/svn/diff-cmd.c
> ===================================================================
> --- subversion/svn/diff-cmd.c (revision 1202879)
> +++ subversion/svn/diff-cmd.c (working copy)
> @@ -171,6 +171,9 @@
> const char *old_target, *new_target;
> apr_pool_t *iterpool;
> svn_boolean_t pegged_diff = FALSE;
> + svn_boolean_t show_copies_as_adds =
> + opt_state->use_patch_diff_format ? TRUE : opt_state->show_copies_as_adds;
This above change would be part of a second patch to add the
--patch-compatible option.
> + svn_boolean_t ignore_prop_diff = opt_state->use_patch_diff_format;
> int i;
> const svn_client_diff_summarize_func_t summarize_func =
> (opt_state->xml ? summarize_xml : summarize_regular);
> @@ -361,8 +364,9 @@
> opt_state->depth,
> ! opt_state->notice_ancestry,
> opt_state->no_diff_deleted,
> - opt_state->show_copies_as_adds,
> + show_copies_as_adds,
> opt_state->force,
> + ignore_prop_diff,
> opt_state->use_git_diff_format,
> svn_cmdline_output_encoding(pool),
> outstream,
> @@ -406,8 +410,9 @@
> opt_state->depth,
> ! opt_state->notice_ancestry,
> opt_state->no_diff_deleted,
> - opt_state->show_copies_as_adds,
> + show_copies_as_adds,
> opt_state->force,
> + ignore_prop_diff,
> opt_state->use_git_diff_format,
> svn_cmdline_output_encoding(pool),
> outstream,
> Index: subversion/svn/log-cmd.c
> ===================================================================
> --- subversion/svn/log-cmd.c (revision 1202879)
> +++ subversion/svn/log-cmd.c (working copy)
> @@ -309,6 +309,7 @@
> TRUE, /* no diff deleted */
> FALSE, /* show copies as adds */
> FALSE, /* ignore content type */
> + FALSE, /* ignore prop diff */
> FALSE, /* use git diff format */
> svn_cmdline_output_encoding(pool),
> outstream,
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 1202879)
> +++ subversion/svn/main.c (working copy)
> @@ -124,6 +124,7 @@
> opt_diff,
> opt_internal_diff,
> opt_use_git_diff_format,
> + opt_use_patch_diff_format,
> opt_allow_mixed_revisions,
> opt_include_externals,
> } svn_cl__longopt_t;
> @@ -341,6 +342,12 @@
> N_("override diff-cmd specified in config file")},
> {"git", opt_use_git_diff_format, 0,
> N_("use git's extended diff format")},
> + {"patch", opt_use_patch_diff_format, 0,
> + N_("generate diff suitable for GNU patch;\n"
> + " "
> + "implies show-copies-as-adds and ignores property\n"
> + " "
> + "differences")},
> {"allow-mixed-revisions", opt_allow_mixed_revisions, 0,
> N_("Allow merge into mixed-revision working copy.\n"
> " "
> @@ -538,7 +545,7 @@
> {'r', 'c', opt_old_cmd, opt_new_cmd, 'N', opt_depth, opt_diff_cmd,
> opt_internal_diff, 'x', opt_no_diff_deleted, opt_show_copies_as_adds,
> opt_notice_ancestry, opt_summarize, opt_changelist, opt_force, opt_xml,
> - opt_use_git_diff_format} },
> + opt_use_git_diff_format, opt_use_patch_diff_format} },
> { "export", svn_cl__export, {0}, N_
> ("Create an unversioned copy of a tree.\n"
> "usage: 1. export [-r REV] URL[@PEGREV] [PATH]\n"
> @@ -2046,6 +2053,9 @@
> case opt_internal_diff:
> opt_state.internal_diff = TRUE;
> break;
> + case opt_use_patch_diff_format:
> + opt_state.use_patch_diff_format = TRUE;
> + break;
> case opt_use_git_diff_format:
> opt_state.use_git_diff_format = TRUE;
> break;
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 1202879)
> +++ subversion/include/svn_client.h (working copy)
> @@ -2877,6 +2877,7 @@
> svn_boolean_t no_diff_deleted,
> svn_boolean_t show_copies_as_adds,
> svn_boolean_t ignore_content_type,
> + svn_boolean_t ignore_prop_diff,
> svn_boolean_t use_git_diff_format,
> const char *header_encoding,
> svn_stream_t *outstream,
> @@ -2886,7 +2887,8 @@
> apr_pool_t *pool);
>
> /** Similar to svn_client_diff6(), but with @a outfile and @a errfile,
> - * instead of @a outstream and @a errstream.
> + * instead of @a outstream and @a errstream, and always showing property
> + * changes.
> *
> * @deprecated Provided for backward compatibility with the 1.7 API.
> * @since New in 1.7.
> @@ -3035,6 +3037,7 @@
> svn_boolean_t no_diff_deleted,
> svn_boolean_t show_copies_as_adds,
> svn_boolean_t ignore_content_type,
> + svn_boolean_t ignore_prop_diff,
We cannot add new arguments to already released, stable, APIs.
You'll need to create a new version of svn_client_diff6 called
svn_client_diff7 and re-implement svn_client_diff6 as a wrapper around
svn_client_diff7 which passes FALSE for ignore_prop_diff.
Rest looks good to me.
Can you provide a follow-up patch? Thanks.
> svn_boolean_t use_git_diff_format,
> const char *header_encoding,
> svn_stream_t *outstream,
> @@ -3044,7 +3047,8 @@
> apr_pool_t *pool);
>
> /** Similar to svn_client_diff_peg6(), but with @a outfile and @a errfile,
> - * instead of @a outstream and @a errstream.
> + * instead of @a outstream and @a errstream, and always showing property
> + * changes.
> *
> * @deprecated Provided for backward compatibility with the 1.7 API.
> * @since New in 1.7.
> Index: subversion/libsvn_client/deprecated.c
> ===================================================================
> --- subversion/libsvn_client/deprecated.c (revision 1202879)
> +++ subversion/libsvn_client/deprecated.c (working copy)
> @@ -864,7 +864,7 @@
> return svn_client_diff6(diff_options, path1, revision1, path2,
> revision2, relative_to_dir, depth,
> ignore_ancestry, no_diff_deleted,
> - show_copies_as_adds, ignore_content_type,
> + show_copies_as_adds, ignore_content_type, FALSE,
> use_git_diff_format, header_encoding,
> outstream, errstream, changelists, ctx, pool);
> }
> @@ -992,6 +992,7 @@
> no_diff_deleted,
> show_copies_as_adds,
> ignore_content_type,
> + FALSE,
> use_git_diff_format,
> header_encoding,
> outstream,
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 1202879)
> +++ subversion/libsvn_client/diff.c (working copy)
> @@ -762,6 +762,9 @@
> relative to for output generation (see issue #2723). */
> const char *relative_to_dir;
>
> + /* Whether property differences are ignored. */
> + svn_boolean_t ignore_prop_diff;
> +
> /* Whether we're producing a git-style diff. */
> svn_boolean_t use_git_diff_format;
>
> @@ -802,6 +805,10 @@
> apr_array_header_t *props;
> svn_boolean_t show_diff_header;
>
> + /* If property differences are ignored, there's nothing to do. */
> + if (diff_cmd_baton->ignore_prop_diff)
> + return SVN_NO_ERROR;
> +
> SVN_ERR(svn_categorize_props(propchanges, NULL, NULL, &props,
> scratch_pool));
>
> @@ -2398,6 +2405,7 @@
> svn_boolean_t no_diff_deleted,
> svn_boolean_t show_copies_as_adds,
> svn_boolean_t ignore_content_type,
> + svn_boolean_t ignore_prop_diff,
> svn_boolean_t use_git_diff_format,
> const char *header_encoding,
> svn_stream_t *outstream,
> @@ -2427,6 +2435,7 @@
>
> diff_cmd_baton.force_empty = FALSE;
> diff_cmd_baton.force_binary = ignore_content_type;
> + diff_cmd_baton.ignore_prop_diff = ignore_prop_diff;
> diff_cmd_baton.relative_to_dir = relative_to_dir;
> diff_cmd_baton.use_git_diff_format = use_git_diff_format;
> diff_cmd_baton.no_diff_deleted = no_diff_deleted;
> @@ -2455,6 +2464,7 @@
> svn_boolean_t no_diff_deleted,
> svn_boolean_t show_copies_as_adds,
> svn_boolean_t ignore_content_type,
> + svn_boolean_t ignore_prop_diff,
> svn_boolean_t use_git_diff_format,
> const char *header_encoding,
> svn_stream_t *outstream,
> @@ -2480,6 +2490,7 @@
>
> diff_cmd_baton.force_empty = FALSE;
> diff_cmd_baton.force_binary = ignore_content_type;
> + diff_cmd_baton.ignore_prop_diff = ignore_prop_diff;
> diff_cmd_baton.relative_to_dir = relative_to_dir;
> diff_cmd_baton.use_git_diff_format = use_git_diff_format;
> diff_cmd_baton.no_diff_deleted = no_diff_deleted;
