You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by HuiHuang <ye...@yahoo.com.cn> on 2009/05/27 00:32:58 UTC

design document

Hi,

The following is design document for "commit from multiple woring copies". 
If there is any problem, please tell me and I will modify it.

Thanks all~

Name: Commit from Multiple Working Copies.
Author: HuiHuang.
Date: 2009-5-26.
Version: 1.0.
 
1) Expected behavior 
When committing files, listing their paths, no matter whether they belong to the same work copy 
or not, if they all live in the same repository, they should be committed in one transaction successfully.
 
2) Actual behavior
2a) If the committing files belong to the same working copy, they will be committed in one 
transaction successfully. 
2b) Otherwise, if they belong to more than one working copy, svn will output an error which 
indicates that their common ancestor is not a work copy and commit action fails.
 
3) Suggested change
This section has two parts. The first part introduces how this issue is solved in SVNKit, and then 
I will give my suggestions on how we should solve this issue in the second part.
 
3a) SVNKit's solution
 
1. SVNKit receives list of paths to commit.
 
2. All paths are grouped by wc root path, so we get a map of wc_root:paths pairs - one for each 
working copy.
 
3. For each pair in Map we create SVNWCAccess object - wc_access - which is actually a collection 
of directories being opened for commit (same as svn_wc_adm_access_t set in native SVN).
 
4. For each wc_access we collect items to commit, same way as for "normal" commit. Each item 
refers to its wc_access. Then we group all items by its repos_url and repos_uuid (fetching it from 
repos if not available).
 
5. So we have commit items grouped by repository root URL, each item may refer to its own 
wc_access (working copy). We call such a group a "commit packet".
 
7. Now we have a list of commit packets-one for each repository. Then we will commit each 
commit packet as a transaction.
 
8. During commit and in post commit code we use that wc_access references that is stored
 in each commit item to update corresponding working copy, write and execute log files 
and then finally to close all open directories.
 
3b) Suggested change for Subversion
 
I think that “One Commit, One Transaction” is the best(and this is also compatible with 
original system). So commit files from different repositories at a time and break them into 
several transactions may be not a good idea. And I suggest that we should constrain 
committing files in the same repository.
 
1. We receives list of paths to commit.
 
2. All paths are grouped by wc root path, so we get a map of wc_root:paths pairs - one for 
each working copy.
 
3. For each pair in Map we create svn_wc_adm_access_t- wc_access.
 
4. For each wc_access we collect items to commit. Each item refers to its wc_access. 
 
5. If there are more than one working copies, then we check all items by its repos_url 
and repos_uuid (fetching it from repos if not available). If they are not from the same repository, 
return with error, otherwise, combine them into one group.
 
6. Commit these items as a transaction.
 
7. During commit and in post commit code we use that wc_access references that is stored 
in each commit item to update corresponding working copy, write and execute log files and 
then finally to close all open directories.

HuiHuang
2009-05-27 



yellow.flying

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2354728

Re: design document

Posted by Stefan Sperling <st...@elego.de>.
On Wed, May 27, 2009 at 08:32:58AM +0800, HuiHuang wrote:
>    Hi,
>     
>    The following is design document for "commit from multiple woring copies".
>    If there is any problem, please tell me and I will modify it.

Thanks, that's very good!

I've committed it in r37850 as file
trunk/notes/commit-from-multiple-working-copies.txt.

To make changes to the file, please send patches for the file,
created with Subversion, from now on.
See http://subversion.tigris.org/hacking.html#patches

Also, note that I have refitted all lines in the file to fit into
80 characters. Please keep lines under 80 characters when you make
changes to the file.

I will try to point out what we could do next, see below.

>    3b) Suggested change for Subversion
> 
>    I think that !DEGOne Commit, One Transaction!+- is the best(and this is
>    also compatible with
>    original system). So commit files from different repositories at a time
>    and break them into
>    several transactions may be not a good idea. And I suggest that we should
>    constrain
>    committing files in the same repository.

Yes, constraining your project to one repository is a good thing.
Because it will make your task much easier. We can add support
for multiple target repositories later by building on top of the
work you are doing for this project.

>    1. We receives list of paths to commit.

Here, you could add which function in the public API of Subversion
receives this list first (from the perspective of the command line
client), and which functions in the public API of Subversion get to
see the list until it is being used.

>    2. All paths are grouped by wc root path, so we get a map of wc_root:paths
>    pairs - one for each working copy.

Here, you could explain what data type we use for the map, and where
we store the map, and give reasons why we want to store the map at
that particular place and not anywhere else.
 
>    3. For each pair in Map we create svn_wc_adm_access_t- wc_access.

Here, you could add information about where the map is created.

>    4. For each wc_access we collect items to commit. Each item refers to its
>    wc_access.

Here, you could add information about where and how the items
are collected, and stored into the map.

>    5. If there are more than one working copies, then we check all items by
>    its repos_url
>    and repos_uuid (fetching it from repos if not available). If they are not
>    from the same repository,
>    return with error, otherwise, combine them into one group.

Here, you could explain where and how the map is checked,
and which error is returned if the map does not pass the check.
     
>    6. Commit these items as a transaction.

Here, you could add information where and how the items get
committed to the server.
 
>    7. During commit and in post commit code we use that wc_access references
>    that is stored
>    in each commit item to update corresponding working copy, write and
>    execute log files and
>    then finally to close all open directories.

Again, we need to know where and how, for each step :)

Thanks!
Stefan

Re: [PATCH] commit_tests

Posted by HuiHuang <ye...@yahoo.com.cn>.
Hi Stefan,

	I am sorry. I know where I make a mistake:

	+# Commit multiple working copies which are [[not]] from the same repository. This
	+# is relate to issue #2381 
	
	There should be no "not" in the sentence. I think this is the reason which cause your
	misunderstanding. Do you think this test is necessary? If it is necessary, I will create
    the patch again, otherwise, I will change this test to commit_multiple_wc_multiple_repos
	as you indicated before.

	Thanks~

Huihuang

------------------				 
yellow.flying
2009-06-13


__________________________________________________
赶快注册雅虎超大容量免费邮箱?
http://cn.mail.yahoo.com

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361774

Re: [PATCH] commit_tests

Posted by HuiHuang <ye...@yahoo.com.cn>.
Thank you Neels, there is confusion and we make it clear now. Thanks for your help.

Best Wishes~

Huihuang

------------------				 
yellow.flying
2009-06-14

__________________________________________________
赶快注册雅虎超大容量免费邮箱?
http://cn.mail.yahoo.com

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361896

Re: [PATCH] commit_tests

Posted by Neels Janosch Hofmeyr <ne...@elego.de>.
Stefan Sperling wrote:
[...]
> +# Also related to issue #2381, "Cannot commit multiple WC paths which
> +# lack a common parent directory". Once that issue has been resolved,
> +# we will remove the XFail from this test.
[...]
> +  # Commit should succeed, even though one target is a "child" of the other,

