svn patch pitfalls

[Charles Acknin <ch...@gmail.com>]

I seem to meet lots of unexpected traps with 'svn patch' these days,
especially with copy and move operations since this is what I've been
working on more or less recently.  I have two of them in mind:

* dry-run patching with copy and move operations

This one is rather because of the way we use GNU patch.  Basically,
'svn patch' calls the external tool on any given output.  That is,
even when dealing with copy and move operations, which the tool
doesn't understand/speak.  Consider the following unidiff patch
created after iota2 was copied from iota (svnpatch bytes aren't
displayed):

[[[
Index: iota2
===================================================================
--- iota2       (revision 1)
+++ iota2       (working copy)
@@ -1 +1,2 @@
 This is the file 'iota'.
+bla
]]]

Now run 'svn patch --dry-run patchfile' (or even 'patch -p0 --dry-run
< patchfile'), and GNU patch bails out it can't find such a file.
Why?  Because it's dry-run, and thus 'svn patch' didn't copy the file
it would have copied if not in dry-run mode.  Let's remind that 'svn
patch' *first* operates on serialized editor commands and *second*
sends unidiff to an external program, precisely to avoid this kind of
problem (when not in dry-run).  But with dry-run, we have a problem,
iota2 is missing when the external tool is invoked and expects to see
it.  I would hate to have to execute editor commands as if it wasn't
dry-run, let the external tool run in dry-run, and then revert, to
solve this problem.

(Note this isn't a problem when dealing with new files, i.e. add-file
operation, since GNU patch knows (because of lack of context I guess)
the file is new and doesn't expect it to be on disk.)

Thoughts?

* Should svn diff --svnpatch imply --no-diff-deleted as well?

When a file is schedule-delete, either because of a delete or a move,
the unidiff appears with deletion lines of the file (diffs against
/dev/null).  Nothing new.  The point is, when applying the patch,
delete-entry will first remove the file from disk, then the external
tool will try to apply unidiff and won't find the file and prompt the
user.  So should we turn on --no-diff-deleted when --svnpatch is?

Help welcome :-)

Charles

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn patch pitfalls

[Charles Acknin <ch...@gmail.com>]

On 10/23/07, Daniel Rall <dl...@collab.net> wrote:
> On Mon, 22 Oct 2007, David Glasser wrote:
> > Writing to the wc during --dry-run seems like a bad idea.  I'd rather
> > drop dry-run functionality than need to do this.  (In fact in the
> > interest of actually finishing this increasingly hairy feature, maybe
> > you should punt --dry-run until later?)
>
> That sounds okay to me, too (I had actually been thinking the same thing).

Ok let's put dry-run back.  We can still leave it ON for the
serialized commands though.  I'm just going to disable the call to the
external tool when in dry-run mode.  How does that sound?  (I have to
admit, a bit like a half-baked feature, but this saves some code
refactoring and API changes.)

Charles

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn patch pitfalls

[Daniel Rall <dl...@collab.net>]

On Mon, 22 Oct 2007, David Glasser wrote:

> On 10/22/07, Charles Acknin <ch...@gmail.com> wrote:
> > On 10/22/07, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> > > > Ugh :-(.  That's a tough one.  The simplest answer for now might be to
> > > > notice whether there were copies/moves during the dry run, trap the
> > > > error from the external patch program, see if the error applies to any
> > > > of the putatively copied/moved items, and transform it into an
> > > > "application deferred" warning instead.  I realize that's lame, but
> > > > it's the best I can come up with right now.
> > >
> > > This is a similar approach to what we use for dry-run merges.  It's lame,
> > > but better than nothing.
> >
> > That sounds very tricky.  For example, GNU patch prompts the user for
> > a direction to take when such thing happen.  Does that mean we're also
> > going to wrap the program's input stream and answer on behalf of the
> > user when the prompting questions match a particular pattern?  That
> > sounds like a lot of code specific to one external program and again
> > very hairy.
> >
> > How about temporarily spawning the files the external tool needs for
> > the time it runs?  That could involve spawning a whole tree.
> 
> Writing to the wc during --dry-run seems like a bad idea.  I'd rather
> drop dry-run functionality than need to do this.  (In fact in the
> interest of actually finishing this increasingly hairy feature, maybe
> you should punt --dry-run until later?)

That sounds okay to me, too (I had actually been thinking the same thing).

Re: svn patch pitfalls

[David Glasser <gl...@davidglasser.net>]

On 10/22/07, Charles Acknin <ch...@gmail.com> wrote:
> On 10/22/07, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> > > Ugh :-(.  That's a tough one.  The simplest answer for now might be to
> > > notice whether there were copies/moves during the dry run, trap the
> > > error from the external patch program, see if the error applies to any
> > > of the putatively copied/moved items, and transform it into an
> > > "application deferred" warning instead.  I realize that's lame, but
> > > it's the best I can come up with right now.
> >
> > This is a similar approach to what we use for dry-run merges.  It's lame,
> > but better than nothing.
>
> That sounds very tricky.  For example, GNU patch prompts the user for
> a direction to take when such thing happen.  Does that mean we're also
> going to wrap the program's input stream and answer on behalf of the
> user when the prompting questions match a particular pattern?  That
> sounds like a lot of code specific to one external program and again
> very hairy.
>
> How about temporarily spawning the files the external tool needs for
> the time it runs?  That could involve spawning a whole tree.

Writing to the wc during --dry-run seems like a bad idea.  I'd rather
drop dry-run functionality than need to do this.  (In fact in the
interest of actually finishing this increasingly hairy feature, maybe
you should punt --dry-run until later?)

--dave

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn patch pitfalls

[Charles Acknin <ch...@gmail.com>]

On 10/22/07, Daniel L. Rall <dl...@finemaltcoding.com> wrote:
> > Ugh :-(.  That's a tough one.  The simplest answer for now might be to
> > notice whether there were copies/moves during the dry run, trap the
> > error from the external patch program, see if the error applies to any
> > of the putatively copied/moved items, and transform it into an
> > "application deferred" warning instead.  I realize that's lame, but
> > it's the best I can come up with right now.
>
> This is a similar approach to what we use for dry-run merges.  It's lame,
> but better than nothing.

That sounds very tricky.  For example, GNU patch prompts the user for
a direction to take when such thing happen.  Does that mean we're also
going to wrap the program's input stream and answer on behalf of the
user when the prompting questions match a particular pattern?  That
sounds like a lot of code specific to one external program and again
very hairy.

How about temporarily spawning the files the external tool needs for
the time it runs?  That could involve spawning a whole tree.

Charles

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn patch pitfalls

["Daniel L. Rall" <dl...@finemaltcoding.com>]

On Sun, 21 Oct 2007, Karl Fogel wrote:

> "Charles Acknin" <ch...@gmail.com> writes:
> > I seem to meet lots of unexpected traps with 'svn patch' these days,
> > especially with copy and move operations since this is what I've been
> > working on more or less recently.  I have two of them in mind:
> >
> > * dry-run patching with copy and move operations
> >
> > This one is rather because of the way we use GNU patch.  Basically,
> > 'svn patch' calls the external tool on any given output.  That is,
> > even when dealing with copy and move operations, which the tool
> > doesn't understand/speak.  Consider the following unidiff patch
> > created after iota2 was copied from iota (svnpatch bytes aren't
> > displayed):
> >
> > [[[
> > Index: iota2
> > ===================================================================
> > --- iota2       (revision 1)
> > +++ iota2       (working copy)
> > @@ -1 +1,2 @@
> >  This is the file 'iota'.
> > +bla
> > ]]]
> >
> > Now run 'svn patch --dry-run patchfile' (or even 'patch -p0 --dry-run
> > < patchfile'), and GNU patch bails out it can't find such a file.
> > Why?  Because it's dry-run, and thus 'svn patch' didn't copy the file
> > it would have copied if not in dry-run mode.  Let's remind that 'svn
> > patch' *first* operates on serialized editor commands and *second*
> > sends unidiff to an external program, precisely to avoid this kind of
> > problem (when not in dry-run).  But with dry-run, we have a problem,
> > iota2 is missing when the external tool is invoked and expects to see
> > it.  I would hate to have to execute editor commands as if it wasn't
> > dry-run, let the external tool run in dry-run, and then revert, to
> > solve this problem.
> >
> > (Note this isn't a problem when dealing with new files, i.e. add-file
> > operation, since GNU patch knows (because of lack of context I guess)
> > the file is new and doesn't expect it to be on disk.)
> >
> > Thoughts?
> 
> Ugh :-(.  That's a tough one.  The simplest answer for now might be to
> notice whether there were copies/moves during the dry run, trap the
> error from the external patch program, see if the error applies to any
> of the putatively copied/moved items, and transform it into an
> "application deferred" warning instead.  I realize that's lame, but
> it's the best I can come up with right now.

This is a similar approach to what we use for dry-run merges.  It's lame,
but better than nothing.
-- 

Daniel Rall

Re: svn patch pitfalls

[Karl Fogel <kf...@red-bean.com>]

"Charles Acknin" <ch...@gmail.com> writes:
> I seem to meet lots of unexpected traps with 'svn patch' these days,
> especially with copy and move operations since this is what I've been
> working on more or less recently.  I have two of them in mind:
>
> * dry-run patching with copy and move operations
>
> This one is rather because of the way we use GNU patch.  Basically,
> 'svn patch' calls the external tool on any given output.  That is,
> even when dealing with copy and move operations, which the tool
> doesn't understand/speak.  Consider the following unidiff patch
> created after iota2 was copied from iota (svnpatch bytes aren't
> displayed):
>
> [[[
> Index: iota2
> ===================================================================
> --- iota2       (revision 1)
> +++ iota2       (working copy)
> @@ -1 +1,2 @@
>  This is the file 'iota'.
> +bla
> ]]]
>
> Now run 'svn patch --dry-run patchfile' (or even 'patch -p0 --dry-run
> < patchfile'), and GNU patch bails out it can't find such a file.
> Why?  Because it's dry-run, and thus 'svn patch' didn't copy the file
> it would have copied if not in dry-run mode.  Let's remind that 'svn
> patch' *first* operates on serialized editor commands and *second*
> sends unidiff to an external program, precisely to avoid this kind of
> problem (when not in dry-run).  But with dry-run, we have a problem,
> iota2 is missing when the external tool is invoked and expects to see
> it.  I would hate to have to execute editor commands as if it wasn't
> dry-run, let the external tool run in dry-run, and then revert, to
> solve this problem.
>
> (Note this isn't a problem when dealing with new files, i.e. add-file
> operation, since GNU patch knows (because of lack of context I guess)
> the file is new and doesn't expect it to be on disk.)
>
> Thoughts?

Ugh :-(.  That's a tough one.  The simplest answer for now might be to
notice whether there were copies/moves during the dry run, trap the
error from the external patch program, see if the error applies to any
of the putatively copied/moved items, and transform it into an
"application deferred" warning instead.  I realize that's lame, but
it's the best I can come up with right now.

-K

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org