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
Received on 2010-03-10 23:23:13 CET