Stefan, I think HuiHuang wants them *not* to be parent-child related. His
point is that he also wants to test the case where we commit two WC paths
which are adjacent, but whose parent dir is not a WC itself.

[...]
> Do you see the difference between doing the above and what you proposed?

Still, HuiHuang, Stefan is right here. The tests should expect the desired
behaviour, no matter if the current svn really behaves like that.

If the current svn behaves wrong, that test fails.
If we know that the test will currently fail, we mark it XFail(..)
As soon as the behaviour is fixed, we see the test pass.

Good speed for you, HuiHuang! :)

~Neels

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361885

Re: [PATCH] commit_tests

Posted by Stefan Sperling <st...@elego.de>.
On Sat, Jun 13, 2009 at 02:22:01PM +0100, Stefan Sperling wrote:
> In tests, we usually want the failure or success to come from
> an invocation of svntest.actions.run_and_verify_status().

Slight correction:

There are more "verify" functions than run_and_verify_status(),
of course. E.g. there is also run_and_verify_svn().
Any run_and_verify_*() function will do.

Stefan

Re: [PATCH] commit_tests

Posted by Stefan Sperling <st...@elego.de>.
On Sat, Jun 13, 2009 at 02:22:01PM +0100, Stefan Sperling wrote:
> So, to be clear, I meant something like:

