You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2013/06/12 12:58:58 UTC
Re: svn commit: r1492134 -
/subversion/trunk/subversion/tests/cmdline/blame_tests.py
danielsh@apache.org wrote on Wed, Jun 12, 2013 at 10:29:35 -0000:
> Author: danielsh
> Date: Wed Jun 12 10:29:34 2013
> New Revision: 1492134
>
> URL: http://svn.apache.org/r1492134
> Log:
> Add an XFail test for 'svn blame -r 3:1' (where 3 > 1).
>
> Not filing an issue since I plan to commit a fix soon.
>
Update: Bert found a potential problem with peg revisions here that
might require revving an RA API, so it'll take me longer to get to
committing a fix than I planned.
Current WIP patch:
Index: subversion/libsvn_client/blame.c
===================================================================
--- subversion/libsvn_client/blame.c (revision 1492120)
+++ subversion/libsvn_client/blame.c (working copy)
@@ -466,7 +466,7 @@ file_rev_handler(void *baton, const char *path, sv
/* Create the rev structure. */
frb->rev = apr_pcalloc(frb->mainpool, sizeof(struct rev));
- if (revnum < frb->start_rev)
+ if (revnum < MIN(frb->start_rev, frb->end_rev))
{
/* We shouldn't get more than one revision before the starting
revision (unless of including merged revisions). */
@@ -479,7 +479,8 @@ file_rev_handler(void *baton, const char *path, sv
}
else
{
- SVN_ERR_ASSERT(revnum <= frb->end_rev);
+ /* 1+ for the "youngest to oldest" blame */
+ SVN_ERR_ASSERT(revnum <= 1 + MAX(frb->end_rev, frb->start_rev));
/* Set values from revision props. */
frb->rev->revision = revnum;
@@ -578,6 +579,7 @@ svn_client_blame5(const char *target,
svn_stream_t *last_stream;
svn_stream_t *stream;
const char *target_abspath_or_url;
+ svn_revnum_t youngest;
if (start->kind == svn_opt_revision_unspecified
|| end->kind == svn_opt_revision_unspecified)
@@ -599,11 +601,6 @@ svn_client_blame5(const char *target,
target_abspath_or_url, ra_session,
start, pool));
- if (end_revnum < start_revnum)
- return svn_error_create
- (SVN_ERR_CLIENT_BAD_REVISION, NULL,
- _("Start revision must precede end revision"));
-
/* We check the mime-type of the yougest revision before getting all
the older revisions. */
if (!ignore_mime_type)
@@ -674,8 +671,14 @@ svn_client_blame5(const char *target,
We need to ensure that we get one revision before the start_rev,
if available so that we can know what was actually changed in the start
revision. */
+ SVN_ERR(svn_ra_get_latest_revnum(ra_session, &youngest, frb.currpool));
SVN_ERR(svn_ra_get_file_revs2(ra_session, "",
- start_revnum - (start_revnum > 0 ? 1 : 0),
+ start_revnum
+ - (0 < start_revnum && start_revnum < end_revnum ? 1 : 0)
+ + (youngest > start_revnum && start_revnum > end_revnum ? 1 : 0),
+ /* ### pass MAX(start_revnum, end_revnum) as peg thru to svn_fs_revision_root() */
+ /* ### blame 5 fails. */
+ /* ### check svn_repos_get_file_revs2 tests. */
end_revnum, include_merged_revisions,
file_rev_handler, &frb, pool));
> * subversion/tests/cmdline/blame_tests.py
> (blame_youngest_to_oldest): New test, XFail.
> (test_list): Run it.
Re: svn commit: r1492134 -
/subversion/trunk/subversion/tests/cmdline/blame_tests.py
Posted by Daniel Shahaf <da...@elego.de>.
Julian Foad wrote on Wed, Jun 12, 2013 at 12:22:31 +0100:
> Daniel Shahaf wrote:
>
> > danielsh@apache.org wrote on Wed, Jun 12, 2013 at 10:29:35 -0000:
> >> Author: danielsh
> >> Date: Wed Jun 12 10:29:34 2013
> >> New Revision: 1492134
> >>
> >> URL: http://svn.apache.org/r1492134
> >> Log:
> >> Add an XFail test for 'svn blame -r 3:1' (where 3 > 1).
> >>
> >> Not filing an issue since I plan to commit a fix soon.
>
> What behaviour change are you planning for 'blame'?
>
> I ask because I don't think it's obvious.
>
> For example, it would be useful to have a mode that looks forward in
> time from a given revision, to find the *next* revision at which each
> line is changed after the starting revision. Is that the behaviour
> you are planning?
>
What I was thinking was to allow blame to consider the sequence of
revisions in opposite order: instead of considering the oldest revision
the first one in the chain, so `svn blame` output prints the youngest
revision that touched a given line, allow 'svn blame' (when invoked as
-r M:N with M<N, which is currently an error) to consider the _youngest_
revision as the first one in the chain, so that `svn blame` output will
print the _oldest_ revision that touched a given line.
In other words: 'svn blame -r 3:1 iota' will, for each line in iota@1,
print the oldest revision which changed or removed that line.
Does that make sense?
Thanks for asking,
Daniel
> - Julian
>
>
> > Update: Bert found a potential problem with peg revisions here that
> > might require revving an RA API, so it'll take me longer to get to
> > committing a fix than I planned.
> >
> > Current WIP patch:
> >
> > Index: subversion/libsvn_client/blame.c
> > ===================================================================
> > --- subversion/libsvn_client/blame.c (revision 1492120)
> > +++ subversion/libsvn_client/blame.c (working copy)
> > @@ -466,7 +466,7 @@ file_rev_handler(void *baton, const char *path, sv
> > /* Create the rev structure. */
> > frb->rev = apr_pcalloc(frb->mainpool, sizeof(struct rev));
> >
> > - if (revnum < frb->start_rev)
> > + if (revnum < MIN(frb->start_rev, frb->end_rev))
> > {
> > /* We shouldn't get more than one revision before the starting
> > revision (unless of including merged revisions). */
> > @@ -479,7 +479,8 @@ file_rev_handler(void *baton, const char *path, sv
> > }
> > else
> > {
> > - SVN_ERR_ASSERT(revnum <= frb->end_rev);
> > + /* 1+ for the "youngest to oldest" blame */
> > + SVN_ERR_ASSERT(revnum <= 1 + MAX(frb->end_rev, frb->start_rev));
> >
> > /* Set values from revision props. */
> > frb->rev->revision = revnum;
> > @@ -578,6 +579,7 @@ svn_client_blame5(const char *target,
> > svn_stream_t *last_stream;
> > svn_stream_t *stream;
> > const char *target_abspath_or_url;
> > + svn_revnum_t youngest;
> >
> > if (start->kind == svn_opt_revision_unspecified
> > || end->kind == svn_opt_revision_unspecified)
> > @@ -599,11 +601,6 @@ svn_client_blame5(const char *target,
> > target_abspath_or_url, ra_session,
> > start, pool));
> >
> > - if (end_revnum < start_revnum)
> > - return svn_error_create
> > - (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> > - _("Start revision must precede end revision"));
> > -
> > /* We check the mime-type of the yougest revision before getting all
> > the older revisions. */
> > if (!ignore_mime_type)
> > @@ -674,8 +671,14 @@ svn_client_blame5(const char *target,
> > We need to ensure that we get one revision before the start_rev,
> > if available so that we can know what was actually changed in the start
> > revision. */
> > + SVN_ERR(svn_ra_get_latest_revnum(ra_session, &youngest, frb.currpool));
> > SVN_ERR(svn_ra_get_file_revs2(ra_session, "",
> > - start_revnum - (start_revnum > 0 ? 1 : 0),
> > + start_revnum
> > + - (0 < start_revnum && start_revnum
> > < end_revnum ? 1 : 0)
> > + + (youngest > start_revnum &&
> > start_revnum > end_revnum ? 1 : 0),
> > + /* ### pass MAX(start_revnum, end_revnum) as
> > peg thru to svn_fs_revision_root() */
> > + /* ### blame 5 fails. */
> > + /* ### check svn_repos_get_file_revs2 tests. */
> > end_revnum, include_merged_revisions,
> > file_rev_handler, &frb, pool));
> >
> >
> >> * subversion/tests/cmdline/blame_tests.py
> >> (blame_youngest_to_oldest): New test, XFail.
> >> (test_list): Run it.
> >
Re: svn commit: r1492134 -
/subversion/trunk/subversion/tests/cmdline/blame_tests.py
Posted by Daniel Shahaf <da...@elego.de>.
Julian Foad wrote on Wed, Jun 12, 2013 at 12:22:31 +0100:
> Daniel Shahaf wrote:
>
> > danielsh@apache.org wrote on Wed, Jun 12, 2013 at 10:29:35 -0000:
> >> Author: danielsh
> >> Date: Wed Jun 12 10:29:34 2013
> >> New Revision: 1492134
> >>
> >> URL: http://svn.apache.org/r1492134
> >> Log:
> >> Add an XFail test for 'svn blame -r 3:1' (where 3 > 1).
> >>
> >> Not filing an issue since I plan to commit a fix soon.
>
> What behaviour change are you planning for 'blame'?
>
> I ask because I don't think it's obvious.
>
> For example, it would be useful to have a mode that looks forward in
> time from a given revision, to find the *next* revision at which each
> line is changed after the starting revision. Is that the behaviour
> you are planning?
>
What I was thinking was to allow blame to consider the sequence of
revisions in opposite order: instead of considering the oldest revision
the first one in the chain, so `svn blame` output prints the youngest
revision that touched a given line, allow 'svn blame' (when invoked as
-r M:N with M<N, which is currently an error) to consider the _youngest_
revision as the first one in the chain, so that `svn blame` output will
print the _oldest_ revision that touched a given line.
In other words: 'svn blame -r 3:1 iota' will, for each line in iota@1,
print the oldest revision which changed or removed that line.
Does that make sense?
Thanks for asking,
Daniel
> - Julian
>
>
> > Update: Bert found a potential problem with peg revisions here that
> > might require revving an RA API, so it'll take me longer to get to
> > committing a fix than I planned.
> >
> > Current WIP patch:
> >
> > Index: subversion/libsvn_client/blame.c
> > ===================================================================
> > --- subversion/libsvn_client/blame.c (revision 1492120)
> > +++ subversion/libsvn_client/blame.c (working copy)
> > @@ -466,7 +466,7 @@ file_rev_handler(void *baton, const char *path, sv
> > /* Create the rev structure. */
> > frb->rev = apr_pcalloc(frb->mainpool, sizeof(struct rev));
> >
> > - if (revnum < frb->start_rev)
> > + if (revnum < MIN(frb->start_rev, frb->end_rev))
> > {
> > /* We shouldn't get more than one revision before the starting
> > revision (unless of including merged revisions). */
> > @@ -479,7 +479,8 @@ file_rev_handler(void *baton, const char *path, sv
> > }
> > else
> > {
> > - SVN_ERR_ASSERT(revnum <= frb->end_rev);
> > + /* 1+ for the "youngest to oldest" blame */
> > + SVN_ERR_ASSERT(revnum <= 1 + MAX(frb->end_rev, frb->start_rev));
> >
> > /* Set values from revision props. */
> > frb->rev->revision = revnum;
> > @@ -578,6 +579,7 @@ svn_client_blame5(const char *target,
> > svn_stream_t *last_stream;
> > svn_stream_t *stream;
> > const char *target_abspath_or_url;
> > + svn_revnum_t youngest;
> >
> > if (start->kind == svn_opt_revision_unspecified
> > || end->kind == svn_opt_revision_unspecified)
> > @@ -599,11 +601,6 @@ svn_client_blame5(const char *target,
> > target_abspath_or_url, ra_session,
> > start, pool));
> >
> > - if (end_revnum < start_revnum)
> > - return svn_error_create
> > - (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> > - _("Start revision must precede end revision"));
> > -
> > /* We check the mime-type of the yougest revision before getting all
> > the older revisions. */
> > if (!ignore_mime_type)
> > @@ -674,8 +671,14 @@ svn_client_blame5(const char *target,
> > We need to ensure that we get one revision before the start_rev,
> > if available so that we can know what was actually changed in the start
> > revision. */
> > + SVN_ERR(svn_ra_get_latest_revnum(ra_session, &youngest, frb.currpool));
> > SVN_ERR(svn_ra_get_file_revs2(ra_session, "",
> > - start_revnum - (start_revnum > 0 ? 1 : 0),
> > + start_revnum
> > + - (0 < start_revnum && start_revnum
> > < end_revnum ? 1 : 0)
> > + + (youngest > start_revnum &&
> > start_revnum > end_revnum ? 1 : 0),
> > + /* ### pass MAX(start_revnum, end_revnum) as
> > peg thru to svn_fs_revision_root() */
> > + /* ### blame 5 fails. */
> > + /* ### check svn_repos_get_file_revs2 tests. */
> > end_revnum, include_merged_revisions,
> > file_rev_handler, &frb, pool));
> >
> >
> >> * subversion/tests/cmdline/blame_tests.py
> >> (blame_youngest_to_oldest): New test, XFail.
> >> (test_list): Run it.
> >
Re: svn commit: r1492134 - /subversion/trunk/subversion/tests/cmdline/blame_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> danielsh@apache.org wrote on Wed, Jun 12, 2013 at 10:29:35 -0000:
>> Author: danielsh
>> Date: Wed Jun 12 10:29:34 2013
>> New Revision: 1492134
>>
>> URL: http://svn.apache.org/r1492134
>> Log:
>> Add an XFail test for 'svn blame -r 3:1' (where 3 > 1).
>>
>> Not filing an issue since I plan to commit a fix soon.
What behaviour change are you planning for 'blame'?
I ask because I don't think it's obvious.
For example, it would be useful to have a mode that looks forward in time from a given revision, to find the *next* revision at which each line is changed after the starting revision. Is that the behaviour you are planning?
- Julian
> Update: Bert found a potential problem with peg revisions here that
> might require revving an RA API, so it'll take me longer to get to
> committing a fix than I planned.
>
> Current WIP patch:
>
> Index: subversion/libsvn_client/blame.c
> ===================================================================
> --- subversion/libsvn_client/blame.c (revision 1492120)
> +++ subversion/libsvn_client/blame.c (working copy)
> @@ -466,7 +466,7 @@ file_rev_handler(void *baton, const char *path, sv
> /* Create the rev structure. */
> frb->rev = apr_pcalloc(frb->mainpool, sizeof(struct rev));
>
> - if (revnum < frb->start_rev)
> + if (revnum < MIN(frb->start_rev, frb->end_rev))
> {
> /* We shouldn't get more than one revision before the starting
> revision (unless of including merged revisions). */
> @@ -479,7 +479,8 @@ file_rev_handler(void *baton, const char *path, sv
> }
> else
> {
> - SVN_ERR_ASSERT(revnum <= frb->end_rev);
> + /* 1+ for the "youngest to oldest" blame */
> + SVN_ERR_ASSERT(revnum <= 1 + MAX(frb->end_rev, frb->start_rev));
>
> /* Set values from revision props. */
> frb->rev->revision = revnum;
> @@ -578,6 +579,7 @@ svn_client_blame5(const char *target,
> svn_stream_t *last_stream;
> svn_stream_t *stream;
> const char *target_abspath_or_url;
> + svn_revnum_t youngest;
>
> if (start->kind == svn_opt_revision_unspecified
> || end->kind == svn_opt_revision_unspecified)
> @@ -599,11 +601,6 @@ svn_client_blame5(const char *target,
> target_abspath_or_url, ra_session,
> start, pool));
>
> - if (end_revnum < start_revnum)
> - return svn_error_create
> - (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> - _("Start revision must precede end revision"));
> -
> /* We check the mime-type of the yougest revision before getting all
> the older revisions. */
> if (!ignore_mime_type)
> @@ -674,8 +671,14 @@ svn_client_blame5(const char *target,
> We need to ensure that we get one revision before the start_rev,
> if available so that we can know what was actually changed in the start
> revision. */
> + SVN_ERR(svn_ra_get_latest_revnum(ra_session, &youngest, frb.currpool));
> SVN_ERR(svn_ra_get_file_revs2(ra_session, "",
> - start_revnum - (start_revnum > 0 ? 1 : 0),
> + start_revnum
> + - (0 < start_revnum && start_revnum
> < end_revnum ? 1 : 0)
> + + (youngest > start_revnum &&
> start_revnum > end_revnum ? 1 : 0),
> + /* ### pass MAX(start_revnum, end_revnum) as
> peg thru to svn_fs_revision_root() */
> + /* ### blame 5 fails. */
> + /* ### check svn_repos_get_file_revs2 tests. */
> end_revnum, include_merged_revisions,
> file_rev_handler, &frb, pool));
>
>
>> * subversion/tests/cmdline/blame_tests.py
>> (blame_youngest_to_oldest): New test, XFail.
>> (test_list): Run it.
>
Re: svn commit: r1492134 - /subversion/trunk/subversion/tests/cmdline/blame_tests.py
Posted by Julian Foad <ju...@btopenworld.com>.
Daniel Shahaf wrote:
> danielsh@apache.org wrote on Wed, Jun 12, 2013 at 10:29:35 -0000:
>> Author: danielsh
>> Date: Wed Jun 12 10:29:34 2013
>> New Revision: 1492134
>>
>> URL: http://svn.apache.org/r1492134
>> Log:
>> Add an XFail test for 'svn blame -r 3:1' (where 3 > 1).
>>
>> Not filing an issue since I plan to commit a fix soon.
What behaviour change are you planning for 'blame'?
I ask because I don't think it's obvious.
For example, it would be useful to have a mode that looks forward in time from a given revision, to find the *next* revision at which each line is changed after the starting revision. Is that the behaviour you are planning?
- Julian
> Update: Bert found a potential problem with peg revisions here that
> might require revving an RA API, so it'll take me longer to get to
> committing a fix than I planned.
>
> Current WIP patch:
>
> Index: subversion/libsvn_client/blame.c
> ===================================================================
> --- subversion/libsvn_client/blame.c (revision 1492120)
> +++ subversion/libsvn_client/blame.c (working copy)
> @@ -466,7 +466,7 @@ file_rev_handler(void *baton, const char *path, sv
> /* Create the rev structure. */
> frb->rev = apr_pcalloc(frb->mainpool, sizeof(struct rev));
>
> - if (revnum < frb->start_rev)
> + if (revnum < MIN(frb->start_rev, frb->end_rev))
> {
> /* We shouldn't get more than one revision before the starting
> revision (unless of including merged revisions). */
> @@ -479,7 +479,8 @@ file_rev_handler(void *baton, const char *path, sv
> }
> else
> {
> - SVN_ERR_ASSERT(revnum <= frb->end_rev);
> + /* 1+ for the "youngest to oldest" blame */
> + SVN_ERR_ASSERT(revnum <= 1 + MAX(frb->end_rev, frb->start_rev));
>
> /* Set values from revision props. */
> frb->rev->revision = revnum;
> @@ -578,6 +579,7 @@ svn_client_blame5(const char *target,
> svn_stream_t *last_stream;
> svn_stream_t *stream;
> const char *target_abspath_or_url;
> + svn_revnum_t youngest;
>
> if (start->kind == svn_opt_revision_unspecified
> || end->kind == svn_opt_revision_unspecified)
> @@ -599,11 +601,6 @@ svn_client_blame5(const char *target,
> target_abspath_or_url, ra_session,
> start, pool));
>
> - if (end_revnum < start_revnum)
> - return svn_error_create
> - (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> - _("Start revision must precede end revision"));
> -
> /* We check the mime-type of the yougest revision before getting all
> the older revisions. */
> if (!ignore_mime_type)
> @@ -674,8 +671,14 @@ svn_client_blame5(const char *target,
> We need to ensure that we get one revision before the start_rev,
> if available so that we can know what was actually changed in the start
> revision. */
> + SVN_ERR(svn_ra_get_latest_revnum(ra_session, &youngest, frb.currpool));
> SVN_ERR(svn_ra_get_file_revs2(ra_session, "",
> - start_revnum - (start_revnum > 0 ? 1 : 0),
> + start_revnum
> + - (0 < start_revnum && start_revnum
> < end_revnum ? 1 : 0)
> + + (youngest > start_revnum &&
> start_revnum > end_revnum ? 1 : 0),
> + /* ### pass MAX(start_revnum, end_revnum) as
> peg thru to svn_fs_revision_root() */
> + /* ### blame 5 fails. */
> + /* ### check svn_repos_get_file_revs2 tests. */
> end_revnum, include_merged_revisions,
> file_rev_handler, &frb, pool));
>
>
>> * subversion/tests/cmdline/blame_tests.py
>> (blame_youngest_to_oldest): New test, XFail.
>> (test_list): Run it.
>