Re: [PATCH] move 'svnversion' functionnality in libsvn_wc v3j

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