Login | Register
My pages Projects Community openCollabNet

Discussions > dev [DISABLED] > Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

subversion
Discussion topic

Hide all messages in topic

All messages in topic

Re: [PATCH] libsvn_wc (log) #3, result_pool/scratch_pool

Author gstein
Full name Greg Stein
Date 2009-09-19 10:40:45 PDT
Message On Sat, Sep 19, 2009 at 11:14, Martin Hauner <martin dot hauner at gmx dot net> wrote:
>...
> I'll move on to adm_files.[ch] now if that is ok. Or is there anything more
> useful?

Much of that will become deprecated, so something like copy.c or
status.c would be another good place. But if you'd like to zing out
adm_files, that wouldn't be a problem!

> I have two questions regarding admin_files.[ch]:
>
> - svn_wc_get_adm_dir
>
> Reading the comment I think that it should have both (result/scratch) pools.
> Looking at the implementation it just returns a static. Is it worth here
> splitting the pool?

Nah. Just rename it to result_pool, and be done with it.

>
> - adm_files.[ch]
>
> In most cases there is no comment about the pool usage. Should I add it?

Not in adm_files.h, I'd think. Since they're internal functions, and
the two-pool paradigm is normal/expected, and half of those functions
will probably disappear... I'd say to not go out of your way to adjust
internal doc about dual pools.

Thanks!
-g

Re: [PATCH] libsvn_wc (log) #3, result_pool/scratch_pool

Author Martin Hauner <martin dot hauner at gmx dot net>
Full name Martin Hauner <martin dot hauner at gmx dot net>
Date 2009-09-19 08:14:49 PDT
Message Hi Greg,

On 19.09.09 14:16, Greg Stein wrote:
> On Sat, Sep 19, 2009 at 06:16, Martin Hauner<martin.hau​ner at gmx dot net> wrote:
>> Hi Greg,
>>
>>> I'll be around this weekend, rather than bumming around Hawaii :-) ...
>>> I'll look forward to the patch!
>>
>> here we go... the patch is now against r39457 which is HEAD just NOW.
>>
>> I hope it doesn't have too many issues. It is not really funny to chase HEAD
>> ;-))
>
> Yeah... sorry about that :-(

That's ok, at least I have learned to update more often :)

> I've applied your patch in r39462. Thanks!!

Wow, that was fast :-) Thanks too :)

I'll move on to adm_files.[ch] now if that is ok. Or is there anything more useful?

I have two questions regarding admin_files.[ch]:

- svn_wc_get_adm_dir

Reading the comment I think that it should have both (result/scratch) pools.
Looking at the implementation it just returns a static. Is it worth here
splitting the pool?

- adm_files.[ch]

In most cases there is no comment about the pool usage. Should I add it?

>
> Cheers,
> -g
>


--
Martin

Subcommander 2.0.0 Beta 5 - http://subcommander.tigris.org
a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.

Re: [PATCH] libsvn_wc (log) #3, result_pool/scratch_pool

Author gstein
Full name Greg Stein
Date 2009-09-19 05:16:48 PDT
Message On Sat, Sep 19, 2009 at 06:16, Martin Hauner <martin dot hauner at gmx dot net> wrote:
> Hi Greg,
>
>> I'll be around this weekend, rather than bumming around Hawaii :-) ...
>> I'll look forward to the patch!
>
> here we go... the patch is now against r39457 which is HEAD just NOW.
>
> I hope it doesn't have too many issues. It is not really funny to chase HEAD
> ;-))