> @@ -1384,14 +1387,16 @@
>    expected_status2.tweak('A/B/lambda', status='M ')
>    svntest.actions.run_and_verify_status(wc2_dir, expected_status2)
>  
> -  # Commit should fail, even though one target is a "child" of the other.
> -  svntest.actions.run_and_verify_svn("Unexpectedly not locked",
> -                                     None, svntest.verify.AnyOutput,
> +  # Commit should succeed, even though one target is a "child" of the other,
> +  # since both working copies come from the same repository.
> +  svntest.actions.run_and_verify_svn(None, None, svntest.verify.AnyOutput,
>                                       'commit', '-m', 'log',
>                                       wc_dir, wc2_dir)

This example was actually totally wrong.
The function call contradicts the comment above it,
and uses None instead of [].

So this led you to copy those errors into the patch you submitted.
I will try to be more careful when providing examples in the future.

I even ended up committing this wrong code in r38030 :(
What I really meant to say is in r38031.

I hope I didn't confuse you too much,
Stefan

Re: [PATCH] commit_tests

Posted by Stefan Sperling <st...@elego.de>.
On Sat, Jun 13, 2009 at 01:54:25PM +0800, HuiHuang wrote:
> Hey Stefan,
> >So instead of adding a new test, we can change the commit_multiple_wc
> >test to expect the commit to succeed instead of fail, and mark the
> >test XFail. Can you make a patch for that?
> 
> Do you mean that I make a patch like this?
> 
> Index: commit_tests.py
> ===================================================================
> --- commit_tests.py	(版本 38022)
> +++ commit_tests.py	(工作副本)
> @@ -1393,6 +1393,7 @@
>    # Verify status unchanged
>    svntest.actions.run_and_verify_status(wc_dir, expected_status)
>    svntest.actions.run_and_verify_status(wc2_dir, expected_status2)
> +  raise svntest.Failure

Raising Failure like that is almost never the right thing to do!

In tests, we usually want the failure or success to come from
an invocation of svntest.actions.run_and_verify_status().
Otherwise we are not testing Subversion. We are just writing
code which raises exceptions but doesn't test anything.

So, to be clear, I meant something like:

Index: subversion/tests/cmdline/commit_tests.py
===================================================================
--- subversion/tests/cmdline/commit_tests.py	(revision 38018)
+++ subversion/tests/cmdline/commit_tests.py	(working copy)
@@ -1353,8 +1353,11 @@
 
 # Commit from multiple working copies is not yet supported.  At
 # present an error is generated and none of the working copies change.
-# Related to issue 959, this test here doesn't use svn:externals but the
+# Related to issue #959, this test here doesn't use svn:externals but the
 # behaviour needs to be considered.
+# Also related to issue #2381, "Cannot commit multiple WC paths which
+# lack a common parent directory". Once that issue has been resolved,
+# we will remove the XFail from this test.
 
 def commit_multiple_wc(sbox):
   "attempted commit from multiple wc fails"
@@ -1384,14 +1387,16 @@
   expected_status2.tweak('A/B/lambda', status='M ')
   svntest.actions.run_and_verify_status(wc2_dir, expected_status2)
 
-  # Commit should fail, even though one target is a "child" of the other.
-  svntest.actions.run_and_verify_svn("Unexpectedly not locked",
-                                     None, svntest.verify.AnyOutput,
+  # Commit should succeed, even though one target is a "child" of the other,
+  # since both working copies come from the same repository.
+  svntest.actions.run_and_verify_svn(None, None, svntest.verify.AnyOutput,
                                      'commit', '-m', 'log',
                                      wc_dir, wc2_dir)
 
-  # Verify status unchanged
+  # Verify status changed
+  expected_status.tweak('A/mu', status='  ', wc_rev=2)
   svntest.actions.run_and_verify_status(wc_dir, expected_status)
+  expected_status2.tweak('A/B/lambda', status='  ', wc_rev=2)
   svntest.actions.run_and_verify_status(wc2_dir, expected_status2)
 
 
@@ -2696,7 +2701,7 @@
               commit_from_long_dir,
               commit_with_lock,
               commit_current_dir,
-              commit_multiple_wc,
+              XFail(commit_multiple_wc),
               commit_nonrecursive,
               failed_commit,
               commit_out_of_date_deletions,

Do you see the difference between doing the above and what you proposed?

Stefan


Re: [PATCH] commit_tests

Posted by Stefan Sperling <st...@elego.de>.
On Sat, Jun 13, 2009 at 10:34:06AM +0800, yellow.flying wrote:
> >Wait, there is something wrong in what we are doing!
> >It looks like I didn't give you good advice :(
> >After looking at commit_tests.py more closely, I realised that there
> >already is a test that does what we need!
> >It is the "commit_multiple_wc" test below which you put your test.
> >So instead of adding a new test, we can change the commit_multiple_wc
> >test to expect the commit to succeed instead of fail, and mark the
> >test XFail. Can you make a patch for that?
> 
> I think you misunderstand me. Yes, I know that there is a test already,
> but I do not think the test is enough, you may observed that in the 
> test the two wcs are nested, and I want test when the two wcs(which 
> are from the same repository) are not nested. So I write 
> commit_multiple_wc2. These two tests will both be used in our case. 
> 
> Do you think it is necessary to do like this?

OK, agreed.

Sorry about the confusion, I didn't look close enough to realise
the working copies were nested.

Maybe we should rename the first test to commit_multiple_wc_nested?

Stefan

Re: [PATCH] commit multiple wc

Posted by Stefan Sperling <st...@elego.de>.
On Sun, Jun 14, 2009 at 01:09:46PM +0100, Stefan Sperling wrote:
> On Sun, Jun 14, 2009 at 08:53:19AM +0800, yellow.flying wrote:
> > @@ -1384,17 +1388,72 @@
> >    expected_status2.tweak('A/B/lambda', status='M ')
> >    svntest.actions.run_and_verify_status(wc2_dir, expected_status2)
> >  
> > -  # Commit should fail, even though one target is a "child" of the other.
> > -  svntest.actions.run_and_verify_svn("Unexpectedly not locked",
> > -                                     None, svntest.verify.AnyOutput,
> > +  # Commit should fail, even though one target is a "child" of the other,
> > +  # since targets come from different wcs.
> > +  svntest.actions.run_and_verify_svn(None, None, svntest.verify.AnyOutput,
> >                                       'commit', '-m', 'log',
> >                                       wc_dir, wc2_dir)
> 
> In this hunk, you have changed the function to expect the commit
> to succeed, but you didn't update the comment above the function!

Hrmmm... I was too quick again... :(

You actually wrote the test to expect failure, so the comment
is correct. But we want the test to check for the situation we
will have when issue #2381 is solved. See r38030.

So, please forget my comments about not changing the comments :)

Sorry about that,
Stefan

Re: [PATCH] commit multiple wc

Posted by Stefan Sperling <st...@elego.de>.
On Sun, Jun 14, 2009 at 08:53:19AM +0800, yellow.flying wrote:
> Hey Stefan,
> 
> 	I am sorry. I can understand what you mean now. I rewrite the patch 
> commit_multiple_wc, and after this patch is submitted, I will  write another 
> patche-- commit_multiple_wc_multiple_repos.
> 
> log message:
>      
>     [[[
>       
>        * subversion/tests/cmdline/commit_tests.py
>          (commit_multiple_wc_nested): Rename commit_multiple_wc as 
>          commit_multiple_wc_nested, and mark it as XFail. It will be changed to Pass 
>          after we have fix issue #2381.
>          (commit_multiple_wc_not_nested): new test, This test is the same as 
>          commit_multiple_wc_nested except that two wcs in this test are not nested. 
>     ]]]
> 
> 	I hope this time it is ok,thank you.

It is mainly OK, except a few minor details. I have committed a patch
based on your patch in r38030, and also added another test.

Please carefully look at the patch I committed to make sure you
agree with it. If you see any problem please tell me!

Also please look at the log message carefully. For example,
your log message didn't mention the changes you made to test_list.
A good rule is to remember is that you need to mention *every* hunk
of the patch in your log message (a "hunk" starts with an @@ ... @@ line),
describing the change made by the hunk.

Some comments on your patch:

> Index: commit_tests.py
> ===================================================================
> --- commit_tests.py	(°æ±¾ 38025)
> +++ commit_tests.py	(¹¤×÷¸±±¾)

If you can, please create patches from the working copy root,
so that full path subversion/tests/cmdline/commit_tests.py
appears in the patch. But that is not very important.

> @@ -1384,17 +1388,72 @@
>    expected_status2.tweak('A/B/lambda', status='M ')
>    svntest.actions.run_and_verify_status(wc2_dir, expected_status2)
>  
> -  # Commit should fail, even though one target is a "child" of the other.
> -  svntest.actions.run_and_verify_svn("Unexpectedly not locked",
> -                                     None, svntest.verify.AnyOutput,
> +  # Commit should fail, even though one target is a "child" of the other,
> +  # since targets come from different wcs.
> +  svntest.actions.run_and_verify_svn(None, None, svntest.verify.AnyOutput,
>                                       'commit', '-m', 'log',
>                                       wc_dir, wc2_dir)

In this hunk, you have changed the function to expect the commit
to succeed, but you didn't update the comment above the function!
Please make sure to always keep comments in sync with code you are
changing. Otherwise, someone might look at the code and the comment
later and get confused.

Also, when you pass "None" for stdin or stderr to a run_and_verify_*
function, the function will throw an exception saying "stderr cannot
be None". This will make your test see to XFail for a different reason
than you want it to fail for. If you don't expect output, pass an empty
list [], not None. See my commit for examples.

In general, always make sure you understand why a test is failing.
This is not easy with XFAIL tests.

For example, in the commit_multiple_wc test, we expect Subversion
to complain about the parent of the working copies not being
a working copy. Running the test, you will see that this is what
really happens:

  [~/svn/svn-trunk/subversion/tests/cmdline] $ ./commit_tests.py 26
  subversion/svn/commit-cmd.c:137: (apr_err=155007)
  subversion/libsvn_client/commit.c:1522: (apr_err=155007)
  subversion/libsvn_wc/lock.c:593: (apr_err=155007)
  subversion/libsvn_wc/lock.c:484: (apr_err=155007)
  svn: '/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svn-test-work/working_copies/commit_tests-26' is not a working copy

Here you see svn complaining about the right thing.
If you see any different error, the test failed, but not for the correct
reason! 

  Traceback (most recent call last):
    File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/main.py", line 1141, in run
      rc = self.pred.run(sandbox)
    File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/testcase.py", line 61, in run
      return self._delegate.run(sandbox)
    File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/testcase.py", line 121, in run
      return self.func(sandbox)
    File "./commit_tests.py", line 1439, in commit_multiple_wc
      wc1_dir, wc2_dir)
    File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/actions.py", line 199, in run_and_verify_svn
      expected_exit, *varargs)
    File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/actions.py", line 232, in run_and_verify_svn2
      exit_code, out, err = main.run_svn(want_err, *varargs)
    File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/main.py", line 579, in run_svn
      *(_with_auth(_with_config_dir(varargs))))
    File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/main.py", line 357, in run_command
      None, *varargs)
    File "/home/stsp/svn/svn-trunk/subversion/tests/cmdline/svntest/main.py", line 512, in run_command_stdin
      raise Failure
  Failure
  XFAIL: commit_tests.py 26: commit from two working copies

So just looking at the last line does not give any information
when we are dealing with XFAIL tests. That makes it a bit hard
to work with them. It's much easier when tests are expected to PASS.

You can get even more detailed test output by passing the --verbose
option to the test script. This often helps to understand better
why a test is failing. Try running:

	commit_tests.py --verbose 25

> +  # Commit should fail, since targets come from different wcs.
> +  svntest.actions.run_and_verify_svn(None, None, svntest.verify.AnyOutput,
> +                                     'commit', '-m', 'log',
> +                                     wc1_dir, wc2_dir)

Here, you also forgot to change the comment.

> @@ -2696,7 +2755,8 @@
>                commit_from_long_dir,
>                commit_with_lock,
>                commit_current_dir,
> -              commit_multiple_wc,
> +              XFail(commit_multiple_wc_nested),
> +              XFail(commit_multiple_wc_not_nested),
>                commit_nonrecursive,
>                failed_commit,
>                commit_out_of_date_deletions,

The above change was not mentioned in your log message (I already
said this above).

Overall, the patch was good!
And I hope the tests will help you writing code that actually
solves issue #2381.

Thanks :)
Stefan


