You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2005/04/10 22:21:42 UTC
Re: svn commit: r14081 - trunk/subversion/libsvn_client
jszakmeister@tigris.org writes:
> Author: jszakmeister
> Date: Sun Apr 10 13:21:56 2005
> New Revision: 14081
>
> Modified:
> trunk/subversion/libsvn_client/cat.c
> Log:
> Fix Issue #1361: svn cat -rBASE contacts the repository. This also prepares
> us for supporting running 'svn cat' at revision WORKING, if we can come to a
> consensus that it should be the default. :-)
> * subversion/libsvn_client/cat.c
> (cat_local_file): New.
>
> (svn_client_cat2): Stop contacting the repository to cat a file in the
> working copy.
>
>
>
> Modified: trunk/subversion/libsvn_client/cat.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_client/cat.c?view=diff&rev=14081&p1=trunk/subversion/libsvn_client/cat.c&r1=14080&p2=trunk/subversion/libsvn_client/cat.c&r2=14081
> ==============================================================================
> --- trunk/subversion/libsvn_client/cat.c (original)
> +++ trunk/subversion/libsvn_client/cat.c Sun Apr 10 13:21:56 2005
> @@ -37,6 +37,106 @@
>
> /*** Code. ***/
>
Please document new functions.
> +static svn_error_t *
> +cat_local_file (const char *path,
> + svn_stream_t *output,
> + svn_wc_adm_access_t *adm_access,
> + const svn_opt_revision_t *revision,
> + apr_pool_t *pool)
> +{
> + const svn_wc_entry_t *entry;
> + svn_subst_keywords_t kw = { 0 };
> + svn_subst_eol_style_t style;
> + apr_hash_t *props;
> + const char *base;
> + svn_string_t *eol_style, *keywords, *special;
> + const char *eol = NULL;
> + svn_boolean_t local_mod = FALSE;
> + apr_time_t tm;
> + apr_file_t *input_file;
> + svn_stream_t *input;
> +
> + SVN_ERR (svn_wc_entry (&entry, path, adm_access, FALSE, pool));
> +
> + if (entry->kind != svn_node_file)
entry could be NULL
> + return svn_error_createf(SVN_ERR_CLIENT_IS_DIRECTORY, NULL,
> + _("'%s' refers to a directory"), path);
> +
> + if (revision->kind != svn_opt_revision_working)
> + {
> + SVN_ERR (svn_wc_get_pristine_copy_path (path, &base, pool));
> + SVN_ERR (svn_wc_get_prop_diffs (NULL, &props, path, adm_access, pool));
> + }
> + else
> + {
> + svn_wc_status2_t *status;
> +
> + base = path;
> + SVN_ERR (svn_wc_prop_list (&props, path, adm_access, pool));
> + SVN_ERR (svn_wc_status2 (&status, path, adm_access, pool));
> + if (status->text_status != svn_wc_status_normal)
> + local_mod = TRUE;
> + }
Should cat_local_file follow svn:special symbolic links?
> +
> + eol_style = apr_hash_get (props, SVN_PROP_EOL_STYLE,
> + APR_HASH_KEY_STRING);
> + keywords = apr_hash_get (props, SVN_PROP_KEYWORDS,
> + APR_HASH_KEY_STRING);
> + special = apr_hash_get (props, SVN_PROP_SPECIAL,
> + APR_HASH_KEY_STRING);
> +
> + if (eol_style)
> + svn_subst_eol_style_from_value (&style, &eol, eol_style->data);
> +
> + if (local_mod && (! special))
> + {
> + /* Use the modified time from the working copy if
> + the file */
> + SVN_ERR (svn_io_file_affected_time (&tm, path, pool));
> + }
> + else
> + {
> + tm = entry->cmt_date;
> + }
> +
> + if (keywords)
> + {
> + const char *fmt;
> + const char *author;
> +
> + if (local_mod)
> + {
> + /* For locally modified files, we'll append an 'M'
> + to the revision number, and set the author to
> + "(local)" since we can't always determine the
> + current user's username */
> + fmt = "%ldM";
> + author = _("(local)");
I think you are inventing stuff that will never appear in a committed
file, and it means that cat_local_file output won't match the working
file. Is that a good idea?
> + }
> + else
> + {
> + fmt = "%ld";
> + author = entry->cmt_author;
> + }
> +
> + SVN_ERR (svn_subst_build_keywords
> + (&kw, keywords->data,
> + apr_psprintf (pool, fmt, entry->cmt_rev),
> + entry->url, tm, author, pool));
> + }
> +
> + SVN_ERR (svn_io_file_open (&input_file, base,
> + APR_READ, APR_OS_DEFAULT, pool));
> + input = svn_stream_from_aprfile (input_file, pool);
> +
> + SVN_ERR (svn_subst_translate_stream (input, output, eol, FALSE, &kw, TRUE));
> +
> + SVN_ERR (svn_stream_close (input));
> + SVN_ERR (svn_io_file_close (input_file, pool));
> +
> + return SVN_NO_ERROR;
> +}
> +
> svn_error_t *
> svn_client_cat2 (svn_stream_t *out,
> const char *path_or_url,
> @@ -52,6 +152,27 @@
> svn_string_t *keywords;
> apr_hash_t *props;
> const char *url;
> +
> + if (! svn_path_is_url (path_or_url)
> + && (peg_revision->kind == svn_opt_revision_base
> + || peg_revision->kind == svn_opt_revision_committed
> + || peg_revision->kind == svn_opt_revision_unspecified)
> + && (revision->kind == svn_opt_revision_base
> + || revision->kind == svn_opt_revision_committed
> + || revision->kind == svn_opt_revision_unspecified))
> + {
> + svn_wc_adm_access_t *adm_access;
> +
> + SVN_ERR (svn_wc_adm_probe_open3 (&adm_access, NULL, path_or_url, FALSE,
> + 0, ctx->cancel_func, ctx->cancel_baton,
> + pool));
I'd prefer svn_wc_adm_open3 on the svn_path_dirname rather than
svn_wc_adm_probe_open3 on the path. It seems silly to allow a
directory to be opened when a file is required.
> +
> + SVN_ERR (cat_local_file (path_or_url, out, adm_access, revision, pool));
> +
> + SVN_ERR (svn_wc_adm_close (adm_access));
> +
> + return SVN_NO_ERROR;
> + }
>
> /* Get an RA plugin for this filesystem object. */
> SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14081 - trunk/subversion/libsvn_client
Posted by John Szakmeister <jo...@szakmeister.net>.
On Sunday 10 April 2005 18:21, Philip Martin wrote:
> jszakmeister@tigris.org writes:
> > Author: jszakmeister
> > Date: Sun Apr 10 13:21:56 2005
> > New Revision: 14081
> >
> > Modified:
> > trunk/subversion/libsvn_client/cat.c
> > Log:
> > Fix Issue #1361: svn cat -rBASE contacts the repository. This also
> > prepares us for supporting running 'svn cat' at revision WORKING, if
> > we can come to a consensus that it should be the default. :-)
> > * subversion/libsvn_client/cat.c
> > (cat_local_file): New.
> >
> > (svn_client_cat2): Stop contacting the repository to cat a file in
> > the working copy.
> >
> >
> >
> > Modified: trunk/subversion/libsvn_client/cat.c
> > Url:
> > http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_client/cat.
> >c?view=diff&rev=14081&p1=trunk/subversion/libsvn_client/cat.c&r1=14080
> >&p2=trunk/subversion/libsvn_client/cat.c&r2=14081
> > =====================================================================
> >========= --- trunk/subversion/libsvn_client/cat.c (original)
> > +++ trunk/subversion/libsvn_client/cat.c Sun Apr 10 13:21:56 2005
> > @@ -37,6 +37,106 @@
> >
> > /*** Code. ***/
>
> Please document new functions.
Will do.
> > +static svn_error_t *
> > +cat_local_file (const char *path,
> > + svn_stream_t *output,
> > + svn_wc_adm_access_t *adm_access,
> > + const svn_opt_revision_t *revision,
> > + apr_pool_t *pool)
> > +{
> > + const svn_wc_entry_t *entry;
> > + svn_subst_keywords_t kw = { 0 };
> > + svn_subst_eol_style_t style;
> > + apr_hash_t *props;
> > + const char *base;
> > + svn_string_t *eol_style, *keywords, *special;
> > + const char *eol = NULL;
> > + svn_boolean_t local_mod = FALSE;
> > + apr_time_t tm;
> > + apr_file_t *input_file;
> > + svn_stream_t *input;
> > +
> > + SVN_ERR (svn_wc_entry (&entry, path, adm_access, FALSE, pool));
> > +
> > + if (entry->kind != svn_node_file)
>
> entry could be NULL
Yep, Julian caught that. I'm actually putting together some cat tests now
so this doesn't slip by again. I somehow missed it from a patch I made a
little while ago.
> > + return svn_error_createf(SVN_ERR_CLIENT_IS_DIRECTORY, NULL,
> > + _("'%s' refers to a directory"), path);
> > +
> > + if (revision->kind != svn_opt_revision_working)
> > + {
> > + SVN_ERR (svn_wc_get_pristine_copy_path (path, &base, pool));
> > + SVN_ERR (svn_wc_get_prop_diffs (NULL, &props, path,
> > adm_access, pool)); + }
> > + else
> > + {
> > + svn_wc_status2_t *status;
> > +
> > + base = path;
> > + SVN_ERR (svn_wc_prop_list (&props, path, adm_access, pool));
> > + SVN_ERR (svn_wc_status2 (&status, path, adm_access, pool));
> > + if (status->text_status != svn_wc_status_normal)
> > + local_mod = TRUE;
> > + }
>
> Should cat_local_file follow svn:special symbolic links?
Good question... I'll look into it.
> > +
> > + eol_style = apr_hash_get (props, SVN_PROP_EOL_STYLE,
> > + APR_HASH_KEY_STRING);
> > + keywords = apr_hash_get (props, SVN_PROP_KEYWORDS,
> > + APR_HASH_KEY_STRING);
> > + special = apr_hash_get (props, SVN_PROP_SPECIAL,
> > + APR_HASH_KEY_STRING);
> > +
> > + if (eol_style)
> > + svn_subst_eol_style_from_value (&style, &eol, eol_style->data);
> > +
> > + if (local_mod && (! special))
> > + {
> > + /* Use the modified time from the working copy if
> > + the file */
> > + SVN_ERR (svn_io_file_affected_time (&tm, path, pool));
> > + }
> > + else
> > + {
> > + tm = entry->cmt_date;
> > + }
> > +
> > + if (keywords)
> > + {
> > + const char *fmt;
> > + const char *author;
> > +
> > + if (local_mod)
> > + {
> > + /* For locally modified files, we'll append an 'M'
> > + to the revision number, and set the author to
> > + "(local)" since we can't always determine the
> > + current user's username */
> > + fmt = "%ldM";
> > + author = _("(local)");
>
> I think you are inventing stuff that will never appear in a committed
> file, and it means that cat_local_file output won't match the working
> file. Is that a good idea?
Well, we have a bit of an unresolved issue... mainly, that we've never
tackled issue #1191 and decided whether or not we should support 'svn
cat' for rWORKING, as well as several other commands for that matter. If
you're uncomfortable with it there, I can remove it and only support the
BASE revision... as that's really the only revision allowed to get
through right now anyways (rWORKING is unsupported).
All of that said, I believe this should match the export working copy
case, and that is what we decided to do there.
[snip]
> > +
> > + SVN_ERR (svn_wc_adm_probe_open3 (&adm_access, NULL,
> > path_or_url, FALSE, + 0,
> > ctx->cancel_func, ctx->cancel_baton, +
> > pool));
>
> I'd prefer svn_wc_adm_open3 on the svn_path_dirname rather than
> svn_wc_adm_probe_open3 on the path. It seems silly to allow a
> directory to be opened when a file is required.
Will fix.
> > +
> > + SVN_ERR (cat_local_file (path_or_url, out, adm_access,
> > revision, pool)); +
> > + SVN_ERR (svn_wc_adm_close (adm_access));
> > +
> > + return SVN_NO_ERROR;
> > + }
> >
> > /* Get an RA plugin for this filesystem object. */
> > SVN_ERR (svn_client__ra_session_from_path (&ra_session, &rev,
Thanks for the review Philip.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14081 - trunk/subversion/libsvn_client
Posted by John Szakmeister <jo...@szakmeister.net>.
On Monday 11 April 2005 01:55, Peter N. Lundblad wrote:
> On Sun, 10 Apr 2005, Josh Pieper wrote:
> > John Szakmeister wrote:
> > > On Sunday 10 April 2005 18:21, Philip Martin wrote:
> > > [snip]
> > >
> > > > Should cat_local_file follow svn:special symbolic links?
> > >
> > > I suppose it should. I have a patch attached with all of the
> > > fixes, including support for resolving the symbolic link. However,
> > > I'm not sure
>
> ...
>
> > I am not sure that we want 'svn cat' to resolve symlinks at all. In
> > other user-visible output related to the special files (diff), the
> > contents of the repository-normal special file are used as is. This
> > has the advantage that it works in the face of symlinks pointing to
> > directories, broken symlinks, and platforms that don't support them
> > at all. Is there a clear consensus to actually resolve the symlinks
> > when cat'ing?
>
> No. What happens when you svn cat -rX, where is not base or working? If
> it is a link to something in the repository, one might expect it to use
> te same revision. I think it is better to leave this alone. I see this
> change as an optimization, not a semantic change.
I'll revise the patch and commit then.
-John
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14081 - trunk/subversion/libsvn_client
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Sun, 10 Apr 2005, Josh Pieper wrote:
> John Szakmeister wrote:
> > On Sunday 10 April 2005 18:21, Philip Martin wrote:
> > [snip]
> > > Should cat_local_file follow svn:special symbolic links?
> >
> > I suppose it should. I have a patch attached with all of the fixes,
> > including support for resolving the symbolic link. However, I'm not sure
...
> I am not sure that we want 'svn cat' to resolve symlinks at all. In
> other user-visible output related to the special files (diff), the
> contents of the repository-normal special file are used as is. This
> has the advantage that it works in the face of symlinks pointing to
> directories, broken symlinks, and platforms that don't support them at
> all. Is there a clear consensus to actually resolve the symlinks when
> cat'ing?
>
No. What happens when you svn cat -rX, where is not base or working? If it
is a link to something in the repository, one might expect it to use te
same revision. I think it is better to leave this alone. I see this change
as an optimization, not a semantic change.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14081 - trunk/subversion/libsvn_client
Posted by Josh Pieper <jj...@pobox.com>.
John Szakmeister wrote:
> On Sunday 10 April 2005 18:21, Philip Martin wrote:
> [snip]
> > Should cat_local_file follow svn:special symbolic links?
>
> I suppose it should. I have a patch attached with all of the fixes,
> including support for resolving the symbolic link. However, I'm not sure
> it's entirely correct. Here's the relevant snippet:
> if (special)
> {
> svn_string_t *resolved_path;
> SVN_ERR (svn_io_read_link (&resolved_path, path, pool));
> path = resolved_path->data;
> }
>
> My question is do I have to now go back an get an access baton for the new
> resolved path? I suppose I should probably check the resolved path and
> make sure it's a file too, or can I be certain that it points to a file?
>
> Thanks for the help.
I am not sure that we want 'svn cat' to resolve symlinks at all. In
other user-visible output related to the special files (diff), the
contents of the repository-normal special file are used as is. This
has the advantage that it works in the face of symlinks pointing to
directories, broken symlinks, and platforms that don't support them at
all. Is there a clear consensus to actually resolve the symlinks when
cat'ing?
-Josh
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r14081 - trunk/subversion/libsvn_client
Posted by John Szakmeister <jo...@szakmeister.net>.
On Sunday 10 April 2005 18:21, Philip Martin wrote:
[snip]
> Should cat_local_file follow svn:special symbolic links?
I suppose it should. I have a patch attached with all of the fixes,
including support for resolving the symbolic link. However, I'm not sure
it's entirely correct. Here's the relevant snippet:
if (special)
{
svn_string_t *resolved_path;
SVN_ERR (svn_io_read_link (&resolved_path, path, pool));
path = resolved_path->data;
}
My question is do I have to now go back an get an access baton for the new
resolved path? I suppose I should probably check the resolved path and
make sure it's a file too, or can I be certain that it points to a file?
Thanks for the help.
-John