[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Daniel Rall <dlr_at_collab.net>
Date: 2007-02-22 19:14:12 CET

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

  • application/pgp-signature attachment: stored
Received on Thu Feb 22 19:18:27 2007

This is an archived mail posted to the Subversion Dev mailing list.