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.
>
> 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".
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.
> 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?
> 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.
> > 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?
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.
Stefan
Received on 2010-03-10 15:39:50 CET