You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Clint Lawrence <cl...@gmail.com> on 2008/06/14 19:02:23 UTC
Help with issue #3217
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.
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.
Cheers,
Clint
Re: Help with issue #3217
Posted by Karl Fogel <kf...@red-bean.com>.
Clint Lawrence <cl...@gmail.com> writes:
> There were two patches attached. libsvn_client-unversioned-log.diff has
> the relevant changes to the library code. The patch you've review
> includes
> only the changes to the svn client.
Ah. I got fooled by an unfortunate combination of circumstances:
- You wrote a singular where you meant a plural in your original
message ("I've attached two patch that are my first steps...")
- One of the attachment's names ended with "-log", and people often
attach their log message separately (we prefer it in the same
attachment as the patch, but people still sometimes do it
separately). So I assumed the first attachment was a log message,
and then when I saw a log message in the second attachment, I
thought it was just a duplicate.
- There was no explicit statement of dependency between the two
patches.
> I've updated both patches and the log messages to include your
> suggestions.
Thanks. I'll take a look.
When one patch depends on another, please always state that in a very
prominent way, like in the log message of the dependee. There are lots
of patches flying around on this list, so it's easy for reviewers to get
confused :-).
> The only way I can work out to get a suitable MIME type is to send as
> a txt
> file (I using Mail.app if anyone knows how to make it work...) Hope that
> is O.K. (make check passes with both patches applied to trunk)
Sure, .txt is fine (I don't know much about how to use Mail.app, sorry).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Help with issue #3217
Posted by Clint Lawrence <cl...@gmail.com>.
On 15/06/2008, at 3:05 AM, Karl Fogel wrote:
>> 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?
Yes that's what I thought was going on. I'll continuing working in that
direction. I'll have to come up with some configuration item and/or
command line flay to control the boolean flag I suppose.
> 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.
There were two patches attached. libsvn_client-unversioned-log.diff has
the relevant changes to the library code. The patch you've review
includes
only the changes to the svn client.
> 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.
I've updated both patches and the log messages to include your
suggestions.
The only way I can work out to get a suitable MIME type is to send as
a txt
file (I using Mail.app if anyone knows how to make it work...) Hope that
is O.K. (make check passes with both patches applied to trunk)
Thanks,
Clint
Re: Help with issue #3217
Posted by Karl Fogel <kf...@red-bean.com>.
Clint Lawrence <cl...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org