Yeah... sorry about that :-(

I've applied your patch in r39462. Thanks!!

Cheers,
-g

Re: [PATCH] libsvn_wc (log) #3, result_pool/scratch_pool

Author Martin Hauner <martin dot hauner at gmx dot net>
Full name Martin Hauner <martin dot hauner at gmx dot net>
Date 2009-09-19 03:16:20 PDT
Message Hi Greg,

> I'll be around this weekend, rather than bumming around Hawaii :-) ...
> I'll look forward to the patch!

here we go... the patch is now against r39457 which is HEAD just NOW.

I hope it doesn't have too many issues. It is not really funny to chase HEAD ;-))

Summary of test results:
   1144 tests PASSED
   24 tests SKIPPED
   37 tests XFAILED (1 WORK-IN-PROGRESS)

> Cheers,
> -g

--
Martin

Subcommander 2.0.0 Beta 5 - http://subcommander.tigris.org
a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
Attachments

Re: [PATCH] libsvn_wc (log) #2, result_pool/scratch_pool

Author gstein
Full name Greg Stein
Date 2009-09-14 08:59:12 PDT
Message On Mon, Sep 14, 2009 at 07:53, Martin Hauner <Martin dot Hauner at gmx dot net> wrote:
> Hi Greg,
>
>> Von: Greg Stein <gstein at gmail dot com>
>> Betreff: Re: [PATCH] libsvn_wc (log) #2, result_pool/scratch_pool
>
>> Hate to say it, but this patch is against a trunk that's about four
>> days old :-( ... it doesn't apply properly :-(
>>
>> Unfortunately, between the time that you must have updated and now,
>> hyrum has changed some of the affected code.
>>
>> The patch is still looking good, but it just doesn't apply :-( ...
>> Maybe update to trunk one more time *just* before sending the patch.
>> See if anything has changed.
>
> Hmm, unfortunately I'm away for a couple of days... so this will have to wait until the weekend.. :(

Thanks!

I'll be around this weekend, rather than bumming around Hawaii :-) ...
I'll look forward to the patch!

Cheers,
-g

Re: [PATCH] libsvn_wc (log) #2, result_pool/scratch_pool

Author "Martin Hauner" <Martin dot Hauner at gmx dot net>
Full name "Martin Hauner" <Martin dot Hauner at gmx dot net>
Date 2009-09-14 04:53:20 PDT
Message Hi Greg,

> Von: Greg Stein <gstein at gmail dot com>
> Betreff: Re: [PATCH] libsvn_wc (log) #2, result_pool/scratch_pool

> Hate to say it, but this patch is against a trunk that's about four
> days old :-( ... it doesn't apply properly :-(
>
> Unfortunately, between the time that you must have updated and now,
> hyrum has changed some of the affected code.
>
> The patch is still looking good, but it just doesn't apply :-( ...
> Maybe update to trunk one more time *just* before sending the patch.
> See if anything has changed.

Hmm, unfortunately I'm away for a couple of days... so this will have to wait until the weekend.. :(


--
Martin


GRATIS für alle GMX-Mitglieder: Die maxdome Movie-FLAT!
Jetzt freischalten unter http://portal.gmx.ne​t/de/go/maxdome01

Re: [PATCH] libsvn_wc (log) #2, result_pool/scratch_pool

Author gstein
Full name Greg Stein
Date 2009-09-14 00:39:47 PDT
Message Hate to say it, but this patch is against a trunk that's about four
days old :-( ... it doesn't apply properly :-(

Unfortunately, between the time that you must have updated and now,
hyrum has changed some of the affected code.

The patch is still looking good, but it just doesn't apply :-( ...
Maybe update to trunk one more time *just* before sending the patch.
See if anything has changed.

Looking forward to it,
-g

On Sun, Sep 13, 2009 at 07:31, Martin Hauner <martin dot hauner at gmx dot net> wrote:
> Hi,
>
> here is an updated patch for the log stuff of libsvn_wc.
>
>
> --
> Martin
>
> Subcommander 2.0.0 Beta 5 - http://subcommander.tigris.org
> a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
>

Re: [PATCH] libsvn_wc (log) #2, result_pool/scratch_pool

Author Martin Hauner <martin dot hauner at gmx dot net>
Full name Martin Hauner <martin dot hauner at gmx dot net>
Date 2009-09-13 04:32:01 PDT
Message Hi,

here is an updated patch for the log stuff of libsvn_wc.


--
Martin

Subcommander 2.0.0 Beta 5 - http://subcommander.tigris.org
a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
Attachments

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author Martin Hauner <martin dot hauner at gmx dot net>
Full name Martin Hauner <martin dot hauner at gmx dot net>
Date 2009-09-12 07:01:23 PDT
Message Hi,

On 12.09.09 11:23, Greg Stein wrote:
> Martin: sorry for the delay here. The patch looks great, but I doubt
> it is going to apply today. Could you update the patch, to be
> applicable against trunk, and then I'll review/commit the sucker.

No problem, I'll update it for trunk.

Note that I also send a patch for some admin stuff in libsnv_wc here:

http://subversion.ti​gris.org/ds/viewMess​age.do?dsForumId=462​&dsMessageId=239​1310

> Thanks!
> -g

--
Martin

Subcommander 2.0.0 Beta 4 - http://subcommander.tigris.org
a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author gstein
Full name Greg Stein
Date 2009-09-12 02:23:41 PDT
Message Yowza. This patch is spot-on. The comment changes are xlnt, and the
style is perfect. I note that the patch follows precedent of pulling
the two-pools down to its own line in an argument list in a function
call. Very typical of what is seen elsewhere.

And yes... while I said that log.[ch] is headed for the graveyard,
I'll note that hwright is plowing thru the file in order to change a
bunch of callpoints into the lower subsystems. ie. while we're going
to get rid of it eventually, tweaking it now let's us advance the
lower (and upper) levels. Thus, a patch to log.[ch] is very handy.

Martin: sorry for the delay here. The patch looks great, but I doubt
it is going to apply today. Could you update the patch, to be
applicable against trunk, and then I'll review/commit the sucker.

Thanks!
-g


On Fri, Sep 11, 2009 at 21:07, Gavin Baumanis <gavinb at thespidernet dot com> wrote:
> Ping.
> This submission has not received any follow-up.
>
> Please note the patch is attached to the original post of this thread;
> http://subversion.ti​gris.org/ds/viewMess​age.do?dsForumId=462​&dsMessageId=238​6500
>
>
> Gavin.
> On 24/08/2009, at 06:52 , Martin Hauner wrote:
>
>> Hi Greg,
>>
>> On 23.08.09 20:04, Greg Stein wrote:
>>> On Sun, Aug 23, 2009 at 11:56, Martin
>>> Hauner<martin.hau​ner at gmx dot net>  wrote:
>>>> ...
>>>>> I would suggest files other than log.[ch] -- the loggy subsystem is
>>>>> going to be completely removed over the next six weeks.
>>>>
>>>> Ouch, another wasted hour ;-)
>>>
>>> If you're "close" with a patch, then you may as well wrap it up and
>>> send it to the list. It certainly can't hurt to apply it.
>>
>> Yes, it was attached to my first email. :)
>>
>>
>>> Cheers,
>>> -g
>>>
>>
>>
>> --
>> Martin
>>
>> Subcommander 2.0.0 Beta 4 - http://subcommander.tigris.org
>> a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
>>
>> --------------------​--------------------​--------------
>> http://subversion.ti​gris.org/ds/viewMess​age.do?dsForumId=462​&dsMessageId=238​6574
>
> --------------------​--------------------​--------------
> http://subversion.ti​gris.org/ds/viewMess​age.do?dsForumId=462​&dsMessageId=239​3774
>

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author gavinbaumanis
Full name Gavin Baumanis
Date 2009-09-11 18:07:53 PDT
Message Ping.
This submission has not received any follow-up.

Please note the patch is attached to the original post of this thread;
http://subversion.ti​gris.org/ds/viewMess​age.do?dsForumId=462​&dsMessageId=238​6500


Gavin.
On 24/08/2009, at 06:52 , Martin Hauner wrote:

> Hi Greg,
>
> On 23.08.09 20:04, Greg Stein wrote:
>> On Sun, Aug 23, 2009 at 11:56, Martin
>> Hauner<martin.hau​ner at gmx dot net> wrote:
>>> ...
>>>> I would suggest files other than log.[ch] -- the loggy subsystem is
>>>> going to be completely removed over the next six weeks.
>>>
>>> Ouch, another wasted hour ;-)
>>
>> If you're "close" with a patch, then you may as well wrap it up and
>> send it to the list. It certainly can't hurt to apply it.
>
> Yes, it was attached to my first email. :)
>
>
>> Cheers,
>> -g
>>
>
>
> --
> Martin
>
> Subcommander 2.0.0 Beta 4 - http://subcommander.tigris.org
> a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
>
> --------------------​--------------------​--------------
> http://subversion.ti​gris.org/ds/viewMess​age.do?dsForumId=462​&dsMessageId=238​6574

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author Martin Hauner <martin dot hauner at gmx dot net>
Full name Martin Hauner <martin dot hauner at gmx dot net>
Date 2009-08-23 13:52:57 PDT
Message Hi Greg,

