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
<http://subversion.tigris.org/issues/show_bug.cgi?id=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".
Yup.
(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
like.
> > 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.
Great.
> > > 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