[PATCH] commit multiple wc

Posted by HuiHuang <ye...@yahoo.com.cn>.
Hey Stefan,

	I am sorry. I can understand what you mean now. I rewrite the patch 
commit_multiple_wc, and after this patch is submitted, I will  write another 
patche-- commit_multiple_wc_multiple_repos.

log message:
     
    [[[
      
       * subversion/tests/cmdline/commit_tests.py
         (commit_multiple_wc_nested): Rename commit_multiple_wc as 
         commit_multiple_wc_nested, and mark it as XFail. It will be changed to Pass 
         after we have fix issue #2381.
         (commit_multiple_wc_not_nested): new test, This test is the same as 
         commit_multiple_wc_nested except that two wcs in this test are not nested. 
    ]]]

	I hope this time it is ok,thank you.

Huihuang


------------------				 
yellow.flying
2009-06-14

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361895

Re: [PATCH] commit_tests

Posted by HuiHuang <ye...@yahoo.com.cn>.
>Wait, there is something wrong in what we are doing!
>It looks like I didn't give you good advice :(
>After looking at commit_tests.py more closely, I realised that there
>already is a test that does what we need!
>It is the "commit_multiple_wc" test below which you put your test.
>So instead of adding a new test, we can change the commit_multiple_wc
>test to expect the commit to succeed instead of fail, and mark the
>test XFail. Can you make a patch for that?

I think you misunderstand me. Yes, I know that there is a test already,
but I do not think the test is enough, you may observed that in the 
test the two wcs are nested, and I want test when the two wcs(which 
are from the same repository) are not nested. So I write 
commit_multiple_wc2. These two tests will both be used in our case. 

Do you think it is necessary to do like this?

Best Regards!
Huihuang


------------------				 
yellow.flying
2009-06-13

-------------------------------------------------------------

__________________________________________________
赶快注册雅虎超大容量免费邮箱?
http://cn.mail.yahoo.com

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361761

Re: [PATCH] commit_tests

Posted by HuiHuang <ye...@yahoo.com.cn>.
Hey Stefan,
>So instead of adding a new test, we can change the commit_multiple_wc
>test to expect the commit to succeed instead of fail, and mark the
>test XFail. Can you make a patch for that?

Do you mean that I make a patch like this?

Index: commit_tests.py
===================================================================
--- commit_tests.py	(版本 38022)
+++ commit_tests.py	(工作副本)
@@ -1393,6 +1393,7 @@
   # Verify status unchanged
   svntest.actions.run_and_verify_status(wc_dir, expected_status)
   svntest.actions.run_and_verify_status(wc2_dir, expected_status2)
+  raise svntest.Failure
 
 
 def commit_nonrecursive(sbox):
@@ -2696,7 +2697,7 @@
               commit_from_long_dir,
               commit_with_lock,
               commit_current_dir,
-              commit_multiple_wc,
+              XFail(commit_multiple_wc),
               commit_nonrecursive,
               failed_commit,
               commit_out_of_date_deletions,


Huihuang
------------------				 
yellow.flying
2009-06-13

-------------------------------------------------------------
��i��'�ē扫h∈&

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361773

Re: [PATCH] commit_tests

Posted by Stefan Sperling <st...@elego.de>.
On Fri, Jun 12, 2009 at 09:47:40AM +0800, HuiHuang wrote:
>    Hey Stefan,
>     
>    Yes, I agree with you. Actually the test is passed in current state.

Wait, there is something wrong in what we are doing!
It looks like I didn't give you good advice :(

After looking at commit_tests.py more closely, I realised that there
already is a test that does what we need!
It is the "commit_multiple_wc" test below which you put your test.

So instead of adding a new test, we can change the commit_multiple_wc
test to expect the commit to succeed instead of fail, and mark the
test XFail. Can you make a patch for that?

The test you wrote is still useful, however.
Your test tries to test the case where the two working copies do
not come from the same repository. That is OK, we will need this
test, too. You could add this test in another patch.

Note that when you wrote your test, you made both working copies
come from the same repository!

+  # Checkout two working copies
+  wc1_dir = os.path.join(wc_dir, 'wc1')
+  wc2_dir = os.path.join(wc_dir, 'wc2')
+  url = sbox.repo_url

Here, you define one repository URL.

+  svntest.actions.run_and_verify_svn("Output on stderr where none
expected",
+                                     svntest.verify.AnyOutput, [],
+                                     'checkout',
+                                     url, wc1_dir)

Here you use the URL to checkout WC1.

+  svntest.actions.run_and_verify_svn("Output on stderr where none
expected",
+                                     svntest.verify.AnyOutput, [],
+                                     'checkout',
+                                     url, wc2_dir)

And here use the same URL again to checkout WC2.
That doesn't look right to me.
Instead, we need two different repositories, with two different URLs,
and checkout each WC from a different URL.

I suggest you take the commit_multiple_wc test as it is now
(it expects failure), copy it to a new test called commit_multiple_wc2
or commit_multiple_wc_multiple_repos or something like that,
and change to use a separate repository for each working copy.
That is probably easier than writing the test from scratch.

There might be other tests using more than one repository,
I don't know. You could try to find examples for how to do this.
Or maybe if you read carefully through the code in
subversion/tests/cmdline/svntest/sandbox.py
you might be able to find out how this can be done.

Stefan

Re: [PATCH] commit_tests

Posted by HuiHuang <ye...@yahoo.com.cn>.
Hey Stefan,

Yes, I agree with you. Actually the test is passed in current state.
But it is not passed as we expect and I do not know how to distinguish
this unexpected pass from the expected pass in the future, so I raise a
failure exception and let it fails now.

Maybe I should remove the failure exception and XFail(), and it is 
passed now?

Thanks~

HuiHuang


2009-06-12 



yellow.flying 



发件人: Stefan Sperling 
发送时间: 2009-06-11  21:47:50 
收件人: HuiHuang 
抄送: dev 
主题: Re: [PATCH] commit_tests 
 
On Thu, Jun 11, 2009 at 07:40:08PM +0800, HuiHuang wrote:
>     Hi,
>         
>        log message:
>         
>        [[[
>          
>           * subversion/tests/cmdline/commit_tests.py
>    Add a test case for committing from multiple WC which have no common 
>    parent path. It is XFail now and will be changed to Pass after we have fix 
>    issue #2381.
>        ]]]
Looks good, expect that you wrote the test to expect the commit
to fail, and then raise a failure exception.
Instead, we could make the test expect the commit to succeed.
For now, the test will fail when trying to commit and raise
an exception at that point.
Then, all we need to change later is to remove the XFail().
Do you agree?
Stefan

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361490