On 23.08.09 20:04, Greg Stein wrote:
> On Sun, Aug 23, 2009 at 11:56, Martin Hauner<martin.hau​ner at gmx dot net> wrote:
>> ...
>>> I would suggest files other than log.[ch] -- the loggy subsystem is
>>> going to be completely removed over the next six weeks.
>>
>> Ouch, another wasted hour ;-)
>
> If you're "close" with a patch, then you may as well wrap it up and
> send it to the list. It certainly can't hurt to apply it.

Yes, it was attached to my first email. :)


> Cheers,
> -g
>


--
Martin

Subcommander 2.0.0 Beta 4 - http://subcommander.tigris.org
a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author gstein
Full name Greg Stein
Date 2009-08-23 11:04:43 PDT
Message On Sun, Aug 23, 2009 at 11:56, Martin Hauner<martin.hau​ner at gmx dot net> wrote:
>...
>> I would suggest files other than log.[ch] -- the loggy subsystem is
>> going to be completely removed over the next six weeks.
>
> Ouch, another wasted hour ;-)

If you're "close" with a patch, then you may as well wrap it up and
send it to the list. It certainly can't hurt to apply it.

>...

Cheers,
-g

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author Martin Hauner <martin dot hauner at gmx dot net>
Full name Martin Hauner <martin dot hauner at gmx dot net>
Date 2009-08-23 08:56:13 PDT
Message Hi Greg,

