Re: [PATCH]log and blame command should log the revision range in operational log

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.