You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2005/11/04 23:26:46 UTC
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
dberlin@tigris.org writes:
> Author: dberlin
> Date: Fri Nov 4 10:04:56 2005
> New Revision: 17190
>
> Modified:
> trunk/subversion/libsvn_wc/diff.c
>
> Log:
> Fix url->wc diff against schedule-delete files.
>
> * subversion/libsvn_wc/diff.c
> (delete_entry): If the entry is marked for deletion, use
> the empty file.
>
>
>
> Modified: trunk/subversion/libsvn_wc/diff.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_wc/diff.c?rev=17190&p1=trunk/subversion/libsvn_wc/diff.c&p2=trunk/subversion/libsvn_wc/diff.c&r1=17189&r2=17190
> ==============================================================================
> --- trunk/subversion/libsvn_wc/diff.c (original)
> +++ trunk/subversion/libsvn_wc/diff.c Fri Nov 4 10:04:56 2005
> @@ -855,6 +855,8 @@
> the empty file against the current working copy. If
> 'reverse_order' is set, then show a deletion. */
>
> + if (entry->schedule == svn_wc_schedule_delete)
> + SVN_ERR (get_empty_file (pb->edit_baton, &full_path));
> SVN_ERR (get_local_mimetypes (&pristine_mimetype, &working_mimetype,
> NULL, adm_access, full_path, pool));
Is that the right thing to do? It doesn't have a regression test so
I'm not sure what behaviour you are trying to achieve.
$ svnadmin create repo
$ svn co file://`pwd`/repo wc
$ echo xxx > wc/foo
$ svn add wc/foo
$ svn ci -m "" wc
$ svn rm wc/foo
$ svn diff -r0 wc
Before your change I get
$ svn diff -r0 wc
../svn/subversion/libsvn_subr/io.c:2219: (apr_err=2)
svn: Can't open file 'wc/foo': No such file or directory
that's obviously a bug, with you change I get
$ svn diff -r0 wc
Index: /tmp/tmp
===================================================================
Index: wc/foo
===================================================================
--- wc/foo (revision 0)
+++ wc/foo (working copy)
@@ -1 +0,0 @@
-xxx
It's that really the correct output? There are two diffs: first an
empty diff, the /tmp/tmp part, and then a second diff, the wc/foo
part, that represents the local change. I suppose it's better than it
was, but I'm not sure it's correct.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Daniel Berlin <db...@dberlin.org>.
>
> Okay.
> Try the attached, it should give the output you are looking for
> If so, i'll work up a changelog and commit it.
With the extra tab removed, of course.
(sorry, i can't configure emacs or vim to not use tabs, gcc
requires them).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
Daniel Berlin <db...@dberlin.org> writes:
> Try the attached, it should give the output you are looking for
> If so, i'll work up a changelog and commit it.
Yes, that appears to make the delete behaviour consistent with the
change behaviour.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Stein wrote:
>
> Also, given that there were problems with his original commit, then
> why shouldn't he go ahead and try to fix them in real time? It isn't
> like his commits will break things further, no?
That's interesting. I was trying to convey the opposite view: since the
original commit was made within minutes of first posting about the problem,
before any response, and turned out to be wrong (to be blunt), I would prefer
to see caution rather haste in fixing the fix. It gets messy if a single
logical fix ends up being implemented by a chain of commits each correcting the
previous one.
> I don't think it would be advisable to get into a mode of "every
> change must be posted for review first." It *really* slows down a
> project. Not saying you're recommending that, but that is how I read
> this request.
I'd say a change that the committer feels is safe need not be reviewed first;
but when a problem that was initially thought to be simple and obvious is shown
to be not so, that is the time to slow down and take more care.
Just my opinion.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Nov 07, 2005 at 01:15:14AM +0000, Julian Foad wrote:
>...
> [...]
> >>Daniel, it's great that you're so quick to come up with a new fix, but
> >>please repost the patch with its log message (and "[PATCH]" in the
> >>subject line) and hold off committing until tomorrow to give the weekday
> >>workers a chance to review it and check whether it's the right fix.
> >
> >Julian: I don't know what gives you the idea that what i say means i
> >wouldn't follow the rules in hacking.html, but i have in the past, and
> >will in the future.
>
> Oops. I realised that was a bit ... inflammatory? presumptuous? ... as
> soon as I sent it. Sorry, but I got the impression you were going to
> commit it within minutes.
Also, given that there were problems with his original commit, then
why shouldn't he go ahead and try to fix them in real time? It isn't
like his commits will break things further, no?
I don't think it would be advisable to get into a mode of "every
change must be posted for review first." It *really* slows down a
project. Not saying you're recommending that, but that is how I read
this request.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Daniel Berlin <db...@dberlin.org>.
On Mon, 2005-11-07 at 01:29 +0000, Philip Martin wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
> > Daniel Berlin wrote:
> >> Similar, but not the same.
> >> I only get the Index: wc/foo part.
> >> I get no Index: /tmp/tmp line.
> >
> > Well, that's curious. Philip, are you sure it was different for you?
>
> It's /dev/null since the null device commit, but internal/external
> diff makes no difference to the output I get. I used GNU diff and
> passed '-x -c' to verify whether or not I it was getting invoked.
So I went and checked out a completely clean tree with that change in
it, built it, and now i *do* get the same output.
However, if i move over to the tree i committed from, i don't.
I do have some very unrelated changes in the tree i commited the
original fix from (by accident, i only noticed them later), but i can't
see how any of them could have changed this output.
However logically, they must be the reason the output is different.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@btopenworld.com> writes:
> Daniel Berlin wrote:
>> Similar, but not the same.
>> I only get the Index: wc/foo part.
>> I get no Index: /tmp/tmp line.
>
> Well, that's curious. Philip, are you sure it was different for you?
It's /dev/null since the null device commit, but internal/external
diff makes no difference to the output I get. I used GNU diff and
passed '-x -c' to verify whether or not I it was getting invoked.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Berlin wrote:
> On Mon, 2005-11-07 at 00:06 +0000, Julian Foad wrote:
>
>>Daniel Berlin wrote (earlier):
>>
>>>>$ svn diff -r0 wc
>>>>Index: /tmp/tmp
>>>>===================================================================
>>>>Index: wc/foo
>>>>===================================================================
>>>>--- wc/foo (revision 0)
>>>>+++ wc/foo (working copy)
>>>>@@ -1 +0,0 @@
>>>>-xxx
>>>>
>>>>It's that really the correct output?
>>>
>>>That's not the output i get, but i've got an external diff configured :(
>>
>>Well, what output do you get? Philip replied "External diff makes no
>>difference as far as I can see." If you got something similar to this, don't
>>you agree it's wrong?
>
> Similar, but not the same.
>
> I only get the Index: wc/foo part.
>
> I get no Index: /tmp/tmp line.
Well, that's curious. Philip, are you sure it was different for you?
[...]
>>Daniel, it's great that you're so quick to come up with a new fix, but please
>>repost the patch with its log message (and "[PATCH]" in the subject line) and
>>hold off committing until tomorrow to give the weekday workers a chance to
>>review it and check whether it's the right fix.
>
> Julian: I don't know what gives you the idea that what i say means i
> wouldn't follow the rules in hacking.html, but i have in the past, and
> will in the future.
Oops. I realised that was a bit ... inflammatory? presumptuous? ... as soon as
I sent it. Sorry, but I got the impression you were going to commit it within
minutes.
Also, I've just noticed that your patch file was called "forphilip.diff". If I
had noticed that previously I might have realised it wasn't yet intended for
all of us readers, but I didn't.
[...]
> As for the test below, i fixed it well before your email, but ...
>>There's no need to create and add a file: the default test repository already
>>has one, called "iota", in order to save test writers from this chore.
>
> But it won't help in this case, for the reasons below.
[...]
> The bug only tickles when you are comparing a revision the file was
> added in.
> Thus, you can't use iota, because it's revision includes the othe
> rfiles, and excluding them won't work.
> You actually have to add a new file, commit it, remove it, and then diff
> *that* revision, to be able to compare out.
Oh. I'm not following the details exactly, but OK. Thanks for explaining.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Daniel Berlin <db...@dberlin.org>.
On Mon, 2005-11-07 at 00:06 +0000, Julian Foad wrote:
> Daniel Berlin wrote (earlier):
> >>
> >> $ svn diff -r0 wc
> >> Index: /tmp/tmp
> >> ===================================================================
> >> Index: wc/foo
> >> ===================================================================
> >> --- wc/foo (revision 0)
> >> +++ wc/foo (working copy)
> >> @@ -1 +0,0 @@
> >> -xxx
> >>
> >> It's that really the correct output?
> >
> > That's not the output i get, but i've got an external diff configured :(
>
> Well, what output do you get? Philip replied "External diff makes no
> difference as far as I can see." If you got something similar to this, don't
> you agree it's wrong?
Similar, but not the same.
I only get the Index: wc/foo part.
I get no Index: /tmp/tmp line.
> If you got something very different, what's up?
>
>
> Daniel Berlin wrote:
> >> deleted perhaps I should get an empty diff:
> >>
> >> $ svn diff -r0 wc
> >> Index: wc/foo
> >> ===================================================================
> >>
> >> In all our diffs there is the issue about whether the header should be
> >> suppressed if there is no difference, but given that we currently show
> >> the header I would expect a single empty diff with the correct header.
> >
> > Okay.
> > Try the attached, it should give the output you are looking for
> > If so, i'll work up a changelog and commit it.
>
> Daniel, it's great that you're so quick to come up with a new fix, but please
> repost the patch with its log message (and "[PATCH]" in the subject line) and
> hold off committing until tomorrow to give the weekday workers a chance to
> review it and check whether it's the right fix.
Julian: I don't know what gives you the idea that what i say means i
wouldn't follow the rules in hacking.html, but i have in the past, and
will in the future.
I'm sorry if i don't precisely write exactly every single step i plan on
taking before committing this patch (which includes posting it with a
proper changelog and waiting), and this bothers you, however you can
assume safely that i would not go off and just randomly start committing
things without posting them properly. Are you next going to point out i
didn't say i would run the regression tests before committing it?
As for the test below, i fixed it well before your email, but ...
> >
> > +def diff_schedule_delete(sbox):
> > + "scheduled deleted"
> > + sbox.build()
> > + wc_dir = sbox.wc_dir
> > + was_cwd = os.getcwd()
> > + os.chdir(wc_dir)
> > + svntest.main.file_append('foo', "xxx")
> > + svntest.main.run_svn(None, 'add', 'foo')
> > + diff_output, err_output = svntest.main.run_svn(None, 'ci', '-m', 'log msg')
>
> There's no need to create and add a file: the default test repository already
> has one, called "iota", in order to save test writers from this chore.
But it won't help in this case, for the reasons below.
> > + if err_output: raise svntest.Failure
> > + svntest.main.run_svn(None, 'rm', 'foo')
> > + diff_output, err = svntest.actions.run_and_verify_svn(None, None, [],
> > + 'diff', '-r', '0' )
> > + if err: raise svntest.Failure
>
> Although lack of stderr output is sufficient to distinguish the fix from the
> original behaviour, it would be much better to test the contents of
> "diff_output", like most of the other tests in that file do.
Except you can't do this if you use iota.
svn diff -r 0 foo (or iota) won't show the bug.
svn commit and svn diff -r 0 will show the output from the rest of the
diff (which includes all the files because they haven't been committed
yet).
The bug only tickles when you are comparing a revision the file was
added in.
Thus, you can't use iota, because it's revision includes the othe
rfiles, and excluding them won't work.
You actually have to add a new file, commit it, remove it, and then diff
*that* revision, to be able to compare out.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Berlin wrote (earlier):
>>
>> $ svn diff -r0 wc
>> Index: /tmp/tmp
>> ===================================================================
>> Index: wc/foo
>> ===================================================================
>> --- wc/foo (revision 0)
>> +++ wc/foo (working copy)
>> @@ -1 +0,0 @@
>> -xxx
>>
>> It's that really the correct output?
>
> That's not the output i get, but i've got an external diff configured :(
Well, what output do you get? Philip replied "External diff makes no
difference as far as I can see." If you got something similar to this, don't
you agree it's wrong? If you got something very different, what's up?
Daniel Berlin wrote:
>> deleted perhaps I should get an empty diff:
>>
>> $ svn diff -r0 wc
>> Index: wc/foo
>> ===================================================================
>>
>> In all our diffs there is the issue about whether the header should be
>> suppressed if there is no difference, but given that we currently show
>> the header I would expect a single empty diff with the correct header.
>
> Okay.
> Try the attached, it should give the output you are looking for
> If so, i'll work up a changelog and commit it.
Daniel, it's great that you're so quick to come up with a new fix, but please
repost the patch with its log message (and "[PATCH]" in the subject line) and
hold off committing until tomorrow to give the weekday workers a chance to
review it and check whether it's the right fix.
> Index: subversion/libsvn_wc/diff.c
> ===================================================================
> --- subversion/libsvn_wc/diff.c (revision 17206)
> +++ subversion/libsvn_wc/diff.c (working copy)
> @@ -855,8 +855,6 @@ delete_entry (const char *path,
> the empty file against the current working copy. If
> 'reverse_order' is set, then show a deletion. */
>
> - if (entry->schedule == svn_wc_schedule_delete)
> - SVN_ERR (get_empty_file (pb->edit_baton, &full_path));
> SVN_ERR (get_local_mimetypes (&pristine_mimetype, &working_mimetype,
> NULL, adm_access, full_path, pool));
>
> @@ -866,6 +864,8 @@ delete_entry (const char *path,
> const char *textbase = svn_wc__text_base_path (full_path,
> FALSE, pool);
>
> + if (entry->schedule == svn_wc_schedule_delete)
> + SVN_ERR (get_empty_file (pb->edit_baton, &textbase));
> SVN_ERR (svn_wc_get_prop_diffs (NULL, &baseprops, full_path,
> adm_access, pool));
> SVN_ERR (pb->edit_baton->callbacks->file_deleted
> @@ -879,12 +879,15 @@ delete_entry (const char *path,
> }
> else
> {
> - /* Or normally, show the working file being added. */
> + const char *secondpath = full_path;
> + if (entry->schedule == svn_wc_schedule_delete)
> + secondpath = empty_file;
> + /* Or normally, show the working file being added. */
> /* ### Show the properties as well. */
> SVN_ERR (pb->edit_baton->callbacks->file_added
> (NULL, NULL, NULL, full_path,
> empty_file,
> - full_path,
> + secondpath,
> 0, entry->revision,
> NULL,
> working_mimetype,
> Index: subversion/tests/clients/cmdline/diff_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/diff_tests.py (revision 17206)
> +++ subversion/tests/clients/cmdline/diff_tests.py (working copy)
> @@ -1952,6 +1952,20 @@ def diff_property_changes_to_base(sbox):
> finally:
> os.chdir(current_dir)
>
> +def diff_schedule_delete(sbox):
> + "scheduled deleted"
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + was_cwd = os.getcwd()
> + os.chdir(wc_dir)
> + svntest.main.file_append('foo', "xxx")
> + svntest.main.run_svn(None, 'add', 'foo')
> + diff_output, err_output = svntest.main.run_svn(None, 'ci', '-m', 'log msg')
There's no need to create and add a file: the default test repository already
has one, called "iota", in order to save test writers from this chore.
> + if err_output: raise svntest.Failure
> + svntest.main.run_svn(None, 'rm', 'foo')
> + diff_output, err = svntest.actions.run_and_verify_svn(None, None, [],
> + 'diff', '-r', '0' )
> + if err: raise svntest.Failure
Although lack of stderr output is sufficient to distinguish the fix from the
original behaviour, it would be much better to test the contents of
"diff_output", like most of the other tests in that file do.
> ########################################################################
> #Run the tests
> @@ -1987,6 +2001,7 @@ test_list = [ None,
> diff_force,
> XFail(diff_renamed_dir),
> XFail(diff_property_changes_to_base),
> + diff_schedule_delete
We prefer to keep the trailing comma on the last item so that each new test
just requires one new line added here.
> ]
Don't forget to update branches/1.3.x/STATUS now that r17190 is not a suitable
candidate for back-porting on its own.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Daniel Berlin <db...@dberlin.org>.
> deleted perhaps I should get an empty diff:
>
> $ svn diff -r0 wc
> Index: wc/foo
> ===================================================================
>
>
> In all our diffs there is the issue about whether the header should be
> suppressed if there is no difference, but given that we currently show
> the header I would expect a single empty diff with the correct header.
Okay.
Try the attached, it should give the output you are looking for
If so, i'll work up a changelog and commit it.
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Philip Martin <ph...@codematters.co.uk>.
Daniel Berlin <db...@dberlin.org> writes:
>> Is that the right thing to do? It doesn't have a regression test so
>> I'm not sure what behaviour you are trying to achieve.
>>
>> $ svnadmin create repo
>> $ svn co file://`pwd`/repo wc
>> $ echo xxx > wc/foo
>> $ svn add wc/foo
>> $ svn ci -m "" wc
>> $ svn rm wc/foo
>> $ svn diff -r0 wc
>>
>> Before your change I get
>>
>> $ svn diff -r0 wc
>> ../svn/subversion/libsvn_subr/io.c:2219: (apr_err=2)
>> svn: Can't open file 'wc/foo': No such file or directory
>>
>> that's obviously a bug, with you change I get
>>
>> $ svn diff -r0 wc
>> Index: /tmp/tmp
>> ===================================================================
>> Index: wc/foo
>> ===================================================================
>> --- wc/foo (revision 0)
>> +++ wc/foo (working copy)
>> @@ -1 +0,0 @@
>> -xxx
>>
>> It's that really the correct output?
>
> That's not the output i get, but i've got an external diff configured
> :(
External diff makes no difference as far as I can see.
>
>> There are two diffs: first an
>> empty diff, the /tmp/tmp part, and then a second diff, the wc/foo
>> part, that represents the local change. I suppose it's better than it
>> was, but I'm not sure it's correct.
>
> So what would you expect?
>
> Only the "wc/foo" part?
Well, if I have a local modification:
$ svn diff wc
Index: wc/foo
===================================================================
--- wc/foo (revision 1)
+++ wc/foo (working copy)
@@ -1 +1,2 @@
xxx
+yyy
and I do a -rN diff against a repository version that has the same
modification I get an empty diff:
$ svn diff -r2 wc
Index: wc/foo
===================================================================
By extension, if I have a local schedule delete:
$ svn diff wc
Index: wc/foo
===================================================================
--- wc/foo (revision 1)
+++ wc/foo (working copy)
@@ -1 +0,0 @@
-xxx
and I do a -rN diff against a repository version that has the file
deleted perhaps I should get an empty diff:
$ svn diff -r0 wc
Index: wc/foo
===================================================================
In all our diffs there is the issue about whether the header should be
suppressed if there is no difference, but given that we currently show
the header I would expect a single empty diff with the correct header.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17190 - trunk/subversion/libsvn_wc
Posted by Daniel Berlin <db...@dberlin.org>.
> Is that the right thing to do? It doesn't have a regression test so
> I'm not sure what behaviour you are trying to achieve.
>
> $ svnadmin create repo
> $ svn co file://`pwd`/repo wc
> $ echo xxx > wc/foo
> $ svn add wc/foo
> $ svn ci -m "" wc
> $ svn rm wc/foo
> $ svn diff -r0 wc
>
> Before your change I get
>
> $ svn diff -r0 wc
> ../svn/subversion/libsvn_subr/io.c:2219: (apr_err=2)
> svn: Can't open file 'wc/foo': No such file or directory
>
> that's obviously a bug, with you change I get
>
> $ svn diff -r0 wc
> Index: /tmp/tmp
> ===================================================================
> Index: wc/foo
> ===================================================================
> --- wc/foo (revision 0)
> +++ wc/foo (working copy)
> @@ -1 +0,0 @@
> -xxx
>
> It's that really the correct output?
That's not the output i get, but i've got an external diff
configured :(
> There are two diffs: first an
> empty diff, the /tmp/tmp part, and then a second diff, the wc/foo
> part, that represents the local change. I suppose it's better than it
> was, but I'm not sure it's correct.
So what would you expect?
Only the "wc/foo" part?
If so, that's simple enough, and i'll add a regression test to verify it
ends up like that.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org