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/07/28 09:08:27 UTC
Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
- }
> -
> + SVN_ERR(index_merge_info(cb, new_rev, pool));
>
Fails to cleanup the earlier failed merge commits.
Attached patch does it.
With regards
Kamesh Jayachandran
[[[
Patch by: Kamesh Jayachandran <ka...@collab.net>
r20893 fails to do a cleanup of some earlier commit with the rev as
current one.
* subversion/libsvn_fs_fs/fs_fs.c
(index_merge_info)
One extra parameter 'contains_merge_info' is added.
Adds the mergeinfo if the commit has some merge info.
(commit_body): calls index_merge_info unconditionally by passing
the 'contains_merge_info'.
]]]
Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 31 Jul 2006, Kamesh Jayachandran wrote:
> Daniel Rall wrote:
> >On Fri, 28 Jul 2006, Kamesh Jayachandran wrote:
> >
> >
> >>Daniel Rall wrote:
> >>
> >>>Okay -- we need to always do the cleanup, 'cuz if we don't we could
> >>>leave stray records around the merge info index. Which don't actively
> >>>harm anything, but perhaps should be deleted...
> >>>
> >>>The API probably shouldn't be named index_merge_info(), then.
> >>>update_merge_info() would be better, but is already taken. Should we
> >>>merge the two routines into a single routine, or rename the existing
> >>>update_merge_info() function to something more specific?
> >>>
> >>>
> >>>
> >>Find the attached patch.
> >>
> >
> >I've committed a variation of this in r20923 which uses slightly less
> >SQL-y function names.
> >
> >I take it you didn't like the idea of merging the merge info index
> >entry point function you added and the function which loops over any
> >merge info for a transaction?
>
> I overlooked at it, will see it through.
While conceptually it's fine, I'm worried that it might hurt
comprehensibility, since it'll make a rather large single function out
of two separate functions which are nicely modularized (though have
only one caller each).
Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Rall wrote:
> On Fri, 28 Jul 2006, Kamesh Jayachandran wrote:
>
>
>> Daniel Rall wrote:
>>
>>> Okay -- we need to always do the cleanup, 'cuz if we don't we could
>>> leave stray records around the merge info index. Which don't actively
>>> harm anything, but perhaps should be deleted...
>>>
>>> The API probably shouldn't be named index_merge_info(), then.
>>> update_merge_info() would be better, but is already taken. Should we
>>> merge the two routines into a single routine, or rename the existing
>>> update_merge_info() function to something more specific?
>>>
>>>
>>>
>> Find the attached patch.
>>
>
> I've committed a variation of this in r20923 which uses slightly less
> SQL-y function names.
>
> I take it you didn't like the idea of merging the merge info index
> entry point function you added and the function which loops over any
> merge info for a transaction?
>
I overlooked at it, will see it through.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 28 Jul 2006, Kamesh Jayachandran wrote:
> Daniel Rall wrote:
> >Okay -- we need to always do the cleanup, 'cuz if we don't we could
> >leave stray records around the merge info index. Which don't actively
> >harm anything, but perhaps should be deleted...
> >
> >The API probably shouldn't be named index_merge_info(), then.
> >update_merge_info() would be better, but is already taken. Should we
> >merge the two routines into a single routine, or rename the existing
> >update_merge_info() function to something more specific?
> >
> >
> Find the attached patch.
I've committed a variation of this in r20923 which uses slightly less
SQL-y function names.
I take it you didn't like the idea of merging the merge info index
entry point function you added and the function which loops over any
merge info for a transaction?
Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
Posted by Kamesh Jayachandran <ka...@collab.net>.
Daniel Rall wrote:
> Okay -- we need to always do the cleanup, 'cuz if we don't we could
> leave stray records around the merge info index. Which don't actively
> harm anything, but perhaps should be deleted...
>
> The API probably shouldn't be named index_merge_info(), then.
> update_merge_info() would be better, but is already taken. Should we
> merge the two routines into a single routine, or rename the existing
> update_merge_info() function to something more specific?
>
>
Find the attached patch.
With regards
Kamesh Jayachandran
[[[
Patch by: Kamesh Jayachandran <ka...@collab.net>
r20893 fails to do a cleanup of some earlier commit with the rev as
current one.
* subversion/libsvn_fs_fs/fs_fs.c
(generate_mergeinfo_sql): Renamed to
'insert_mergeinfo_index_for_single_path'
(update_mergeinfo_index): Renamed to 'insert_mergeinfo_index'
changed to call insert_mergeinfo_index_for_single_path.
(index_merge_info): Renamed to 'replace_merge_info_index'
One extra parameter 'contains_merge_info' is added.
Adds the mergeinfo if the commit has some merge info.
(commit_body): calls 'replace_merge_info_index' unconditionally by passing
the 'contains_merge_info'.
]]]
Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 31 Jul 2006, Kamesh Jayachandran wrote:
> Madan U Sreenivasan wrote:
> >On Sat, 29 Jul 2006 03:18:39 +0530, Daniel Rall <dl...@collab.net> wrote:
> >
> >>Okay -- we need to always do the cleanup, 'cuz if we don't we could
> >>leave stray records around the merge info index. Which don't actively
> >>harm anything, but perhaps should be deleted...
> >>
> >>The API probably shouldn't be named index_merge_info(), then.
> >>update_merge_info() would be better, but is already taken.
> >
> >Where? I can only see update_wc_merge_info().
> Dlr, would have meant 'update_mergeinfo_index' in
> subversion/libsvn_fs_fs/fs_fs.c
Yeah, I meant that one, sorry.
- Dan
Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
Posted by Kamesh Jayachandran <ka...@collab.net>.
Madan U Sreenivasan wrote:
> On Sat, 29 Jul 2006 03:18:39 +0530, Daniel Rall <dl...@collab.net> wrote:
>
>> Okay -- we need to always do the cleanup, 'cuz if we don't we could
>> leave stray records around the merge info index. Which don't actively
>> harm anything, but perhaps should be deleted...
>>
>> The API probably shouldn't be named index_merge_info(), then.
>> update_merge_info() would be better, but is already taken.
>
> Where? I can only see update_wc_merge_info().
Dlr, would have meant 'update_mergeinfo_index' in
subversion/libsvn_fs_fs/fs_fs.c
With regards
Kamesh Jayachandran
>
> Regards,
> Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
Posted by Madan U Sreenivasan <ma...@collab.net>.
On Sat, 29 Jul 2006 03:18:39 +0530, Daniel Rall <dl...@collab.net> wrote:
> Okay -- we need to always do the cleanup, 'cuz if we don't we could
> leave stray records around the merge info index. Which don't actively
> harm anything, but perhaps should be deleted...
>
> The API probably shouldn't be named index_merge_info(), then.
> update_merge_info() would be better, but is already taken.
Where? I can only see update_wc_merge_info().
Regards,
Madan.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r20893 - branches/merge-tracking/subversion/libsvn_fs_fs
Posted by Daniel Rall <dl...@collab.net>.
Okay -- we need to always do the cleanup, 'cuz if we don't we could
leave stray records around the merge info index. Which don't actively
harm anything, but perhaps should be deleted...
The API probably shouldn't be named index_merge_info(), then.
update_merge_info() would be better, but is already taken. Should we
merge the two routines into a single routine, or rename the existing
update_merge_info() function to something more specific?
On Fri, 28 Jul 2006, Kamesh Jayachandran wrote:
> - }
> >-
> >+ SVN_ERR(index_merge_info(cb, new_rev, pool));
> >
> Fails to cleanup the earlier failed merge commits.
> Attached patch does it.
>
> With regards
> Kamesh Jayachandran
> [[[
> Patch by: Kamesh Jayachandran <ka...@collab.net>
>
> r20893 fails to do a cleanup of some earlier commit with the rev as
> current one.
>
> * subversion/libsvn_fs_fs/fs_fs.c
> (index_merge_info)
> One extra parameter 'contains_merge_info' is added.
> Adds the mergeinfo if the commit has some merge info.
>
> (commit_body): calls index_merge_info unconditionally by passing
> the 'contains_merge_info'.
> ]]]
>
>
> Index: subversion/libsvn_fs_fs/fs_fs.c
> ===================================================================
> --- subversion/libsvn_fs_fs/fs_fs.c (revision 20894)
> +++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
> @@ -4119,7 +4119,7 @@
> current transaction. */
> static svn_error_t *
> index_merge_info(struct commit_baton *cb, svn_revnum_t new_rev,
> - apr_pool_t *pool)
> + svn_boolean_t contains_merge_info, apr_pool_t *pool)
> {
> const char *deletestring;
> fs_txn_data_t *ftd = cb->txn->fsap_data;
> @@ -4143,7 +4143,8 @@
> SVN_ERR(fs_sqlite_exec(ftd->mtd, deletestring, NULL, NULL));
>
> /* Record any merge info from the current transaction. */
> - SVN_ERR(update_mergeinfo_index(cb->txn, new_rev, pool));
> + if (contains_merge_info)
> + SVN_ERR(update_mergeinfo_index(cb->txn, new_rev, pool));
>
> /* This is moved here from commit_txn, because we don't want to
> write the final current file if the sqlite commit fails.
> @@ -4273,8 +4274,7 @@
> SVN_ERR(svn_fs_fs__move_into_place(revprop_filename, final_revprop,
> old_rev_filename, pool));
>
> - if (contains_merge_info)
> - SVN_ERR(index_merge_info(cb, new_rev, pool));
> + SVN_ERR(index_merge_info(cb, new_rev, contains_merge_info, pool));
>
> /* Update the 'current' file. */
> SVN_ERR(write_final_current(cb->fs, cb->txn->id, new_rev, start_node_id,