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 2008/08/14 17:02:02 UTC
How wrong can we get a short doc-string?
For our collective shame ...
In libsvn_client/merge.c:
[[[
> /* Get REVISION of the file at URL. SOURCE is a path that refers to that
> file's entry in the working copy, or NULL if we don't have one. Return in
> *FILENAME the name of a file containing the file contents, in *PROPS a hash
> containing the properties and in *REV the revision. All allocation occurs
> in POOL. */
There's no argument named 'REVISION' (it's 'REV').
There's no argument named 'URL', nor anything that looks like one.
There's no argument named 'SOURCE', but two possible alternatives.
There is an argument named 'RA_SESSION' that's not mentioned.
There is an argument named 'WC_TARGET' that's not mentioned.
Bloody hell.
Excuse my language.
> static svn_error_t *
> single_file_merge_get_file(const char **filename,
> svn_ra_session_t *ra_session,
> apr_hash_t **props,
> svn_revnum_t rev,
> const char *wc_target,
> apr_pool_t *pool)
]]]
I'll go and figure out what it's supposed to be, as I think I may want to use it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: How wrong can we get a short doc-string?
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-08-14 at 21:41 +0100, Julian Foad wrote:
> On Thu, 2008-08-14 at 16:31 -0400, Karl Fogel wrote:
> > Julian Foad <ju...@btopenworld.com> writes:
> > > Nothing heard. Committed revision 32483.
[...]
> > One thing that's not clear is what "the file that is at the root URL of
> > RA_SESSION" means. Often, the root URL of an RA_SESSION is a directory,
> > and there are many files that could be in its root; here there is no
> > clear way here to know which of those files the doc string is talking
> > about. If this function requires that RA_SESSION be rooted at a file,
> > then it should say so, and ideally say what error would be returned if
> > RA_SESSION is rooted at a directory.
> >
> > As far as phrasing goes: I find that sometimes it's conceptually clearer
> > to start from the result effects and work backwards. E.g.:
> >
> > /* Set *FILENAME to the path of (and *PROPS to a hash containing the
> > properties of) the file at revision REV in the root url of
> > RA_SESSION. Store the file's text content in a new temporary file
> > in the same directory as the path WC_TARGET. [todo: need to
> > specify whether WC_TARGET can be a directory or not]
> > Do all allocation in POOL. */
> >
> > Thoughts?
Committed an update in r32485, using most of your suggestions.
I didn't mention what errors would be returned as that's not a necessary
part of its contract, but the rest should be much better now.
Thanks.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: How wrong can we get a short doc-string?
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-08-14 at 16:31 -0400, Karl Fogel wrote:
> Julian Foad <ju...@btopenworld.com> writes:
> > Nothing heard. Committed revision 32483.
>
> Sorry -- catching up after r32483:
No problem.
Thanks for the input below. I'll commit an update.
- Julian
> The doc string now reads like this:
>
> /* Get revision REV of the file that is at the root URL of RA_SESSION.
> Store the file's text content in a new temporary file in the same
> directory as the path WC_TARGET. Set *FILENAME to the path to that file.
> Set *PROPS to a hash containing the file's properties.
> All allocation occurs in POOL. */
> /* ### TODO: Create the temporary file under .svn/tmp/ instead of next to
> the working file. */
> static svn_error_t *
> single_file_merge_get_file(const char **filename,
> svn_ra_session_t *ra_session,
> apr_hash_t **props,
> svn_revnum_t rev,
> const char *wc_target,
> apr_pool_t *pool)
>
> One thing that's not clear is what "the file that is at the root URL of
> RA_SESSION" means. Often, the root URL of an RA_SESSION is a directory,
> and there are many files that could be in its root; here there is no
> clear way here to know which of those files the doc string is talking
> about. If this function requires that RA_SESSION be rooted at a file,
> then it should say so, and ideally say what error would be returned if
> RA_SESSION is rooted at a directory.
>
> As far as phrasing goes: I find that sometimes it's conceptually clearer
> to start from the result effects and work backwards. E.g.:
>
> /* Set *FILENAME to the path of (and *PROPS to a hash containing the
> properties of) the file at revision REV in the root url of
> RA_SESSION. Store the file's text content in a new temporary file
> in the same directory as the path WC_TARGET. [todo: need to
> specify whether WC_TARGET can be a directory or not]
> Do all allocation in POOL. */
>
> Thoughts?
>
> -Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: How wrong can we get a short doc-string?
Posted by Karl Fogel <kf...@red-bean.com>.
Julian Foad <ju...@btopenworld.com> writes:
> Nothing heard. Committed revision 32483.
Sorry -- catching up after r32483:
The doc string now reads like this:
/* Get revision REV of the file that is at the root URL of RA_SESSION.
Store the file's text content in a new temporary file in the same
directory as the path WC_TARGET. Set *FILENAME to the path to that file.
Set *PROPS to a hash containing the file's properties.
All allocation occurs in POOL. */
/* ### TODO: Create the temporary file under .svn/tmp/ instead of next to
the working file. */
static svn_error_t *
single_file_merge_get_file(const char **filename,
svn_ra_session_t *ra_session,
apr_hash_t **props,
svn_revnum_t rev,
const char *wc_target,
apr_pool_t *pool)
One thing that's not clear is what "the file that is at the root URL of
RA_SESSION" means. Often, the root URL of an RA_SESSION is a directory,
and there are many files that could be in its root; here there is no
clear way here to know which of those files the doc string is talking
about. If this function requires that RA_SESSION be rooted at a file,
then it should say so, and ideally say what error would be returned if
RA_SESSION is rooted at a directory.
As far as phrasing goes: I find that sometimes it's conceptually clearer
to start from the result effects and work backwards. E.g.:
/* Set *FILENAME to the path of (and *PROPS to a hash containing the
properties of) the file at revision REV in the root url of
RA_SESSION. Store the file's text content in a new temporary file
in the same directory as the path WC_TARGET. [todo: need to
specify whether WC_TARGET can be a directory or not]
Do all allocation in POOL. */
Thoughts?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: How wrong can we get a short doc-string?
Posted by Julian Foad <ju...@btopenworld.com>.
On Thu, 2008-08-14 at 19:01 +0100, Julian Foad wrote:
> How's this?
>
> [[[
> >
> > -/* Get REVISION of the file at URL. SOURCE is a path that refers to that
> > - file's entry in the working copy, or NULL if we don't have one. Return in
> > - *FILENAME the name of a file containing the file contents, in *PROPS a hash
> > - containing the properties and in *REV the revision. All allocation occurs
> > - in POOL. */
> > +/* Get revision REV of the file that is at the root URL of RA_SESSION.
> > + Store the file's text content in a new temporary file in the same
> > + directory as the path WC_TARGET. Set *FILENAME to the path to that file.
> > + Set *PROPS to a hash containing the file's properties.
> > + All allocation occurs in POOL. */
> > static svn_error_t *
> > single_file_merge_get_file(const char **filename,
> > svn_ra_session_t *ra_session,
> > @@ -3227,7 +3320,7 @@
> > apr_file_t *fp;
> > svn_stream_t *stream;
> >
> > - /* ### Create this temporary file under .svn/tmp/ instead of next to
> > + /* ### TODO: Create this temporary file under .svn/tmp/ instead of next to
> > ### the working file.*/
> > SVN_ERR(svn_io_open_unique_file2(&fp, filename,
> > wc_target, ".tmp",
> ]]]
Nothing heard. Committed revision 32483.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: How wrong can we get a short doc-string?
Posted by Julian Foad <ju...@btopenworld.com>.
How's this?
[[[
>
> -/* Get REVISION of the file at URL. SOURCE is a path that refers to that
> - file's entry in the working copy, or NULL if we don't have one. Return in
> - *FILENAME the name of a file containing the file contents, in *PROPS a hash
> - containing the properties and in *REV the revision. All allocation occurs
> - in POOL. */
> +/* Get revision REV of the file that is at the root URL of RA_SESSION.
> + Store the file's text content in a new temporary file in the same
> + directory as the path WC_TARGET. Set *FILENAME to the path to that file.
> + Set *PROPS to a hash containing the file's properties.
> + All allocation occurs in POOL. */
> static svn_error_t *
> single_file_merge_get_file(const char **filename,
> svn_ra_session_t *ra_session,
> @@ -3227,7 +3320,7 @@
> apr_file_t *fp;
> svn_stream_t *stream;
>
> - /* ### Create this temporary file under .svn/tmp/ instead of next to
> + /* ### TODO: Create this temporary file under .svn/tmp/ instead of next to
> ### the working file.*/
> SVN_ERR(svn_io_open_unique_file2(&fp, filename,
> wc_target, ".tmp",
]]]
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: How wrong can we get a short doc-string?
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Aug 14, 2008 at 06:02:02PM +0100, Julian Foad wrote:
> For our collective shame ...
>
> In libsvn_client/merge.c:
> [[[
> > /* Get REVISION of the file at URL. SOURCE is a path that refers to that
> > file's entry in the working copy, or NULL if we don't have one. Return in
> > *FILENAME the name of a file containing the file contents, in *PROPS a hash
> > containing the properties and in *REV the revision. All allocation occurs
> > in POOL. */
>
> There's no argument named 'REVISION' (it's 'REV').
> There's no argument named 'URL', nor anything that looks like one.
> There's no argument named 'SOURCE', but two possible alternatives.
> There is an argument named 'RA_SESSION' that's not mentioned.
> There is an argument named 'WC_TARGET' that's not mentioned.
Could this possibly have been an auto-merge on update or such
that went wrong? Or did someone pick the wrong docstring by
accident while resolving a conflict or something?
Note that this function used to be in a different file once:
------------------------------------------------------------------------
r24045 | lundblad | 2007-03-23 14:57:41 +0100 (Fri, 23 Mar 2007) | 21 lines
Split a file into two.
Suggested by: sanity
* subversion/libsvn_client/diff.c
(svn_client_diff_summarize_dup): Move to "public APIs' section.
(ENSURE_VALID_REVISION_KINDS, check_scheme_match,
svn_client__dry_run_deletions, dry_run_deleted_p, merge_props_changed,
merge_file_changed, merge_file_added, merge_file_deleted, merge_dir_added,
merge_delete_notify_baton_t, merge_delete_notify_func, merge_dir_deleted,
merge_callbacks, get_wc_merge_info, get_wc_and_repos_merge_info,
calculate_merge_ranges, notification_receiver_baton_t, notification_receiver,
default_conflict_resolver, determine_merges_performed,
update_wc_merge_info, merge_type, grok_range_info_from_opt_revisions,
do_merge, single_file_merge_get_file, do_single_file_merge,
discover_and_merge_children, svn_client_merge3, svn_client_merge2,
svn_client_merge, svn_client_merge_peg3, svn_client_merge_peg2,
svn_client_merge_peg): Move to...
* subversion/libsvn_client/merge.c: ... this new file.
------------------------------------------------------------------------
> Bloody hell.
Hey, no need to swear.
> Excuse my language.
Hey, no need to to excuse yourself.
:)
> > static svn_error_t *
> > single_file_merge_get_file(const char **filename,
> > svn_ra_session_t *ra_session,
> > apr_hash_t **props,
> > svn_revnum_t rev,
> > const char *wc_target,
> > apr_pool_t *pool)
> ]]]
>
> I'll go and figure out what it's supposed to be, as I think I may want to use it.
Good luck digging! Don't forget to take at least 5 health potions,
a sword, and of course a shovel.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org