You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2005/11/29 23:53:54 UTC
Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
Other reviewers: please could you help with a few questions, right at the end.
Fabien, I've put notes in line about the problems I found, but I decided to
make the changes myself and attach a new patch. I hope that's OK.
Fabien COELHO wrote:
>
> This patch moves the 'svnversion' functionnality to libsvn_wc so that
> it could be used by other clients such as 'svn'.
[...]
> - I let the cancelation feature in although they are not used by svnversion.
> It seems reasonnable that the working copy walking may be canceled?
> - no_ignore and config are removed as they are not used (yet?).
> no_ignore being TRUE does not make sense if no details about
> what is found is reported. As for config, I have no opinion,
> it is easy to put it back so that it may be forwarded downwards.
That's all good.
[...]
> +/** This structure reports the mix of revisions found within
> + * a working copy, including whether some parts are switched
> + * or currently modified. If no working copy is found, an error
> + * is raised, possibly SVN_ERR_WC_PATH_NOT_FOUND if the path
> + * is a directory or SVN_ERR_WC_NOT_DIRECTORY if it is anything else.
This structure doesn't know anything about error codes. I've re-written these
doc strings.
> + */
> +typedef struct svn_wc_revision_status_t
> +{
> + svn_revnum_t min_rev; /* lowest revision found. */
> + svn_revnum_t max_rev; /* highest revision found. */
> +
> + /* is anything ... */
> + svn_boolean_t switched;
> + svn_boolean_t modified; /* any type of modification... */
> +}
> +svn_wc_revision_status_t;
> +
> +/** Compute the revision summary status in the @a result structure
> + * for a working copy at @a wc_path. The @a trail_url is used to
> + * determine if WC_PATH itself is switched. The summary may address the
> + * last changed vs the current revisions if @a committed.
> + * Cancel stuff @a cancal_func and @a cancel_baton is passed downwards,
> + * although they may be NULL.
> + *
> + * @since New in 1.4
> + */
> +svn_error_t *
> +svn_wc_revision_status (svn_wc_revision_status_t *result,
> + const char *wc_path,
> + const char *trail_url,
> + svn_boolean_t committed,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool);
> Index: subversion/libsvn_wc/revision_status.c
> ===================================================================
[...]
> +/* public interface. */
> +svn_error_t *
> +svn_wc_revision_status (svn_wc_revision_status_t *result,
> + const char *wc_path,
> + const char *trail_url,
> + svn_boolean_t committed,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool)
> {
> int wc_format;
> struct status_baton sb;
> + apr_pool_t *subpool;
> + const char *anchor, *target;
> + svn_wc_adm_access_t *anchor_access, *target_access;
> + svn_wc_traversal_info_t *traversal_info;
> + const svn_delta_editor_t *editor;
> + void *edit_baton, *set_locks_baton;
> + svn_revnum_t edit_revision = SVN_INVALID_REVNUM;
No value is needed for "edit_revision".
>
> + /* set result as nil */
> + result->min_rev = SVN_INVALID_REVNUM;
> + result->max_rev = SVN_INVALID_REVNUM;
> + result->switched = FALSE;
> + result->modified = FALSE;
>
> + /* initialize walking stick */
That's amusing. Maybe you know why, but I'll explain anyway. A "walking
stick" is specifically what an old or infirm person uses to help them walk.
For these structures we always say "baton", because a baton is something (OK, a
stick) that is passed on from one person to the next, to the next, in a relay
race. In Subversion it is passed on from one function to the next, to the
next. <http://subversion.tigris.org/faq.html#baton>
> + subpool = svn_pool_create (pool);
> + sb.result = result;
> + sb.committed = committed;
> sb.wc_path = NULL;
> sb.wc_url = NULL;
> - sb.pool = pool;
> + sb.pool = subpool;
I don't think we need a sub-pool here - there's no iteration going on at this
level.
>
> + wc_path = svn_path_internal_style (wc_path, subpool);
Internal style is part of the API conventions, so this line goes in the caller.
>
> + SVN_ERR (svn_wc_check_wc (wc_path, &wc_format, subpool));
>
> if (! wc_format)
> {
> svn_node_kind_t kind;
> + SVN_ERR(svn_io_check_path (wc_path, &kind, subpool));
> + svn_pool_destroy (subpool);
> if (kind == svn_node_dir)
> {
> - SVN_INT_ERR (svn_cmdline_printf (pool, _("exported%s"),
> - no_newline ? "" : "\n"));
> - svn_pool_destroy (pool);
> - return EXIT_SUCCESS;
> + return svn_error_createf(SVN_ERR_WC_PATH_NOT_FOUND, NULL,
> + _("'%s' is exported\n"), wc_path);
> }
> else
> {
> - svn_error_clear
> - (svn_cmdline_fprintf (stderr, pool,
> - _("'%s' not versioned, and not exported\n"),
> - wc_path));
> - svn_pool_destroy (pool);
> - return EXIT_FAILURE;
> + return svn_error_createf(SVN_ERR_WC_NOT_DIRECTORY, NULL,
> + _("'%s' not versioned, and not exported\n"),
> + wc_path);
> }
> }
That whole block (svn_wc_check_wc; if (! wc_format) {...}) should go in the
caller. This function should just return an error if the path isn't a WC, and
it probably doesn't need to check that explicitly because the first WC
operation it attempts will probably return an appropriate error. (I'll try to
check that.)
[...]
> + traversal_info = svn_wc_init_traversal_info (subpool);
>
> + SVN_ERR (svn_wc_adm_open_anchor (&anchor_access, &target_access, &target,
> + wc_path, FALSE, -1,
> + cancel_func, cancel_baton,
> + subpool));
> +
> + anchor = svn_wc_adm_access_path (anchor_access);
"anchor" is not used.
> +
> + SVN_ERR (svn_wc_get_status_editor2 (&editor, &edit_baton, &set_locks_baton,
You can pass NULL instead of "&set_locks_baton", as you don't want it.
> + &edit_revision, anchor_access, target,
> + NULL /* config */,
> + TRUE /* recurse */,
> + TRUE /* get_all */,
> + FALSE /* no_ignore */,
> + analyze_status, &sb,
> + cancel_func, cancel_baton,
> + traversal_info,
You can pass NULL instead of "traversal_info" ... or at least the doc string
says you can, and you will be able to after we fix the crash that presently
results.
> + subpool));
[...]
> Index: subversion/svnversion/main.c
> ===================================================================
[...]
> - wc_path = svn_path_internal_style (wc_path, pool);
That "internal style" conversion should stay here; the "internal style" is used
for paths in APIs (as well as UTF-8).
[...]
> + /* but it was a directory, let us guess it was exported... */
> + SVN_INT_ERR (svn_cmdline_fputs(N_("exported"), stdout, pool));
That should be "_" rather than "N_". It's an internationalisation thing. "_"
is normally used. "N_" is used where a constant result is needed (array
initialisers, etc.).
[...]
OK. I decided to make these changes myself, and am attaching my version "3j"
(for "Julian"). At the same time, I made several other tweaks such as undoing
some changes of layout and style and the like that just caused extra
differences that made the patch bigger (harder to review).
Probably the biggest change I made was in the error handling that deals with a
target that is not versioned, and calls it "exported" if it's a directory. I
put back the code that was there originally, to check this before calling the
new function, and removed all such specific error handling from the new function.
Note: In the svn_wc_get_status_editor2() call I have set traversal_info to
null. That requires a fix such as the one I posted earlier today: "[PATCH] Fix
crash in svn_wc_get_status_editor2".
On my version of Fabien's patch, I would like help on these specific questions:
* Is svn_wc_get_status_editor2() called correctly (with associated "open" and
"close" calls around it)? I'm not familiar enough with it to spot any subtle
problems.
* Is the "trail_url" a reasonable thing to put in an API? It seems a bit ...
hackish. Would it be better to require a full expected URL? Would it be
better to not have the function do that particular processing, but let the
caller do it?
* The patch removes a bit of code from svnversion that appears to have been
saying, "If the user cancels, don't error out, just print whatever results we
have found so far." I think it's fine to remove this, but it's not related to
the purpose of the patch. I think I should do that in a separate patch. Yes?
(The "###" in the log message is only a review-stage reminder.)
- Julian
Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
Posted by Michael W Thelen <mi...@pietdepsi.com>.
Julian Foad wrote:
>> Nope, I've been tracking it but I probably should have filed an issue
>> for it sooner. If no developer comments on it, or responds to the
>> questions Julian posed in his email, I'll go ahead and file an issue for
>> it. Thank you for making sure it wasn't forgotten.
>
> Thanks Michael. Go ahead; it's no trouble for me to close the issue
> when it does get resolved.
No problem, I've filed it as issue #2477:
http://subversion.tigris.org/issues/show_bug.cgi?id=2477
--
Michael W Thelen
It is a mistake to think you can solve any major problems just with
potatoes. -- Douglas Adams
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
Posted by Julian Foad <ju...@btopenworld.com>.
Michael W Thelen wrote:
> Fabien COELHO wrote:
>
>>This patch submission and Julian's update has been lost for the past 6
>>weeks. Is it totally lost or in some queue?
>
> Nope, I've been tracking it but I probably should have filed an issue
> for it sooner. If no developer comments on it, or responds to the
> questions Julian posed in his email, I'll go ahead and file an issue for
> it. Thank you for making sure it wasn't forgotten.
Thanks Michael. Go ahead; it's no trouble for me to close the issue when it
does get resolved.
I repeat the questions here for easy reference; the mail containing them and my
version of the patch is archived at
<http://svn.haxx.se/dev/archive-2005-11/1520.shtml>:
> On my version of Fabien's patch, I would like help on these specific questions:
>
> * Is svn_wc_get_status_editor2() called correctly (with associated "open" and
> "close" calls around it)? I'm not familiar enough with it to spot any subtle
> problems.
>
> * Is the "trail_url" a reasonable thing to put in an API? It seems a bit ...
> hackish. Would it be better to require a full expected URL? Would it be
> better to not have the function do that particular processing, but let the
> caller do it?
>
> * The patch removes a bit of code from svnversion that appears to have been
> saying, "If the user cancels, don't error out, just print whatever results we
> have found so far." I think it's fine to remove this, but it's not related to
> the purpose of the patch. I think I should do that in a separate patch. Yes?
> (The "###" in the log message is only a review-stage reminder.)
If an experienced committer says it looks basically OK, I would be prepared to
commit it without having those questions answered specifically.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
Posted by Michael W Thelen <mi...@pietdepsi.com>.
Fabien COELHO wrote:
>> Subject: Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
>>
>> Other reviewers: please could you help with a few questions, right at
>> the end.
>
> This patch submission and Julian's update has been lost for the past 6
> weeks. Is it totally lost or in some queue?
Nope, I've been tracking it but I probably should have filed an issue
for it sooner. If no developer comments on it, or responds to the
questions Julian posed in his email, I'll go ahead and file an issue for
it. Thank you for making sure it wasn't forgotten.
--
Michael W Thelen
It is a mistake to think you can solve any major problems just with
potatoes. -- Douglas Adams
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
Posted by Fabien COELHO <fa...@coelho.net>.
Dear devs,
> Subject: Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
>
> Other reviewers: please could you help with a few questions, right at the
> end.
This patch submission and Julian's update has been lost for the past 6
weeks. Is it totally lost or in some queue?
Have a nice day,
--
Fabien.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
Posted by Julian Foad <ju...@btopenworld.com>.
I have at last taken another look at this patch and committed it in r18266,
with some doc strings expanded.
About my three questions: I've tracked down the reason for removing code that
traps a "CANCELLED" error (it is redundant code - the situation could not
occur) and committed it separately in r18265. I've left the "trail URL"
parameter in the API. I'll trust that the status editor is used correctly.
I've closed the patch issue #2477.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j
Posted by Fabien COELHO <fa...@coelho.net>.
Dear Julian,
Thanks a lot for all the improvements you suggested.
>> +/** This structure reports the mix of revisions found within
>> ...
>> + * is raised, possibly SVN_ERR_WC_PATH_NOT_FOUND if the path
>> + * is a directory or SVN_ERR_WC_NOT_DIRECTORY if it is anything else.
>
> This structure doesn't know anything about error codes. I've re-written
> these doc strings.
Indeed, I should have moved that in the function documentation.
>> + svn_revnum_t edit_revision = SVN_INVALID_REVNUM;
>
> No value is needed for "edit_revision".
Possibly.
In fact I inlined the lisvn_client function which was called in the
initial 'svnversion' implementation, and then I removed everything that
looked useless, but I kept everything else when I was not sure.
> I don't think we need a sub-pool here - there's no iteration going on at this
> level.
Fine with me.
> Internal style is part of the API conventions, so this line goes in the
> caller.
Oops.
> That whole block (svn_wc_check_wc; if (! wc_format) {...}) should go in the
> caller. This function should just return an error if the path isn't a WC,
> and it probably doesn't need to check that explicitly because the first WC
> operation it attempts will probably return an appropriate error. (I'll try
> to check that.)
Hummm. ISTM that any client that use this function will need to check
whether it is applied on a wc. So the checking code would have to be
replicated everywhere. On the other hand, cheking for the status of
something which is not a wc doest not make much sense. It is just that the
initial svnversion implementation choses to tell that it is "exported"
altough it may not be the case at all. So I followed the current choice,
but I agree with your point that it may not be part of the API.
>> + anchor = svn_wc_adm_access_path (anchor_access);
> "anchor" is not used.
It is again a result of the incomplete inlining/simplification process.
> You can pass NULL instead of "&set_locks_baton", as you don't want it.
Idem.
> You can pass NULL instead of "traversal_info" ... or at least the doc string
> says you can, and you will be able to after we fix the crash that presently
> results.
Idem.
>> + SVN_INT_ERR (svn_cmdline_fputs(N_("exported"), stdout, pool));
>
> That should be "_" rather than "N_". It's an internationalisation thing.
Ok. BTW, I'm not sure that the output should be translated.
> OK. I decided to make these changes myself, and am attaching my version "3j"
> (for "Julian").
Good.
They are all fine with me.
> On my version of Fabien's patch, I would like help on these specific
> questions:
>
> * Is svn_wc_get_status_editor2() called correctly (with associated "open" and
> "close" calls around it)? I'm not familiar enough with it to spot any subtle
> problems.
As the code basis is inlined/simplified from a libwc_client function, it
should be as good as it was there;-)
> * Is the "trail_url" a reasonable thing to put in an API? It seems a bit ...
> hackish. Would it be better to require a full expected URL? Would it be
> better to not have the function do that particular processing, but let the
> caller do it?
I'm not really happy with it as well, but I understand why it is useful.
Idem, ISTM that all caller could have to do the processing, hence it makes
sense to have it within the function, and give a NULL if one is really not
interested in that issue.
--
Fabien.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org