Re: [PATCH] commit_tests

Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jun 11, 2009 at 07:40:08PM +0800, HuiHuang wrote:
>     Hi,
>         
>        log message:
>         
>        [[[
>          
>           * subversion/tests/cmdline/commit_tests.py
>    Add a test case for committing from multiple WC which have no common 
>    parent path. It is XFail now and will be changed to Pass after we have fix 
>    issue #2381.
>        ]]]

Looks good, expect that you wrote the test to expect the commit
to fail, and then raise a failure exception.

Instead, we could make the test expect the commit to succeed.
For now, the test will fail when trying to commit and raise
an exception at that point.

Then, all we need to change later is to remove the XFail().

Do you agree?

Stefan

RE: [PATCH] commit_tests

Posted by Bert Huijben <rh...@sharpsvn.net>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@xbc.nu]
> Sent: donderdag 11 juni 2009 17:54
> To: Bert Huijben
> Cc: 'HuiHuang'; dev@subversion.tigris.org
> Subject: Re: [PATCH] commit_tests
> 
> Bert Huijben wrote:
> >
> >             Hi,
> >
> >
> >
> > (Please send your messages to this list as plain text,
> >
> 
> Funny, why don't you do the same :)
> 
> > to allow answering inline. Thanks!).
> >
> 
> How's the format prevent that?

My mail clients put a vertical HTML line before the existing text. This way, if I convert the mail to plain html I don't get the common '>' indentations. And if I would just type my response inline (without hand-editing the entire mail) you wouldn't see what I added.

	Bert

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361530


Re: [PATCH] commit_tests

Posted by Branko Cibej <br...@xbc.nu>.
Bert Huijben wrote:
>
>             Hi,
>
>  
>
> (Please send your messages to this list as plain text,
>

Funny, why don't you do the same :)

> to allow answering inline. Thanks!).
>

How's the format prevent that?

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361340

RE: [PATCH] commit_tests

Posted by Bert Huijben <rh...@sharpsvn.net>.
Hi,

 

(Please send your messages to this list as plain text, to allow answering
inline. Thanks!).

 

This test fails with the current implementation, and with the future
implementation.

 

Our tests should either check the current behavior (and just pass now), or
test for the future behavior and fail somewhere before reaching that
(XFail).

 

In its current state it passes all the way through the raise svntest.Failure
at the end and will fail earlier when the behavior is fixed.  This doesn't
help in diagnosing errors/changes, because the error state will never
change: It will always fail.

 

            Bert

 

From: HuiHuang [mailto:yellow.flying@yahoo.com.cn] 
Sent: donderdag 11 juni 2009 13:40
To: Stefan Sperling
Cc: dev
Subject: [PATCH] commit_tests

 

 Hi, 

     

    log message:

     

    [[[

      

       * subversion/tests/cmdline/commit_tests.py 

Add a test case for committing from multiple WC which have no common 

parent path. It is XFail now and will be changed to Pass after we have fix 

issue #2381.

    ]]]

 

    Huihuang

 

 

2009-06-11 

  _____  

yellow.flying 

  _____

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361266

[PATCH] commit_tests

Posted by HuiHuang <ye...@yahoo.com.cn>.
Hi,
     
    log message:
     
    [[[
      
       * subversion/tests/cmdline/commit_tests.py 
Add a test case for committing from multiple WC which have no common 
parent path. It is XFail now and will be changed to Pass after we have fix 
issue #2381.
    ]]]

    Huihuang


2009-06-11 



yellow.flying

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2361248

Re: design document

Posted by Stefan Sperling <st...@elego.de>.
On Fri, May 29, 2009 at 08:40:50AM -0500, Hyrum K. Wright wrote:
> <sidebar>
> Access batons are slowly disappearing from internal use in the working  
> copy library, and will hopefully go extinct in the client library as  
> well.  In the future, we will just use a single svn_wc_context_t,  
> instead of carrying around a collection of access batons.  I don't know 
> how this will impact the problem at hand, but it's good to keep in mind.
>
> Because this work is pretty tightly coupled to the working copy, I'd  
> encourage whoever is coding and reviewing it to closely follow any  
> public API changes made to libsvn_wc.
> </sidebar>

Hyrum,

thanks for pointing this out.

If you can, please also review the design and patches as they
are developed. You and Greg are pretty much the only people we
have with a clue about wc-ng right now, and having your comments
on this matter would greatly help to increase this number from its
current value of 2 to something more sane.

Thanks,
Stefan

Re: design document

Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On May 29, 2009, at 8:31 AM, C. Michael Pilato wrote:

> yellow.flying wrote:
>> **>I wasn't anticipating the change you seem to be proposing, where  
>> the
>>> committables are grouped by working copy.
>>> My redesign of the commit process long ago assumes no need to group
>>> committables by working copy, only by repository.  Commits are  
>>> driven based
>>> on the committable's URL today -- *not* based on its working copy  
>>> path.
>>
>> I see that the committing files are still grouped by working copy  
>> in native
>> implement of commit now, but it can be extend to group by  
>> repository.  So
>> if "committables" you design can deal with the later situation, I can
>> reuse it.
>>
>> you say "no need to group committables by working copy, only by  
>> repository",
>> do you mean that "base_dir_access" in the following function is not
>> necessary
>> a working copy access baton but a access baton to the base  
>> directory of
>> several working copies from the same repository?
>
> I had forgotten about the access baton situation.  (Actually, I  
> think my
> code was written before we had access batons ... it was the addition  
> of the
> access baton paradigm that made this all stop working, if I recall  
> correctly.)
>
> Could this be as simple as adding a pointer to a working copy access  
> baton
> to the committable structure, plus a master array of top-level  
> working copy
> access batons (for post-commit releasing)?

<sidebar>
Access batons are slowly disappearing from internal use in the working  
copy library, and will hopefully go extinct in the client library as  
well.  In the future, we will just use a single svn_wc_context_t,  
instead of carrying around a collection of access batons.  I don't  
know how this will impact the problem at hand, but it's good to keep  
in mind.

Because this work is pretty tightly coupled to the working copy, I'd  
encourage whoever is coding and reviewing it to closely follow any  
public API changes made to libsvn_wc.
</sidebar>

-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2356778

Re: design document

Posted by "C. Michael Pilato" <cm...@collab.net>.
yellow.flying wrote:
> **>I wasn't anticipating the change you seem to be proposing, where the
>>committables are grouped by working copy.
>>My redesign of the commit process long ago assumes no need to group
>>committables by working copy, only by repository.  Commits are driven based
>>on the committable's URL today -- *not* based on its working copy path.
>  
> I see that the committing files are still grouped by working copy in native
> implement of commit now, but it can be extend to group by repository.  So
> if "committables" you design can deal with the later situation, I can
> reuse it. 
>  
> you say "no need to group committables by working copy, only by repository",
> do you mean that "base_dir_access" in the following function is not
> necessary
> a working copy access baton but a access baton to the base directory of
> several working copies from the same repository?

I had forgotten about the access baton situation.  (Actually, I think my
code was written before we had access batons ... it was the addition of the
access baton paradigm that made this all stop working, if I recall correctly.)

