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_at_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
Received on 2010-09-14 13:50:35 CEST