> "S.Ramaswamy" <ramaswamy@collab.net> writes:
>
>> Changelog:
>>
>> * subversion/libsvn_client/commit.c
>> (repos_change_baton_t) : New struct with a single boolean
>> member to remember if changes were made to the repository.
>>
>
> So, I like the spirit of the change, but I think you took my
> suggestions too literally. As it turns out, there is no existing
> context baton for import operations. So why not just add
> 'svn_boolean_t *' parameters to all the places you added
> 'repos_change_baton_t *' ones in your patch. We don't to create a
> context baton just to hold one variable.
I checked around to see if I was pioneering the use of single variable
batons before sending the patch - saw one in simple_provider.c or some such
- I will add the additional members that you suggested in the other mail.
> And if we were thinking,
> "Hey, a context baton is easy to grow later should we need to," then we
> should call that thing 'struct import_ctx_t' or something not so tied
> to the "Did-the-repos-change" question.
>
>> @@ -387,6 +396,9 @@
>> directories. If we stop doing that, here is the place to
>> change your world. */
>> }
>> + /* remember that the repository was modified */
>> + if (apr_hash_count (dirents))
>> + rcb->repos_changed = TRUE;
>>
>> svn_pool_destroy (subpool);
>> return SVN_NO_ERROR;
>
> I think it makes more sense to put this boolean setting line
> immediately after the editor->add_directory() calls to which it
> corresponds. Yes, that means it will get hit multiple times,
... which is why I didn't put it there; will move it up.
> but it
> will greatly improve code understandability.
>
>> @@ -437,6 +449,7 @@
>> apr_array_header_t *ignores;
>> apr_array_header_t *batons = NULL;
>> const char *edit_path = "";
>> + repos_change_baton_t *rcb = apr_pcalloc (pool, sizeof (*rcb));;
>
> Oops. Double-semicolon.
>
>> Index: subversion/tests/clients/cmdline/basic_tests.py
>> ===================================================================
>> --- subversion/tests/clients/cmdline/basic_tests.py (revision 14060)
>> +++ subversion/tests/clients/cmdline/basic_tests.py (working copy) @@
>> -1632,7 +1632,34 @@
>> else:
>> raise svntest.Failure
>>
>> +#----------------------------------------------------------------------
>> +def basic_empty_revision(sbox):
>> + "verify that an empty revision cannot be created"
>
> This function name and description are entirely too vague. As we know
> from experience, 'svn import' isn't the only route by which folks
> have, in the past, been able to generate an empty commit. Suggest
> moving this test to a new 'import_tests.py' (I'm really surprised we
> don't have one of these already!), perhaps as
> import_avoid_empty_revision(), and with a more focused description.
Should the existing import tests be moved from basic_tests.py to the new
import_tests.py ?
Thanks
Ramaswamy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Apr 15 19:20:46 2005