You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by sv...@apache.org on 2013/05/16 06:01:25 UTC
svn commit: r1483186 - in /subversion/branches/1.8.x: ./ STATUS
subversion/libsvn_client/repos_diff.c subversion/tests/cmdline/diff_tests.py
Author: svn-role
Date: Thu May 16 04:01:25 2013
New Revision: 1483186
URL: http://svn.apache.org/r1483186
Log:
Merge the r1482969 group from trunk:
* r1482969, r1482970
Fix issue #4366 ("client SEGFAULTs diffing a repos rev in which an
empty file was added").
Justification:
SEGFAULTs are consider rude in polite company.
Votes:
+1: cmpilato, philip, rhuijben
Modified:
subversion/branches/1.8.x/ (props changed)
subversion/branches/1.8.x/STATUS
subversion/branches/1.8.x/subversion/libsvn_client/repos_diff.c
subversion/branches/1.8.x/subversion/tests/cmdline/diff_tests.py
Propchange: subversion/branches/1.8.x/
------------------------------------------------------------------------------
Merged /subversion/trunk:r1482969-1482970
Modified: subversion/branches/1.8.x/STATUS
URL: http://svn.apache.org/viewvc/subversion/branches/1.8.x/STATUS?rev=1483186&r1=1483185&r2=1483186&view=diff
==============================================================================
--- subversion/branches/1.8.x/STATUS (original)
+++ subversion/branches/1.8.x/STATUS Thu May 16 04:01:25 2013
@@ -135,14 +135,6 @@ Approved changes:
# blocking issues. If in doubt see this link for details:
# http://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization
- * r1482969, r1482970
- Fix issue #4366 ("client SEGFAULTs diffing a repos rev in which an
- empty file was added").
- Justification:
- SEGFAULTs are consider rude in polite company.
- Votes:
- +1: cmpilato, philip, rhuijben
-
* r1482973
Avoid using predictable temporary filenames based on "tempfile".
Justification:
Modified: subversion/branches/1.8.x/subversion/libsvn_client/repos_diff.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.8.x/subversion/libsvn_client/repos_diff.c?rev=1483186&r1=1483185&r2=1483186&view=diff
==============================================================================
--- subversion/branches/1.8.x/subversion/libsvn_client/repos_diff.c (original)
+++ subversion/branches/1.8.x/subversion/libsvn_client/repos_diff.c Thu May 16 04:01:25 2013
@@ -933,12 +933,12 @@ apply_textdelta(void *file_baton,
}
/* Open the file to be used as the base for second revision */
- src_stream = svn_stream_lazyopen_create(lazy_open_source, fb, FALSE,
+ src_stream = svn_stream_lazyopen_create(lazy_open_source, fb, TRUE,
scratch_pool);
/* Open the file that will become the second revision after applying the
text delta, it starts empty */
- result_stream = svn_stream_lazyopen_create(lazy_open_result, fb, FALSE,
+ result_stream = svn_stream_lazyopen_create(lazy_open_result, fb, TRUE,
scratch_pool);
svn_txdelta_apply(src_stream,
Modified: subversion/branches/1.8.x/subversion/tests/cmdline/diff_tests.py
URL: http://svn.apache.org/viewvc/subversion/branches/1.8.x/subversion/tests/cmdline/diff_tests.py?rev=1483186&r1=1483185&r2=1483186&view=diff
==============================================================================
--- subversion/branches/1.8.x/subversion/tests/cmdline/diff_tests.py (original)
+++ subversion/branches/1.8.x/subversion/tests/cmdline/diff_tests.py Thu May 16 04:01:25 2013
@@ -4520,6 +4520,36 @@ def diff_dir_replaced_by_dir(sbox):
svntest.actions.run_and_verify_svn(None, expected_output, [],
'diff', '--summarize', wc_dir)
+
+@Issue(4366)
+def diff_repos_empty_file_addition(sbox):
+ "repos diff of rev which adds empty file"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ # Add and commit an empty file.
+ svntest.main.file_append(sbox.ospath('newfile'), "")
+ svntest.main.run_svn(None, 'add', sbox.ospath('newfile'))
+ expected_output = svntest.wc.State(sbox.wc_dir, {
+ 'newfile': Item(verb='Adding'),
+ })
+ expected_status = svntest.actions.get_virginal_state(sbox.wc_dir, 1)
+ expected_status.add({
+ 'newfile' : Item(status=' ', wc_rev=2),
+ })
+ svntest.actions.run_and_verify_commit(sbox.wc_dir, expected_output,
+ expected_status, None, sbox.wc_dir)
+
+ # Now diff the revision that added the empty file.
+ expected_output = [
+ 'Index: newfile\n',
+ '===================================================================\n',
+ ]
+ svntest.actions.run_and_verify_svn(None, expected_output, [],
+ 'diff', '-c', '2', sbox.repo_url)
+
+
########################################################################
#Run the tests
@@ -4598,6 +4628,7 @@ test_list = [ None,
local_tree_replace,
diff_dir_replaced_by_file,
diff_dir_replaced_by_dir,
+ diff_repos_empty_file_addition,
]
if __name__ == '__main__':
Re: svn commit: r1483186 - in /subversion/branches/1.8.x: ./ STATUS
subversion/libsvn_client/repos_diff.c subversion/tests/cmdline/diff_tests.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/20/2013 10:05 AM, Ivan Zhakov wrote:
> I just noted that this fix also fix crash of merging addition of empty
> file. Which very often case without workaround. So fix is critical,
> because we use repos diff in our merge code.
Agree. This is a critical bug fix. And because it's a critical fix, if we
were in the last week of our soak period, we'd roll a new RC and extend by
another week to accommodate the fix:
Per the policy:
"If a critical bugfix is made during the final week of the stabilization
period, the final week is restarted. The final A.B.0 release is always
identical to the release candidate made one week before (with the exceptions
discussed below)."
But we aren't in the final week. We're in the *first* one. So, as long as
we put out another RC by the end of week three, no addition soak time is needed.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1483186 - in /subversion/branches/1.8.x: ./ STATUS
subversion/libsvn_client/repos_diff.c subversion/tests/cmdline/diff_tests.py
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Mon, May 20, 2013 at 5:59 PM, C. Michael Pilato <cm...@collab.net> wrote:
> On 05/20/2013 08:45 AM, Ivan Zhakov wrote:
>> On Mon, May 20, 2013 at 4:35 PM, C. Michael Pilato <cm...@collab.net> wrote:
>>> This bug fix is a single boolean toggle that affects one type of operation
>>> (a repository diff which adds empty files) through effectively a single
>>> codepath. The bug is well understood; the fix extremely localized. There's
>>> no need to restart the soak period for this.
>>>
>> Sometimes single boolean toggle may affect a lot of code flow.
>> In this case the fix made in one of inner functions: diff algorithm. Which
>> is used in many places: merges and diffing.
>
> You've got your layering wrong on this one, Ivan. The code fixed by this
> change is used by repository diffs, which sits *atop* the lower-level diff
> display generation and merge handling logic.
>
> As I've already explained, this bug is triggered by one scenario: a
> repository diff which adds empty files. I'll grant you that sometimes that
> diff is shown to us on the console and sometimes it's merged into the
> working copy. But it's a single, well-understood scenario that's failing
> here, and the fix makes precisely that change needed to fix it (by reverting
> to the previous behavior of ensuring that the tempfile gets opened and
> closed rather than merely skipped). The fix is not widespread. It's not
> far-reaching. We didn't blaze any new trails here. In summary: this is not
> a destabilizing change.
>
I just noted that this fix also fix crash of merging addition of empty
file. Which very often case without workaround. So fix is critical,
because we use repos diff in our merge code.
>>> We also have a one-week soak period extension as part of our policy:
>>> because this is a critical bugfix, *if* we were currently in our final week
>>> of soak time, we would need to re-roll a new RC with the fix and extend our
>>> soak time by another week.
>>>
>> I'm fine to extend soak period because of this change instead of full
>> restart, but we need some kind of soak period extension.
>
> Please read our policy. The docs pretty clear:
> http://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization
>
I read it many times, Mike :)
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1483186 - in /subversion/branches/1.8.x: ./ STATUS
subversion/libsvn_client/repos_diff.c subversion/tests/cmdline/diff_tests.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/20/2013 08:45 AM, Ivan Zhakov wrote:
> On Mon, May 20, 2013 at 4:35 PM, C. Michael Pilato <cm...@collab.net> wrote:
>> This bug fix is a single boolean toggle that affects one type of operation
>> (a repository diff which adds empty files) through effectively a single
>> codepath. The bug is well understood; the fix extremely localized. There's
>> no need to restart the soak period for this.
>>
> Sometimes single boolean toggle may affect a lot of code flow.
> In this case the fix made in one of inner functions: diff algorithm. Which
> is used in many places: merges and diffing.
You've got your layering wrong on this one, Ivan. The code fixed by this
change is used by repository diffs, which sits *atop* the lower-level diff
display generation and merge handling logic.
As I've already explained, this bug is triggered by one scenario: a
repository diff which adds empty files. I'll grant you that sometimes that
diff is shown to us on the console and sometimes it's merged into the
working copy. But it's a single, well-understood scenario that's failing
here, and the fix makes precisely that change needed to fix it (by reverting
to the previous behavior of ensuring that the tempfile gets opened and
closed rather than merely skipped). The fix is not widespread. It's not
far-reaching. We didn't blaze any new trails here. In summary: this is not
a destabilizing change.
>> We also have a one-week soak period extension as part of our policy:
>> because this is a critical bugfix, *if* we were currently in our final week
>> of soak time, we would need to re-roll a new RC with the fix and extend our
>> soak time by another week.
>>
> I'm fine to extend soak period because of this change instead of full
> restart, but we need some kind of soak period extension.
Please read our policy. The docs pretty clear:
http://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1483186 - in /subversion/branches/1.8.x: ./ STATUS
subversion/libsvn_client/repos_diff.c subversion/tests/cmdline/diff_tests.py
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Mon, May 20, 2013 at 4:35 PM, C. Michael Pilato <cm...@collab.net> wrote:
> On 05/20/2013 06:58 AM, Ivan Zhakov wrote:
>> On Thu, May 16, 2013 at 8:01 AM, <sv...@apache.org> wrote:
>>> Author: svn-role
>>> Date: Thu May 16 04:01:25 2013
>>> New Revision: 1483186
>>>
>>> URL: http://svn.apache.org/r1483186
>>> Log:
>>> Merge the r1482969 group from trunk:
>>>
>>> * r1482969, r1482970
>>> Fix issue #4366 ("client SEGFAULTs diffing a repos rev in which an
>>> empty file was added").
>>> Justification:
>>> SEGFAULTs are consider rude in polite company.
>>> Votes:
>>> +1: cmpilato, philip, rhuijben
>>>
>> Issue #4366 also affects merging of empty files: merge of empty files
>> add causes crash in Subversion 1.8.0-rc2. It seems to be significant
>> reason to re-roll RC and restart soak period.
>
> +1 to a new RC (scheduled by common agreement of the devs -- no need to rush
> one out the door).
>
> -1 to restarting the soak period for this.
>
> May I remind us all: a full four-week soak period restart is done to allow
> time to exercise the many different codepaths affected by a destabilizing
> bugfix. It is *not* done simply because the bug that got fixed is a
> high-priority one.
>
> This bug fix is a single boolean toggle that affects one type of operation
> (a repository diff which adds empty files) through effectively a single
> codepath. The bug is well understood; the fix extremely localized. There's
> no need to restart the soak period for this.
>
Sometimes single boolean toggle may affect a lot of code flow. In this
case the fix made in one of inner functions: diff algorithm. Which is
used in many places: merges and diffing.
> We also have a one-week soak period extension as part of our policy:
> because this is a critical bugfix, *if* we were currently in our final week
> of soak time, we would need to re-roll a new RC with the fix and extend our
> soak time by another week.
>
I'm fine to extend soak period because of this change instead of full
restart, but we need some kind of soak period extension.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1483186 - in /subversion/branches/1.8.x: ./ STATUS
subversion/libsvn_client/repos_diff.c subversion/tests/cmdline/diff_tests.py
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/20/2013 06:58 AM, Ivan Zhakov wrote:
> On Thu, May 16, 2013 at 8:01 AM, <sv...@apache.org> wrote:
>> Author: svn-role
>> Date: Thu May 16 04:01:25 2013
>> New Revision: 1483186
>>
>> URL: http://svn.apache.org/r1483186
>> Log:
>> Merge the r1482969 group from trunk:
>>
>> * r1482969, r1482970
>> Fix issue #4366 ("client SEGFAULTs diffing a repos rev in which an
>> empty file was added").
>> Justification:
>> SEGFAULTs are consider rude in polite company.
>> Votes:
>> +1: cmpilato, philip, rhuijben
>>
> Issue #4366 also affects merging of empty files: merge of empty files
> add causes crash in Subversion 1.8.0-rc2. It seems to be significant
> reason to re-roll RC and restart soak period.
+1 to a new RC (scheduled by common agreement of the devs -- no need to rush
one out the door).
-1 to restarting the soak period for this.
May I remind us all: a full four-week soak period restart is done to allow
time to exercise the many different codepaths affected by a destabilizing
bugfix. It is *not* done simply because the bug that got fixed is a
high-priority one.
This bug fix is a single boolean toggle that affects one type of operation
(a repository diff which adds empty files) through effectively a single
codepath. The bug is well understood; the fix extremely localized. There's
no need to restart the soak period for this.
We also have a one-week soak period extension as part of our policy:
because this is a critical bugfix, *if* we were currently in our final week
of soak time, we would need to re-roll a new RC with the fix and extend our
soak time by another week.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development
Re: svn commit: r1483186 - in /subversion/branches/1.8.x: ./ STATUS
subversion/libsvn_client/repos_diff.c subversion/tests/cmdline/diff_tests.py
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On Thu, May 16, 2013 at 8:01 AM, <sv...@apache.org> wrote:
> Author: svn-role
> Date: Thu May 16 04:01:25 2013
> New Revision: 1483186
>
> URL: http://svn.apache.org/r1483186
> Log:
> Merge the r1482969 group from trunk:
>
> * r1482969, r1482970
> Fix issue #4366 ("client SEGFAULTs diffing a repos rev in which an
> empty file was added").
> Justification:
> SEGFAULTs are consider rude in polite company.
> Votes:
> +1: cmpilato, philip, rhuijben
>
Issue #4366 also affects merging of empty files: merge of empty files
add causes crash in Subversion 1.8.0-rc2. It seems to be significant
reason to re-roll RC and restart soak period.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com