You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2007/02/20 09:15:22 UTC
[PATCH]log and blame command should log the revision range in operational
log
Hi All,
Find the attached patch and log.
with regards
Kamesh Jayachandran
Re: [PATCH]log and blame command should log the revision range in operational log
Posted by Daniel Rall <dl...@collab.net>.
On Fri, 02 Mar 2007, Kamesh Jayachandran wrote:
>
> >Thanks Kamesh! I was just looking at the "ops" log generated by
> >log_tests.py over ra_serf, and noticed that we're logging r0 quite
> >often (usually for the end revision, and sometimes for both
> >revisions).
> svn log url
> implicitly results in rHEAD:0.(0 exclusive).
>
> Regarding r0:0 it is our log_tests 3 (log_with_empty_repos).
>
> > Looking at the code, it looks like this is the value
> >we're probably using for the 'log' operation. It just seemed a little
> >strange to me when looking at the operational logging output, so I
> >wanted to double-check that this is
>
> >a) expected
> Yes.
> >and b) desirable.
> >
> Yes.
Understood, thanks Kamesh. Committed to trunk in r23576.
- Dan
Re: [PATCH]log and blame command should log the revision range in
operational log
Posted by Kamesh Jayachandran <ka...@collab.net>.
> Thanks Kamesh! I was just looking at the "ops" log generated by
> log_tests.py over ra_serf, and noticed that we're logging r0 quite
> often (usually for the end revision, and sometimes for both
> revisions).
svn log url
implicitly results in rHEAD:0.(0 exclusive).
Regarding r0:0 it is our log_tests 3 (log_with_empty_repos).
> Looking at the code, it looks like this is the value
> we're probably using for the 'log' operation. It just seemed a little
> strange to me when looking at the operational logging output, so I
> wanted to double-check that this is
> a) expected
Yes.
> and b) desirable.
>
Yes.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]log and blame command should log the revision range in operational log
Posted by Daniel Rall <dl...@collab.net>.
On Mon, 26 Feb 2007, Kamesh Jayachandran wrote:
>
> Dan,
>
> >Why do we call the action "log-all" instead of just "log"? It'd also
> >be nice if we had a better name for "log-partial", if we do want to
> >use a different name to indicate that it's a log of multiple paths
> >instead of just 1. I'd personally be in favor of calling them both
> >"log", for simplicity's sake -- we now have the paths listed, which
> >can be used to differentiate between single-path and multi-path
> >invocations of 'log'.
> >
> >I'm attaching a patch which implements my thoughts -- let me know
> >what'cha think.
> >
> >...
> >
>
> Your patch looks precise and neat.
>
> My +1.
Thanks Kamesh! I was just looking at the "ops" log generated by
log_tests.py over ra_serf, and noticed that we're logging r0 quite
often (usually for the end revision, and sometimes for both
revisions). Looking at the code, it looks like this is the value
we're probably using for the 'log' operation. It just seemed a little
strange to me when looking at the operational logging output, so I
wanted to double-check that this is a) expected and b) desirable.
[27/Feb/2007:13:35:36 -0800] jrandom log '/' r9:0
[27/Feb/2007:13:35:44 -0800] jrandom log '/' r0:0
[27/Feb/2007:13:35:48 -0800] jrandom log '/A/D/H' r2:2
[27/Feb/2007:13:35:52 -0800] jrandom log '/' r1:0
[27/Feb/2007:13:36:13 -0800] jrandom log '/' r9:9
[27/Feb/2007:13:36:13 -0800] jrandom log '/' r9:9
[27/Feb/2007:13:36:13 -0800] jrandom log '/' r9:9
[27/Feb/2007:13:36:13 -0800] jrandom log '/' r8:8
[27/Feb/2007:13:36:35 -0800] jrandom log '/A/D/G, /A/D/H' r9:0
[27/Feb/2007:13:36:55 -0800] jrandom log '/A/B/E/beta' r8:0
[27/Feb/2007:13:37:14 -0800] jrandom log '/A/B/E/alpha' r8:0
[27/Feb/2007:13:37:25 -0800] jrandom log '/A/mu2' r6:0
[27/Feb/2007:13:37:25 -0800] jrandom log '/A/mu2' r6:0
[27/Feb/2007:13:37:25 -0800] jrandom log '/A/mu2' r3:3
[27/Feb/2007:13:37:25 -0800] jrandom log '/A/mu2' r3:3
[27/Feb/2007:13:37:25 -0800] jrandom log '/A/mu' r2:2
[27/Feb/2007:13:37:25 -0800] jrandom log '/A/mu' r2:2
[27/Feb/2007:13:37:26 -0800] jrandom log '/' r1:0
[27/Feb/2007:13:37:29 -0800] jrandom log '/' r1:1
[27/Feb/2007:13:37:29 -0800] jrandom log '/' r1:1
[27/Feb/2007:13:37:48 -0800] jrandom log '/' r9:0
[27/Feb/2007:13:37:48 -0800] jrandom log '/A/B' r9:0
[27/Feb/2007:13:37:48 -0800] jrandom log '/A/B' r2:9
[27/Feb/2007:13:37:48 -0800] jrandom log '/A/B' r1:1
[27/Feb/2007:13:38:08 -0800] jrandom log '/A/B/E/beta' r9:0
[27/Feb/2007:13:38:09 -0800] jrandom log '/A/B/E/beta' r1:0
Re: [PATCH]log and blame command should log the revision range in
operational log
Posted by Kamesh Jayachandran <ka...@collab.net>.
Dan,
> Why do we call the action "log-all" instead of just "log"? It'd also
> be nice if we had a better name for "log-partial", if we do want to
> use a different name to indicate that it's a log of multiple paths
> instead of just 1. I'd personally be in favor of calling them both
> "log", for simplicity's sake -- we now have the paths listed, which
> can be used to differentiate between single-path and multi-path
> invocations of 'log'.
>
> I'm attaching a patch which implements my thoughts -- let me know
> what'cha think.
>
> ...
>
Your patch looks precise and neat.
My +1.
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH]log and blame command should log the revision range in operational log
Posted by Daniel Rall <dl...@collab.net>.
On Sat, 24 Feb 2007, Kamesh Jayachandran wrote:
> >That's a good question. When initially reviewing your patch, I found
> >the fact that there are 3 cases here somewhat suspect myself. I have
> >a feeling that the "log" and "log-all" cases should be collapsed
> >together into a single "log" action. Something like:
>
> > if (paths->nelts > 1)
> > action log-partial
> > else
> > action log
>
> done.
Hmm, after looking at the surrounding code a bit more closely, I see
that we still need to handle the case where no <path> elements are
provided in the DAV request.
The existing behavior of logging *something* for this case seems
pretty reasonable, but it would be nice if what we logged indicated
that no paths were provided (e.g. "log-nothing" or "log-no-op" some
such, or just an empty path).
> >Okay. I'm in favor of logging all paths, but think that change should
> >be part of the same change set which adds logging of paths for the
> >other case(s).
>
> Implemented that too in this patch.
Great!
...
> --- subversion/mod_dav_svn/reports/log.c (revision 23461)
> +++ subversion/mod_dav_svn/reports/log.c (working copy)
> @@ -219,6 +219,7 @@
> svn_boolean_t strict_node_history = FALSE; /* off by default */
> apr_array_header_t *paths
> = apr_array_make(resource->pool, 0, sizeof(const char *));
> + const char *comma_separated_paths = NULL;
>
> /* Sanity check. */
> ns = dav_svn__find_ns(doc->namespaces, SVN_XML_NAMESPACE);
> @@ -258,6 +259,23 @@
> target = svn_path_join(resource->info->repos_path, rel_path,
> resource->pool);
> APR_ARRAY_PUSH(paths, const char *) = target;
> + if (comma_separated_paths)
> + {
> + comma_separated_paths = apr_pstrcat(resource->pool,
> + comma_separated_paths,
> + ",",
> + svn_path_uri_encode(target,
> + resource->pool),
> + NULL);
> + }
> + else
> + {
> + comma_separated_paths = apr_pstrcat(resource->pool,
> + svn_path_uri_encode(target,
> + resource->pool),
> + NULL);
> + }
Hmm, isn't this going to use quite a bit of memory? I think we'd be
better off using a svn_stringbuf_t.
> +
Spurious newline.
> }
> /* else unknown element; skip it */
> }
> @@ -317,18 +335,19 @@
> cleanup:
>
> /* We've detected a 'high level' svn action to log. */
> - if (paths->nelts == 0)
> - action = "log";
> - else if (paths->nelts == 1)
> - action = apr_psprintf(resource->pool, "log-all '%s'",
> - svn_path_uri_encode(APR_ARRAY_IDX
> - (paths, 0, const char *),
> - resource->pool));
> + if (paths->nelts > 1)
> + action = apr_psprintf(resource->pool, "log-partial '%s' " \
> + "r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> + comma_separated_paths,
> + start, end);
> else
> - action = apr_psprintf(resource->pool, "log-partial '%s'",
> + action = apr_psprintf(resource->pool, "log-all '%s' r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> svn_path_uri_encode(APR_ARRAY_IDX
> (paths, 0, const char *),
Since we already built comma_separated_paths, why not use it here as
well? That allows us to do away with this if/else conditional.
> - resource->pool));
> + resource->pool),
> + start, end);
Why do we call the action "log-all" instead of just "log"? It'd also
be nice if we had a better name for "log-partial", if we do want to
use a different name to indicate that it's a log of multiple paths
instead of just 1. I'd personally be in favor of calling them both
"log", for simplicity's sake -- we now have the paths listed, which
can be used to differentiate between single-path and multi-path
invocations of 'log'.
I'm attaching a patch which implements my thoughts -- let me know
what'cha think.
...
> --- subversion/mod_dav_svn/reports/file-revs.c (revision 23461)
> +++ subversion/mod_dav_svn/reports/file-revs.c (working copy)
> @@ -311,8 +311,10 @@
>
> /* We've detected a 'high level' svn action to log. */
> apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION",
> - apr_psprintf(resource->pool, "blame '%s'",
> - svn_path_uri_encode(path, resource->pool)));
> + apr_psprintf(resource->pool, "blame '%s' r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> + svn_path_uri_encode(path, resource->pool),
> + start, end));
...
I committed the 'blame' portion of the patch in r23488.
RE: [PATCH]log and blame command should log the revision range in operational log
Posted by Kamesh Jayachandran <ka...@collab.net>.
Hi,
>That's a good question. When initially reviewing your patch, I found
>the fact that there are 3 cases here somewhat suspect myself. I have
>a feeling that the "log" and "log-all" cases should be collapsed
>together into a single "log" action. Something like:
> if (paths->nelts > 1)
> action log-partial
> else
> action log
done.
>Okay. I'm in favor of logging all paths, but think that change should
>be part of the same change set which adds logging of paths for the
>other case(s).
Implemented that too in this patch.
Find the attached patch and log.
Thanks for the review.
With regards
Kamesh Jayachandran
Re: [PATCH]log and blame command should log the revision range in operational log
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 22 Feb 2007, Kamesh Jayachandran wrote:
> >> Index: subversion/mod_dav_svn/reports/log.c
> >> ===================================================================
> >> --- subversion/mod_dav_svn/reports/log.c (revision 23436)
> >> +++ subversion/mod_dav_svn/reports/log.c (working copy)
> >> @@ -320,15 +320,20 @@
> >> if (paths->nelts == 0)
> >> action = "log";
>
> >Why don't we log the revision range for this case?
>
> I overlooked it. Out of curiosity tried to see when 'paths->nelts'
> is zero. To my surprise paths->nelts will always be non-zero on
> trunk atleast. Is it a dead code?
That's a good question. When initially reviewing your patch, I found
the fact that there are 3 cases here somewhat suspect myself. I have
a feeling that the "log" and "log-all" cases should be collapsed
together into a single "log" action. Something like:
if (paths->nelts > 1)
action log-partial
else
action log
> >> else if (paths->nelts == 1)
> >> - action = apr_psprintf(resource->pool, "log-all '%s'",
> >> + action = apr_psprintf(resource->pool, "log-all '%s' r%" SVN_REVNUM_T_FMT \
> >> + ":%" SVN_REVNUM_T_FMT,
> >> svn_path_uri_encode(APR_ARRAY_IDX
> >> (paths, 0, const char *),
> >> - resource->pool));
> >> + resource->pool),
> >> + start, end);
> >> else
> >> - action = apr_psprintf(resource->pool, "log-partial '%s'",
> >> + action = apr_psprintf(resource->pool, "log-partial '%s' " \
> >> + "r%" SVN_REVNUM_T_FMT \
> >> + ":%" SVN_REVNUM_T_FMT,
> >> svn_path_uri_encode(APR_ARRAY_IDX
> >> (paths, 0, const char *),
> >> - resource->pool));
> >> + resource->pool),
> >> + start, end);
>
> >The only difference between these blocks now are the "log-all"
> >vs. "log-partial" identifiers. These two blocks could be collapsed
> >into one which contains an inline if/else statement which selects
> >which fragment of action text to use.
>
> 'log-partial' portion has one defect it just logs the first path not
> all paths, thought of doing in the next commit.
Okay. I'm in favor of logging all paths, but think that change should
be part of the same change set which adds logging of paths for the
other case(s).
- Dan
RE: [PATCH]log and blame command should log the revision range in operational log
Posted by Kamesh Jayachandran <ka...@collab.net>.
>> Index: subversion/mod_dav_svn/reports/log.c
>> ===================================================================
>> --- subversion/mod_dav_svn/reports/log.c (revision 23436)
>> +++ subversion/mod_dav_svn/reports/log.c (working copy)
>> @@ -320,15 +320,20 @@
>> if (paths->nelts == 0)
>> action = "log";
>Why don't we log the revision range for this case?
I overlooked it. Out of curiosity tried to see when 'paths->nelts' is zero. To my surprise paths->nelts will always be non-zero on trunk atleast. Is it a dead code?
>> else if (paths->nelts == 1)
>> - action = apr_psprintf(resource->pool, "log-all '%s'",
>> + action = apr_psprintf(resource->pool, "log-all '%s' r%" SVN_REVNUM_T_FMT \
>> + ":%" SVN_REVNUM_T_FMT,
>> svn_path_uri_encode(APR_ARRAY_IDX
>> (paths, 0, const char *),
>> - resource->pool));
>> + resource->pool),
>> + start, end);
>> else
>> - action = apr_psprintf(resource->pool, "log-partial '%s'",
>> + action = apr_psprintf(resource->pool, "log-partial '%s' " \
>> + "r%" SVN_REVNUM_T_FMT \
>> + ":%" SVN_REVNUM_T_FMT,
>> svn_path_uri_encode(APR_ARRAY_IDX
>> (paths, 0, const char *),
>> - resource->pool));
>> + resource->pool),
>> + start, end);
>The only difference between these blocks now are the "log-all"
>vs. "log-partial" identifiers. These two blocks could be collapsed
>into one which contains an inline if/else statement which selects
>which fragment of action text to use.
'log-partial' portion has one defect it just logs the first path not all paths, thought of doing in the next commit.
Thanks
With regards
Kamesh Jayachanadran
Re: [PATCH]log and blame command should log the revision range in operational log
Posted by Daniel Rall <dl...@collab.net>.
I'm conceptually in favor of this patch, but think the implementation
could be improved.
On Tue, 20 Feb 2007, Kamesh Jayachandran wrote:
...
> [[[
> 'log' and 'blame' commands need to log the revision range.
Better written as:
Log the revision range for 'log' and 'blame' operations.
> * subversion/mod_dav_svn/reports/log.c
> (dav_svn__log_report): Log revision range.
> * subversion/mod_dav_svn/reports/file-revs.c
> (dav_svn__file_revs_report): Log revision range for 'blame' command.
>
> Patch by: kameshj
> Suggested by: Honey George <ge...@collab.net>
> ]]]
> Index: subversion/mod_dav_svn/reports/log.c
> ===================================================================
> --- subversion/mod_dav_svn/reports/log.c (revision 23436)
> +++ subversion/mod_dav_svn/reports/log.c (working copy)
> @@ -320,15 +320,20 @@
> if (paths->nelts == 0)
> action = "log";
Why don't we log the revision range for this case?
> else if (paths->nelts == 1)
> - action = apr_psprintf(resource->pool, "log-all '%s'",
> + action = apr_psprintf(resource->pool, "log-all '%s' r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> svn_path_uri_encode(APR_ARRAY_IDX
> (paths, 0, const char *),
> - resource->pool));
> + resource->pool),
> + start, end);
> else
> - action = apr_psprintf(resource->pool, "log-partial '%s'",
> + action = apr_psprintf(resource->pool, "log-partial '%s' " \
> + "r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> svn_path_uri_encode(APR_ARRAY_IDX
> (paths, 0, const char *),
> - resource->pool));
> + resource->pool),
> + start, end);
The only difference between these blocks now are the "log-all"
vs. "log-partial" identifiers. These two blocks could be collapsed
into one which contains an inline if/else statement which selects
which fragment of action text to use.
>
> apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION", action);
>
> Index: subversion/mod_dav_svn/reports/file-revs.c
> ===================================================================
> --- subversion/mod_dav_svn/reports/file-revs.c (revision 23436)
> +++ subversion/mod_dav_svn/reports/file-revs.c (working copy)
> @@ -311,8 +311,10 @@
>
> /* We've detected a 'high level' svn action to log. */
> apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION",
> - apr_psprintf(resource->pool, "blame '%s'",
> - svn_path_uri_encode(path, resource->pool)));
> + apr_psprintf(resource->pool, "blame '%s' r%" SVN_REVNUM_T_FMT \
> + ":%" SVN_REVNUM_T_FMT,
> + svn_path_uri_encode(path, resource->pool),
> + start, end));
>
> /* Flush the contents of the brigade (returning an error only if we
> don't already have one). */
Looks good.