Jeremy Huddleston Sequoia <jeremyhu@xxxxxxxxx> wrote:
> From: Matt Wright <mww@xxxxxxxxx>
>
> Signed-off-by: Matt Wright <mww@xxxxxxxxx>
Hi Jeremy/Matt: I expect to see a description of said "problem"
More comments inline below...
> diff --git a/git-svn.perl b/git-svn.perl
> index 050f2a36f4..d29730be3b 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -1815,6 +1815,36 @@ sub complete_url_ls_init {
>
> sub verify_ref {
> my ($ref) = @_;
> +
> + if ($ref =~ /^(.*)\^0$/) {
> + my $baseref = $1;
> + my $p = "$ENV{GIT_DIR}/$baseref";
> + $p = "$ENV{GIT_DIR}/refs/remotes/$baseref" unless -e $p;
> + $p = "$ENV{GIT_DIR}/refs/$baseref" unless -e $p;
> + $p = "$ENV{GIT_DIR}/refs/heads/$baseref" unless -e $p;
OK, this looks like we're reproducing rev-parse functionality...
> + my $resolved = undef;
> + if (-e $p) {
> + open FH, $p;
> + $resolved = <FH>;
> + chomp $resolved;
> + close FH;
> + } elsif (-e "$ENV{GIT_DIR}/packed-refs") {
> + open FH, "$ENV{GIT_DIR}/packed-refs";
> + while (<FH>) {
> + if ($_ =~ /^([0-9a-fA-F]+) ((refs\/)?(remotes\/|heads\/|\/)?$baseref)$/) {
> + $resolved = $1;
> + last;
> + }
> + }
And even more so... This would be a pain to maintain with
proposed changes to ref storage (reftable/lmdb/...), so I really
don't want to reproduce rev-parse functionality in Perl.
But while we're in Perl; prefer something like:
m!^([0-9a-f]+) ((refs/)?(remotes/|heads/|/)?$baseref)$!
So you don't have to escape '/' (leaning-toothpick syndrome) by
using m!$REGEX!. You can also skip the unnecessary [A-F] match.
> + }
> +
> + if (defined($resolved)) {
> + return verify_ref("$1^0") if $resolved =~ /^ref: (.*)$/;
> + return $resolved
> + }
> + }
So without more details, we really need an explanation of why
this patch was made. The test suite has been thousands of times
over the years on other platforms without changes to the
verify_ref() sub.
> eval { command_oneline([ 'rev-parse', '--verify', $ref ],
> { STDERR => 0 }); };
> }
