You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/10/09 06:30:49 UTC
[PATCH][merge-tracking] code simplification of get_merge_info_for_path
Hi All,
Find that attached patch and log.
With regards
Kamesh Jayachandran
Re: [PATCH][merge-tracking] code simplification of get_merge_info_for_path
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 09 Oct 2006, Kamesh Jayachandran wrote:
> Malcolm Rowe wrote:
...
> >However, I don't understand why this function needs to copy part of path
> >into pool - do we need to modify toappend later?
> >
> >i.e. what's wrong with just this?
> >
> > toappend = &path[parentpath->len + 1];
> >
> >Alternatively, if you do need to copy it, just call apr_pstrdup() rather
> >than pcalloc/strcpy.
...
> Yes Malcom, 'toappend' is not modified by
> 'append_component_to_paths'(Accepts it as const char*).
> Attaching the new patch and log.
>
> With regards
> Kamesh Jayachandran
> [[[
> Patch by: Kamesh Jayachandran <ka...@collab.net>
>
> No need to duplicate the component dir as it is never modified by
> function('append_component_to_paths') using it.
You can avoid duplicating the description for single changes like this
in the summary. If you want to provide both a summary and a specific
change description (which is generally better, as it saves browsers of
your log message from reading the details), provide a higher-level
description of "why" and/or "what" in the summary, and a very specific
"what" within the following detail section.
> * subversion/libsvn_fs_util/merge-info-sqlite-index.c
> (get_merge_info_for_path):
> No need to duplicate the component dir as it is never modified by
> function('append_component_to_paths') using it.
>
> ]]]
Looks good Kamesh, committed in r21844.
Re: [PATCH][merge-tracking] code simplification of get_merge_info_for_path
Posted by Kamesh Jayachandran <ka...@collab.net>.
Malcolm Rowe wrote:
> On Mon, Oct 09, 2006 at 12:00:49PM +0530, Kamesh Jayachandran wrote:
>
>> Use strcpy rather than mimicing the behaviour of 'strcpy'.
>>
>>
>
>
>> @@ -490,8 +488,7 @@
>> path string. */
>> toappend = apr_pcalloc(pool,
>> (strlen(path) - parentpath->len) + 1);
>> - for (i = 0, p = &path[parentpath->len + 1]; *p; i++, p++)
>> - *(toappend + i) = *p;
>> + strcpy(toappend, path+(parentpath->len + 1));
>> append_component_to_paths(&translatedhash, cacheresult,
>> toappend, pool);
>>
>>
>
> Nice find, although I find the &path[n] syntax easier to parse myself
> when n is complex.
>
> However, I don't understand why this function needs to copy part of path
> into pool - do we need to modify toappend later?
>
> i.e. what's wrong with just this?
>
> toappend = &path[parentpath->len + 1];
>
> Alternatively, if you do need to copy it, just call apr_pstrdup() rather
> than pcalloc/strcpy.
>
> Regards,
> Malcolm
>
Yes Malcom, 'toappend' is not modified by
'append_component_to_paths'(Accepts it as const char*).
Attaching the new patch and log.
With regards
Kamesh Jayachandran
Re: [PATCH][merge-tracking] code simplification of get_merge_info_for_path
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Oct 09, 2006 at 12:00:49PM +0530, Kamesh Jayachandran wrote:
> Use strcpy rather than mimicing the behaviour of 'strcpy'.
>
> @@ -490,8 +488,7 @@
> path string. */
> toappend = apr_pcalloc(pool,
> (strlen(path) - parentpath->len) + 1);
> - for (i = 0, p = &path[parentpath->len + 1]; *p; i++, p++)
> - *(toappend + i) = *p;
> + strcpy(toappend, path+(parentpath->len + 1));
> append_component_to_paths(&translatedhash, cacheresult,
> toappend, pool);
>
Nice find, although I find the &path[n] syntax easier to parse myself
when n is complex.
However, I don't understand why this function needs to copy part of path
into pool - do we need to modify toappend later?
i.e. what's wrong with just this?
toappend = &path[parentpath->len + 1];
Alternatively, if you do need to copy it, just call apr_pstrdup() rather
than pcalloc/strcpy.
Regards,
Malcolm