Could this be as simple as adding a pointer to a working copy access baton
to the committable structure, plus a master array of top-level working copy
access batons (for post-commit releasing)?

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2356774

Re: design document

Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On May 30, 2009, at 5:45 AM, HuiHuang wrote:

>
> >Could this be as simple as adding a pointer to a working copy  
> access baton
> >to the committable structure, plus a master array of top-level  
> working copy
> >access batons (for post-commit releasing)?
>
> This is what I think, too. But Hyrum say that we will just use a  
> single
> svn_wc_context_t instead of carrying around a collection of access  
> batons
> in the future. So I had better to konw svn_wc_context_t first and then
> think about how to use it in my work.

I'd stay with the status quo for right now, with the expectation that  
things will change over the course of the next few months.  If you  
introduce *new* wc APIs, which I don't think will be needed, then they  
should use svn_wc_context_t.

>
> To Hyrum,
>
> Thank you very much. Here I have a question. I find out definitions of
> svn_wc_adm_access_t and svn_wc_context_t as following, obviously there
> are more information in svn_wc_adm_access_t than in svn_wc_context_t.
> So how can svn_wc_context_t replace svn_wc_adm_access_t?

svn_wc_context_t is an opaque structure which contains a pointer to an  
svn_wc__db_t.  It's this database handle which contains the real  
information used to interface with the working copy metadata.  The  
database will also handle locking, concurrency, and multiple directory  
opening, only instead of keeping track of a collection of them, we  
only need one.  This is both easier, but it will also help as we move  
toward a one-db-per-working copy paradigm.

> By the way, would you mind to tell me where can I find public API  
> changes
> made to libsvn_wc? Thanks~

'svn log subversion/include/svn_wc.h' :)

By the way, I'm happy to answer questions, as are a number of other  
people, but sometimes you can get the information you are looking for  
using a judicious combination of 'svn log' and 'svn praise'.   
'praise' (or 'blame', if you prefer) gives line-by-line change  
history, and our log messages usually contain a good rationale for the  
accompanying change.  Using these tools, you can mine the repository,  
which is often much better than mining developers' heads.

> //In libsvn_wc/lock.c
> struct svn_wc_adm_access_t
> {
>   /* PATH to directory which contains the administrative area */
>   const char *path;
>   /* And the absolute form of the path.  */
>   const char *abspath;
>   enum svn_wc__adm_access_type {
>     /* SVN_WC__ADM_ACCESS_UNLOCKED indicates no lock is held allowing
>        read-only access */
>     svn_wc__adm_access_unlocked,
>     /* SVN_WC__ADM_ACCESS_WRITE_LOCK indicates that a write lock is  
> held
>        allowing read-write access */
>     svn_wc__adm_access_write_lock,
>     /* SVN_WC__ADM_ACCESS_CLOSED indicates that the baton has been
>        closed. */
>     svn_wc__adm_access_closed
>   } type;
>   /* LOCK_EXISTS is set TRUE when the write lock exists */
>   svn_boolean_t lock_exists;
>   /* Handle to the administrative database. */
>   svn_wc__db_t *db;
>   /* Was the DB provided to us? If so, then we'll never close it.  */
>   svn_boolean_t db_provided;
>   /* ENTRIES_HIDDEN is all cached entries including those in
>      state deleted or state absent. It may be NULL. */
>   apr_hash_t *entries_all;
>   /* POOL is used to allocate cached items, they need to persist for  
> the
>      lifetime of this access baton */
>   apr_pool_t *pool;
> };
>
>
> //In libsvn_wc/wc.h
> /*** Context handling ***/
> struct svn_wc_context_t
> {
>   /* The wc_db handle for this working copy. */
>   svn_wc__db_t *db;
>   /* The state pool for this context. */
>   apr_pool_t *state_pool;
> };
>
> Thanks all!
>
> Huihuang
>
>
> 2009-05-30
> yellow.flying
>
>
>

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2357120

Re: design document

Posted by HuiHuang <ye...@yahoo.com.cn>.
>Could this be as simple as adding a pointer to a working copy access baton
>to the committable structure, plus a master array of top-level working copy
>access batons (for post-commit releasing)?

This is what I think, too. But Hyrum say that we will just use a single 
svn_wc_context_t instead of carrying around a collection of access batons
in the future. So I had better to konw svn_wc_context_t first and then 
think about how to use it in my work.

To Hyrum,

Thank you very much. Here I have a question. I find out definitions of 
svn_wc_adm_access_t and svn_wc_context_t as following, obviously there
are more information in svn_wc_adm_access_t than in svn_wc_context_t.
So how can svn_wc_context_t replace svn_wc_adm_access_t? 

By the way, would you mind to tell me where can I find public API changes 
made to libsvn_wc? Thanks~ 

//In libsvn_wc/lock.c
struct svn_wc_adm_access_t
{
  /* PATH to directory which contains the administrative area */
  const char *path;
  /* And the absolute form of the path.  */
  const char *abspath;
  enum svn_wc__adm_access_type {
    /* SVN_WC__ADM_ACCESS_UNLOCKED indicates no lock is held allowing
       read-only access */
    svn_wc__adm_access_unlocked,
    /* SVN_WC__ADM_ACCESS_WRITE_LOCK indicates that a write lock is held
       allowing read-write access */
    svn_wc__adm_access_write_lock,
    /* SVN_WC__ADM_ACCESS_CLOSED indicates that the baton has been
       closed. */
    svn_wc__adm_access_closed
  } type;
  /* LOCK_EXISTS is set TRUE when the write lock exists */
  svn_boolean_t lock_exists;
  /* Handle to the administrative database. */
  svn_wc__db_t *db;
  /* Was the DB provided to us? If so, then we'll never close it.  */
  svn_boolean_t db_provided;
  /* ENTRIES_HIDDEN is all cached entries including those in
     state deleted or state absent. It may be NULL. */
  apr_hash_t *entries_all;
  /* POOL is used to allocate cached items, they need to persist for the
     lifetime of this access baton */
  apr_pool_t *pool;
};


//In libsvn_wc/wc.h
/*** Context handling ***/
struct svn_wc_context_t
{
  /* The wc_db handle for this working copy. */
  svn_wc__db_t *db;
  /* The state pool for this context. */
  apr_pool_t *state_pool;
};

Thanks all!

Huihuang


2009-05-30 



yellow.flying

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2357090

Re: design document

Posted by HuiHuang <ye...@yahoo.com.cn>.
>I wasn't anticipating the change you seem to be proposing, where the
>committables are grouped by working copy.
>My redesign of the commit process long ago assumes no need to group
>committables by working copy, only by repository.  Commits are driven based
>on the committable's URL today -- *not* based on its working copy path.

I see that the committing files are still grouped by working copy in native 
implement of commit now, but it can be extend to group by repository.  So 
if "committables" you design can deal with the later situation, I can reuse it. 

