You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Alexey Neyman <st...@att.net> on 2011/11/03 20:05:34 UTC
Patch ping, 5 years later: removing properties
Hi all,
Sorry for a long delay :) 5 years ago, I sent a patch to the mailing list that
would allow to specify what to include/exclude from the diff output:
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=742594
There were two problems this patch tried to address:
1. There was no way to exclude properties from the diff output.
2. There was no way to obtain a list of modified/added/deleted files and
whether the content and/or properties have been modified.
I got a response from Ben Collins-Sussman that the patch I sent is superseded
by --summarize option which was implemented in the trunk (which would
eventually become 1.4):
http://svn.haxx.se/dev/archive-2006-06/0202.shtml
Actually, --summarize only addresses the issue #2, but not the issue #1. As to
issue #1, ability to disable property diffs is still missing from Subversion.
Common wisdom suggests running the diff through 'filterdiff --clean':
http://stackoverflow.com/questions/2755848/how-do-you-get-subversion-diff-
summary-to-ignore-mergeinfo-properties
http://stackoverflow.com/questions/402522/is-there-a-metadata-exclusion-
filter-for-the-svn-diff-command
But this is still unreliable: if property text includes a diff-like chunk, it
will go through.
What is the sentiment about implementing --no-property-diff option to 'svn
diff'? I can implement it if there's agreement such option is needed.
Regards,
Alexey.
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Feb 01, 2012 at 12:12:39PM -0800, Alexey Neyman wrote:
> Hi Stefan,
>
> Oops, I forgot to send it out after applying the feedback comments. Nothing to
> be sorry about. Here it is, updated to apply to r1239239.
Thanks! Committed in r1239553.
I'll make some more follow-up tweaks, but nothing major.
Thanks a lot for this submission.
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Alexey Neyman <st...@att.net>.
Hi Stefan,
Oops, I forgot to send it out after applying the feedback comments. Nothing to
be sorry about. Here it is, updated to apply to r1239239.
[[[
* subversion/svn/cl.h
* subversion/svn/main.c
New options, --no-diff-properties and --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 and --no-diff-properties.
* subversion/svn/log-cmd.c
(log_entry_receiver): Request property changes from svn_client_diff6.
]]]
Regards,
Alexey.
On Wednesday, February 01, 2012 09:53:00 am Stefan Sperling wrote:
> Alexey,
>
> are you still working on this patch?
>
> Since your last submission there has been some feedback and I think
> we've reached consensus. I am sorry that our discussion flip-flopped
> a little and mislead you.
>
> I would very much like to see a version of your patch which
> incorporates the feedback you've received since your last submission.
>
> If you don't have time or interested to work on this patch anymore
> I'd like to pick it up and finish it. Else I will wait for an updated
> patch from you.
>
> Thanks!
>
> On Thu, Dec 22, 2011 at 11:18:18AM +0000, Julian Foad wrote:
> > Stefan Sperling wrote:
> > > Alexey Neyman wrote:
> > >> Stefan Sperling wrote:
> > >>> I'd prefer keeping --no-diff-properties and add a --patch option
> > >>> later which implies --no-diff-properties and maybe other options.
> > >>>
> > >> I was explicitly asked by Julian Foad to avoid adding more
> > >>
> > >> low-level options (such as --no-diff-properties) and add
> > >> "interface-level" options instead. I am fine with either approach.
> >
> > Actually, my contribution to this design discussion was only to say that
> > having a high-level option was "potentially a good direction to go
> > [...]": <http://svn.haxx.se/dev/archive-2011-11/0081.shtml>.
> >
> > > I'd prefer having both low-level and high-level options available.
> > >
> > > That way it is easier to tell what each individual option does.
> > > Higher-level options can simply explain themselves as being
> > > equivalent to some set of lower-level options.
> >
> > That sounds good to me.
> >
> > > But feel free to let us argue about it instead of getting involved
> > > in the bikeshedding. We can apply your patch as-is and change the
> > > option name later. It's no big deal. Before doing so I'll wait a bit
> > > to see what the others have to say.
> >
> > Agreed.
> >
> > - Julian
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Stefan Sperling <st...@elego.de>.
Alexey,
are you still working on this patch?
Since your last submission there has been some feedback and I think
we've reached consensus. I am sorry that our discussion flip-flopped
a little and mislead you.
I would very much like to see a version of your patch which
incorporates the feedback you've received since your last submission.
If you don't have time or interested to work on this patch anymore
I'd like to pick it up and finish it. Else I will wait for an updated
patch from you.
Thanks!
On Thu, Dec 22, 2011 at 11:18:18AM +0000, Julian Foad wrote:
> Stefan Sperling wrote:
>
> > Alexey Neyman wrote:
> >> Stefan Sperling wrote:
> >>> I'd prefer keeping --no-diff-properties and add a --patch option
> >>> later which implies --no-diff-properties and maybe other options.
> >>
> >> I was explicitly asked by Julian Foad to avoid adding more
> >> low-level options (such as --no-diff-properties) and add
> >> "interface-level" options instead. I am fine with either approach.
>
> Actually, my contribution to this design discussion was only to say that having a high-level option was "potentially a good direction to go [...]": <http://svn.haxx.se/dev/archive-2011-11/0081.shtml>.
>
> > I'd prefer having both low-level and high-level options available.
> >
> > That way it is easier to tell what each individual option does.
> > Higher-level options can simply explain themselves as being
> > equivalent to some set of lower-level options.
>
> That sounds good to me.
>
> > But feel free to let us argue about it instead of getting involved
> > in the bikeshedding. We can apply your patch as-is and change the
> > option name later. It's no big deal. Before doing so I'll wait a bit
> > to see what the others have to say.
>
> Agreed.
>
> - Julian
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Sperling wrote:
> Alexey Neyman wrote:
>> Stefan Sperling wrote:
>>> I'd prefer keeping --no-diff-properties and add a --patch option
>>> later which implies --no-diff-properties and maybe other options.
>>
>> I was explicitly asked by Julian Foad to avoid adding more
>> low-level options (such as --no-diff-properties) and add
>> "interface-level" options instead. I am fine with either approach.
Actually, my contribution to this design discussion was only to say that having a high-level option was "potentially a good direction to go [...]": <http://svn.haxx.se/dev/archive-2011-11/0081.shtml>.
> I'd prefer having both low-level and high-level options available.
>
> That way it is easier to tell what each individual option does.
> Higher-level options can simply explain themselves as being
> equivalent to some set of lower-level options.
That sounds good to me.
> But feel free to let us argue about it instead of getting involved
> in the bikeshedding. We can apply your patch as-is and change the
> option name later. It's no big deal. Before doing so I'll wait a bit
> to see what the others have to say.
Agreed.
- Julian
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Dec 21, 2011 at 11:32:43AM -0800, Alexey Neyman wrote:
> Hi Stefan,
>
> Thanks for a review. A few questions:
>
> On Wednesday, December 21, 2011 03:44:07 am Stefan Sperling wrote:
> > I'd prefer keeping --no-diff-properties and add a --patch option later
> > which implies --no-diff-properties and maybe other options.
>
> I was explicitly asked by Julian Foad to avoid adding more low-level options
> (such as --no-diff-properties) and add "interface-level" options instead. I am
> fine with either approach.
I'd prefer having both low-level and high-level options available.
That way it is easier to tell what each individual option does.
Higher-level options can simply explain themselves as being equivalent
to some set of lower-level options.
But feel free to let us argue about it instead of getting involved in
the bikeshedding. We can apply your patch as-is and change the option
name later. It's no big deal. Before doing so I'll wait a bit to see
what the others have to say.
> > I'm not sure I like the name --patch.
> > Maybe call it --patch-compatible or something?
>
> Again, the option name was suggested on the list by C.Michael Pilato. I guess
> it's to be in line with the existing --git (which is not --git-compatible).
There is no existing 'git' subcommand in svn. But there is a 'patch'
subcommand. Having an option with the same name as a subcommand can
cause confusion.
I notice your --patch option description mentions GNU patch.
GNU patch is not the only patch implementation in use.
E.g. there is Larry Wall's original BSD-licensed patch implementation
on which GNU patch is based. I would rather have the --patch-compat
description say "compatibility to third-party patch programs" or
something like that.
> > > @@ -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.
>
> I am a bit confused here. The comment for svn_client_diff6 says "New in 1.8",
> and 1.8 is not released yes, is it? I thought that non-released API is subject
> to change. If it is not the case, I'll create another wrapper.
Oh, you are correct. In that case it's fine. Sorry, I couldn't tell from
the context of the patch whether this was the case. I should've checked
the file...
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 12/21/2011 02:32 PM, Alexey Neyman wrote:
> I am a bit confused here. The comment for svn_client_diff6 says "New in 1.8",
> and 1.8 is not released yes, is it? I thought that non-released API is subject
> to change. If it is not the case, I'll create another wrapper.
You are correct. If the API has already been revved for the next release,
you needn't rev it again for the same release. Just modify it as you
require and then update the previous version's docs and implementation (as
they tend to be mere wrappers around the new version).
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Alexey Neyman <st...@att.net>.
Hi Stefan,
Thanks for a review. A few questions:
On Wednesday, December 21, 2011 03:44:07 am Stefan Sperling wrote:
> I'd prefer keeping --no-diff-properties and add a --patch option later
> which implies --no-diff-properties and maybe other options.
I was explicitly asked by Julian Foad to avoid adding more low-level options
(such as --no-diff-properties) and add "interface-level" options instead. I am
fine with either approach.
> I'm not sure I like the name --patch.
> Maybe call it --patch-compatible or something?
Again, the option name was suggested on the list by C.Michael Pilato. I guess
it's to be in line with the existing --git (which is not --git-compatible).
> > @@ -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.
I am a bit confused here. The comment for svn_client_diff6 says "New in 1.8",
and 1.8 is not released yes, is it? I thought that non-released API is subject
to change. If it is not the case, I'll create another wrapper.
Regards,
Alexey.
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Stefan Sperling <st...@elego.de>.
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;
Re: [PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Daniel Shahaf <da...@elego.de>.
Alexey Neyman wrote on Wed, Nov 16, 2011 at 16:09:51 -0800:
> 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.
As we say here: the answer's in the formulation of the question. You
could tell the external diff what it's diffing -- or, perhaps easier,
configure a separate [helpers]diff-cmd and [helpers]propdiff-cmd .
> 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 haven't looked at those yet.
[PATCH] Re: Patch ping, 5 years later: removing properties
Posted by Alexey Neyman <an...@lnxw.com>.
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'.
[[[
* 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.
]]]
Best regards,
Alexey.
>
> C. Michael Pilato wrote on Thu, Nov 03, 2011 at 15:16:23 -0400:
> > The introduction of the --git option to 'svn diff' opens the door for
> > different "flavors" for 'svn diff' output. Would you be interested in a
> > new 'svn diff --patch' flavor, which generates only diff output that's
> > suitable for consumption with GNU patch? Implied by --patch would be no
> > property diff output and the display of copied items as additions (like
> > --show-copies-as-adds does).
> >
> > Of course, --no-property-diff works for me, too. :-)
> >
> > On 11/03/2011 03:05 PM, Alexey Neyman wrote:
> > > Hi all,
> > >
> > > Sorry for a long delay :) 5 years ago, I sent a patch to the mailing
> > > list that
> > >
> > > would allow to specify what to include/exclude from the diff output:
> > > http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessag
> > > eId=742594
> > >
> > > There were two problems this patch tried to address:
> > >
> > > 1. There was no way to exclude properties from the diff output.
> > >
> > > 2. There was no way to obtain a list of modified/added/deleted files
> > > and whether the content and/or properties have been modified.
> > >
> > > I got a response from Ben Collins-Sussman that the patch I sent is
> > > superseded by --summarize option which was implemented in the trunk
> > > (which would
> > >
> > > eventually become 1.4):
> > > http://svn.haxx.se/dev/archive-2006-06/0202.shtml
> > >
> > > Actually, --summarize only addresses the issue #2, but not the issue
> > > #1. As to issue #1, ability to disable property diffs is still missing
> > > from Subversion.
> > >
> > > Common wisdom suggests running the diff through 'filterdiff --clean':
> > > http://stackoverflow.com/questions/2755848/how-do-you-get-subversion-
> > > diff-
> > >
> > > summary-to-ignore-mergeinfo-properties
> > >
> > > http://stackoverflow.com/questions/402522/is-there-a-metadata-exclusi
> > > on-
> > >
> > > filter-for-the-svn-diff-command
> > >
> > > But this is still unreliable: if property text includes a diff-like
> > > chunk, it will go through.
> > >
> > > What is the sentiment about implementing --no-property-diff option to
> > > 'svn diff'? I can implement it if there's agreement such option is
> > > needed.
> > >
> > > Regards,
> > > Alexey.
Re: Patch ping, 5 years later: removing properties
Posted by Daniel Shahaf <da...@elego.de>.
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.
C. Michael Pilato wrote on Thu, Nov 03, 2011 at 15:16:23 -0400:
> The introduction of the --git option to 'svn diff' opens the door for
> different "flavors" for 'svn diff' output. Would you be interested in a new
> 'svn diff --patch' flavor, which generates only diff output that's suitable
> for consumption with GNU patch? Implied by --patch would be no property
> diff output and the display of copied items as additions (like
> --show-copies-as-adds does).
>
> Of course, --no-property-diff works for me, too. :-)
>
>
> On 11/03/2011 03:05 PM, Alexey Neyman wrote:
> > Hi all,
> >
> > Sorry for a long delay :) 5 years ago, I sent a patch to the mailing list that
> > would allow to specify what to include/exclude from the diff output:
> >
> > http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=742594
> >
> > There were two problems this patch tried to address:
> >
> > 1. There was no way to exclude properties from the diff output.
> >
> > 2. There was no way to obtain a list of modified/added/deleted files and
> > whether the content and/or properties have been modified.
> >
> > I got a response from Ben Collins-Sussman that the patch I sent is superseded
> > by --summarize option which was implemented in the trunk (which would
> > eventually become 1.4):
> >
> > http://svn.haxx.se/dev/archive-2006-06/0202.shtml
> >
> > Actually, --summarize only addresses the issue #2, but not the issue #1. As to
> > issue #1, ability to disable property diffs is still missing from Subversion.
> > Common wisdom suggests running the diff through 'filterdiff --clean':
> >
> > http://stackoverflow.com/questions/2755848/how-do-you-get-subversion-diff-
> > summary-to-ignore-mergeinfo-properties
> > http://stackoverflow.com/questions/402522/is-there-a-metadata-exclusion-
> > filter-for-the-svn-diff-command
> >
> > But this is still unreliable: if property text includes a diff-like chunk, it
> > will go through.
> >
> > What is the sentiment about implementing --no-property-diff option to 'svn
> > diff'? I can implement it if there's agreement such option is needed.
> >
> > Regards,
> > Alexey.
>
>
> --
> C. Michael Pilato <cm...@collab.net>
> CollabNet <> www.collab.net <> Distributed Development On Demand
>
Re: Patch ping, 5 years later: removing properties
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Alexey.
In your original email <http://subversion.tigris.org/ds/viewMessage.do?dsForumId=1065&dsMessageId=741864>, you described the problem this way. You have a tree imported into SVN from CVS, and another copy of it in which you have added Subversion properties such as svn:executable to lots of files and made a few other changes. Now you want to see a diff between the two trees, ignoring the properties to make it easier to focus on the other changes.
I can see why you want to ignore the properties in this case, but I don't think Subversion needs to do this for you, as it's not a general need. I think "svn diff | filterdiff --clean" is the right solution to your problem.
You wrote:
> > But this [filterdiff] is still unreliable: if property
> > text includes a diff-like chunk, it will go through.
That was true in Subversion 1.6, but in 1.7 a multi-line property is diffed line-by-line just like a multi-line file, so no output line will ever start with "@@", so "patch" and "filterdiff" will not be confused. Even with 1.6, this wasn't a problem in practice was it?
C. Michael Pilato wrote:
> Would you be interested in a new 'svn diff --patch' flavor,
> which generates only diff output that's suitable
> for consumption with GNU patch? Implied by --patch
> would be no property diff output and the display of copied
> items as additions (like --show-copies-as-adds does).
That is potentially a good direction to go in the user interface, adding options focused on a specific high-level goal. With the current low-level options (--no-diff-deleted, --show-copies-as-adds, and --notice-ancestry), it can be rather hard for a user (myself included) to figure out what options need to be specified, if indeed it can be done at all, to give a particular result such as a fully two-way diff or a diff that's as compatible as possible with standard 'patch'.
- Julian
Alexey Neyman wrote:
> > What is the sentiment about implementing --no-property-diff
> > option to 'svn diff'? I can implement it if there's
> > agreement such option is needed.
Re: Patch ping, 5 years later: removing properties
Posted by "C. Michael Pilato" <cm...@collab.net>.
The introduction of the --git option to 'svn diff' opens the door for
different "flavors" for 'svn diff' output. Would you be interested in a new
'svn diff --patch' flavor, which generates only diff output that's suitable
for consumption with GNU patch? Implied by --patch would be no property
diff output and the display of copied items as additions (like
--show-copies-as-adds does).
Of course, --no-property-diff works for me, too. :-)
On 11/03/2011 03:05 PM, Alexey Neyman wrote:
> Hi all,
>
> Sorry for a long delay :) 5 years ago, I sent a patch to the mailing list that
> would allow to specify what to include/exclude from the diff output:
>
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=742594
>
> There were two problems this patch tried to address:
>
> 1. There was no way to exclude properties from the diff output.
>
> 2. There was no way to obtain a list of modified/added/deleted files and
> whether the content and/or properties have been modified.
>
> I got a response from Ben Collins-Sussman that the patch I sent is superseded
> by --summarize option which was implemented in the trunk (which would
> eventually become 1.4):
>
> http://svn.haxx.se/dev/archive-2006-06/0202.shtml
>
> Actually, --summarize only addresses the issue #2, but not the issue #1. As to
> issue #1, ability to disable property diffs is still missing from Subversion.
> Common wisdom suggests running the diff through 'filterdiff --clean':
>
> http://stackoverflow.com/questions/2755848/how-do-you-get-subversion-diff-
> summary-to-ignore-mergeinfo-properties
> http://stackoverflow.com/questions/402522/is-there-a-metadata-exclusion-
> filter-for-the-svn-diff-command
>
> But this is still unreliable: if property text includes a diff-like chunk, it
> will go through.
>
> What is the sentiment about implementing --no-property-diff option to 'svn
> diff'? I can implement it if there's agreement such option is needed.
>
> Regards,
> Alexey.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand