You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Garrett Rooney <ro...@electricjellyfish.net> on 2004/04/10 15:56:02 UTC
Re: svn commit: r9312 - trunk/subversion/libsvn_fs_fs
On Apr 10, 2004, at 10:17 AM, jpieper@tigris.org wrote:
> * subversion/libsvn_fs_fs/fs_fs.c
> (svn_fs__fs_open, svn_fs__fs_youngest_revision,
> svn_fs__fs_get_node_revision, svn_fs__fs_get_proplist,
> svn_fs__fs_rep_contents_dir, svn_fs__fs_rev_get_root,
> svn_fs__fs_revision_proplist): Impelemented at least partially
> and removed abort's on the control paths so implemented.
> (svn_fs__fs_file_length, svn_fs__fs_noderev_same_prop_key,
> svn_fs__fs_noderev_same_data_key): New
> (read_header_block, open_and_seek_revision, read_rep_line,
> get_fs_id_at_offset, get_root_offset): New helper routines that
> assist the above newly implemented functions.
I hate to be nitpicky, but the formatting in this file is pretty
random. There are a bunch of places with weird [*] if/else blocks,
some overly long lines, some sections that are indented too far, and
the 'space before the paren for function calls' seems to disappear and
reappear at random.
I've got a patch for all of this except the space before the paren
stuff, since I didn't know which direction this code was going to go on
that issue, but I'll hold off on committing it until after I hear from
you, since I'd hate to conflict with any more work you've been doing
here.
Also, how are you creating test cases for this? Manually? And from
what I can see it's just opening up the 'current' revision number. Is
that intended to exist forever, or is it temporary? I'm just curious
how we're going to atomically replace that file when commits occur...
Will it be a link of some sort?
Other than that, great stuff! It's nice to see some of the ideas for
libsvn_fs_fs solidifying into code.
-garrett
[*] something like:
if (foo)
{
blah ();
} else {
somethingelse ();
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9312 - trunk/subversion/libsvn_fs_fs
Posted by kf...@collab.net.
Josh Pieper <jp...@andrew.cmu.edu> writes:
> Sorry about that. Subversion's coding style is radically different
> from what I am used to, so I am trying to break myself of those habits
> and check my work carefully, but obviously not carefully enough. Is
> there some script I can use to check for obvious things before
> checking in?
There's no script (that I know of), but there are editor settings
available. See
tools/dev/svn-dev.vim
tools/dev/svn-dev.el
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9312 - trunk/subversion/libsvn_fs_fs
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On Apr 10, 2004, at 2:22 PM, Josh Pieper wrote:
>>> * subversion/libsvn_fs_fs/fs_fs.c
>>> (svn_fs__fs_open, svn_fs__fs_youngest_revision,
>>> svn_fs__fs_get_node_revision, svn_fs__fs_get_proplist,
>>> svn_fs__fs_rep_contents_dir, svn_fs__fs_rev_get_root,
>>> svn_fs__fs_revision_proplist): Impelemented at least partially
>>> and removed abort's on the control paths so implemented.
>>> (svn_fs__fs_file_length, svn_fs__fs_noderev_same_prop_key,
>>> svn_fs__fs_noderev_same_data_key): New
>>> (read_header_block, open_and_seek_revision, read_rep_line,
>>> get_fs_id_at_offset, get_root_offset): New helper routines that
>>> assist the above newly implemented functions.
>>
>> I hate to be nitpicky, but the formatting in this file is pretty
>> random. There are a bunch of places with weird [*] if/else blocks,
>> some overly long lines, some sections that are indented too far, and
>> the 'space before the paren for function calls' seems to disappear and
>> reappear at random.
>
> Sorry about that. Subversion's coding style is radically different
> from what I am used to, so I am trying to break myself of those habits
> and check my work carefully, but obviously not carefully enough. Is
> there some script I can use to check for obvious things before
> checking in?
I don't know of any automated check, other than eventually having
enough people point out problems in patches to the point where you
automatically notice them on your own. That's what happened with me
anyway ;-)
>> I've got a patch for all of this except the space before the paren
>> stuff, since I didn't know which direction this code was going to go
>> on
>> that issue, but I'll hold off on committing it until after I hear from
>> you, since I'd hate to conflict with any more work you've been doing
>> here.
>
> Go ahead and check it in, I have done no more than what's in the
> repository now.
Cool. I'll check in my cleanups once I confirm they compile.
>> Also, how are you creating test cases for this? Manually? And from
>> what I can see it's just opening up the 'current' revision number. Is
>> that intended to exist forever, or is it temporary? I'm just curious
>> how we're going to atomically replace that file when commits occur...
>> Will it be a link of some sort?
>
> The one test case I have now is hand-made, however ghudson is working
> on a program to generate the test cases from dump files, work is just
> progressing slowly.
Cool.
> As to the 'current' file, the plan is to acquire a repository wide
> lock when updating it and when writing new revision files. It could
> be a symlink if platform portability wasn't a concern.
Even with a symlink I think we'd need some kind of locking, since I
don't think there's a way to automatically repoint a symlink in one
atomic operation. At least the last time I tried to do so I couldn't
seem to find a way to do it. If there is, I'd love to know about it
though...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9312 - trunk/subversion/libsvn_fs_fs
Posted by Josh Pieper <jp...@andrew.cmu.edu>.
> > * subversion/libsvn_fs_fs/fs_fs.c
> > (svn_fs__fs_open, svn_fs__fs_youngest_revision,
> > svn_fs__fs_get_node_revision, svn_fs__fs_get_proplist,
> > svn_fs__fs_rep_contents_dir, svn_fs__fs_rev_get_root,
> > svn_fs__fs_revision_proplist): Impelemented at least partially
> > and removed abort's on the control paths so implemented.
> > (svn_fs__fs_file_length, svn_fs__fs_noderev_same_prop_key,
> > svn_fs__fs_noderev_same_data_key): New
> > (read_header_block, open_and_seek_revision, read_rep_line,
> > get_fs_id_at_offset, get_root_offset): New helper routines that
> > assist the above newly implemented functions.
>
> I hate to be nitpicky, but the formatting in this file is pretty
> random. There are a bunch of places with weird [*] if/else blocks,
> some overly long lines, some sections that are indented too far, and
> the 'space before the paren for function calls' seems to disappear and
> reappear at random.
Sorry about that. Subversion's coding style is radically different
from what I am used to, so I am trying to break myself of those habits
and check my work carefully, but obviously not carefully enough. Is
there some script I can use to check for obvious things before
checking in?
> I've got a patch for all of this except the space before the paren
> stuff, since I didn't know which direction this code was going to go on
> that issue, but I'll hold off on committing it until after I hear from
> you, since I'd hate to conflict with any more work you've been doing
> here.
Go ahead and check it in, I have done no more than what's in the
repository now.
> Also, how are you creating test cases for this? Manually? And from
> what I can see it's just opening up the 'current' revision number. Is
> that intended to exist forever, or is it temporary? I'm just curious
> how we're going to atomically replace that file when commits occur...
> Will it be a link of some sort?
The one test case I have now is hand-made, however ghudson is working
on a program to generate the test cases from dump files, work is just
progressing slowly.
As to the 'current' file, the plan is to acquire a repository wide
lock when updating it and when writing new revision files. It could
be a symlink if platform portability wasn't a concern.
-Josh
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org