Julian Foad wrote:
> Max Bowsher wrote:
>> OK, here are the results of my recent work on keywords as hash.
>>
>> Firstly, I've split the whole thing into 4 individually committable
>> sections:
>> 1. include & libsvn_subr
>> 2. libsvn_wc
>> 3. libsvn_client
>> 4. cmdline client & tests
>
> Please could you say what functional change is made in each part, not
> just what files are changed?
Well, obviously the log message for a given part should say what functional
change is in that, but I wasn't sure that it did because it seems to say too
much. (See below.)
>> keywords_to_keyhash: Rename to kwstruct_to_kwhash. Document why we
>> need to check for NULL. Convert NULL to NULL, not to an empty hash,
>> because NULL is a special signifier for svn_subst_translate_stream and
>> friends.
>
>> [[[
>> Revise keywords API - represent keywords as a hash for better extensibility.
>> Implement printf-like format characters for keyword expansion. Change all
>> other libraries to use new keyword hash functions from libsvn_subst.
>> Includes a new keyword, UUID, and tests for same.
>> See issue #890 for details.
>> Based on a patch by John Peacock <jpeacock rowman com>.
>> This is phase 1 of 4: libsvn_subr.
>
> Are all of those changes in this patch? If not, please present a patch
> whose log message just describes what it does. Then the patch will be
> able to stand alone and it will not be necessary to say "this is 1 of 4"
> which might turn out not to be true in the end.
> I'm not sure that representing "no entries" as NULL is the right
> choice. It makes tests slighly simpler, but means that every piece of
When I said "tests", I meant when callers test whether there are any entries:
e.g. "if (entries)" vs. "if (entries->is_empty())".
> code using the hash has to test for NULL first, and every piece of code
> wanting to add an entry first has to say "if (NULL) { allocate memory
> for the hash somewhere }" which surely is ugly.
>
> I'm afraid I'll have to leave this with you for a few days as I won't be
> able to work on it over Easter. Thanks.
I have had this patch marked for attention, but I'm afraid that every time I
look at it I am overwhelmed by the number of different changes that seem (or
are said) to be in it so I am not going to get around to reviewing it in its
present form. I am very sorry.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Apr 10 22:52:23 2005