Stefan Küng wrote:
> 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?)
Stefan, thanks for your feedback. This is really useful information.
> 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.
OK, well the issue #3434 didn't mention that. Since you already have a
patch implementation which does what you want, could you show us the API
definition (at least) for this part of it? That would help a lot, since
then we wouldn't have to guess about what you want :-)
> 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.
Again, please suggest an API for this. I know we could design it
ourselves, but it'll be great if you can save us some time!
> >>> 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.
In this sentence I'm asking about the "svn" CLI specifically. It doesn't
offer a preview checklist.
> >> 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.
Correct, but it does indicate that it's quite likely not. (Again, I'm
talking CLI only.)
> 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).
Thanks.
- Julian
Received on 2010-03-10 19:14:44 CET