diff mbox series

[(Apple,Git),11/13] Fix problem found from running the test suite.

Message ID 20190129193818.8645-12-jeremyhu@apple.com (mailing list archive)
State New, archived
Headers show
Series Differences between git-2.20.1 and Apple Git-116 | expand

Commit Message

Jeremy Sequoia Jan. 29, 2019, 7:38 p.m. UTC 
From: Matt Wright <mww@apple.com>

Signed-off-by: Matt Wright <mww@apple.com>
---
 git-svn.perl | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

Comments

Junio C Hamano Jan. 29, 2019, 11:12 p.m. UTC | #1 
[jc: just forwarding to the area expert]

Jeremy Huddleston Sequoia <jeremyhu@apple.com> writes:

> From: Matt Wright <mww@apple.com>
>
> Signed-off-by: Matt Wright <mww@apple.com>
> ---
>  git-svn.perl | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> 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;
> +
> +		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;
> +				}
> +			}
> +		}
> +
> +		if (defined($resolved)) {
> +			return verify_ref("$1^0") if $resolved =~ /^ref: (.*)$/;
> +			return $resolved
> +		}
> +	}
> +
>  	eval { command_oneline([ 'rev-parse', '--verify', $ref ],
>  	                       { STDERR => 0 }); };
>  }
Eric Wong Jan. 29, 2019, 11:30 p.m. UTC | #2 
Jeremy Huddleston Sequoia <jeremyhu@apple.com> wrote:
> From: Matt Wright <mww@apple.com>
> 
> Signed-off-by: Matt Wright <mww@apple.com>

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 }); };
>  }
diff mbox series

Patch

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;
+
+		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;
+				}
+			}
+		}
+
+		if (defined($resolved)) {
+			return verify_ref("$1^0") if $resolved =~ /^ref: (.*)$/;
+			return $resolved
+		}
+	}
+
 	eval { command_oneline([ 'rev-parse', '--verify', $ref ],
 	                       { STDERR => 0 }); };
 }