You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2006/01/13 16:17:18 UTC
[PATCH] blame: Make default revision follow peg revision
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
Re: [PATCH] blame: Make default revision follow peg revision
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2006-01-20 at 16:35 -0800, Garrett Rooney wrote:
> I'm totally in favor of consistency, but I'm not sure this sort of
> change is appropriate due to compat issues. On the other hand, I
> think it's rather unlikely that people are relying on the current
> behavior in scripts or something, so maybe we can just say the current
> behavior is a bug... It's a tough call.
We've made changes of greater magnitude related to peg revs before
(including adding peg-rev support in the first place) and it hasn't been
a problem in practice.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] blame: Make default revision follow peg revision
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/20/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> 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.
I'm totally in favor of consistency, but I'm not sure this sort of
change is appropriate due to compat issues. On the other hand, I
think it's rather unlikely that people are relying on the current
behavior in scripts or something, so maybe we can just say the current
behavior is a bug... It's a tough call.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] blame: Make default revision follow peg revision
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 2/7/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> 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.
Honestly, I'm leaning towards +0, I think it's a slight compat issue,
but I'm willing to let myself be convinced that it's a bug fix instead
;-)
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] blame: Make default revision follow peg revision
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Feb 07, 2006 at 09:54:46PM +0000, Julian Foad wrote:
> Malcolm Rowe wrote:
> >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.
>
> +1, because we should have consistency between commands in the absence of a
> good reason not to.
>
Ok, done, r18400. Thanks for the review.
> (Ideally, it would be "to comply with our user interface general
> specifications", but we haven't written them down. :-( )
>
Yup. :-( for sure.
> >+ svntest.actions.run_and_verify_commit('.', expected_output,
> >+ None, None, None, None,
> >+ None, None)
>
> The last five 'None's are optional so can be removed.
>
Ah, thanks. I had tried something like that, but I think I removed them
all by mistake (and one of them isn't optional). I'm still pretty much
at the 'beginner/cargo-cult' stage with Python.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] blame: Make default revision follow peg revision
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
>
> 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.
+1, because we should have consistency between commands in the absence of a
good reason not to.
> [[[
> 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.
Please say here why you're changing it, i.e. "for consistency with most of the
other commands."
(Ideally, it would be "to comply with our user interface general
specifications", but we haven't written them down. :-( )
>
> * subversion/svn/blame-cmd.c
[...]
> Index: subversion/tests/cmdline/blame_tests.py
> ===================================================================
[...]
> + expected_output = svntest.wc.State('.', {
> + 'iota' : Item(verb='Sending'),
> + })
> + svntest.actions.run_and_verify_commit('.', expected_output,
> + None, None, None, None,
> + None, None)
The last five 'None's are optional so can be removed.
From Greg's email that you linked to:
> Operative rev defaults to peg rev:
> cat
> export
> info
> ls
> propget
> proplist
>
> Operative rev defaults to HEAD:
> blame
> co (bug: @pegrev not trimmed off default wc pathname)
> diff (second operative rev defaults to HEAD)
>
> Does not appear to support peg revs even though it could:
> cp
> log
> switch
"log" has since moved to the first list, and this moves "blame" to the first list.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] blame: Make default revision follow peg revision
Posted by Malcolm Rowe <ma...@farside.org.uk>.
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 <ma...@farside.org.uk> -----
From: Malcolm Rowe <ma...@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 <ma...@farside.org.uk> -----
From: Malcolm Rowe <ma...@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
Re: [PATCH] blame: Make default revision follow peg revision
Posted by Malcolm Rowe <ma...@farside.org.uk>.
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 <ma...@farside.org.uk> -----
From: Malcolm Rowe <ma...@farside.org.uk>
To: dev@subversion.tigris.org
Subject: [PATCH] blame: Make default revision follow peg revision
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
----- End forwarded message -----
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org