[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r919460 - filtering svn patch targets

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Wed, 10 Mar 2010 16:23:50 +0000

Hi, Stefan and Stefan.

Stefan Sperling wrote:
> On Wed, Mar 10, 2010 at 01:19:37PM +0000, Julian Foad wrote:
> > Stefan Sperling wrote:
> > > On Wed, Mar 10, 2010 at 10:36:13AM +0000, Julian Foad wrote:
> > > > This patch appears to have the filter reversed: it lets the user specify
> > > > paths that the filter should REMOVE, which IMO is not so useful.
> > >
> > > Why isn't that useful? I think either way (include and exclude) is useful.

(Re. issue #3434
> > I understand that TortoiseSVN wants the include/exclude functionality
> > available at the API. I wonder if "supply a list of patterns to
> > include" is really the best API. If I were writing a GUI that wanted to
> > show a list of what the patch is about to patch, I would want an API
> > that told me, first of all, what are the paths that this patch file is
> > going to patch. Maybe a callback that says: "I'm about to patch the
> > target file TARGET_ABSPATH. If you want, you can set TARGET_ABSPATH to a
> > different target, or set it to NULL if you don't want to apply this
> > file-patch at all." Using that, I could either get a list of all
> > targets in the patch (and make it a dry-run by returning TARGET_ABSPATH
> > = NULL on each callback), or control which targets are applied, by any
> > pattern- or list-matching scheme I like, or apply to different paths, or
> > just apply the patch normally by not providing a callback function at
> > all, all with the same simple API.
> The requirements as given by the TortoiseSVN developers were "I want
> to pass a list of paths the patch is going to modify, the rest of
> the paths should be ignored by the patching process".


(Stefan K, is that really what you want? You don't need to know what
paths are in the patch, for example?)

> Because of the convoluted wording of the issue (they used the word
> "filter") I implemented filtering functionality rather than the
> include list, but adding the include list is trivial.
> I extended their requierements so that the list also supports globbing,
> such that it's easier to use from the svn CLI client. Note that the globbing
> is of course optional so passing literal paths (like the TortoiseSVN
> devs want to do) works fine.

If this application were a patch program, then such extensions would be
right on track. Now, it's definitely useful for a version control
program to be able to apply patches, and will be especially useful one
day when it can apply tree-change and property-change patches. But
we're having such a hard job just maintaining the version-control parts
of Subversion that I get uncomfortable when we start to extend non-core
features beyond the essentials.

To be frank, I am heartily looking forward to the day when we can have
your expertise focused back on tree conflicts, WC-NG, merging or such

> > Is there a real practical need to have the include/exclude functionality
> > in "svn" as well? I can't think of a use that's important enough to
> > justify it. Can you?
> First and foremost, it allows us to test this feature independently
> of TortoiseSVN. That alone of course does not justify exposing the
> feature at the CLI level. However, since TortoiseSVN users obviously
> see the need for this feature (assuming the TortoiseSVN devs didn't
> just make it up and nobody is actually using this feature of TortoiseSVN),
> why should we not provide it in the CLI client as well?

The usual reasons. Because lots of GUI features are inappropriate in a
CLI, and this might well be one such. Because well designed software
doesn't provide features just because somebody wanted such features.

> > GNU patch doesn't have it (and it doesn't have an
> > "include-only" filter either).
> Good point. But following that rationale I'd rather drop the feature
> entirely, even at the API level.

I wouldn't discourage dropping it if Stefan K would be happy with that.

> > > My plan is to add an --include-pattern option (at the CLI level as well
> > > as the API level), and if both --include-pattern and --exclude-pattern
> > > options have been specified, have the --exclude-pattern option operate on
> > > any files left after one or more --include-pattern options were used to
> > > limit the patch target list to a given list of files.
> >
> > I just don't want us to make it more flexible than it needs to be.
> >
> > Have you thought about consistency with other svn commands? How does
> > this relate to "--targets="?
> That's irrelevant right now. 'svn patch' does not support --targets
> and no subcommand but svn patch supports --exclude-patterns. But there
> is no reason why --targets and --exclude-patterns could not co-exist
> within the same subcommand. The exclude patterns would simply act as
> a filter of the lines contained in the file passed to --targets.
> > To the "ignore patterns"?
> > Are the glob patterns using the same syntax as auto-props?
> The --exclude-patterns, auto-props and svn:ignore all use apr_fnmatch()
> to do pattern matching. They work the same.


> > > I'm also planning on renaming the 'glob_filter' parameter of the API
> > > to 'exclude_patterns' so it matches the terminology used by the CLI
> > > and does not say 'filter' anymore.
> >
> > That sounds good.
> Well, for me the question boils down to:
> Do we want the --exclude-patterns and --include-patterns options,
> and if not, what do we tell the TortoiseSVN (and AnkhSVN) devs about
> their requirement that svn patch should be able to patch targets
> selectively?

All I know about what they want is what Stefan K wrote in issue #3434:
"svn_client_patch() should have a 'filter' parameter, specifiying one
(or more?) paths that will get patched." Nothing more, nothing less.

We seem to be on a roll of adding new command-line options, which are
undoubtedly useful but I feel it is contrary to one of Subversion's
original design principles (small command set), and that's making me
slightly edgy.

> We could also implement your proposal, but it seems that TortoiseSVN
> and AnkhSVN devs are happy with where we're currently heading, and
> they are the ones who brought up this requirement originally.
> I certainly would not have added this feature if they hadn't requested it.
> If you want to convince them to adapt their requirements to some
> callback-driven way of doing this, that's fine by me. But I'm happy
> with --exclude-patterns and --include-patterns, so I'm not gonna
> push for a different way of doing this.

Fair point. I suggested it as a request for comments, not as a request
for re-doing what you've done.

- Julian
Received on 2010-03-10 17:24:28 CET

This is an archived mail posted to the Subversion Dev mailing list.