you say "no need to group committables by working copy, only by repository",
do you mean that "base_dir_access" in the following function is not necessary 
a working copy access baton but a access baton to the base directory of 
several working copies from the same repository?

cmt_err = svn_client__harvest_committables(&committables,
                                                  &lock_tokens,
                                                  base_dir_access,
                                                  rel_targets,
                                                  depth,
                                                  ! keep_locks,
                                                  changelists,
                                                  ctx,
                                                  pool)

>That's what allows us to theoretically get atomicity in a commit that spans
>multiple working copies which point to the same repository.
>Again, the code may be so stale and so tweaked by now that the design I had
>in mind is now useless.  And I'm certainly not tied to those old ideas.  I
>just don't want to see unnecessary effort invested if we can avoid it.

Yes, I see there is plenty of code I can reuse.

Thanks


2009-05-29 



yellow.flying

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2356652

weekly report for 2009-06-01~2009-06-06

Posted by HuiHuang <ye...@yahoo.com.cn>.
Hey Stefan,

This week:
1. build the whole project successfully.
2. write a patch to commit-from-multiple-working-copies.txt 

next week:
1. complete design document.
2. try to write some code.

thanks~ Good Luck!

Huihuang


2009-06-07 



yellow.flying

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2360081

Re: weekly report for 2009-05-23~2009-05-29

Posted by Stefan Sperling <st...@elego.de>.
On Sat, May 30, 2009 at 07:17:15PM +0800, HuiHuang wrote:
>    Hey Stefan,
>     
>    This week:
>     
>    1. check out SVNKit's code and read its implement of commit.
>    2. read svn native implement.
>    3. write design document.
>     
>    next week:
>     
>    1. discuss and complete design document.
>    2. try to build the whole project.
>     
>    Thank you very much for your help these days.
>    Good Luck. 

Thank you, too :)

Note that I'll be travelling for a few days now, so I might not
always be able to respond quickly during next week.

Have fun!
Stefan

Re: design document

Posted by Stefan Sperling <st...@elego.de>.
On Thu, May 28, 2009 at 10:23:17AM -0400, C. Michael Pilato wrote:
> Stefan Sperling wrote:
> > Did I understand you correctly?
> 
> I wasn't anticipating the change you seem to be proposing, where the
> committables are grouped by working copy.
> 
> My redesign of the commit process long ago assumes no need to group
> committables by working copy, only by repository.  Commits are driven based
> on the committable's URL today -- *not* based on its working copy path.
> That's what allows us to theoretically get atomicity in a commit that spans
> multiple working copies which point to the same repository.

Right. I was referring to the notion that we'll need multiple access
batons, one for each working copy, on the libsvn_wc side (see Hui Huangs
design proposal which this thread is based on). Of course, the commit
itself will still be keyed by repository.

> Again, the code may be so stale and so tweaked by now that the design I had
> in mind is now useless.  And I'm certainly not tied to those old ideas.  I
> just don't want to see unnecessary effort invested if we can avoid it.

OK, but we are really drifting into issue #1167 here.
We may want to amend that issue with a pointer to this thread.

Stefan

weekly report for 2009-05-23~2009-05-29

Posted by HuiHuang <ye...@yahoo.com.cn>.
Hey Stefan,

This week:

1. check out SVNKit's code and read its implement of commit. 
2. read svn native implement.
3. write design document.

next week:

1. discuss and complete design document.
2. try to build the whole project.

Thank you very much for your help these days. 
Good Luck. 

Huihuang
2009-05-30 



yellow.flying

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2357095

Re: design document

Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> On Thu, May 28, 2009 at 08:58:23AM -0400, C. Michael Pilato wrote:
>> When I rewrote the commit driving code a long time ago, I anticipated the
>> need to handle commits to multiple repositories.  The code may have gone a
>> bit stale, but the logic that harvests "committables" stores those items in
>> a hash that was designed to be primarily keyed on some unique repository
>> attribute (UUID, repos URL, or something).  Of course, I think this was back
>> before we stored such things in our working copy, so I used a single static
>> key for that hash for the time being.  But I still hold some hope that that
>> code can be revived and massaged into doing what is expected.
> 
> So you're saying that there already are provisions in the existing
> code for the "commit to multiple repositories" problem which are
> orthogonal to what Hui Huang is doing?
> 
> I mean, according to your description, the current data structure
> hierarchy looks somewhat like this:
> 
>  +-----------------+
>  | repo hash table | <-- keyed statically right now,
>  +-----------------+     so it only has one entry
>    |
>    v
> (commitables) <-- list of commitables (or a hash table or whatever)
> 
> When we start storing commitables for multiple working copies,
> we will still have commitables grouped per repository anyway.
> So we'll change the above to something like:
> 
>  +-----------------+
>  | repo hash table | <-- still keyed statically
>  +-----------------+
>    | 
>    v
> (commitables WC1, commitables WC2, ..., commitables WCn)
> 
> Extending the commit mechanism to use multiple keys into the
> per-repository hash instead of a static one is something which
> can still be done later. It even could be done right now before
> Hui Huang works on the code, because it's "one level above"
> of what he is doing.
> 
> Did I understand you correctly?

I wasn't anticipating the change you seem to be proposing, where the
committables are grouped by working copy.

My redesign of the commit process long ago assumes no need to group
committables by working copy, only by repository.  Commits are driven based
on the committable's URL today -- *not* based on its working copy path.
That's what allows us to theoretically get atomicity in a commit that spans
multiple working copies which point to the same repository.

Again, the code may be so stale and so tweaked by now that the design I had
in mind is now useless.  And I'm certainly not tied to those old ideas.  I
just don't want to see unnecessary effort invested if we can avoid it.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2356178

Re: design document

Posted by Stefan Sperling <st...@elego.de>.
On Thu, May 28, 2009 at 08:58:23AM -0400, C. Michael Pilato wrote:
> When I rewrote the commit driving code a long time ago, I anticipated the
> need to handle commits to multiple repositories.  The code may have gone a
> bit stale, but the logic that harvests "committables" stores those items in
> a hash that was designed to be primarily keyed on some unique repository
> attribute (UUID, repos URL, or something).  Of course, I think this was back
> before we stored such things in our working copy, so I used a single static
> key for that hash for the time being.  But I still hold some hope that that
> code can be revived and massaged into doing what is expected.

So you're saying that there already are provisions in the existing
code for the "commit to multiple repositories" problem which are
orthogonal to what Hui Huang is doing?

I mean, according to your description, the current data structure
hierarchy looks somewhat like this:

 +-----------------+
 | repo hash table | <-- keyed statically right now,
 +-----------------+     so it only has one entry
   |
   v
(commitables) <-- list of commitables (or a hash table or whatever)

When we start storing commitables for multiple working copies,
we will still have commitables grouped per repository anyway.
So we'll change the above to something like:

 +-----------------+
 | repo hash table | <-- still keyed statically
 +-----------------+
   | 
   v
