At the risk of putting words into other people's mouths, it sounds
like this change got a -0 from garrett, a +0 from ghudson, and nothing
from anyone else. That's okay - it's hardly the most important patch
around :-)
But I'm of the opinion that the current behaviour is a shortcoming in
the peg-rev support for blame, so I'd like to commit this as a bugfix.
So instead, I'll ask: Are there any objections to this patch? If not,
I'll commit in a few days.
Regards,
Malcolm
----- Forwarded message from Malcolm Rowe <malcolm-svn-dev@farside.org.uk> -----
From: Malcolm Rowe <malcolm-svn-dev@farside.org.uk>
Date: Fri, 20 Jan 2006 17:05:06 +0000
Ping. Anyone have any comments on this?
The patch is fairly straightforward; what I'm more interested in is
whether the change is a good idea or not. There seemed to be general
support for consistency when this was discussed before, but no definite
decision on whether our current behaviour was a bug or a feature.
Regards,
Malcolm
----- Forwarded message from Malcolm Rowe <malcolm-svn-dev@farside.org.uk> -----
From: Malcolm Rowe <malcolm-svn-dev@farside.org.uk>
Date: Fri, 13 Jan 2006 16:17:18 +0000
A short while ago [1], Greg pointed out that 'svn blame' was inconsistent
with most of the rest of our command set in requiring an operative
revision to be specified in order to blame an old version of a file even
if a peg revision was specified.
In other words, while
$ svn cat http://svn.collab.net/repos/svn/trunk/COMMITTERS@2753
gives an old version of the COMMITTERS file,
$ svn blame http://svn.collab.net/repos/svn/trunk/COMMITTERS@2753
locates the old file, then blames the current HEAD revision. Instead,
you need to do
$ svn blame http://svn.collab.net/repos/svn/trunk/COMMITTERS@2753 -r2753
Most of our commands (see [2] for a survey) will default the operative
revision to the peg revision, like 'cat' does, so this seems like a bug
that should be fixed, albeit one for which the fix results in a subtle
change to the default semantics (i.e., you'd need to add an explicit
'-rHEAD' if that's what you meant).
A patch (and testcase) is attached for review. Any comments?
[[[
Change the behaviour of 'svn blame' so that the default operative revision
range will be taken from the target's peg revision (if one is provided).
The behaviour is unchanged if no peg revision was provided, or if an
explicit revision range was provided.
* subversion/svn/blame-cmd.c
(svn_cl__blame): Check for a peg revision before determining the
operative end revision (if required), and use that peg revision as
the default operative end revision. If no peg revision is specified,
fall back to HEAD or BASE as before. Also rename local variable
'is_head_or_base' to 'end_revision_unspecified', to better represent
the condition that's being evaluated.
* subversion/tests/cmdline/blame_tests.py
(blame_peg_rev): New test.
(test_list): Add the test.
]]]
Regards,
Malcolm
[1] http://svn.haxx.se/dev/archive-2005-11/1293.shtml
[2] http://svn.haxx.se/dev/archive-2005-11/1301.shtml
Index: subversion/svn/blame-cmd.c
===================================================================
--- subversion/svn/blame-cmd.c (revision 18090)
+++ subversion/svn/blame-cmd.c (working copy)
@@ -171,15 +171,15 @@ svn_cl__blame (apr_getopt_t *os,
{
svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state;
svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
apr_pool_t *subpool;
apr_array_header_t *targets;
blame_baton_t bl;
int i;
- svn_boolean_t is_head_or_base = FALSE;
+ svn_boolean_t end_revision_unspecified = FALSE;
SVN_ERR (svn_opt_args_to_target_array2 (&targets, os,
opt_state->targets, pool));
/* Blame needs a file on which to operate. */
if (! targets->nelts)
return svn_error_create (SVN_ERR_CL_INSUFFICIENT_ARGS, 0, NULL);
@@ -192,15 +192,15 @@ svn_cl__blame (apr_getopt_t *os,
range to be -r1:X. */
opt_state->end_revision = opt_state->start_revision;
opt_state->start_revision.kind = svn_opt_revision_number;
opt_state->start_revision.value.number = 1;
}
else
- is_head_or_base = TRUE;
+ end_revision_unspecified = TRUE;
}
if (opt_state->start_revision.kind == svn_opt_revision_unspecified)
{
opt_state->start_revision.kind = svn_opt_revision_number;
opt_state->start_revision.value.number = 1;
}
@@ -254,25 +254,29 @@ svn_cl__blame (apr_getopt_t *os,
svn_error_t *err;
const char *target = ((const char **) (targets->elts))[i];
const char *truepath;
svn_opt_revision_t peg_revision;
svn_pool_clear (subpool);
SVN_ERR (svn_cl__check_cancel (ctx->cancel_baton));
- if (is_head_or_base)
+
+ /* Check for a peg revision. */
+ SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
+ subpool));
+
+ if (end_revision_unspecified)
{
- if (svn_path_is_url (target))
+ if (peg_revision.kind != svn_opt_revision_unspecified)
+ opt_state->end_revision = peg_revision;
+ else if (svn_path_is_url (target))
opt_state->end_revision.kind = svn_opt_revision_head;
else
opt_state->end_revision.kind = svn_opt_revision_base;
}
- /* Check for a peg revision. */
- SVN_ERR (svn_opt_parse_path (&peg_revision, &truepath, target,
- subpool));
if (opt_state->xml)
{
/* "<target ...>" */
/* We don't output this tag immediately, which avoids creating
a target element if this path is skipped. */
const char *outpath = truepath;
if (! svn_path_is_url (target))
Index: subversion/tests/cmdline/blame_tests.py
===================================================================
--- subversion/tests/cmdline/blame_tests.py (revision 18090)
+++ subversion/tests/cmdline/blame_tests.py (working copy)
@@ -222,25 +222,65 @@ def blame_on_unknown_revision(sbox):
'-rHEAD:HEAD')
if output[0].find(" - This is the file 'iota'.") == -1:
raise svntest.Failure
+# The default blame revision range should be 1:N, where N is the
+# peg-revision of the target, or BASE or HEAD if no peg-revision is
+# specified.
+#
+def blame_peg_rev(sbox):
+ "blame targets with peg-revisions"
+
+ sbox.build()
+
+ expected_output_r1 = [
+ " 1 jrandom This is the file 'iota'.\n" ]
+
+ current_dir = os.getcwd()
+ os.chdir(sbox.wc_dir)
+ try:
+ # Modify iota and commit it (r2).
+ open('iota', 'w').write("This is no longer the file 'iota'.\n")
+
+ expected_output = svntest.wc.State('.', {
+ 'iota' : Item(verb='Sending'),
+ })
+ svntest.actions.run_and_verify_commit('.', expected_output,
+ None, None, None, None,
+ None, None)
+
+ # Check that we get a blame of r1 when we specify a peg revision of r1
+ # and no explicit revision.
+ svntest.actions.run_and_verify_svn(None, expected_output_r1, [],
+ 'blame', 'iota@1')
+
+ # Check that an explicit revision overrides the default provided by
+ # the peg revision.
+ svntest.actions.run_and_verify_svn(None, expected_output_r1, [],
+ 'blame', 'iota@2', '-r1')
+
+ finally:
+ os.chdir(current_dir)
+
+
########################################################################
# Run the tests
# list all tests here, starting with None:
test_list = [ None,
blame_space_in_name,
blame_binary,
blame_directory,
blame_in_xml,
blame_on_unknown_revision,
+ blame_peg_rev,
]
if __name__ == '__main__':
svntest.main.run_tests(test_list)
# NOTREACHED
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Feb 7 19:56:27 2006