Clint Lawrence <clint.lawrence_at_gmail.com> writes:
> I'd like to have a go at issue 3217, "svn commit should show unknown
> files in editor". I'm slowly trying to find my way around the source
> and work out how to approach it. I've attached two patch that are my
> first steps, but I'd appreciate some feed back to check I'm headed in
> the right direction before going too much further.
First, always post patches as MIME type "text/plain" attachments. It
makes a surprisingly large difference in ease of review.
Also, put the log message and the patch in the same attachment, though
keep the "[[[" and "]]]" as you have them now.
> To start I've modified the log_msg_func callback to accept a new
> argument that will be an array of paths representing unversioned
> items. I've added the necessary logic in libsvn_client and updated the
> command line client's svn_cl__get_log_message so it accepts the list
> of unversioned items and prints them in the log message.
>
> If all that looks O.K. then my next step would be to extend the
> harvest_committables function so it can find any unversioned
> files. Then once that works, add the necessary logic into the library
> to filter out all the items that have been set to be ignored. The part
> I can't work out at the moment is *how* to find the unversioned
> items. When first looking through this I thought it should be easy to
> borrow that logic from the status command (including filtering
> ignores) but on closer inspection it looks like status and commit work
> quite differently. Hints on where to look would be appreciated.
I think I know what the problem is: harvest_committables() is guided
strictly by the entries lists -- that is, it only ever hears about files
that are under version control, or are formally marked for addition into
version control. It's probably never even told about unversioned files.
(A cursory look over the code seems to confirm this.)
Now, this could be changed. Changing it would affect the performance of
harvest_committables(), since it would now have to look at the directory
on disk as well as at the svn entries list. Therefore, you'd probably
want to pass a boolean flag indicating whether or not to discover
unversioned items.
Does that make sense?
Some patch review below...
> [[[
> Working towards issue #3217: svn commit should show unknown files in editor
>
> Modify the log message callback to use the newly created
> svn_client_get_commit_log4_t type. The new callback type
> includes a list of unversioned items that are in the
> working copy that we can now include in the log message
> when calling and external editor.
>
> * subversion/svn/cl.h
> (svn_cl__get_log_message): Add parameter unversioned_items so
> it is now compatible with svn_client_get_commit_log4_t.
>
> * subversion/svn/util.c
> (svn_cl__get_log_message): Modify this callback to be of the
> new svn_client_get_commit_log4_t type. When calling an external
> editor add the list of unversioned_items after the commit_items.
>
> * subversion/svn/main.c
> (main): assign svn_cl__get_log_message to log_msg_func4 rather
> than log_msg_func3.
>
> * subversion/svn/move-cmd.c
> subversion/svn/mkdir-cmd.c
> subversion/svn/copy-cmd.c
> subversion/svn/commit-cmd.c
> subversion/svn/delete-cmd.c
> subversion/svn/import-cmd.c
> subversion/svn/propedit-cmd.c
> Replace use of log_msg_func3 and log_msg_baton3 with the
> new log_msg_func4 and log_msg_baton4.
> ]]]
Er, but there is no 'svn_client_get_commit_log4_t' type... At least, not
in this patch, and not on trunk.
Similarly, neither the log message nor the patch shows log_msg_func4 or
its baton being declared as new structure fields where I'd expect them.
You don't need that final "*" item; you can just note that all callers
were updated at the log entry for the new type.
> --- subversion/svn/cl.h (revision 31731)
> +++ subversion/svn/cl.h (working copy)
> @@ -546,12 +546,13 @@
> apr_hash_t *config,
> apr_pool_t *pool);
>
> -/* A function of type svn_client_get_commit_log3_t. */
> +/* A function of type svn_client_get_commit_log4_t. */
> svn_error_t *svn_cl__get_log_message(const char **log_msg,
> - const char **tmp_file,
> - const apr_array_header_t *commit_items,
> - void *baton,
> - apr_pool_t *pool);
> + const char **tmp_file,
> + const apr_array_header_t *commit_items,
> + const apr_array_header_t *unversioned_items,
> + void *baton,
> + apr_pool_t *pool);
Oops, please don't change the indentation of existing parameters if you
can avoid it. Not only is the new indentation above wrong, but the
extra shifting makes the real diff hard to see.
I'll stop there, as the rest of the patch is quite straightforward.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-15 04:05:39 CEST