You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@wandisco.com> on 2010/09/13 16:50:05 UTC
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
On Mon, 2010-09-13, stsp@apache.org wrote:
> * subversion/libsvn_diff/parse-diff.c
> (svn_diff_parse_next_patch): When generating a reverse diff, do not swap
> the old and new filenames of the patch. The old filename in a unidiff
> is often useless (e.g. it contains a ".orig" extension), but the new
> filename should always be valid.
Sounds odd. I don't doubt this change is correct, but I'm sure the
reasoning for it shouldn't have anything to do with what kind of
filenames you think people might use :-)
- Julian
> * subversion/tests/cmdline/patch_tests.py
> (patch_reverse): Swap filenames in test patch, so it is more realistic.
>
> Modified:
> subversion/trunk/subversion/libsvn_diff/parse-diff.c
> subversion/trunk/subversion/tests/cmdline/patch_tests.py
>
> Modified: subversion/trunk/subversion/libsvn_diff/parse-diff.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_diff/parse-diff.c?rev=996581&r1=996580&r2=996581&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_diff/parse-diff.c (original)
> +++ subversion/trunk/subversion/libsvn_diff/parse-diff.c Mon Sep 13 15:45:24 2010
> @@ -1242,13 +1242,6 @@ svn_diff_parse_next_patch(svn_patch_t **
> } while (! eof);
>
> (*patch)->reverse = reverse;
> - if (reverse)
> - {
> - const char *temp;
> - temp = (*patch)->old_filename;
> - (*patch)->old_filename = (*patch)->new_filename;
> - (*patch)->new_filename = temp;
> - }
>
> if ((*patch)->old_filename == NULL || (*patch)->new_filename == NULL)
> {
>
> Modified: subversion/trunk/subversion/tests/cmdline/patch_tests.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/patch_tests.py?rev=996581&r1=996580&r2=996581&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/patch_tests.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/patch_tests.py Mon Sep 13 15:45:24 2010
> @@ -1476,8 +1476,8 @@ def patch_reverse(sbox):
> "@@ -1 +0,0 @@\n",
> "-new\n",
> "\n",
> - "--- A/mu 2009-06-24 15:23:55.000000000 +0100\n",
> - "+++ A/mu.orig 2009-06-24 15:21:23.000000000 +0100\n",
> + "--- A/mu.orig\t2009-06-24 15:23:55.000000000 +0100\n",
> + "+++ A/mu\t2009-06-24 15:21:23.000000000 +0100\n",
> "@@ -6,9 +6,6 @@\n",
> " through a computer ballot system drawn from over 100,000 company\n",
> " and 50,000,000 individual email addresses from all over the world.\n",
>
>
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Julian Foad <ju...@wandisco.com>.
Stefan Sperling wrote:
> Looking at the patch(1) man page, it sounds like we don't really need
> to reinvent the wheel here:
Phew! - wheels are complex, and the ones we make too often seem to have
nails sticking out of them.
> patch will choose the file name by performing the following steps, with
> the first match used:
>
> 1. If patch is operating in strict IEEE Std 1003.1-2008 (``POSIX'')
> mode, the first of the ``old'', ``new'' and ``index'' file names
> that exist is used. Otherwise, patch will examine either the
> ``old'' and ``new'' file names or, for a non-context diff, the
> ``index'' file name, and choose the file name with the fewest path
> components, the shortest basename, and the shortest total file name
> length (in that order).
>
> 2. If no file exists, patch checks for the existence of the files in an
> SCCS or RCS directory (using the appropriate prefix or suffix) using
> the criteria specified above. If found, patch will attempt to get
> or check out the file.
>
> 3. If no suitable file was found to patch, the patch file is a context
> or unified diff, and the old file was zero length, the new file name
> is created and used.
>
> 4. If the file name still cannot be determined, patch will prompt the
> user for the file name to use.
>
> I suppose we could implement the method described in step 1.
> That would make us compatible to UNIX patch, and should resolve the two
> use cases I have in mind.
>
> Step 2 doesn't apply to us. Step 3 is used already. Step 4 could be
> implemented as well (but I'd prefer to do that post-1.7).
>
> Thoughts?
+1 to step 1.
- Julian
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Sat, Sep 18, 2010 at 09:21:31AM +0200, Alan Barrett wrote:
> On Sat, 18 Sep 2010, Daniel Shahaf wrote:
> > "Fewest path components, shortest basename, total filename length".
> >
> > It's predictable, but it seems a bit arbitrary?
>
> It does seem a bit arbitrary, but I was able to rationalise it as
> follows: "fewest path components" is good for choosing between
> "subdir/file" and "../../other-branch/dir/subdir/file"; "shortest
> basename" is good for choosing between "file.old" and "file", or
> between "file" and "file.new"; "total filename length" is good for
> choosing between "subdir/file" and "subdir.old/file". You still need a
> tiebreaker of last resort after all that.
If there's a tie, we can use the "new" name.
I suppose what UNIX patch does has been working well for everybody
for a heck of a long time. There's no point in trying to improve on
it unless we encounter real shortcomings in practice.
Stefan
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Alan Barrett <ap...@cequrux.com>.
On Sat, 18 Sep 2010, Daniel Shahaf wrote:
> "Fewest path components, shortest basename, total filename length".
>
> It's predictable, but it seems a bit arbitrary?
It does seem a bit arbitrary, but I was able to rationalise it as
follows: "fewest path components" is good for choosing between
"subdir/file" and "../../other-branch/dir/subdir/file"; "shortest
basename" is good for choosing between "file.old" and "file", or
between "file" and "file.new"; "total filename length" is good for
choosing between "subdir/file" and "subdir.old/file". You still need a
tiebreaker of last resort after all that.
--apb (Alan Barrett)
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Fri, Sep 17, 2010 at 11:53:16 +0200:
> On Fri, Sep 17, 2010 at 01:42:09AM +0200, Daniel Shahaf wrote:
> > Which of the two methods described in step 1 do you have in mind?
>
> Non-POSIX, for context diffs.
>
> Rephrased to apply to svn patch:
>
> svn patch will examine the ``old'' and ``new'' file names,
> and choose the file name with the fewest path components, the shortest
> basename, and the shortest total file name length (in that order).
>
> > Whatever we do, the algorithm should be predictable, i.e., a user should
> > be able to know (from reading a patch) what path it would be applied to.
>
> The above is predictable.
>
"Fewest path components, shortest basename, total filename length".
It's predictable, but it seems a bit arbitrary?
Daniel
> Stefan
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Sep 17, 2010 at 01:42:09AM +0200, Daniel Shahaf wrote:
> Which of the two methods described in step 1 do you have in mind?
Non-POSIX, for context diffs.
Rephrased to apply to svn patch:
svn patch will examine the ``old'' and ``new'' file names,
and choose the file name with the fewest path components, the shortest
basename, and the shortest total file name length (in that order).
> Whatever we do, the algorithm should be predictable, i.e., a user should
> be able to know (from reading a patch) what path it would be applied to.
The above is predictable.
Stefan
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Wed, Sep 15, 2010 at 21:40:16 +0200:
> On Wed, Sep 15, 2010 at 06:15:58PM +0200, Daniel Shahaf wrote:
> > Stefan Sperling wrote on Tue, Sep 14, 2010 at 21:18:26 +0200:
> > > On Tue, Sep 14, 2010 at 12:49:06PM +0100, Julian Foad wrote:
> > > > Right. The issue seems to be "How do I determine what path I should
> > > > apply the patch to?" If the patch style is
> > > >
> > > > --- wc/file
> > > > +++ wc/file
> > > >
> > > > or
> > > >
> > > > --- old_wc/file
> > > > +++ new_wc/file
> > > >
> > > > then you're going to be stripping the first component so it doesn't
> > > > matter which path you use. If the patch is just to one file and looks
> > > > something like
> > > >
> > > > --- wc/file.old
> > > > +++ wc/file
> > > >
> > > > or
> > > >
> > > > --- wc/file
> > > > +++ wc/file.new
> > > >
> > > > or
> > > >
> > > > --- wc/file.old
> > > > +++ wc/file.new
> > > >
> > > > then obviously it's not so simple and the patch consumer (person) may
> > > > need to be able to tell "svn patch" which path it should look at if
> > > > we're to be able to handle cases like this.
> > >
> > > Yes. I've been trying to come up with a good UI for selecting the right
> > > filename for svn patch to use. Any ideas?
> >
> > Paraphrasing my commit reply a few hours ago, how about:
> >
> > * Always use the name from the /^+++/ line by default (regardless of --rd).
> > * Have a --strip-extension flag. (Effect: filename loses everything after the last dot)
> > * Have a the following config option:
> > [[[
> > # ~/.subversion/config
> > [miscellany]
> > patch-strip-extensions = .new .old .orig .org ~ \
> > .merge-left .merge-right .working
> > ]]]
> > for extensions that are stripped automagically.
> >
> > Sounds sane? (I haven't thought about this /too/ much, so beware of
> > bugs in the above idea)
>
> Looking at the patch(1) man page, it sounds like we don't really need
> to reinvent the wheel here:
>
> patch will choose the file name by performing the following steps, with
> the first match used:
>
> 1. If patch is operating in strict IEEE Std 1003.1-2008 (``POSIX'')
> mode, the first of the ``old'', ``new'' and ``index'' file names
> that exist is used. Otherwise, patch will examine either the
> ``old'' and ``new'' file names or, for a non-context diff, the
> ``index'' file name, and choose the file name with the fewest path
> components, the shortest basename, and the shortest total file name
> length (in that order).
(OT: using 'old' first means it'll use "foo.orig" if that's the old path
and my editor uses .orig extension for backup files)
>
> 2. If no file exists, patch checks for the existence of the files in an
> SCCS or RCS directory (using the appropriate prefix or suffix) using
> the criteria specified above. If found, patch will attempt to get
> or check out the file.
>
> 3. If no suitable file was found to patch, the patch file is a context
> or unified diff, and the old file was zero length, the new file name
> is created and used.
>
> 4. If the file name still cannot be determined, patch will prompt the
> user for the file name to use.
>
> I suppose we could implement the method described in step 1.
Which of the two methods described in step 1 do you have in mind?
Whatever we do, the algorithm should be predictable, i.e., a user should
be able to know (from reading a patch) what path it would be applied to.
> That would make us compatible to UNIX patch, and should resolve the two
> use cases I have in mind.
>
> Step 2 doesn't apply to us. Step 3 is used already. Step 4 could be
> implemented as well (but I'd prefer to do that post-1.7).
>
> Thoughts?
>
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Wed, Sep 15, 2010 at 06:15:58PM +0200, Daniel Shahaf wrote:
> Stefan Sperling wrote on Tue, Sep 14, 2010 at 21:18:26 +0200:
> > On Tue, Sep 14, 2010 at 12:49:06PM +0100, Julian Foad wrote:
> > > Right. The issue seems to be "How do I determine what path I should
> > > apply the patch to?" If the patch style is
> > >
> > > --- wc/file
> > > +++ wc/file
> > >
> > > or
> > >
> > > --- old_wc/file
> > > +++ new_wc/file
> > >
> > > then you're going to be stripping the first component so it doesn't
> > > matter which path you use. If the patch is just to one file and looks
> > > something like
> > >
> > > --- wc/file.old
> > > +++ wc/file
> > >
> > > or
> > >
> > > --- wc/file
> > > +++ wc/file.new
> > >
> > > or
> > >
> > > --- wc/file.old
> > > +++ wc/file.new
> > >
> > > then obviously it's not so simple and the patch consumer (person) may
> > > need to be able to tell "svn patch" which path it should look at if
> > > we're to be able to handle cases like this.
> >
> > Yes. I've been trying to come up with a good UI for selecting the right
> > filename for svn patch to use. Any ideas?
>
> Paraphrasing my commit reply a few hours ago, how about:
>
> * Always use the name from the /^+++/ line by default (regardless of --rd).
> * Have a --strip-extension flag. (Effect: filename loses everything after the last dot)
> * Have a the following config option:
> [[[
> # ~/.subversion/config
> [miscellany]
> patch-strip-extensions = .new .old .orig .org ~ \
> .merge-left .merge-right .working
> ]]]
> for extensions that are stripped automagically.
>
> Sounds sane? (I haven't thought about this /too/ much, so beware of
> bugs in the above idea)
Looking at the patch(1) man page, it sounds like we don't really need
to reinvent the wheel here:
patch will choose the file name by performing the following steps, with
the first match used:
1. If patch is operating in strict IEEE Std 1003.1-2008 (``POSIX'')
mode, the first of the ``old'', ``new'' and ``index'' file names
that exist is used. Otherwise, patch will examine either the
``old'' and ``new'' file names or, for a non-context diff, the
``index'' file name, and choose the file name with the fewest path
components, the shortest basename, and the shortest total file name
length (in that order).
2. If no file exists, patch checks for the existence of the files in an
SCCS or RCS directory (using the appropriate prefix or suffix) using
the criteria specified above. If found, patch will attempt to get
or check out the file.
3. If no suitable file was found to patch, the patch file is a context
or unified diff, and the old file was zero length, the new file name
is created and used.
4. If the file name still cannot be determined, patch will prompt the
user for the file name to use.
I suppose we could implement the method described in step 1.
That would make us compatible to UNIX patch, and should resolve the two
use cases I have in mind.
Step 2 doesn't apply to us. Step 3 is used already. Step 4 could be
implemented as well (but I'd prefer to do that post-1.7).
Thoughts?
Stefan
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Stefan Sperling wrote on Tue, Sep 14, 2010 at 21:18:26 +0200:
> On Tue, Sep 14, 2010 at 12:49:06PM +0100, Julian Foad wrote:
> > Right. The issue seems to be "How do I determine what path I should
> > apply the patch to?" If the patch style is
> >
> > --- wc/file
> > +++ wc/file
> >
> > or
> >
> > --- old_wc/file
> > +++ new_wc/file
> >
> > then you're going to be stripping the first component so it doesn't
> > matter which path you use. If the patch is just to one file and looks
> > something like
> >
> > --- wc/file.old
> > +++ wc/file
> >
> > or
> >
> > --- wc/file
> > +++ wc/file.new
> >
> > or
> >
> > --- wc/file.old
> > +++ wc/file.new
> >
> > then obviously it's not so simple and the patch consumer (person) may
> > need to be able to tell "svn patch" which path it should look at if
> > we're to be able to handle cases like this.
>
> Yes. I've been trying to come up with a good UI for selecting the right
> filename for svn patch to use. Any ideas?
Paraphrasing my commit reply a few hours ago, how about:
* Always use the name from the /^+++/ line by default (regardless of --rd).
* Have a --strip-extension flag. (Effect: filename loses everything after the last dot)
* Have a the following config option:
[[[
# ~/.subversion/config
[miscellany]
patch-strip-extensions = .new .old .orig .org ~ \
.merge-left .merge-right .working
]]]
for extensions that are stripped automagically.
Sounds sane? (I haven't thought about this /too/ much, so beware of
bugs in the above idea)
Daniel
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Sep 14, 2010 at 12:49:06PM +0100, Julian Foad wrote:
> Right. The issue seems to be "How do I determine what path I should
> apply the patch to?" If the patch style is
>
> --- wc/file
> +++ wc/file
>
> or
>
> --- old_wc/file
> +++ new_wc/file
>
> then you're going to be stripping the first component so it doesn't
> matter which path you use. If the patch is just to one file and looks
> something like
>
> --- wc/file.old
> +++ wc/file
>
> or
>
> --- wc/file
> +++ wc/file.new
>
> or
>
> --- wc/file.old
> +++ wc/file.new
>
> then obviously it's not so simple and the patch consumer (person) may
> need to be able to tell "svn patch" which path it should look at if
> we're to be able to handle cases like this.
Yes. I've been trying to come up with a good UI for selecting the right
filename for svn patch to use. Any ideas?
> Or do we look at the "Index" line as well, or instead?
No, we don't look there.
> The 'file.old'/'file.new' style perhaps isn't well suited to complex
> (tree-change) patches, so maybe we don't need sophisticated support for
> it and should concentrate on supporting the styles in which any 'old' or
> 'new' designations only appear in the leading stripped components of the
> paths.
I think we need to support both cases.
But complex tree changes will be covered by git-style diffs.
The case we're looking at is a "normal" unidiff, so there isn't much
tree-change support to begin with and we don't need to worry about it.
> > b) make svn patch --reverse-diff work for locally added files,
> > which currently doesn't work because of a bug -- svn patch
> > says the change had already been applied and does nothing.
>
> +1 to fixing this bug!
OK. I'll go down that route so. Thanks!
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Julian Foad <ju...@wandisco.com>.
On Mon, 2010-09-13, Stefan Sperling wrote:
> On Mon, Sep 13, 2010 at 05:50:05PM +0100, Julian Foad wrote:
> > On Mon, 2010-09-13, stsp@apache.org wrote:
> > > * subversion/libsvn_diff/parse-diff.c
> > > (svn_diff_parse_next_patch): When generating a reverse diff, do not swap
> > > the old and new filenames of the patch. The old filename in a unidiff
> > > is often useless (e.g. it contains a ".orig" extension), but the new
> > > filename should always be valid.
> >
> > Sounds odd. I don't doubt this change is correct, but I'm sure the
> > reasoning for it shouldn't have anything to do with what kind of
> > filenames you think people might use :-)
>
> Ah, good point. Let's see. The usual use cases for reverse diffs are:
>
> 1) someone accidentally produced the patch in the wrong order
> 2) you've applied a patch and you now want to revert it
>
> So far, svn patch has been addressing use case number 1.
> But of course I forgot about use case 1 when I made this commit, and I was
> thinking of use case 2.
Right. The issue seems to be "How do I determine what path I should
apply the patch to?" If the patch style is
--- wc/file
+++ wc/file
or
--- old_wc/file
+++ new_wc/file
then you're going to be stripping the first component so it doesn't
matter which path you use. If the patch is just to one file and looks
something like
--- wc/file.old
+++ wc/file
or
--- wc/file
+++ wc/file.new
or
--- wc/file.old
+++ wc/file.new
then obviously it's not so simple and the patch consumer (person) may
need to be able to tell "svn patch" which path it should look at if
we're to be able to handle cases like this.
Or do we look at the "Index" line as well, or instead?
The 'file.old'/'file.new' style perhaps isn't well suited to complex
(tree-change) patches, so maybe we don't need sophisticated support for
it and should concentrate on supporting the styles in which any 'old' or
'new' designations only appear in the leading stripped components of the
paths.
It looks like some figuring along these lines is needed!
> You might say that "svn revert" is what people should use for use case 2.
Certainly not. "svn revert" is for removing all your local changes (to
specified nodes), not for removing a single patch. It's common to want
to remove a patch by applying the same patch in reverse.
> But the underlying problem I'm trying to fix is that we should have a way
> to revert changes that svn patch has made to locally addded nodes. At the
> moment "svn revert" doesn't deal with this nicely. The two ways I see are:
>
> a) make svn revert smart enough to deal with it, e.g. by having
> svn patch store the pristine added file/directory in the pristine
> store/wc.db before patching it, and then have "svn revert" revert to
> the pristine added state, and only if run twice revert the addition
> itself (or make it depend on a command line option)
Nah, reverting to a previous locally-modified WC state is a whole new
concept. Call it 'local patch queues', 'local commits', 'stacked
changes', whatever.
Such a feature would be desirable not only for "svn patch" but for all
commands that merge changes into my local WC state - particularly "svn
merge". And not only for locally added files but also for
already-versioned files, so "svn update" is in the picture too. If
we're going to have such a feature, I don't see why we would want it to
be only for "svn patch" or only for locally added files.
> b) make svn patch --reverse-diff work for locally added files,
> which currently doesn't work because of a bug -- svn patch
> says the change had already been applied and does nothing.
+1 to fixing this bug!
> I was aiming to fix the bug I found in case b) and ran into a few issues,
> one of which was this filename problem. But it seems we'll need a better
> approach, because my commit broke use case 1) above (the diff produced
> in wrong order by accident) :(
>
> Thanks for pointing this out, I'll think more about it.
- Julian
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
Posted by Stefan Sperling <st...@elego.de>.
On Mon, Sep 13, 2010 at 05:50:05PM +0100, Julian Foad wrote:
> On Mon, 2010-09-13, stsp@apache.org wrote:
> > * subversion/libsvn_diff/parse-diff.c
> > (svn_diff_parse_next_patch): When generating a reverse diff, do not swap
> > the old and new filenames of the patch. The old filename in a unidiff
> > is often useless (e.g. it contains a ".orig" extension), but the new
> > filename should always be valid.
>
> Sounds odd. I don't doubt this change is correct, but I'm sure the
> reasoning for it shouldn't have anything to do with what kind of
> filenames you think people might use :-)
Ah, good point. Let's see. The usual use cases for reverse diffs are:
1) someone accidentally produced the patch in the wrong order
2) you've applied a patch and you now want to revert it
So far, svn patch has been addressing use case number 1.
But of course I forgot about use case 1 when I made this commit, and I was
thinking of use case 2.
You might say that "svn revert" is what people should use for use case 2.
But the underlying problem I'm trying to fix is that we should have a way
to revert changes that svn patch has made to locally addded nodes. At the
moment "svn revert" doesn't deal with this nicely. The two ways I see are:
a) make svn revert smart enough to deal with it, e.g. by having
svn patch store the pristine added file/directory in the pristine
store/wc.db before patching it, and then have "svn revert" revert to
the pristine added state, and only if run twice revert the addition
itself (or make it depend on a command line option)
b) make svn patch --reverse-diff work for locally added files,
which currently doesn't work because of a bug -- svn patch
says the change had already been applied and does nothing.
I was aiming to fix the bug I found in case b) and ran into a few issues,
one of which was this filename problem. But it seems we'll need a better
approach, because my commit broke use case 1) above (the diff produced
in wrong order by accident) :(
Thanks for pointing this out, I'll think more about it.
Stefan