You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Näslund <da...@longitudo.com> on 2009/08/17 20:39:55 UTC
Re: [PATCH] replace svn_wc_entry() in libsvn_client/patch.
Forgot!
make check was clean except for:
switch_tests.py 18, 21
But both of theese failed on trunk as has been for the last couple of
days.
On Mon, Aug 17, 2009 at 10:34:24PM +0200, Daniel Näslund wrote:
> Hi!
>
> [[[
> Replaced some svn_wc_entry() with svn_wc__get_entry_versioned().
>
> * subversion/libsvn_client/patch.c
> (merge_dir_added): Replaced svn_wc_entry(). Used absolute paths.
> (init_patch_target): Replaced svn_wc_entry().
> ]]]
>
> Mvh
> Daniel
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2384521
Re: [PATCH] v3. replace alls svn_wc_entry() in
libsvn_client/patch.c
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Aug 19, 2009, at 9:14 AM, Daniel Näslund wrote:
> On Tue, Aug 18, 2009 at 12:17:30PM -0500, Hyrum K. Wright wrote:
>> On Aug 18, 2009, at 12:12 PM, Julian Foad wrote:
>>
>>> Replacing one line of code with a long block, eight times
>>> over, cannot be a good thing. Can you make a replacement function
>>> that
>>> does exactly what we need in one line, and so avoid this code bloat?
>>> Even a local helper function in this file would be an improvement,
>>> but I
>>> assume this same pattern will be repeated in many files so a new
>>> svn_wc__... function would be best.
>>
>> My inner self is saying the same thing here.
>> svn_wc__get_entry_versioned() was intentioned to replace
>> svn_wc__entry_versioned(), and we've found a few places where we can
>> get away with using it in place of svn_wc_entry(), but a modified
>> helper would be nice at this point.
>
> My bad. I was assumning that since this was a temporary solution I
> could
> get away with a long nasty repeating block of code.
>
> I call the new function svn_wc__maybe_get_entry(). I guess I've seen
> better names but I couldn't come up with anything else!
I couldn't think of anything better, so I figured I'd commit the patch
and then bikeshed about the name later. I tweaked a couple of things
like whitespace in function declarations, and added a missing pointer
dereference. Committed in r38893.
-Hyrum
>>>> [[[
>>>> Replaced svn_wc_entry() with svn_wc__get_entry_versioned(). Catch
>>>> SVN_ERR_ENTRY_NOT_FOUND since it is not expected.
>>>
>>> I know you know why you're doing this, but tell us in the log
>>> message:
>>>
>>> As part of getting rid of adm_access batons for WC-NG, replace
>>> some calls to svn_wc_entry() with svn_wc__get_entry_versioned().
>>>
>
> Point taken. Trying this instead:
>
> [[[
> As part of getting rid of adm_access batons for WC-NG, replace some
> calls to svn_wc_entry() with svn_wc__maybe_get_entry().
>
> * subversion/include/private/svn_wc_private.h
> (svn_wc__maybe_get_entry): Declare.
>
> * subversion/libsvn_wc/entries.c
> (svn_wc__maybe_get_entry): New. Wrapper for
> svn_wc__get_entry_versioned() with error and return semantics like
> svn_wc_entry().
>
> * subversion/libsvn_client/patch.c
> (merge_file_changed, merge_file_added, merge_file_deleted,
> merge_dir_added, init_patch_target): Replaced svn_wc_entry() with
> svn_wc__maybe_get_entry(). Use absolute paths.
> ]]]
>
> Two test failures (also on trunk):
> switch_tests 18 21
>
> Mvh
> Daniel
> <remove_entry_patch_v3.diff>
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2385794
[PATCH] v3. replace alls svn_wc_entry() in libsvn_client/patch.c
Posted by Daniel Näslund <da...@longitudo.com>.
On Tue, Aug 18, 2009 at 12:17:30PM -0500, Hyrum K. Wright wrote:
> On Aug 18, 2009, at 12:12 PM, Julian Foad wrote:
>
> > Replacing one line of code with a long block, eight times
> > over, cannot be a good thing. Can you make a replacement function that
> > does exactly what we need in one line, and so avoid this code bloat?
> > Even a local helper function in this file would be an improvement,
> > but I
> > assume this same pattern will be repeated in many files so a new
> > svn_wc__... function would be best.
>
> My inner self is saying the same thing here.
> svn_wc__get_entry_versioned() was intentioned to replace
> svn_wc__entry_versioned(), and we've found a few places where we can
> get away with using it in place of svn_wc_entry(), but a modified
> helper would be nice at this point.
My bad. I was assumning that since this was a temporary solution I could
get away with a long nasty repeating block of code.
I call the new function svn_wc__maybe_get_entry(). I guess I've seen
better names but I couldn't come up with anything else!
> >> [[[
> >> Replaced svn_wc_entry() with svn_wc__get_entry_versioned(). Catch
> >> SVN_ERR_ENTRY_NOT_FOUND since it is not expected.
> >
> > I know you know why you're doing this, but tell us in the log message:
> >
> > As part of getting rid of adm_access batons for WC-NG, replace
> > some calls to svn_wc_entry() with svn_wc__get_entry_versioned().
> >
Point taken. Trying this instead:
[[[
As part of getting rid of adm_access batons for WC-NG, replace some
calls to svn_wc_entry() with svn_wc__maybe_get_entry().
* subversion/include/private/svn_wc_private.h
(svn_wc__maybe_get_entry): Declare.
* subversion/libsvn_wc/entries.c
(svn_wc__maybe_get_entry): New. Wrapper for
svn_wc__get_entry_versioned() with error and return semantics like
svn_wc_entry().
* subversion/libsvn_client/patch.c
(merge_file_changed, merge_file_added, merge_file_deleted,
merge_dir_added, init_patch_target): Replaced svn_wc_entry() with
svn_wc__maybe_get_entry(). Use absolute paths.
]]]
Two test failures (also on trunk):
switch_tests 18 21
Mvh
Daniel
Re: [PATCH] v2. replace ALL svn_wc_entry() in libsvn_client/patch.c
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Aug 18, 2009, at 12:12 PM, Julian Foad wrote:
> Daniel Näslund wrote:
>> [[[
>> Replaced svn_wc_entry() with svn_wc__get_entry_versioned(). Catch
>> SVN_ERR_ENTRY_NOT_FOUND since it is not expected.
>
> I know you know why you're doing this, but tell us in the log message:
>
> As part of getting rid of adm_access batons for WC-NG, replace
> some calls to svn_wc_entry() with svn_wc__get_entry_versioned().
>
>
>> * subversion/libsvn_client/patch.c
>> (merge_file_changed, merge_file_added, merge_file_deleted,
>> merge_dir_added, init_patch_target): Replaced svn_wc_entry(). Used
>> absolute paths.
>> ]]]
> [...]
>> @@ -369,7 +391,26 @@
>> case svn_node_none:
>> {
>> const svn_wc_entry_t *entry;
>> - SVN_ERR(svn_wc_entry(&entry, mine, adm_access, FALSE,
>> subpool));
>> + svn_error_t *err;
>> +
>> + err = svn_wc__get_entry_versioned(&entry, patch_b->ctx-
>> >wc_ctx,
>> + mine_abspath,
>> svn_node_none,
>> + FALSE, FALSE,
>> + subpool, subpool);
>> +
>
> The indentation is off below here...
>
>> + /* We catch the error since a NULL value is expected
>> when the entry
>> + is hidden. */
>> + if (err && err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
>> + {
>> + svn_error_clear(err);
>> + entry = NULL;
>> + }
>> + else if (err)
>> + {
>> + svn_pool_destroy(subpool);
>> + return svn_error_return(err);
>> + }
>
> Replacing one line of code with a long block like this, eight times
> over, cannot be a good thing. Can you make a replacement function that
> does exactly what we need in one line, and so avoid this code bloat?
> Even a local helper function in this file would be an improvement,
> but I
> assume this same pattern will be repeated in many files so a new
> svn_wc__... function would be best.
My inner self is saying the same thing here.
svn_wc__get_entry_versioned() was intentioned to replace
svn_wc__entry_versioned(), and we've found a few places where we can
get away with using it in place of svn_wc_entry(), but a modified
helper would be nice at this point.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2385076
Re: [PATCH] v2. replace ALL svn_wc_entry() in libsvn_client/patch.c
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Näslund wrote:
> [[[
> Replaced svn_wc_entry() with svn_wc__get_entry_versioned(). Catch
> SVN_ERR_ENTRY_NOT_FOUND since it is not expected.
I know you know why you're doing this, but tell us in the log message:
As part of getting rid of adm_access batons for WC-NG, replace
some calls to svn_wc_entry() with svn_wc__get_entry_versioned().
> * subversion/libsvn_client/patch.c
> (merge_file_changed, merge_file_added, merge_file_deleted,
> merge_dir_added, init_patch_target): Replaced svn_wc_entry(). Used
> absolute paths.
> ]]]
[...]
> @@ -369,7 +391,26 @@
> case svn_node_none:
> {
> const svn_wc_entry_t *entry;
> - SVN_ERR(svn_wc_entry(&entry, mine, adm_access, FALSE, subpool));
> + svn_error_t *err;
> +
> + err = svn_wc__get_entry_versioned(&entry, patch_b->ctx->wc_ctx,
> + mine_abspath, svn_node_none,
> + FALSE, FALSE,
> + subpool, subpool);
> +
The indentation is off below here...
> + /* We catch the error since a NULL value is expected when the entry
> + is hidden. */
> + if (err && err->apr_err == SVN_ERR_ENTRY_NOT_FOUND)
> + {
> + svn_error_clear(err);
> + entry = NULL;
> + }
> + else if (err)
> + {
> + svn_pool_destroy(subpool);
> + return svn_error_return(err);
> + }
Replacing one line of code with a long block like this, eight times
over, cannot be a good thing. Can you make a replacement function that
does exactly what we need in one line, and so avoid this code bloat?
Even a local helper function in this file would be an improvement, but I
assume this same pattern will be repeated in many files so a new
svn_wc__... function would be best.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2384858
[PATCH] v2. replace ALL svn_wc_entry() in libsvn_client/patch.c
Posted by Daniel Näslund <da...@longitudo.com>.
Hi!
It's better to do the whole file at once, I think.
make check was clean except for:
external_tests.py 16
switch_tests.py 18, 21
They also fail on trunk for me.
[[[
Replaced svn_wc_entry() with svn_wc__get_entry_versioned(). Catch
SVN_ERR_ENTRY_NOT_FOUND since it is not expected.
* subversion/libsvn_client/patch.c
(merge_file_changed, merge_file_added, merge_file_deleted,
merge_dir_added, init_patch_target): Replaced svn_wc_entry(). Used
absolute paths.
]]]
Mvh
Daniel