(commitables WC1, commitables WC2, ..., commitables WCn)

Extending the commit mechanism to use multiple keys into the
per-repository hash instead of a static one is something which
can still be done later. It even could be done right now before
Hui Huang works on the code, because it's "one level above"
of what he is doing.

Did I understand you correctly?

Stefan

Re: design document

Posted by "C. Michael Pilato" <cm...@collab.net>.
Stefan Sperling wrote:
> On Thu, May 28, 2009 at 09:46:14AM +0200, Branko Cibej wrote:
>> Stefan Sperling wrote:
>>> On Wed, May 27, 2009 at 03:21:40PM +0200, Branko Cibej wrote:
>>>> But why restrict to a single repsitory? I agree that one transaction per
>>>> repository makes sense; however, I see no reason to not launch several
>>>> commit transactions within one svn_client_commit. By the way, this would
>>>> be an elegant solution for
>>>> http://subversion.tigris.org/issues/show_bug.cgi?id=1167
>>>>     
>>> Let's just go one step at a time, and focus on the "multiple working
>>> copies" issue first. Once that is solved, we can easily extend it
>>> to "multiple repositories".
>>>   
>> I agree on the one-step-at-a-time approach, but I'm not sure about the
>> "easily" unless the current design at least takes the next step into
>> account.
> 
> Then please invest the time to suggest how the design can be made to
> take the next step into account without major effort.
> 
> The focus right now is #2381, and not #1167. Making #1167 an extra
> requirement creates extra work for Hui Huang. Having to solve an extra
> problem that isn't on the charter of his gsoc project is not what gsoc
> is about.
> 
> I am sure that it is possible to solve both issues eventually,
> no matter what we do now. We can still bend the existing design later
> if we find that it is insufficent to also satisfy #1167. But if you
> have a clever idea that would already help #1167 a little, then please
> tell it to us so we can discuss whether it's worth doing it.
> But it must not create another huge pile of work, because there already
> is a huge pile of work in front of Hui Huang.

When I rewrote the commit driving code a long time ago, I anticipated the
need to handle commits to multiple repositories.  The code may have gone a
bit stale, but the logic that harvests "committables" stores those items in
a hash that was designed to be primarily keyed on some unique repository
attribute (UUID, repos URL, or something).  Of course, I think this was back
before we stored such things in our working copy, so I used a single static
key for that hash for the time being.  But I still hold some hope that that
code can be revived and massaged into doing what is expected.

-- 
C. Michael Pilato <cm...@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2356125

Re: design document

Posted by Stefan Sperling <st...@elego.de>.
On Thu, May 28, 2009 at 09:46:14AM +0200, Branko Cibej wrote:
> Stefan Sperling wrote:
> > On Wed, May 27, 2009 at 03:21:40PM +0200, Branko Cibej wrote:
> >> But why restrict to a single repsitory? I agree that one transaction per
> >> repository makes sense; however, I see no reason to not launch several
> >> commit transactions within one svn_client_commit. By the way, this would
> >> be an elegant solution for
> >> http://subversion.tigris.org/issues/show_bug.cgi?id=1167
> >>     
> >
> > Let's just go one step at a time, and focus on the "multiple working
> > copies" issue first. Once that is solved, we can easily extend it
> > to "multiple repositories".
> >   
> 
> I agree on the one-step-at-a-time approach, but I'm not sure about the
> "easily" unless the current design at least takes the next step into
> account.

Then please invest the time to suggest how the design can be made to
take the next step into account without major effort.

The focus right now is #2381, and not #1167. Making #1167 an extra
requirement creates extra work for Hui Huang. Having to solve an extra
problem that isn't on the charter of his gsoc project is not what gsoc
is about.

I am sure that it is possible to solve both issues eventually,
no matter what we do now. We can still bend the existing design later
if we find that it is insufficent to also satisfy #1167. But if you
have a clever idea that would already help #1167 a little, then please
tell it to us so we can discuss whether it's worth doing it.
But it must not create another huge pile of work, because there already
is a huge pile of work in front of Hui Huang.

Stefan

Re: design document

Posted by Branko Cibej <br...@xbc.nu>.
Stefan Sperling wrote:
> On Wed, May 27, 2009 at 03:21:40PM +0200, Branko Cibej wrote:
>   
>> HuiHuang wrote:
>>     
>>> 3b) Suggested change for Subversion
>>>
>>>  
>>>
>>> I think that “One Commit, One Transaction” is the best(and this is
>>> also compatible with
>>>
>>> original system). So commit files from different repositories at a
>>> time and break them into
>>>
>>> several transactions may be not a good idea. And I suggest that we
>>> should constrain
>>>
>>> committing files in the same repository.
>>>
>>>       
>> But why restrict to a single repsitory? I agree that one transaction per
>> repository makes sense; however, I see no reason to not launch several
>> commit transactions within one svn_client_commit. By the way, this would
>> be an elegant solution for
>> http://subversion.tigris.org/issues/show_bug.cgi?id=1167
>>     
>
> Let's just go one step at a time, and focus on the "multiple working
> copies" issue first. Once that is solved, we can easily extend it
> to "multiple repositories".
>   

I agree on the one-step-at-a-time approach, but I'm not sure about the
"easily" unless the current design at least takes the next step into
account.

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2356064


Re: design document

Posted by Stefan Sperling <st...@elego.de>.
On Wed, May 27, 2009 at 03:21:40PM +0200, Branko Cibej wrote:
> HuiHuang wrote:
> >
> > 3b) Suggested change for Subversion
> >
> >  
> >
> > I think that “One Commit, One Transaction” is the best(and this is
> > also compatible with
> >
> > original system). So commit files from different repositories at a
> > time and break them into
> >
> > several transactions may be not a good idea. And I suggest that we
> > should constrain
> >
> > committing files in the same repository.
> >
> 
> But why restrict to a single repsitory? I agree that one transaction per
> repository makes sense; however, I see no reason to not launch several
> commit transactions within one svn_client_commit. By the way, this would
> be an elegant solution for
> http://subversion.tigris.org/issues/show_bug.cgi?id=1167

Let's just go one step at a time, and focus on the "multiple working
copies" issue first. Once that is solved, we can easily extend it
to "multiple repositories".

Stefan


Re: design document

Posted by Branko Cibej <br...@xbc.nu>.
HuiHuang wrote:
>
> 3b) Suggested change for Subversion
>
>  
>
> I think that “One Commit, One Transaction” is the best(and this is
> also compatible with
>
> original system). So commit files from different repositories at a
> time and break them into
>
> several transactions may be not a good idea. And I suggest that we
> should constrain
>
> committing files in the same repository.
>

But why restrict to a single repsitory? I agree that one transaction per
repository makes sense; however, I see no reason to not launch several
commit transactions within one svn_client_commit. By the way, this would
be an elegant solution for
http://subversion.tigris.org/issues/show_bug.cgi?id=1167

-- Brane

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2355772