On 23.08.09 17:13, Greg Stein wrote:
> Hey Martin,
>
> I've always applied a line length of 78. A character in the 79th
> position can cause Emacs (in certain setups) to display a line
> continuation character in position 80, and then nothing on the next
> line. Annoying. But trimming columns to 78 keeps within the "80"
> column limit.

Oh, I thought it was 72. 78 is nice, then there will be less formatting
changes.

> I would suggest files other than log.[ch] -- the loggy subsystem is
> going to be completely removed over the next six weeks.

Ouch, another wasted hour ;-)

> Some of the adm_* files could be a good place to start.

ok, I will consider the log code as a learning session and move on to the adm
files :-)

> And yes: multiple smaller patches is much better than a mother patch.
> You'll be able to get feedback as you go, and start to learn more
> about style and clarification of the goals, rather than waiting until
> "the end" for that info. You'll also be racing time as we make other
> changes to the code, and you'll have a hard time finding somebody
> wanting to review 2000 lines of pool alterations :-)

Yep, exactly what I thought.. :-)


> Cheers,
> -g

--
Martin

Subcommander 2.0.0 Beta 4 - http://subcommander.tigris.org
a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author gstein
Full name Greg Stein
Date 2009-08-23 08:27:47 PDT
Message On Sun, Aug 23, 2009 at 11:21, Neels Janosch Hofmeyr<neels at elego dot de> wrote:
>...
> Martin, just a quick rant: if you do cosmetic changes, it's nice to
> - not mix cosmetic changes with functional changes in a patch, and

There is a balance to this, I think. A cosmetic change here or there
in a functional patch is okay, I think. Just not widespread changes
along with functional changes.

> - group a whole bunch of cosmetic changes in a single patch.

