On 10.03.2010 17:23, Julian Foad wrote:
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?)
Yes, I would also need to know which paths the patch wants to modify,
and then later I want to tell the API which of those paths it should
actually modify.
This is to let the user choose which paths to modify and which ones not.
And it would be great if I could tell where to save the result, i.e. not
having the patch applied to the target file but to a copy of that file
instead so I can show the user a preview of the result without actually
modifying the real file yet.
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?
Yes: there are a lot of svn clients out there which could benefit from this.
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.
As one of the TSVN devs I can assure you that we didn't just make this
requirement up: TSVN currently does exactly this, but of course with its
own implementation of 'patch' which isn't that good and only works if
the patch applies cleanly.
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.
Just because GNU patch doesn't have a feature doesn't mean the feature
isn't useful.
And there's a reason why GNU patch is rarely used on Windows: it's
unusable for most situations.
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.
Well, all I can say is that without this feature, I can't use the patch
command from the svn library in TortoiseMerge. I would have to keep the
sub-par patch TMerge already has instead, because it has that feature
that is required for previews. And previews are important. Users are
very uncomfortable to apply changes without first knowing what those
changes are and where they go to (i.e., which files are affected).
Sure, if you do an update you also won't know beforehand what changes
are applied to your working copy (unless you run 'svn st -u').
But there you know the people who have commit access. In case of patch
files you don't really know the people who sent you a patch for
something. So reviewing the patch before you apply it to your working
copy (which most likely has local modifications, so reverting isn't
really an option either).
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
