[SAMBA4][PATCH] Recursive registry key delete (from bz #5178)

Andrew Kroeger andrew at id10ts.net
Tue Feb 19 21:46:22 GMT 2008


Jelmer Vernooij wrote:
> Hi Andrew!
> 
> Am Dienstag, den 19.02.2008, 09:58 -0600 schrieb Andrew Kroeger:
>> The attached patches implement recursive registry deletes as discussed
>> in bz #5178, and with Jelmer via IRC.
>>
>> In addition the the recursive delete implementation, I have also updated
>> the registry tests to work correctly with the recursive delete.
>>
>> I would appreciate if someone could review my patches and let me know if
>> there are any issues I need to address with them.
> Thanks! Overall, these changes look ok. Some minor comments:

Thanks!  I appreciate your review and comments.

> Rather than ifdeffing out some tests, I would rather see the rpc-winreg
> test be converted to the new torture API and having the individual tests
> marked as known failing. AFAIK these tests pass against Windows, so it
> would be a shame to disable them against Windows as well.

No problem.  I had seen other tests that have blocks ifdeffed out, but
that probably predates the new torture API.  Do you have any quick
pointers to use of the new torture API (vs. the old way of doing things)
so I can make sure I'm following the correct examples?

> Please don't move functions within the file unless there's a reason for
> it. It makes it harder to review.

I moved ldb_del_key() after ldb_del_value() in ldb.c, because I need to
call ldb_del_value() from ldb_del_key().  As ldb_del_value() is static,
it isn't defined in any of the header files.  Would it be better to just
add a declaration of the function earlier in the .c file, or is moving
functions OK in this case?

> The gotos in the recursive ldb delete function seem to make the code
> more rather than less complex and they each only save one line. Any
> chance you can remove at least one of the two labels?

No problem.  I can clean those up.  I put those in during my development
to ensure I was cleaning up properly every time I added a new error
case.  Now that all the error cases have been flushed out, I can remove
the gotos all together.

>> P.S. - The attached patches should also be in the v4-0-registry branch
>> of my Git repo at git://git.id10ts.net/samba.git.
>>
>> P.P.S - I'm not sure as to the patch submission protocol with Git - is
>> it OK to just point to the patches in my repo, or is it better to keep
>> sending patches to samba-technical to maintain an audit trail of what I
>> am actually submitting?
> Having the patches applied to the mailing list post is nice because it
> makes it easier to review and comment in-line where necessary.

Sounds good to me.  I will work your feedback into a new set of patches.

Still being new to Git, will it create any problems if I reset my
published repo to remove the original patch versions I pushed?  If I
understand things correctly, that shouldn't cause any problems unless
someone has based any new commits off of the history I pushed to my
topic branch, correct?

Thanks,
Andrew


More information about the samba-technical mailing list