Re: thoughts about svnpatch

Stefan Sperling wrote:
> On Mon, Jun 22, 2009 at 09:09:15PM +0200, Stefan Küng wrote:
>> Hi,
>>
>> I'm currently playing around with the new svnpatch feature. I haven't
>> got it working quite yet though, but I'd like to propose some
>> changes/enhancements:
>
> Great!
>
>> * applying an svnpatch file only works if the patch file is applied to a
>> working copy. Would it be possible to make this work for unversioned
>> source trees too?
>
> Should be possible to do. Please open an issue.

Done:
http://subversion.tigris.org/issues/show_bug.cgi?id=3433

>> * I'm missing a dry-run function to preview what applying the patch
>> would do - there's a 'dry-run' variable in the patch code, so I guess
>> this is already planned?
>
> I hadn't planned this yet.
> What exactly would you like this feature to do?
> Should it just send notifications?

Just sending notifications would be ok for me. I can then parse those.
What I would need in those notifications is:
* the path which is affected/changed
* the change type for that path (e.g., 'modified', 'added', 'deleted', ...)

I'd like to show the user a dialog/window with the paths that will get
modified when the patch is applied. This would act as a short preview.
And also as a selection dialog: the user should be able to choose which
parts of the patch file (s)he wants to apply and which parts not.

> Note that the 'svn patch' code consists of two parts:
>
> 1. The part which concerns itself with svnpatch blocks. These blocks
> basically contain compressed svn:// protocol information encoded
> in ASCII text. This was written by Charles Acknin.
> 2. The part which concerns itself with applying unidiff files.
> This part was mostly written by myself, and replaced the
> call to an external patch program which was originally
> part of part 1.
>
> There's a comment in libsvn_client/patch.c which separates the
> two parts:
>
> Part 1
> /* --- Text-diff application routines --- */
> Part 2
>
> Since I didn't know about the dry-run variable, I guess it belongs
> to part 1. I don't have much of an idea about part 1, expect that
> it looks to me to add far too much new code for what it's doing
> and I think we should try to find a way to trim down the size of
> part 1 before releasing it. We can release both parts independently
> of one another.
>
> In case you're interested (and would like to help :) this is what
> still remains to be done for part 2 before we can release it:
>
> - detect eol-style of the patch file and the target file,
> and use the appropriate eol indicator when reading from the patch
> file and writing to the target -- we want patches generated on windows
> to work on *nix and vice-versa.
> - line offset searching so that we can avoid merge conflicts
> if the target file contains the original hunk text at a
> different line than the original file.
> - a strip-count option (-p) for stripping leading directory
> components from target paths specified in the patch file

This might even be possible to do automatically: the svn_client_patch()
function knows the target path, and it knows the paths inside the
svnpatch file. Stripping path parts from the paths in the svnpatch file
until it 'matches' should be not very hard to implement?
TortoiseMerge currently does that before giving up if it can't find a match.

> - a --encoding option for specifying the patch file's encoding in case
> it is not encoded in the charset of the currently active locale
> - a few other TODOs in libsvn_client/patch.c, mostly handling error
> conditions
>
> I haven't opened issues for any of these, maybe I should...
>
>> * I would also like to have some kind of filter for svn_client_patch so
>> only items which match the filter will actually get patched. That way I
>> can present the user with a list of affected items (with the dry-run
>> feature) and then let the user choose which of the items (s)he actually
>> wants to patch.
>
> Good idea! Please open an issue for this, too.

http://subversion.tigris.org/issues/show_bug.cgi?id=3434

And one other thought:
it would be great if I could specify a temp file where svn_client_patch
would store the result of the patching operation. But that would of
course only work if the 'filter' specifies a single item to be patched.

I could use this to show the user a preview of the patched file (in the
UI diff tool, e.g., TortoiseMerge).
If the patch always writes to the real target file, that file would get
modified without the user knowing beforehand *what* exactly gets changed
and without a chance to undo that patching. With the temp file, undoing
is possible as long as the user doesn't 'save' that preview to the
original file.

That's how TortoiseMerge currently handles unified diff files with its
'apply patch' feature. And I'd like to keep that UI if possible - I
think it's a good UI.

>
>> * does the svn_client_patch() only work with svnpatch files or also with
>> unified diff files? From browsing through the code I think it should?
>
> It can and always has worked on any unidiff files. svnpatch blocks
> are ignored as noise by standard patch tools.

Thanks for clarifying this.

Stefan

-- 
       ___
  oo  // \\      "De Chelonian Mobile"
 (_,\/ \_/ \     TortoiseSVN
   \ \_/_\_/>    The coolest Interface to (Sub)Version Control
   /_/   \_\     http://tortoisesvn.net
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2364271