I think this is totally fine. As long as *no* functional changes
occur, then I see no problem with a whole ton of disparate cosmetic
changes.

Cheers,
-g

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author neels
Full name Neels Janosch Hofmeyr
Date 2009-08-23 08:24:16 PDT
Message Martin Hauner wrote:
> I also wonder if I should modify the formatting where the lines get longer
> than 72 characters. There are a lot of other lines that don't strictly follow

Quoting HACKING:
[[[
Restrict lines to 79 columns, so that code will display well in a minimal
standard display window. (There can be exceptions, such as when declaring a
block of 80-column text with a few extra columns taken up by indentation,
quotes, etc., if splitting each line in two would be unreasonably messy.)
]]]

IMHO 76 is nicer, so code can still be indented without redoing the
wrapping. It's not a showstopper, really, if a line gets longer, but it's
nice to adhere to the limit nevertheless.

Martin, just a quick rant: if you do cosmetic changes, it's nice to
- not mix cosmetic changes with functional changes in a patch, and
- group a whole bunch of cosmetic changes in a single patch.

Anyway, if you don't have commit access, I guess it's fine to not bother too
much about cosmetic fixes.

~Neels
Attachments

Re: [PATCH] libsvn_wc #1, result_pool/scratch_pool

Author gstein
Full name Greg Stein
Date 2009-08-23 08:13:57 PDT
Message Hey Martin,

I've always applied a line length of 78. A character in the 79th
position can cause Emacs (in certain setups) to display a line
continuation character in position 80, and then nothing on the next
line. Annoying. But trimming columns to 78 keeps within the "80"
column limit.

If you're going to make *many* line-length reflow changes (ie. just
formatting rather than functional), then we tend to make those
separate revisions from functional ones. It is a subjective matter, as
doing a few formatting updates along with a functional change is fine.
Obviously, if your functional change requires reflowing, then that's
totally cool.

I would suggest files other than log.[ch] -- the loggy subsystem is
going to be completely removed over the next six weeks.

Some of the adm_* files could be a good place to start.

And yes: multiple smaller patches is much better than a mother patch.
You'll be able to get feedback as you go, and start to learn more
about style and clarification of the goals, rather than waiting until
"the end" for that info. You'll also be racing time as we make other
changes to the code, and you'll have a hard time finding somebody
wanting to review 2000 lines of pool alterations :-)

Cheers,
-g

On Sun, Aug 23, 2009 at 09:55, Martin Hauner<martin.hau​ner at gmx dot net> wrote:
> Hi,
>
> At first I started to create a single large patch. But I think going with
> several smaller patches and doing this step by step is a safer route. It will be
> a lot easier to review and reduces the risk of introducing any errors.
>
> I will try to do this starting with the lower level functions, starting with
> log.h/log.c.
>
> Is this ok?
>
> I also wonder if I should modify the formatting where the lines get longer
> than 72 characters. There are a lot of other lines that don't strictly follow
> the line limit?
>
>
> --
> Martin
>
> Subcommander 2.0.0 Beta 4 - http://subcommander.tigris.org
> a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
>
> --------------------​--------------------​--------------
> http://subversion.ti​gris.org/ds/viewMess​age.do?dsForumId=462​&dsMessageId=238​6500

[PATCH] libsvn_wc #1, result_pool/scratch_pool

Author Martin Hauner <martin dot hauner at gmx dot net>
Full name Martin Hauner <martin dot hauner at gmx dot net>
Date 2009-08-23 06:55:18 PDT
Message Hi,

At first I started to create a single large patch. But I think going with
several smaller patches and doing this step by step is a safer route. It will be
a lot easier to review and reduces the risk of introducing any errors.

I will try to do this starting with the lower level functions, starting with
log.h/log.c.

Is this ok?

I also wonder if I should modify the formatting where the lines get longer
than 72 characters. There are a lot of other lines that don't strictly follow
the line limit?


--
Martin

Subcommander 2.0.0 Beta 4 - http://subcommander.tigris.org
a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.
Attachments
Messages per page: