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 16:15:58 UTC
Re: svn commit: r996581 - in /subversion/trunk/subversion:
libsvn_diff/parse-diff.c tests/cmdline/patch_tests.py
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 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