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

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