You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/09/15 14:51:15 UTC
Re: svn commit: r997249 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/cl.h svn/main.c
svn/patch-cmd.c tests/cmdline/patch_tests.py
tests/libsvn_client/client-test.c
stsp@apache.org wrote on Wed, Sep 15, 2010 at 10:05:15 -0000:
> Author: stsp
> Date: Wed Sep 15 10:05:14 2010
> New Revision: 997249
>
> URL: http://svn.apache.org/viewvc?rev=997249&view=rev
> Log:
> Add an --old-patch-target-names option to svn patch.
> This option is useful in two cases:
>
> 1) A patch contains names like
> --- foo.c
> +++ foo.c.new
> and should be applied to foo.c.
>
> 2) A patch contains names like
> --- foo.c.orig
> +++ foo.c
> and should be applied in reverse to foo.c, e.g. to undo prior application
> of the patch.
What happens if a user forgets to supply the new option? (Does svn
complain that 'foo.c.new is nonexistent/unversioned'?)
For case (2): suppose I have such a patch (was emailed to me). I
applied it using 'svn patch $patchfile'. Now I want to unapply it; so
so I use 'svn patch --rd $patchfile' or 'svn patch --rd --optn $patchfile'?
AIUI, currently only the latter will work?
Is the UI "'svn patch' always uses the filename in the /^+++/ line"?
Re: svn commit: r997249 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/cl.h svn/main.c
svn/patch-cmd.c tests/cmdline/patch_tests.py
tests/libsvn_client/client-test.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 17, 2010 at 02:01:27AM +0200, Daniel Shahaf wrote:
> Devil's advocate: It's a very narrow-scoped option, and might become
> obsolete if we extend this part of the code a bit.
Yes, that's why I wasn't truely happy with it.
The plan to detect the proper filename dynamically using the algorithm
described in the man page makes this option obsolete.
> My point was that I think --rd shouldn't swap the filenames.
This also becomes of no concern with dynamic filename determination.
Stefan
Re: svn commit: r997249 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/cl.h svn/main.c
svn/patch-cmd.c tests/cmdline/patch_tests.py
tests/libsvn_client/client-test.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, Sep 15, 2010 at 20:42:55 +0200:
> On Wed, Sep 15, 2010 at 04:51:15PM +0200, Daniel Shahaf wrote:
> > stsp@apache.org wrote on Wed, Sep 15, 2010 at 10:05:15 -0000:
> > > Author: stsp
> > > Date: Wed Sep 15 10:05:14 2010
> > > New Revision: 997249
> > >
> > > URL: http://svn.apache.org/viewvc?rev=997249&view=rev
> > > Log:
> > > Add an --old-patch-target-names option to svn patch.
> > > This option is useful in two cases:
> > >
> > > 1) A patch contains names like
> > > --- foo.c
> > > +++ foo.c.new
> > > and should be applied to foo.c.
> > >
> > > 2) A patch contains names like
> > > --- foo.c.orig
> > > +++ foo.c
> > > and should be applied in reverse to foo.c, e.g. to undo prior application
> > > of the patch.
> >
> > What happens if a user forgets to supply the new option? (Does svn
> > complain that 'foo.c.new is nonexistent/unversioned'?)
>
> $ svn patch test.diff
> Skipped missing target: 'foo.new'
> Summary of conflicts:
> Skipped paths: 1
Okay. So it always uses the /^+++/ path. Pro: simple and predictable,
con: sometimes it's possible to guess the correct path via other means
(the Index: line, or extension stripping, etc; see the other thread).
>
> > For case (2): suppose I have such a patch (was emailed to me). I
> > applied it using 'svn patch $patchfile'. Now I want to unapply it; so
> > so I use 'svn patch --rd $patchfile' or 'svn patch --rd --optn $patchfile'?
> > AIUI, currently only the latter will work?
>
> Which one works depends on the way path names appear in the patch.
>
> Say you have a diff produced with svn diff.
> That will work with either combination of options.
>
> $ svn diff
> Index: alpha
> ===================================================================
> --- alpha (revision 2)
> +++ alpha (working copy)
> @@ -1 +1,2 @@
> alpha
> +aaa
>
> You need the new option only to handle cases where people used e.g. the
> UNIX diff tool to create a patch using left/right names they made up
> while producing the diff.
>
> > Is the UI "'svn patch' always uses the filename in the /^+++/ line"?
>
> Yes. By default, that's the name that is used.
>
> If you reverse the diff, the filenames get swapped as well.
> So if you use the --optn option, the filenames get swapped again, or not at
> all, depending which way you look at it :)
>
My point was that I think --rd shouldn't swap the filenames.
> This UI may not be ideal, and I'm open for suggestions on how we could
> improve it. Maybe there's a way to handle this without user interaction,
> or a better name for the new option?
>
> The UNIX patch tool prompts for a filename when it cannot find a target.
> We could do the same, defining yet another prompt callback type.
>
> But I don't like the fact that UNIX patch does not do tab-completion in its
> prompt. I think the lack of tab-completion really affects usability.
> So if svn had a prompt for this, I'd like it to support tab-completion.
>
Tab completion? Can we do that using APR or similar?
> So the new --optn option is an easier way to implement this.
> Maybe it's sufficient? Note that having the option around doesn't hurt
> even if we decide to implement prompting later.
Devil's advocate: It's a very narrow-scoped option, and might become
obsolete if we extend this part of the code a bit.
>
> Stefan
Re: svn commit: r997249 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/cl.h svn/main.c
svn/patch-cmd.c tests/cmdline/patch_tests.py
tests/libsvn_client/client-test.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 15, 2010 at 08:53:17PM +0200, Hyrum K. Wright wrote:
> How about just adding '--patch-left' and '--patch-right' or something.
> The either both have to used, or neither, and if they are, use those
> names as the once in the patch.
I don't like these names because it's not clear what left and right refer to.
Also, why would you need to use both these options together?
> If people can use the UNIX diff tool
> to create left/right names they just made up, let's just let them
> worry about feeding those names back into Subversion (rather than
> guessing).
One goal of svn patch is to facilitate application of arbitrary unidiffs.
The person generating the patch may not be the same person worrying
about applying the patch to a Subversion working copy.
In any case, people can always edit the patch file. But having an option
like this saves a bit of manual edits in the cases described.
Stefan
Re: svn commit: r997249 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/cl.h svn/main.c
svn/patch-cmd.c tests/cmdline/patch_tests.py tests/libsvn_client/client-test.c
Posted by "Hyrum K. Wright" <hy...@mail.utexas.edu>.
On Wed, Sep 15, 2010 at 8:42 PM, Stefan Sperling <st...@elego.de> wrote:
> On Wed, Sep 15, 2010 at 04:51:15PM +0200, Daniel Shahaf wrote:
>> stsp@apache.org wrote on Wed, Sep 15, 2010 at 10:05:15 -0000:
>> > Author: stsp
>> > Date: Wed Sep 15 10:05:14 2010
>> > New Revision: 997249
>> >
>> > URL: http://svn.apache.org/viewvc?rev=997249&view=rev
>> > Log:
>> > Add an --old-patch-target-names option to svn patch.
>> > This option is useful in two cases:
>> >
>> > 1) A patch contains names like
>> > --- foo.c
>> > +++ foo.c.new
>> > and should be applied to foo.c.
>> >
>> > 2) A patch contains names like
>> > --- foo.c.orig
>> > +++ foo.c
>> > and should be applied in reverse to foo.c, e.g. to undo prior application
>> > of the patch.
>>
>> What happens if a user forgets to supply the new option? (Does svn
>> complain that 'foo.c.new is nonexistent/unversioned'?)
>
> $ svn patch test.diff
> Skipped missing target: 'foo.new'
> Summary of conflicts:
> Skipped paths: 1
>
>> For case (2): suppose I have such a patch (was emailed to me). I
>> applied it using 'svn patch $patchfile'. Now I want to unapply it; so
>> so I use 'svn patch --rd $patchfile' or 'svn patch --rd --optn $patchfile'?
>> AIUI, currently only the latter will work?
>
> Which one works depends on the way path names appear in the patch.
>
> Say you have a diff produced with svn diff.
> That will work with either combination of options.
>
> $ svn diff
> Index: alpha
> ===================================================================
> --- alpha (revision 2)
> +++ alpha (working copy)
> @@ -1 +1,2 @@
> alpha
> +aaa
>
> You need the new option only to handle cases where people used e.g. the
> UNIX diff tool to create a patch using left/right names they made up
> while producing the diff.
>
>> Is the UI "'svn patch' always uses the filename in the /^+++/ line"?
>
> Yes. By default, that's the name that is used.
>
> If you reverse the diff, the filenames get swapped as well.
> So if you use the --optn option, the filenames get swapped again, or not at
> all, depending which way you look at it :)
>
> This UI may not be ideal, and I'm open for suggestions on how we could
> improve it. Maybe there's a way to handle this without user interaction,
> or a better name for the new option?
>
> The UNIX patch tool prompts for a filename when it cannot find a target.
> We could do the same, defining yet another prompt callback type.
>
> But I don't like the fact that UNIX patch does not do tab-completion in its
> prompt. I think the lack of tab-completion really affects usability.
> So if svn had a prompt for this, I'd like it to support tab-completion.
>
> So the new --optn option is an easier way to implement this.
> Maybe it's sufficient? Note that having the option around doesn't hurt
> even if we decide to implement prompting later.
How about just adding '--patch-left' and '--patch-right' or something.
The either both have to used, or neither, and if they are, use those
names as the once in the patch. If people can use the UNIX diff tool
to create left/right names they just made up, let's just let them
worry about feeding those names back into Subversion (rather than
guessing).
-Hyrum
Re: svn commit: r997249 - in /subversion/trunk/subversion:
include/svn_client.h libsvn_client/patch.c svn/cl.h svn/main.c
svn/patch-cmd.c tests/cmdline/patch_tests.py
tests/libsvn_client/client-test.c
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 15, 2010 at 04:51:15PM +0200, Daniel Shahaf wrote:
> stsp@apache.org wrote on Wed, Sep 15, 2010 at 10:05:15 -0000:
> > Author: stsp
> > Date: Wed Sep 15 10:05:14 2010
> > New Revision: 997249
> >
> > URL: http://svn.apache.org/viewvc?rev=997249&view=rev
> > Log:
> > Add an --old-patch-target-names option to svn patch.
> > This option is useful in two cases:
> >
> > 1) A patch contains names like
> > --- foo.c
> > +++ foo.c.new
> > and should be applied to foo.c.
> >
> > 2) A patch contains names like
> > --- foo.c.orig
> > +++ foo.c
> > and should be applied in reverse to foo.c, e.g. to undo prior application
> > of the patch.
>
> What happens if a user forgets to supply the new option? (Does svn
> complain that 'foo.c.new is nonexistent/unversioned'?)
$ svn patch test.diff
Skipped missing target: 'foo.new'
Summary of conflicts:
Skipped paths: 1
> For case (2): suppose I have such a patch (was emailed to me). I
> applied it using 'svn patch $patchfile'. Now I want to unapply it; so
> so I use 'svn patch --rd $patchfile' or 'svn patch --rd --optn $patchfile'?
> AIUI, currently only the latter will work?
Which one works depends on the way path names appear in the patch.
Say you have a diff produced with svn diff.
That will work with either combination of options.
$ svn diff
Index: alpha
===================================================================
--- alpha (revision 2)
+++ alpha (working copy)
@@ -1 +1,2 @@
alpha
+aaa
You need the new option only to handle cases where people used e.g. the
UNIX diff tool to create a patch using left/right names they made up
while producing the diff.
> Is the UI "'svn patch' always uses the filename in the /^+++/ line"?
Yes. By default, that's the name that is used.
If you reverse the diff, the filenames get swapped as well.
So if you use the --optn option, the filenames get swapped again, or not at
all, depending which way you look at it :)
This UI may not be ideal, and I'm open for suggestions on how we could
improve it. Maybe there's a way to handle this without user interaction,
or a better name for the new option?
The UNIX patch tool prompts for a filename when it cannot find a target.
We could do the same, defining yet another prompt callback type.
But I don't like the fact that UNIX patch does not do tab-completion in its
prompt. I think the lack of tab-completion really affects usability.
So if svn had a prompt for this, I'd like it to support tab-completion.
So the new --optn option is an easier way to implement this.
Maybe it's sufficient? Note that having the option around doesn't hurt
even if we decide to implement prompting later.
Stefan