On Wed, Mar 10, 2010 at 10:50:13PM +0100, Stefan Küng wrote: > On 10.03.2010 22:43, Stefan Sperling wrote: > >On Wed, Mar 10, 2010 at 08:58:24PM +0100, Stefan Küng wrote: > >>On 10.03.2010 19:14, Julian Foad wrote: > >> > >>>>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 :-) > >> > >>It's basically a class with methods like: > >>* ParsePatchfile > >>* GetCount //returns the number of paths affected > >>* GetPath(int index) // returns the affected path > >>* PatchPath(int index, TCHAR * target) // applies the patch for the > >>path and saves the result in 'target' > >>The names of the class methods are actually a little bit different, > >>but that's pretty much what we have right now and what we need. > > > >What about if you just parse the patch using the diff parser > >we have in libsvn_diff? Would that be enough? > >See this file for details: > >http://svn.apache.org/repos/asf/subversion/trunk/subversion/include/private/svn_diff_private.h > >We could make those functions public if you want. > > Yes, those functions and structs would suffice to get all the target > paths of the patch file.
OK, so we have base 1 covered. > >Another thing: Why do you need to apply the patch to some tempfile? > >Why is the preview of the patched result really necessary? > >Why not just show the user the patch? When reviewing what a patch > >does, it's much easier to look at the patch itself rather than the > >patched result -- at least from my point of view, I don't know if your > >users are different. > > > >If you agree to these points, we could drop the svn_client_patch() API > >change again. > > Reviewing a patch file is not what most users expect and are > familiar with. Especially if there are many changes spread through a > big file, it's not enough to just see the patch file with the three > context lines to really get what the changes do - you have to see > the full file with all the changes. > > Applying the patch to a temp file allows me to show the changes in > TortoiseMerge, the old and new file side-by-side. That's what users > are used to and where they can really see the result. > > Also, if you just review the patch file itself, there's no guarantee > that when the patch is applied that that result is what you expect: > depending on the algorithm to find the position of where to apply a > hunk, such a hunk can get applied somewhere you didn't expect. So a > 'real' preview is necessary. This is really your second request -- you want to get at the tempfiles svn patch generates during patching. Have you seen my proposal in http://subversion.tigris.org/issues/show_bug.cgi?id=3598 ? Basically, we could have svn_client_patch() look like this: svn_error_t * svn_client_patch(const char *abs_patch_path, const char *local_abspath, svn_boolean_t dry_run, int strip_count, svn_boolean_t reverse, apr_hash_t **tempfiles, svn_client_ctx_t *ctx, apr_pool_t *result_pool, apr_pool_t *scratch_pool); If you pass non-NULL for tempfiles, and TRUE for dry-run, you'd get back in *tempfiles a mapping {path as in patch file => tempfile containing patched result}, the tempfiles are left open by svn_client_patch() so you need to close them, and the working copy is not modified. Would that suffice to cover base 2? What is not possible (without adding the --include-pattern option) is selecting which files to patch. Is selecting individual patch targets really that important? Stefan
