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
