{"thread":{"id":"10795","subject":"[PATCH 0/3] git-svn log fixes","startedAt":"2007-11-11T06:10:32Z","lastAt":"2007-11-18T00:36:45Z","messageCount":11,"participants":["David D Kilzer","Benoit Sigoure","David D. Kilzer","Eric Wong","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"59272","messageId":"1194761435-7286-1-git-send-email-ddkilzer@kilzer.net","threadId":"10795","inReplyTo":null,"subject":"[PATCH 0/3] git-svn log fixes","fromName":"David D Kilzer","fromEmail":"ddkilzer@kilzer.net","sentAt":"2007-11-11T06:10:32Z","receivedAt":"2007-11-11T06:10:32Z","isPatch":true,"sender":{"key":"ddkilzer@kilzer.net","avatar":"https://avatars.githubusercontent.com/u/263571?v=4"},"body":"This patch series contains varies fixes for the \"git-svn log\" command:\n\n[PATCH 1/3] git-svn log: fix ascending revision ranges\n[PATCH 2/3] git-svn log: include commit log for the smallest revision in a range\n[PATCH 3/3] git-svn log: handle unreachable revisions like \"svn log\"\n\nI am looking for feedback since this is my first significant patch series\nfor git.  (I posted a couple of documentation fixes and Makefile tweaks\npreviously.)\n\nThanks!\n\nDave\n--\n git-svn.perl           |   60 ++++++++++++++++++++++++------------\n t/t9116-git-svn-log.sh |   80 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 120 insertions(+), 20 deletions(-)\n--\n"},{"id":"59275","messageId":"1194761435-7286-2-git-send-email-ddkilzer@kilzer.net","threadId":"10795","inReplyTo":"1194761435-7286-1-git-send-email-ddkilzer@kilzer.net","subject":"[PATCH 1/3] git-svn log: fix ascending revision ranges","fromName":"David D Kilzer","fromEmail":"ddkilzer@kilzer.net","sentAt":"2007-11-11T06:10:33Z","receivedAt":"2007-11-11T06:10:33Z","isPatch":true,"sender":{"key":"ddkilzer@kilzer.net","avatar":"https://avatars.githubusercontent.com/u/263571?v=4"},"body":"Fixed typo in Git::SVN::Log::git_svn_log_cmd().  Previously a command like\n\"git-svn log -r1:4\" would only show a commit log separator.\n\nAdded tests for ascending and descending revision ranges.\n\nSigned-off-by: David D Kilzer <ddkilzer@kilzer.net>\n---\n git-svn.perl           |    2 +-\n t/t9116-git-svn-log.sh |   14 ++++++++++++++\n 2 files changed, 15 insertions(+), 1 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex ec25ea4..3c5a87d 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -3591,7 +3591,7 @@ sub git_svn_log_cmd {\n \t\t$c_max = $gs->rev_db_get($r_max);\n \t\t$c_min = $gs->rev_db_get($r_min);\n \t\tif (defined $c_min && defined $c_max) {\n-\t\t\tif ($r_max > $r_max) {\n+\t\t\tif ($r_max > $r_min) {\n \t\t\t\tpush @cmd, \"$c_min..$c_max\";\n \t\t\t} else {\n \t\t\t\tpush @cmd, \"$c_max..$c_min\";\ndiff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh\nindex 0d4e6b3..618d7e9 100755\n--- a/t/t9116-git-svn-log.sh\n+++ b/t/t9116-git-svn-log.sh\n@@ -45,4 +45,18 @@ test_expect_success 'run log against a from trunk' \"\n \tgit svn log -r3 a | grep ^r3\n \t\"\n \n+printf 'r2 \\nr4 \\n' > expected-range-r2-r4\n+\n+test_expect_success 'test ascending revision range' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r2-r4 -\n+\t\"\n+\n+printf 'r4 \\nr2 \\n' > expected-range-r4-r2\n+\n+test_expect_success 'test descending revision range' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 4:1 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4-r2 -\n+\t\"\n+\n test_done\n-- \n1.5.3.4\n"},{"id":"59273","messageId":"1194761435-7286-3-git-send-email-ddkilzer@kilzer.net","threadId":"10795","inReplyTo":"1194761435-7286-2-git-send-email-ddkilzer@kilzer.net","subject":"[PATCH 2/3] git-svn log: include commit log for the smallest revision in a range","fromName":"David D Kilzer","fromEmail":"ddkilzer@kilzer.net","sentAt":"2007-11-11T06:10:34Z","receivedAt":"2007-11-11T06:10:34Z","isPatch":true,"sender":{"key":"ddkilzer@kilzer.net","avatar":"https://avatars.githubusercontent.com/u/263571?v=4"},"body":"The \"svn log -rM:N\" command shows commit logs inclusive in the range [M,N].\nPreviously \"git-svn log\" always excluded the commit log for the smallest\nrevision in a range, whether the range was ascending or descending.  With\nthis patch, the smallest revision in a range is always shown.\n\nUpdated tests for ascending and descending revision ranges.\n\nSigned-off-by: David D Kilzer <ddkilzer@kilzer.net>\n---\n git-svn.perl           |    6 +++---\n t/t9116-git-svn-log.sh |    8 ++++----\n 2 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 3c5a87d..39585d8 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -3592,9 +3592,9 @@ sub git_svn_log_cmd {\n \t\t$c_min = $gs->rev_db_get($r_min);\n \t\tif (defined $c_min && defined $c_max) {\n \t\t\tif ($r_max > $r_min) {\n-\t\t\t\tpush @cmd, \"$c_min..$c_max\";\n+\t\t\t\tpush @cmd, \"--boundary\", \"$c_min..$c_max\";\n \t\t\t} else {\n-\t\t\t\tpush @cmd, \"$c_max..$c_min\";\n+\t\t\t\tpush @cmd, \"--boundary\", \"$c_max..$c_min\";\n \t\t\t}\n \t\t} elsif ($r_max > $r_min) {\n \t\t\tpush @cmd, $c_max;\n@@ -3773,7 +3773,7 @@ sub cmd_show_log {\n \tmy (@k, $c, $d, $stat);\n \tmy $esc_color = qr/(?:\\033\\[(?:(?:\\d+;)*\\d*)?m)*/;\n \twhile (<$log>) {\n-\t\tif (/^${esc_color}commit ($::sha1_short)/o) {\n+\t\tif (/^${esc_color}commit -?($::sha1_short)/o) {\n \t\t\tmy $cmt = $1;\n \t\t\tif ($c && cmt_showable($c) && $c->{r} != $r_last) {\n \t\t\t\t$r_last = $c->{r};\ndiff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh\nindex 618d7e9..5000892 100755\n--- a/t/t9116-git-svn-log.sh\n+++ b/t/t9116-git-svn-log.sh\n@@ -45,18 +45,18 @@ test_expect_success 'run log against a from trunk' \"\n \tgit svn log -r3 a | grep ^r3\n \t\"\n \n-printf 'r2 \\nr4 \\n' > expected-range-r2-r4\n+printf 'r1 \\nr2 \\nr4 \\n' > expected-range-r1-r2-r4\n \n test_expect_success 'test ascending revision range' \"\n \tgit reset --hard trunk &&\n-\tgit svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r2-r4 -\n+\tgit svn log -r 1:4 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r1-r2-r4 -\n \t\"\n \n-printf 'r4 \\nr2 \\n' > expected-range-r4-r2\n+printf 'r4 \\nr2 \\nr1 \\n' > expected-range-r4-r2-r1\n \n test_expect_success 'test descending revision range' \"\n \tgit reset --hard trunk &&\n-\tgit svn log -r 4:1 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4-r2 -\n+\tgit svn log -r 4:1 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4-r2-r1 -\n \t\"\n \n test_done\n-- \n1.5.3.4\n"},{"id":"59274","messageId":"1194761435-7286-4-git-send-email-ddkilzer@kilzer.net","threadId":"10795","inReplyTo":"1194761435-7286-3-git-send-email-ddkilzer@kilzer.net","subject":"[PATCH 3/3] git-svn log: handle unreachable revisions like \"svn log\"","fromName":"David D Kilzer","fromEmail":"ddkilzer@kilzer.net","sentAt":"2007-11-11T06:10:35Z","receivedAt":"2007-11-11T06:10:35Z","isPatch":true,"sender":{"key":"ddkilzer@kilzer.net","avatar":"https://avatars.githubusercontent.com/u/263571?v=4"},"body":"When unreachable revisions are given to \"svn log\", it displays all commit\nlogs in the given range that exist in the current tree.  (If no commit\nlogs are found in the current tree, it simply prints a single commit log\nseparator.)  This patch makes \"git-svn log\" behave the same way.\n\nTen tests added to t/t9116-git-svn-log.sh.\n\nSigned-off-by: David D Kilzer <ddkilzer@kilzer.net>\n---\n git-svn.perl           |   58 ++++++++++++++++++++++++++++--------------\n t/t9116-git-svn-log.sh |   66 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 105 insertions(+), 19 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 39585d8..c584715 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -2257,9 +2257,10 @@ sub rev_db_get {\n }\n \n sub find_rev_before {\n-\tmy ($self, $rev, $eq_ok) = @_;\n+\tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n \t--$rev unless $eq_ok;\n-\twhile ($rev > 0) {\n+\t$min_rev ||= 1;\n+\twhile ($rev >= $min_rev) {\n \t\tif (my $c = $self->rev_db_get($rev)) {\n \t\t\treturn ($rev, $c);\n \t\t}\n@@ -2268,6 +2269,19 @@ sub find_rev_before {\n \treturn (undef, undef);\n }\n \n+sub find_rev_after {\n+\tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n+\t++$rev unless $eq_ok;\n+\t$max_rev ||= $self->rev_db_max();\n+\twhile ($rev <= $max_rev) {\n+\t\tif (my $c = $self->rev_db_get($rev)) {\n+\t\t\treturn ($rev, $c);\n+\t\t}\n+\t\t++$rev;\n+\t}\n+\treturn (undef, undef);\n+}\n+\n sub _new {\n \tmy ($class, $repo_id, $ref_id, $path) = @_;\n \tunless (defined $repo_id && length $repo_id) {\n@@ -3587,19 +3601,19 @@ sub git_svn_log_cmd {\n \t\t\tpush @cmd, $c;\n \t\t}\n \t} elsif (defined $r_max) {\n-\t\tmy ($c_min, $c_max);\n-\t\t$c_max = $gs->rev_db_get($r_max);\n-\t\t$c_min = $gs->rev_db_get($r_min);\n-\t\tif (defined $c_min && defined $c_max) {\n-\t\t\tif ($r_max > $r_min) {\n-\t\t\t\tpush @cmd, \"--boundary\", \"$c_min..$c_max\";\n-\t\t\t} else {\n-\t\t\t\tpush @cmd, \"--boundary\", \"$c_max..$c_min\";\n-\t\t\t}\n-\t\t} elsif ($r_max > $r_min) {\n-\t\t\tpush @cmd, $c_max;\n+\t\tif ($r_max < $r_min) {\n+\t\t\t($r_min, $r_max) = ($r_max, $r_min);\n+\t\t}\n+\t\tmy (undef, $c_max) = $gs->find_rev_before($r_max, 1, $r_min);\n+\t\tmy (undef, $c_min) = $gs->find_rev_after($r_min, 1, $r_max);\n+\t\t# If there are no commits in the range, both $c_max and $c_min\n+\t\t# will be undefined.  If there is at least 1 commit in the\n+\t\t# range, both will be defined.\n+\t\treturn () if !defined $c_min;\n+\t\tif ($c_min eq $c_max) {\n+\t\t\tpush @cmd, '--max-count=1', $c_min;\n \t\t} else {\n-\t\t\tpush @cmd, $c_min;\n+\t\t\tpush @cmd, '--boundary', \"$c_min..$c_max\";\n \t\t}\n \t}\n \treturn (@cmd, @files);\n@@ -3705,9 +3719,13 @@ sub show_commit_changed_paths {\n \tprint \"Changed paths:\\n\", @{$c->{changed}};\n }\n \n+sub commit_log_separator {\n+    return ('-' x 72) . \"\\n\";\n+}\n+\n sub show_commit_normal {\n \tmy ($c) = @_;\n-\tprint '-' x72, \"\\nr$c->{r} | \";\n+\tprint commit_log_separator(), \"r$c->{r} | \";\n \tprint \"$c->{c} | \" if $show_commit;\n \tprint \"$c->{a} | \", strftime(\"%Y-%m-%d %H:%M:%S %z (%a, %d %b %Y)\",\n \t\t\t\t localtime($c->{t_utc})), ' | ';\n@@ -3768,6 +3786,10 @@ sub cmd_show_log {\n \n \tconfig_pager();\n \t@args = git_svn_log_cmd($r_min, $r_max, @args);\n+\tif (!@args) {\n+\t\tprint commit_log_separator() unless $incremental || $oneline;\n+\t\treturn;\n+\t}\n \tmy $log = command_output_pipe(@args);\n \trun_pager();\n \tmy (@k, $c, $d, $stat);\n@@ -3816,14 +3838,12 @@ sub cmd_show_log {\n \t\tprocess_commit($c, $r_min, $r_max, \\@k);\n \t}\n \tif (@k) {\n-\t\tmy $swap = $r_max;\n-\t\t$r_max = $r_min;\n-\t\t$r_min = $swap;\n+\t\t($r_min, $r_max) = ($r_max, $r_min);\n \t\tprocess_commit($_, $r_min, $r_max) foreach reverse @k;\n \t}\n out:\n \tclose $log;\n-\tprint '-' x72,\"\\n\" unless $incremental || $oneline;\n+\tprint commit_log_separator() unless $incremental || $oneline;\n }\n \n package Git::SVN::Migration;\ndiff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh\nindex 5000892..56dd8fe 100755\n--- a/t/t9116-git-svn-log.sh\n+++ b/t/t9116-git-svn-log.sh\n@@ -30,6 +30,12 @@ test_expect_success 'setup repository and import' \"\n \tgit reset --hard trunk &&\n \techo aye >> README &&\n \tgit commit -a -m aye &&\n+\tgit svn dcommit &&\n+\tgit reset --hard b &&\n+\techo spy >> README &&\n+\tgit commit -a -m spy &&\n+\techo try >> README &&\n+\tgit commit -a -m try &&\n \tgit svn dcommit\n \t\"\n \n@@ -59,4 +65,64 @@ test_expect_success 'test descending revision range' \"\n \tgit svn log -r 4:1 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4-r2-r1 -\n \t\"\n \n+printf 'r1 \\nr2 \\n' > expected-range-r1-r2\n+\n+test_expect_success 'test ascending revision range with unreachable revision' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 1:3 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r1-r2 -\n+\t\"\n+\n+printf 'r2 \\nr1 \\n' > expected-range-r2-r1\n+\n+test_expect_success 'test descending revision range with unreachable revision' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 3:1 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r2-r1 -\n+\t\"\n+\n+printf 'r2 \\n' > expected-range-r2\n+\n+test_expect_success 'test ascending revision range with unreachable upper boundary revision and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 2:3 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r2 -\n+\t\"\n+\n+test_expect_success 'test descending revision range with unreachable upper boundary revision and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 3:2 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r2 -\n+\t\"\n+\n+printf 'r4 \\n' > expected-range-r4\n+\n+test_expect_success 'test ascending revision range with unreachable lower boundary revision and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 3:4 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4 -\n+\t\"\n+\n+test_expect_success 'test descending revision range with unreachable lower boundary revision and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 4:3 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4 -\n+\t\"\n+\n+echo ------------------------------------------------------------------------ > expected-separator\n+\n+test_expect_success 'test ascending revision range with unreachable boundary revisions and no commits' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 5:6 | diff -u expected-separator -\n+\t\"\n+\n+test_expect_success 'test descending revision range with unreachable boundary revisions and no commits' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 6:5 | diff -u expected-separator -\n+\t\"\n+\n+test_expect_success 'test ascending revision range with unreachable boundary revisions and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 3:5 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4 -\n+\t\"\n+\n+test_expect_success 'test descending revision range with unreachable boundary revisions and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 5:3 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4 -\n+\t\"\n+\n test_done\n-- \n1.5.3.4\n"},{"id":"59308","messageId":"D6A0D2B9-A355-4216-8D15-84993C26B503@lrde.epita.fr","threadId":"10795","inReplyTo":"1194761435-7286-4-git-send-email-ddkilzer@kilzer.net","subject":"Re: [PATCH 3/3] git-svn log: handle unreachable revisions like \"svn log\"","fromName":"Benoit Sigoure","fromEmail":"tsuna@lrde.epita.fr","sentAt":"2007-11-11T10:51:33Z","receivedAt":"2007-11-11T10:51:33Z","isPatch":true,"sender":{"key":"tsunanet@gmail.com","avatar":"https://avatars.githubusercontent.com/u/128281?v=4"},"body":"Hi David,\nthanks for the patches, the series looks good to me, I added some  \ncomments below, for this patch.\n\nOn Nov 11, 2007, at 7:10 AM, David D Kilzer wrote:\n\n> When unreachable revisions are given to \"svn log\", it displays all  \n> commit\n> logs in the given range that exist in the current tree.  (If no commit\n> logs are found in the current tree, it simply prints a single  \n> commit log\n> separator.)  This patch makes \"git-svn log\" behave the same way.\n>\n> Ten tests added to t/t9116-git-svn-log.sh.\n>\n> Signed-off-by: David D Kilzer <ddkilzer@kilzer.net>\n> ---\n>  git-svn.perl           |   58 +++++++++++++++++++++++++++ \n> +--------------\n>  t/t9116-git-svn-log.sh |   66 +++++++++++++++++++++++++++++++++++++ \n> +++++++++++\n>  2 files changed, 105 insertions(+), 19 deletions(-)\n>\n> diff --git a/git-svn.perl b/git-svn.perl\n> index 39585d8..c584715 100755\n> --- a/git-svn.perl\n> +++ b/git-svn.perl\n> @@ -2257,9 +2257,10 @@ sub rev_db_get {\n>  }\n>\n>  sub find_rev_before {\n> -\tmy ($self, $rev, $eq_ok) = @_;\n> +\tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n\nCould you please document this function?  I guess that you had to  \nfigure out what each argument was for, so please save the time of the  \ncontributors that will read this code after you :)\n\n>  \t--$rev unless $eq_ok;\n> -\twhile ($rev > 0) {\n> +\t$min_rev ||= 1;\n> +\twhile ($rev >= $min_rev) {\n>  \t\tif (my $c = $self->rev_db_get($rev)) {\n>  \t\t\treturn ($rev, $c);\n>  \t\t}\n> @@ -2268,6 +2269,19 @@ sub find_rev_before {\n>  \treturn (undef, undef);\n>  }\n>\n> +sub find_rev_after {\n> +\tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n> +\t++$rev unless $eq_ok;\n> +\t$max_rev ||= $self->rev_db_max();\n> +\twhile ($rev <= $max_rev) {\n> +\t\tif (my $c = $self->rev_db_get($rev)) {\n> +\t\t\treturn ($rev, $c);\n> +\t\t}\n> +\t\t++$rev;\n> +\t}\n> +\treturn (undef, undef);\n> +}\n> +\n\nToo much code duplication.  It should be possible to write a sub  \nfind_rev_ (or _find_rev, don't know what's the naming convention for  \ninternal details) that takes a 5th argument, an anonymous sub that  \ndoes the comparison.  So that basically, find_rev_before will be  \nsomething along these (untested) lines:\n\nsub find_rev_before {\n\tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n\treturn find_rev_($self, $rev, $eq_ok, $min_rev, sub { my ($a, $b) =  \n@_; return $a >= $b; });\n}\n\n>  sub _new {\n>  \tmy ($class, $repo_id, $ref_id, $path) = @_;\n>  \tunless (defined $repo_id && length $repo_id) {\n> @@ -3587,19 +3601,19 @@ sub git_svn_log_cmd {\n>  \t\t\tpush @cmd, $c;\n>  \t\t}\n>  \t} elsif (defined $r_max) {\n> -\t\tmy ($c_min, $c_max);\n> -\t\t$c_max = $gs->rev_db_get($r_max);\n> -\t\t$c_min = $gs->rev_db_get($r_min);\n> -\t\tif (defined $c_min && defined $c_max) {\n> -\t\t\tif ($r_max > $r_min) {\n> -\t\t\t\tpush @cmd, \"--boundary\", \"$c_min..$c_max\";\n> -\t\t\t} else {\n> -\t\t\t\tpush @cmd, \"--boundary\", \"$c_max..$c_min\";\n> -\t\t\t}\n> -\t\t} elsif ($r_max > $r_min) {\n> -\t\t\tpush @cmd, $c_max;\n> +\t\tif ($r_max < $r_min) {\n> +\t\t\t($r_min, $r_max) = ($r_max, $r_min);\n> +\t\t}\n> +\t\tmy (undef, $c_max) = $gs->find_rev_before($r_max, 1, $r_min);\n> +\t\tmy (undef, $c_min) = $gs->find_rev_after($r_min, 1, $r_max);\n> +\t\t# If there are no commits in the range, both $c_max and $c_min\n> +\t\t# will be undefined.  If there is at least 1 commit in the\n> +\t\t# range, both will be defined.\n> +\t\treturn () if !defined $c_min;\n\nFair enough but I'd strengthen the test by writing something like:\n\t\treturn () if not defined $c_min || not defined $c_max;\nunless you can prove that `rev_db_get' can never return `undef' or  \nsomething like that.\n\n> +\t\tif ($c_min eq $c_max) {\n> +\t\t\tpush @cmd, '--max-count=1', $c_min;\n>  \t\t} else {\n> -\t\t\tpush @cmd, $c_min;\n> +\t\t\tpush @cmd, '--boundary', \"$c_min..$c_max\";\n>  \t\t}\n>  \t}\n>  \treturn (@cmd, @files);\n> @@ -3705,9 +3719,13 @@ sub show_commit_changed_paths {\n>  \tprint \"Changed paths:\\n\", @{$c->{changed}};\n>  }\n>\n> +sub commit_log_separator {\n> +    return ('-' x 72) . \"\\n\";\n> +}\n> +\n\nThis is basically a constant, I think that declaring it with a  \nprototype helps Perl to optimize it:\nsub commit_log_separator() {\n\n>  sub show_commit_normal {\n>  \tmy ($c) = @_;\n> -\tprint '-' x72, \"\\nr$c->{r} | \";\n> +\tprint commit_log_separator(), \"r$c->{r} | \";\n>  \tprint \"$c->{c} | \" if $show_commit;\n>  \tprint \"$c->{a} | \", strftime(\"%Y-%m-%d %H:%M:%S %z (%a, %d %b %Y)\",\n>  \t\t\t\t localtime($c->{t_utc})), ' | ';\n> diff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh\n> index 5000892..56dd8fe 100755\n> --- a/t/t9116-git-svn-log.sh\n> +++ b/t/t9116-git-svn-log.sh\n> @@ -59,4 +65,64 @@ test_expect_success 'test descending revision  \n> range' \"\n[...]\n> +\n> +echo  \n> ---------------------------------------------------------------------- \n> -- > expected-separator\n\nThis will choke on shells with buggy/fragile `echo'.  I think it'd be  \nsafer to use printf here.\n\n\nCheers,\n\n-- \nBenoit Sigoure aka Tsuna\nEPITA Research and Development Laboratory\n\n\n"},{"id":"59330","messageId":"189577.85054.qm@web52407.mail.re2.yahoo.com","threadId":"10795","inReplyTo":"D6A0D2B9-A355-4216-8D15-84993C26B503@lrde.epita.fr","subject":"Re: [PATCH 3/3] git-svn log: handle unreachable revisions like \"svn log\"","fromName":"David D. Kilzer","fromEmail":"ddkilzer@kilzer.net","sentAt":"2007-11-11T14:20:18Z","receivedAt":"2007-11-11T14:20:18Z","isPatch":true,"sender":{"key":"ddkilzer@kilzer.net","avatar":"https://avatars.githubusercontent.com/u/263571?v=4"},"body":"Benoit Sigoure <tsuna@lrde.epita.fr> wrote:\n\n> thanks for the patches, the series looks good to me, I added some  \n> comments below, for this patch.\n\nThanks for the review, Benoit!  Comments below.\n\n> On Nov 11, 2007, at 7:10 AM, David D Kilzer wrote:\n> \n> >  sub find_rev_before {\n> > -\tmy ($self, $rev, $eq_ok) = @_;\n> > +\tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n> \n> Could you please document this function?  I guess that you had to  \n> figure out what each argument was for, so please save the time of the  \n> contributors that will read this code after you :)\n\nWhat is the format for documenting functions in git Perl scripts?  I haven't\nsee any \"perlpod\" use anywhere.  Do you just want comments before the function?\n\nThis method returns the git commit hash and svn revision of the first svn\nrevision that exists on the current branch that is less than $rev (or\nless-than-or-equal-to $rev if $eq_ok is true).\n\nPlease note that I don't have a full understanding of how find_rev_before()\nworks (other than it's computing an offset into a sparse? data file based on\nthe revision number) since I'm still new to git.\n\n> > +sub find_rev_after {\n> > +\tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n> > +\t++$rev unless $eq_ok;\n> > +\t$max_rev ||= $self->rev_db_max();\n> > +\twhile ($rev <= $max_rev) {\n> > +\t\tif (my $c = $self->rev_db_get($rev)) {\n> > +\t\t\treturn ($rev, $c);\n> > +\t\t}\n> > +\t\t++$rev;\n> > +\t}\n> > +\treturn (undef, undef);\n> > +}\n> \n> Too much code duplication.  It should be possible to write a sub  \n> find_rev_ (or _find_rev, don't know what's the naming convention for  \n> internal details) that takes a 5th argument, an anonymous sub that  \n> does the comparison.  So that basically, find_rev_before will be  \n> something along these (untested) lines:\n> \n> sub find_rev_before {\n> \tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n> \treturn find_rev_($self, $rev, $eq_ok, $min_rev, sub { my ($a, $b) =  \n> @_; return $a >= $b; });\n> }\n\nI think that combining find_rev_before() and find_rev_after() would greatly\nsacrifice readability of the code in exchange for removing ~10 lines of code. \nAlso, you must do more than just replace the comparison in the while() loop:\n\n- before() decrements $rev while after() increments it\n- stop limits are different ($max_rev versus $min_rev)\n\nThis is what such a method might look like (untested).  Since you already\nrequested find_rev_before() be documented, is this really going to help?\n\nsub find_rev_ {\n\tmy ($self, $rev, $eq_ok, $is_before, $limit_rev) = @_;\n\t($is_before ? --$rev : ++$rev) unless $eq_ok;\n\t$limit_rev ||= ($is_before ? 1 : $self->rev_db_max());\n\twhile ($is_before ? $rev >= $limit_rev : $rev <= $limit_rev) {\n\t\tif (my $c = $self->rev_db_get($rev)) {\n\t\t\treturn ($rev, $c);\n\t\t}\n\t\t\t$is_before ? --$rev : ++$rev;\n\t}\n\treturn (undef, undef);\n}\n\nDefining wrapper functions would help, but I still think it's just as clear to\nkeep the two methods separate.\n\nsub find_rev_before() {\n\tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n\treturn $self->find_rev_($rev, $eq_ok, 1, $min_rev);\n}\n\nsub find_rev_after() {\n\tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n\treturn $self->find_rev_($rev, $eq_ok, 0, $max_rev);\n}\n\nDo you agree, or do you think the methods should still be combined?\n\n> > +\t\tif ($r_max < $r_min) {\n> > +\t\t\t($r_min, $r_max) = ($r_max, $r_min);\n> > +\t\t}\n> > +\t\tmy (undef, $c_max) = $gs->find_rev_before($r_max, 1, $r_min);\n> > +\t\tmy (undef, $c_min) = $gs->find_rev_after($r_min, 1, $r_max);\n> > +\t\t# If there are no commits in the range, both $c_max and $c_min\n> > +\t\t# will be undefined.  If there is at least 1 commit in the\n> > +\t\t# range, both will be defined.\n> > +\t\treturn () if !defined $c_min;\n> \n> Fair enough but I'd strengthen the test by writing something like:\n> \t\treturn () if not defined $c_min || not defined $c_max;\n> unless you can prove that `rev_db_get' can never return `undef' or  \n> something like that.\n\nWill make this change.\n\n> > +sub commit_log_separator {\n> > +    return ('-' x 72) . \"\\n\";\n> > +}\n> > +\n> \n> This is basically a constant, I think that declaring it with a  \n> prototype helps Perl to optimize it:\n> sub commit_log_separator() {\n\nWill do.\n\n> > +echo  \n> > ---------------------------------------------------------------------- \n> > -- > expected-separator\n> \n> This will choke on shells with buggy/fragile `echo'.  I think it'd be  \n> safer to use printf here.\n\nWill do.\n\nThanks!\n\nDave\n"},{"id":"59337","messageId":"4A2CBEB1-F7B1-4DEC-9F72-506EBCDA2233@lrde.epita.fr","threadId":"10795","inReplyTo":"189577.85054.qm@web52407.mail.re2.yahoo.com","subject":"Re: [PATCH 3/3] git-svn log: handle unreachable revisions like \"svn log\"","fromName":"Benoit Sigoure","fromEmail":"tsuna@lrde.epita.fr","sentAt":"2007-11-11T16:36:50Z","receivedAt":"2007-11-11T16:36:50Z","isPatch":true,"sender":{"key":"tsunanet@gmail.com","avatar":"https://avatars.githubusercontent.com/u/128281?v=4"},"body":"On Nov 11, 2007, at 3:20 PM, David D. Kilzer wrote:\n\n> Benoit Sigoure <tsuna@lrde.epita.fr> wrote:\n>\n>> On Nov 11, 2007, at 7:10 AM, David D Kilzer wrote:\n>>\n>>>  sub find_rev_before {\n>>> -\tmy ($self, $rev, $eq_ok) = @_;\n>>> +\tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n>>\n>> Could you please document this function?  I guess that you had to\n>> figure out what each argument was for, so please save the time of the\n>> contributors that will read this code after you :)\n>\n> What is the format for documenting functions in git Perl scripts?   \n> I haven't\n> see any \"perlpod\" use anywhere.  Do you just want comments before  \n> the function?\n>\n> This method returns the git commit hash and svn revision of the  \n> first svn\n> revision that exists on the current branch that is less than $rev (or\n> less-than-or-equal-to $rev if $eq_ok is true).\n\nPersonally, I don't care.  Maybe Eric has his own preference.  For  \nme, as long as the code is documented one way or another, that's fine  \nby me.  Under-documented code hinders new contributors, so I think  \nit's important to add documentation whenever possible.\n\n> Please note that I don't have a full understanding of how  \n> find_rev_before()\n> works (other than it's computing an offset into a sparse? data file  \n> based on\n> the revision number) since I'm still new to git.\n>\n>>> +sub find_rev_after {\n>>> +\tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n>>> +\t++$rev unless $eq_ok;\n>>> +\t$max_rev ||= $self->rev_db_max();\n>>> +\twhile ($rev <= $max_rev) {\n>>> +\t\tif (my $c = $self->rev_db_get($rev)) {\n>>> +\t\t\treturn ($rev, $c);\n>>> +\t\t}\n>>> +\t\t++$rev;\n>>> +\t}\n>>> +\treturn (undef, undef);\n>>> +}\n>>\n>> Too much code duplication.  It should be possible to write a sub\n>> find_rev_ (or _find_rev, don't know what's the naming convention for\n>> internal details) that takes a 5th argument, an anonymous sub that\n>> does the comparison.  So that basically, find_rev_before will be\n>> something along these (untested) lines:\n>>\n>> sub find_rev_before {\n>> \tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n>> \treturn find_rev_($self, $rev, $eq_ok, $min_rev, sub { my ($a, $b) =\n>> @_; return $a >= $b; });\n>> }\n>\n> I think that combining find_rev_before() and find_rev_after() would  \n> greatly\n> sacrifice readability of the code in exchange for removing ~10  \n> lines of code.\n> Also, you must do more than just replace the comparison in the while \n> () loop:\n>\n> - before() decrements $rev while after() increments it\n> - stop limits are different ($max_rev versus $min_rev)\n>\n> This is what such a method might look like (untested).  Since you  \n> already\n> requested find_rev_before() be documented, is this really going to  \n> help?\n>\n> sub find_rev_ {\n> \tmy ($self, $rev, $eq_ok, $is_before, $limit_rev) = @_;\n> \t($is_before ? --$rev : ++$rev) unless $eq_ok;\n> \t$limit_rev ||= ($is_before ? 1 : $self->rev_db_max());\n> \twhile ($is_before ? $rev >= $limit_rev : $rev <= $limit_rev) {\n> \t\tif (my $c = $self->rev_db_get($rev)) {\n> \t\t\treturn ($rev, $c);\n> \t\t}\n> \t\t\t$is_before ? --$rev : ++$rev;\n> \t}\n> \treturn (undef, undef);\n> }\n>\n> Defining wrapper functions would help, but I still think it's just  \n> as clear to\n> keep the two methods separate.\n>\n> sub find_rev_before() {\n> \tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n> \treturn $self->find_rev_($rev, $eq_ok, 1, $min_rev);\n> }\n>\n> sub find_rev_after() {\n> \tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n> \treturn $self->find_rev_($rev, $eq_ok, 0, $max_rev);\n> }\n>\n> Do you agree, or do you think the methods should still be combined?\n\nEr... you're right, I overlooked the differences between the two  \nfunctions.  So merging them would make the code more complex than it  \nneeds to be, or so I think.\n\nThanks!\n\n-- \nBenoit Sigoure aka Tsuna\nEPITA Research and Development Laboratory\n\n\n"},{"id":"59418","messageId":"20071112025045.GA25705@mayonaise","threadId":"10795","inReplyTo":"189577.85054.qm@web52407.mail.re2.yahoo.com","subject":"Re: [PATCH 3/3] git-svn log: handle unreachable revisions like \"svn log\"","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2007-11-12T02:50:45Z","receivedAt":"2007-11-12T02:50:45Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"\"David D. Kilzer\" <ddkilzer@kilzer.net> wrote:\n\nHi Dave, thanks for the patches, and thanks to Benoit for the review.\n\n> Benoit Sigoure <tsuna@lrde.epita.fr> wrote:\n> \n> > thanks for the patches, the series looks good to me, I added some  \n> > comments below, for this patch.\n> \n> Thanks for the review, Benoit!  Comments below.\n> \n> > On Nov 11, 2007, at 7:10 AM, David D Kilzer wrote:\n> > \n> > >  sub find_rev_before {\n> > > -\tmy ($self, $rev, $eq_ok) = @_;\n> > > +\tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n> > \n> > Could you please document this function?  I guess that you had to  \n> > figure out what each argument was for, so please save the time of the  \n> > contributors that will read this code after you :)\n> \n> What is the format for documenting functions in git Perl scripts?  I\n> haven't see any \"perlpod\" use anywhere.  Do you just want comments\n> before the function?\n\nJust a regular comment is enough, perlpod uses too much space.\n\n> This method returns the git commit hash and svn revision of the first svn\n> revision that exists on the current branch that is less than $rev (or\n> less-than-or-equal-to $rev if $eq_ok is true).\n> \n> Please note that I don't have a full understanding of how find_rev_before()\n> works (other than it's computing an offset into a sparse? data file based on\n> the revision number) since I'm still new to git.\n\nPretty much.  The .rev_db format is documented above the _rev_db_set\nsub.  I'm considering replacing the current rev_db format with something\nmore compact for larger repos, though.\n\n> > > +sub find_rev_after {\n> > > +\tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n> > > +\t++$rev unless $eq_ok;\n> > > +\t$max_rev ||= $self->rev_db_max();\n> > > +\twhile ($rev <= $max_rev) {\n> > > +\t\tif (my $c = $self->rev_db_get($rev)) {\n> > > +\t\t\treturn ($rev, $c);\n> > > +\t\t}\n> > > +\t\t++$rev;\n> > > +\t}\n> > > +\treturn (undef, undef);\n> > > +}\n> > \n> > Too much code duplication.  It should be possible to write a sub  \n> > find_rev_ (or _find_rev, don't know what's the naming convention for  \n> > internal details) that takes a 5th argument, an anonymous sub that  \n> > does the comparison.  So that basically, find_rev_before will be  \n> > something along these (untested) lines:\n> > \n> > sub find_rev_before {\n> > \tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n> > \treturn find_rev_($self, $rev, $eq_ok, $min_rev, sub { my ($a, $b) =  \n> > @_; return $a >= $b; });\n> > }\n> \n> I think that combining find_rev_before() and find_rev_after() would greatly\n> sacrifice readability of the code in exchange for removing ~10 lines of code. \n> Also, you must do more than just replace the comparison in the while() loop:\n> \n> - before() decrements $rev while after() increments it\n> - stop limits are different ($max_rev versus $min_rev)\n> \n> This is what such a method might look like (untested).  Since you already\n> requested find_rev_before() be documented, is this really going to help?\n> \n> sub find_rev_ {\n> \tmy ($self, $rev, $eq_ok, $is_before, $limit_rev) = @_;\n> \t($is_before ? --$rev : ++$rev) unless $eq_ok;\n> \t$limit_rev ||= ($is_before ? 1 : $self->rev_db_max());\n> \twhile ($is_before ? $rev >= $limit_rev : $rev <= $limit_rev) {\n> \t\tif (my $c = $self->rev_db_get($rev)) {\n> \t\t\treturn ($rev, $c);\n> \t\t}\n> \t\t\t$is_before ? --$rev : ++$rev;\n> \t}\n> \treturn (undef, undef);\n> }\n\nfind_rev_ is too complicated, please keep them as separate functions.\n\n> Defining wrapper functions would help, but I still think it's just as clear to\n> keep the two methods separate.\n> \n> sub find_rev_before() {\n> \tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n> \treturn $self->find_rev_($rev, $eq_ok, 1, $min_rev);\n> }\n> \n> sub find_rev_after() {\n> \tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n> \treturn $self->find_rev_($rev, $eq_ok, 0, $max_rev);\n> }\n> \n> Do you agree, or do you think the methods should still be combined?\n> \n> > > +\t\tif ($r_max < $r_min) {\n> > > +\t\t\t($r_min, $r_max) = ($r_max, $r_min);\n> > > +\t\t}\n> > > +\t\tmy (undef, $c_max) = $gs->find_rev_before($r_max, 1, $r_min);\n> > > +\t\tmy (undef, $c_min) = $gs->find_rev_after($r_min, 1, $r_max);\n> > > +\t\t# If there are no commits in the range, both $c_max and $c_min\n> > > +\t\t# will be undefined.  If there is at least 1 commit in the\n> > > +\t\t# range, both will be defined.\n> > > +\t\treturn () if !defined $c_min;\n> > \n> > Fair enough but I'd strengthen the test by writing something like:\n> > \t\treturn () if not defined $c_min || not defined $c_max;\n> > unless you can prove that `rev_db_get' can never return `undef' or  \n> > something like that.\n\nI prefer '!' instead of 'not' unless operator precedence matters.\n\n> Will make this change.\n> \n> > > +sub commit_log_separator {\n> > > +    return ('-' x 72) . \"\\n\";\n> > > +}\n> > > +\n> > \n> > This is basically a constant, I think that declaring it with a  \n> > prototype helps Perl to optimize it:\n> > sub commit_log_separator() {\n\nuse constant commit_log_separator => ('-' x 72) . \"\\n\";\n\nis probably most readable...\n\n-- \nEric Wong\n"},{"id":"59429","messageId":"1194850612-6907-1-git-send-email-ddkilzer@kilzer.net","threadId":"10795","inReplyTo":"1194761435-7286-4-git-send-email-ddkilzer@kilzer.net","subject":"[PATCH 3/3 v2] git-svn log: handle unreachable revisions like \"svn log\"","fromName":"David D Kilzer","fromEmail":"ddkilzer@kilzer.net","sentAt":"2007-11-12T06:56:52Z","receivedAt":"2007-11-12T06:56:52Z","isPatch":true,"sender":{"key":"ddkilzer@kilzer.net","avatar":"https://avatars.githubusercontent.com/u/263571?v=4"},"body":"When unreachable revisions are given to \"svn log\", it displays all commit\nlogs in the given range that exist in the current tree.  (If no commit\nlogs are found in the current tree, it simply prints a single commit log\nseparator.)  This patch makes \"git-svn log\" behave the same way.\n\nTen tests added to t/t9116-git-svn-log.sh.\n\nSigned-off-by: David D Kilzer <ddkilzer@kilzer.net>\n---\n\nNo changes were needed to parts 1 and 2 of this series, so I am not\nreposting them.\n\nUpdated this patch based on feedback from Benoit Sigoure and Eric Wong:\n\n- Commented find_rev_before() and find_rev_after().\n- Changed commit_log_separator() into a constant.\n- Made return statement safer by adding another check in git_svn_log_cmd().\n- Changed echo statement to printf in t/t9116-git-svn-log.sh.\n\nAll tests pass on maint branch.\n\n git-svn.perl           |   63 ++++++++++++++++++++++++++++++++--------------\n t/t9116-git-svn-log.sh |   66 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 110 insertions(+), 19 deletions(-)\n\ndiff --git a/git-svn.perl b/git-svn.perl\nindex 39585d8..f017f94 100755\n--- a/git-svn.perl\n+++ b/git-svn.perl\n@@ -2256,10 +2256,15 @@ sub rev_db_get {\n \t$ret;\n }\n \n+# Finds the first svn revision that exists on (if $eq_ok is true) or\n+# before $rev for the current branch.  It will not search any lower\n+# than $min_rev.  Returns the git commit hash and svn revision number\n+# if found, else (undef, undef).\n sub find_rev_before {\n-\tmy ($self, $rev, $eq_ok) = @_;\n+\tmy ($self, $rev, $eq_ok, $min_rev) = @_;\n \t--$rev unless $eq_ok;\n-\twhile ($rev > 0) {\n+\t$min_rev ||= 1;\n+\twhile ($rev >= $min_rev) {\n \t\tif (my $c = $self->rev_db_get($rev)) {\n \t\t\treturn ($rev, $c);\n \t\t}\n@@ -2268,6 +2273,23 @@ sub find_rev_before {\n \treturn (undef, undef);\n }\n \n+# Finds the first svn revision that exists on (if $eq_ok is true) or\n+# after $rev for the current branch.  It will not search any higher\n+# than $max_rev.  Returns the git commit hash and svn revision number\n+# if found, else (undef, undef).\n+sub find_rev_after {\n+\tmy ($self, $rev, $eq_ok, $max_rev) = @_;\n+\t++$rev unless $eq_ok;\n+\t$max_rev ||= $self->rev_db_max();\n+\twhile ($rev <= $max_rev) {\n+\t\tif (my $c = $self->rev_db_get($rev)) {\n+\t\t\treturn ($rev, $c);\n+\t\t}\n+\t\t++$rev;\n+\t}\n+\treturn (undef, undef);\n+}\n+\n sub _new {\n \tmy ($class, $repo_id, $ref_id, $path) = @_;\n \tunless (defined $repo_id && length $repo_id) {\n@@ -3494,6 +3516,7 @@ package Git::SVN::Log;\n use strict;\n use warnings;\n use POSIX qw/strftime/;\n+use constant commit_log_separator => ('-' x 72) . \"\\n\";\n use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline\n             %rusers $show_commit $incremental/;\n my $l_fmt;\n@@ -3587,19 +3610,19 @@ sub git_svn_log_cmd {\n \t\t\tpush @cmd, $c;\n \t\t}\n \t} elsif (defined $r_max) {\n-\t\tmy ($c_min, $c_max);\n-\t\t$c_max = $gs->rev_db_get($r_max);\n-\t\t$c_min = $gs->rev_db_get($r_min);\n-\t\tif (defined $c_min && defined $c_max) {\n-\t\t\tif ($r_max > $r_min) {\n-\t\t\t\tpush @cmd, \"--boundary\", \"$c_min..$c_max\";\n-\t\t\t} else {\n-\t\t\t\tpush @cmd, \"--boundary\", \"$c_max..$c_min\";\n-\t\t\t}\n-\t\t} elsif ($r_max > $r_min) {\n-\t\t\tpush @cmd, $c_max;\n+\t\tif ($r_max < $r_min) {\n+\t\t\t($r_min, $r_max) = ($r_max, $r_min);\n+\t\t}\n+\t\tmy (undef, $c_max) = $gs->find_rev_before($r_max, 1, $r_min);\n+\t\tmy (undef, $c_min) = $gs->find_rev_after($r_min, 1, $r_max);\n+\t\t# If there are no commits in the range, both $c_max and $c_min\n+\t\t# will be undefined.  If there is at least 1 commit in the\n+\t\t# range, both will be defined.\n+\t\treturn () if !defined $c_min || !defined $c_max;\n+\t\tif ($c_min eq $c_max) {\n+\t\t\tpush @cmd, '--max-count=1', $c_min;\n \t\t} else {\n-\t\t\tpush @cmd, $c_min;\n+\t\t\tpush @cmd, '--boundary', \"$c_min..$c_max\";\n \t\t}\n \t}\n \treturn (@cmd, @files);\n@@ -3707,7 +3730,7 @@ sub show_commit_changed_paths {\n \n sub show_commit_normal {\n \tmy ($c) = @_;\n-\tprint '-' x72, \"\\nr$c->{r} | \";\n+\tprint commit_log_separator, \"r$c->{r} | \";\n \tprint \"$c->{c} | \" if $show_commit;\n \tprint \"$c->{a} | \", strftime(\"%Y-%m-%d %H:%M:%S %z (%a, %d %b %Y)\",\n \t\t\t\t localtime($c->{t_utc})), ' | ';\n@@ -3768,6 +3791,10 @@ sub cmd_show_log {\n \n \tconfig_pager();\n \t@args = git_svn_log_cmd($r_min, $r_max, @args);\n+\tif (!@args) {\n+\t\tprint commit_log_separator unless $incremental || $oneline;\n+\t\treturn;\n+\t}\n \tmy $log = command_output_pipe(@args);\n \trun_pager();\n \tmy (@k, $c, $d, $stat);\n@@ -3816,14 +3843,12 @@ sub cmd_show_log {\n \t\tprocess_commit($c, $r_min, $r_max, \\@k);\n \t}\n \tif (@k) {\n-\t\tmy $swap = $r_max;\n-\t\t$r_max = $r_min;\n-\t\t$r_min = $swap;\n+\t\t($r_min, $r_max) = ($r_max, $r_min);\n \t\tprocess_commit($_, $r_min, $r_max) foreach reverse @k;\n \t}\n out:\n \tclose $log;\n-\tprint '-' x72,\"\\n\" unless $incremental || $oneline;\n+\tprint commit_log_separator unless $incremental || $oneline;\n }\n \n package Git::SVN::Migration;\ndiff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh\nindex 5000892..902ed41 100755\n--- a/t/t9116-git-svn-log.sh\n+++ b/t/t9116-git-svn-log.sh\n@@ -30,6 +30,12 @@ test_expect_success 'setup repository and import' \"\n \tgit reset --hard trunk &&\n \techo aye >> README &&\n \tgit commit -a -m aye &&\n+\tgit svn dcommit &&\n+\tgit reset --hard b &&\n+\techo spy >> README &&\n+\tgit commit -a -m spy &&\n+\techo try >> README &&\n+\tgit commit -a -m try &&\n \tgit svn dcommit\n \t\"\n \n@@ -59,4 +65,64 @@ test_expect_success 'test descending revision range' \"\n \tgit svn log -r 4:1 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4-r2-r1 -\n \t\"\n \n+printf 'r1 \\nr2 \\n' > expected-range-r1-r2\n+\n+test_expect_success 'test ascending revision range with unreachable revision' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 1:3 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r1-r2 -\n+\t\"\n+\n+printf 'r2 \\nr1 \\n' > expected-range-r2-r1\n+\n+test_expect_success 'test descending revision range with unreachable revision' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 3:1 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r2-r1 -\n+\t\"\n+\n+printf 'r2 \\n' > expected-range-r2\n+\n+test_expect_success 'test ascending revision range with unreachable upper boundary revision and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 2:3 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r2 -\n+\t\"\n+\n+test_expect_success 'test descending revision range with unreachable upper boundary revision and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 3:2 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r2 -\n+\t\"\n+\n+printf 'r4 \\n' > expected-range-r4\n+\n+test_expect_success 'test ascending revision range with unreachable lower boundary revision and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 3:4 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4 -\n+\t\"\n+\n+test_expect_success 'test descending revision range with unreachable lower boundary revision and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 4:3 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4 -\n+\t\"\n+\n+printf -- '------------------------------------------------------------------------\\n' > expected-separator\n+\n+test_expect_success 'test ascending revision range with unreachable boundary revisions and no commits' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 5:6 | diff -u expected-separator -\n+\t\"\n+\n+test_expect_success 'test descending revision range with unreachable boundary revisions and no commits' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 6:5 | diff -u expected-separator -\n+\t\"\n+\n+test_expect_success 'test ascending revision range with unreachable boundary revisions and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 3:5 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4 -\n+\t\"\n+\n+test_expect_success 'test descending revision range with unreachable boundary revisions and 1 commit' \"\n+\tgit reset --hard trunk &&\n+\tgit svn log -r 5:3 | grep '^r[0-9]' | cut -d'|' -f1 | diff -u expected-range-r4 -\n+\t\"\n+\n test_done\n-- \n1.5.3.4\n"},{"id":"60177","messageId":"20071117214738.GB22835@soma","threadId":"10795","inReplyTo":"1194850612-6907-1-git-send-email-ddkilzer@kilzer.net","subject":"Re: [PATCH 3/3 v2] git-svn log: handle unreachable revisions like \"svn log\"","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2007-11-17T21:47:38Z","receivedAt":"2007-11-17T21:47:38Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"David D Kilzer <ddkilzer@kilzer.net> wrote:\n> When unreachable revisions are given to \"svn log\", it displays all commit\n> logs in the given range that exist in the current tree.  (If no commit\n> logs are found in the current tree, it simply prints a single commit log\n> separator.)  This patch makes \"git-svn log\" behave the same way.\n> \n> Ten tests added to t/t9116-git-svn-log.sh.\n> \n> Signed-off-by: David D Kilzer <ddkilzer@kilzer.net>\n> ---\n> \n> No changes were needed to parts 1 and 2 of this series, so I am not\n> reposting them.\n> \n> Updated this patch based on feedback from Benoit Sigoure and Eric Wong:\n> \n> - Commented find_rev_before() and find_rev_after().\n> - Changed commit_log_separator() into a constant.\n> - Made return statement safer by adding another check in git_svn_log_cmd().\n> - Changed echo statement to printf in t/t9116-git-svn-log.sh.\n> \n> All tests pass on maint branch.\n\nThanks,\n\nAcked and pushed out to http://git.bogomips.org/git-svn.git\n\nDavid D Kilzer (3):\n      git-svn log: fix ascending revision ranges\n      git-svn log: include commit log for the smallest revision in a range\n      git-svn log: handle unreachable revisions like \"svn log\"\n\nDavid Reiss (1):\n      git-svn: Fix a typo and add a comma in an error message in git-svn\n\nEric Wong (1):\n      git-svn: add tests for command-line usage of init and clone commands\n\nKonstantin V. Arkhipov (1):\n      git-svn's dcommit must use subversion's config\n\n-- \nEric Wong\n"},{"id":"60189","messageId":"7vy7cwz8wy.fsf@gitster.siamese.dyndns.org","threadId":"10795","inReplyTo":"20071117214738.GB22835@soma","subject":"Re: [PATCH 3/3 v2] git-svn log: handle unreachable revisions like \"svn log\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-11-18T00:36:45Z","receivedAt":"2007-11-18T00:36:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n> Acked and pushed out to http://git.bogomips.org/git-svn.git\n\nOk, will pull to 'master' but from git://git.bogomips.org/git-svn.git/.\n\nThanks.\n\n> David D Kilzer (3):\n>       git-svn log: fix ascending revision ranges\n>       git-svn log: include commit log for the smallest revision in a range\n>       git-svn log: handle unreachable revisions like \"svn log\"\n>\n> David Reiss (1):\n>       git-svn: Fix a typo and add a comma in an error message in git-svn\n>\n> Eric Wong (1):\n>       git-svn: add tests for command-line usage of init and clone commands\n>\n> Konstantin V. Arkhipov (1):\n>       git-svn's dcommit must use subversion's config\n"}]}