[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r20728 - in branches/merge-tracking/subversion: bindings/swig bindings/swig/include bindings/swig/python/libsvn_swig_py include libsvn_ra_dav mod_dav_svn

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2006-07-28 01:27:27 CEST

Daniel Berlin wrote:
>>While I don't think that it's necessary for SWIG's sake, should the
>>implementation which delegates to the vtable, and vtable API/functions
>>reflect this variable rename as well (for consistency)?
>
> It doesn't matter. trying to match names up between headers and source
> is a losing war :)

Better for you the writer to do it once than for every reader to have to do it
every time he/she reads the code.

>>How 'bout calling this "mergeinfo" instead of "temp"? As it's added
>>to RESULT, it doesn't really seem temporary...
>
> Again, whatever makes you feel good. If you want to change it, change
> it. It's a temporary variable, used to store the result of a parse.

Of course it's temporary, but every other local variable is temporary too.
Should we call them all "temp"?

>>>+ /* ### todo: I don't understand why the static, file-global
>>>+ variables shared by update and status are called `report_head'
>>>+ and `report_tail', instead of `request_head' and `request_tail'.
>>>+ Maybe Greg can explain? Meanwhile, I'm tentatively using
>>>+ "request_*" for my local vars below. */
>>
>>I think the "report_" prefix is used in ra_dav because it's referring
>>to the payload of a specific type of HTTP request (REPORT). In HTTP
>>client/server apps, "request" usually refers to the entirety of the
>>HTTP request (e.g. including the request method and headers), rather
>>than solely to the request body.
>
> This was copied from log.c :)

That's not much of an excuse as you well know! That comment definitely wants
removing. It's out of date anyway: the variables it refers to don't exist as
far as I can see.

>>>+ /* These get determined from the request document. */
>>>+ svn_revnum_t rev = SVN_INVALID_REVNUM;
>>>+ svn_boolean_t include_parents = 0; /* off by default */
>>
>>This should be svn_boolean_t's constant FALSE rather than 0.
>
> style copied from log.c again :)
> svn_boolean_t discover_changed_paths = 0; /* off by default */
> svn_boolean_t strict_node_history = 0; /* off by default */

Better change it there too, then.

[...]
> As usual, the mod_dav_svn formatting problems are from copying from
> other mod_dav_svn code, so i'm not sure whether we want to be consistent
> or "right".

That's easy: we want both!

- Julian

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jul 28 01:26:58 2006

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.